User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.8 Safari/536.11 Steps to reproduce: Run the conformance test for preserve drawing buffer on context. https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-attribute-preserve-drawing-buffer.html Actual results: The second test failed "FAIL Did not render ok with preserveDrawingBuffer false." Expected results: "PASS Rendered ok with preserveDrawingBuffer false."

Tested on Ubuntu with native nv driver 170 and gtx-460, bug present.

Since the testcase is already part of the conformance tests, we can't forget about that.

FWIW, on Win7 + recent Trunk I get that Failure despite of using ANGLE or native (AMD 4850, Driver 8.961.3.0); in Chrome Canary both Modes are WFM.

Someone should update that test, since clearly the 'working' top left row doesn't match the 'control' top right row.

Implement Dirty semantics for WebGL contexts. Note that we can't quite use the existing Invalidation semantics, since MakeContextClean is called in SurfaceFromElement. I'm not entirely clear why this is, but I'm not entirely clear on how Layers/Canvas uses these Invalidation mechanics.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=070b8e66090d

Forgot a MakeCurrent(). Good thing we assert for this now. :)

New try run: https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=cf6dff74d63a

Comment on attachment 656746 [details] [diff] [review] patch2 Review of attachment 656746 [details] [diff] [review]: ----------------------------------------------------------------- I see. This is a cleaner approach than the previous one, as clearing immediately after presenting allows to do without the tri-state and the 2nd-clear optimization is still there. I would like a big explicit explanatory comment on how there are two different "dirty" notions there corresponding to being presented to the compositor and being read in other ways. Maybe the identifier names could be changed to make that more explicit, too. ::: content/canvas/src/WebGLContext.h @@ +534,5 @@ > already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder, > CanvasLayer *aOldLayer, > LayerManager *aManager); > + > + // A note that this uses 'clean' without having anything to do with mIsScreenDirty. Placeholder @@ +542,5 @@ > + // after it's been presented to the compositor. This function does that if needed. > + // See section 2.2 in the WebGL spec. > + // Do not call this for functions like Render(), since this will often clear the screen. > + // 'Dirty' here doesn't mean we can't use it, just that it's been updated since being cleared. > + void MakeScreenClean(); I'm uncomfortable about Make being too close to Mark, which is easy to misread. @@ +550,5 @@ > uint32_t Generation() { return mGeneration.value(); } > > const WebGLRectangleObject *FramebufferRectangleObject() const; > > // this is similar to GLContext::ClearSafely, but is more comprehensive Isn't this comment out of date now? I thought ClearSafely was comprehensive now. ::: content/canvas/src/WebGLContextGL.cpp @@ +240,5 @@ > > MakeContextCurrent(); > > if (!wfb) { > + gl->fBindFramebuffer(target, 0); So does GetOffscreenFBO still have a purpose?

