| 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: | P2 | 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
Jason Lawrence
2020-03-16 09:10:38 PDT
I have marked this test as failing while this issue is investigated. https://trac.webkit.org/changeset/258500/webkit On iOS, it looks like copyTexImage2D is altering the alpha channel for RGB/LUMINANCE copies when it should be leaving those bits untouched. 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. unfortunately, this test is still failing on iOS 13 Simulators. I can try to reproduce again once I get another working simulator build. 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. 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. 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. Investigating. 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 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 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.
Created attachment 412260 [details]
Patch
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. 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. 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?
(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? > 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.
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. Committed r269051: <https://trac.webkit.org/changeset/269051> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412260 [details]. |