WebGLFramebufferAttachment's are not WebGLRectangleObject's

RESOLVED FIXED in mozilla12

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 578090 [details] [diff] [review]
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.
Attachment #578090 - Flags: review?(jgilbert)
(Assignee)

Updated

6 years ago
Blocks: 680721
(Assignee)

Comment 1

6 years ago
Created attachment 578439 [details] [diff] [review]
fix framebuffer attachments
Attachment #578090 - Attachment is obsolete: true
Attachment #578090 - Flags: review?(jgilbert)
Attachment #578439 - Flags: review?(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
Attachment #578439 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 3

6 years ago
(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 :)
(Assignee)

Comment 4

6 years ago
Created attachment 590295 [details] [diff] [review]
fix framebuffer attachments
Attachment #578439 - Attachment is obsolete: true
Attachment #590295 - Flags: review?(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.
Attachment #590295 - Flags: review?(jgilbert) → review+
Depends on: 707460
Duplicate of this bug: 630391
(Assignee)

Comment 7

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7a9a3e7a8ddd
(Assignee)

Comment 8

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=92f68b3413d0
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9a4b9014002
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/f9a4b9014002
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.