Bug 238134

Created attachment 455221 [details] Patch

webgl/2.0.y/conformance/reading/read-pixels-test.html now passes in entirety.

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

Created attachment 455308 [details] Patch

Created attachment 455310 [details] Patch

Comment on attachment 455310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455310&action=review > Source/WebCore/ChangeLog:3 > + Validate readpixels format and type inside of WebCore Silly question: Why doesn't ANGLE validate this for us?

Created attachment 455380 [details] Patch

Comment on attachment 455380 [details] Patch Just FYI: ANGLE does do this validation. The only validation that should be needed at the WebCore level is to ensure that the incoming typed array type is compatible with the format being requested in readPixels. See Chromium's associated validation: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568 https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608

(In reply to Kenneth Russell from comment #8) > Comment on attachment 455380 [details] > Patch > > Just FYI: ANGLE does do this validation. The only validation that should be > needed at the WebCore level is to ensure that the incoming typed array type > is compatible with the format being requested in readPixels. See Chromium's > associated validation: > > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/ > renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568 > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/ > renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608 Thanks, Ken. My original patch here was on ANGLE. It seems there are two things going on: 1. ANGLE's readpixel validation allows UNSIGNED_INT_24_8, which does not seem to be accurate to my spec reading. 2. ANGLE's readpixel validation early outs for read-pixels-test.html because of a missing attachment which is undefined behavior, see these lines in validationES.cpp if (readBuffer == nullptr) { context->validationError(entryPoint, GL_INVALID_OPERATION, kMissingReadAttachment); return false; } I'm happy to fix the first in upstream ANGLE, and fix the undefined behavior in the test which is causing it to fail. I wonder if it still makes sense to swap the order of the undefined behavior check of missing attachment to after the pixel/type validation in ANGLE.

(In reply to John Cunningham from comment #9) > (In reply to Kenneth Russell from comment #8) > > Comment on attachment 455380 [details] > > Patch > > > > Just FYI: ANGLE does do this validation. The only validation that should be > > needed at the WebCore level is to ensure that the incoming typed array type > > is compatible with the format being requested in readPixels. See Chromium's > > associated validation: > > > > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/ > > renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568 > > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/ > > renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608 > > Thanks, Ken. My original patch here was on ANGLE. It seems there are two > things going on: > > 1. ANGLE's readpixel validation allows UNSIGNED_INT_24_8, which does not > seem to be accurate to my spec reading. > > 2. ANGLE's readpixel validation early outs for read-pixels-test.html because > of a missing attachment which is undefined behavior, see these lines in > validationES.cpp > > if (readBuffer == nullptr) > { > context->validationError(entryPoint, GL_INVALID_OPERATION, > kMissingReadAttachment); > return false; > } > > I'm happy to fix the first in upstream ANGLE, and fix the undefined behavior > in the test which is causing it to fail. I wonder if it still makes sense to > swap the order of the undefined behavior check of missing attachment to > after the pixel/type validation in ANGLE. Thanks for the confirmation; I see. For (1), it sounds fine to look into fixing this in ANGLE; but the bug is in Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp, WebGLRenderingContextBase::validateArrayBufferType. The case for GraphicsContextGL::UNSIGNED_INT_24_8 should just be removed; then that part of the test will pass. Not sure about the second bug though. Happy to discuss further.

After further discussion on Slack, I see that the existing validateArrayBufferType is used for both Tex{Sub}Image and ReadPixels operations. Still - would you consider passing an additional parameter to validateArrayBufferType, whether it's for ReadPixels or not? Then it could reject UNSIGNED_INT_24_8 for ReadPixels and avoid duplicating this code.

<rdar://problem/90910118>

Landed in ANGLE upstream: https://bugs.chromium.org/p/angleproject/issues/detail?id=7119

Should roll into WebKit in Bug 239114.

I think this bug can be closed (if scope is just update of ANGLE library) since it was within ANGLE library and it was updated as mentioned in Comment 13. But, from Comment 10 to remove a case bit within "Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp", I noticed from Webkit Github mirror that it is still present on line 5504. Webkit Mirror file link - https://github.com/WebKit/WebKit/blob/2e84b9750f8c04ed405b84e8fbfb2dbc0bf24497/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp Thanks!