Bug 84620 – [chromium] accelerated animations on backgrounded tabs should still tick without the threaded compositor.

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

Currently the do not tick.

Created attachment 138818 [details]
Patch

tl;dr - when requesting a composite in single threaded mode, if we don't successfully schedule a composite, and we really need one (i.e., we have an animation to tick), then schedule one manually.

For this to work scheduleComposite needs to return a bool. Since changing the return type would break the API and make it tricky to roll WebKit, I have instead added bool tryScheduleComposite() to the API, and once chromium is using this API function, scheduleComposite will be removed. I have renamed scheduleComposite tryScheduleComposite throughout the code both for consistency and because the new name is clearer and suggests that it returns a boolean.

The chromium side of this fix is in https://chromiumcodereview.appspot.com/10227004. Ticking on background tabs will not happen in single-threaded mode until it lands.

With this change, we are also able to enable the single-threaded flavor of the animation unit tests (yay!).

It will also allow DumpRenderTree to use threaded animations without the threaded compositor!

This change also makes a significant optimization. Previously, we set needsAnimateLayers = true at every commit because we _might_ have added an animation. This is overkill and forces us to walk the layer tree checking for animations when there may be none. LayerChromium now notifies CCLTH that it needsAnimateLayers when it adds an animation (and this setting is propagated to CCLTHI as appropriate). The result is that needsAnimateLayers is only true when we actually have animations to tick.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 138818 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidgetClient.h:86
> +    virtual bool tryScheduleComposite() 

please document this method.  what does it mean for tryScheduleComposite to return false?
you mean the embedder can decide that it doesn't want to schedule a composite operation
right now?  what does that mean?  if this is about the page being invisible, can't we use
the visibility information that the WebViewImpl already has?  why does the embedder need
to be involved?

[Nat Duca]

Comment on attachment 138818 [details]
Patch

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

Lots of comments, but I do think the basic approach is sound.

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:80
> +    virtual bool tryScheduleComposite() = 0;

The key thing here is changing scheduleComposite to return something indicating whether the composite is going to happen promptly or not. We can do the renaming later, as a refactoring-only change.

Currently, scheduleComposite() only guarantees that we'll call ::composite at a later time. When a render_widget is offscreen, that "later time" can be forever delayed --- until the widget is visible. In general, thats a good thing.

DRT currently violates this assumption: WebViewHost::scheduleComposite() doesn't actually trigger a call to composite.

There are some cases, e.g. when an animation is starting, when scheduleComposite leading to a composite() is truly necessary.

So, we have three options, I think:
1. make scheduleComposite not guarantee a composite() call. This feels funky.
2. make scheduleComposite say when its not guaranteeing a prompt return
3. pass a "i need you to really call composite, no matter what" flag to scheduleComposite.

#2 is what this patch does, and it seems like a good middle ground to me. #3 would work, but it requires some additional functionality to be added to render_widget, which always scares the crap out of me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:670
> +bool CCLayerTreeHost::tryScheduleComposite()

assert that you're not in threaded mode

maybe just have the proxy go m_layerTreeHost->client->tryScheduleComposite() instead of having it go through the lth at all?

if we had a scheduleComposite on this class, lets have it go away

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:256
> +            m_redrawTimer = CCSingleThreadProxyRedrawTimer::create(this);

dont lazily create this. make it up front to keep this code linear.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> +        m_redrawTimer.clear();

redraw timer -> forced composite timer?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:282
> +        if (m_layerTreeHost->needsAnimateLayers())

This shouldn't be inside the debugscopedsetimplthread --- this is main thread code. Only the didBeComeInvisibleOnImplThread should be in the debugScopedSet

>> Source/WebKit/chromium/public/WebWidgetClient.h:86
>> +    virtual bool tryScheduleComposite() 
> 
> please document this method.  what does it mean for tryScheduleComposite to return false?
> you mean the embedder can decide that it doesn't want to schedule a composite operation
> right now?  what does that mean?  if this is about the page being invisible, can't we use
> the visibility information that the WebViewImpl already has?  why does the embedder need
> to be involved?

