Open Bug 1184283 Opened 9 years ago Updated 2 years ago

support multiple vsync sources (for different rates and special situations)

Categories

(Core :: Graphics, task, P3)

Product:
Core
Component:
Graphics
Version:
35 Branch
Type:
task

Tracking

()

Tracking Flags:
Tracking Status
platform-rel --- -

People

(Reporter: vlad, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted][webvr][games:p?][platform-rel-Games])

Attachments

(11 files, 12 obsolete files)

MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting
58 bytes, text/x-review-board-request
froydnj
: review+
Details
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files
58 bytes, text/x-review-board-request
roc
: review+
Details
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager
58 bytes, text/x-review-board-request
mchang
: review+
roc
: review+
Details
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)
58 bytes, text/x-review-board-request
mchang
: review+
Details
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent
58 bytes, text/x-review-board-request
roc
: review+
mchang
: review+
Details
MozReview Request: Bug 1184283 - Widget vsync source/observer
58 bytes, text/x-review-board-request
mchang
: review+
Details
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer
58 bytes, text/x-review-board-request
roc
: review+
Details
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync
58 bytes, text/x-review-board-request
mchang
: review+
roc
: review+
Details
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID
58 bytes, text/x-review-board-request
mchang
: review+
roc
: review+
Details
MozReview Request: Bug 1184283 - test... removals
58 bytes, text/x-review-board-request
Details 
For VR and other cases, we need the ability to have a widget run from a different vsync source other than the global one (possibly even a software timer vsync source).

To keep this clean, we want to be able to swap a widget's Display (the thing that provides vsync notifications; may get renamed) and have that automagically cascade down to all associated Compositors and RefreshTimers.

To do this:
- create a RefreshTimerVsyncDispatcher per window, instead of one global one
- [maybe] merge RefreshTimerVD and CompositorVD into single VsyncDispatcher
- add functionality on widget to swap its dispatcher between multiple displays
- ensure that all downstream stuff (compositors, refresh timers) know about the unique VD's

chat log:

14:54 &vlad mchang: I think in the future, as long as we have a unique
            compositor/refreshtimer dispatcher for each widget, this becomes
            easy
14:55 &vlad right now, we have a single global refresh timer dispatcher, right?
14:55 mchang vlad: correct
14:55 &vlad and a per-window compositor dispatcher
14:55 mchang vlad: we have a unique per-window compositor dispatcher
14:55 &vlad so step one would be to have a per-window refresh timer dispatcher,
            even if they all do the same thing right now
14:56 mchang yeah that would work, then we could kill some of the layers as well
14:56 &vlad and then be able to add/remove refresh timer dispatchers to
            vsyncsource like the compositor ones
14:56 &vlad and at *that* point, we may be able to merge the two dispatchers
            into a single interface
14:56 mchang correct
14:56 mchang vlad: yeah sorry, i was just trying to get the vsync stuff working
             in the existing architecture
14:57 &vlad like I said, not your fault at all :) there's lots of accumulated
            cruft here
14:57 mchang we wanted to keep the refresh timer / driver relationship the same
14:57 &vlad so ok, once we get to per-window dispatchers
14:57 &vlad we should be able to swap that dispatcher's source without affecting
            anything downstream from it, right?
14:57 mchang correct
14:57 &vlad it'll just get notified at the new rates
14:57 mchang yes
14:57 mchang just a heads up, we use the term Display
14:57 mchang 1 vsync source, multiple displays
14:58 mchang each display correspodning to a new vsync rate
14:58 &vlad erm
14:58 &vlad so each Display has a VsyncSource?
14:58 mchang no, 1 global vsync source, the global vysnc source has multiple
             displays
14:58 mchang where you can do all the swapping
14:58 mchang but feel free to rewrite it if you want
14:59 &vlad how does that work? how do you decide which display's source to use?
14:59 mchang if it isn’t working out the way we expected
14:59 &vlad I mean right now I just see GetGlobalDisplay() :)
14:59 mchang vlad: right now just 1 global display :)
14:59 &vlad haha
14:59 mchang vlad: but we were expecting to have some id associated with each
             display
14:59 mchang most of the OS’ provide some way to query the attached displays
14:59 &vlad okay, so how about this
14:59 &vlad the widget will decide which Display to use, and that display will
            then provide a vsync source?
15:00 mchang Err, mostly correct. The vsync Source should have all the displays,
             and the widget decides which display to listen to
15:00 &vlad which nicely handles the case of when a window moves to a new
            display (since the widget will be the first thing to know about
            that)
15:01 mchang but if you mean vsync source here as in “notification of when vsync
             happens on that display”, then yes
15:01 &vlad ok, seems like just a naming thing then.. to me a vsync source
            sounds like a single source of vsync notifications, not a manager of
            vsync :)
15:01 &vlad "the thing vsync comes from" :)
15:01 mchang then yes :)
15:01 &vlad ok, so the widget decides which Display to use
15:01 mchang yup
15:01 mchang and if we have per-widget timer, we can unify the VsyncDispatchers
15:02 mchang i think, unless the IPC stuff is too annoying for you since content
             still has to be notified
