Bug 209518 - [WebGL2] Uniform Buffer Objects
Summary: [WebGL2] Uniform Buffer Objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Darpinian
URL:
Keywords: InRadar
Depends on: 184232
Blocks: 126404 209513
  Show dependency treegraph
 
Reported: 2020-03-24 15:50 PDT by Kenneth Russell
Modified: 2020-07-24 14:42 PDT (History)
14 users (show)

See Also:


Attachments
Patch (57.91 KB, patch)
2020-06-12 17:25 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (56.73 KB, patch)
2020-06-12 17:51 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Rebaseline fixed tests and address comments (164.81 KB, patch)
2020-06-16 15:03 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
fix gl-get-calls.html (173.58 KB, patch)
2020-06-17 17:27 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
fix gtk build error (60.54 KB, patch)
2020-06-17 17:50 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
include test rebaselines (172.51 KB, patch)
2020-06-17 22:29 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
fix readBuffer test (177.84 KB, patch)
2020-06-18 17:46 PDT, James Darpinian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2020-03-24 15:50:29 PDT
The following uniform buffer object methods must be implemented for WebGL 2.0 support:
bindBufferBase
bindBufferRange
getUniformIndices
getActiveUniforms
getUniformBlockIndex
getActiveUniformBlockParameter
getActiveUniformBlockName
uniformBlockBinding
Comment 1 James Darpinian 2020-06-12 17:19:05 PDT
I have all the tests passing now. Lots of misc fixes needed in addition to the uniform buffer functions, including a JSC fix.
Comment 2 James Darpinian 2020-06-12 17:25:17 PDT
Created attachment 401810 [details]
Patch
Comment 3 James Darpinian 2020-06-12 17:51:47 PDT
Created attachment 401813 [details]
Patch
Comment 4 Dean Jackson 2020-06-12 18:52:58 PDT
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 5 Kenneth Russell 2020-06-16 13:02:39 PDT
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.
Comment 6 James Darpinian 2020-06-16 15:03:45 PDT
Created attachment 402044 [details]
Rebaseline fixed tests and address comments
Comment 7 James Darpinian 2020-06-17 17:27:04 PDT
Created attachment 402169 [details]
fix gl-get-calls.html
Comment 8 James Darpinian 2020-06-17 17:50:49 PDT
Created attachment 402172 [details]
fix gtk build error
Comment 9 James Darpinian 2020-06-17 22:29:11 PDT
Created attachment 402186 [details]
include test rebaselines
Comment 10 James Darpinian 2020-06-18 17:46:49 PDT
Created attachment 402257 [details]
fix readBuffer test
Comment 11 James Darpinian 2020-06-18 17:48:50 PDT
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 12 James Darpinian 2020-06-18 17:50:01 PDT
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 13 Dean Jackson 2020-06-19 12:50:32 PDT
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.
Comment 14 EWS 2020-06-19 12:59:33 PDT
Committed r263281: <https://trac.webkit.org/changeset/263281>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402257 [details].
Comment 15 Radar WebKit Bug Importer 2020-06-19 13:00:19 PDT
<rdar://problem/64541904>
Comment 16 Radar WebKit Bug Importer 2020-06-19 13:00:21 PDT
<rdar://problem/64541903>
Comment 17 Kenneth Russell 2020-06-19 13:02:42 PDT
Fantastic work James getting this over the finish line!