Bug 88638 – [Qt] requestAnimationFrame should only trigger when a new frame can be displayed.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Zeno Albisser]

Right now the QtWebProcess is executing javascript way too often when relying on requestAnimationFrame.
This is because a timer is being used to service scripted animations.
In case of WK2 we should instead use LayerTreeHostQt::renderNextFrame to be notified when we should execute scripted code
Qt/WK1 nevertheless does not have the concept of painting a complete frame at once, so we need to fallback to the timer in this case.
Because Qt needs to support WK1 and WK2 from the same binary we must be able to decide at runtime if we want to use the timer or rely on renderNextFrame.

[Zeno Albisser]

Created attachment 146525 [details]
patch for review.

[WebKit Review Bot]

Comment on attachment 146525 [details]
patch for review.

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

[Zeno Albisser]

Created attachment 146541 [details]
patch for review.

[Jocelyn Turcotte]

Comment on attachment 146541 [details]
patch for review.

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

> Source/WebKit2/ChangeLog:3
> +        requestAnimationFrame should only trigger when a new frame can be displayed.

Please prepend with [Qt]

Looks fine to me otherwise once it passes the chrome ews.

[WebKit Review Bot]

Comment on attachment 146541 [details]
patch for review.

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

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/accelerated-drawing/svg-filters.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
platform/chromium/compositing/lost-compositor-context-permanently.html
http/tests/media/video-play-progress.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 146546 [details]
Archive of layout-test-results from ec2-cr-linux-02

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

[Zeno Albisser]

Created attachment 146574 [details]
patch for testing with ews.

[Nat Duca]

Comment on attachment 146574 [details]
patch for testing with ews.

This "returning false if you want to use the timer" is a bit confusing. For instance, Chrome has a scheduleAnimation() that returns a bool but in the false case, you're not necessarily being told to schedule a timer.

Would an enum help clarify this? Maybe in doing that, you can get rid of the #ifdefs entirely? There are three states that I know of:
yes i scheduled this
no i didn't schedule this and i need the caller to do it for me
yes I scheduled this but not for anytime soon (case where a raf was registered while the webview was hidden)

[Zeno Albisser]

