Display Link seems to be the way to get vsynced drawing on OS X. CoreAnimation uses it and we probably want to too. It basically works by having a separate thread that calls a callback when we need to paint. The relevant API seems to be CVDisplayLinkCreateWithCGDisplay and CVDisplayLinkSetOutputCallback The callback looks like: typedef CVReturn (*CVDisplayLinkOutputCallback)( CVDisplayLinkRef displayLink, const CVTimeStamp *inNow, const CVTimeStamp *inOutputTime, CVOptionFlags flagsIn, CVOptionFlags *flagsOut, void *displayLinkContext );

Will we still get these calls even if the browser is theoretically idle? Not like we're ever really idle right now.... :(

When we become idle we can start/stop the DisplayLink as necessary according to the API. For reference: Guide: http://developer.apple.com/mac/library/DOCUMENTATION/GraphicsImaging/Conceptual/CoreVideo/CVProg_Concepts/CVProg_Concepts.html API: http://developer.apple.com/mac/library/DOCUMENTATION/GraphicsImaging/Reference/CoreVideoRef/Reference/reference.html

This is something we should use for the OpenGL layers backend on Mac, right? How would we use this with the current cairo-quartz drawing code?

WIP to listen to vsync events on OSX desktop. Not sure what the best way to clean up the VsyncDispatcher or when ClearOnShutdown executes, on what thread, versus nsAppShell on cocoa. Still have to play with the timing as lots of asserts blow up on debug builds, but it works on release. If anyone has a better idea on how to clean up the nsAppShell and VsyncDispatcher which finishes on ClearOnShutdown, that'd be useful.

Aurora on the left, noticeably jankier.

Nightly w/ Silk + Compositor ONLY, not refresh driver, versus Desktop Chrome.

Nightly on left, Chrome on Right

Huge improvement in scrolling smoothness. I'm not sure the videos do justice, but very large and nice improvement. This is just dispatching vsync notifications to the compositor.

For all the videos, you're better off downloading the videos and watching them natively. The dropbox movie player makes everything look worse.

Composite times aren't looking as clean as I expected it to

Set the Vsync display callback on OS X using the CVDisplayLink. Use the supplied timestamp to drive Vsync notifications. One note is that the aNow time is when the function gets executed whereas as the aOutputTime is when the NEXT frame will be rendered. So aOutputTime is actually in the future, which is actually more accurate for animations.

Right now, we only get vsync profiler markers on b2g. Enable it across platforms and register vsync markers in the VsyncDispatcher. Since the callback execution time on OS X is pretty noisy, this explains why the profiles for the compositor look so much noisier on OS X than b2g.

When we create the nsAppShell, we start listening to vsync events. When we shut down the nsAppShell, we disable and unlink the CVDisplayLink. I enable this by passing an nsBaseAppShell to the VSyncDispatcher. The nsAppShell is the link between HardwareVsync <----> nsAppShell <---- VsyncDispatcher ---> to enable / disable vsync. To do this, I had to create a new virtual method on nsBaseAppShell. Right now though, we can only disable vsync on shutdown and automatically enable it on startup. Not sure this is the right approach though or if it is ok to leave vsync notifications on.