We might make this return an enum to make it clear that its not returning a yes/no decsion but a "yes || yes_but_delayed || no" --- render_widget can answer yes an yes_but_delayed. DRT answers no, and always has. Its just not been a problem, before.

Created attachment 139242 [details]
Patch

(In reply to comment #4)
> (From update of attachment 138818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138818&action=review
>
> Lots of comments, but I do think the basic approach is sound.
>
> > Source/Platform/chromium/public/WebLayerTreeViewClient.h:80
> > +    virtual bool tryScheduleComposite() = 0;
>
> The key thing here is changing scheduleComposite to return something indicating whether the composite is going to happen promptly or not. We can do the renaming later, as a refactoring-only change.
>
Switched tryScheduleComposite back to scheduleComposite.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:670
> > +bool CCLayerTreeHost::tryScheduleComposite()
>
> assert that you're not in threaded mode
Done.
>
> maybe just have the proxy go m_layerTreeHost->client->tryScheduleComposite() instead of having it go through the lth at all?
>
> if we had a scheduleComposite on this class, lets have it go away
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:256
> > +            m_redrawTimer = CCSingleThreadProxyRedrawTimer::create(this);
>
> dont lazily create this. make it up front to keep this code linear.
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> > +        m_redrawTimer.clear();
>
> redraw timer -> forced composite timer?
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:282
> > +        if (m_layerTreeHost->needsAnimateLayers())
>
> This shouldn't be inside the debugscopedsetimplthread --- this is main thread code. Only the didBeComeInvisibleOnImplThread should be in the debugScopedSet
Done.
>
> >> Source/WebKit/chromium/public/WebWidgetClient.h:86
> >> +    virtual bool tryScheduleComposite()
> >
> > please document this method.  what does it mean for tryScheduleComposite to return false?
> > you mean the embedder can decide that it doesn't want to schedule a composite operation
> > right now?  what does that mean?  if this is about the page being invisible, can't we use
> > the visibility information that the WebViewImpl already has?  why does the embedder need
> > to be involved?
I've added documentation to explain that 'true' means that a composite is immanent, and 'false'
means that the composite will be delayed, possibly forever.
>
> We might make this return an enum to make it clear that its not returning a yes/no decsion but a "yes || yes_but_delayed || no" --- render_widget can answer yes an yes_but_delayed. DRT answers no, and always has. Its just not been a problem, before.
Being able to distinguish indefinite from undetermined doesn't seem valuable right now. Can we maybe add this later when the need arises?

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:281:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 20 files


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

(In reply to comment #6)
> Attachment 139242 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:281:  This { should be at the end of the previous line  [whitespace/braces] [4]
> Total errors found: 1 in 20 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

It is a false positive: https://bugs.webkit.org/show_bug.cgi?id=85087

[WebKit Review Bot]

Comment on attachment 139242 [details]
Patch

Attachment 139242 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12557498

New failing tests:
CCLayerTreeHostTestSynchronizeAnimationStartTimes.runMultiThread

[WebKit Review Bot]

Created attachment 139338 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nat Duca]

Comment on attachment 139242 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:342
> +bool CCLayerTreeHost::needsAnimateLayers() const

who uses this accessor?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:44
> +static const double forcedCompositeTickRate = 0.1;

60hz please

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:58
> +        m_proxy->compositeImmediately();

Iirw, we're not supposed to composite when we're not visible (due to backbuffer dropping stuff). So, you're going to need to make sure compositeImmediately() doens't actually draw or swap.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> +    if (!m_layerTreeHost->client()->scheduleComposite() && m_layerTreeHost->needsAnimateLayers())

stash schedComp return value in a bool to help things along.

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:281
>> +        {
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

Make the stylebot happy if its not already, please. You can pull this out to a helper function if necessary.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:273
>      }

Where's the single thread unit test that verifies that we behave correctly in this new case?

[Nat Duca]

Also, where's your WebWigetHost changes? Does DRT compile with this change?

Created attachment 139840 [details]
Patch

(In reply to comment #10)
> (From update of attachment 139242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139242&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:342
> > +bool CCLayerTreeHost::needsAnimateLayers() const
>
> who uses this accessor?
The single threaded proxy uses it to determine if it needs to schedule a commit when backgrounded.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:44
> > +static const double forcedCompositeTickRate = 0.1;
>
> 60hz please
I've added code to switch between 60hz and 10hz depending on whether or now we're backgrounded.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:58
> > +        m_proxy->compositeImmediately();
>
> Iirw, we're not supposed to composite when we're not visible (due to backbuffer dropping stuff). So, you're going to need to make sure compositeImmediately() doens't actually draw or swap.
Good catch. I've updated doComposite to bail if we're invisible.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> > +    if (!m_layerTreeHost->client()->scheduleComposite() && m_layerTreeHost->needsAnimateLayers())
>
> stash schedComp return value in a bool to help things along.
Done.
>
> >> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:281
> >> +        {
> >
> > This { should be at the end of the previous line  [whitespace/braces] [4]
>
> Make the stylebot happy if its not already, please. You can pull this out to a helper function if necessary.
Made a helper function.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:273
> >      }
>
> Where's the single thread unit test that verifies that we behave correctly in this new case?
The animation tests in CCLayerTreeHostTest.cpp now run in single threaded mode. In particular, CCLayerTreeHostTestTickAnimationWhileBackgrounded.runSingleThread covers the new case. Please let me know if it's insufficient.
> Also, where's your WebWigetHost changes? Does DRT compile with this change?
Yep, the changes are there and DRT does compile.

[Darin Fisher (:fishd, Google)]

Comment on attachment 139840 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:260
> +    bool compositeIsImminent = m_layerTreeHost->client()->scheduleComposite();

Couldn't we just require that embedders MUST call composite() in response
to scheduleComposite()?  But, we could arrange for Chrome to call composite()
more slowly (at 100ms intervals) when the page is hidden.  This wouldn't be
simpler?

Created attachment 140293 [details]
Patch

Needed one more define in order for aura to compile.

[Dana Jansens]

Comment on attachment 140293 [details]
Patch

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

Seems sane to me :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:345
> +void CCLayerTreeHost::setNeedsAnimateLayers()
> +{
> +    m_needsAnimateLayers = true;
> +}
> +
> +bool CCLayerTreeHost::needsAnimateLayers() const
> +{
> +    return m_needsAnimateLayers;
> +}

These could go in the .h?

[Nat Duca]

> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:260
> > +    bool compositeIsImminent = m_layerTreeHost->client()->scheduleComposite();
> 
> Couldn't we just require that embedders MUST call composite() in response
> to scheduleComposite()?  But, we could arrange for Chrome to call composite()
> more slowly (at 100ms intervals) when the page is hidden.  This wouldn't be
> simpler?

I'd rather not start a policy of doing that. Take a peek at a production chrome with a half dozen tabs open in about:tracing and look at the amount of ticking we've already got going on. I'd rather reduce that then increase it.

[James Robinson]

Can we do 1hz instead of 10hz for background?  We've done that for setTimeout()/setInterval() and I'd like to stay roughly consistent.

Created attachment 140751 [details]
Patch

(In reply to comment #17)
> Can we do 1hz instead of 10hz for background?  We've done that for setTimeout()/setInterval() and I'd like to stay roughly consistent.

Done.

Created attachment 140773 [details]
Patch

Gardening patch.

[Nat Duca]

Comment on attachment 140773 [details]
Patch

LGTM

[James Robinson]

This isn't necessary any more with https://bugs.webkit.org/show_bug.cgi?id=86013 - right?

[Eric Seidel (no email)]

Comment on attachment 140773 [details]
Patch

Cleared review? from attachment 140773 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).