Last Comment Bug 707460 - Fix WebGL framebuffer statuses and errors
: Fix WebGL framebuffer statuses and errors
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)
:
Mentors:
Depends on: 703927
Blocks: webgl-conformance 685184 706674
  Show dependency treegraph
 
Reported: 2011-12-03 10:34 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-01-25 07:23 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix framebuffer errors (24.30 KB, patch)
2011-12-03 10:34 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
fix framebuffer errors (25.03 KB, patch)
2012-01-20 12:50 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-12-03 10:34:18 PST
Created attachment 578849 [details] [diff] [review]
fix framebuffer errors

We don't pass the recent revisions of the framebuffer-object-attachment tests; as discussed on public_webgl "framebuffer-object-attachment test is not passable" thread, there were issues in it, fixed in r16237 (see bug 703927). This patch makes us pass the r16237 revision.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-12-03 10:41:11 PST
https://tbpl.mozilla.org/?tree=Try&rev=76fa2c4d0534
 (apparently tryserver currently has a problem making debug fail)
Comment 2 Jeff Gilbert [:jgilbert] 2011-12-06 15:16:57 PST
Comment on attachment 578849 [details] [diff] [review]
fix framebuffer errors

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

::: content/canvas/src/WebGLContext.h
@@ +1960,5 @@
> +                case LOCAL_GL_STENCIL_ATTACHMENT:
> +                    return format == LOCAL_GL_STENCIL_INDEX8;
> +                case LOCAL_GL_DEPTH_STENCIL_ATTACHMENT:
> +                    return format == LOCAL_GL_DEPTH_STENCIL;
> +            }

Is there a case where we don't hit any of the above? If not, this should probably have an 'unreachable' assert and return false.

@@ +1963,5 @@
> +                    return format == LOCAL_GL_DEPTH_STENCIL;
> +            }
> +        }
> +
> +        return true;

Likewise, is this reachable? Even for an unspecified attachment (which I'm not a fan of having an object for, that's beside the point), we shouldn't hit this since we should either have a null RectangleObject, or a rect with 0 width and height.

@@ +2212,4 @@
>          mContext->MakeContextCurrent();
>  
> +        WebGLenum status;
> +        mContext->CheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER, &status);

Why change to this from gl->fCheckFramebufferStatus?

::: content/canvas/src/WebGLContextGL.cpp
@@ +868,5 @@
>              || y >= framebufferHeight
>              || y+height <= 0)
>          {
>              // we are completely outside of range, can exit now with buffer filled with zeros
> +            return DummyFramebufferOperation(info);

Hah, I like this. I see below you also added this to our early-out for readPixels; does it also need to go in the early-out for texSubImage2d?

@@ +954,5 @@
>          return ErrorInvalidOperation("copyTexImage2D: texture format requires an alpha channel "
>                                       "but the framebuffer doesn't have one");
>  
> +    if (mBoundFramebuffer)
> +        if (!mBoundFramebuffer->CheckAndInitializeRenderbuffers())

What's wrong with the old
'if (mBoundFramebuffer && !mBoundFramebuffer->CheckAndInitializeRenderbuffers())'?
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-01-20 12:50:04 PST
(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 578849 [details] [diff] [review]
> fix framebuffer errors
> 
> Review of attachment 578849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1960,5 @@
> > +                case LOCAL_GL_STENCIL_ATTACHMENT:
> > +                    return format == LOCAL_GL_STENCIL_INDEX8;
> > +                case LOCAL_GL_DEPTH_STENCIL_ATTACHMENT:
> > +                    return format == LOCAL_GL_DEPTH_STENCIL;
> > +            }
> 
> Is there a case where we don't hit any of the above? If not, this should
> probably have an 'unreachable' assert and return false.

Good point, fixed in coming patch.


> 
> @@ +1963,5 @@
> > +                    return format == LOCAL_GL_DEPTH_STENCIL;
> > +            }
> > +        }
> > +
> > +        return true;
> 
> Likewise, is this reachable? Even for an unspecified attachment (which I'm
> not a fan of having an object for, that's beside the point), we shouldn't
> hit this since we should either have a null RectangleObject, or a rect with
> 0 width and height.

Same.

> 
> @@ +2212,4 @@
> >          mContext->MakeContextCurrent();
> >  
> > +        WebGLenum status;
> > +        mContext->CheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER, &status);
> 
> Why change to this from gl->fCheckFramebufferStatus?

WebGLContext::CheckFramebufferStatus implements WebGL's own framebuffer completeness rules, which are different from OpenGL ES 2's. It does call gl->fCheckFramebufferStatus if it didn't find a webgl-specific rule indicating that the status is non-complete.

> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +868,5 @@
> >              || y >= framebufferHeight
> >              || y+height <= 0)
> >          {
> >              // we are completely outside of range, can exit now with buffer filled with zeros
> > +            return DummyFramebufferOperation(info);
> 
> Hah, I like this. I see below you also added this to our early-out for
> readPixels; does it also need to go in the early-out for texSubImage2d?

We only do this for framebuffer operations. texSubImage2d is not a framebuffer operation :) copyTexSubImage2d is, though, and it does use DummyFramebufferOperation.


> 
> @@ +954,5 @@
> >          return ErrorInvalidOperation("copyTexImage2D: texture format requires an alpha channel "
> >                                       "but the framebuffer doesn't have one");
> >  
> > +    if (mBoundFramebuffer)
> > +        if (!mBoundFramebuffer->CheckAndInitializeRenderbuffers())
> 
> What's wrong with the old
> 'if (mBoundFramebuffer &&
> !mBoundFramebuffer->CheckAndInitializeRenderbuffers())'?

Nothing? I guess it felt more natural to me this way as the second if() is doing an action.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-01-20 12:50:43 PST
Created attachment 590305 [details] [diff] [review]
fix framebuffer errors
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-01-24 13:16:37 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/a4e932fb3c3d

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