Bug 66280 – [GTK] requestAnimationFrame support for gtk port

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

This bug is for supporting requestAnimationFrame JS interface on webkit-gtk.

[ChangSeok Oh]

Created attachment 105786 [details]
A draft patch

A draft patch.

[ChangSeok Oh]

Created attachment 106114 [details]
Proposed patch

[Carlos Garcia Campos]

Comment on attachment 106114 [details]
Proposed patch

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

Instead of duplicating the code between webkit1 and webkit2 it would be better to move common code to a new class in WebCore and share it between webkit1 and webkit2

> configure.ac:935
> +# check whether to enable requestAnimationFrame support
> +AC_MSG_CHECKING([whether to enable requestAnimationFrame support])
> +AC_ARG_ENABLE(request_animation_frame,
> +              AC_HELP_STRING([--enable-request-animation-frame],
> +                             [enable support for requestAnimationFrame [default=no]]),
> +              [],[enable_request_animation_frame="no"])
> +AC_MSG_RESULT([$enable_request_animation_frame])
> +

Why not enable it by default?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:673
> +#if ENABLE(REQUEST_ANIMATION_FRAME)
> +void ChromeClient::scheduleAnimation()
> +{
> +    g_signal_emit_by_name(m_webView, "schedule-animation");
> +}

Maybe we could implement the animation stuff here, so that we don't need to add a new signal to web view.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:210
> +#if ENABLE(REQUEST_ANIMATION_FRAME)
> +    SCHEDULE_ANIMATION,
> +#endif

I would avoid adding a new signal for this.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1365
> +
> +        g_main_context_wakeup(gMainContext);

Why do you need to wakeup the main context here?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1369
> +        priv->requestAnimationFrameTimerSource = g_timeout_source_new(0);
> +        ASSERT(priv->requestAnimationFrameTimerSource);
> +

I don't understand this. The first time you create a timeout surce with 0, to be dispatched inmediately (I think you could use a g_idle_source() instead), but in the callback you check whether if elapsed time is 16ms, why not schedule a 16ms timeout here instead? I guess the first time timerFiredCallback is called it will always schedule a new timeout source.

[Martin Robinson]

Comment on attachment 106114 [details]
Proposed patch

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

Nice work. Please fix all of Carlos' points as well. They all look valid.

In general, I would like to the see the animation stuff put into a WebCore class. It should be shared between WebKit1 and WebKit2, as it's almost identical.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:634
> +    double m_timeStamp;

I don't think the name "m_timeStamp" is specific enough. What is the timestamp for?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47
> +static const double AnimationThrottleTimeInterval = 0.016;

This should be called gAnimationThrottleTimeInterval. It's a magic number too, so you should have a comment explaining why it's 0.016.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:154
> +    WebPage* self = static_cast<WebPage*>(data);

You should probably call this "page" since this is a static method.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:161
> +    double elapsedTime = AnimationThrottleTimeInterval;
> +    if (timeStamp)
> +        elapsedTime = now - timeStamp;

When is timestamp zero?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:167
> +        if (self->corePage()->mainFrame() && self->corePage()->mainFrame()->view())
> +            self->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));

You access self->corePage()->mainFrame() three times, so you should cache it.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:176
> +    g_timeout_add_full(G_PRIORITY_DEFAULT, remainingTime * 1000, reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)self, 0);

Please insert newlines into lines that extend beyond 120 characters.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:184
> +    if (!m_requestAnimationFrameTimerSource) {
> +        GMainContext* gMainContext = g_main_context_default();

Please use an early return here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:186
> +        g_main_context_ref(gMainContext);

Why ref and unref the main context? Is this happening on a thread other than the main thread?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:190
> +        m_requestAnimationFrameTimerSource = g_timeout_source_new(static_cast<guint>(0));

Do you really need to cast 0 to guint? I find that really surprising.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:195
> +        // Or else some animations will be freezed while resizing window or scrolling.

freezed -> frozen
resizing window -> resizing the window

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:199
> +        g_source_set_callback(m_requestAnimationFrameTimerSource.get(), reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)this, 0);

You shouldn't need to cast to gpointer here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:202
> +        g_main_context_unref(gMainContext);

If you must ref and unref the main context, it's better to use a GRefPtr here. You'll need to add a specialization in JavaScriptCore/wtf/gobject/GRefPtr.h and JavaScriptCore/wtf/gobject/GRefPtr.cpp.

> Source/WebKit/gtk/ChangeLog:12
> +        Implement requestAnimationFrame for WebKit gtk port.
> +        GSourceTimer is added to WebKitWebViewPriv and used to call callback function.
> +        The timer limits to call callback function more than one time in every 16ms.
> +        This timer has lower priority than G_PRIORITY_HIGH_IDLE + 20.
> +        Because, or else some dirty rects will not be updated while resizing and scrolling.

Super nit! Please even out the line breaks here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1307
> +static const double AnimationThrottleTimeInterval = 0.016; // For 60fps

This should be called gAnimationThrottleTimeInterval.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1311
> +    if (!webView)
> +        return;

