Bug 68911 – Sync requestAnimationFrame callback to CVDisplayLink on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

Currently there is an implementation of requestAnimationFrame which uses a WebCore::Timer to fire its events. When it fires, it passes currentTime as the timestamp. This does not sync with display refresh and causes visual glitches. There should be a better timestamp which is based on the display refresh. We can also use this to determine the rate of firing of the Timer to avoid excessive calls to rAF.

[Chris Marrin]

This bug will cover the feature of providing a platform specific API for getting the timestamp based on the display refresh rate. I will do a platform specific implementation for WebKit 2 on Mac, and leave the rest as an exercise for the port author :-)

[Chris Marrin]

My intention is to create a call, similar to scheduleAnimation() but which simply asks platform specific code for a time estimate of when the next frame will be visible. That can not only be used as the timestamp sent to rAF, but as the basis for when the next call to rAF is made. That should avoid any excessive calls to rAF. 

For Mac, I plan to look into adding a displayLink thread which will maintain a timestamp value tied to refresh. I didn't try using a displayLink at first because I initially thought I'd use it to actually drive the firing of the callback, which would have been complicated and require a lot of communication between the threads. Just having the displayLink maintain a timestamp means I just need to provide thread safe access to that value. Hopefully that will keep overhead low but will achieve the synchronization goal.

[Radar WebKit Bug Importer]

<rdar://problem/10218848>

[Chris Marrin]

After doing a lot of testing, I've decided that using CVDisplayLink as a passive timestamp manager can never give the callback firing accuracy we desire. So I'm repurposing this bug to just be for adding the CVDisplayLink sync work for mac.

[Chris Marrin]

Created attachment 110172 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 110172 [details]
Patch

Can more of this code be in WebCore so it can be shared between WebKit1 and WebKit2?

[Simon Fraser (smfr)]

Comment on attachment 110172 [details]
Patch

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

Does this all work when subframe is calling rAF?

> Source/WebKit/mac/WebView/WebView.mm:6109
> +    WebView* webView = (WebView*) data;

The * goes on the right for Obj-C pointers. No space after the ). You should also use a C++-style cast.

> Source/WebKit/mac/WebView/WebView.mm:6114
> +        webView->_private->animationScheduleState = AnimationStateIdle;

Is there any danger that webView->_private may have gone away already?

> Source/WebKit/mac/WebView/WebView.mm:6125
> +    WebView* webView = (WebView*) data;

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:6128
> +    double outputTimeSeconds = static_cast<double>(outputTime->videoTime) / static_cast<double>(outputTime->videoTimeScale);

Can you not just use the hostTime field for these?

> Source/WebKit/mac/WebView/WebView.mm:6129
> +    double webKitNow = currentTime();

Is this using the same timebase as the CVTimeStamp times?

> Source/WebKit/mac/WebView/WebView.mm:6146
> +        ASSERT(!error);

I think you should actually handle this error (e.g. return). There may be situations where it fails (e.g. a headless machine).

> Source/WebKit/mac/WebView/WebView.mm:6152
> +        error = CVDisplayLinkStart(_private->displayLink);
> +        ASSERT(!error);

Ditto.

> Source/WebKit2/UIProcess/WebPageProxy.h:416
> +    void changeScreen(CGDirectDisplayID);

I think this name is a bit too generic. Maybe windowScreenDidChange().

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:275
> +    shutdownDisplayLink();

This feels like it might be too late. Can a WebPage be disconnected from the view but stay alive?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:602
> +    double nowSeconds = static_cast<double>(now->videoTime) / static_cast<double>(now->videoTimeScale);
> +    double outputTimeSeconds = static_cast<double>(outputTime->videoTime) / static_cast<double>(outputTime->videoTimeScale);
> +    double webKitNow = currentTime();

Same comments about time as above.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:626
> +    if (webPage->corePage()->mainFrame() && webPage->corePage()->mainFrame()->view())
> +        webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(timestamp));

