706674 - WebGLFramebufferAttachment's are not WebGLRectangleObject's

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix framebuffer attachments

The object-deletion-behaviour.html conformance test got updated again, revealing another issue in our implementation. It is now attaching a newly created texture to a FBO, and only then does it call texImage2D on it. We fail in this case, because we wrongly believe that a framebuffer attachment is a WebGLRectangleObject i.e. stores mWidth and mHeight members, so when we attach a texture we record its dimensions at that time and we don't realize that they can change later.

The solution is to remove the WebGLRectangleObject inheritance from WebGLFramebufferAttachment, and instead have WebGLFramebufferAttachment query its texture or renderbuffer for their dimensions, when needed.

In the course of doing this, a couple other needed changes appeared:
 - let WebGLTexture::ImageInfo inherit WebGLRectangleObject, so that WebGLFramebufferAttachment can return a pointer to it right away in the texture case
 - add a FramebufferRectangleObject() method to WebGLContext, that will return the dimensions of the bound FBO if there is one, otherwise will just return the contexts' own dimensions.
 - in order to do that, it was very convenient to let WebGLContext inherit WebGLRectangleObject so that FramebufferRectangleObject() could just return a pointer to that in the no-bound-FBO case.
 - move WebGLRectangleObject up in the file, above WebGLContext 
 - there was a plain bug in WebGLFramebufferAttachment::HasAlpha(), we were not using the right image info within the texture.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix framebuffer attachments

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578439 [details] [diff] [review]
fix framebuffer attachments

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

::: content/canvas/src/WebGLContext.h
@@ +2235,5 @@
>              mask |= LOCAL_GL_STENCIL_BUFFER_BIT;
>          }
>  
> +        const WebGLRectangleObject *rect = mColorAttachment.RectangleObject();
> +        mContext->ForceClearFramebufferWithDefaultValues(mask, nsIntRect(0, 0, rect->Width(), rect->Height()));

Are we guaranteed a valid color attachment, and that the resulting RectangleObject() is non-null?

@@ +2340,5 @@
>  };
>  
> +inline const WebGLRectangleObject *WebGLContext::FramebufferRectangleObject() const {
> +    return mBoundFramebuffer ? mBoundFramebuffer->RectangleObject()
> +                              : static_cast<const WebGLRectangleObject*>(this);

Critical issue: It looks like your alignment is off by a space. :P

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 578439 [details] [diff] [review]
> fix framebuffer attachments
> 
> Review of attachment 578439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +2235,5 @@
> >              mask |= LOCAL_GL_STENCIL_BUFFER_BIT;
> >          }
> >  
> > +        const WebGLRectangleObject *rect = mColorAttachment.RectangleObject();
> > +        mContext->ForceClearFramebufferWithDefaultValues(mask, nsIntRect(0, 0, rect->Width(), rect->Height()));
> 
> Are we guaranteed a valid color attachment, and that the resulting
> RectangleObject() is non-null?

That was a good remark and indeed the patch in bug 707460 reworked that part and added this in between:

+        if (!rect ||
+            !rect->Width() ||
+            !rect->Height())
+            return false;

I would suggest r+ing this patch and taking this conversation to bug 707460 once I've replied to your comments there.


> 
> @@ +2340,5 @@
> >  };
> >  
> > +inline const WebGLRectangleObject *WebGLContext::FramebufferRectangleObject() const {
> > +    return mBoundFramebuffer ? mBoundFramebuffer->RectangleObject()
> > +                              : static_cast<const WebGLRectangleObject*>(this);
> 
> Critical issue: It looks like your alignment is off by a space. :P

I am so sorry :) The internet almost blew up :)

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix framebuffer attachments

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 590295 [details] [diff] [review]
fix framebuffer attachments

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

Looks good, but should be landed with bug 707460 as per earlier comment.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=7a9a3e7a8ddd

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=92f68b3413d0

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f9a4b9014002

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/f9a4b9014002