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

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 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)

(Assignee)

Description

5 years ago
(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.
(Assignee)

Comment 1

5 years ago
Created attachment 8375289 [details]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 8375903 [details] [diff] [review]
patch 1: Add a test
Attachment #8375903 - Flags: review?(dglastonbury)
(Assignee)

Comment 3

5 years ago
Created attachment 8375904 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D
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+
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 8376624 [details] [diff] [review]
patch 1: Add a test
Attachment #8375903 - Attachment is obsolete: true
Attachment #8376624 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 8376625 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D
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?
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Actually, this doesn't apply to us with WebGL_draw_buffers, since draw_buffers doesn't expose glReadBuffer.
(Assignee)

Comment 13

5 years ago
Created attachment 8377882 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

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?
(Assignee)

Comment 15

5 years ago
Created attachment 8377957 [details] [diff] [review]
patch 1: Add test

r=kamidphish
Attachment #8376624 - Attachment is obsolete: true
Attachment #8377957 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8377957 - Attachment description: readpixels-test → patch 1: Add test
(Assignee)

Comment 16

5 years ago
Created attachment 8377958 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

And now with the actual patch.
Attachment #8377882 - Attachment is obsolete: true
Attachment #8377882 - Flags: review?(dglastonbury)
Attachment #8377958 - Flags: review?(dglastonbury)
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
Blocks: 843666
(Assignee)

Comment 20

5 years ago
Created attachment 8382684 [details] [diff] [review]
patch 3: ReadPixels should generate INV_FB_OP before INV_OP

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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Part 2 doesn't apply.
Keywords: checkin-needed
(Assignee)

Comment 24

5 years ago
Created attachment 8383977 [details] [diff] [review]
patch 1: Add a test

r=kamidphish
Attachment #8377957 - Attachment is obsolete: true
Attachment #8383977 - Flags: review+
(Assignee)

Comment 25

5 years ago
Created attachment 8383978 [details] [diff] [review]
patch 2: Fix ReadPixels and CopyTex(Sub)Image2D

r=kamidphish
Attachment #8377958 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8383978 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

5 years ago
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.