1004309 - Add function to assert our shadowed state is correct

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We do DEBUG-only shadowed-state checking in a number of places. We should just centralize our shadowed-state checks, and call an asserting function when we want to assure we're shadowed correctly.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: assert-state

Comment 2

[u480271]

Comment on attachment 8415674 [details] [diff] [review]
assert-state

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

::: content/canvas/src/WebGLContext.cpp
@@ +988,5 @@
>  
>      // Fun GL fact: No need to worry about the viewport here, glViewport is just
>      // setting up a coordinates transformation, it doesn't affect glClear at all.
>  
> +    AssertCachedState();

Q: Should we be asserting this in more locations?

::: content/canvas/src/WebGLContext.h
@@ +204,5 @@
>  
>      void DummyFramebufferOperation(const char *info);
>  
>      WebGLTexture *activeBoundTextureForTarget(GLenum target) const {
> +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_BINDING_2D);

Q: Seems like target should be LOCAL_GL_TEXTURE_2D or a cube map face. We have a helper to checked that?

::: content/canvas/src/WebGLContextUtils.cpp
@@ +246,5 @@
>      return error;
>  }
> +
> +static GLuint
> +GetUint(GLContext& gl, GLenum pname)

nit: I know what you intend by GLContext& (that a value must be passed), but the all the calls are GetUint(*gl, ...) which is bit ugly and different to the rest of WebGL code that deals with GLContext*.

@@ +260,5 @@
> +IsCacheCorrect(float cached, float actual)
> +{
> +    if (IsNaN(cached)) {
> +        // GL is allowed to do anything it wants for NaNs, so if we're shadowing
> +        // a NaN, then whatever `actual` is might be correct.

Q: When do we get NaNs in WebGL? (From memory I saw it in uninitialized clear colors)

@@ +294,5 @@
> +    MOZ_ASSERT_IF(IsWebGL2(),
> +                  gl->fIsEnabled(LOCAL_GL_RASTERIZER_DISCARD) == mRasterizerDiscardEnabled);
> +
> +
> +    realGLboolean colorWriteMask[4] = {2, 2, 2, 2};

Do these initial values have any particular meaning?

@@ +322,5 @@
> +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_VALUE_MASK) == mStencilWriteMaskBack);
> +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_WRITEMASK) == mStencilWriteMaskFront);
> +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_WRITEMASK) == mStencilWriteMaskBack);
> +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_REF) == mStencilRefFront);
> +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_REF) == mStencilRefBack);

[sugguestion]

How about:

    MOZ_ASSERT(IsCacheCorrect(gl, LOCAL_GL_STENCIL_BACK_REF, mStencilRefBack);

Clear to read what is going on. (ie. hide GetUint() inside the function).

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #2)
> Comment on attachment 8415674 [details] [diff] [review]
> assert-state
> 
> Review of attachment 8415674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +988,5 @@
> >  
> >      // Fun GL fact: No need to worry about the viewport here, glViewport is just
> >      // setting up a coordinates transformation, it doesn't affect glClear at all.
> >  
> > +    AssertCachedState();
> 
> Q: Should we be asserting this in more locations?
Probably, but we can do that as follow up. This patch merely keeps the status quo here.
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +204,5 @@
> >  
> >      void DummyFramebufferOperation(const char *info);
> >  
> >      WebGLTexture *activeBoundTextureForTarget(GLenum target) const {
> > +        MOZ_ASSERT(target != LOCAL_GL_TEXTURE_BINDING_2D);
> 
> Q: Seems like target should be LOCAL_GL_TEXTURE_2D or a cube map face. We
> have a helper to checked that?
We probably should.
> 
> ::: content/canvas/src/WebGLContextUtils.cpp
> @@ +246,5 @@
> >      return error;
> >  }
> > +
> > +static GLuint
> > +GetUint(GLContext& gl, GLenum pname)
> 
> nit: I know what you intend by GLContext& (that a value must be passed), but
> the all the calls are GetUint(*gl, ...) which is bit ugly and different to
> the rest of WebGL code that deals with GLContext*.
I would really like to use GLContext& throughout our code, when it's non-nullable.
I supposed I can roll this into another bug which does this conversion, and we can talk about it there.
> 
> @@ +260,5 @@
> > +IsCacheCorrect(float cached, float actual)
> > +{
> > +    if (IsNaN(cached)) {
> > +        // GL is allowed to do anything it wants for NaNs, so if we're shadowing
> > +        // a NaN, then whatever `actual` is might be correct.
> 
> Q: When do we get NaNs in WebGL? (From memory I saw it in uninitialized
> clear colors)
I believe apps can make calls with NaNs, so we need to deal with them properly.
> 
> @@ +294,5 @@
> > +    MOZ_ASSERT_IF(IsWebGL2(),
> > +                  gl->fIsEnabled(LOCAL_GL_RASTERIZER_DISCARD) == mRasterizerDiscardEnabled);
> > +
> > +
> > +    realGLboolean colorWriteMask[4] = {2, 2, 2, 2};
> 
> Do these initial values have any particular meaning?
They're out-of-bounds of what could be returned. In theory this makes it easy to tell if the call succeeded, but really, there's no reason this call shouldn't so, we should just init with zeros.
> 
> @@ +322,5 @@
> > +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_VALUE_MASK) == mStencilWriteMaskBack);
> > +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_WRITEMASK) == mStencilWriteMaskFront);
> > +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_WRITEMASK) == mStencilWriteMaskBack);
> > +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_REF) == mStencilRefFront);
> > +    MOZ_ASSERT(GetUint(*gl, LOCAL_GL_STENCIL_BACK_REF) == mStencilRefBack);
> 
> [sugguestion]
> 
> How about:
> 
>     MOZ_ASSERT(IsCacheCorrect(gl, LOCAL_GL_STENCIL_BACK_REF,
> mStencilRefBack);
> 
> Clear to read what is going on. (ie. hide GetUint() inside the function).
Yes, this is nicer.

Comment 4

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1efb117b969c

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf5f00fc6ac for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=38895095&tree=Mozilla-Inbound

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also seems to have broken some android mochitests: https://tbpl.mozilla.org/php/getParsedLog.php?id=38899793&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=38899591&tree=Mozilla-Inbound

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: assert-state

I had to change a couple things. One is we can't check bindings in all the same places we want to check state. (A somewhat fuzzy distinction, but we want to check state in our general 'clear the current FB' func, where we have another framebuffer bound that we want to get cleared. (so the framebuffer binding check asserts)

Comment 8

[u480271]

Comment on attachment 8420214 [details] [diff] [review]
assert-state

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

LGTM. Green on try?

::: content/canvas/src/WebGLContext.h
@@ +116,5 @@
>  };
>  
> +#ifdef DEBUG
> +static bool
> +IsTextureBinding(GLenum binding)

