Bug 200499 – Short-cut WebGLRenderingContext::getParameter() for ALPHA_BITS when alpha channel is disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Chris Lord]

After bug #200434, retrieving ALPHA_BITS can return an incorrect value. Add a shortcut, similar to the retrieval of DEPTH and STENCIL bits.

[Chris Lord]

Created attachment 375692 [details]
Patch

[Chris Lord]

Created attachment 375693 [details]
Patch

[Chris Lord]

This fixes the ALPHA_BITS failure in context-attributes-alpha-depth-stencil-antialias.html on wpe and ios, as expected.

[Darin Adler]

Comment on attachment 375693 [details]
Patch

Failing test is:

webgl/2.0.0/conformance/context/context-attributes-alpha-depth-stencil-antialias.html

This makes me think the failure is indeed related to this patch.

[Darin Adler]

Just incorrectly rebased perhaps?

[Chris Lord]

Created attachment 375759 [details]
Patch

[Chris Lord]

(In reply to Darin Adler from comment #4)
> Comment on attachment 375693 [details]
> Patch
> 
> Failing test is:
> 
> webgl/2.0.0/conformance/context/context-attributes-alpha-depth-stencil-
> antialias.html
> 
> This makes me think the failure is indeed related to this patch.

The test is actually passing now and was failing before - I've updated the patch to mark it as passing.

[Chris Lord]

Created attachment 375787 [details]
Patch

[Darin Adler]

Comment on attachment 375787 [details]
Patch

Would be better to have test coverage for both changes.

[WebKit Commit Bot]

Comment on attachment 375787 [details]
Patch

Clearing flags on attachment: 375787

Committed r248448: <https://trac.webkit.org/changeset/248448>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Lord]

(In reply to Darin Adler from comment #9)
> Comment on attachment 375787 [details]
> Patch
> 
> Would be better to have test coverage for both changes.

Thanks for the review!

I'm a little stumped as to what further tests could be added to test this - it was test failures that let me identify it initially and it seems pretty well-tested in context-attributes-alpha-depth-stencil-
> antialias.html in both WebGL conformance test suites (and it's replicated in the fast webgl test suite too).

Do you have any suggestions?

[Darin Adler]

Not sure my understanding is correct, but I believe that the test covers either WebGL2RenderingContext. I am suggesting adding a second test that covers WebGLRenderingContext. Otherwise we have one tested change and one untested change.

[Chris Lord]

(In reply to Darin Adler from comment #13)
> Not sure my understanding is correct, but I believe that the test covers
> either WebGL2RenderingContext. I am suggesting adding a second test that
> covers WebGLRenderingContext. Otherwise we have one tested change and one
> untested change.

The same test exists for WebGLRenderingContext (https://github.com/WebKit/webkit/blob/master/LayoutTests/webgl/1.0.2/conformance/context/context-attributes-alpha-depth-stencil-antialias.html), both tests were failing for wpe (and both contexts required the fix) - I'm not what's happening with this test for ios.

[Darin Adler]

OK. Here’s what confused me. Typically if tests are failing, the patch needs to update expected results for those tests. So I would have expected 4 different updates to expected results in the patch.

Glad to hear there’s no new test required. Puzzled about the status of these tests.

[Alexey Proskuryakov]

The WebGL 1.0.2 test was fixed for WPE earlier in bug 200434, but it still fails on iOS and on Gtk. There was no change to its pass/fail status when this patch landed. So as Darin observed, this test does not cover your change in WebGLRenderingContext.cpp.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=webgl%2F1.0.2%2Fconformance%2Fcontext%2Fcontext-attributes-alpha-depth-stencil-antialias.html

[Chris Lord]

(In reply to Alexey Proskuryakov from comment #16)
> The WebGL 1.0.2 test was fixed for WPE earlier in bug 200434, but it still
> fails on iOS and on Gtk. There was no change to its pass/fail status when
> this patch landed. So as Darin observed, this test does not cover your
> change in WebGLRenderingContext.cpp.
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#tests=webgl%2F1.0.2%2Fconformance%2Fcontext%2Fcontext-attributes-alpha-
> depth-stencil-antialias.html

I'm having some trouble understanding how to read this page, could you give me some pointers? How do I access the test log for this test, for example?

context-attributes-alpha-depth-stencil-antialias has many tests underneath it and this patch only affects one of them, in case you hadn't already considered that - I'd need to see the pass/fail log to see if this patch isn't tested. Note that it did pass fine on several platforms before, the outcome depended on the behaviour of the GL backend.

[Alexey Proskuryakov]

The UI is not great indeed, haven't updated it in years.

Short story: This test is written in a way that explicitly prevents seeing those individual results, for whatever reason. But maybe fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html is similar enough that you can investigate it instead.

Here is the diff for iOS: <https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r248526%20(5016)/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias-pretty-diff.html>

Here is one for Gtk:
<https://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20(Tests)/r248521%20(5144)/fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias-pretty-diff.html>

Long version:

You should see that the bottom line of rectangles (corresponding to WPE Linux) is green at the left, and red at the right. This means that this test is passing on newer builds, but failing on older builds. GTK and Mac results remain red, meaning that it's still failing.

If you click on any of the rectangles, there will be a "build log" link. Click on it, that sends you to a buildbot page for the test run. Then click on "view layout test results" link. On the results page, there will be an "options" link at the top right - you need to uncheck "Only show unexpected results", because this failure is marked as expected.

Then you will see the test in the list, with links like "diff" and "pretty diff".

[Chris Lord]

(In reply to Alexey Proskuryakov from comment #18)
> You should see that the bottom line of rectangles (corresponding to WPE
> Linux) is green at the left, and red at the right. This means that this test
> is passing on newer builds, but failing on older builds. GTK and Mac results
> remain red, meaning that it's still failing.
> 
> If you click on any of the rectangles, there will be a "build log" link.
> Click on it, that sends you to a buildbot page for the test run. Then click
> on "view layout test results" link. On the results page, there will be an
> "options" link at the top right - you need to uncheck "Only show unexpected
> results", because this failure is marked as expected.
> 
> Then you will see the test in the list, with links like "diff" and "pretty
> diff".

Thanks for this - so it seems this may have introduced new failures in this test. Given that it affects the gtk backend too, this should be easy for me to reproduce and debug. I'll set a build going now and look into this tomorrow.

[Chris Lord]

Testing locally, the Gtk test failures happen with or without this patch, so I guess those failures are unrelated and I misread the log. It seems the wpe results have disappeared from the logs in the link above now so I'm having a tough time determining much.

The sub-test that this affects can still pass even without this patch, it seems to depend on the behaviour of the backend (which is why this patch was only needed on wpe and ios). Gtk isn't failing on the test that this patch affects. I note that there's only an expected results file for the 2.0.0 conformance tests, so I suppose this was already passing on iOS on the 1.x WebGL context?

[Alexey Proskuryakov]

I am not entirely sure which test you are asking about at this point. The flakiness dashboard has authoritative information on whether tests were matching expected results on each platform.

There are multiple issues being discussed here at this point.

1. This patch changes cross-platforms code, but there is no WebGL 1 test on macOS or iOS that went from failing to passing as a result. This means that we don't have a layer of defense against this functionality breaking again in the future.

2. There are various (and different) failures on tests that were discussed above, affecting most platforms.

#2 should probably be discussed in a separate bugzilla bug.

[Chris Lord]

(In reply to Alexey Proskuryakov from comment #21)
> I am not entirely sure which test you are asking about at this point. The
> flakiness dashboard has authoritative information on whether tests were
> matching expected results on each platform.
> 
> There are multiple issues being discussed here at this point.
> 
> 1. This patch changes cross-platforms code, but there is no WebGL 1 test on
> macOS or iOS that went from failing to passing as a result. This means that
> we don't have a layer of defense against this functionality breaking again
> in the future.
> 
> 2. There are various (and different) failures on tests that were discussed
> above, affecting most platforms.
> 
> #2 should probably be discussed in a separate bugzilla bug.

Re. 1, this patch effects the 'Testing alpha = false' part of context-attributes-alpha-depth-stencil-antialias, more specifically, the part that tests that ALPHA_BITS == 0 on a GL context that has no alpha component. This only has an effect if this test was failing previously, which was happening on WPE in all cases, and on iOS (only on the WebGL2 context it seems). The reason other cases were unaffected is because their GraphicsContext implementations returned the expected result without needing any circumvention.

Re. 2, I assume there are bugs filed for failing WebGL tests that are failing unexpectedly - I'm new to WebKit, so I don't know what the expectations are around this yet. Important/supported platforms seem to be covered by automatic regression testing, my patches are focusing on WPE which isn't covered.