(In reply to comment #8)
> (From update of attachment 146574 [details])
> This "returning false if you want to use the timer" is a bit confusing. For instance, Chrome has a scheduleAnimation() that returns a bool but in the false case, you're not necessarily being told to schedule a timer.
> 
> Would an enum help clarify this? Maybe in doing that, you can get rid of the #ifdefs entirely? There are three states that I know of:
> yes i scheduled this
> no i didn't schedule this and i need the caller to do it for me
> yes I scheduled this but not for anytime soon (case where a raf was registered while the webview was hidden)

I look at the timer as a fallback solution. And therefore i understand returning false much more as a "you asked me to schedule an animation, but i did not do it. - Please handle that yourself". Falling back to the timer seems to be appropriate in the cases i have seen, but may not be the generic solution.
Personally i would prefer a boolean to an enum, it seems more straight forward to me.
But i do not have a strong preference about it anyway.

[James Robinson]

Comment on attachment 146574 [details]
patch for testing with ews.

You need a ChangeLog entry for the WebCore changes describing what they do and why they are needed.

[Nat Duca]

(In reply to comment #9)
> > Would an enum help clarify this? Maybe in doing that, you can get rid of the #ifdefs entirely? There are three states that I know of:
> > yes i scheduled this
> > no i didn't schedule this and i need the caller to do it for me
> > yes I scheduled this but not for anytime soon (case where a raf was registered while the webview was hidden)
> 
> I look at the timer as a fallback solution. And therefore i understand returning false much more as a "you asked me to schedule an animation, but i did not do it. - Please handle that yourself". Falling back to the timer seems to be appropriate in the cases i have seen, but may not be the generic solution.
> Personally i would prefer a boolean to an enum, it seems more straight forward to me.
> But i do not have a strong preference about it anyway.

How do you represent the decision:
"I dont want you to tick scheduledAnimations via the timer because I'm invisible"?

Does the chrome return true?

[Zeno Albisser]

(In reply to comment #11)
> (In reply to comment #9)
> > > Would an enum help clarify this? Maybe in doing that, you can get rid of the #ifdefs entirely? There are three states that I know of:
> > > yes i scheduled this
> > > no i didn't schedule this and i need the caller to do it for me
> > > yes I scheduled this but not for anytime soon (case where a raf was registered while the webview was hidden)
...
> How do you represent the decision:
> "I dont want you to tick scheduledAnimations via the timer because I'm invisible"?
> 
> Does the chrome return true?

I've been looking at this patch again, and I cannot see a good reason why an enum should be used for that. In the case you mentioned, I think returning true would be appropriate.
It should not matter to the caller how the callee is handling the scheduling. The callee decides if it is handled immediately or delayed because of being hidden.
The only thing that seems to be important for the caller is to know, if the scheduling will be handled or if the timer should be used.
So i only really see two rather than three states here.

I will of course fix the ChangeLog. :)

[Zeno Albisser]

Created attachment 149372 [details]
patch for review.

[Nat Duca]

Sure, thats true for your current use cases on your port.

The difficult case arises with compositor-accelerated css animations. If you dont plan on doing them, then the third state is not going to be an issue for you. But, if you plan on using them, then the callee needs to know if raf is going to be called soon so that it can fire the animaitonStarted event back to the animation controller.

I'm not going to argue too hard since its not in your immediate codebase to have that issue. I'm just sort of warning you of fun things to come. :)

[Zeno Albisser]

Created attachment 151259 [details]
patch for review. - use an enum as a return value for scheduleAnimation as requested.

[WebKit Review Bot]

Comment on attachment 151259 [details]
patch for review. - use an enum as a return value for scheduleAnimation as requested.

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

[Zeno Albisser]

Created attachment 151271 [details]
patch for review.

[WebKit Review Bot]

Comment on attachment 151271 [details]
patch for review.

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

[Zeno Albisser]

Created attachment 151313 [details]
patch for review.

[Simon Hausmann]

Comment on attachment 151313 [details]
patch for review.

I think the scheduleAnimation() interface should remain as-is and serve one single purpose only: schedule an animation. I don't think it should at the same time serve as a "decision" maker whether to fall back to timers or not. For our purposes it is not even necessary to decide that for each scheduleAnimation() call but instead it could simply live as a setting in WebCore::Settings. In WK1 we'd set a scheduleAnimationsWithTimer alike property to true, in WK2 we'd leave it as false.

That way I think you could also avoid the #if PLATFORM(QT) in WebChromeClient::scheduleAnimation(), making it easier for other ports that use WK2 AC to re-use the same code path of properly synchronized RAF calls.

[Simon Hausmann]

Multiple long (and very interesting) discussions brought us now to the following design proposal:

1) WebChromeClient::scheduleAnimation() should send a message to the UI process
2) On the UI process we should tie ourselves to Qt's animation driver / unified timer, which will either be a timer driven or frame driven update mechanism (QQuickWindowManager takes care of that). The tie-in happens for example through a QAbstractAnimation implementation, so it's not bound to QQuick in any way and will just work. The connection happens behind the scenes.
3) When Qt decides that animations should be advanced, we send a ServiceScriptedAnimations message to the web process, that should do what the name says :)


In an ideal world the ServiceScriptedAnimations message to the WebProcess will result in a DidRenderFrame response to the UI process _just_ in time before the scene graph calls our QSGRenderNode.

This model also supports a RequestAnimationFrame "use-case" where the JS callback won't end up changing anything in the DOM/canvas (maybe just send a network message but otherwise decide nothing changed) and then schedules the next RAF. That next RAF callback will be then served at the refresh rate decided by the QAnimationDriver, which seems like the correct thing to do.

[Simon Hausmann]

Tagging this bug with [Qt], because it is primarily a Qt issue at this point.

In the long run any port that uses WK2 and does _not_ have a DisplayRefreshMonitor implementation should use the WK2 IPC messages/callbacks that result from this work, to avoid the timer driver RAF fallback that has the beautiful tendency to hog the CPU :)

[Zeno Albisser]

Created attachment 154054 [details]
patch for review.

[Kenneth Rohde Christiansen]

Comment on attachment 154054 [details]
patch for review.

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

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:103
> +        m_chromeClient->refreshAnimationCallback();

We normally don't use Callback in the naming.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:647
> +void ChromeClientQt::refreshAnimationCallback()
> +{
> +    m_webPage->mainFrame()->d->frame->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));
> +}

None here can be null?

[Noam Rosenthal]

Comment on attachment 154054 [details]
patch for review.

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

