Bug 81716 – [chromium] Skip frames when checkerboarding an animation

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Nat Duca]

And figure out how to get return values out.
We need this to make checkerboarding of animated layers block rendering.

[Dana Jansens]

1. Makes draw vs forced draw in scheduler
2. Don't draw when prepareToUpdate fails and it's not a forced draw
3. If the draw failes, set needsRedraw again so we will redraw next vsync.

[Dana Jansens]

Created attachment 133149 [details]
Patch

Here's a patch but when it drops a frame it doesn't recover and basically locks up.

I'm super not feeling well and going to go home. If any of you can look at it that'd be cool if not I will when I am feeling better.

[WebKit Review Bot]

Attachment 133149 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 133149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133149&action=review

Looking really really good. Awesome turnaround time. Tweaks and ping me when you're ready for another round.

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

Yaaay stylebot! :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:278
> +    // If we are unable to draw an animation on some layer, then we don't want to draw the frame unless forced to.

I'm thinking this sentence is sort of better clobbered and a new one written, than tweaked. Maybe with a few setences. :) E.g. "ideally, we want to X. however, blahblah may mean we have no other option. therefore zzzz."

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:-311
> -                break;

do you want the prepare function to take a bool saying force, so we can skip the calculation cost if we can't draw?

Though, maybe its better to always do the full calculate to avoid scary "the tree is half updated" type situations.

Just make the call whichever way you feel is best, no need to discuss I think.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:41
>      virtual bool hasMoreResourceUpdates() const = 0;

I'd like to delete the canDraw() code and instead have that state fold into this new thing. That could be a followon patch for risk mitigation if you look at it and decide its a bit scary for now. If we go for deferring this, lets file a bug now.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:44
> +    virtual bool scheduledActionDrawAndSwap(bool forcedDraw) = 0;

Mind making this 2 actual methods, one per action? Notes below on why I think this is better...

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:52
> +bool CCSchedulerStateMachine::shouldDrawForced() const

not seeing why we need this method...  if you're trying to acccess this in the tests, then there is a special StateMachine subclass in the test.cpp that provides setters/accessors to internal states.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:59
> +    ASSERT(!m_needsForcedRedraw); // shouldDrawForced() should always be checked first.

What's the win here? Think of a unit test that might want to check shouldDraw() for some reason... I think its fine to have this still return true here.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:89
> +        if (shouldDrawForced())

why this and not m_needsForcedRedraw?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:90
> +            return ACTION_DRAW_FORCED;

i think you could just write this as if (shouldDraw()) return m_needsForcedRedraw ? FORCED : POSSIBLE;

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:215
> +void CCSchedulerStateMachine::notifyDrawSuccess(bool success)

didDrawIfPossibleComplete(bool success)

shouldn't be called for forced case

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:100
> +    // attempting the redraw.

Make comment more precise. E.g. indicates whether ACTION_DRAW_IF_POSSIBLE succeeded or not.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:545
> +bool CCThreadProxy::scheduledActionDrawAndSwap(bool forcedDraw)

We should call this drawAndSwapInternal, and make the CCSchedulerClient have two methods that mirror the actions. You can then have the proxy have each implemenetation call into this internal function. Its convenient to say "for every state machine action, there is a scheduledAction on the scheduler cilent."

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:559
> +    bool drawFrame = m_layerTreeHostImpl->prepareToDraw(frame);