Stash webPage->corePage()->mainFrame() in a local var?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:633
> +        if (!m_displayID)
> +            m_displayID = CGMainDisplayID();

Is the display ID always non-zero?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:642
> +        CVReturn error = CVDisplayLinkCreateWithCGDisplay(m_displayID, &m_displayLink);
> +        ASSERT(!error);
> +    
> +        error = CVDisplayLinkSetOutputCallback(m_displayLink, displayLinkCallback, this);
> +        ASSERT(!error);
> +    
> +        error = CVDisplayLinkStart(m_displayLink);
> +        ASSERT(!error);

I think you need to handle these errors.

[Chris Marrin]

(In reply to comment #7)
> (From update of attachment 110172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110172&action=review
> 
> Does this all work when subframe is calling rAF?

I'm writing a test case for this

> > Source/WebKit/mac/WebView/WebView.mm:6114
> > +        webView->_private->animationScheduleState = AnimationStateIdle;
> 
> Is there any danger that webView->_private may have gone away already?

I've added protectors on both WK and WK2

> > Source/WebKit/mac/WebView/WebView.mm:6128
> > +    double outputTimeSeconds = static_cast<double>(outputTime->videoTime) / static_cast<double>(outputTime->videoTimeScale);
> 
> Can you not just use the hostTime field for these?

hostTime is not a consistent value on all platforms, apparently. At any rate, it's not the same timebase as WebKit time

> 
> > Source/WebKit/mac/WebView/WebView.mm:6129
> > +    double webKitNow = currentTime();
> 
> Is this using the same timebase as the CVTimeStamp times?

No. Those times are based on when the program started or something.

...
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:275
> > +    shutdownDisplayLink();
> 
> This feels like it might be too late. Can a WebPage be disconnected from the view but stay alive?

Maybe. But the CVDisplayLink can stay alive as long as the WebPage lives. The only downside might be some overhead of constantly firing the displayLink. But the displayLink just checks the flag and then returns if there's no work. So I don't think the overhead matters.

[Chris Marrin]

Created attachment 110761 [details]
Patch

[Chris Marrin]

Latest patch takes a different approach, on recommendation from Adam and Simon. It moves the CVDisplayLink down into WebCore. This not only avoids duplicate code, but also allows fallback to the timer based rAF if the CVDisplayLink can't be initialized. This happens if you're on a headless system.

I also now manage the displayLinks with a singleton manager and maintain only one displayLink for each display to avoid many threads when there are many different Windows using rAF.

[Chris Marrin]

Created attachment 110765 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110765 [details]
Patch

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

[Chris Marrin]

Created attachment 110782 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 110782 [details]
Patch

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

This is pretty close; just some nits to tidy up.

> Source/WebCore/dom/Document.cpp:5129
>          m_scriptedAnimationController = ScriptedAnimationController::create(this);
> +        m_scriptedAnimationController->windowScreenDidChange(page()->displayID());

I think it would be better to pass in the displayID to the ScriptedAnimationController ctor.

> Source/WebCore/dom/ScriptedAnimationController.cpp:156
> +#if PLATFORM(MAC)
> +    DisplayRefreshMonitorManager::sharedManager()->windowScreenDidChange(displayID, this);
> +#else

Shame to have Mac #ifdefs in this file. Maybe define USE_DISPLAY_MONITOR somewhere?

> Source/WebCore/dom/ScriptedAnimationController.cpp:166
> +        m_useTimer = !DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this);

This is a little hard to grok. Maybe 
if (DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this))
  return;
 
m_useTimer = true
...

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:78
> +    for (size_t i = 0; i < monitor->m_clients.size(); ++i)
> +        monitor->m_clients[i]->fireDisplayRefreshIfNeeded(timestamp);

Can fireDisplayRefreshIfNeeded modify the m_clients array?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:84
> +    static DisplayRefreshMonitorManager* staticManager = new DisplayRefreshMonitorManager;
> +    return staticManager;

