Last Comment Bug 706674 - WebGLFramebufferAttachment's are not WebGLRectangleObject's
: WebGLFramebufferAttachment's are not WebGLRectangleObject's
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
: 630391 (view as bug list)
Depends on: 707460
Blocks: webgl-conformance
  Show dependency treegraph
 
Reported: 2011-11-30 14:40 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-01-25 07:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix framebuffer attachments (28.81 KB, patch)
2011-11-30 14:40 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
fix framebuffer attachments (28.96 KB, patch)
2011-12-01 16:26 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Splinter Review
fix framebuffer attachments (28.96 KB, patch)
2012-01-20 12:23 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-11-30 14:40:31 PST
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.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 16:26:56 PST
Created attachment 578439 [details] [diff] [review]
fix framebuffer attachments
Comment 2 Jeff Gilbert [:jgilbert] 2011-12-05 13:59:56 PST
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) 2012-01-20 12:22:09 PST
(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) 2012-01-20 12:23:22 PST
Created attachment 590295 [details] [diff] [review]
fix framebuffer attachments
Comment 5 Jeff Gilbert [:jgilbert] 2012-01-20 14:34:15 PST
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 6 Jeff Gilbert [:jgilbert] 2012-01-20 14:54:11 PST
*** Bug 630391 has been marked as a duplicate of this bug. ***
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-01-23 11:36:21 PST
https://tbpl.mozilla.org/?tree=Try&rev=7a9a3e7a8ddd
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-01-23 12:28:27 PST
https://tbpl.mozilla.org/?tree=Try&rev=92f68b3413d0
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-01-24 13:15:45 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9a4b9014002
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:23:25 PST
https://hg.mozilla.org/mozilla-central/rev/f9a4b9014002

Note You need to log in before you can comment on or make changes to this bug.