We have some exceptions caused by bad arguments not following the spec, we need to adjust out current checking mechanism in CopyTexImage2D https://www.khronos.org/registry/webgl/sdk/tests/conformance2/textures/misc/copy-texture-image.html?webglVersion=2&quiet=0

Fix indentation inconsistency.

Comment on attachment 8810300 [details] [diff] [review] Part2 - Add DEPTH_STENCIL_ATTACHMENT support for WebGLFramebuffer::ValidForRead Review of attachment 8810300 [details] [diff] [review]: ----------------------------------------------------------------- We still have to add a record inside WebGLFramebuffer because CopyTexImage2D doesn't pass buffer target enum. We should record it when we call FramebufferTexture2D.

Comment on attachment 8810299 [details] [diff] [review] Part1 - Fix conformance failure texture-image caused by incorrect internal format checking order. Review of attachment 8810299 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the indent.

Comment on attachment 8810300 [details] [diff] [review] Part2 - Add DEPTH_STENCIL_ATTACHMENT support for WebGLFramebuffer::ValidForRead Review of attachment 8810300 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure what you're trying to record here. ::: dom/canvas/WebGLFramebuffer.h @@ +154,4 @@ > > //// > > + GLenum mCurrentAttachEnum; Framebuffers have no concept like this that I'm aware of.

Comment on attachment 8813984 [details] [diff] [review] Part1 - Fix conformance failure texture-image caused by incorrect internal format checking order. Review of attachment 8813984 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLTextureUpload.cpp @@ +1925,5 @@ > > + if (!ValidateSizedInternalFormat(dstFormat->sizedFormat)) { > + webgl->ErrorInvalidEnum("%s: Invalid Sized internalFormat.", funcName); > + return nullptr; > + } Why is this needed?

(In reply to Jeff Gilbert [:jgilbert] from comment #7) > Comment on attachment 8813984 [details] [diff] [review] > Part1 - Fix conformance failure texture-image caused by incorrect internal > format checking order. > > Review of attachment 8813984 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGLTextureUpload.cpp > @@ +1925,5 @@ > > > > + if (!ValidateSizedInternalFormat(dstFormat->sizedFormat)) { > > + webgl->ErrorInvalidEnum("%s: Invalid Sized internalFormat.", funcName); > > + return nullptr; > > + } > > Why is this needed? The conformance test has several subtests with invalid internal format enum which should return ERROR_INVALID_ENUM, but we didn't check it, so it will continue executing until it fails checks about format consistency which returns ERROR_INVALID_OPERATION and it is not correct for the test. https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTextureUpload.cpp?q=%2Bfunction%3A%22mozilla%3A%3AValidateCopyDestUsage%28const+char+%2A%2C+mozilla%3A%3AWebGLContext+%2A%2C+const+webgl%3A%3AFormatInfo+%2A%2C+GLenum%29%22&redirect_type=single#1877

After discussing with Ethan, we think this test issue is a matter of internal format checking order. So both INVALID_ENUM and INVALID_OPERATION should be correct in this case. Maybe we should change the test instead of Gecko part.

Comment on attachment 8816018 [details] [diff] [review] Part2 - Add Support for non-color buffer in WebGLFramebuffer::ValidateForRead Review of attachment 8816018 [details] [diff] [review]: ----------------------------------------------------------------- This function is only for color buffers.

(In reply to Michael Leu[:lenzak800](UTC+8) from comment #11) > After discussing with Ethan, we think this test issue is a matter of > internal format checking order. > > So both INVALID_ENUM and INVALID_OPERATION should be correct in this case. > > Maybe we should change the test instead of Gecko part. This happens a lot, so this is fairly likely.

Here's the PR for the test: https://github.com/KhronosGroup/WebGL/pull/2226

Comment on attachment 8822810 [details] Bug 1316546 - CopyTexImage can convert unsigned fixed-point to signed fixed-point. - https://reviewboard.mozilla.org/r/101598/#review102108

Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5345e4d62611 CopyTexImage can convert unsigned fixed-point to signed fixed-point. - r=lenzak800

Based on discussion with the WG, it sounds like this is an inaccuracy in the GLES3 spec: https://github.com/KhronosGroup/WebGL/pull/2226 Word is that CopyTexImage should not accept unorm->snorm conversions.