DEFINE_STATIC_LOCAL perhaps, and return a reference?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:87
> +size_t DisplayRefreshMonitorManager::findMonitor(PlatformDisplayID displayID) const

Why not just
DisplayRefreshMonitor* monitorForDisplay(PlatformDisplayID)
?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:101
> +    size_t i = findMonitor(client->m_displayID);

I'd use monitorIndex or index, reserving i for use in loops.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:106
> +        monitor = new DisplayRefreshMonitor(client->m_displayID);
> +        m_monitors.append(monitor);

m_monitors could contain OwnPtrs to avoid manual new/delete.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:123
> +    size_t i = findMonitor(client->m_displayID);
> +    if (i == notFound)
> +        return;
> +    
> +    DisplayRefreshMonitor* monitor = m_monitors[i];
> +    if (monitor->removeClient(client)) {

You've done two array walks here; should be able to do it in one.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:140
> +    client->scheduleAnimation();
> +    return m_monitors[i]->scheduleAnimation();

Maybe too many scheduleAnimation() methods which don't really all mean the same thing. Reading the code, I'm not sure why there are two calls here.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:150
> +    client->m_displayID = displayID;
> +    client->m_displayIDIsSet = true; 

Have a setDisplayID method?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:60
> +    bool m_scheduled;
> +    PlatformDisplayID m_displayID;
> +    bool m_displayIDIsSet;

Put the bools next to eachother to optimize packing.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:118
> +    bool scheduleAnimation(DisplayRefreshMonitorClient*);

Should say what the return value means.

> Source/WebKit/mac/WebView/WebView.mm:3296
> +    _private->page->windowScreenDidChange((PlatformDisplayID)[[[[[self window] screen] deviceDescription] objectForKey:@"NSScreenNumber"] intValue]);

Might want to do some null checking on private and private->page.

> Source/WebKit/mac/WebView/WebView.mm:3324
> +    [self _windowDidChangeScreen:notification];

You shouldn't send an unrelated notification to the _windowDidChangeScreen method.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1886
> +    [self _windowDidChangeScreen:notification];

Ditto.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1167
> +    // FIXME: IPC has trouble sending params it doesn't understand, so cast into a common type
> +    process()->send(Messages::WebPage::WindowScreenDidChange(displayID), m_pageID);

It's unclear what is actionable about this FIXME. Does it not work here?

[Chris Marrin]

(In reply to comment #14)
> (From update of attachment 110782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110782&action=review
> 
> This is pretty close; just some nits to tidy up.
> 
> > Source/WebCore/dom/Document.cpp:5129
> >          m_scriptedAnimationController = ScriptedAnimationController::create(this);
> > +        m_scriptedAnimationController->windowScreenDidChange(page()->displayID());
> 
> I think it would be better to pass in the displayID to the ScriptedAnimationController ctor.

done

> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:156
> > +#if PLATFORM(MAC)
> > +    DisplayRefreshMonitorManager::sharedManager()->windowScreenDidChange(displayID, this);
> > +#else
> 
> Shame to have Mac #ifdefs in this file. Maybe define USE_DISPLAY_MONITOR somewhere?

I added WTF_USE_REQUEST_ANIMATION_FRAME_DISPLAYMONITOR to Platform.h

> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:166
> > +        m_useTimer = !DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this);
> 
> This is a little hard to grok. Maybe 
> if (DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this))
>   return;
> 
> m_useTimer = true
> …


done

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:78
> > +    for (size_t i = 0; i < monitor->m_clients.size(); ++i)
> > +        monitor->m_clients[i]->fireDisplayRefreshIfNeeded(timestamp);
> 
> Can fireDisplayRefreshIfNeeded modify the m_clients array?

The only way to modify it is to call windowScreenDidChange(). That only gets called when the ScriptedAnimationController is created or when an event comes in to change it. Both of those are synchronous, so I don't think they can happen in the callback.

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:84
> > +    static DisplayRefreshMonitorManager* staticManager = new DisplayRefreshMonitorManager;
> > +    return staticManager;
> 
> DEFINE_STATIC_LOCAL perhaps, and return a reference?

done

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:87
> > +size_t DisplayRefreshMonitorManager::findMonitor(PlatformDisplayID displayID) const
> 
> Why not just
> DisplayRefreshMonitor* monitorForDisplay(PlatformDisplayID)
> ?

In the case of removing a monitor, I need its index to pass to Vector::remove()

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:101
> > +    size_t i = findMonitor(client->m_displayID);
> 
> I'd use monitorIndex or index, reserving i for use in loops.

done

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:106
> > +        monitor = new DisplayRefreshMonitor(client->m_displayID);
> > +        m_monitors.append(monitor);
> 
> m_monitors could contain OwnPtrs to avoid manual new/delete.

Well, it would only get rid of the delete, which is in only one place. I started doing this, and the transfer of ownership upon creation feels trickier and more fragile than just doing a manual delete in this case. So I don't think it would help make the code more clear or more safe.

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:123
> > +    size_t i = findMonitor(client->m_displayID);
> > +    if (i == notFound)
> > +        return;
> > +    
> > +    DisplayRefreshMonitor* monitor = m_monitors[i];
> > +    if (monitor->removeClient(client)) {
> 
> You've done two array walks here; should be able to do it in one.

Hmmm, where am I doing that? findMonitor() walks the m_monitors array and removeClient() walks the m_clients array. Those are two different arrays. How can I eliminate one? Or are you seeing something else?

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:140
> > +    client->scheduleAnimation();
> > +    return m_monitors[i]->scheduleAnimation();
> 
> Maybe too many scheduleAnimation() methods which don't really all mean the same thing. Reading the code, I'm not sure why there are two calls here.

Fixed

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:150
> > +    client->m_displayID = displayID;
> > +    client->m_displayIDIsSet = true; 
> 
> Have a setDisplayID method?

done

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:60
> > +    bool m_scheduled;
> > +    PlatformDisplayID m_displayID;
> > +    bool m_displayIDIsSet;
> 
> Put the bools next to eachother to optimize packing.

done

> 
> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:118
> > +    bool scheduleAnimation(DisplayRefreshMonitorClient*);
> 
> Should say what the return value means.

done

> 
> > Source/WebKit/mac/WebView/WebView.mm:3296
> > +    _private->page->windowScreenDidChange((PlatformDisplayID)[[[[[self window] screen] deviceDescription] objectForKey:@"NSScreenNumber"] intValue]);
> 
> Might want to do some null checking on private and private->page.

done

> 
> > Source/WebKit/mac/WebView/WebView.mm:3324
> > +    [self _windowDidChangeScreen:notification];
> 

done

> You shouldn't send an unrelated notification to the _windowDidChangeScreen method.

I've split this out into a doWindowDidChangeScreen, which both notifications call

> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1886
> > +    [self _windowDidChangeScreen:notification];
> 
> Ditto.

ditto

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1167
> > +    // FIXME: IPC has trouble sending params it doesn't understand, so cast into a common type
> > +    process()->send(Messages::WebPage::WindowScreenDidChange(displayID), m_pageID);
> 
> It's unclear what is actionable about this FIXME. Does it not work here?

Sorry, that was cruft

Another patch on the way

[Chris Marrin]

Created attachment 110870 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 110870 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.h:91
> +    void displayRefreshFired(double timestamp) { serviceScriptedAnimations(convertSecondsToDOMTimeStamp(timestamp)); }

Declare as virtual to make it more obvious that it's implementing the client interface.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:121
> +    size_t i = findMonitor(client->m_displayID);
> +    if (i == notFound)
> +        return;
> +    

i -> index

[Chris Marrin]

Committed r97405: <http://trac.webkit.org/changeset/97405>

[Ryosuke Niwa]

This patch appears to have broken Leopard build:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/40587/steps/compile-webkit/logs/stdio