ReadPixels from depth-only FBs behavior is buggy, underspecified, and differs from Chrome

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Love these bugs >>)
ReadPixels from complete FBs without a ColorAttachment0 (or ColorAttachmentI with ReadBuffer) doesn't appear to be defined well. Since the specs don't mention it, I can only assume that we're supposed to have the same behavior as reading out-of-bounds.

I have a post on the WebGL mailing list to discuss this, and figure out how we should handle this at a spec level.

Once we know what to do, we should fix our code.
Posted file testcase
Fails on Firefox for 0x0 reads.
Fails on Chrome for both 0x0 and 1x1 reads.

Before the 1x1 read, pixels[4] is [1,2,3,4].
After the 1x1 read:
Firefox leaves pixels as [1,2,3,255]
Chrome leaves pixels as [0,0,0,255]

Not only should we generate an error in the 0x0 read case (for consistency), but in the 1x1 case, we should not write to the buffer if we emit an error.
Posted patch patch 1: Add a test (obsolete) — Splinter Review
Attachment #8375903 - Flags: review?(dglastonbury)
Attachment #8375904 - Flags: review?(dglastonbury)
Comment on attachment 8375903 [details] [diff] [review]
patch 1: Add a test

Review of attachment 8375903 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/webgl/non-conf-tests/test_depth_readpixels.html
@@ +28,5 @@
> +  gl.framebufferRenderbuffer(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT,
> +                             gl.RENDERBUFFER, rb);
> +
> +  if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE) {
> +    todo(false, 'Depth-only FB incomplete. This is valid.')

missing ;
Attachment #8375903 - Flags: review?(dglastonbury) → review+
Comment on attachment 8375904 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

Review of attachment 8375904 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but I think it would be nice to be explicit about what the "correct planes" (ie. GL_COLOR_ATTACHMENT) means.

::: content/canvas/src/WebGLContextGL.cpp
@@ +532,5 @@
>  
> +    if (mBoundFramebuffer) {
> +        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> +        if (mBoundFramebuffer->HasCompletePlanes(readPlaneBits))
> +            return ErrorInvalidOperation("copyTexImage2D: read source doesn't have the correct planes.");

Would it be better to say something like:
"copyTexImage2D: read source is missing GL_COLOR_ATTACHMENT"
to be explicit?
Attachment #8375904 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> Comment on attachment 8375903 [details] [diff] [review]
> patch 1: Add a test
> 
> Review of attachment 8375903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/test/webgl/non-conf-tests/test_depth_readpixels.html
> @@ +28,5 @@
> > +  gl.framebufferRenderbuffer(gl.FRAMEBUFFER, gl.DEPTH_ATTACHMENT,
> > +                             gl.RENDERBUFFER, rb);
> > +
> > +  if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE) {
> > +    todo(false, 'Depth-only FB incomplete. This is valid.')
> 
> missing ;

Oops, thanks.

(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8375904 [details] [diff] [review]
> patch 2: Fix ReadPixels and CopyTex(Sub)Image2D
> 
> Review of attachment 8375904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, but I think it would be nice to be explicit about what the "correct
> planes" (ie. GL_COLOR_ATTACHMENT) means.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +532,5 @@
> >  
> > +    if (mBoundFramebuffer) {
> > +        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> > +        if (mBoundFramebuffer->HasCompletePlanes(readPlaneBits))
> > +            return ErrorInvalidOperation("copyTexImage2D: read source doesn't have the correct planes.");
> 
> Would it be better to say something like:
> "copyTexImage2D: read source is missing GL_COLOR_ATTACHMENT"
> to be explicit?

Yeah, I was trying to be mindful of what we'll need here in the future, but that would be more explicit for the time being.
Posted patch patch 1: Add a test (obsolete) — Splinter Review
Attachment #8375903 - Attachment is obsolete: true
Attachment #8376624 - Flags: review+
Attachment #8375904 - Attachment is obsolete: true
Attachment #8376625 - Flags: review?(dglastonbury)
Comment on attachment 8376625 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

Review of attachment 8376625 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +624,5 @@
> +    MOZ_ASSERT(mContext->mBoundFramebuffer == this);
> +    bool hasPlanes = true;
> +    if (mask & LOCAL_GL_COLOR_BUFFER_BIT) {
> +        hasPlanes &= ColorAttachmentCount() &&
> +                     ColorAttachment(0).IsDefined();

What is the interaction with WEBGL_draw_buffers? The spec. doesn't say, but https://www.opengl.org/wiki/GlReadBuffer says:

"If a non-zero framebuffer object is bound, then the constants GL_COLOR_ATTACHMENTi​ may be used to indicate the ith color attachment, where i ranges from zero to the value of GL_MAX_COLOR_ATTACHMENTS​ minus one."

Maybe put this one on the todo list?
(In reply to Dan Glastonbury :djg :kamidphish from comment #10)
> Comment on attachment 8376625 [details] [diff] [review]
> patch 2: Fix ReadPixels and CopyTex(Sub)Image2D
> 
> Review of attachment 8376625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLFramebuffer.cpp
> @@ +624,5 @@
> > +    MOZ_ASSERT(mContext->mBoundFramebuffer == this);
> > +    bool hasPlanes = true;
> > +    if (mask & LOCAL_GL_COLOR_BUFFER_BIT) {
> > +        hasPlanes &= ColorAttachmentCount() &&
> > +                     ColorAttachment(0).IsDefined();
> 
> What is the interaction with WEBGL_draw_buffers? The spec. doesn't say, but
> https://www.opengl.org/wiki/GlReadBuffer says:
> 
> "If a non-zero framebuffer object is bound, then the constants
> GL_COLOR_ATTACHMENTi​ may be used to indicate the ith color attachment,
> where i ranges from zero to the value of GL_MAX_COLOR_ATTACHMENTS​ minus
> one."
> 
> Maybe put this one on the todo list?
I should do a follow-up patch for this, I think.
Actually, this doesn't apply to us with WebGL_draw_buffers, since draw_buffers doesn't expose glReadBuffer.
Fix bitrot.
Attachment #8376625 - Attachment is obsolete: true
Attachment #8376625 - Flags: review?(dglastonbury)
Attachment #8377882 - Flags: review?(dglastonbury)
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Actually, this doesn't apply to us with WebGL_draw_buffers, since
> draw_buffers doesn't expose glReadBuffer.

Oh. glReadBuffers doesn't exist in GLES 2, does it? So the only way to read from a color attachment, other than 0, is to attach to another FBO in the 0th slot?
Posted patch patch 1: Add test (obsolete) — Splinter Review
r=kamidphish
Attachment #8376624 - Attachment is obsolete: true
Attachment #8377957 - Flags: review+
Attachment #8377957 - Attachment description: readpixels-test → patch 1: Add test
And now with the actual patch.
Attachment #8377882 - Attachment is obsolete: true
Attachment #8377882 - Flags: review?(dglastonbury)
Attachment #8377958 - Flags: review?(dglastonbury)
(In reply to Dan Glastonbury :djg :kamidphish from comment #14)
> (In reply to Jeff Gilbert [:jgilbert] from comment #12)
> > Actually, this doesn't apply to us with WebGL_draw_buffers, since
> > draw_buffers doesn't expose glReadBuffer.
> 
> Oh. glReadBuffers doesn't exist in GLES 2, does it? So the only way to read
> from a color attachment, other than 0, is to attach to another FBO in the
> 0th slot?

Yes.
Blocks: 973394
Comment on attachment 8377958 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

Review of attachment 8377958 [details] [diff] [review]:
-----------------------------------------------------------------

gtg
Attachment #8377958 - Flags: review?(dglastonbury) → review+
Blocks: 843666
I think we can do either, but let's prefer to generate INV_FB_OP when the FB is incomplete before generating INV_OP that's caused by the incomplete FB.
Attachment #8382684 - Flags: review?(dglastonbury)
Comment on attachment 8382684 [details] [diff] [review]
patch 3: ReadPixels should generate INV_FB_OP before INV_OP

Review of attachment 8382684 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with what you've done here.
Attachment #8382684 - Flags: review?(dglastonbury) → review+
Keywords: checkin-needed
Part 2 doesn't apply.
Keywords: checkin-needed
r=kamidphish
Attachment #8377957 - Attachment is obsolete: true
Attachment #8383977 - Flags: review+
r=kamidphish
Attachment #8377958 - Attachment is obsolete: true
Attachment #8383978 - Flags: review+
Keywords: checkin-needed
It applies now. It must have been an old version of the patch.
They all apply to inbound, but it's closed at the moment, so marking this checkin-needed again.
You need to log in before you can comment on or make changes to this bug.