I'm pretty sure you want to use an ASSERT here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    if (!priv || !priv->requestAnimationFrameTimerSource)
> +        return;

You shouldn't really need to check if priv is null.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1342
> +        if (webView && core(webView)->mainFrame() && core(webView)->mainFrame()->view())
> +            core(webView)->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(now));
> +        return false;

Please cache core(webView)->mainFrame(). Might want to just use early return through this whole section.

[ChangSeok Oh]

Comment on attachment 106114 [details]
Proposed patch

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

Thank you for review. :)
I'm not convinced sharing code between WebKit1 & WebKit2 is possible and better way. But let me consider more.

>> configure.ac:935
>> +
> 
> Why not enable it by default?

I think this feature is experimental yet. Formal spec is not fixed so that we may need to modify this later.
But I don't mind changing default = yes.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:634
>> +    double m_timeStamp;
> 
> I don't think the name "m_timeStamp" is specific enough. What is the timestamp for?

I'll change m_timeStampForPreviousScene.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47
>> +static const double AnimationThrottleTimeInterval = 0.016;
> 
> This should be called gAnimationThrottleTimeInterval. It's a magic number too, so you should have a comment explaining why it's 0.016.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:154
>> +    WebPage* self = static_cast<WebPage*>(data);
> 
> You should probably call this "page" since this is a static method.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:161
>> +        elapsedTime = now - timeStamp;
> 
> When is timestamp zero?

First call for requestAnimationFrame.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:167
>> +            self->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));
> 
> You access self->corePage()->mainFrame() three times, so you should cache it.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:176
>> +    g_timeout_add_full(G_PRIORITY_DEFAULT, remainingTime * 1000, reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)self, 0);
> 
> Please insert newlines into lines that extend beyond 120 characters.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:186
>> +        g_main_context_ref(gMainContext);
> 
> Why ref and unref the main context? Is this happening on a thread other than the main thread?

Maybe No. I'll remove ref, unref.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:190
>> +        m_requestAnimationFrameTimerSource = g_timeout_source_new(static_cast<guint>(0));
> 
> Do you really need to cast 0 to guint? I find that really surprising.

Oops. sorry.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:195
>> +        // Or else some animations will be freezed while resizing window or scrolling.
> 
> freezed -> frozen
> resizing window -> resizing the window

Done.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:199
>> +        g_source_set_callback(m_requestAnimationFrameTimerSource.get(), reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)this, 0);
> 
> You shouldn't need to cast to gpointer here.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:202
>> +        g_main_context_unref(gMainContext);
> 
> If you must ref and unref the main context, it's better to use a GRefPtr here. You'll need to add a specialization in JavaScriptCore/wtf/gobject/GRefPtr.h and JavaScriptCore/wtf/gobject/GRefPtr.cpp.

Let me try again after removing ref, unref. If I find any issue. then I'll add changes for GRefPtr. :)

>> Source/WebKit/gtk/ChangeLog:12
>> +        Because, or else some dirty rects will not be updated while resizing and scrolling.
> 
> Super nit! Please even out the line breaks here.

Done

>> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:673
>> +}
> 
> Maybe we could implement the animation stuff here, so that we don't need to add a new signal to web view.

Yor're right. My first implementation was like you mentioned.
But I moved code from ChromeClient to webview after seeing cmarrin's patch for mac port (bug59146). Is it better maintaining consistency among ports?

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:210
>> +#endif
> 
> I would avoid adding a new signal for this.

ditto.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1307
>> +static const double AnimationThrottleTimeInterval = 0.016; // For 60fps
> 
> This should be called gAnimationThrottleTimeInterval.

Done

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1311
>> +        return;
> 
> I'm pretty sure you want to use an ASSERT here.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
>> +        return;
> 
> You shouldn't really need to check if priv is null.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1342
>> +        return false;
> 
> Please cache core(webView)->mainFrame(). Might want to just use early return through this whole section.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1365
>> +        g_main_context_wakeup(gMainContext);
> 
> Why do you need to wakeup the main context here?

Oops. This line is useless in current implementation. I'll remove this.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1369
>> +
> 
> I don't understand this. The first time you create a timeout surce with 0, to be dispatched inmediately (I think you could use a g_idle_source() instead), but in the callback you check whether if elapsed time is 16ms, why not schedule a 16ms timeout here instead? I guess the first time timerFiredCallback is called it will always schedule a new timeout source.

There are some reasons.  
At the first time scheduleAnimation is called, we don't need to wait 16ms to update first scene. 
Using g_timeout_source_new(16) here is valid in the best case. However let's suppose that the content is too heavy or system performance is too low, so that the system can't support 60fps. It means it takes more time than 16ms to update next scene. In that case we have to call timerFiredCallback without extra delay to show better fps.
For instance, if it takes 30ms to be called scheduleAnimation again from first call, then next scene will be updated after 46ms(30 + 16)) from first update. Here we don't need to waste extra 16ms.

