Bug 209139

Summary:

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

Product:

WebKit

Reporter:

Jason Lawrence <Lawrence.j>

Component:

WebGL

Assignee:

Kenneth Russell <kbr>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dino, ews-watchlist, graouts, jdarpinian, justin_fan, kbr, kkinnunen, kondapallykalyan, mmaxfield, webkit-bot-watchers-bugzilla, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

iPhone / iPad

OS:

iOS 13

See Also:

https://bugs.webkit.org/show_bug.cgi?id=205736
https://bugs.webkit.org/show_bug.cgi?id=218318

Bug Depends on:

205618, 212249

Bug Blocks:

207858, 215804

Attachments:

Description	Flags
Patch	none

Jason Lawrence

Reported 2020-03-16 09:10:38 PDT

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

Attachments
Patch (5.57 KB, patch) 2020-10-25 01:20 PDT, Kenneth Russell	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-03-16 09:11:16 PDT

<rdar://problem/60496751>

Jason Lawrence

Comment 2 2020-03-16 09:15:14 PDT

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

Justin Fan

Comment 3 2020-05-22 11:45:48 PDT

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

Comment 4 2020-07-07 12:11:32 PDT

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

Comment 5 2020-07-07 13:28:19 PDT

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

Comment 6 2020-07-21 14:36:01 PDT

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

Comment 7 2020-07-21 14:54:47 PDT

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

Comment 8 2020-07-21 14:58:04 PDT

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

Comment 9 2020-09-17 14:48:00 PDT

Investigating.

Kenneth Russell

Comment 10 2020-09-17 17:52:58 PDT

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

Comment 11 2020-10-01 16:52:38 PDT

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

Comment 12 2020-10-23 19:00:36 PDT

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

Comment 13 2020-10-25 01:20:36 PDT

Created attachment 412260 [details] Patch

Kimmo Kinnunen

Comment 14 2020-10-26 00:27:08 PDT

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

Comment 15 2020-10-26 13:10:33 PDT

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

Comment 16 2020-10-26 13:33:48 PDT

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

Comment 17 2020-10-26 16:57:38 PDT

(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

Comment 18 2020-10-27 00:12:14 PDT

> 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 19 2020-10-27 00:20:14 PDT

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

Comment 20 2020-10-27 10:35:14 PDT

Committed r269051: <https://trac.webkit.org/changeset/269051> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412260 [details].

Note You need to log in before you can comment on or make changes to this bug.