Bug 209518 – [WebGL2] Uniform Buffer Objects

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The following uniform buffer object methods must be implemented for WebGL 2.0 support:
bindBufferBase
bindBufferRange
getUniformIndices
getActiveUniforms
getUniformBlockIndex
getActiveUniformBlockParameter
getActiveUniformBlockName
uniformBlockBinding

[James Darpinian]

I have all the tests passing now. Lots of misc fixes needed in addition to the uniform buffer functions, including a JSC fix.

[James Darpinian]

Created attachment 401810 [details]
Patch

[James Darpinian]

Created attachment 401813 [details]
Patch

[Dean Jackson]

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.");

[Kenneth Russell]

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.

[James Darpinian]

Created attachment 402044 [details]
Rebaseline fixed tests and address comments

[James Darpinian]

Created attachment 402169 [details]
fix gl-get-calls.html

[James Darpinian]

Created attachment 402172 [details]
fix gtk build error

[James Darpinian]

Created attachment 402186 [details]
include test rebaselines

[James Darpinian]

Created attachment 402257 [details]
fix readBuffer test

[James Darpinian]

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.

[James Darpinian]

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).

[Dean Jackson]

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.

[EWS]

Committed r263281: <https://trac.webkit.org/changeset/263281>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402257 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64541904>

[Radar WebKit Bug Importer]

<rdar://problem/64541903>

[Kenneth Russell]

Fantastic work James getting this over the finish line!