draw = prepareToDraw || forcedDraw
if(draw) drawLayers

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:564
> +        if (m_readbackRequestOnImplThread) {

I think this should be unindented. The universe hangs if we dont satisfy this request --- if we forget to set forcedDraw or something, we might read back something stale, but at least we dont hang.

You might instead assert inside this branch that we did draw.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:571
> +        m_layerTreeHostImpl->swapBuffers();

again, i think unindent and make a single line saing if(didDraw) swap

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:575
>      if (m_finishAllRenderingCompletionEventOnImplThread) {

might want to assert that force is true inside this branch

[Dana Jansens]

(In reply to comment #4)
> (From update of attachment 133149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133149&action=review
> 
> Looking really really good. Awesome turnaround time. Tweaks and ping me when you're ready for another round.
> 
> >> Source/WebCore/ChangeLog:8
> >> +        No new tests. (OOPS!)
> > 
> > You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> Yaaay stylebot! :)

That was on purpose I swear :)

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:278
> > +    // If we are unable to draw an animation on some layer, then we don't want to draw the frame unless forced to.
> 
> I'm thinking this sentence is sort of better clobbered and a new one written, than tweaked. Maybe with a few setences. :) E.g. "ideally, we want to X. however, blahblah may mean we have no other option. therefore zzzz."

k

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:-311
> > -                break;
> 
> do you want the prepare function to take a bool saying force, so we can skip the calculation cost if we can't draw?
> 
> Though, maybe its better to always do the full calculate to avoid scary "the tree is half updated" type situations.
> 
> Just make the call whichever way you feel is best, no need to discuss I think.

thought about both ways and chose complete frames for the reason you state above.

> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:41
> >      virtual bool hasMoreResourceUpdates() const = 0;
> 
> I'd like to delete the canDraw() code and instead have that state fold into this new thing. That could be a followon patch for risk mitigation if you look at it and decide its a bit scary for now. If we go for deferring this, lets file a bug now.

FIXME

> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:44
> > +    virtual bool scheduledActionDrawAndSwap(bool forcedDraw) = 0;
> 
> Mind making this 2 actual methods, one per action? Notes below on why I think this is better...

k

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:52
> > +bool CCSchedulerStateMachine::shouldDrawForced() const
> 
> not seeing why we need this method...  if you're trying to acccess this in the tests, then there is a special StateMachine subclass in the test.cpp that provides setters/accessors to internal states.

yeh, ill use member var directly.

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:59
> > +    ASSERT(!m_needsForcedRedraw); // shouldDrawForced() should always be checked first.
> 
> What's the win here? Think of a unit test that might want to check shouldDraw() for some reason... I think its fine to have this still return true here.

k

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:89
> > +        if (shouldDrawForced())
> 
> why this and not m_needsForcedRedraw?

k

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:90
> > +            return ACTION_DRAW_FORCED;
> 
> i think you could just write this as if (shouldDraw()) return m_needsForcedRedraw ? FORCED : POSSIBLE;

k

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:215
> > +void CCSchedulerStateMachine::notifyDrawSuccess(bool success)
> 
> didDrawIfPossibleComplete(bool success)
> 
> shouldn't be called for forced case

k

> > Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:100
> > +    // attempting the redraw.
> 
> Make comment more precise. E.g. indicates whether ACTION_DRAW_IF_POSSIBLE succeeded or not.

k

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:545
> > +bool CCThreadProxy::scheduledActionDrawAndSwap(bool forcedDraw)
> 
> We should call this drawAndSwapInternal, and make the CCSchedulerClient have two methods that mirror the actions. You can then have the proxy have each implemenetation call into this internal function. Its convenient to say "for every state machine action, there is a scheduledAction on the scheduler cilent."

k

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:559
> > +    bool drawFrame = m_layerTreeHostImpl->prepareToDraw(frame);
> 
> draw = prepareToDraw || forcedDraw
> if(draw) drawLayers

yay, i was going to do that but thought maybe it was less clear, but i like it :)

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:564
> > +        if (m_readbackRequestOnImplThread) {
> 
> I think this should be unindented. The universe hangs if we dont satisfy this request --- if we forget to set forcedDraw or something, we might read back something stale, but at least we dont hang.
> 
> You might instead assert inside this branch that we did draw.

k

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:571
> > +        m_layerTreeHostImpl->swapBuffers();
> 
> again, i think unindent and make a single line saing if(didDraw) swap

k

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:575
> >      if (m_finishAllRenderingCompletionEventOnImplThread) {
> 
> might want to assert that force is true inside this branch

assert that force is true or that draw is true?? assuming draw for now..

[Dana Jansens]

Created attachment 133260 [details]
Patch

[Dana Jansens]

Addressed all the comments, but not ready for commit yet since we don't recover from dropping frames still. I'm going to work on figuring that one out now.

Repro case: poster circle in a small browser window.

[Dana Jansens]

Created attachment 133324 [details]
Patch

[Dana Jansens]

Working patch! It requires us to swap buffers even though we did not draw though. Else everything just locks up and I am not sure why.

[Dana Jansens]

I think there might be a tiny bit of jitter when it drops a frame. That might be the bad swap? Or timing issue.

[Shawn Singh]

(In reply to comment #10)
> I think there might be a tiny bit of jitter when it drops a frame. That might be the bad swap? Or timing issue.

Can you please try https://bugs.webkit.org/show_bug.cgi?id=80899 to see if that fixes your "lock-up" ?

As per our offline discussion, I don't expect it to fix that particular weirdness.  But... I do expect that patch to be necessary before this lands, since early exiting like this means the damage tracker updated its state without swap.  So yeah.  I'll work on a cleaner patch for that right now.


However, something else feels weird about this.  With this patch, are we actually skipping draw, but still swapping?  why would we want to do that?

[Shawn Singh]

> 
> However, something else feels weird about this.  With this patch, are we actually skipping draw, but still swapping?  why would we want to do that?

sorry for the noise.  I see that is the pending question Dana is trying to figure out already.

[Dana Jansens]

Created attachment 133336 [details]
Patch

[Dana Jansens]

Fixed the swapbuffers issue!

diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp
index 1b148ca..8f7dd35 100644
--- a/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp
@@ -92,6 +92,12 @@ void CCScheduler::didSwapBuffersComplete()
     m_frameRateController->didFinishFrame();
 }
 
+void CCScheduler::didSkipFrame()
+{
+    TRACE_EVENT("CCScheduler::didSkipFrame", this, 0);
+    m_frameRateController->didFinishFrame();
+}
+
 void CCScheduler::didLoseContext()
 {
     TRACE_EVENT("CCScheduler::didLoseContext", this, 0);
diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h b/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h
index c876e20..1c9c071 100644
--- a/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h
@@ -77,6 +77,7 @@ public:
 
     void setMaxFramesPending(int);
     void didSwapBuffersComplete();
+    void didSkipFrame();
 
     void didLoseContext();
     void didRecreateContext();
diff --git a/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp b/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp
index 7f237ad..43ab528 100644
--- a/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp
@@ -569,7 +569,10 @@ bool CCThreadProxy::scheduledActionDrawAndSwapInternal(bool forcedDraw)
         m_readbackRequestOnImplThread = 0;
     }
 
-    m_layerTreeHostImpl->swapBuffers();
+    if (drawFrame)
+        m_layerTreeHostImpl->swapBuffers();
+    else
+        m_schedulerOnImplThread->didSkipFrame();
 
     // Process any finish request
     if (m_finishAllRenderingCompletionEventOnImplThread) {

[Nat Duca]

Comment on attachment 133336 [details]
Patch

LGTM

[Dana Jansens]

Created attachment 133345 [details]
Patch

Better fix to the swapbuffers problem. Just don't beginFrame() on the CCFrameRateController if we didn't draw.

[Dana Jansens]

Created attachment 133360 [details]
Patch

Add a unit test to make sure we don't beginFrame on the CCFrameRateController when we don't draw the frame.

[Adrienne Walker]

Comment on attachment 133360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133360&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:173
> +            m_client->scheduledActionDrawAndSwapForced();

What if we really can't draw a frame, like there's no root layer CCLTHI or something? Should this really begin a frame anyway?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:99
> +    // Indicates whether ACTION_DRAW_IF_POSSIBLE draw to the screen or not.

s/draw/drew/

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:550
>      if (!m_layerTreeHostImpl)
> -        return;
> +        return true;

Should this return false, since we're not swapping?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:577
>      // Process any finish request
>      if (m_finishAllRenderingCompletionEventOnImplThread) {
> +        ASSERT(drawFrame);

Can we consider ourselves finished if we haven't drawn anything?

[Dana Jansens]

Comment on attachment 133360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133360&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:173
>> +            m_client->scheduledActionDrawAndSwapForced();
> 
> What if we really can't draw a frame, like there's no root layer CCLTHI or something? Should this really begin a frame anyway?

It should never happen due to canDraw() like comment below.

>> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:99
>> +    // Indicates whether ACTION_DRAW_IF_POSSIBLE draw to the screen or not.
> 
> s/draw/drew/

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:550
>> +        return true;
> 
> Should this return false, since we're not swapping?

Yes, done.

We should not ever get here like this, as canDraw() returns false if there is no LayerTreeHostImpl. I'll add an assert here as it'll be more clear than the assert for returning false on a forcedDraw

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:577
>> +        ASSERT(drawFrame);
> 
> Can we consider ourselves finished if we haven't drawn anything?

This is when falling back to software mode. I didn't know this either so explanation:

It goes like
WebViewImpl::setIsAcceleratedCompositingActive(false)
 CCLayerTreeHost::finishAllRendering()
  CCThreadProxy::finishAllRendering()
  wait...
...meanwhile in impl town...
CCThreadProxy::finishAllRenderingOnImplThread()
 CCScheduler::setNeedsForcedRedraw()

So there will be a forced redraw when this happens, since they have higher priority than non-forced redraws. Thus I think ASSERT is fair here.

[Dana Jansens]

Created attachment 133378 [details]
Patch

[Dana Jansens]

For the sake of super clarity:

CCThreadProxy::finishAllRenderingOnImplThread() <- sets m_finishAllRenderingCompletionEventOnImplThread

[Adrienne Walker]

Comment on attachment 133378 [details]
Patch

Thanks for all of the explanations.  That seems reasonable that when we're forced to draw, we've already checked that we can draw.  This last patch looks good to me.

[WebKit Review Bot]

Comment on attachment 133378 [details]
Patch

Clearing flags on attachment: 133378

Committed r111805: <http://trac.webkit.org/changeset/111805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.