Closed Bug 552020 Opened 10 years ago Closed 5 years ago

Switch OS X drawing to use Display Link

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jrmuizel, Assigned: mchang)

References

Details

Attachments

(12 files, 15 obsolete files)

62 bytes, text/plain
Details
64 bytes, text/plain
Details
70 bytes, text/plain
Details
66 bytes, text/plain
Details
64 bytes, text/plain
Details
93 bytes, text/plain
Details
95 bytes, text/plain
Details
93 bytes, text/plain
Details
3.74 KB, patch
mchang
: review+
Details | Diff | Splinter Review
3.24 KB, patch
mchang
: review+
Details | Diff | Splinter Review
250.20 KB, text/x-log
Details
13.25 KB, patch
mstange
: review+
Details | Diff | Splinter Review
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.... :(
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?
Blocks: 1071275
Assignee: nobody → mchang
Status: NEW → ASSIGNED
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.
Attached file Profile of Silk OMTA on OS X (obsolete) —
Composite times aren't looking as clean as I expected it to
Attachment #8517072 - Attachment is obsolete: true
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.
Attachment #8517038 - Attachment is obsolete: true
Attachment #8517824 - Flags: review?(bgirard)
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.
Attachment #8517825 - Flags: review?(bgirard)
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.
Attachment #8517828 - Flags: review?(bgirard)
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.
Attachment #8517824 - Flags: review?(bgirard) → review+
Attachment #8517825 - Flags: review?(bgirard) → review+
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
Attachment #8517828 - Flags: review?(bgirard) → review+
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.
Attachment #8518598 - Flags: review?(roc)
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!
Attachment #8518598 - Flags: review?(roc) → review-
Carrying r+, updated with feedback from comment 18.
Attachment #8517824 - Attachment is obsolete: true
Attachment #8517825 - Attachment is obsolete: true
Attachment #8517828 - Attachment is obsolete: true
Attachment #8518598 - Attachment is obsolete: true
Attachment #8520263 - Flags: review+
Carrying r+. Updated with feedback from comment 19.
Attachment #8520265 - Flags: review+
Updated with feedback from comment 21.
Attachment #8520267 - Flags: review?(roc)
(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.
Attachment #8520269 - Flags: review?(bgirard)
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.
Attachment #8520267 - Flags: review?(roc) → review+
Attachment #8520269 - Flags: review?(bgirard) → review+
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.
Attachment #8520263 - Flags: review?(mstange)
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.
Attachment #8520263 - Attachment is obsolete: true
Attachment #8520265 - Attachment is obsolete: true
Attachment #8520263 - Flags: review?(mstange)
Attachment #8521059 - Flags: review?(mstange)
Carrying r+, updated with comment 27.
Attachment #8520267 - Attachment is obsolete: true
Attachment #8521060 - Flags: review+
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.
Attachment #8520264 - Attachment is obsolete: true
Attachment #8520269 - Attachment is obsolete: true
Attachment #8521059 - Attachment is obsolete: true
Attachment #8521060 - Attachment is obsolete: true
Attachment #8521059 - Flags: review?(mstange)
Attachment #8522516 - Flags: review?(mstange)
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
Attachment #8522516 - Flags: review?(mstange) → review+
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
Attachment #8522568 - Attachment description: displaylink.patch → Part 1: Part 1: Hook into CVDisplayLink to get vsync events on OSX. r=benwa,mstange
Attachment #8522568 - Flags: review+
Attachment #8522516 - Attachment is obsolete: true
Attached file Symbolized Crash Dump
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.
Attachment #8523292 - Attachment filename: crash.log → crash.log.txt
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
Attachment #8522568 - Attachment is obsolete: true
Attachment #8524216 - Flags: review?(mstange)
Attachment #8524216 - Flags: review?(mstange) → review+
You need to log in before you can comment on or make changes to this bug.