(In reply to Benoit Jacob [:bjacob] from comment #9) > Comment on attachment 656746 [details] [diff] [review] > patch2 > > Review of attachment 656746 [details] [diff] [review]: > ----------------------------------------------------------------- > > I see. This is a cleaner approach than the previous one, as clearing > immediately after presenting allows to do without the tri-state and the > 2nd-clear optimization is still there. > > I would like a big explicit explanatory comment on how there are two > different "dirty" notions there corresponding to being presented to the > compositor and being read in other ways. Maybe the identifier names could be > changed to make that more explicit, too. Absolutely. > > ::: content/canvas/src/WebGLContext.h > @@ +534,5 @@ > > already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder, > > CanvasLayer *aOldLayer, > > LayerManager *aManager); > > + > > + // A note that this uses 'clean' without having anything to do with mIsScreenDirty. > > Placeholder 'Placeholder'? > > @@ +542,5 @@ > > + // after it's been presented to the compositor. This function does that if needed. > > + // See section 2.2 in the WebGL spec. > > + // Do not call this for functions like Render(), since this will often clear the screen. > > + // 'Dirty' here doesn't mean we can't use it, just that it's been updated since being cleared. > > + void MakeScreenClean(); > > I'm uncomfortable about Make being too close to Mark, which is easy to > misread. Yeah, I noticed this myself. Semantically, 'mark' should not do much besides making a note of something, whereas 'make' strongly implies that work is done. I think the clearest approach here is to use 'set' instead of 'mark', and continue to use 'make'. I am having trouble thinking of a good synonym to 'make' in this context. > @@ +550,5 @@ > > uint32_t Generation() { return mGeneration.value(); } > > > > const WebGLRectangleObject *FramebufferRectangleObject() const; > > > > // this is similar to GLContext::ClearSafely, but is more comprehensive > > Isn't this comment out of date now? I thought ClearSafely was comprehensive > now. Yes, these two functions should be functionally identical. However, ClearSafely unilaterally clears depth and stencil buffers, whereas here we avoid clearing those if we don't have them, since not clearing them saves us some state-thrashing. > ::: content/canvas/src/WebGLContextGL.cpp > @@ +240,5 @@ > > > > MakeContextCurrent(); > > > > if (!wfb) { > > + gl->fBindFramebuffer(target, 0); > > So does GetOffscreenFBO still have a purpose? No, it's returned 0 for months now. I can file a clean-up bug.

One more time around. I changed some things about ForceClearFramebufferWithDefaultValues. I removed the unused arg, and changed it to only set bindings if it needs to. Since we no longer always set bindings to what we need, I added a DEBUG block that asserts that everything's set properly. Also, it looks like we can't change 'MarkContextClean' to 'Set*' or something else, since 'MarkContextClean' comes down to us through inheritance.

Comment on attachment 661003 [details] [diff] [review] patch3 Review of attachment 661003 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContext.cpp @@ +1193,5 @@ > + > + MOZ_ASSERT(!ditherEnabled); > + MOZ_ASSERT(!scissorTestEnabled); > + > + if (initializeColorBuffer) { Why this if? It wouldn't hurt to check this unconditionally, so that a bug in the value if initializeColorBuffer would be caught. @@ +1210,5 @@ > + colorClearValue[2] == 0.0f && > + colorClearValue[3] == 0.0f); > + } > + > + if (initializeDepthBuffer) { Same @@ +1223,3 @@ > } > > if (initializeStencilBuffer) { Same ::: content/canvas/src/WebGLContext.h @@ +539,5 @@ > + * 1) 'ContextClean', which is inherited by canvas drawing contexts to denote > + * their invalidation state. > + * 2) 'ScreenClean', which is whether the webgl 'screen' has been cleared > + * to blank. > + */ Nice @@ +541,5 @@ > + * 2) 'ScreenClean', which is whether the webgl 'screen' has been cleared > + * to blank. > + */ > + > + // A note that this uses 'clean' without having anything to do with mIsScreenDirty. This comment still looks non-final. Remove it? (Talking about the // line here)

(In reply to Benoit Jacob [:bjacob] from comment #12) > Comment on attachment 661003 [details] [diff] [review] > patch3 > > Review of attachment 661003 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContext.cpp > @@ +1193,5 @@ > > + > > + MOZ_ASSERT(!ditherEnabled); > > + MOZ_ASSERT(!scissorTestEnabled); > > + > > + if (initializeColorBuffer) { > > Why this if? It wouldn't hurt to check this unconditionally, so that a bug > in the value if initializeColorBuffer would be caught. We can't check this unconditionally, since if we don't have a depth buffer (or rather, we're not supposed to be clearing depth, for whatever reason), we should not bother to perturb the depth clear states. In turn, we should not then expect them to have any particular values. > ::: content/canvas/src/WebGLContext.h > > @@ +541,5 @@ > > + * 2) 'ScreenClean', which is whether the webgl 'screen' has been cleared > > + * to blank. > > + */ > > + > > + // A note that this uses 'clean' without having anything to do with mIsScreenDirty. > > This comment still looks non-final. Remove it? (Talking about the // line > here) Removed. Running it through Try since to check that our new asserts don't trip: https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=45a775f4e37c

Build errors in the previous (forgot some |gl->|s), so here's a new one: https://tbpl.mozilla.org/?tree=Try&rev=983a5445774b

Reftest errors! \o/ With this patch we're failing to draw properly with webgl and preserveDrawingBuffer=false on Win7 and Android. Mac looks fine, which seems strange. WinXP is also unaffected. My guess is that we're doing something with invalidation incorrectly. preserveDrawingBuffer=false is tricky, because after we present our buffer, we clear it. If we try to present again, we will publish a blank canvas. We should not be presenting again unless a draw command has run on the default framebuffer.

From bug 788026 comment 17 by mattwoodrow: > Since we now draw thebes layers, and composite them to the screen in separate > steps, it's possible for us to composite twice without drawing in between. > > This causes issues with WebGL, since we clear the texture when we composite. If we're invalidating the canvas too many times (more often than we present), then extra invalidations will pull the newly cleared canvas. This is inviable. The good solution to this is to multibuffer the backing surfaces. We can also hack it together to appear to work properly by not actually clearing the canvas until webgl draws to it. We then need to fake readback after the fake clear to not actually readPixels (since that would show that we haven't actually cleared yet), but instead return what the readPixels should return if we had actually cleared already.

Blocks a blocker

(In reply to JP Rosevear [:jpr] from comment #17) > Blocks a blocker It's only blocked by the blocker. FWIW, this is a very important part of WebGL conformance that can heavily affect how WebGL apps run. As such, I'm leaving it blocking-basecamp=?.

Actually, I believe this test relies on undefined behavior: "If this flag is false, attempting to perform operations using this context as a source image after the rendering function has returned can lead to undefined behavior. This includes readPixels or toDataURL calls, or using this context as the source image of another context's texImage2D or drawImage call." I do not believe it is possible for us to satisfy this test if we are compositing the canvas more often than we are producing frames. The issues is that currently our 'present' and 'composite' steps are the same, and we can't currently composite if we don't present. That is, we can't present, then composite the same frame multiple times. What we need to support is presenting a frame in state A, then clearing the frame to state B. A recomposite will use state A, whereas the actual state (state B) is evident when using readPixels or similar. FWIW, Chrome appears to have a bug here also. Comment 4 here is incorrect. In this test, state A is red, and state B is black. With PDB:false, we should get [A,B,A], as shown on the right side. Chrome (and one of my patched versions of Firefox) show [A, B, B], which, while untestable at this level, shows that the canvas is displaying that it has been cleared when it should not.

I should note that this is indeed fixed by the Streaming Buffers stuff. We may be able to patch this to work properly without bug 716859 by copying the canvas to a Layers-internal buffer for use with recompositing.

I should note that one effect of comment 19 is that PDB:false would actually be slower than PDB:true for the trivial single-buffered system we have currently.

How are you doing on this bug, Jeff?

(In reply to Andrew Overholt [:overholt] from comment #22) > How are you doing on this bug, Jeff? A complete fix is not possible without bug 716859, so it has folded in to that.

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known P2 bugs found before or during C1".

Bug 716859 has been minused, maybe this should too?

(In reply to JP Rosevear [:jpr] from comment #17) > Blocks a blocker There seems to be a misunderstanding there, as pointed out by Jeff in Comment 18: (In reply to Jeff Gilbert [:jgilbert] from comment #18) > (In reply to JP Rosevear [:jpr] from comment #17) > > Blocks a blocker > > It's only blocked by the blocker. As Jeff points out in that same comment, it is still an important thing to work on: > FWIW, this is a very important part of > WebGL conformance that can heavily affect how WebGL apps run. As such, I'm > leaving it blocking-basecamp=?. However, it is not nearly important enough for B2G shippability to be a blocker. It only affects correct rendering of some WebGL pages. I don't know that it actually affects any known-to-be-important-to-B2G-shippability WebGL page or application. Please remove the blocking-basecamp? nom.

Thanks, not a blocker.

Bug is still present in Firefox 18.0 stable. Ubuntu 12.10, Driver Nvidia 304.64, GPU GTX-460): - FF: fails - Chrome: passes OSX 10.7.4, Driver ATI 7.18.18, GPU ATI Radeon HD 6750M - FF: fails - Chrome: passes Windows 7, Driver AMD 9.2.0.0, GPU AMD Radeon HD 6900 - FF: fails - Chrome: passes

This was fixed in bug 716859.