Summary: | [WebGL2] Uniform Buffer Objects | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||||||||
Component: | WebGL | Assignee: | James Darpinian <jdarpinian> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, changseok, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, japhet, jdarpinian, justin_fan, kbr, kondapallykalyan, noam, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 184232 | ||||||||||||||||||
Bug Blocks: | 126404, 209513 | ||||||||||||||||||
Attachments: |
|
Description
Kenneth Russell
2020-03-24 15:50:29 PDT
I have all the tests passing now. Lots of misc fixes needed in addition to the uniform buffer functions, including a JSC fix. Created attachment 401810 [details]
Patch
Created attachment 401813 [details]
Patch
Comment on attachment 401813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401813&action=review > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1917 > + { Our coding style puts the open brace on the same line as the case, and the closing brace on a single line at the same indentation as the case. That way the scoped code doesn't look doubly indented. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1995 > + GLint r = 0; Could we name this "result"? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2003 > + case GraphicsContextGL::UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES: > + { Same note here about indentation. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6274 > + synthesizeGLError(GraphicsContextGL::INVALID_VALUE, functionName, "location length > max"); Suggest: location length is too large > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1941 > + // Use getActiveUniformBlockivRobustANGLE instead > + notImplemented(); Could you add: RELEASE_LOG(WebGL, "Use getActiveUniformBlockivRobustANGLE instead."); Comment on attachment 401813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401813&action=review Great work James figuring out all of the needed changes to make these tests pass. Dean pointed out "run-webkit-tests --reset-results", which can be used to locally regenerate results for now-passing layout-tests - maybe could be useful. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6838 > + return buffer; Suggestion: consider making this virtual and adding the WebGL 2.0-specific validation in WebGL2RenderingContext. Created attachment 402044 [details]
Rebaseline fixed tests and address comments
Created attachment 402169 [details]
fix gl-get-calls.html
Created attachment 402172 [details]
fix gtk build error
Created attachment 402186 [details]
include test rebaselines
Created attachment 402257 [details]
fix readBuffer test
Comment on attachment 401813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401813&action=review >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1917 >> + { > > Our coding style puts the open brace on the same line as the case, and the closing brace on a single line at the same indentation as the case. That way the scoped code doesn't look doubly indented. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1995 >> + GLint r = 0; > > Could we name this "result"? Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2003 >> + { > > Same note here about indentation. Done. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6274 >> + synthesizeGLError(GraphicsContextGL::INVALID_VALUE, functionName, "location length > max"); > > Suggest: location length is too large Done >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6838 >> + return buffer; > > Suggestion: consider making this virtual and adding the WebGL 2.0-specific validation in WebGL2RenderingContext. Done >> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1941 >> + notImplemented(); > > Could you add: > > RELEASE_LOG(WebGL, "Use getActiveUniformBlockivRobustANGLE instead."); Done. Comment on attachment 402257 [details]
fix readBuffer test
I think this one should finally pass EWS (except for style because of the underscores in GetIntegeri_v).
Comment on attachment 402257 [details] fix readBuffer test View in context: https://bugs.webkit.org/attachment.cgi?id=402257&action=review > Source/WebCore/html/canvas/WebGL2RenderingContext.h:288 > + bool setIndexedBufferBinding(const char *functionName, GCGLenum target, GCGLuint index, WebGLBuffer*); Nit: should be "char*" not "char *", but I don't think it's worth another patch here. We'll fix it up later. > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1940 > + RELEASE_LOG(WebGL, "Use getActiveUniformBlockivRobustANGLE instead."); In case someone gets here, we should probably tell them where they are. e.g. "GraphicsContextGLOpenGL::getActiveUniformBlockiv should be replaced with getActiveUniformBlockivRobustANGLE". Again, follow-up patch is fine. Committed r263281: <https://trac.webkit.org/changeset/263281> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402257 [details]. Fantastic work James getting this over the finish line! |