Add function to assert our shadowed state is correct

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-internal)

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch assert-state (obsolete) — Splinter Review
Attachment #8415674 - Flags: review?(dglastonbury)
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).
Attachment #8415674 - Flags: review?(dglastonbury) → review+
(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.
Posted patch assert-state (obsolete) — Splinter Review
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)
Attachment #8415674 - Attachment is obsolete: true
Attachment #8420214 - Flags: review?(dglastonbury)
Flags: needinfo?(jgilbert)
Whiteboard: webgl-internal
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.
Attachment #8420214 - Flags: review?(dglastonbury) → review+
Posted patch patchSplinter Review
Attachment #8426493 - Flags: review?(dglastonbury)
Attachment #8420214 - Attachment is obsolete: true
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.
Blocks: 1014207
(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 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?
Attachment #8426493 - Flags: review?(dglastonbury) → review+
(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.
Oops, I meant bug 1014207.
(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)
https://hg.mozilla.org/mozilla-central/rev/455d142f3a73
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1015561
You need to log in before you can comment on or make changes to this bug.