707460 - Fix WebGL framebuffer statuses and errors

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: 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)]

https://tbpl.mozilla.org/?tree=Try&rev=76fa2c4d0534
 (apparently tryserver currently has a problem making debug fail)

Comment 2

[Kelsey Gilbert [:jgilbert]]

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)]

(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)]

Attachment: fix framebuffer errors

Comment 5

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

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

Comment 6

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

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