Bug 75577 – [chromium] Route all animate calls through CCLayerTreeHost in composited mode to simplify rate limiting logic

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Route all animate calls through CCLayerTreeHost in composited mode to simplify rate limiting logic

[James Robinson]

Created attachment 121161 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

See ChangeLog for description of what this does. One thing that's a bit ugly is that WebWidget declares animate() and layout(). Since I wanted to break up CCLayerTreeHostClient's animateAndLayout() function, the obvious names are animate() and layout() but they collide in WebViewImpl which implements both WebWidget and CCLayerTreeHostClient. The implementations for layout() are the same so I just left that alone, but the two animate() overrides require different behavior. I solved this by (somewhat arbitrarily) renaming the CCLayerTreeHostClient version to updateAnimations().

This requires a chromium-side change for the WebLayerTreeViewClient interface change which I'll land before landing this or aura will stop compiling. It looks like WebLayerTreeViewClient implementations don't currently use animateAndLayout() for anything at all. I expect that they will want to hook up with updateAnimations() but they don't immediately need a layout(), so I've left that out of the API for now. I may end up adding layout() as well in a future patch.

[James Robinson]

Chromium side here: http://codereview.chromium.org/9008078/

[Nat Duca]

Comment on attachment 121161 [details]
Patch

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

Unofficial LGTM

> Source/WebKit/chromium/src/WebViewImpl.cpp:1141
> +    if (m_layerTreeHost)

should this be m_acceleratedCompositingActive or something like that?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1149
> +void WebViewImpl::updateAnimations(double frameBeginTime)

I think the trace event for animate has disappeared with this patch. Might just put an trace here? I assume layout is still there, I think that was explicitly traced.

[James Robinson]

Ah whoops, WebViewImpl::animate() and WebViewImpl::layout() have traces, but in the threaded path I did lose the trace for the animate call. I think I'll just move the animate trace in WebViewImpl to WebViewImpl::updateAnimations(), which all codepaths still go through. Does that sound good?

[James Robinson]

(In reply to comment #5)
> (From update of attachment 121161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121161&action=review
> 
> Unofficial LGTM
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1141
> > +    if (m_layerTreeHost)
> 
> should this be m_acceleratedCompositingActive or something like that?

Yes, I think it should be. Fixing.

[James Robinson]

Created attachment 121178 [details]
Patch

[James Robinson]

Created attachment 121193 [details]
Patch

[James Robinson]

I changed my mind re WebLayerTreeViewClient::layout() - i'm going to need that for the next patch (making WebViewImpl depend on WebLayerTreeView instead of CCLayerTreeHost) and since it requires a two-sided patch it's easier to just do it all at once.

[Nat Duca]

Comment on attachment 121193 [details]
Patch

Ship it. Unofficial lgtm

[Darin Fisher (:fishd, Google)]

Comment on attachment 121193 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1136
> +#if USE(ACCELERATED_COMPOSITING)

nit: What about moving this code into WebViewImpl::updateAnimations, or if not,
then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it
should only be called when not in accelerated compositing mode?  Maybe add an
assertion too?

> Source/WebKit/chromium/src/WebViewImpl.h:112
> +    // This also implements WebCore::CCLayerTreeHostClient::layout()

micro-nit: maybe put this comment inline so the flow of function declarations is not disrupted?

  virtual void layout(); // Also implements CCLayerTreeHostClient::layout()

[James Robinson]

Comment on attachment 121193 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1136
>> +#if USE(ACCELERATED_COMPOSITING)
> 
> nit: What about moving this code into WebViewImpl::updateAnimations, or if not,
> then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it
> should only be called when not in accelerated compositing mode?  Maybe add an
> assertion too?

currently updateAnimations() is called in composited and non-composited mode. the three possibilities are:

(1) non-composited:
render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate()
WebViewImpl::animate() calls WebViewImpl::updateAnimations()

(2) composited but render_widget scheduling (single-threaded)
render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate()
WebViewImpl::animate() calls CCLayerTreeHost::updateAnimations()
CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations()

(3) composited with compositor-driven scheduling (threaded)
CCThreadProxy calls CCLayerTreeHost::updateAnimations()
CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations()

so i'm not sure i can fold any functions into each other

>> Source/WebKit/chromium/src/WebViewImpl.h:112
>> +    // This also implements WebCore::CCLayerTreeHostClient::layout()
> 
> micro-nit: maybe put this comment inline so the flow of function declarations is not disrupted?
> 
>   virtual void layout(); // Also implements CCLayerTreeHostClient::layout()

Sure, will do

[James Robinson]

Chromium implementations of new pure virtuals on WebLayerTreeViewClient landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=116580.

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> (From update of attachment 121193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121193&action=review
> 
> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1136
> >> +#if USE(ACCELERATED_COMPOSITING)
> > 
> > nit: What about moving this code into WebViewImpl::updateAnimations, or if not,
> > then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it
> > should only be called when not in accelerated compositing mode?  Maybe add an
> > assertion too?
> 
> currently updateAnimations() is called in composited and non-composited mode. the three possibilities are:
> 
> (1) non-composited:
> render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate()
> WebViewImpl::animate() calls WebViewImpl::updateAnimations()
> 
> (2) composited but render_widget scheduling (single-threaded)
> render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate()
> WebViewImpl::animate() calls CCLayerTreeHost::updateAnimations()
> CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations()
> 
> (3) composited with compositor-driven scheduling (threaded)
> CCThreadProxy calls CCLayerTreeHost::updateAnimations()
> CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations()
> 
> so i'm not sure i can fold any functions into each other

OK... makes sense.  I hadn't understood all of the call paths.
I'm happy with the code as you have it.

[James Robinson]

Committed r104248: <http://trac.webkit.org/changeset/104248>