[Chromium] Do not force draws if prepareToDraw() fails
Created attachment 134702 [details] Patch
In short, this restricts forceDraw to override all conditions in shouldDraw(), so we will transition to attempt a draw as before. But something might be really wrong at the actual draw time (indicated by CCLayerTreeHostImpl::prepareToDraw() failing). Here I think we should not override with force draw. Instead skip the SwapBuffers() and do everything else such as signaling completion. The specific failure on Android (flashes of corruption with the root surface clear color) comes from this race: - new WebView created (visible by default) and it creates an empty document - kForceCompositing early on enters compositing aggressively and creates an empty root layer
Comment on attachment 134702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134702&action=review Nice work :) Nat would you know if this is going to interact strangely with thumbnailing? I'm less familiar with that. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:367 > + if (!rootLayer() || rootLayer()->bounds().isEmpty()) Can this condition go in canDraw() and make this just call canDraw()? It'll let us prevent attempting non-forced frames at all when root layer is empty, and keep these conditions more in one place. > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:829 > +class CCLayerTreeHostTestEmptyContentsShouldNotDraw : public CCLayerTreeHostTestThreadOnly { Do we have a test to verify that a forced draw will not deadlock if the root layer is empty/nonvisible/notpresent? > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:861 > + endTest(); Should this be done after coming back to didCommitAndDrawFrame() again to make sure we don't skip the finishAllRendering attempt?
Woah, this feels like a pretty sweeping change and not at all what I think you wanted to do. Or maybe i'm not following your logic here... all I see is you deleting code that causes us to draw in situations where we need to draw. ForceDraw is meant to be a state you use when you NEED a draw when you're invisible, and need to draw. E.g. if the thumbnailer is demanding a compositeAndReadback. Usually we'd decline to paint because we're invisible. You set forceDraw to make that happen. Your description of the actual bug happening is a little confusing to me. Can you elaborate on it, and explain why removing force draw helps?
Oh I think the problem here is only that prepareToDraw() can return false in two different cases and this patch needs more granularity. I got caught up in thinking about the canDraw() type states and forgot about the fact that prepareToDraw() returns false for other reasons too when the root layer is valid. I think we just need a canDraw() check on the forced draw path. And we don't do the draw/swap if canDraw() is false or prepareToDraw() is false.
(In reply to comment #4) > Your description of the actual bug happening is a little confusing to me. Can you elaborate on it, and explain why removing force draw helps? Nat problem is forced draw with empty-bound root layer equals big empty framebuffer that pretends to have contents but does not, and this causes a flash until the real contents happen. We need to prevent the draw/swap when there is no valid thing to draw, ie canDraw() is false, extended to check for rootLayer being non-empty.
Ah. Hmm.... Is it the draw that we're trying to prevent, the swap, or both? Maybe the thing I'm missing is that we need forcedDraw to get the state machine to call those deferred thinsg like compositeAndReadback/FinishAllRendering, but that we dont actually need to draw for either of those actual states. The forcedDraw is really forceAScheduledActionDrawAndSwap ,not a force-a-picture-onscreen, right? Can we take this opportunity to just ditch canDraw and put it into the prepareToDraw state? Or is that still not possible?
(In reply to comment #7) > Ah. Hmm.... Is it the draw that we're trying to prevent, the swap, or both? > > Maybe the thing I'm missing is that we need forcedDraw to get the state machine to call those deferred thinsg like compositeAndReadback/FinishAllRendering, but that we dont actually need to draw for either of those actual states. The forcedDraw is really forceAScheduledActionDrawAndSwap ,not a force-a-picture-onscreen, right? > > Can we take this opportunity to just ditch canDraw and put it into the prepareToDraw state? Or is that still not possible? I think that makes it worse. prepareToDraw() == false shouldn't prevent the forced draw *if* it was because of animations. But it should if it was because of canDraw() type things. If prepareToDraw() returns an enum instead then we could do that here yes. But it would also change the next state from the state machine in these situations, and it would be attempting to draw constantly while there is nothing to draw, I assume that's okay?
(In reply to comment #8) > (In reply to comment #7) > > Ah. Hmm.... Is it the draw that we're trying to prevent, the swap, or both? It's the swap. All the draw would be is a glClear(), which could be avoided while we're at it though.
(In reply to comment #3) > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:367 > > + if (!rootLayer() || rootLayer()->bounds().isEmpty()) > > Can this condition go in canDraw() and make this just call canDraw()? It'll let us prevent attempting non-forced frames at all when root layer is empty, and keep these conditions more in one place. > Will do. > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:829 > > +class CCLayerTreeHostTestEmptyContentsShouldNotDraw : public CCLayerTreeHostTestThreadOnly { > > Do we have a test to verify that a forced draw will not deadlock if the root layer is empty/nonvisible/notpresent? > I think CCLayerTreeHostTestCompositeAndReadbackWhileInvisible and CCLayerTreeHostTestSetRepeatedLostContext cover this. They both block for all rendering to complete. > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:861 > > + endTest(); > > Should this be done after coming back to didCommitAndDrawFrame() again to make sure we don't skip the finishAllRendering attempt? But isn't this fine since finishAllRendering() is blocking?
(In reply to comment #7) > Ah. Hmm.... Is it the draw that we're trying to prevent, the swap, or both? > Mainly the swap(), but maybe also fall into the existing logic for the below? if (drawFrame) m_layerTreeHostImpl->drawLayers(frame); Regardless, I'm not trying to change the logic for when *scheduledActionDrawAndSwapInternal()* gets called at all with this patch, which I think is what 'force draw' originally did, but maybe now it became a but too aggressive in forcing. > Maybe the thing I'm missing is that we need forcedDraw to get the state machine to call those deferred thinsg like compositeAndReadback/FinishAllRendering, but that we dont actually need to draw for either of those actual states. The forcedDraw is really forceAScheduledActionDrawAndSwap ,not a force-a-picture-onscreen, right? > Yes, that's basically what I'm trying to achieve. It should always force to try and draw and it should always signal completion. But if there is nothing to draw simply skip drawLayers() and swapBuffers(). I thought prepareToDraw() returning false would exactly cover the latter, but as Dana indicates there is a case I'm breaking (with animations?)? > Can we take this opportunity to just ditch canDraw and put it into the prepareToDraw state? Or is that still not possible?
I think at the heart of my patch is this: CCThreadProxy::scheduledActionDrawAndSwapInternal() ... bool drawFrame = m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw; // remove '|| forcedDraw' Note that (before and after my patch) the function still signals completion on such regardless of drawFrame. Plus also treating rootLayer.ContentBounds().isEmpty similar to !rootLayer at the actual draw time. The rest is removing the FORCED_DRAW state as it seems its obsolete. I think the important logic (which also originally did not require an extra state) is to allow forcing transitions to try to draw even when invisible etc.
I super apologize, I had an epic brain fart in our discussion and forgot about the animation checkerboarding. When we are going to to be unable to draw an animating layer, we have prepareToDraw() return false. (In reply to comment #10) > > Should this be done after coming back to didCommitAndDrawFrame() again to make sure we don't skip the finishAllRendering attempt? > > > But isn't this fine since finishAllRendering() is blocking? I was thinking that we setNeedsForcedRedraw() through there, but want to make sure the test has a chance to hit the FAIL() in case it is doing the wrong thing? (In reply to comment #12) > I think at the heart of my patch is this: > > CCThreadProxy::scheduledActionDrawAndSwapInternal() > ... > bool drawFrame = m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw; // remove '|| forcedDraw' > The rest is removing the FORCED_DRAW state as it seems its obsolete. I think the important logic (which also originally did not require an extra state) is to allow forcing transitions to try to draw even when invisible etc. I think we need the forcedDraw still for thumbnailing. It forces us to draw a frame even though animation is resisting our attempts (!prepareToDraw()). Here's what I think our situation looks like. 1. REDRAW_IF_POSSIBLE a) Should draw and swap if canDraw() && prepareToDraw() b) Should not draw or swap otherwise. 2. REDRAW_FORCED a) If canDraw() && prepareToDraw() then draw and swap. b) If !canDraw() then we get garbage anyways so don't draw or swap (thumbnailer won't mind yes?) c) If canDraw() && !prepareToDraw() ignore it and draw anyways and swap. Nat you suggested in chat that for 2b) just draw anyways cuz it's overoptimization. But then we have to worry about partial swaps and if we should swap the bad contents. It seems easier and just as valid to just not draw/swap in that case?
I'd like to keep force draw. If we can remove it after the fact as a cleanup patch, thats fine --- but we may end up wanting to cherry pick this code to M19 and if we do, I'd like it to be minimal in terms of risk.
(In reply to comment #14) > I'd like to keep force draw. If we can remove it after the fact as a cleanup patch, thats fine --- but we may end up wanting to cherry pick this code to M19 and if we do, I'd like it to be minimal in terms of risk. Sounds good. I'll keep the DRAW_FORCED state then and only skip the drawLayers() + swap() then if there is nothing to draw. > Nat you suggested in chat that for 2b) just draw anyways cuz it's overoptimization. But then we have to worry about partial swaps and if we should swap the bad contents. It seems easier and just as valid to just not draw/swap in that case? I think 2b) is the exact case I'm trying to fix :) So I guess it's simply something like: bool drawFrame = (prepareToDraw() || forcedDraw) && canDraw();
Just for prioritization, keep in mind that there is no thumbnailer any more on Chrome on Android or ChromeOS.
(In reply to comment #16) > Just for prioritization, keep in mind that there is no thumbnailer any more on Chrome on Android or ChromeOS. (meaning there's no thumbnailer on any platform where we currently support the threaded mode)
Oh, thanks James, good point. I think that's more support for your suggested approach to 2b Daniel. (You can check canDraw before prepareToDraw to avoid some work.)
Created attachment 135242 [details] Patch
> (In reply to comment #10) > > > Should this be done after coming back to didCommitAndDrawFrame() again to make sure we don't skip the finishAllRendering attempt? > > > > > > But isn't this fine since finishAllRendering() is blocking? > > I was thinking that we setNeedsForcedRedraw() through there, but want to make sure the test has a chance to hit the FAIL() in case it is doing the wrong thing? > I still couldn't get it to work that way (wait for the next call to didCommitAndDraw() frame), because that callback is conditional on m_nextFrameIsNewlyCommittedFrameOnImplThread. Which means it gets only called for newly committed frames. It works for the forced readback which does a synchronous commit. I think it's ok though because finishAllRendering() only gets posted after drawLayers() would have been called.
(In reply to comment #20) > > I think it's ok though because finishAllRendering() only gets posted after drawLayers() would have been called. I meant 'completion for finishAllRendering()' does not get signaled until after drawLayers() would have been called on the impl thread.
Created attachment 135243 [details] Patch
(In reply to comment #21) > > I think it's ok though because finishAllRendering() only gets posted after drawLayers() would have been called. > > I meant 'completion for finishAllRendering()' does not get signaled until after drawLayers() would have been called on the impl thread. Oh I see the finishAllRendering() will block until the impl side is done. Then this should be great, sorry for the confusion.
Comment on attachment 135243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135243&action=review Other than ordering comment below, this seems good to me, thanks. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:581 > + bool drawFrame = (m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw) && m_layerTreeHostImpl->canDraw(); Do the canDraw() first please, so it can early out on the prepareToDraw().
Created attachment 135258 [details] Patch
Enne or James, passing this off to you. Can you PTAL at this?
Comment on attachment 135258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135258&action=review Seems OK. Going to R+, but please make sure Nat has a chance to take a look before you land. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:591 > m_readbackRequestOnImplThread->success = !m_layerTreeHostImpl->isContextLost(); if we couldn't actually do a readback, success should be false - right?
Created attachment 135374 [details] Patch
(In reply to comment #27)> > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:591 > > m_readbackRequestOnImplThread->success = !m_layerTreeHostImpl->isContextLost(); > > if we couldn't actually do a readback, success should be false - right? True, changed.
Comment on attachment 135374 [details] Patch LGTM. Thank you. Can you make some comments on the canDraw entry in CCLTHI.h that explains the role of canDraw versus prepareToDraw? I'm uneasy about the duplicated roles that we have there.
Created attachment 135660 [details] Patch
(In reply to comment #30) > (From update of attachment 135374 [details]) > LGTM. Thank you. > > Can you make some comments on the canDraw entry in CCLTHI.h that explains the role of canDraw versus prepareToDraw? I'm uneasy about the duplicated roles that we have there. Done. I guess I'm expecting that there is at least a background root layer if canDraw() succeeds but prepareToDraw() might fail.
Created attachment 135663 [details] Patch for landing
Comment on attachment 135663 [details] Patch for landing Rejecting attachment 135663 [details] from commit-queue. sievers@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Created attachment 135665 [details] Patch
Comment on attachment 135665 [details] Patch Clearing flags on attachment: 135665 Committed r113241: <http://trac.webkit.org/changeset/113241>
All reviewed patches have been landed. Closing bug.
This broke gpu_tests: http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20%28dbg%29/builds/2145/steps/gpu_tests/logs/ThreadedCompositor I can't revert due to conflicts, so I will suppress the test instead. Please fix.
(In reply to comment #38) > This broke gpu_tests: http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20%28dbg%29/builds/2145/steps/gpu_tests/logs/ThreadedCompositor > > I can't revert due to conflicts, so I will suppress the test instead. Please fix. Yes, had to revert the added rootLayer()->bounds().isEmpty() check since m_overflowControlsHostLayer does not set its size. I'll see if there's another simple way to check for empty contents (I guess underneath that root layer then) that works for empty documents. Or maybe RenderLayerCompositor should just set the size on that layer like it does for the clip layer?
(In reply to comment #39) > (In reply to comment #38) > > This broke gpu_tests: http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20%28dbg%29/builds/2145/steps/gpu_tests/logs/ThreadedCompositor > > > > I can't revert due to conflicts, so I will suppress the test instead. Please fix. > > Yes, had to revert the added rootLayer()->bounds().isEmpty() check since m_overflowControlsHostLayer does not set its size. I'll see if there's another simple way to check for empty contents (I guess underneath that root layer then) that works for empty documents. > Or maybe RenderLayerCompositor should just set the size on that layer like it does for the clip layer? It's not a bounding box hierarchy. You can't just check one layer's bounds and make assumptions about the rest of the tree. When do you have a root layer but nothing in it?
(In reply to comment #40) > It's not a bounding box hierarchy. You can't just check one layer's bounds and make assumptions about the rest of the tree. > > When do you have a root layer but nothing in it? New WebView's create and attach an empty document and with force-compositing-mode this creates an empty root layer. There are races where we can end up drawing/swapping this empty frame. So looks like I need to traverse then? I'll check if looking for descendants with 'isDrawable' is good enough here (or if I need to check bounds for 0 also somehow).
https://bugs.webkit.org/show_bug.cgi?id=83351 is great too and for practical purposes means that we already don't swap frames with undefined pixels anymore if there is no content. Instead it would fill with the background color. However, the little caveat: The default background if the document is empty and doesnt specify one (white?) is maybe not always ideal and it might still be nicer to skip the swap. (While if there is at least some pixels to draw, even if they don't fill the viewport, that probably implies that there is also a more well-defined background color from the document).