Bug 209139 – [ iOS wk2 ] webgl/1.0.3/conformance/textures/copy-tex-image-2d-formats.html is failing.

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

[Jason Lawrence]

webgl/1.0.3/conformance/textures/copy-tex-image-2d-formats.html

Description:
This test is failing on iOS wk2. This appears to be related to the solution provided here: https://trac.webkit.org/changeset/258462.

History:
https://results.webkit.org/?suite=layout-tests&test=webgl%2F1.0.3%2Fconformance%2Ftextures%2Fcopy-tex-image-2d-formats.html&platform=ios&limit=50000

Diff:
--- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/webgl/1.0.3/conformance/textures/copy-tex-image-2d-formats-expected.txt
+++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/webgl/1.0.3/conformance/textures/copy-tex-image-2d-formats-actual.txt
@@ -1,5 +1,56 @@
 This test runs the WebGL Test listed below in an iframe and reports PASS or FAIL.
 
 Test: ../../resources/webgl_test_files/conformance/textures/copy-tex-image-2d-formats.html
-[ PASS ] All tests passed
+[ 1: PASS ] getError was expected value: NO_ERROR : During Initialization
+[ 2: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D ALPHA from RGBA
+[ 3: PASS ] should be 0,0,0,127
+[ 4: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 5: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE from RGBA
+[ 6: PASS ] should be 64,64,64,255
+[ 7: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 8: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE_ALPHA from RGBA
+[ 9: PASS ] should be 64,64,64,127
+[ 10: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 11: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGB from RGBA
+[ 12: PASS ] should be 64,255,191,255
+[ 13: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 14: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGBA from RGBA
+[ 15: PASS ] should be 64,255,191,127
+[ 16: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 17: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D ALPHA from RGB
+[ 18: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE from RGB
+[ 19: PASS ] should be 64,64,64,255
+[ 20: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 21: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D LUMINANCE_ALPHA from RGB
+[ 22: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGB from RGB
+[ 23: PASS ] should be 64,255,191,255
+[ 24: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 25: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D RGBA from RGB
+[ 26: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D ALPHA from RGBA
+[ 27: PASS ] should be 0,0,0,127
+[ 28: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 29: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE from RGBA
+[ 30: PASS ] should be 64,64,64,255
+[ 31: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 32: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE_ALPHA from RGBA
+[ 33: PASS ] should be 64,64,64,127
+[ 34: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 35: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGB from RGBA
+[ 36: PASS ] should be 64,255,191,255
+[ 37: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 38: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGBA from RGBA
+[ 39: PASS ] should be 64,255,191,127
+[ 40: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 41: PASS ] getError was expected value: NO_ERROR : During Initialization
+[ 42: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D ALPHA from RGB
+[ 43: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE from RGB
+[ 44: FAIL ] at (0, 0) expected: 64,64,64,255 was 64,64,64,0
+[ 45: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 46: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D LUMINANCE_ALPHA from RGB
+[ 47: PASS ] getError was expected value: NO_ERROR : should be able to copyTexImage2D RGB from RGB
+[ 48: FAIL ] at (0, 0) expected: 64,255,191,255 was 64,255,191,0
+[ 49: PASS ] getError was expected value: NO_ERROR : should be no errors
+[ 50: PASS ] getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D RGBA from RGB
+[ 51: PASS ] successfullyParsed is true
+[ FAIL ] 2 failures reported

[Radar WebKit Bug Importer]

<rdar://problem/60496751>

[Jason Lawrence]

I have marked this test as failing while this issue is investigated.
https://trac.webkit.org/changeset/258500/webkit

[Justin Fan]

On iOS, it looks like copyTexImage2D is altering the alpha channel for RGB/LUMINANCE copies when it should be leaving those bits untouched.

[Kenneth Russell]

jdarpinian@ pointed out that he just reimplemented CopyTexImage2D on iOS in Bug 212249 to work around other driver bugs; that might have already fixed this problem.

[Justin Fan]

unfortunately, this test is still failing on iOS 13 Simulators. I can try to reproduce again once I get another working simulator build.

[Justin Fan]

There are only two sub-failures occurring in this test; when copyTexImage2D is attempted from LUMINANCE and RGB formats to the RGB format with an RGB backbuffer, the alpha channel is written as 255 when it should be 0.

[Justin Fan]

Sorry, ignore the last. What I meant to say was "when copyTexImage2D is attempted from an RGB backbuffer to LUMINANCE or RGB, the alpha channel is written as 255 when it should be 0.

[Kenneth Russell]

Not 100% sure how the test reads back the texture's data, but it sounds like it's drawing the resulting texture back to an RGBA back buffer and then calling readPixels with RGBA format.

Thanks Justin for the triage - we'll investigate.

[Kenneth Russell]

Investigating.

[Kenneth Russell]

The same failure as originally reported still happens in the iOS Simulator:

PASS getError was expected value: NO_ERROR : should be able to copyTexImage2D LUMINANCE from RGB
FAIL at (0, 0) expected: 64,64,64,255 was 64,64,64,0
PASS getError was expected value: NO_ERROR : should be no errors
PASS getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D LUMINANCE_ALPHA from RGB
PASS getError was expected value: NO_ERROR : should be able to copyTexImage2D RGB from RGB
FAIL at (0, 0) expected: 64,255,191,255 was 64,255,191,0
PASS getError was expected value: NO_ERROR : should be no errors
PASS getError was expected value: INVALID_OPERATION : should not be able to copyTexImage2D RGBA from RGB

[Kenneth Russell]

Still haven't had time to debug into ANGLE on the iOS Simulator yet. Potentially related bugs pointed out by jdarpinian@:

copyTexImage2D to LUMINANCE/ALPHA fails with internal context error on mac
http://anglebug.com/5051

copy-texture-image-same-texture.html fails on OpenGL backend
http://anglebug.com/2994

[Kenneth Russell]

The bug is that readPixels readbacks from an {alpha:false} back buffer are coming back with the alpha channel zeroed out, rather than 0xFF. It has nothing to do with CopyTexImage2D from the RGB back buffer.

[Kenneth Russell]

Created attachment 412260 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 412260 [details]
Patch

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

looks good to me, not a reviewer.

> Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:610
> +    if (!readingToPixelBufferObject && !attrs.alpha && (format == GraphicsContextGL::RGBA || format == GraphicsContextGL::BGRA) && (type == GraphicsContextGL::UNSIGNED_BYTE) && (state.boundReadFBO == m_context->m_fbo || (attrs.antialias && state.boundReadFBO == m_context->m_multisampleFBO)))

Out of curiosity, did we not want to extend ANGLE workarounds around this area instead of existing more unstructured workarounds? Or would this be more task for downstream core developers? 

It'd help maybe making  this more perfect in cases like preserveDrawingBuffer, which probably shouldn't need the workaround on grounds of the backing being normal texture instead of an iosurface
I guess part of the tedious work would be to understand what's the root cause of the problem.

I did a bit of similar work in https://bugs.webkit.org/show_bug.cgi?id=215804 but it's not finished due to not being that important. It might or might not be same root cause but different trigger this time. E.g. in that bug, glclear is "buggy", but it might be that BlitFramebuffer is buggy. So in this case, it might be that read pixels work but then rendering from blitted-to texture would be black. Anyhow, this is an tangent to the actual question wrt do we want the fix eventually in ANGLE or not.

[Kenneth Russell]

Yes, ideally this fix would be inside ANGLE. There are a couple of weird things going on with this bug from my perspective:

1) On the iOS Simulator, the WebGL back buffer texture isn't actually bound to the IOSurface. The [EAGLContext texImageIOSurface:target:internalFormat:width:height:format:type:plane:] API doesn't exist in the iOS simulator. Earlier in the development cycle of the ANGLE backend for WebKit, we emulated this behavior using readbacks in Bug 205618.

https://developer.apple.com/documentation/opengles/eaglcontext/2890259-teximageiosurface?language=objc

This means that the backing texture of the {alpha:false} canvas is just a vanilla RGB texture. I'm not sure why readbacks are coming back with transparent alpha. Compositing of the canvas on the page works fine.

2) This workaround's been done at the WebKit level for some time on both iOS and macOS, just not on iOS in the ANGLE backend. It seems appropriate to do that as a short term fix.

3) It's possible ANGLE's Metal backend could address this. If you have a solid fix for Bug 215804 then let's certainly get it in to ANGLE - but I'm inclined to check as soon as possible whether the longstanding OpenGL texture/IOSurface interaction problems have been addressed in Metal's IOSurface integration APIs.

[Kimmo Kinnunen]

Thanks for the info and the observations!

> This means that the backing texture of the {alpha:false} canvas is just a vanilla RGB texture. I'm not sure why readbacks are coming back with transparent alpha. 

The pbuffers with RGB internal format are mapped to native internal format RGBA textures on simulator. The initialise alpha should set the initial alpha ok but maybe the blit overrides this?

[Kenneth Russell]

(In reply to Kimmo Kinnunen from comment #16)
> Thanks for the info and the observations!
> 
> > This means that the backing texture of the {alpha:false} canvas is just a vanilla RGB texture. I'm not sure why readbacks are coming back with transparent alpha. 
> 
> The pbuffers with RGB internal format are mapped to native internal format
> RGBA textures on simulator. The initialise alpha should set the initial
> alpha ok but maybe the blit overrides this?

I see - thanks for that reminder and clarification.

Actually - the bug is in the alpha:false, antialias:false path. Setting antialias:true for the glNoAlpha context in https://trac.webkit.org/browser/webkit/trunk/LayoutTests/webgl/1.0.3/resources/webgl_test_files/conformance/textures/copy-tex-image-2d-formats.html makes the bug disappear.

Would it be OK if I land this change and then we follow up to try to fix the underlying issue and remove this workaround for both macOS and iOS?

[Kimmo Kinnunen]

> Would it be OK if I land this change 

Not sure if you ask me or in general, but: certainly it's ok for my part.

[Dean Jackson]

Comment on attachment 412260 [details]
Patch

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

>> Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:610
>> +    if (!readingToPixelBufferObject && !attrs.alpha && (format == GraphicsContextGL::RGBA || format == GraphicsContextGL::BGRA) && (type == GraphicsContextGL::UNSIGNED_BYTE) && (state.boundReadFBO == m_context->m_fbo || (attrs.antialias && state.boundReadFBO == m_context->m_multisampleFBO)))
> 
> Out of curiosity, did we not want to extend ANGLE workarounds around this area instead of existing more unstructured workarounds? Or would this be more task for downstream core developers? 
> 
> It'd help maybe making  this more perfect in cases like preserveDrawingBuffer, which probably shouldn't need the workaround on grounds of the backing being normal texture instead of an iosurface
> I guess part of the tedious work would be to understand what's the root cause of the problem.
> 
> I did a bit of similar work in https://bugs.webkit.org/show_bug.cgi?id=215804 but it's not finished due to not being that important. It might or might not be same root cause but different trigger this time. E.g. in that bug, glclear is "buggy", but it might be that BlitFramebuffer is buggy. So in this case, it might be that read pixels work but then rendering from blitted-to texture would be black. Anyhow, this is an tangent to the actual question wrt do we want the fix eventually in ANGLE or not.

I'm ok with this landing for now in WebKit, but I agree that it might want to live in ANGLE.

I'll mark it as r+ and Ken can decide.

[EWS]

Committed r269051: <https://trac.webkit.org/changeset/269051>

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