> I guess the first time timerFiredCallback is called it will always schedule a new timeout source.
Actually it's not true. it depend on the content. If JS callback function call requestAnimationFrame again, scheduleAnimation is called back again, Or else it's just one-shot.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 106114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106114&action=review
> 
> Thank you for review. :)
> I'm not convinced sharing code between WebKit1 & WebKit2 is possible and better way. But let me consider more.

Yes, it should be possible.

> >> configure.ac:935
> >> +
> > 
> > Why not enable it by default?
> 
> I think this feature is experimental yet. Formal spec is not fixed so that we may need to modify this later.
> But I don't mind changing default = yes.

I think it's harmless, what do you think Martin?

> >> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:673
> >> +}
> > 
> > Maybe we could implement the animation stuff here, so that we don't need to add a new signal to web view.
> 
> Yor're right. My first implementation was like you mentioned.
> But I moved code from ChromeClient to webview after seeing cmarrin's patch for mac port (bug59146). Is it better maintaining consistency among ports?

I think it's that important at this level, but if you still want the implementation in WebView, add a private method instead of adding an undocummented signal to the API.

> >> Source/WebKit/gtk/webkit/webkitwebview.cpp:1369
> >> +
> > 
> > I don't understand this. The first time you create a timeout surce with 0, to be dispatched inmediately (I think you could use a g_idle_source() instead), but in the callback you check whether if elapsed time is 16ms, why not schedule a 16ms timeout here instead? I guess the first time timerFiredCallback is called it will always schedule a new timeout source.
> 
> There are some reasons.  
> At the first time scheduleAnimation is called, we don't need to wait 16ms to update first scene. 
> Using g_timeout_source_new(16) here is valid in the best case. However let's suppose that the content is too heavy or system performance is too low, so that the system can't support 60fps. It means it takes more time than 16ms to update next scene. In that case we have to call timerFiredCallback without extra delay to show better fps.
> For instance, if it takes 30ms to be called scheduleAnimation again from first call, then next scene will be updated after 46ms(30 + 16)) from first update. Here we don't need to waste extra 16ms.

I still don't see whay you need to create a new source and attach it to the context manually, instead of just using g_idle_add() or g_timeout add(). Would it be possible to use g_idle_add() for the first time and schedule a g_timeout callback that will be called every 16ms, so that you don't need to keep track of elapsed time?

[Martin Robinson]

(In reply to comment #5)

> There are some reasons.  
> At the first time scheduleAnimation is called, we don't need to wait 16ms to update first scene. 
> Using g_timeout_source_new(16) here is valid in the best case. However let's suppose that the content is too heavy or system performance is too low, so that the system can't support 60fps. It means it takes more time than 16ms to update next scene. In that case we have to call timerFiredCallback without extra delay to show better fps.
> For instance, if it takes 30ms to be called scheduleAnimation again from first call, then next scene will be updated after 46ms(30 + 16)) from first update. Here we don't need to waste extra 16ms.

Isn't starving the main loop a problem if you use 0 second timeouts?

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 106114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106114&action=review
> > 
> > Thank you for review. :)
> > I'm not convinced sharing code between WebKit1 & WebKit2 is possible and better way. But let me consider more.
> 
> Yes, it should be possible.

Please do! 

> > >> configure.ac:935
> > >> +
> > > 
> > > Why not enable it by default?
> > 
> > I think this feature is experimental yet. Formal spec is not fixed so that we may need to modify this later.
> > But I don't mind changing default = yes.
> 
> I think it's harmless, what do you think Martin?

It's a bad idea to enable experimental features by default, because we might end up breaking ABI (of a sort) if the feature is removed or changed. Disabling it by default protects us a bit from that. Immature features are also less secure, I'd say.

[ChangSeok Oh]

Created attachment 107978 [details]
updated patch

cmarrin's patch submitted a few days ago moved timer implementation from webView to ScriptedAnimationController, so that makes time implementation be common code for WebKit1 & 2. (I realized it after finishing revising my patch. :p). cmarrin's algorithm looks similar to mine. When I tested it on gtk port webkit, it worked well, so no reason not to accept it for GTK port.
Here is new smaller patch to just enable the feature.

[Martin Robinson]

Comment on attachment 107978 [details]
updated patch

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

This is indeed a lot smaller! Looks good to me, except for one small thing.

> configure.ac:892
> +                             [enable support for requestAnimationFrame [default=no]]),

We typically postfix experimental feature descriptions with (experimental). Do you mind doing the same here?

[ChangSeok Oh]

(In reply to comment #9)
> (From update of attachment 107978 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107978&action=review
> 
> This is indeed a lot smaller! Looks good to me, except for one small thing.
> 
> > configure.ac:892
> > +                             [enable support for requestAnimationFrame [default=no]]),
> 
> We typically postfix experimental feature descriptions with (experimental). Do you mind doing the same here?

Sure :)

[ChangSeok Oh]

Created attachment 107999 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 107999 [details]
Updated patch

Clearing flags on attachment: 107999

Committed r95564: <http://trac.webkit.org/changeset/95564>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.