Closed
Bug 1015561
Opened 10 years ago
Closed 5 years ago
"Assertion failure: false (Bad cached value.)"
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox65 | --- | unaffected |
People
(Reporter: jruderman, Assigned: wlitwinczyk)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, regression, testcase)
Attachments
(6 files, 10 obsolete files)
Assertion failure: false (Bad cached value.), at content/canvas/src/WebGLContextUtils.cpp:423 The assertion added in bug 1004309 caught something!
Reporter | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → wlitwinczyk
Assignee | ||
Comment 2•10 years ago
|
||
Updated crash log, OSX
Assignee | ||
Comment 3•10 years ago
|
||
Not sure if we want to do this clamping in another area or not. For example, clamp inside StencilFuncSeparate() and save the clamped value as mStencilRef[Back|Front]. Page 95: http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf says: > Stencil comparison operations and queries of ref > clamp its value to the range [0, 2^s - 1] where s > is the number of bits in the stencil buffer attached > to the framebuffer. More patches for new tests to come.
Attachment #8485986 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8485986 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Ugh, turns out that this breaks on Linux now because those drivers don't mask the return value with the stencil bits.
Assignee | ||
Comment 5•10 years ago
|
||
This test currently catches the assertion bug. Using the setTimeout() function is crucial because without it everything works fine...
Attachment #8486761 -
Flags: review?(jgilbert)
Comment 6•10 years ago
|
||
Comment on attachment 8486761 [details] [diff] [review] stencil_cache_mochitests_patch Review of attachment 8486761 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html @@ +9,5 @@ > + <script src="driver-info.js"></script> > + </head> > + <body onload="runTests();"> > + <script> > + "use strict"; Don't indent the contents of the script tag, even if you indent the rest of the HTML tree.
Attachment #8486761 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Standalone test case
Assignee | ||
Comment 8•10 years ago
|
||
So other platforms are actually pretty well behaved, Linux isn't. Try: https://tbpl.mozilla.org/?tree=Try&rev=0c5c612247ae It looks like I need more conditional checking. I know I can get vendor + renderer from GLContext, but how can I get what OS I'm running?
Assignee | ||
Comment 9•10 years ago
|
||
Eh, I'm not too happy about it. You probably have a better idea of how to work around these problems. getParameter(STENCIL_BITS) doesn't work correctly as the attachments aren't bound to the underlying GL context until FinalizeAttachments() is called. This leads to binding a stencil buffer and then getting 0 stencil bits. The stencil ref/back ref code is kind of ugly.
Attachment #8485986 -
Attachment is obsolete: true
Attachment #8494065 -
Flags: review?(jgilbert)
Assignee | ||
Comment 10•10 years ago
|
||
Added more test cases. I think I covered enough of the combinations of framebuffers and attachments. Although, I didn't test with textures, only renderbuffers.
Attachment #8486761 -
Attachment is obsolete: true
Attachment #8494066 -
Flags: review?(jgilbert)
Assignee | ||
Comment 11•10 years ago
|
||
Updated solo test
Attachment #8489662 -
Attachment is obsolete: true
Comment 12•10 years ago
|
||
Comment on attachment 8494065 [details] [diff] [review] stencil_ref_patch Review of attachment 8494065 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for sending this back, but I think we should really have the same code to handle getting the number of stencil bits in both the query itself, and for masking in our workaround. ::: dom/canvas/WebGLContextState.cpp @@ +244,5 @@ > + case LOCAL_GL_STENCIL_REF: > + case LOCAL_GL_STENCIL_BACK_REF: { > + if (mBoundFramebuffer && mBoundFramebuffer->HasDepthStencilConflict()) { > + // Error, we don't know which stencil buffer's bits to use > + ErrorInvalidOperation("getParameter: framebuffer has two stencil buffers bound"); The best error here is probably INVALID_FRAMEBUFFER_OPERATION. @@ +250,5 @@ > + } > + > + // Assuming stencils have 8 bits > + GLint stencilMask = (1 << 8) - 1; > + const bool noStencilBound = mBoundFramebuffer && This is probably easier to follow as: if bound framebuffer: if stencil/depthStencil: stencilBits = 8 else: if options.stencil: stencilBits = 8 @@ +272,5 @@ > + { > + mBoundFramebuffer->FinalizeAttachments(); > + } > + > + gl->fGetIntegerv(pname, &i); Actually, pull this out into some GetStencilBits function, and use it both here and above in STENCIL_{BACK_}REF. ::: dom/canvas/WebGLContextUtils.cpp @@ +643,5 @@ > AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_CLEAR_VALUE, mStencilClearValue); > > + const GLRenderer renderer = gl->Renderer(); > + > + bool stencilRefNeedsWorkaround = renderer == GLRenderer::GalliumLlvmpipe; This is backwards. A good implementation should do this masking for us. We should workaround on platforms that *don't* mask. (Specifically, the only workaround we need is to do the masking on platforms which don't do it for us already)
Attachment #8494065 -
Flags: review?(jgilbert) → review-
Comment 13•10 years ago
|
||
Comment on attachment 8494066 [details] [diff] [review] stencil_cache_mochitests_patch Review of attachment 8494066 [details] [diff] [review]: ----------------------------------------------------------------- Let's do another round. ::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html @@ +8,5 @@ > + <script src="webgl-util.js"></script> > + <script src="driver-info.js"></script> > + </head> > + <body onload="runTests();"> > + <script type="text/javascript"> Technically, it would follow that this tag is indented one more time, since it's in the <body> scope. @@ +9,5 @@ > + <script src="driver-info.js"></script> > + </head> > + <body onload="runTests();"> > + <script type="text/javascript"> > + "use strict"; Don't indent the contents of the script tag in a file which is mostly script tag. @@ +76,5 @@ > + ok(gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE, > + 'Framebuffer should be incomplete'); > + } > + > + if (shouldHaveStencilBits) { If shouldError, then we shouldn't test the result of gl.getParameter. @@ +162,5 @@ > + document.body.appendChild(c); > + gl = c.getContext('experimental-webgl', {stencil:useStencil}); > + ok(gl, 'Expected WebGL creation to succeed.'); > + > + var haveStencil = gl.getContextAttributes()['stencil']; Sure, though you can just do `gl.getContextAttributes().stencil`. @@ +167,5 @@ > + > + var stencilBits = gl.getParameter(gl.STENCIL_BITS); > + if (haveStencil) { > + ok(stencilBits > 0, 'If stencil creation succeeded, there should be > 0 stencil bits, got ' + stencilBits + '.'); > + checkStencil(stencilBits, false); Shouldn't this be checkStencil(stencilBits, true)? @@ +177,5 @@ > + } > + } > + > + function runTests() { > + // Need to use setTimeout. Without it Firefox works fine. You actually want to test both with and without. It exercises different codepaths because of how we switch from readback ShSurf_Basic surfaces to some accelerated backend. @@ +182,5 @@ > + function testTrueFalse(testFunc, next) { > + return function() { > + setTimeout(function() { > + testFunc(); > + gl.clear(gl.COLOR_BUFFER_BIT); A more sure-fire way to reset the canvas is to do: gl.canvas.width += 1; gl.canvas.width -= 1; This triggers a canvas resize (well, two of them), which will return the canvas backbuffer to its original state. @@ +205,5 @@ > + if (lst.length == 0) return function() { SimpleTest.finish(); } > + > + var testFunc = lst[0]; > + var smaller_list = lst.splice(1, lst.length-1); > + return testTrueFalse(testFunc, buildTest(smaller_list)); This recursive function building is hard to follow. Can't we just do this iteratively?
Attachment #8494066 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8494065 -
Attachment is obsolete: true
Attachment #8494754 -
Flags: review?(jgilbert)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8494066 -
Attachment is obsolete: true
Attachment #8494755 -
Flags: review?(jgilbert)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8494067 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
Comment on attachment 8494754 [details] [diff] [review] stencil_ref_patch Review of attachment 8494754 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContextState.cpp @@ +70,5 @@ > return JS::StringValue(str); > } > > +bool > +WebGLContext::GetStencilBits(GLint& stencilBits) Use pointers for out-params: bool Foo(GLint* out_stencilBits) ::: dom/canvas/WebGLContextUtils.cpp @@ +663,5 @@ > + const GLuint stencilRefMask = (1 << stencilBits) - 1; > + > + // Good platforms return masked values, but our cached values are not masked > + AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_REF, mStencilRefFront & stencilRefMask); > + AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_BACK_REF, mStencilRefBack & stencilRefMask); Let's do AssertMaskedUintParamCorrect(gl, enum, mask, value). This will work regardless of whether the underlying implementation gets it right.
Attachment #8494754 -
Flags: review?(jgilbert) → review-
Comment 18•10 years ago
|
||
Comment on attachment 8494755 [details] [diff] [review] stencil_cache_mochitests_patch Review of attachment 8494755 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html @@ +201,5 @@ > + gl.canvas.width -= 1; > + } > + } > + > + function runTestsWithSetTimeout(curIndex) { Instead of chaining these setTimeouts together, you should be fine just delaying with requestAnimationFrame once, and then rerun all the tests. There shouldn't be any difference between doing that and having a setTimeout between each test.
Attachment #8494755 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8494754 -
Attachment is obsolete: true
Attachment #8494850 -
Flags: review?(jgilbert)
Assignee | ||
Comment 20•10 years ago
|
||
Changed to use requestAnimationFrame instead
Attachment #8494755 -
Attachment is obsolete: true
Attachment #8494852 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8494852 -
Flags: review?(jgilbert) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8494850 [details] [diff] [review] stencil_ref_patch Review of attachment 8494850 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContext.h @@ +823,5 @@ > // State and State Requests (WebGLContextState.cpp) > public: > void Disable(GLenum cap); > void Enable(GLenum cap); > + bool GetStencilBits(GLint* stencilBits); Please use the `out_` prefix, so it's clear how this will be used.
Attachment #8494850 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 22•10 years ago
|
||
I don't think this was changed, but uploading because I'm leaving
Attachment #8494850 -
Attachment is obsolete: true
Attachment #8496282 -
Flags: review?(jgilbert)
Assignee | ||
Comment 23•10 years ago
|
||
Fixed mochitest. Skip running on OSX 10.6 due to driver bug with stencil ref, and skip B2G linux because the driver is blacklisted from creating a context. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=54002796830c
Attachment #8494852 -
Attachment is obsolete: true
Attachment #8496285 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8496282 -
Flags: review?(jgilbert) → review+
Updated•10 years ago
|
Keywords: leave-open
Updated•10 years ago
|
Attachment #8496285 -
Flags: review?(jgilbert) → review+
Comment 27•9 years ago
|
||
jgilbert: I hit this "Bad cached value" assertion failure when running this WebGL fuzzer on a debug build. Is this a different bug? https://bug614678.bugzilla.mozilla.org/attachment.cgi?id=493971 Assertion failure: false (Bad cached value.), at dom/canvas/WebGLContextUtils.cpp:991 #01: mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b91780] #02: mozilla::WebGLContext::ClearScreen()[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b90be3] #03: mozilla::WebGLContext::ClearBackbufferIfNeeded()[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b90a9c] #04: mozilla::WebGLContext::DrawArrays_check(int, int, int, char const*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b967c3] #05: mozilla::WebGLContext::DrawArrays(unsigned int, int, int)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b975be] #06: mozilla::dom::WebGLRenderingContextBinding::drawArrays(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, JSJitMethodCallArgs const&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ac17a5] #07: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b53982] #08: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d1143c] #09: Interpret(JSContext*, js::RunState&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d2e291] #10: js::RunScript(JSContext*, js::RunState&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d21f55] #11: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d38baa] #12: EvalKernel(JSContext*, JS::CallArgs const&, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3658797] #13: js::DirectEval(JSContext*, JS::CallArgs const&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3658fc3] #14: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3833b99] Segmentation fault: 11
Flags: needinfo?(jgilbert)
Updated•8 years ago
|
Assignee: wlitwinczyk → nobody
Comment 28•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months. :jgilbert, maybe it's time to close this bug?
Flags: needinfo?(jgilbert)
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jgilbert)
Resolution: --- → FIXED
Updated•5 years ago
|
Assignee: nobody → wlitwinczyk
Updated•5 years ago
|
Keywords: leave-open
Updated•5 years ago
|
status-firefox65:
--- → unaffected
status-firefox-esr60:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•