Bug 220372 – [WebGL2] fbostatequery, negativebufferapi, negativevertexarrayapi, shaderstatequery conformance failures

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The following WebGL 2.0 conformance tests are failing multiple sub-tests:

deqp/functional/gles3/fbostatequery.html
deqp/functional/gles3/negativebufferapi.html
deqp/functional/gles3/negativevertexarrayapi.html
deqp/functional/gles3/shaderstatequery.html

The reasons vary, but there are some incorrect early-outs that would otherwise generate errors in ANGLE's validation, and a couple of places where additional validation is needed in order to generate the correct errors.

[Kenneth Russell]

Note to self: the ?filter= query arg is most useful for running individual test cases, e.g.:

http://localhost:8080/webgl/2.0.0/resources/webgl_test_files/deqp/functional/gles3/shaderstatequery.html?filter=shader.program_active_uniform_blocks

[Kenneth Russell]

Created attachment 417423 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 417423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417423&action=review

Overall. looks great!

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:505
> +    // compared to others. Performs a context loss check internally.

mandatory typo marker

[Dean Jackson]

Comment on attachment 417423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417423&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1745
> +    // Determine the number of elements the bound buffer can hold, given the offset, size, type and stride

Nit: End with period.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2055
> +    if (isContextLostOrPending() || !sampler || !sampler->validate(contextGroup(), *this) || sampler->isDeleted())

Can a sampler be valid if it is deleted? If not, can we delete the last test here?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:-2085
> -    if (isContextLostOrPending() || !sync || sync->isDeleted() || !validateWebGLObject("deleteSync", sync))
> -        return;

Is it ok to delete this?

[Kenneth Russell]

Comment on attachment 417423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417423&action=review

Thanks for your reviews Dean and Kimmo. Still working through the test failures - a couple of the tests have contradictory validation requirements and I'm trying to figure out why they pass in Chromium and fail in WebKit.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1745
>> +    // Determine the number of elements the bound buffer can hold, given the offset, size, type and stride
> 
> Nit: End with period.

Done.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2055
>> +    if (isContextLostOrPending() || !sampler || !sampler->validate(contextGroup(), *this) || sampler->isDeleted())
> 
> Can a sampler be valid if it is deleted? If not, can we delete the last test here?

The way the validation code's factored, all objects need the deletion check. The implementations of WebGLObject::validate() basically only check whether the object is owned by this context. Further validation is delegated to the WebGLRenderingContextBase. All this could be revisited, of course.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:-2085
>> -        return;
> 
> Is it ok to delete this?

Yes. It's completely redundant to the checks already done in deleteObject.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:505
>> +    // compared to others. Performs a context loss check internally.
> 
> mandatory typo marker

loss -> lost? It's actually that way in comments in Chromium, but I'll make this change here.

Oh, I see, also "objetcts".

[Kenneth Russell]

Created attachment 417503 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Darin Adler]

Comment on attachment 417503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417503&action=review

It’s great that this has so many fixes; they look good. Are we confident that each of these was covered by a test? With somewhere in the neighborhood of 30 failures fixed here and somewhere near 30 different logic changes, it’s hard for me to spot which improvements are tested and which are untested.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1755
> +    GCGLsizei bytesPerElement = size * typeSize;

What prevents this from overflowing?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2787
> +        // We can look directly at m_attributes because in WebGL 2,
> +        // they are required to be honored.

No need to break this into two lines. Reads better in one line.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2790
> +        bool missingImage = (attachment == GraphicsContextGL::DEPTH && !hasDepth) || (attachment == GraphicsContextGL::STENCIL && !hasStencil);

Not a fan of naming a boolean "missing image" since it’s not an image. Prefer a name like hasMissingImage.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1380
> +        // This differs in behavior to ValidateWebGLObject; null objects are allowed
> +        // in these entry points.

Given the function’s name, it does not seem like we need this comment. What else would we assume "nullable" means other than "null is allowed"? I also think the logic is easier to understand when more compact and less vertical.

    return !object || validateWebGLObject(functionName, object);

[EWS]

Committed r271444: <https://trac.webkit.org/changeset/271444>

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

[Radar WebKit Bug Importer]

<rdar://problem/73158102>

[Kenneth Russell]

(In reply to Darin Adler from comment #8)
> Comment on attachment 417503 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417503&action=review
> 
> It’s great that this has so many fixes; they look good. Are we confident
> that each of these was covered by a test? With somewhere in the neighborhood
> of 30 failures fixed here and somewhere near 30 different logic changes,
> it’s hard for me to spot which improvements are tested and which are
> untested.

Thanks for your review. Yes, all of these changes are covered by one test or another; they were all informed by trying to pass individual cases of the tests that were improved with this fix.

Apologies for not splitting up this patch, but it was much faster to try to get all of these done at the same time. There were actually some interdependencies among some of the now-passing tests.

Will reply to other review comments inline too.

[Kenneth Russell]

Comment on attachment 417503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417503&action=review

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1755
>> +    GCGLsizei bytesPerElement = size * typeSize;
> 
> What prevents this from overflowing?

These are guaranteed to be very small numbers per the validation checks above.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2787
>> +        // they are required to be honored.
> 
> No need to break this into two lines. Reads better in one line.

Fair enough. Some of this code originated in another project which has an 80 column line limit.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2790
>> +        bool missingImage = (attachment == GraphicsContextGL::DEPTH && !hasDepth) || (attachment == GraphicsContextGL::STENCIL && !hasStencil);
> 
> Not a fan of naming a boolean "missing image" since it’s not an image. Prefer a name like hasMissingImage.

Got it, will keep that in mind in the future.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1380
>> +        // in these entry points.
> 
> Given the function’s name, it does not seem like we need this comment. What else would we assume "nullable" means other than "null is allowed"? I also think the logic is easier to understand when more compact and less vertical.
> 
>     return !object || validateWebGLObject(functionName, object);

Fair enough. Again this code originated in a different project where I added this comment originally. I wanted to be very explicit since there were some subtle bugs in these areas.