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.
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 :-)
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.
<rdar://problem/10218848>
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.
Created attachment 110172 [details] Patch
Comment on attachment 110172 [details] Patch Can more of this code be in WebCore so it can be shared between WebKit1 and WebKit2?
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.
(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.
Created attachment 110761 [details] Patch
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.
Created attachment 110765 [details] Patch
Comment on attachment 110765 [details] Patch Attachment 110765 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10046081
Created attachment 110782 [details] Patch
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?
(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
Created attachment 110870 [details] Patch
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
Committed r97405: <http://trac.webkit.org/changeset/97405>
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