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

(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.

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

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

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

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 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.

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

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

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.

(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?

(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/integration/mozilla-inbound/rev/455d142f3a73