Double plus good.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Very green on Try: https://tbpl.mozilla.org/?tree=Try&rev=c7d40cc09d40

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc29fc7b91a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/060ce12574c8 for webgl bustage: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=39530105&tree=Mozilla-Inbound

Comment 12

[Kelsey Gilbert [:jgilbert]]

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

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 14

[Kelsey Gilbert [:jgilbert]]

Finally passing Try:
https://tbpl.mozilla.org/?tree=Try&rev=6bff5b8e4f30

The initial values for stencil WRITEMASK and VALUE_MASK are sorta tricky: all-ones, up to the number of stencil planes. This means for no stencil they should be 0, and for 8-bit stencil, they should be 0xff. Many implementations seem to default to UINT32_MAX.

Regardless, we cannot rely on the driver's defaults here, since we ignore surface we tell the driver to create, thus the initial values for these variables are garbage to us.

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Finally passing Try:
> https://tbpl.mozilla.org/?tree=Try&rev=6bff5b8e4f30
> 
> The initial values for stencil WRITEMASK and VALUE_MASK are sorta tricky:
> all-ones, up to the number of stencil planes. This means for no stencil they
> should be 0, and for 8-bit stencil, they should be 0xff. Many
> implementations seem to default to UINT32_MAX.
> 
> Regardless, we cannot rely on the driver's defaults here, since we ignore
> surface we tell the driver to create, thus the initial values for these
> variables are garbage to us.

Filed bug 1004309 for this.

Comment 16

[u480271]

Comment on attachment 8426493 [details] [diff] [review]
patch

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +1659,5 @@
> +    gl->GetUIntegerv(LOCAL_GL_STENCIL_BACK_VALUE_MASK, &mStencilValueMaskBack);
> +    gl->GetUIntegerv(LOCAL_GL_STENCIL_WRITEMASK, &mStencilWriteMaskFront);
> +    gl->GetUIntegerv(LOCAL_GL_STENCIL_BACK_WRITEMASK, &mStencilWriteMaskBack);
> +
> +    AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_VALUE_MASK,      mStencilValueMaskFront);

This is redundant, no?

Comment 17

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > Finally passing Try:
> > https://tbpl.mozilla.org/?tree=Try&rev=6bff5b8e4f30
> > 
> > The initial values for stencil WRITEMASK and VALUE_MASK are sorta tricky:
> > all-ones, up to the number of stencil planes. This means for no stencil they
> > should be 0, and for 8-bit stencil, they should be 0xff. Many
> > implementations seem to default to UINT32_MAX.
> > 
> > Regardless, we cannot rely on the driver's defaults here, since we ignore
> > surface we tell the driver to create, thus the initial values for these
> > variables are garbage to us.
> 
> Filed bug 1004309 for this.

You have a circular reference here. *This* is bug 1004309.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Oops, I meant bug 1014207.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> Comment on attachment 8426493 [details] [diff] [review]
> patch
> 
> Review of attachment 8426493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +1659,5 @@
> > +    gl->GetUIntegerv(LOCAL_GL_STENCIL_BACK_VALUE_MASK, &mStencilValueMaskBack);
> > +    gl->GetUIntegerv(LOCAL_GL_STENCIL_WRITEMASK, &mStencilWriteMaskFront);
> > +    gl->GetUIntegerv(LOCAL_GL_STENCIL_BACK_WRITEMASK, &mStencilWriteMaskBack);
> > +
> > +    AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_VALUE_MASK,      mStencilValueMaskFront);
> 
> This is redundant, no?

Yeah. I'm inclined to leave it, since it:
1. Makes sure we retrieved the values correctly. (I messed up the pnames in at least one previous patch)
2. Gives us a non-debug use of AssertUintParamCorrect, which otherwise would cause builds to fail unless I #ifdef DEBUG the decl off. (thanks, warnings-as-errors)

Comment 20

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/455d142f3a73

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/455d142f3a73