15:02 &vlad VsyncSource will lose its
            AddCompositorVD/RemoveCompositorVD/GetRefreshTimerVsyncDispatcher()
            methods  (or will just gain a AddMainDisplayVsyncDispatcher(), for
            when you don't have a widget available)
Unfortunately the quick hacky way to make this work for VR is turning out to be tricky (each window has multiple RefreshDrivers for each document; we need to override both the toplevel one and the one for the window that just went fullscreen for this to work).  With my hack we end up with two separate timers (one at the higher correct refresh rate, and another the original regular rate one) that cause all sort of weirdness to happen.

Conceptually, what I think we need (using horrible new names that don't conflict with current stuff):

- VSyncSender: An object that provides vsync events for a single device/source.  (This is currently VSyncSource::Display.)
- for each nsIWidget, the corresponding VSyncSender.
- The Compositor will listen to the VSyncSender for its widget
- nsRefreshDriver will listen to the VSyncSender 
- Compositor and nsRefreshDriver should create timers that listen to their content's widget's VSyncSender

There should be no global vsync sender.  Everything should be based on the nsIWidget's VSyncSender.

If the nsIWidget's VSyncSender ever changes, the Compositor and nsRefreshDriver should pick up the change automatically.  (There should probably be an adapter at the nsIWidget's level that will listen to the VSyncSender and then send the events along -- that way the VSyncSender can change without downstream listeners being aware.)

For widgets and platforms where we can't actually get real VSync, the VSyncSender should just be a software source, much like PreciseRefreshDriverTimer with the refresh rate of the display.  Does that make sense?

Changes that will need to be made:

- VsyncSource, VsyncSource::Display need to be turned into the single VsyncSender class (likely called VsyncSource).  We should have a static method to get a global one, but it should be avoided as much as possible.
- Each nsIWidget needs to be aware of what display its on, and to know how to ask for the appropriate VsyncSender for its display.  Initially, it can just assume it's always on the same display and ignore changes, except in specific cases (e.g. for VR, when we move to a HMD we can poke the widget to update the sender correctly).  Child widgets need to be aware of this too -- simplest would be to always have child widgets forward requests for the VsyncSender to the root widget.
- Each nsRefreshDriver needs to create a RefreshDriverTimer for its widget. This timer will just forward the events from the widget, so it shouldn't be significant overhead to have this.  If it doesn't have a widget, then it should use the vsync source for the global display.
- The Compositor behaves much like it does already -- it already asks nsIWidget for a CompositorVsyncDispatcher.
- CompositorVsyncDispatcher/RefreshDriverVsyncDispatcher likely go away and get merged with a single interface for observing vsync events

Is there something I'm missing?
More detail and discussion between me and mchang: https://etherpad.mozilla.org/14bpZSA7dY
Whiteboard: gfx-noted
Here's a WIP set of patches -- mchang, feedback would be welcome, especially since the dispatchers had some logic that I wasn't sure was necessary (e.g. having the "parent" refresh driver always run after any "child" drivers -- is this to avoid flicker/out-of-order rendering?).

https://github.com/vvuk/gecko-dev/commit/dfec1b1dd44466dc002b72e3a307d4f188e7a21d
- Adds vsync observers to nsIWidget
- Makes RefreshDriverTimers refcounted
- Adds WidgetVsyncRefreshDriverTimer to RefreshDriver, and preferentially uses it if a widget is available
- Teaches nsBaseWidget how to connect a widget to vsync; either the toplevel widget, a PVSync in the case of a child process, or directly to a VsyncSource.  (This patch just pretends it's a child refresh driver for simplicity, that's fixed later on)

https://github.com/vvuk/gecko-dev/commit/712c409efed0abce84eb03e6339d559249ac80ee
- Moves the Compositor to listen to the widget vsync directly

https://github.com/vvuk/gecko-dev/commit/f84ba2e1d8b395cecab494f8ff09f695e468b13f
- Removes CompositorVsyncDispatcher

https://github.com/vvuk/gecko-dev/commit/b920d8fd3f6ef3342ead6dab3f29e6297e77b55c
- Makes VsyncSource::Display have AddVsyncObserver/RemoveVsyncObserver methods to add things directly to a list
- makes Widget and VsyncParent use the VsyncSource directly (soon to be VsyncSource::Display, once that's refcounted) instead of going through an intermediate dispatcher
- Removes RefreshDriverVsyncDispatcher

Does anything in the above set of changes look objectionable?  What's a good way to test that everything is working?

Note that this doesn't look at the non-hw vsync case at all; I'm assuming that in that case, we'll have a software vsync source.  That needs to be hooked up, though.
So I can remember to look at this.
Flags: needinfo?(mchang)
Whiteboard: gfx-noted → [gfx-noted][vrm2]
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #3)
> Does anything in the above set of changes look objectionable? 

Mostly looks good. Probably the major one is updating vsync off main thread. We really should keep it main thread.

> What's a good
> way to test that everything is working?

I'd prefer if we took it step by step rather than this big change. Let's make sure we can do the widget stuff in one step and keeping the dispatchers. Then migrate the compositor listening to the widget, then finally migrate the refresh timers. 

We should be able to see, in this case, that the refresh timers are ticking at 60 hz while the compositor is going at 75 hz. Probably best way to test is to make sure the timestamps you're getting at both the compositor and refresh driver are teh same that we're sending out in the VsyncSource.

> 
> Note that this doesn't look at the non-hw vsync case at all; I'm assuming
> that in that case, we'll have a software vsync source.  That needs to be
> hooked up, though.

Yeah, it shouldn't matter. There shouldn't be a different path. the vsync source will just come from https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/SoftwareVsyncSource.cpp. Unless you mean the non-silk path. In that case, it doesn't matter. I was planning to delete non-silk paths in Gecko 43.
Flags: needinfo?(mchang)
Here's a RB review for the current WIP patch: https://reviewboard.mozilla.org/r/16375/

This is a combined diff of the work that's visible on https://github.com/vvuk/gecko-dev/commits/vsync-wip
Flags: needinfo?(mchang)
Some comments left on the review board request.
Flags: needinfo?(mchang)
Whiteboard: [gfx-noted][vrm2] → [gfx-noted][webvr]
Updated work is at https://github.com/vvuk/gecko-dev/tree/vsync with reviews hopefully happening at https://github.com/vvuk/gecko-dev/pull/29 -- that might turn into a RB review if either mchang or roc prefer that.
Add more .gitignore/.hgignore entires
From 87f3ba62f0eedab204b1cea041ea398765f28d7d Mon Sep 17 00:00:00 2001
nsID stringification helper for logging
From 82519ccddc288ae8d12ed97ad1a7a3265debf6f6 Mon Sep 17 00:00:00 2001
Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator
From 5de25d7c837b995cc95771004be111453fa80cb3 Mon Sep 17 00:00:00 2001
 calls
Bug 1184283 - Move vsync notification/distribution to nsIWidget and
From 831de6fd89c66a3e72ef9bd5d04aef80ef9a6800 Mon Sep 17 00:00:00 2001
 nsBaseWidget
All incoming vsync notifications (from hardware or other sources) come
into a widget, which will then distribute it to its child widgets,
its refresh drivers, compositors, etc.

This gives us a single place to override the current vsync source,
so that we can handle per-monitor vsync, VR HMDs, etc.

Child processes have their PuppetWidgets listen to vsync directly
for the appropriate source (instead of going through their
parent cross-process widget).
Attachment #8673760 - Flags: review?(roc)
Attachment #8673760 - Flags: review?(mchang)
Bug 1196366, add support for Oculus 0.7 runtime
From 99bddaa5693a4d194f990417f96c8738cac24cad Mon Sep 17 00:00:00 2001
Bug 1184283 - Have HMDs create their own VsyncDisplay, and have
From a1b4d8891f5c6188cd904a228a74be33db463efb Mon Sep 17 00:00:00 2001
 widget keep track of currently attached HMD
Attachment #8673762 - Flags: review?(mchang)
Bug 1184283 - Silk docs update
From f1ee226f433a25b719b72c5ef3e2e677c1ba5ee8 Mon Sep 17 00:00:00 2001
Attachment #8673763 - Flags: review?(mchang)
Bug 1184283 - Great Vsync Renaming
From 78ab6c47f28d476709ba144535b5d0e7e250c591 Mon Sep 17 00:00:00 2001
Attachment #8673764 - Flags: review?(mchang)
build fixup
From c663bc0fd1ef5490e909cf606b46f9f87fdbe892 Mon Sep 17 00:00:00 2001
Okay, here we go. I still have some work to do -- there's a NUWA issue where PVSync is being created too early.  I'm setting up a b2g emulator build env to fix that.  But I'll do those fixes as patches on top of this, so reviews can start happening.
Comment on attachment 8673760 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

https://reviewboard.mozilla.org/r/21985/#review19747

Just a few questions:

Do we explicitly call unregister display somewhere in the VsyncSource, or are you waiting for true multi monitor support.

What's the role of the mLastActiveTimer for the nsRefreshDriver?

I'm still confused as to why the nsBaseWidget VsyncForwardingObserver needs both an mWidget and mObserved widget. What's the difference? Or does that only exist on the root base widget?

::: gfx/layers/ipc/CompositorParent.cpp:306
(Diff revision 1)
> -  // The CompositorVsyncDispatcher is cleaned up before this in the nsBaseWidget, which stops vsync listeners
> +  // just in case we somehow got destroyed without a Destroy() call

Does this actually happen? We already assert that we don't have an mVsyncObserver one line above. Do we introduce a new way to shut down the compositor somewhere?

::: gfx/layers/ipc/CompositorParent.cpp:470
(Diff revision 1)
> -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> +  mCompositorParent->GetWidget()->AddVsyncObserver(mVsyncObserver);

Which thread do we observe vsync on now? Is it both the compositor thread & main thread or since it's the widget, only on the main thread? Can we assert which thread?

::: gfx/tests/gtest/TestVsync.cpp:106
(Diff revision 1)
> +#if 0

Can we clean up the tests instead of #if 0? Can we modify this to ensure that a widget gets the vsync notifications, or is that too complicated?

::: gfx/thebes/SoftwareVsyncSource.cpp:48
(Diff revision 1)
> -  NotifyVsync(mozilla::TimeStamp::Now());
> +  OnVsync(mozilla::TimeStamp::Now());

Does InternalEnableVsync need to exist? Can you just post an OnVsync message in EnableVsync instead?

::: gfx/thebes/VsyncSource.cpp:34
(Diff revision 1)
> -  // Called on the vsync thread
> +  { // scope lock

Do we have a policy on which threads we can add/remove vsync observers? I'm kind of worried about making a copy of observers in OnVsync() and having an observer remove itself from the array. Can we get a case where an observer is being notified on the vsync thread and gets removed on the main thread and that disables vsync on the main thread while the vsync thread is still going?

::: gfx/thebes/VsyncSource.cpp:53
(Diff revision 1)
> +  if (found)

nit: add enclosing {}.

::: gfx/thebes/VsyncSource.cpp:53
(Diff revision 1)
> +  if (found)

nit: add enclosing {}

::: gfx/thebes/VsyncSource.cpp:129
(Diff revision 1)
> +  return -1;

nit: make this a kDisplayNotFound const.

::: gfx/thebes/VsyncSource.cpp:140
(Diff revision 1)
> +  int32_t existingDisplayIndex = GetDisplayIndex(id);
> +  unused << existingDisplayIndex;

nit: wrap this in a bool DoesDisplayExist(),which can be used in Register/Unregister display.

::: gfx/thebes/VsyncSource.cpp:166
(Diff revision 1)
> +    return nullptr;

would it be better to just return the global display here? Or do we handle this case everywhere we call GetDisplay?

::: gfx/thebes/gfxAndroidPlatform.cpp:444
(Diff revision 1)
> -      DisableVsync();
> +    DisableVsync();

Do we have to unregister the global display here?

::: gfx/thebes/gfxAndroidPlatform.cpp:484
(Diff revision 1)
> -    nsRefPtr<GonkVsyncSource> vsyncSource = new GonkVsyncSource();
> +    nsRefPtr<VsyncDisplay> display = new GonkDisplay(VsyncSource::kGlobalDisplayID);

nit: rename to globalDisplay.

::: gfx/thebes/gfxPlatform.cpp:626
(Diff revision 1)
> +        if (gPlatform->mVsyncSource) {

When would we not have a vsync source? I think without a vsync source, nothing would render?

::: gfx/thebes/gfxPlatform.cpp
(Diff revision 1)
> - * The preference "layout.frame_rate" has 3 meanings depending on the value:

nit: Please keep this comment and re-add itabove IsInLayoutAsapMode.

::: gfx/thebes/gfxWindowsPlatform.cpp:8
(Diff revision 1)
> +#pragma warning(once: 4509)

nit: Move this out to a different bug.

::: gfx/thebes/gfxWindowsPlatform.cpp:2709
(Diff revision 1)
> -
> +    

nit: delete whitespace.

::: gfx/thebes/gfxWindowsPlatform.cpp:2753
(Diff revision 1)
> -
> +    

nit: whitespace.

::: gfx/thebes/gfxWindowsPlatform.cpp:2792
(Diff revision 1)
> -  return d3dVsyncSource.forget();
> +  

nit: whitespace.

::: gfx/thebes/gfxWindowsPlatform.cpp:2796
(Diff revision 1)
> +#if 0

nit: delete.

::: layout/base/nsRefreshDriver.cpp:333
(Diff revision 1)
> -      , mProcessedVsync(true)
> +    

nit: whitespace.

::: layout/base/nsRefreshDriver.cpp:340
(Diff revision 1)
>          // Compress vsync notifications such that only 1 may run at a time

We should only have to do this in the parent process. The child process should already have compressed vsync notifications from the 'compress' keyword in IPDL. Please add back the parent process assert.

::: layout/base/nsRefreshDriver.cpp:406
(Diff revision 1)
> -    // longer tick this timer.
> +    // the scheduled TickRefreshDriver() runs. Check mVsyncRefreshDriverTimer

nit: Can we assert this is true?

::: layout/base/nsRefreshDriver.cpp:449
(Diff revision 1)
> - * Since the content process takes some time to setup
> + * PreciseRefreshDriverTimer schedules ticks based on the current time

Do we even have a PreciseRefreshDriverTimer anymore? That should've been deleted.

::: layout/base/nsRefreshDriver.cpp:753
(Diff revision 1)
> +  // .... if it's not being displayed, should we just use the throttled timer?

Yes, I think we should use the throttled timer.

::: layout/ipc/VsyncChild.cpp:85
(Diff revision 1)
> +  // isn't this supposed to be on the PBackground thread?!

PBackground goes from the PBacgrkound thread on the parent process to the main thread in the content process :(.

::: widget/nsBaseWidget.cpp:1088
(Diff revision 1)
> +class VsyncForwardingObserver final : public mozilla::gfx::VsyncObserver {

Does this only exist on the root widget then forwards it to non-root widgets?

::: widget/nsBaseWidget.cpp:1159
(Diff revision 1)
> +    // Child

nit: just wrap around an else?

::: widget/nsBaseWidget.cpp:1233
(Diff revision 1)
> +  // XXX can this be a bare pointer?

What's the difference between the owner widget versus observed widget?

::: widget/nsBaseWidget.cpp:1330
(Diff revision 1)
> +  if (unpause && mIncomingVsyncObserver) {

When do we not have an mIncomingVsyncObserver?
Attachment #8673760 - Flags: review?(mchang)
Comment on attachment 8673762 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

https://reviewboard.mozilla.org/r/21989/#review19819

::: gfx/vr/gfxVR.cpp:96
(Diff revision 1)
> +  mDisplayID = VsyncSource::kGlobalDisplayID;

Does this work? Shouldn't it get it's own separate display id? Or if there is a generic "VR" display id, can we do that instead?

::: gfx/vr/gfxVROculus.cpp:311
(Diff revision 1)
> +  if (mVsyncDisplay) {

Do you ever expect to have an HMD Oculus without a vsync display after these patches land? If so, just assert instead?
Attachment #8673762 - Flags: review?(mchang)
Comment on attachment 8673764 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/21993/#review19821
Attachment #8673764 - Flags: review?(mchang) → review+
https://reviewboard.mozilla.org/r/21989/#review19819

> Does this work? Shouldn't it get it's own separate display id? Or if there is a generic "VR" display id, can we do that instead?

This is basically saying that, unless a subclass overrides it, any VR devices will just use the regular global display ID.  I could introduce another constant that says "just keep using whatever you're using" too.

> Do you ever expect to have an HMD Oculus without a vsync display after these patches land? If so, just assert instead?

Yeah, good point.
Comment on attachment 8673763 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/21991/#review19823

::: gfx/doc/Silk.md:26
(Diff revision 1)
> -Hardware vsync events from (1), occur on a specific **Display** Object.
> +Hardware vsync events from (1), occur on a specific **VsyncDisplay** Object.

Does this also need the great global renaming applied?

::: gfx/doc/Silk.md:58
(Diff revision 1)
> +When multiple displays are present on a system, multiple **VsyncDisplay**s should exist, one for each display.

note: Since this isn't implemented yet, but I think we've had previous discussions (forget where), that instead of trying to guess which monitor to listen to if a widget is across monitors, we should just listen to the global display until the widget is entirely in 1 display.

::: gfx/doc/Silk.md:122
(Diff revision 1)
> -Since the **RefreshTimer** has a lifetime of the process, we only need to create a single **RefreshTimerVsyncDispatcher** per **Display** when Firefox starts.
> +We create a **WidgetVsyncRefreshTimer** per **RefreshDriver**, because it refers to a specific screen.

Oh that's a new one, I might have missed that during the review. Is there a reason we can't consolidate it to 1 WidgetVsyncRefreshTimer per display?
Attachment #8673763 - Flags: review?(mchang)
https://reviewboard.mozilla.org/r/21985/#review19747

There isn't currently any display unregistering (except for VR).  It's not really needed right now.  It will be once we have multi monitor support, especially when we support hot-plugging monitors.  (When that happens, the "global" display will have to be smart in case it gets unplugged.)

mLastActiveTimer is going away.  That was an attempt at an optimization to avoid recreating the timer class all the time for short-lived things (like the caret -- since it blinks only once per second, it goes "inactive" after every blink, and then becomes active.  So the RefreshDriver bounces between active/inactive timers every second).

VsyncForwardingObserver has mWidget -- which is the widget it notifies when a vsync event happens.  mObservedWidget is the (parent) widget it's listening to incoming vsync events for, if it's listening to a parent and not a VsyncSource directtly.  That is,  vsync events flow like this:  mObservedWidget -> VsyncForwardingObserver -> mWidget

> Does this actually happen? We already assert that we don't have an mVsyncObserver one line above. Do we introduce a new way to shut down the compositor somewhere?

Er, indeed.  No need for this.

> Does InternalEnableVsync need to exist? Can you just post an OnVsync message in EnableVsync instead?

It doesn't really, but it has symmetry with InternalDisableVsync (and it also sends the TimeStamp::Now() parameter, which we could post anyway so that's not a big deal).  I'd rather keep it for cleanliness.

> Do we have a policy on which threads we can add/remove vsync observers? I'm kind of worried about making a copy of observers in OnVsync() and having an observer remove itself from the array. Can we get a case where an observer is being notified on the vsync thread and gets removed on the main thread and that disables vsync on the main thread while the vsync thread is still going?

Hmm.. I'm not sure what the issue is -- having an observer remove itself from the array is exactly why a copy is made.  Vsync being disabled will always happen after any processing that's currently taking place happens; we never aggressively kill any vsync thread, instead we tell it to exit.  So basically any thread can add/remove vsync observers.

> nit: wrap this in a bool DoesDisplayExist(),which can be used in Register/Unregister display.

It can't be -- UnregisterDisplay and GetDisplay actually use the resulting existingDisplayIndex value, so DoesDisplayExist wouldn't help (or it would cause us to search twice).  The only place it could be used is in RegisterDisplay.

> When would we not have a vsync source? I think without a vsync source, nothing would render?

Yep. I'd like to actually clean up mVsyncSource/mVsyncManager stuff in a followup patch -- we can simplify it a lot, and we don't need separate GetHardwareVsync/GetSoftwareVsync etc.  So I'll leave this as is for now but will do a followup.

> Do we even have a PreciseRefreshDriverTimer anymore? That should've been deleted.

Whoops, stale comment.

> Does this only exist on the root widget then forwards it to non-root widgets?

Nope, it exists on every widget -- it listens to incoming vsync events (from widgets or vsync sources) and forwards to whoever is listening to vsync on that widget.  Tehcnically, nsBaseWidget *could* be a VsyncForwardingObserver, and I might actually make that change...
Comment on attachment 8673760 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

https://reviewboard.mozilla.org/r/21985/#review19789

Generally looks great. Some minor comments.

::: gfx/thebes/VsyncSource.cpp:36
(Diff revision 1)
> -
> +    if (!mVsyncObservers.Contains(aObserver)) {

Any particular reason why you check Contains first? Might be simpler to not do that.

::: gfx/thebes/VsyncSource.cpp:180
(Diff revision 1)
> +  }

I think you can write aDisplays.AppendElements(mDisplays).

::: gfx/thebes/gfxPlatform.h:653
(Diff revision 1)
> +     * Initialized software vsync; can be overriden by the platform, but a generic

"Initialize"

::: gfx/thebes/gfxWindowsPlatform.cpp:2709
(Diff revision 1)
> -
> +    

remove trailing whitespace

::: layout/base/nsRefreshDriver.cpp:333
(Diff revision 1)
> -      , mProcessedVsync(true)
> +    

remove trailing whitespace

::: layout/base/nsRefreshDriver.cpp:731
(Diff revision 1)
> -    double rate = GetRegularTimerInterval(&isDefault);
> +    // don't recreate if we have a timer and its widget widget is the same

"widget widget"

::: layout/base/nsRefreshDriver.cpp:746
(Diff revision 1)
> +    printf_stderr("RefreshDriver %p Creating widget vsync timer for widget %p\n", this, w);

remove this printf

::: layout/base/nsRefreshDriver.cpp:757
(Diff revision 1)
> -      sRegularRateTimer = new StartupRefreshDriverTimer(rate);
> +    sRegularRateTimer = new StartupRefreshDriverTimer(rate);

I suggest leaving this as-is for now, but in a followup we should try removing StartupRefreshDriverTimer and using the throttled timer instead ... unless Mason knows why we shouldn't do that.

::: widget/nsBaseWidget.h:281
(Diff revision 1)
> +  bool RemoveVsyncObserver(mozilla::gfx::VsyncObserver *aObserver) override;

Why are these returning bool when no-one uses those results?

::: widget/nsBaseWidget.h:290
(Diff revision 1)
> +  void ForwardVsyncNotification(mozilla::TimeStamp aVsyncTimestamp);

It would be helpful to document what UpdateVsyncObserver and ForwardVsyncNotification do.

::: widget/nsBaseWidget.cpp:1287
(Diff revision 1)
> +}

Why do we have this function? Can't we just call GetTopLeveLWidget directly?

enerally
Attachment #8673760 - Flags: review?(roc)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #24)
> https://reviewboard.mozilla.org/r/21985/#review19747
> 
> There isn't currently any display unregistering (except for VR).  It's not
> really needed right now.  It will be once we have multi monitor support,
> especially when we support hot-plugging monitors.  (When that happens, the
> "global" display will have to be smart in case it gets unplugged.)

Hmm, sorry, I should've been more clear. We register the global display when we create
the vsync source, but we never unregister it. I only saw calls to shutdown(), which doesn't
unregister it. Are we just assuming it will get cleaned up in shutdown()? At least for cleanliness
sake, I'd prefer we explicitly unregister the global display and assert that there are no more
displays in VsyncSource::Shutdown.

> 
> mLastActiveTimer is going away.  That was an attempt at an optimization to
> avoid recreating the timer class all the time for short-lived things (like
> the caret -- since it blinks only once per second, it goes "inactive" after
> every blink, and then becomes active.  So the RefreshDriver bounces between
> active/inactive timers every second).

Please delete this then before landing.

> 
> VsyncForwardingObserver has mWidget -- which is the widget it notifies when
> a vsync event happens.  mObservedWidget is the (parent) widget it's
> listening to incoming vsync events for, if it's listening to a parent and
> not a VsyncSource directtly.  That is,  vsync events flow like this: 
> mObservedWidget -> VsyncForwardingObserver -> mWidget

Ahh ok, that makes more sense. Please add this comment above the member declarations.

> > Do we have a policy on which threads we can add/remove vsync observers? I'm kind of worried about making a copy of observers in OnVsync() and having an observer remove itself from the array. Can we get a case where an observer is being notified on the vsync thread and gets removed on the main thread and that disables vsync on the main thread while the vsync thread is still going?
> 
> Hmm.. I'm not sure what the issue is -- having an observer remove itself
> from the array is exactly why a copy is made.  Vsync being disabled will
> always happen after any processing that's currently taking place happens; we
> never aggressively kill any vsync thread, instead we tell it to exit.  So
> basically any thread can add/remove vsync observers.

ok, please update the docs to include this.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> ::: layout/base/nsRefreshDriver.cpp:757
> (Diff revision 1)
> > -      sRegularRateTimer = new StartupRefreshDriverTimer(rate);
> > +    sRegularRateTimer = new StartupRefreshDriverTimer(rate);
> 
> I suggest leaving this as-is for now, but in a followup we should try
> removing StartupRefreshDriverTimer and using the throttled timer instead ...
> unless Mason knows why we shouldn't do that.

I was mostly only worried about start up time. The startup timer ticks at a faster
rate, which would in theory keep start up times the same as pre-silk. I was worried about
slowing start up time with the throttled timer as the time it takes to setup
the PVsync connection could be a while, and we'd miss a few ticks on the throttled
refresh driver. We could probably measure this to see if there is any
difference in start up time.
Is there any progress on this?
Yes.  Latest try run looks like this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51ae9c4595c

Currently looking at dom/svg/test/test_pathAnimInterpolation.xhtml on linux
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #29)
> Currently looking at dom/svg/test/test_pathAnimInterpolation.xhtml on linux

I'm studying this in rr right now.
The problem is that the main thread event queue is growing huge, which makes the ping/pong protocol used by the test runner when a test finishes take a really long time to complete, causing test timeouts. For example when we send the SPPingService "pong" message from SpecialPowersObserver.prototype.receiveMessage, there are 1226 pending events in the queue. When we've finally processed all those events, there are 2463 more events in the queue.

Inspecting a sample of these events shows that there are a lot of consecutive NS_NewRunnableMethodImpl<VsyncForwardingObserver::NotifyVsync>s.
So the problem seems to be that there's no throttling on the paths being used here (non-e10s).

SoftwareVsyncSource::OnVsync runs periodically off a timer on a non-main thread. Each time it runs, VsyncForwardingObserver::NotifyVsync takes the !NS_IsMainThread path and dispatches a runnable to run itself on the main thread. When that runnable runs on the main thread, we go through nsBaseWidget::ForwardVsyncNotification to nsRefreshDriver's InnerVsyncObserver::NotifyVsync, and since we're on the main thread no throttling code runs, we just go straight into mTimer->TickRefreshDriver. So if a refresh driver tick takes a while, SoftwareVsyncSource::OnVsync will run multiple times during the tick, and some more consecutive VsyncForwardingObserver::NotifyVsyncs will be added to the event queue, hence the queue can get longer and longer.
It seems to me that VsyncForwardingObserver::NotifyVsync should not be dispatching itself to the main thread at all.
Attached patch fix (obsolete) — — Splinter Review 
This fixes that test failure for me. Note the additional race-prevention fix that I think we need too.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> So the problem seems to be that there's no throttling on the paths being
> used here (non-e10s).
> 
> SoftwareVsyncSource::OnVsync runs periodically off a timer on a non-main
> thread. Each time it runs, VsyncForwardingObserver::NotifyVsync takes the
> !NS_IsMainThread path and dispatches a runnable to run itself on the main
> thread. When that runnable runs on the main thread, we go through
> nsBaseWidget::ForwardVsyncNotification to nsRefreshDriver's
> InnerVsyncObserver::NotifyVsync, and since we're on the main thread no
> throttling code runs, we just go straight into mTimer->TickRefreshDriver. So
> if a refresh driver tick takes a while, SoftwareVsyncSource::OnVsync will
> run multiple times during the tick, and some more consecutive
> VsyncForwardingObserver::NotifyVsyncs will be added to the event queue,
> hence the queue can get longer and longer.

Ahh thanks for finding that. This looks like the equivalent of [1] in master. Just an FYI, see bug 1210261. As it stands in master today, we compress IPC messages on the content side and throttle on the parent side. Previously, we would backlog up to 1 vsync message on the parent process, but we want to switch it to backlogging none and slip down to 30 FPS if we miss a vsync.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?case=true&from=nsRefreshDriver.cpp#328
See Also: → 1210261
Try results are a bit better but there are still some timeouts that look due to these patches.
Two more try server runs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6754e6dc487

And this is removing the inner observer class, and doing some AddRef/Release virtual goop so that we can just have one class:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=317f0d39f8d1

I need to get rr set up so that I can look at those timeouts closer; I've been trying with just gdb, but that's being a pain.
So in debug builds, we have huge main thread delay -- when the animation in the devtools/client/animationinspector/test/doc_simple_animation.html test is running, opening up devtools causes main thread refreshes to be delayed up to 500ms (a runnable is scheduled by the vsync notification and then doesn't execute for 500ms.. we end up skipping scheduling a new one a ton along the way).

The animation itself runs smoothly on the content side.  Here's a profile of this: http://mzl.la/1OPfnXz

You can see the difference between Content frames and GeckoMain frames.  This could be debug only issues, but the profile shows a bunch of time spent in scripts which doesn't make much sense..
(Hmm, I thought I commented on this already, but I think that failed because bugzilla 2FA failed me)

The issue was that I changed some prefs checking around to use gfxPrefs, and I left out a == 0.  So, the compositor was always running in "ASAP" mode, which was causing all sorts of resource contention and problems.  With that fixed, we have this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48db5ca39ed3

The Linux crashes in IME code referencing a null widget are very strange, and I'm going to rebase to see if they're related to something else.  A few OSX leaks that I need to look into.  I am not at all sure what's going on with the b2g ics debug emulator though.  Nothing in the logs looks relevant that I can see, but something bad is going on.
Might be finally, finally getting very close:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e54337dfe53a&selectedJob=14829602

Simplified a lot of the ownership and lifetimes.. everything is now properly reference counted.  I'll do more cleanup tomorrow and look into the remaining test failures; many or most of them look known or intermittent.
Comment on attachment 8673757 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21979/diff/1-2/
Attachment #8673757 - Attachment description: MozReview Request: Add more .gitignore/.hgignore entires → MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls
Comment on attachment 8673758 [details]
MozReview Request: Bug 1184283 - add pure virtual refcounting macro to nsISupportsImpl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21981/diff/1-2/
Attachment #8673758 - Attachment description: MozReview Request: nsID stringification helper for logging → MozReview Request: Bug 1184283 - add pure virtual refcounting macro to nsISupportsImpl
Comment on attachment 8673759 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21983/diff/1-2/
Attachment #8673759 - Attachment description: MozReview Request: Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator → MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files
Comment on attachment 8673760 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21985/diff/1-2/
Attachment #8673760 - Attachment description: MozReview Request: Bug 1184283 - Move vsync notification/distribution to nsIWidget and → MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager
Attachment #8673760 - Flags: review?(roc)
Attachment #8673760 - Flags: review?(mchang)
Comment on attachment 8673761 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21987/diff/1-2/
Attachment #8673761 - Attachment description: MozReview Request: Bug 1196366, add support for Oculus 0.7 runtime → MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)
Comment on attachment 8673762 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21989/diff/1-2/
Attachment #8673762 - Attachment description: MozReview Request: Bug 1184283 - Have HMDs create their own VsyncDisplay, and have → MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent
Attachment #8673762 - Flags: review?(mchang)
Comment on attachment 8673763 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21991/diff/1-2/
Attachment #8673763 - Attachment description: MozReview Request: Bug 1184283 - Silk docs update → MozReview Request: Bug 1184283 - Widget vsync source/observer
Attachment #8673763 - Flags: review?(mchang)
Comment on attachment 8673764 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21993/diff/1-2/
Attachment #8673764 - Attachment description: MozReview Request: Bug 1184283 - Great Vsync Renaming → MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer
Attachment #8673765 - Attachment description: MozReview Request: build fixup → MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync
Comment on attachment 8673765 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21995/diff/1-2/
From 0b3eb8f3e83f09e0af4c472725153ee790802190 Mon Sep 17 00:00:00 2001
---
 dom/ipc/ContentChild.cpp |  6 ++++--
 dom/ipc/PBrowser.ipdl    |  1 +
 dom/ipc/TabChild.cpp     |  4 ++++
 dom/ipc/TabParent.cpp    | 10 ++++++++--
 widget/PuppetWidget.cpp  | 15 +++++++++++++++
 widget/PuppetWidget.h    |  5 +++++
 6 files changed, 37 insertions(+), 4 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32439/
From fde9cd24a6dbcdd5d6ea09d06b962b4698e82c6a Mon Sep 17 00:00:00 2001
testing needs some love
---
 gfx/tests/gtest/TestVsync.cpp | 108 +++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 74 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32441/
https://reviewboard.mozilla.org/r/21991/#review19823

> Does this also need the great global renaming applied?

Let me do another pass on the docs..

> note: Since this isn't implemented yet, but I think we've had previous discussions (forget where), that instead of trying to guess which monitor to listen to if a widget is across monitors, we should just listen to the global display until the widget is entirely in 1 display.

This patch doesn't handle multiple-monitor scenarios at all yet, though it sets things up easily to do so.

> Oh that's a new one, I might have missed that during the review. Is there a reason we can't consolidate it to 1 WidgetVsyncRefreshTimer per display?

Yes, because this timer listens to the widget only -- it has no knowledge of what the widget is listening to.
https://reviewboard.mozilla.org/r/21993/#review19821

RB thinks this is flagged as r+ based on an old version of the patch.  It should just be r? right now, but I think only Mason can unflag that.
https://reviewboard.mozilla.org/r/21985/#review19747

> would it be better to just return the global display here? Or do we handle this case everywhere we call GetDisplay?

We handle it wherever we call GetDisplay (which isn't very many places right now).  I could go either way on this; I guess we could return the default display and warn?  It's just a strange situation where we expect to find a display ID and don't.  We can look into this when multi-monitor happens.

> Do we have to unregister the global display here?

Not sure what this comment is referring to (thanks reviewboard!), but the code right now is correct - if vsync fails to enable, we fall back to software vsync.  The global display doesn't get registered in that case (here anyway).

> We should only have to do this in the parent process. The child process should already have compressed vsync notifications from the 'compress' keyword in IPDL. Please add back the parent process assert.

This code will run on both child and parent -- I can do the "compression" on just the parent if needed, but I'm not sure how 'compress' in IPDL will help when the child takes a long time to process a vsync in the refresh driver?

> nit: Can we assert this is true?

This shoould not be the case any more; this comment can go away.

> Nope, it exists on every widget -- it listens to incoming vsync events (from widgets or vsync sources) and forwards to whoever is listening to vsync on that widget.  Tehcnically, nsBaseWidget *could* be a VsyncForwardingObserver, and I might actually make that change...

This code changed to simplify lifetimes.

> What's the difference between the owner widget versus observed widget?

This all got simplified -- but the difference is, observed widget is the one we're listening to for incoming vsync signals (if we're listening to a widget at all); owner widget is the widget that's this forwardingobserver's owner.
[new review requests coming shortly; thanks reviewboard!]
Attachment #8673760 - Attachment is obsolete: true
Attachment #8673760 - Flags: review?(roc)
Attachment #8673760 - Flags: review?(mchang)
Attachment #8673762 - Attachment is obsolete: true
Attachment #8673762 - Flags: review?(mchang)
Attachment #8673763 - Attachment is obsolete: true
Attachment #8673763 - Flags: review?(mchang)
From f20e01220547e0826e80ffcf9645a2b87dbc2bd6 Mon Sep 17 00:00:00 2001
---
 gfx/thebes/gfxDWriteFontList.cpp |  1 +
 gfx/thebes/gfxFontUtils.cpp      | 14 ++------------
 gfx/thebes/gfxUtils.cpp          | 11 +++++++++++
 gfx/thebes/gfxUtils.h            |  6 ++++++
 4 files changed, 20 insertions(+), 12 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32479/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32479/
Attachment #8712256 - Flags: review?(jmuizelaar)
From 8078484110cf1a7855ac99f5dd77afca762b73ba Mon Sep 17 00:00:00 2001
---
 xpcom/glue/nsISupportsImpl.h | 9 +++++++++
 1 file changed, 9 insertions(+)

Review commit: https://reviewboard.mozilla.org/r/32481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32481/
Attachment #8712257 - Flags: review?(nfroyd)
From eea448ffd791f976e426d639267dff89735d7eb6 Mon Sep 17 00:00:00 2001
Add new gfx::VsyncSource, gfx::VsyncObserver, gfx::VsyncManager,
gfx::SoftwareVsyncSource.
These are refcounted classes, with each source keyed by a source uuid,
all managed by a VsyncManager.
---
 gfx/doc/Silk.md                    | 173 +++++++----------
 gfx/thebes/SoftwareVsyncSource.cpp | 148 ---------------
 gfx/thebes/SoftwareVsyncSource.h   |  64 -------
 gfx/thebes/VsyncSource.cpp         | 147 ---------------
 gfx/thebes/VsyncSource.h           |  82 --------
 gfx/thebes/gfxVsync.cpp            | 370 +++++++++++++++++++++++++++++++++++++
 gfx/thebes/gfxVsync.h              | 194 +++++++++++++++++++
 widget/VsyncDispatcher.cpp         | 201 --------------------
 widget/VsyncDispatcher.h           |  98 ----------
 9 files changed, 632 insertions(+), 845 deletions(-)
 delete mode 100644 gfx/thebes/SoftwareVsyncSource.cpp
 delete mode 100644 gfx/thebes/SoftwareVsyncSource.h
 delete mode 100644 gfx/thebes/VsyncSource.cpp
 delete mode 100644 gfx/thebes/VsyncSource.h
 create mode 100644 gfx/thebes/gfxVsync.cpp
 create mode 100644 gfx/thebes/gfxVsync.h
 delete mode 100644 widget/VsyncDispatcher.cpp
 delete mode 100644 widget/VsyncDispatcher.h

Review commit: https://reviewboard.mozilla.org/r/32483/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32483/
Attachment #8712258 - Flags: review?(roc)
Attachment #8712258 - Flags: review?(mchang)
From 9cc6d6e42b4567f5452da1dc87ef73bda35b7cf4 Mon Sep 17 00:00:00 2001
---
 gfx/layers/composite/AsyncCompositionManager.cpp |   5 +-
 gfx/thebes/gfxAndroidPlatform.cpp                |  94 +++---
 gfx/thebes/gfxAndroidPlatform.h                  |   2 +-
 gfx/thebes/gfxPlatform.cpp                       |  63 ++--
 gfx/thebes/gfxPlatform.h                         |  43 ++-
 gfx/thebes/gfxPlatformMac.cpp                    | 242 +++++++-------
 gfx/thebes/gfxPlatformMac.h                      |   4 +-
 gfx/thebes/gfxWindowsPlatform.cpp                | 404 +++++++++++------------
 gfx/thebes/gfxWindowsPlatform.h                  |   2 +-
 gfx/thebes/moz.build                             |   6 +-
 widget/gonk/HwcComposer2D.cpp                    |   6 +-
 widget/gonk/HwcComposer2D.h                      |   5 +
 widget/gonk/nsScreenManagerGonk.cpp              |   8 +-
 13 files changed, 449 insertions(+), 435 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32485/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32485/
Attachment #8712259 - Flags: review?(roc)
Attachment #8712259 - Flags: review?(mchang)
From e8ccb71e9d727663cf46392ad16448168993a4f8 Mon Sep 17 00:00:00 2001
---
 gfx/thebes/gfxDWriteCommon.h      |  1 -
 gfx/thebes/gfxHarfBuzzShaper.h    | 10 +++++-----
 gfx/thebes/gfxPlatform.cpp        |  4 ++--
 gfx/thebes/gfxScriptItemizer.cpp  |  2 ++
 gfx/thebes/gfxUtils.cpp           |  5 ++++-
 gfx/thebes/gfxWindowsPlatform.cpp |  3 +++
 6 files changed, 16 insertions(+), 9 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32487/
Attachment #8712260 - Flags: review?(mchang)
From 286c86446d94ef037d573d6ca160646c40904da7 Mon Sep 17 00:00:00 2001
---
 ipc/glue/BackgroundChildImpl.cpp  |  4 +--
 ipc/glue/BackgroundChildImpl.h    |  2 +-
 ipc/glue/BackgroundParentImpl.cpp |  4 +--
 ipc/glue/BackgroundParentImpl.h   |  2 +-
 ipc/glue/PBackground.ipdl         |  4 ++-
 layout/ipc/PVsync.ipdl            |  6 ++--
 layout/ipc/VsyncChild.cpp         | 61 ++++++++++++++++++++++++++-------------
 layout/ipc/VsyncChild.h           | 40 +++++++++++++------------
 layout/ipc/VsyncParent.cpp        | 37 ++++++++++++++----------
 layout/ipc/VsyncParent.h          | 18 ++++++++----
 10 files changed, 109 insertions(+), 69 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32489/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32489/
Attachment #8712261 - Flags: review?(roc)
Attachment #8712261 - Flags: review?(mchang)
From 56797f8b982b340151770938af74f8b6cef1ed14 Mon Sep 17 00:00:00 2001
Adds add/remove vsync observer to nsIWidget; Implement a VsyncSource in
nsBaseWidget

There is an internal VsyncForwardingObserver in nsBaseWidget that is
both a VsyncSource and a VsyncObserver.  It observes whatever the widget
is currently listening to (a parent widget, a direct source id which
might be remote, etc.) and forwards it to anything that is related to
that widget (Compositors, RefreshDrivers, other child widgets).

We track widget parent changes so that we can listen to the new parent
appropriately.
---
 widget/moz.build        |   2 -
 widget/nsBaseWidget.cpp | 515 ++++++++++++++++++++++++++++++++++++++++++------
 widget/nsBaseWidget.h   |  39 +++-
 widget/nsIWidget.h      |  20 +-
 4 files changed, 501 insertions(+), 75 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32491/
Attachment #8712262 - Flags: review?(roc)
Attachment #8712262 - Flags: review?(mchang)
From e59d0350aedf39fc532b0bbefcedd6a7e15f0c03 Mon Sep 17 00:00:00 2001
---
 layout/base/nsPresContext.cpp   |   2 +-
 layout/base/nsRefreshDriver.cpp | 572 +++++++++++++++-------------------------
 layout/base/nsRefreshDriver.h   |   8 +-
 3 files changed, 222 insertions(+), 360 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32493/
Attachment #8712263 - Flags: review?(roc)
Attachment #8712263 - Flags: review?(mchang)
From ad629846fe4f0412d238c14783cb5790f1707ea6 Mon Sep 17 00:00:00 2001
---
 gfx/layers/ipc/CompositorParent.cpp | 38 +++++++++++++++++++------------------
 gfx/layers/ipc/CompositorParent.h   | 10 ++++++----
 2 files changed, 26 insertions(+), 22 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32495/
Attachment #8712264 - Flags: review?(roc)
Attachment #8712264 - Flags: review?(mchang)
From 2a6dec0256c772b385fb7b7d345091c132ae2131 Mon Sep 17 00:00:00 2001
---
 dom/ipc/ContentChild.cpp |  6 ++++--
 dom/ipc/PBrowser.ipdl    |  1 +
 dom/ipc/TabChild.cpp     |  4 ++++
 dom/ipc/TabParent.cpp    | 10 ++++++++--
 widget/PuppetWidget.cpp  | 15 +++++++++++++++
 widget/PuppetWidget.h    |  5 +++++
 6 files changed, 37 insertions(+), 4 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32497/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32497/
Attachment #8712265 - Flags: review?(roc)
Attachment #8712265 - Flags: review?(mchang)
From 15269f09e7fada5ef2335989989e833fba63d160 Mon Sep 17 00:00:00 2001
testing needs some love
---
 gfx/tests/gtest/TestVsync.cpp | 108 +++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 74 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/32499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32499/
Attachment #8712256 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

https://reviewboard.mozilla.org/r/32479/#review29199

::: gfx/thebes/gfxUtils.h:307
(Diff revision 1)
> +

Can we just have the second version? I don't see the point of the first variant.
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

https://reviewboard.mozilla.org/r/32481/#review29221

::: xpcom/glue/nsISupportsImpl.h:562
(Diff revision 1)
> + * Use this macro to declare pure virtual AddRef & Release methods, for
> + * implementation by derived classes.
> + */
> +#define NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING                               \

I'm skeptical of the need for this macro.

AFAICS, both of the classes in downstream patches that need this (VsyncSource and VsyncObserver) always use threadsafe refcounting in their concrete subclasses.  So why not use threadsafe refcounting in the base class(es) always?

Are you trying to leave open the option of somebody implementing one of these classes with non-threadsafe refcounting and then carefully using that inside this existing framework?  That seems very error-prone.
Attachment #8712257 - Flags: review?(nfroyd)
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review29295

::: gfx/thebes/gfxVsync.h:34
(Diff revision 1)
> +  NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING

Add a comment here explaining why this has to be pure virtual (because this gets mixed into classes that have other refcounting).

::: gfx/thebes/gfxVsync.h:35
(Diff revision 1)
> +  

trailing whitespace

::: gfx/thebes/gfxVsync.h:46
(Diff revision 1)
> +  VsyncSource *AttachedSource() { return mVsyncSource; }

mVsyncSource gets set during AddVsyncObserver and cleared by RemoveVsyncObserver and VsyncSource::Shutdown. You'd better document when this is non-null and make sure that callers can't use it in a way that races with RemoveVsyncObserver and Shutdown.

::: gfx/thebes/gfxVsync.h:56
(Diff revision 1)
> +// gfxPlatform does on the parent process

I think just
"Lives as long as the gfxPlatform does".

::: gfx/thebes/gfxVsync.cpp:16
(Diff revision 1)
> +#define PARENT_OR_CHILD (XRE_IsParentProcess() ? "Parent" : "Child")

Make this a static function instead of a macro.
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

https://reviewboard.mozilla.org/r/32485/#review29299

::: gfx/thebes/gfxPlatformMac.cpp:547
(Diff revision 1)
> -      // Release the display link
> +    // Make sure the timer is cancelled, which might enable it after 

trailing whitespace

::: gfx/thebes/gfxPlatformMac.cpp:556
(Diff revision 1)
> -
> +  

trailing whitespace
Attachment #8712259 - Flags: review?(roc) → review+
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

https://reviewboard.mozilla.org/r/32489/#review29301
Attachment #8712261 - Flags: review?(roc) → review+
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/32491/#review29303

::: widget/moz.build
(Diff revision 1)
> -    'VsyncDispatcher.h',

Looks like the changes in this file might be in the wrong patch.

::: widget/nsBaseWidget.cpp:1245
(Diff revision 1)
> +      OnVsync(aVsyncTimestamp);

{} around *all* 'if' bodies, please

::: widget/nsBaseWidget.cpp:1262
(Diff revision 1)
> +  void EnableVsync() override {

Please add assertions to all the main-thread-only method implementations (including this one) that we're on the main thread. Apart from anything else it documents that this code is main-thread-only.

::: widget/nsBaseWidget.cpp:1289
(Diff revision 1)
> +    if (!mPaused)

Why is mPaused atomic? Which functions (if any) can access it off the main thread? This should be documented in this class, preferably by asserts about the current thread.

::: widget/nsBaseWidget.cpp:2369
(Diff revision 1)
> +  // Destroy the layer manager, which will 

fix comment
https://reviewboard.mozilla.org/r/32483/#review29295

> Make this a static function instead of a macro.

I should just put that alongside XRE_IsParentProcess.  Something like XRE_ProcessTypeString().
https://reviewboard.mozilla.org/r/32481/#review29221

> I'm skeptical of the need for this macro.
> 
> AFAICS, both of the classes in downstream patches that need this (VsyncSource and VsyncObserver) always use threadsafe refcounting in their concrete subclasses.  So why not use threadsafe refcounting in the base class(es) always?
> 
> Are you trying to leave open the option of somebody implementing one of these classes with non-threadsafe refcounting and then carefully using that inside this existing framework?  That seems very error-prone.

The issue is that VsyncSource/VsyncObserver are mixed in to other classes that already implement their own refcounting, or are mixed together with other interface classes that also need to be refcounted.  In the nsISupports world, all the refcounting was always on nsISupports, and everything shared the nsISuspports base class, so it all worked fine.  But without such a base class, we need to implement refcounting in the concretemost class, so that there is only one refcount regardless of which parent class interface is used to access this.  For example, I have a concrete class that is both a VsyncObserver and a VsyncSource; as well as classes that are already otherwise refcounted (RefreshDriverTimer) but also need to be a VsyncObserver.

It would be nice if we could easily declare that the concrete refcounting implementation must also be thread-safe, but I didn't see an easy way of doing that.  I can skip the macro and just put the virtual decls themselves in the base classes, but doing it with a macro seems simpler.
https://reviewboard.mozilla.org/r/32491/#review29303

> Why is mPaused atomic? Which functions (if any) can access it off the main thread? This should be documented in this class, preferably by asserts about the current thread.

NotifyVsync() checks it to see if it should forward the notification or not.  Everything else should be just main thread only.

> fix comment

Actually, that DestroyLayerManager shouldn't even be there any more, it's just in the desstructor.
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/1-2/
Attachment #8712258 - Flags: review?(roc)
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/1-2/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/1-2/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/1-2/
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32491/diff/1-2/
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32493/diff/1-2/
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32495/diff/1-2/
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32497/diff/1-2/
Comment on attachment 8712266 [details]
MozReview Request: Bug 1184283 - test... removals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32499/diff/1-2/
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32479/diff/1-2/
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32481/diff/1-2/
Attachment #8712257 - Flags: review?(nfroyd)
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/2-3/
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/2-3/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/2-3/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/2-3/
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32491/diff/2-3/
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32493/diff/2-3/
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32495/diff/2-3/
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32497/diff/2-3/
Comment on attachment 8712266 [details]
MozReview Request: Bug 1184283 - test... removals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32499/diff/2-3/
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review29359

Looks mostly good. Just some update on the docs, with the biggest issue being new figures and preferably another section explaining the widget hierarchy.

::: gfx/doc/Silk.md:16
(Diff revision 2)
> -4. The **CompositorVsyncDispatcher** notifies the **Compositor** that a vsync has occured.
> +4. A **VsyncSource** may either be a direct display getting notifications from the *Hardware Vsync Thread*, or it may be a **VsyncChild** (via the PVSync protocol) that is listening to cross-process Vsync notifications from the main/compositor thread.

Please be very explicit here. If you use an or, please just break it down into two cases and explain when each case happens.

::: gfx/doc/Silk.md:21
(Diff revision 2)
> -The implementation is broken into the following sections and will reference this figure. Note that **Objects** are bold fonts while *Threads* are italicized.
> +<b>NOTE: This figure is obsolete!</b>  The implementation is broken into the following sections and will reference this figure. Note that **Objects** are bold fonts while *Threads* are italicized.

Please update the figure then. Also, it'd be nice if you had a smaller figure with how the widgets themselves are organized. e.g. toplevel widgets + child widgets.

::: gfx/doc/Silk.md:33
(Diff revision 2)
> -As of this writing, both Firefox OS and OS X create their own hardware specific *Hardware Vsync Thread* that executes after a vsync has occured.
> +OS X creates one *Hardware Vsync Thread* and uses DisplayLink to listen to vsync events.  We do not currently support multiple displays, so we use one global **CVDisplayLinkRef** that works across all active displays.

nit: Newline's after periods.

::: gfx/doc/Silk.md:40
(Diff revision 2)
> -It is the responsibility of the **CompositorVsyncDispatcher** to notify the **Compositor** that is awaiting vsync notifications.
> +When a vsync occurs on a **VsyncSource**, the *Hardware Vsync Thread* callback notifies all of its observers (generally Widgets) with the vsync's timestamp.

nit: specify when it isn't a widget. What case is this?

::: gfx/doc/Silk.md:46
(Diff revision 2)
> -There is only one **VsyncSource** object throughout the entire lifetime of Firefox.
> +The **VsyncManager** object is accessible from **gfxPlatform**, and is instantiated only on the parent process when **gfxPlatform** is created.  The **VsyncManager** is destroyed when **gfxPlatform** is destroyed.  There is only one **VsyncManager** object throughout the entire lifetime of Firefox.

nit: newlines between periods.

::: gfx/doc/Silk.md:76
(Diff revision 2)
> -When a vsync event occurs, the **CompositorVsyncDispatcher** notifies the **Compositor** of a vsync event, which notifies the **GeckoTouchDispatcher**.
> +When a vsync event occurs, the **Compositor** is notified of a vsync event, which notifies the **GeckoTouchDispatcher**.

Do you mean **CompositorVsyncObserver** which notifies the **Compositor**?

::: gfx/doc/Silk.md:131
(Diff revision 2)
> -However, there is some amount of time required to setup the IPC connection upon process creation and during this time, the **RefreshDrivers** must tick to set up the process.
> +However, there is some amount of time required to setup the IPC connection upon process creation.  This is done the first time a particular display is requested in nsBaseWidget.

nit: newline

::: gfx/doc/Silk.md:132
(Diff revision 2)
>  

question: So We're sure that all nsBaseWidgets should create their RefreshTimer BEFORE it creates the refresh driver? If so, please add this.

::: gfx/doc/Silk.md:181
(Diff revision 2)
> -5. VsyncParent/VsyncChild - Lives as long as the content process
> +* RefreshTimer - Lives as long as the process

Shouldn't the WidgetRefreshTimer live as long as the widget?

::: gfx/thebes/gfxVsync.h:36
(Diff revision 2)
> +  NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING

nice, TIL.

::: gfx/thebes/gfxVsync.h:45
(Diff revision 2)
> +  // thread.

nit: Please add something like "VsyncObservers should really just post the appropriate task onto the correct thread and finish".

::: gfx/thebes/gfxVsync.h:54
(Diff revision 2)
> +  // source on one thread, another thread asking for the

Is there anyway we can make this not race? e.g. a clean shutdown / switch process with locks and all?

::: gfx/thebes/gfxVsync.h:136
(Diff revision 2)
> +  // TODO: Windows / Linux. DOCUMENT THIS WHEN IMPLEMENTING ON THOSE PLATFORMS

nit: Please just add a link to https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp?from=gfxWindowsPlatform.cpp#2853 for Windows.

For Android/Linux: Put software vsync.

::: gfx/thebes/gfxVsync.cpp:70
(Diff revision 2)
> +    found = mVsyncObservers.RemoveElement(aObserver);

When would we get into a case where we're removing a vsync observer that doesn't exist? This seems like something that shouldn't be allowed.
Attachment #8712258 - Flags: review?(mchang)
Attachment #8712259 - Flags: review?(mchang) → review+
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

https://reviewboard.mozilla.org/r/32485/#review29381

::: gfx/thebes/gfxPlatform.cpp:694
(Diff revision 3)
> -        gPlatform->mVsyncSource = nullptr;
> +        if (gPlatform->mVsyncManager) {

nit: I think this should always be true on master. We no longer have non-vsync code paths. Change this to an assert.

::: gfx/thebes/gfxWindowsPlatform.cpp:2913
(Diff revision 3)
> +      { // scope lock

nit: delete this block. We'll already check when we enter the for loop below.
Attachment #8712260 - Flags: review?(mchang) → review+
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

https://reviewboard.mozilla.org/r/32487/#review29383
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

https://reviewboard.mozilla.org/r/32489/#review29387

::: layout/ipc/VsyncChild.h:27
(Diff revision 3)
> +    public gfx::VsyncSource

Why does VsyncChild have to also be a VsyncSource? If these are on the content process, it's not a real source, unless that's coming in a later patch?

::: layout/ipc/VsyncChild.cpp:81
(Diff revision 3)
> +  

nit: whitespace

::: layout/ipc/VsyncParent.cpp:53
(Diff revision 3)
> +  if (mDestroyed)

don't we already check this at DispatchVsyncEvent? Also isn't this racy since we don't have a lock for mDestroyed. I think mDestroyed is only valid on the background thread.
Attachment #8712261 - Flags: review?(mchang)
https://reviewboard.mozilla.org/r/32489/#review29387

> Why does VsyncChild have to also be a VsyncSource? If these are on the content process, it's not a real source, unless that's coming in a later patch?

It is a real source -- it's the bridge between the parent process and the child process vsync.  Child-process widgets will end up listening to a VsyncSource that's actually a VsyncChild.  Though now that you mention this, I may want to rework how gfxPlatform/VsyncManager/etc. works on the child... the child could create a ContentVsyncManager that tries to create a PVsync connection for any requested ID.

> don't we already check this at DispatchVsyncEvent? Also isn't this racy since we don't have a lock for mDestroyed. I think mDestroyed is only valid on the background thread.

Does it matter?  Checking mDestroyed there is effectively an optimization -- if it races and we end up dispatching to the background thread while we're in the process of setting mDestroyed to true, it will be caught when DispatchVsyncEvent executes on the background thread.  That said, there's no reason to check Destroyed here.. since the window where it might actually prevet an unnecessary dispatch is very tiny.
https://reviewboard.mozilla.org/r/32483/#review29359

> nit: specify when it isn't a widget. What case is this?

None right now; it could be something else though, for example something specific for processing VR events.

> question: So We're sure that all nsBaseWidgets should create their RefreshTimer BEFORE it creates the refresh driver? If so, please add this.

I'm not sure what you're asking -- when a RefreshDriver is created, it finds out which widget its attached to, and creates a WidgetVsyncRefreshTimer to listen to that widget.

> nice, TIL.

Well, your TIL is very new, since I added that macro in the commit one above this one :)

> Is there anyway we can make this not race? e.g. a clean shutdown / switch process with locks and all?

I don't really want to -- making it not race will be tricky.  We can possibly even remove this method, it's more convenience and for debugging.

> When would we get into a case where we're removing a vsync observer that doesn't exist? This seems like something that shouldn't be allowed.

Just.. habit I guess? I can make this assert instead?
> > don't we already check this at DispatchVsyncEvent? Also isn't this racy since we don't have a lock for mDestroyed. I think mDestroyed is only valid on the background thread.
> 
> Does it matter?  Checking mDestroyed there is effectively an optimization --
> if it races and we end up dispatching to the background thread while we're
> in the process of setting mDestroyed to true, it will be caught when
> DispatchVsyncEvent executes on the background thread.  That said, there's no
> reason to check Destroyed here.. since the window where it might actually
> prevet an unnecessary dispatch is very tiny.

I'd still rather not have a race or unprotected racy accesses in general. Can we please either delete the check or if you want it, add a lock for it?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #103)
> https://reviewboard.mozilla.org/r/32483/#review29359
> 
> > nit: specify when it isn't a widget. What case is this?
> 
> None right now; it could be something else though, for example something
> specific for processing VR events.

Can you please make this explicit in the docs then. Right now it's only for widgets, but in the future it can be used for something else. Then whenever that something else comes out, we'll update the docs again.

> > Is there anyway we can make this not race? e.g. a clean shutdown / switch process with locks and all?
> 
> I don't really want to -- making it not race will be tricky.  We can
> possibly even remove this method, it's more convenience and for debugging.

Is it guaranteed to only be accessed on one thread? Otherwise, at least add a lock?
 
> > When would we get into a case where we're removing a vsync observer that doesn't exist? This seems like something that shouldn't be allowed.
> 
> Just.. habit I guess? I can make this assert instead?

Yes please :).
https://reviewboard.mozilla.org/r/32483/#review29585

::: gfx/thebes/gfxVsync.h:62
(Diff revision 3)
> +  VsyncSource *mVsyncSource;

Just saw this again. Why the naked pointer instead of RefPtr?
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/32491/#review29587

::: widget/nsBaseWidget.cpp:208
(Diff revision 3)
> +  if (!sVsyncChildTable) {

I see that the uses are only in the content process, but it looks like we'll create/destroy on both the parent and child processes. Why do we do that?

::: widget/nsBaseWidget.cpp:1282
(Diff revision 3)
> +    if (AttachedSource()) {

nit: assert which thread this happens on.

::: widget/nsBaseWidget.cpp:1297
(Diff revision 3)
> +    mObservedWidget = aWidget;

Does this mean that the VsyncFowradingObserver or a widget? e.g. the toplevel widget actually listens to the vsync source, and the child widgets listen to their parent widget?

I think I saw it further down. If that's the case, please assert mSourceId is -1 or some constant that is "listening to a widget, not a source".

::: widget/nsIWidget.h:561
(Diff revision 3)
> +     * observers from a vsync observer callback.

Do you mean it's safe to remove an observer while in NotifyVsync? Or which observer callback do you mean?
Attachment #8712262 - Flags: review?(mchang)
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review29639

::: layout/base/nsRefreshDriver.cpp:83
(Diff revision 1)
> +#define PARENT_STR (XRE_IsParentProcess() ? "Parent" : "Child")

Fix this along with the other one

::: layout/base/nsRefreshDriver.cpp:541
(Diff revision 1)
> -      Unused << mVsyncChild->SendUnobserve();
> +      mWidget->RemoveVsyncObserver(this);

I'm uncomfortable with making this call while holding the lock, since this call will take its own locks. How about moving it later, outside the lock?

If you don't do that, then we need to very carefully define and check the lock ordering here. Maybe we should do that anyway...

::: layout/base/nsRefreshDriver.cpp:738
(Diff revision 1)
> -static InactiveRefreshDriverTimer* sThrottledRateTimer;
> +static RefPtr<InactiveRefreshDriverTimer> sThrottledRateTimer;

We don't like global destructors so you should probably use StaticRefPtr here.
Attachment #8712263 - Flags: review?(roc)
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

https://reviewboard.mozilla.org/r/32497/#review29643

::: widget/PuppetWidget.cpp:1449
(Diff revision 3)
> +    return;

{}
Attachment #8712265 - Flags: review?(roc) → review+
https://reviewboard.mozilla.org/r/32493/#review29639

> I'm uncomfortable with making this call while holding the lock, since this call will take its own locks. How about moving it later, outside the lock?
> 
> If you don't do that, then we need to very carefully define and check the lock ordering here. Maybe we should do that anyway...

Removing it out of the lock is safe.. mRunning gets set and checked with mRefreshTickLock, so worst case we'll get another NotifyVsync that we'll immediately bail out of

> We don't like global destructors so you should probably use StaticRefPtr here.

TIL about StaticRefPtr!
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #76)
> The issue is that VsyncSource/VsyncObserver are mixed in to other classes
> that already implement their own refcounting, or are mixed together with
> other interface classes that also need to be refcounted.  In the nsISupports
> world, all the refcounting was always on nsISupports, and everything shared
> the nsISuspports base class, so it all worked fine.  But without such a base
> class, we need to implement refcounting in the concretemost class, so that
> there is only one refcount regardless of which parent class interface is
> used to access this.  For example, I have a concrete class that is both a
> VsyncObserver and a VsyncSource; as well as classes that are already
> otherwise refcounted (RefreshDriverTimer) but also need to be a
> VsyncObserver.

Thanks for walking me through the code; I hadn't noticed how RefreshDriverTimer plays into this.  This is probably the best way to do things, then...
Attachment #8712257 - Flags: review?(nfroyd) → review+
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

https://reviewboard.mozilla.org/r/32481/#review29657

I think some comments about how derived classes should have threadsafe refcounting would be appropriate.
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review29665

::: layout/base/nsRefreshDriver.cpp:400
(Diff revision 3)
> -      , mVsyncRate(TimeDuration::Forever())
> +      mTimer->TickRefreshDriverWithMostRecent();

nit: assert thread.

::: layout/base/nsRefreshDriver.cpp:413
(Diff revision 3)
> -          MonitorAutoLock lock(mRefreshTickLock);
> +      MonitorAutoLock lock(mRefreshTickLock);

nit: I think the parent process / child process path still applies here right? Main thread NotifVsync should be only the child process and non-main thread only on the parent process? If so, please keep the assertions.

::: layout/base/nsRefreshDriver.cpp:446
(Diff revision 3)
> +      NS_ReleaseOnMainThread(mWidget);

Does this only happen on the parent process? Content process should be main thread? If so, please assert again.

::: layout/base/nsRefreshDriver.cpp:454
(Diff revision 3)
> -      if (!XRE_IsParentProcess()) {
> +    TimeStamp vsyncTime;

nit: assert thread. Sorry for all these thread assertions, but it really makes the code a lot easier to read.

::: layout/base/nsRefreshDriver.cpp:497
(Diff revision 3)
> +      // XXX fixme (need to ask widget for the source, get the source;

Please fix :)

::: layout/base/nsRefreshDriver.cpp:514
(Diff revision 3)
> -  {
> +      mLastTick = TimeStamp::Now();

Why do we need to set mLastTick here? Shouldn't the refresh driver already do this?

::: layout/base/nsRefreshDriver.cpp:525
(Diff revision 3)
> +    mLastTick = TimeStamp::Now();

Why do we need this? Can't we just use mLastFireTime?

::: layout/base/nsRefreshDriver.cpp:572
(Diff revision 3)
> - * during the intial startup process.
> +/*

PreciseRefreshDriverTimer shouldn't exist anymore. Please delete this comment.

::: layout/base/nsRefreshDriver.cpp:736
(Diff revision 3)
> -static RefreshDriverTimer* sRegularRateTimer;
> +static RefPtr<RefreshDriverTimer> sRegularRateTimer;

Does the regular rate refresh timer still exist? I thought widgets should have their own timer and refresh drivers should just attach to the timer that existson their widget?

::: layout/base/nsRefreshDriver.cpp:793
(Diff revision 3)
> -// layout.frame_rate=0 indicates "ASAP mode".
> +// not attached to a widget.

oh TIL? What kind of documents don't attach to a widget?

::: layout/base/nsRefreshDriver.cpp
(Diff revision 3)
> -  if (rate < 0) {

++ to delete this. Although you probably still have to account for running the refresh drive rin ASAP mode for talos tests.

::: layout/base/nsRefreshDriver.cpp:853
(Diff revision 3)
> +  // .... if it's not being displayed, should we just use the throttled timer?

yes, I think so.

::: layout/base/nsRefreshDriver.cpp:1057
(Diff revision 3)
> -  RefreshDriverTimer *newTimer = ChooseTimer();
> +  RefPtr<RefreshDriverTimer> newTimer = ChooseTimer();

Yay for refcounted refresh timers :)
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

https://reviewboard.mozilla.org/r/32495/#review29669

::: gfx/layers/ipc/CompositorParent.cpp:386
(Diff revision 3)
> +  if (mVsyncObserver) {

nit: Shouldn't it always have one?

::: gfx/layers/ipc/CompositorParent.cpp:562
(Diff revision 3)
>  {

nit: assert thread.

::: gfx/layers/ipc/CompositorParent.cpp:570
(Diff revision 3)
> -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> +  mCompositorParent->GetWidget()->RemoveVsyncObserver(mVsyncObserver);

ditto

::: gfx/layers/ipc/CompositorParent.cpp:740
(Diff revision 3)
> +  if (mCompositorScheduler) {

nit: assert it exists.
Attachment #8712264 - Flags: review?(mchang) → review+
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

https://reviewboard.mozilla.org/r/32497/#review29671
Attachment #8712265 - Flags: review?(mchang) → review+
https://reviewboard.mozilla.org/r/32483/#review29585

> Just saw this again. Why the naked pointer instead of RefPtr?

I explicitly don't want to hold a ref to it, since otherwise it would set up a cycle.  That cycle might be okay because it will be broken via RemoveVsyncObserver... hmm. Keeping this here was mainly a bookkeeping thing, though it becomes convenient in a few places; I'll see about switching it to a RefPtr.
https://reviewboard.mozilla.org/r/32491/#review29587

> nit: assert which thread this happens on.

Any thread can ask.

> Does this mean that the VsyncFowradingObserver or a widget? e.g. the toplevel widget actually listens to the vsync source, and the child widgets listen to their parent widget?
> 
> I think I saw it further down. If that's the case, please assert mSourceId is -1 or some constant that is "listening to a widget, not a source".

Yeah, I've been thinking about this.. we do need some way in widget to indicate "default-source-or-parent" to avoid some special casing.  I was going to do that as a followup but we can probably do it here.

I've been working on this for a little bit; it caused and dug up a few issues that I fixed.  It's not a big change; will be in the next set of patches.

> Do you mean it's safe to remove an observer while in NotifyVsync? Or which observer callback do you mean?

Correct, safe to remove an observer while in NotifyVsync.
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32479/diff/2-3/
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32481/diff/2-3/
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/3-4/
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/3-4/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/3-4/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/3-4/
Attachment #8712261 - Flags: review?(mchang)
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32491/diff/3-4/
Attachment #8712262 - Flags: review?(mchang)
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32493/diff/3-4/
Attachment #8712263 - Flags: review?(roc)
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32495/diff/3-4/
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32497/diff/3-4/
Comment on attachment 8712266 [details]
MozReview Request: Bug 1184283 - test... removals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32499/diff/3-4/
FWIW -- latest set of try runs for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf5f2b4db792

I'm going to rebase next to pick up some fixes to some of the intermittent oranges.  The remaining oranges, except for one or two, seem unrelated or are already known intermittent.
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review30277
Attachment #8712258 - Flags: review?(roc) → review+
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/32491/#review30279

::: widget/nsBaseWidget.cpp:1245
(Diff revision 4)
> +      OnVsync(aVsyncTimestamp);

OK, so when we race on mPaused I guess there are two cases:

a) Main thread sets mPaused as we run through here. I guess in that case we might call OnVsync when we didn't need to, and that's OK.

b) Main thread clears mPaused as we run through here. In that case we won't call OnVsync but maybe we need to. I guess that case is OK because OnVsync will eventually get called?

Are we in fact guaranteeing that OnVsync gets called every frame when we're not paused, even when nothing's changing? I guess we already wake up after every frame interval. Not great...
Attachment #8712262 - Flags: review?(roc)
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review30281
Attachment #8712261 - Flags: review?(mchang) → review+
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

https://reviewboard.mozilla.org/r/32489/#review30363
https://reviewboard.mozilla.org/r/32489/#review29387

> It is a real source -- it's the bridge between the parent process and the child process vsync.  Child-process widgets will end up listening to a VsyncSource that's actually a VsyncChild.  Though now that you mention this, I may want to rework how gfxPlatform/VsyncManager/etc. works on the child... the child could create a ContentVsyncManager that tries to create a PVsync connection for any requested ID.

I think from irc, right now this is the only VsyncSource that isn't actually directly listening to hardware vsync. Just add a comment to the top here please. IIRC, it was listed in the docs.
Attachment #8712262 - Flags: review?(mchang)
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/32491/#review30367

::: widget/nsBaseWidget.cpp:1323
(Diff revisions 3 - 4)
> +    Unobserve();

Why do we have to unobserve when we're trying to observe a source?
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review30379

::: layout/base/nsRefreshDriver.cpp:885
(Diff revisions 3 - 4)
> +  if (!sThrottledRateTimer) {

Any particular reason to create the InactiveRefreshDriverTimer in the creation of a refresh driver rather during ChooseTimer()? Seems odd to have two different places to create two different timers.

::: layout/base/nsRefreshDriver.cpp:532
(Diff revision 4)
> -    if (XRE_IsParentProcess()) {
> +    VSYNC_LOG("[%s][%p] RefreshDriverTimer adding widget vsync observer\n", XRE_GetProcessTypeString(), this);

Don't you need the mRefreshTickLock here to set mRunning as well?
Attachment #8712258 - Flags: review?(mchang)
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review30383
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32479/diff/3-4/
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32481/diff/3-4/
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/4-5/
Attachment #8712258 - Flags: review?(mchang)
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/4-5/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/4-5/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/4-5/
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32479/diff/3-4/
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32481/diff/3-4/
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/4-5/
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/4-5/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/4-5/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/4-5/
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32491/diff/4-5/
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32493/diff/4-5/
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32495/diff/4-5/
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32497/diff/4-5/
Comment on attachment 8712266 [details]
MozReview Request: Bug 1184283 - test... removals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32499/diff/4-5/
Attachment #8712258 - Flags: review?(mchang)
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review38885

::: gfx/doc/Silk.md:21
(Diff revision 5)
> -5. The **RefreshTimerVsyncDispatcher** notifies the Chrome **RefreshTimer** that a vsync has occured.
> -6. The **RefreshTimerVsyncDispatcher** sends IPC messages to all content processes to tick their respective active **RefreshTimer**.
> -7. The **Compositor** dispatches input events on the *Compositor Thread*, then composites. Input events are only dispatched on the *Compositor Thread* on b2g.
> +5. The Widget, in turn, will notify any of the things that are listening to *it* -- these are currently RefreshDrivers, Compositors, or other Widgets.
> +6. When it receives a vsync event, the **Compositor** dispatches input events on the *Compositor Thread*, then composites. Input events are only dispatched on the *Compositor Thread* on b2g.
> +7. When it receives a vsync event, The **RefreshDriver** paints on the *Main Thread*.
> -8. The **RefreshDriver** paints on the *Main Thread*.
>  
> -The implementation is broken into the following sections and will reference this figure. Note that **Objects** are bold fonts while *Threads* are italicized.
> +<b>NOTE: This figure is obsolete!</b>  The implementation is broken into the following sections and will reference this figure. Note that **Objects** are bold fonts while *Threads* are italicized.

Please fix the other 3 issues from the previous review.
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

https://reviewboard.mozilla.org/r/32491/#review38989
Attachment #8712262 - Flags: review?(mchang) → review+
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review38993

::: layout/base/nsRefreshDriver.cpp:449
(Diff revision 5)
>  
> -    void OnTimerStart()
> +  void TickRefreshDriverWithMostRecent()
> -    {
> +  {
> -      if (!XRE_IsParentProcess()) {
> -        mLastChildTick = TimeStamp::Now();
> +    MOZ_ASSERT(NS_IsMainThread());
> +    TimeStamp vsyncTime;
> +

nit: Also assert only on the parent process.

::: layout/base/nsRefreshDriver.cpp:493
(Diff revision 5)
> -        RecordJank(sample);
> +      RecordJank(sample);
> -      } else {
> +    } else {
> -        // Request the vsync rate from the parent process. Might be a few vsyncs
> +      // Request the vsync rate from the parent process. Might be a few vsyncs
> -        // until the parent responds.
> +      // until the parent responds.
> -        mVsyncRate = mVsyncRefreshDriverTimer->mVsyncChild->GetVsyncRate();
> +
> +      // XXX fixme (need to ask widget for the source, get the source;

You have to fix this before shipping. I know that the product teams are measuring perf before / after e10s and we need these measurements.

::: layout/base/nsRefreshDriver.cpp:520
(Diff revision 5)
> -    TimeDuration mVsyncRate;
> -    bool mProcessedVsync;
> -  }; // RefreshDriverVsyncObserver
>  
> -  virtual ~VsyncRefreshDriverTimer()
> -  {
> +    if (!XRE_IsParentProcess()) {
> +      mLastTick = TimeStamp::Now();

Why do we only update this only on the content process? If it's only used for content in telemetry, please leave the name as mLastChildTick.

::: layout/base/nsRefreshDriver.cpp:580
(Diff revision 5)
>  
> -/**
> - * Since the content process takes some time to setup
> - * the vsync IPC connection, this timer is used
> - * during the intial startup process.
> - * During initial startup, the refresh drivers
> +  nsCOMPtr<nsICancelableRunnable> mTickRunnable;
> +}; // WidgetVsyncRefreshDriverTimer
> +
> +/*
> + * PreciseRefreshDriverTimer schedules ticks based on the current time

nit: I don't think this comment exists anymore. Is this a bad merge?

::: layout/base/nsRefreshDriver.cpp:802
(Diff revision 5)
> -// Compute the interval to use for the refresh driver timer, in milliseconds.
> -// outIsDefault indicates that rate was not explicitly set by the user
> -// so we might choose other, more appropriate rates (e.g. vsync, etc)
> -// layout.frame_rate=0 indicates "ASAP mode".
> -// In ASAP mode rendering is iterated as fast as possible (typically for stress testing).
> -// A target rate of 10k is used internally instead of special-handling 0.
> +// Compute the interval to use for the non-vsync refresh
> +// driver timer, in milliseconds.  This timer is only used
> +// when a vsync timer is not available, or for documents
> +// not attached to a widget.
> +/* static */ double
> +nsRefreshDriver::GetRegularTimerInterval()

I don't think this is accurate. The inactive timer has exponential backoff for the timer, so there's no real interval for the inactive timer. What do we use for documents not attached to a widget?
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712256 [details]
MozReview Request: Bug 1184283 - Add GenerateUUID to gfxUtils; use it instead of direct UUIDGenerator calls

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32479/diff/4-5/
Attachment #8712258 - Flags: review?(mchang)
Attachment #8712263 - Flags: review?(mchang)
Comment on attachment 8712257 [details]
MozReview Request: Bug 1184283 - nsISupports pure virtual refcounting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32481/diff/4-5/
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32483/diff/5-6/
Comment on attachment 8712259 [details]
MozReview Request: Bug 1184283 - convert gfxPlatform vsync impls to new VsyncSource/VsyncManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32485/diff/5-6/
Comment on attachment 8712260 [details]
MozReview Request: Bug 1184283 - misc changes and compilation fixes (some caused by unification file ordering changes)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32487/diff/5-6/
Comment on attachment 8712261 [details]
MozReview Request: Bug 1184283 - change PVsync to implement gfx::VsyncSource, and to track a gfx::VsyncSource on parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32489/diff/5-6/
Comment on attachment 8712262 [details]
MozReview Request: Bug 1184283 - Widget vsync source/observer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32491/diff/5-6/
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32493/diff/5-6/
Comment on attachment 8712264 [details]
MozReview Request: Bug 1184283 - have compositor listen to its Widget for vsync

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32495/diff/5-6/
Comment on attachment 8712265 [details]
MozReview Request: Bug 1184283 - inform e10s child browser tabs/puppet widgets proper vsync display ID

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32497/diff/5-6/
Comment on attachment 8712266 [details]
MozReview Request: Bug 1184283 - test... removals

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32499/diff/5-6/
Attachment #8712258 - Flags: review?(mchang)
Comment on attachment 8712258 [details]
MozReview Request: Bug 1184283 - add new gfxVsync.{cpp,h}, remove old files

https://reviewboard.mozilla.org/r/32483/#review45841

::: gfx/doc/Silk.md:180
(Diff revision 6)
>  With Content processes, the start up process is more complicated.
>  We send vsync IPC messages via the use of the PBackground thread on the parent process, which allows us to send messages from the Parent process' without waiting on the *main thread*.
>  This sends messages from the Parent::*PBackground Thread* to the Child::*Main Thread*.
>  The *main thread* receiving IPC messages on the content process is acceptable because **RefreshDrivers** must execute on the *main thread*.
> -However, there is some amount of time required to setup the IPC connection upon process creation and during this time, the **RefreshDrivers** must tick to set up the process.
> -To get around this, we initially use software **RefreshTimers** that already exist during content process startup and swap in the **VsyncRefreshTimer** once the IPC connection is created.
> +However, there is some amount of time required to setup the IPC connection upon process creation.
> +This is done the first time a particular display is requested in nsBaseWidget.

Don't we need a sentence here explainining how we setup the child process? Or does it always wait until the IPC channel is setup?

::: gfx/doc/Silk.md:197
(Diff revision 6)
> -1. VsyncSource::Display::RefreshTimerVsyncDispatcher receives a Vsync notification from the OS in the parent process.
> -2. RefreshTimerVsyncDispatcher notifies VsyncRefreshTimer::RefreshDriverVsyncObserver that a vsync occured on the parent process on the hardware vsync thread.
> -3. RefreshTimerVsyncDispatcher notifies the VsyncParent on the hardware vsync thread that a vsync occured.
> -4. The VsyncRefreshTimer::RefreshDriverVsyncObserver in the parent process posts a task to the main thread that ticks the refresh drivers.
> -5. VsyncParent posts a task to the PBackground thread to send a vsync IPC message to VsyncChild.
> -6. VsyncChild receive a vsync notification on the content process on the main thread and ticks their respective RefreshDrivers.
> +1. [parent] VsyncSource receives a hardware vsync notification from the underlying OS.
> +2. [parent] VsyncSource notifies its observers: Widgets and VsyncParents
> +3. [parent] VsyncParent posts a task to the PBackground thread to send a vsync IPC message to its VsyncChild
> +4. [child] VsyncChild receives a notification that vsync has occurred (on the main thread)
> +5. [child] VsyncChild notifies all of its observers (currently exclusively Widgets)
> +6. [child] Normal Widget vsync processing occurs (it gets dispatched to Widgets, Compositors, and RefreshDrivers)

The child doesn't have a compositor does it?

::: gfx/thebes/gfxVsync.h:53
(Diff revision 6)
> +  // The source that this observer is observing.  A VsyncObserver can
> +  // only be attached to one VsyncSource at a time.  This will be
> +  // non-null after the observer is attached to a VsyncSource, and
> +  // will become null when it is removed from a source, or when its
> +  // currently attached source is shut down.  This method is thread
> +  // safe, but can race -- if this observer is being removed from a

Make this thread safe and non-racy please.
Comment on attachment 8712263 [details]
MozReview Request: Bug 1184283 - RefreshDriver listen to widget via WidgetVsyncRefreshDriverTimer

https://reviewboard.mozilla.org/r/32493/#review45849

::: layout/base/nsRefreshDriver.h:382
(Diff revision 6)
>      return mFrameRequestCallbackDocs.Length() != 0;
>    }
>  
>    void FinishedWaitingForTransaction();
>  
> -  mozilla::RefreshDriverTimer* ChooseTimer() const;
> +  RefPtr<mozilla::RefreshDriverTimer> mActiveTimer;

nit: rename to mWidgetTimer.

::: layout/base/nsRefreshDriver.cpp:127
(Diff revision 6)
>   */
> -class RefreshDriverTimer {
> +class RefreshDriverTimer
> +  : public RefreshDriverTimerBase
> +{
> +public:
> +  enum RefreshDriverTimerType {

When would we ever have an unknown type?

::: layout/base/nsRefreshDriver.cpp:447
(Diff revision 6)
> +    mWidget = nullptr;
> -    }
> +  }
>  
> -    void OnTimerStart()
> +  void TickRefreshDriverWithMostRecent()
> -    {
> +  {
> -      if (!XRE_IsParentProcess()) {
> +    MOZ_ASSERT(NS_IsMainThread());

nit: assert XRE_IsParentProcess also

::: layout/base/nsRefreshDriver.cpp:493
(Diff revision 6)
> -        RecordJank(sample);
> +      RecordJank(sample);
> -      } else {
> +    } else {
> -        // Request the vsync rate from the parent process. Might be a few vsyncs
> +      // Request the vsync rate from the parent process. Might be a few vsyncs
> -        // until the parent responds.
> +      // until the parent responds.
> -        mVsyncRate = mVsyncRefreshDriverTimer->mVsyncChild->GetVsyncRate();
> +
> +      // XXX fixme (need to ask widget for the source, get the source;

Fix this. Some folks rely on this probe to determine e10s/non-e10s ship status.

::: layout/base/nsRefreshDriver.cpp:520
(Diff revision 6)
> -    TimeDuration mVsyncRate;
> -    bool mProcessedVsync;
> -  }; // RefreshDriverVsyncObserver
>  
> -  virtual ~VsyncRefreshDriverTimer()
> -  {
> +    if (!XRE_IsParentProcess()) {
> +      mLastTick = TimeStamp::Now();

nit: please keep name mLastChildTick.

::: layout/base/nsRefreshDriver.cpp:547
(Diff revision 6)
>  
> -    if (XRE_IsParentProcess()) {
> -      mVsyncDispatcher->SetParentRefreshTimer(nullptr);
> -    } else {
> -      Unused << mVsyncChild->SendUnobserve();
> +    { // scope lock
> +      MutexAutoLock lock(mRefreshTickLock);
> +      mRunning = false;
> +      if (mTickRunnable) {
> +        mTickRunnable->Cancel();

assert: XRE_IsParentProcess

::: layout/base/nsRefreshDriver.cpp:580
(Diff revision 6)
>  
> -/**
> - * Since the content process takes some time to setup
> - * the vsync IPC connection, this timer is used
> - * during the intial startup process.
> - * During initial startup, the refresh drivers
> +  RefPtr<CancelableRunnable> mTickRunnable;
> +}; // WidgetVsyncRefreshDriverTimer
> +
> +/*
> + * PreciseRefreshDriverTimer schedules ticks based on the current time

nit: There is no PreciseRefreshDriver timer. This looks like the result of a bad merge?

::: layout/base/nsRefreshDriver.cpp:1036
(Diff revision 6)
>                   "image doc refresh driver should never have its own timer");
>        return;
>      }
>    }
>  
> -  // We got here because we're either adjusting the time *or* we're
> +  nsIWidget *w = mPresContext->GetRootWidget();

nit: rename 'w' to 'widget'

::: widget/nsBaseWidget.cpp:1591
(Diff revision 6)
>    if (mIncomingVsyncObserver) {
>      mIncomingVsyncObserver->Destroy();
>      mIncomingVsyncObserver = nullptr;
>    }
> +
> +  // break cycle

nit: add comment explaining the cycle you're breaking.
Attachment #8712263 - Flags: review?(mchang)
Whiteboard: [gfx-noted][webvr] → [gfx-noted][webvr][games:p1]
Whiteboard: [gfx-noted][webvr][games:p1] → [gfx-noted][webvr][games:p1][platform-rel-Games]
platform-rel: --- → ?
Am I correct that this is no longer relevant for WebVR, since it has its own presentation mechanism? (that does support 90hz iirc?) Has this bug been implemented? (webvr is games:p1)

Outside WebVR, what happens if I have two displays, one with 60hz vsync and another with 120hz vsync, and I move my Firefox window between these two? Does rAF() interval adapt automatically to that? Does anyone have such a configuration to test? Or is this bug item about implementing this support exactly? (dual displays with different refresh rate would be games:p3)

Also, iirc if I'm running a single display configuration with 120hz, rAF()s in Firefox on Windows will run at 120fps, which is the important use case here besides WebVR, so I presume we should be well covered here. (single display at 120hz would be games:p2)

Marking as games:p? for now pending investigating above.
Whiteboard: [gfx-noted][webvr][games:p1][platform-rel-Games] → [gfx-noted][webvr][games:p?][platform-rel-Games]
platform-rel: ? → -

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: vladimir → nobody
Flags: needinfo?(bhood)
Flags: needinfo?(bhood)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: