Bug 220372 - [WebGL2] fbostatequery, negativebufferapi, negativevertexarrayapi, shaderstatequery conformance failures
Summary: [WebGL2] fbostatequery, negativebufferapi, negativevertexarrayapi, shaderstat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
Depends on: 205610
Blocks: 126404
  Show dependency treegraph
 
Reported: 2021-01-06 11:03 PST by Kenneth Russell
Modified: 2021-01-18 20:01 PST (History)
13 users (show)

See Also:


Attachments
Patch (70.40 KB, patch)
2021-01-11 17:56 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (95.91 KB, patch)
2021-01-12 18:01 PST, Kenneth Russell
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 2021-01-06 11:03:02 PST
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.
Comment 1 Kenneth Russell 2021-01-07 18:35:23 PST
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
Comment 2 Kenneth Russell 2021-01-11 17:56:00 PST
Created attachment 417423 [details]
Patch
Comment 3 Kimmo Kinnunen 2021-01-12 01:47:16 PST
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
Comment 4 Dean Jackson 2021-01-12 10:49:15 PST
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?
Comment 5 Kenneth Russell 2021-01-12 11:04:29 PST
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".
Comment 6 Kenneth Russell 2021-01-12 18:01:12 PST
Created attachment 417503 [details]
Patch
Comment 7 EWS Watchlist 2021-01-12 18:01:59 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 8 Darin Adler 2021-01-13 10:50:27 PST
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);
Comment 9 EWS 2021-01-13 10:56:39 PST
Committed r271444: <https://trac.webkit.org/changeset/271444>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417503 [details].
Comment 10 Radar WebKit Bug Importer 2021-01-13 10:57:13 PST
<rdar://problem/73158102>
Comment 11 Kenneth Russell 2021-01-13 12:37:35 PST
(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.
Comment 12 Kenneth Russell 2021-01-13 12:37:46 PST
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.