Comment on attachment 8517824 [details] [diff] [review] Part 1: Hook into CVDisplayLink to listen to vsync events Review of attachment 8517824 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/VsyncView.mm @@ +21,5 @@ > + void* aDisplayLinkContext) > +{ > + VsyncView* vsyncView = (VsyncView*) aDisplayLinkContext; > + if ([vsyncView vsyncEnabled]) > + { { on previous line @@ +27,5 @@ > + // is displayed. aOutputTime is when the next frame should be displayed. > + // Now is VERY VERY noisy, aOutputTime is in the future though. > + int64_t timestamp = aOutputTime->hostTime; > + mozilla::TimeStamp vsyncTime = mozilla::TimeStamp(timestamp); > + mozilla::VsyncDispatcher::GetInstance()->NotifyVsync(vsyncTime); I'm looking at the documentation for NotifyVsync and it's not clear what the definition of 'vsync time' is. Can we please document this for the benefits of others? Ideally it shouldn't differ per platform but if it does it's a good place to document how it differs. Particularly we need to make the distinction because 'This is the timestamp for when the frame goes to the screen' or 'This is the timestamp that the vsync interval started at' or something else. @@ +31,5 @@ > + mozilla::VsyncDispatcher::GetInstance()->NotifyVsync(vsyncTime); > + return kCVReturnSuccess; > + } > + > + return kCVReturnDisplayLinkNotRunning; IMO this is clearer if the return in the else branch since it clearly says depending on the branch we're will return something completely different. @@ +51,5 @@ > +- (void)enableVsync > +{ > + // Synchronize buffer swaps with vertical refresh rate > + GLint swapInt = 1; > + [[self openGLContext] setValues:&swapInt forParameter:NSOpenGLCPSwapInterval]; We already have this code else where. We should consolidate these together. @@ +53,5 @@ > + // Synchronize buffer swaps with vertical refresh rate > + GLint swapInt = 1; > + [[self openGLContext] setValues:&swapInt forParameter:NSOpenGLCPSwapInterval]; > + > + // Create a display link capable of being used with all active displays You mentioned there was problems with multi-monitor with different vsync. Is this problem dealt with? If not can we put a TODO comment explaining what happens in the multi-monitor, how it's wrong and what needs to be done to fix it. File a follow up for that.

Comment on attachment 8517828 [details] [diff] [review] Part 3: Enable nsAppShell to enable / disable vsync Review of attachment 8517828 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsBaseAppShell.h @@ +34,5 @@ > + * Notified when the VsyncDispatcher is shutdown. At this point > + * the app shell should stop listening to vsync events from hardware to prevent > + * calling the VsyncDispatcher > + */ > + virtual void DisableVsync() {}; We talked on IRC how this API doesn't let you start/stop the vsync driver as we go in/out of idle. It's still a bit strange not to add the stub at the same time at least however. We agreed that it's a hard blocker for turning it on and that we should be careful not to forget about it (it's easy to miss 60 more idle wake ups) ::: xpcom/ds/TimeStamp.h @@ -386,5 @@ > */ > MOZ_CONSTEXPR TimeStamp() : mValue(0) {} > // Default copy-constructor and assignment are OK > > -#ifdef MOZ_WIDGET_GONK You need someone else to review this change

Like on b2g, OS X timestamps provided by the vsync callback are in the same units at mozilla::TimeStamp. This patch provides the ability to create mozilla::TimeStamp's from the OS X vsync callback system timestamps.

Comment on attachment 8518598 [details] [diff] [review] Part 4: Enable creation of mozilla::TimeStamp from OS X system timestamps Review of attachment 8518598 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +398,1 @@ > { Actually how about making this more explicit by making it a static method TimeStamp::FromSystemTime? Thanks!

Carrying r+, updated with feedback from comment 18.

Carrying r+. Updated with feedback from comment 19.

Updated with feedback from comment 21.

(In reply to Benoit Girard (:BenWa) from comment #18) > @@ +51,5 @@ > > +- (void)enableVsync > > +{ > > + // Synchronize buffer swaps with vertical refresh rate > > + GLint swapInt = 1; > > + [[self openGLContext] setValues:&swapInt forParameter:NSOpenGLCPSwapInterval]; > > We already have this code else where. We should consolidate these together. > Here is a patch that consolidates this across cocoa code.

Comment on attachment 8520267 [details] [diff] [review] Part 4: Enable creation of mozilla::TimeStamp from OS X system timestamps Review of attachment 8520267 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/TimeStamp.h @@ +392,5 @@ > + * retrieved by mozilla::TimeStamp. Since we need this for > + * vsync timestamps, we enable the creation of mozilla::TimeStamps > + * on platforms that support vsync aligned refresh drivers / compositors > + * Verified true as of Nov 7, 2014: B2G and OS X > + * UNTESTED ON OTHER PLATFORMS Please make this #ifdef GONK/MAC.

Comment on attachment 8520263 [details] [diff] [review] Part 1: Hook into CVDisplayLink to listen to vsync events Review of attachment 8520263 [details] [diff] [review]: ----------------------------------------------------------------- mstange should review this too.

Comment on attachment 8520263 [details] [diff] [review] Part 1: Hook into CVDisplayLink to listen to vsync events I don't understand what you're doing with the VsyncView and its OpenGL context. You're never passing it to any display link function, are you? If there was a call to, say, CVDisplayLinkSetCurrentCGDisplayFromOpenGLContext, I'd understand, but as it stands this looks very confusing to me.

After chatting on irc, we don't need an opengl context to listen to vsync events. Remove the VsyncView file and integrate enabling/disabling vsync into nsAppShell.

Carrying r+, updated with comment 27.

Comment on attachment 8521059 [details] [diff] [review] Part 1: Hook into CVDisplayLink to listen to vsync events I'm trying to think of a way to do this without exposing the nsBaseAppShell interface to the vsync stuff. How about this: Instead of adding the now methods on nsBaseAppShell, create a new interface "VsyncSource" with ref-counting, put an implementation called OSXVsyncSource into nsBaseAppShell.mm, create an instance of OSXVsyncSource in nsAppShell::Init, pass that instance to the VsyncDispatcher (using SetSource instead of SetBaseShell), and have the VsyncDispatcher store it in an nsRefPtr<VsyncSource>. Then you could even put a call to DisableVsync into the virtual VsyncSource destructor, and vsync would automatically be disabled during the destructor of VsyncDispatcher by the default destructor of nsRefPtr<VsyncSource>.

Implemented comment 32. Looks a lot nicer without exposing the nsAppShell.

Carrying r+.

Carrying r+

Comment on attachment 8522516 [details] [diff] [review] Part 1: Hook into CVDisplayLink to listen to vsync events Review of attachment 8522516 [details] [diff] [review]: ----------------------------------------------------------------- Much better! ::: widget/VsyncDispatcher.cpp @@ +30,5 @@ > } > > VsyncDispatcher::VsyncDispatcher() > : mCompositorObserverLock("CompositorObserverLock") > + , mVsyncSource(nullptr) You don't need this, nsRefPtr starts out initialized to null. ::: widget/cocoa/nsAppShell.mm @@ +36,5 @@ > > #include <IOKit/pwr_mgt/IOPMLib.h> > #include "nsIDOMWakeLockListener.h" > #include "nsIPowerManagerService.h" > +#include <mozilla/TimeStamp.h> #include "mozilla/TimeStamp.h" @@ +37,5 @@ > #include <IOKit/pwr_mgt/IOPMLib.h> > #include "nsIDOMWakeLockListener.h" > #include "nsIPowerManagerService.h" > +#include <mozilla/TimeStamp.h> > +#include <VsyncDispatcher.h> #include "mozilla/VsyncDispatcher.h" @@ +133,5 @@ > + return; > + } > + > + // Set the renderer output callback function > + if (CVDisplayLinkSetOutputCallback(mDisplayLink, &VsyncCallback, (__bridge void*) this) != kCVReturnSuccess) { You should no longer need the (__bridge void*) thing because "this" is now a C++ pointer, not an Objective C one. @@ +156,5 @@ > + } > + > + virtual bool IsVsyncEnabled() MOZ_OVERRIDE > + { > + return mDisplayLink != nil; let's do != nullptr

Comment on attachment 8522568 [details] [diff] [review] Part 1: Part 1: Hook into CVDisplayLink to get vsync events on OSX. r=benwa,mstange Carrying r+, updated with feedback from comment 36

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc2d99376cd https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fb851f823a https://hg.mozilla.org/integration/mozilla-inbound/rev/a99bd0b3b075

Backed out for OSX non-unified bustage and frequent OSX 10.6 tsvgr crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/647ec1593edb https://treeherder.mozilla.org/logviewer.html#?job_id=3911933&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=3913032&repo=mozilla-inbound

I fixed the un-unified-build bustage, but I've been unable to reproduce locally or see any crashes on anything but OS X 10.6 for the talos tests. The crash stacktrace also doesn't make sense, especially because it crashes while rendering the .svg, not during start up / shutdown when we would interact with the VsyncCode. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7de70c906541

as a note, this triggers a 5.09% CART regression on OSX 10.8. The graph server shows this regression when it landed and improvement when this was backed out: http://graphs.mozilla.org/graph.html#tests=%5B%5B309,63,24%5D%5D&sel=1415746351928.0056,1416028575423.7078,26.086956521739125,65.21739130434784&displayrange=30&datatype=running

So just calling gfxPrefs::GetSingleton in nsAppShell::Init() causes the crash on 10.6: https://hg.mozilla.org/try/rev/1539849bc117 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c20971f2b13e Hmm, I have to figure out another place to init or is this a bug in gfxPrefs?

I don't know where the Preferences system is initialized, but it's very much possible that nsAppShell::Init is just too early to call gfxPrefs::GetSingleton. It's possible that this is causing you to get wrong values for some prefs, and those wrong values could be responsible for the crash (like gfx.work-around-driver-bugs being set to false instead of true). Try moving the OSXVsyncSource lifetime management to nsChildView instead: create it when the first nsChildView::Init call happens, and destroy it when the last child view goes away, similar to what we do for the event thread (search for gNumberOfWidgetsNeedingEventThread). Then you might also be able to get rid of the shutdown order management, since the last nsIWidget should be destroyed before KillClearOnShutdown() destroys the VsyncDispatcher.

Initialize vsync at gfxPlatform::Init rather than at nsAppShell::Init. So far so good on try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=adf5ba55a14c

https://hg.mozilla.org/integration/mozilla-inbound/rev/23614ce89c50 https://hg.mozilla.org/integration/mozilla-inbound/rev/8da176df25ab https://hg.mozilla.org/integration/mozilla-inbound/rev/f134d9b40fe8 Also tried the talos 10.8 CART test on try multiple times: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=94753e08cf4b The regression from comment 42 looks like it disappeared as well.

https://hg.mozilla.org/mozilla-central/rev/23614ce89c50 https://hg.mozilla.org/mozilla-central/rev/8da176df25ab https://hg.mozilla.org/mozilla-central/rev/f134d9b40fe8