Please submit Mac disabling of GraphicsSurface separately

> Source/WebCore/ChangeLog:9
> +        Disable CopyToTexture feature for GraphicsSurface on Mac.
> +        While this is actually working, it is currently used for Tiles
> +        that are single buffered, and therefore requires a call to glFlush.
> +        This call blocks the GPU for about 40ms which would make smooth animations impossible.

This needs to be a different patch.

[Zeno Albisser]

Comment on attachment 154054 [details]
patch for review.

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

>> Source/WebCore/ChangeLog:9
>> +        This call blocks the GPU for about 40ms which would make smooth animations impossible.
> 
> This needs to be a different patch.

sure. - I'll create a separate one.

>> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:103
>> +        m_chromeClient->refreshAnimationCallback();
> 
> We normally don't use Callback in the naming.

I renamed this to serviceScriptedAnimations, as this is what it actually does.

>> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:647
>> +}
> 
> None here can be null?

If i read the code correctly, - none of them.
m_webPage: initialized in constructor initialization list
mainFrame: created when called
d: initialized in constructor initialization list
frame: in QWebFramePrivate::init()
view: instantiated by FrameLoaderClientQt. As far as i see the only one that theoretically can be null. But then again, if it was null there would not be a FrameView and the animation would not be triggered.

[Zeno Albisser]

Created attachment 154134 [details]
patch for review. - fixed issues as commented before.

[Zeno Albisser]

Created attachment 154380 [details]
patch for review.

[Jocelyn Turcotte]

Comment on attachment 154380 [details]
patch for review.

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

> Source/WTF/ChangeLog:7
> +        the servicing of scripted animations to the renderNextFrame call for WK2.

the renderNextFrame call -> layer syncing

> Source/WebKit2/ChangeLog:7
> +        the servicing of scripted animations to the renderNextFrame call for WK2.

Same here.

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:360
> +    m_webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));

Please add a comment on what this achieves and when it is needed.

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:90
> +#if ENABLE(REQUEST_ANIMATION_FRAME) && !USE(REQUEST_ANIMATION_FRAME_TIMER)

The guard should go in the cpp too, or be removed from the header.

[Jocelyn Turcotte]

Comment on attachment 154380 [details]
patch for review.

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

>> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:90
>> +#if ENABLE(REQUEST_ANIMATION_FRAME) && !USE(REQUEST_ANIMATION_FRAME_TIMER)
> 
> The guard should go in the cpp too, or be removed from the header.

As chatted on Irc you could instead move LayerTreeHost::scheduleAnimation under USE(UI_SIDE_COMPOSITING) and not use guards here.
WebChromeClient::scheduleAnimation could look like:

#if ENABLE(REQUEST_ANIMATION_FRAME) && !USE(REQUEST_ANIMATION_FRAME_TIMER)
void WebChromeClient::scheduleAnimation()
{
#if USE(UI_SIDE_COMPOSITING)
    m_page->drawingArea()->layerTreeHost()->scheduleAnimation();
#endif
}
#endif

[Zeno Albisser]

Created attachment 154613 [details]
patch for review.

[Simon Hausmann]

Comment on attachment 154613 [details]
patch for review.

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

Great work! Awesome to have this in place finally :) Just one small comment below:

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:361
> +    m_webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));

I wonder if strictly speaking this call should be guarded by #ifdefs (RAF && !RAF_TIMER && !RAF_REFRESH_MONITOR). It's not doing any harm right now, but if a port decides to use WK2 AC and the platform happens to have a DisplayRefreshMonitor implementation, then enabling it will silently cause too many calls to serviceScriptedAnimations.

Anyway, just a thought, not sure how likely it is that somebody is going to run into this.

[Zeno Albisser]

(In reply to comment #32)
> I wonder if strictly speaking this call should be guarded by #ifdefs (RAF && !RAF_TIMER && !RAF_REFRESH_MONITOR). It's not doing any harm right now, but if a port decides to use WK2 AC and the platform happens to have a DisplayRefreshMonitor implementation, then enabling it will silently cause too many calls to serviceScriptedAnimations.
> 
> Anyway, just a thought, not sure how likely it is that somebody is going to run into this.

I guess an additional #ifdef will not do any harm. - I'll add it before landing.

[Zeno Albisser]

Committed r123786: <http://trac.webkit.org/changeset/123786>