1101974 - Create a single VsyncDisptacher per nsBaseWidget

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1092978 +++

In order to make it easier to provide silk support for multi-monitor / VR cases, access the vsync dispatcher through the CompositorParent.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Access VsyncDispatcher through CompositorParent

WIP, works on OS X. Deletes VsyncDispatcher from mozilla-central/widget and essentially makes the CompositorVsyncObserver the vsync dispatcher - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.h?from=CompositorParent.h&case=true#98. This creates a strong 1:1 relationship between the CompositorParent and Vsync source.

I also delete support to enable touch resampling without the compositor aligned vsync, which is how we actually want to run it in real life as well. Will do more testing tomorrow.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Move VsyncDispatcher to CompositorParent

Deletes mozilla-central/widget/VsyncDispatcher* and makes the CompositorVsyncObserver the vsync dispatcher. This creates a strong 1:1 relationship between each CompositorParent<-->VsyncSource. This also lets us access the nsIWidget associated with each CompositorParent.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Initialize Vsync Source during CompositorParent creation

Previously, we initialized the hardware vsync source independently when gfxPlatform initialized. This patch initializes the hardware vsync source after gfxPlatform is created and at the end of the creation of the CompositorParent. The VsyncDispatcher now holds the reference to the Vsync Source so it can enable/disable vsync.

Comment 4

[Mason Chang [Inactive] [:mchang]]

(In reply to Mason Chang [:mchang] from comment #3)
> Created attachment 8526264 [details] [diff] [review]
> Part 2: Initialize Vsync Source during CompositorParent creation
> 
> Previously, we initialized the hardware vsync source independently when
> gfxPlatform initialized. This patch initializes the hardware vsync source
> after gfxPlatform is created and at the end of the creation of the
> CompositorParent. The VsyncDispatcher now holds the reference to the Vsync
> Source so it can enable/disable vsync.

Err, I should say, this patch lets us delay initializing the hardware vsync source until needed, which is at CompositorParent creation time rather than during gfxPlatform initialization.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8526264 [details] [diff] [review]
Part 2: Initialize Vsync Source during CompositorParent creation

Review of attachment 8526264 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.h
@@ +589,5 @@
> +
> +    /**
> +     * Initialize and get hardware vsync based on each platform.
> +     */
> +    virtual nsRefPtr<mozilla::layers::VsyncSource> GetVsyncSource();

The return type should be already_AddRefed<mozilla::layers::VsyncSource>.

@@ +597,5 @@
>      virtual ~gfxPlatform();
>  
>      void AppendCJKPrefLangs(eFontPrefLang aPrefLangs[], uint32_t &aLen,
>                              eFontPrefLang aCharLang, eFontPrefLang aPageLang);
> +    

remove whitespace

::: gfx/thebes/gfxPlatformMac.cpp
@@ +504,2 @@
>  {
> +  return new OSXVsyncSource();

nsRefPtr<VsyncSource> osxVsyncSource = new OSXVsyncSource();
return osxVsyncSource.forget();

is the idiomatic way of doing this.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Initialize Vsync Source during CompositorParent creation

Updated with feedback from comment 5.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=073db0be5b3a

Comment 8

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82e94c16732d
https://hg.mozilla.org/integration/mozilla-inbound/rev/08df10804c35

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out at mchang's request:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac6c1eabd76
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2f62c1faf9

Comment 10

[Mason Chang [Inactive] [:mchang]]

I talked with :kats, we have some architecture questions we'd like to resolve first. No need to land this until we resolve those issues with multi-monitor / webvr.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Proposed Architecture

Kats and I discussed some proposed architecture that will work with both multi-monitor / full screen monitor with different display vsync rates. We also would like to resolve the issue from https://bugzilla.mozilla.org/show_bug.cgi?id=1092978#c79.

The big change is that we have one VsyncDispatcher per nsBaseWidget. Each VsyncDispatcher lives as long as the nsBaseWidget. We do this because each new window on desktop is its own widget. Both the RefreshDriver and Compositor access the VsyncDispatcher instance through nsBaseWidget. They all have the same lifetimes as well. There is a strong 1:1 relationship between the nsBaseWidget, nsRefreshDriver, CompositorParent, and VsyncDispatcher. 

Now we introduce a new VsyncSource. The VsyncSource is global across Firefox. It starts when Firefox / b2g starts and ends when Firefox / b2g shuts down. There is 1 vsync source total. The VsyncSource has 1 internal display object per display connected. This is because each display has its own vsync thread. Thus if we listen to vsync twice on the same display, we'd get two threads each ticking on the same vsync event. So the vsync source has 1 display object with 1 hardware vsync thread per display. On each display's vsync event, it notifies multiple VsyncDispatchers. The VsyncSource has a map of vsync dispatcher <---> display. There are multiple VsyncDispatchers per VsyncSource. When a window is idle, it can just stop listening to vsync events. 

When a widget goes full screen / moves to a different display, the nsBaseWidget -> VsyncDispatcher -> VsyncSource.UpdateDisplay() so that nsBaseWidget starts listening to the new vsync rate.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Nice diagram! Can you also describe the implications for multi-process Firefox? e.g. I'm assuming you don't want a VsyncSource per process; that should only live in the parent process. The VsyncDispatchers in the parent process should send notifications to the VsyncDispatchers in the child processes (the ones on the PuppetWidgets), I think. Also copy the above description and diagram and create a patch to put it in gfx/doc/SilkVsync.md or something like that. Don't worry too much about it being informal or incomplete, but we might as well start getting this doc in the tree as an official design doc rather than leaving out scattered on bugs. Also it will help us track changes in the design and the rationale for those changes more easily.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8527003 [details]
Proposed Architecture

If it's not too much trouble add a second VsyncDispatcher here or otherwise indicate the 1:many VsyncSource:VsyncDispatcher relationship somehow

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Proposed Architecture

Added the 1:many relationships and the multi-process case with VsyncDispatcherChild.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8527784 [details]
Proposed Architecture

Thanks

Comment 16

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Enable VsyncDispatcher instance per nsBaseWidget

Implements the proposed architecture. Contains the following:

1) VsyncSources for b2g and OS X that are initialized at gfxPlatform initialization.
2) Creates a VsyncDispatcher per nsBaseWidget on the parent process only.
3) Reformats the VsyncDispatcher to listen to 1 CompositorVsyncObserver
4) Changes the HwcComposer to enable/disable vsync only on the main thread and initialize the hardware vsync callbacks during HwcComposer initialization.

Most of it might actually be ready to review, I just want to confirm with Jerry that initializing vsync at the gfxPlatform is early enough for the refresh driver. On b2g, I think it is. On Desktop, they should be very close, which might be early enough that we don't have to do tricks in the refresh driver.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Create VsyncDispatcher in nsBaseWidget

Creates a VsyncDispatcher at nsBaseWidget the same time we create a CompositorParent. The lifetime of the VsyncDispatcher is until the nsBaseWidget is destroyed.

Comment 18

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Access VsyncDispatcher through nsIWidget interface

Replace occurrences to access the VsyncDispatcher through the nsIWidget interface rather than through the singleton VsyncDispatcher::GetInstance() interface. This is just in the CompositorParent for now.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create the VsyncSource and Display Framework / Objects

Implements the VsyncSource and Display objects as discussed previously. Since the VsyncSource should only live in the parent process, and has to be alive even if we have no VsyncDispatchers listening, we do a manual memory allocation to ensure that the VsyncSource is destroyed when Firefox shuts down in gfxPlatform.

Comment 20

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Refactor VsyncSource in OS X

Refactor the VsyncSource on OS X. We have two objects now: The VsyncSource and a GlobalDisplay, which has the vsync rate that works for all connected displays. The VsyncSource can have references to different displays, but for now we only care about the GlobalDisplay. Move all the logic related to starting / stopping vsync to the display object.

Comment 21

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

Refactor the current VsyncDispatcher to use the new vsync framework. The nicest part is that we can associate the VsyncDispatcher with only 1 CompositorParent, which lets us delete a fair amount of code.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8533243 [details] [diff] [review]
Part 4: Refactor VsyncSource in OS X

Review of attachment 8533243 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatformMac.cpp
@@ +520,1 @@
>  gfxPlatformMac::InitHardwareVsync()

Can you call this CreateHardwareVsyncSource? And I think it should also return an already_AddRefed<VsyncSource>.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8533239 [details] [diff] [review]
Part 2: Access VsyncDispatcher through nsIWidget interface

Review of attachment 8533239 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review the CompositorVsyncObserver initially and there's a bunch of stuff I probably would have objected to if I had. It's probably better to have the original reviewers do this review so it maintains some sort of internal consistency.

::: gfx/layers/ipc/CompositorParent.cpp
@@ -169,5 @@
>      return nullptr;
>    }
>  
>    CreateCompositorMap();
> -

spurious line deletion. please avoid whitespace changes in functions you're not actually modifying.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8533240 [details] [diff] [review]
Part 3: Create the VsyncSource and Display Framework / Objects

Review of attachment 8533240 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +387,5 @@
> +}
> +
> +void
> +VsyncSource::Display::AddVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher)
> +{

Move this down to just above the RemoveVsyncDispatcher function

@@ +605,5 @@
>  
>      RegisterStrongMemoryReporter(new GfxMemoryImageReporter());
>  
> +    if (XRE_IsParentProcess()) {
> +      if (gfxPrefs::HardwareVsyncEnabled() && gfxPrefs::VsyncAlignedCompositor()) {

Combine the if conditions

@@ +657,5 @@
>          gPlatform->mMemoryPressureObserver = nullptr;
>          gPlatform->mSkiaGlue = nullptr;
> +
> +        if (gPlatform->mVsyncSource && XRE_IsParentProcess()) {
> +          delete gPlatform->mVsyncSource;

This assumes that gfxPlatform subclass that implements the InitHardwareVsync will return the result of a |new|. This assumption is undocumented and brittle. Either add a function FiniHardwareVsync() that is parallel to InitHardwareVsync and lets the gfxPlatform subclass destroy the VsyncSource object, or (better) do what mstange suggested and keep this in a RefPtr so that we can just null it out here and have it self-destruct.

::: gfx/thebes/gfxPlatform.h
@@ +163,5 @@
> +// gfxPlatform does on the parent process
> +namespace mozilla {
> +namespace gfx {
> +class VsyncSource
> +{

Move VsyncSource into gfx/thebes/VsyncSource.h and the implementation into gfx/thebes/VsyncSource.cpp

@@ +187,5 @@
> +  }; // end Display
> +
> +  void AddVsyncDispatcher(mozilla::VsyncDispatcher* aVsyncDispatcher);
> +  void RemoveVsyncDispatcher(mozilla::VsyncDispatcher* aVsyncDispatcher);
> +  // Called when the widget switches to a different monitor

s/monitor/display/

But really you can just omit this function from the API entirely for now, since it's unimplemented. The Add/RemoveVsyncDispatcher signatures will need to change as well once multi-display support is implemented, so the SwitchDisplay function can be added then. This should be noted in the design doc.

@@ +386,5 @@
>      *CreateFontGroup(const mozilla::FontFamilyList& aFontFamilyList,
>                       const gfxFontStyle *aStyle,
>                       gfxUserFontSet *aUserFontSet) = 0;
> +
> +

Don't touch whitespace in functions you're mot modifying

@@ +702,5 @@
>      int8_t  mOpenTypeSVGEnabled;
>  
>      int8_t  mBidiNumeralOption;
>  
> +    // whether to always search font cmaps globally

ditto whitespace

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8533250 [details] [diff] [review]
Part 5: Refactor VsyncDispatcher to use the new architecture

Review of attachment 8533250 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +46,5 @@
>  
> +  if (gfxPrefs::VsyncAlignedCompositor() && mCompositorVsyncObserver) {
> +    mCompositorVsyncObserver->NotifyVsync(aVsyncTimestamp);
> +  } else {
> +    DispatchTouchEvents(aVsyncTimestamp);

Why is this in an else block?

@@ +76,5 @@
> +  // shuts down and the CompositorParent shuts down.
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(NS_IsMainThread());
> +  gfxPlatform::GetPlatform()->GetHardwareVsync()->RemoveVsyncDispatcher(this);
> +  mCompositorVsyncObserver = nullptr;

I'm not totally happy with this shutdown procedure but I'm not sure why. One thing is that the VsyncDispatcher auto-registers itself on construction but then the widget needs to explicitly call shutdown on it which seems wrong to me. Even if we do end up in the scenario that's described in the comment, where the we get "dead vsync notifications", how is that harmful? We'll pass it along to the mCompositorVsyncObserver which can either drop them or use them (since the compositor is still going).

It might help to actually describe the shutdown procedure for a widget - how you expect it to work, and which threads everything happens on. Put that in the design doc as well.

::: widget/VsyncDispatcher.h
@@ +52,4 @@
>  
>    // Compositor vsync observers must be added/removed on the compositor thread
>    void AddCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
>    void RemoveCompositorVsyncObserver(VsyncObserver* aVsyncObserver);

Since there is only one compositor-vsync-observer I would prefer to replace these two functions with a SetCompositorVsyncObserver(...). The caller can call it with a non-null or null value to add/remove.

Comment 27

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> Comment on attachment 8533250 [details] [diff] [review]
> Part 5: Refactor VsyncDispatcher to use the new architecture
> 
> Review of attachment 8533250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.cpp
> @@ +46,5 @@
> >  
> > +  if (gfxPrefs::VsyncAlignedCompositor() && mCompositorVsyncObserver) {
> > +    mCompositorVsyncObserver->NotifyVsync(aVsyncTimestamp);
> > +  } else {
> > +    DispatchTouchEvents(aVsyncTimestamp);
> 
> Why is this in an else block?

On b2g, scrolls can start a composite. If we have touch resampling enabled, we don't actually start compositing until we know we have to scroll, which means we have to dispatch touch moves. Touch moves require vsync, but we really always want to dispatch resampled touches after we composite otherwise we have a race condition and it's actually jankier. So in the normal case, touch events are dispatched after we composite. This else is required if we don't have a compositor listening to vsync events and get a touch event.

> 
> @@ +76,5 @@
> > +  // shuts down and the CompositorParent shuts down.
> > +  MOZ_ASSERT(XRE_IsParentProcess());
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  gfxPlatform::GetPlatform()->GetHardwareVsync()->RemoveVsyncDispatcher(this);
> > +  mCompositorVsyncObserver = nullptr;
> 
> I'm not totally happy with this shutdown procedure but I'm not sure why. One
> thing is that the VsyncDispatcher auto-registers itself on construction but
> then the widget needs to explicitly call shutdown on it which seems wrong to
> me. Even if we do end up in the scenario that's described in the comment,
> where the we get "dead vsync notifications", how is that harmful? We'll pass
> it along to the mCompositorVsyncObserver which can either drop them or use
> them (since the compositor is still going).
> 
> It might help to actually describe the shutdown procedure for a widget - how
> you expect it to work, and which threads everything happens on. Put that in
> the design doc as well.

I can update the design doc to include this. The dead vsync notifications occur when the Compositor stops listening to vsync events. Since the Compositor fetches the VsyncDispatcher through the nsBaseWidget, if the baseWidget is destroyed and we unobserve vsync, we'll have an invalid reference to the VsyncDispatcher. Does that clarify things?

> 
> ::: widget/VsyncDispatcher.h
> @@ +52,4 @@
> >  
> >    // Compositor vsync observers must be added/removed on the compositor thread
> >    void AddCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> >    void RemoveCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> 
> Since there is only one compositor-vsync-observer I would prefer to replace
> these two functions with a SetCompositorVsyncObserver(...). The caller can
> call it with a non-null or null value to add/remove.

Comment 28

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create the VsyncSource and Display Framework / Objects

Updated with feedback from comment 25.

Comment 29

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Refactor VsyncSource in OS X

Carrying r+. Updated to use an nsRefPtr to clean up the VsyncSource from comment 23.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8533236 [details] [diff] [review]
Part 1: Create VsyncDispatcher in nsBaseWidget

Review of attachment 8533236 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseWidget.cpp
@@ +230,5 @@
>    NS_IF_RELEASE(mContext);
>    delete mOriginalBounds;
> +
> +  // Can have base widgets that are things like tooltips which don't have vsyncDispatchers
> +  if (gfxPrefs::HardwareVsyncEnabled() && mVsyncDispatcher) {

Surely we don't need to check HardwareVsyncEnabled here? If there is a vsyncdispatcher we should shut it down.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8533332 [details] [diff] [review]
Part 3: Create the VsyncSource and Display Framework / Objects

Review of attachment 8533332 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.cpp
@@ +18,5 @@
> +  GetGlobalDisplay().AddVsyncDispatcher(aVsyncDispatcher);
> +}
> +
> +void
> +VsyncSource::Display::AddVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher)

Move this down in the file to just above VsyncSource::Display::RemoveVsyncDispatcher

::: gfx/thebes/VsyncSource.h
@@ +8,5 @@
> +#include "nsISupportsImpl.h"
> +#include "nsTArray.h"
> +
> +// Controls how and when to enable/disable vsync. Lives as long as the
> +// gfxPlatform does on the parent process

Move this comment down to just above the "class VsyncSource" line

::: gfx/thebes/gfxPlatform.cpp
@@ +548,5 @@
>      }
>  
>      RegisterStrongMemoryReporter(new GfxMemoryImageReporter());
>  
> +    if (XRE_IsParentProcess() && gfxPrefs::HardwareVsyncEnabled()) {

Do we not need the VsyncAlignedCompositor pref check any more?

::: gfx/thebes/gfxPlatform.h
@@ +24,5 @@
>  #include "mozilla/RefPtr.h"
>  #include "GfxInfoCollector.h"
>  
>  #include "mozilla/layers/CompositorTypes.h"
> +#include "mozilla/TimeStamp.h"

Shouldn't need this include any more

Comment 32

[Mason Chang [Inactive] [:mchang]]

Did I answer your questions from comment 26 in comment 27? Thanks.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #27)
> So in the
> normal case, touch events are dispatched after we composite.

Where is the code for this?

> I can update the design doc to include this. The dead vsync notifications
> occur when the Compositor stops listening to vsync events. Since the
> Compositor fetches the VsyncDispatcher through the nsBaseWidget, if the
> baseWidget is destroyed and we unobserve vsync, we'll have an invalid
> reference to the VsyncDispatcher. Does that clarify things?

The part 2 patch has the CompositorVsyncObserver holding an nsRefPtr to the widget. This means the widget will not be destroyed as long as the CompositorVsyncObserver is alive (i.e. until CompositorParent::Destroy runs). Therefore, when the CompositorVsyncObserver calls mWidget->GetVsyncDispatcher() it will always return a valid reference (or potentially null). In this case a null guard should be sufficient. Or maybe I'm missing something? It's really hard to follow and it would be great if you documented the design in this patch set so that I can first verify the design makes sense and then check that the code matches the design.

Comment 34

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> (In reply to Mason Chang [:mchang] from comment #27)
> > So in the
> > normal case, touch events are dispatched after we composite.

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#281

> 
> > I can update the design doc to include this. The dead vsync notifications
> > occur when the Compositor stops listening to vsync events. Since the
> > Compositor fetches the VsyncDispatcher through the nsBaseWidget, if the
> > baseWidget is destroyed and we unobserve vsync, we'll have an invalid
> > reference to the VsyncDispatcher. Does that clarify things?
> 
> The part 2 patch has the CompositorVsyncObserver holding an nsRefPtr to the
> widget. This means the widget will not be destroyed as long as the
> CompositorVsyncObserver is alive (i.e. until CompositorParent::Destroy
> runs). Therefore, when the CompositorVsyncObserver calls
> mWidget->GetVsyncDispatcher() it will always return a valid reference (or
> potentially null). In this case a null guard should be sufficient. Or maybe
> I'm missing something? It's really hard to follow and it would be great if
> you documented the design in this patch set so that I can first verify the
> design makes sense and then check that the code matches the design.

I'll write a design doc up before proceeding with another round of reviews. I could be missing something with how we shutdown the Compositor after the widget.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #34)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> > (In reply to Mason Chang [:mchang] from comment #27)
> > > So in the
> > > normal case, touch events are dispatched after we composite.
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp?from=CompositorParent.cpp&case=true#281

Ah, thanks, I totally missed that! In that case the code is fine, but please add a comment in that else block explaining this.

> I'll write a design doc up before proceeding with another round of reviews.
> I could be missing something with how we shutdown the Compositor after the
> widget.

Cool, thanks.

Comment 36

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #33)
> The part 2 patch has the CompositorVsyncObserver holding an nsRefPtr to the
> widget. This means the widget will not be destroyed as long as the
> CompositorVsyncObserver is alive (i.e. until CompositorParent::Destroy
> runs). Therefore, when the CompositorVsyncObserver calls
> mWidget->GetVsyncDispatcher() it will always return a valid reference (or
> potentially null). In this case a null guard should be sufficient. Or maybe
> I'm missing something? It's really hard to follow and it would be great if
> you documented the design in this patch set so that I can first verify the
> design makes sense and then check that the code matches the design.

Shutdown Process

When the nsBaseWidget's destructor runs - http://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?from=nsBaseWidget.cpp#221 - It calls nsBaseWidget::DestroyCompositor on the main thread. The main issue is that we destroy the Compositor through the nsBaseWidget, so the widget will not be kept alive by the nsRefPtr on the CompositorVsyncObserver.

During nsBaseWidget::DestroyCompositor, we first destroy the CompositorChild. This sends a sync IPC call to CompositorParent::RecvStop, which calls CompositorParent::Destroy here - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp#474. During this time, the main thread is blocked on the parent process. CompositorParent::Destroy runs on the Compositor thread and cleans up some resources, including setting the CompositorVsyncObserver to nullptr. CompositorParent::Destroy also explicitly keeps the CompositorParent alive and posts another task to run CompositorParent::DeferredDestroy on the Compositor loop so that all ipdl code can finish executing.

Once CompositorParent::RecvStop finishes, the main thread in the parent process continues destroying nsBaseWidget. nsBaseWidget posts another task to DeferedDestroyCompositor on the main thread(http://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#168). At the same time, the Compositor thread is executing tasks until CompositorParent::DeferredDestroy runs. Now we have a two tasks as both the nsBaseWidget:DeferredDestroyCompositor releases a reference to the Compositor on the main thread and the CompositorParent::DeferredDestroy releases a reference to the Compositor on the compositor thread. Finally, the CompositorParent itself is destroyed on the main thread once both deferred destroy's execute.

With the CompositorVsyncObserver, any accesses to the widget after nsBaseWidget::~nsBaseWidget executes are invalid. While the sync call to CompositorParent::RecvStop executes, we set the CompositorVsyncObserver to null. If the CompositorVsyncObserver's vsync notification executes on the hardware vsync thread, it will post a task to the Compositor loop and reference an invalid widget. In addition, posting a task to the CompositorLoop would also be invalid as we could destroy the Compositor before the Vsync's tasks executes. Any accesses to the widget between the time the nsBaseWidget's destructor runs and the CompositorVsyncObserver's destructor runs on the main thread aren't safe yet a hardware vsync event could occur between these times. I think it is safer to just explicitly stop vsync notifications during nsBaseWidget destruction instead of finding a more fragile approach. 

I don't think null checks are valid here either, unless we explicitly set the CompositorVsyncObserver::mWidget to null during CompositorParent::Destroy. I also don't want to add a null check for every vsync event.

Does this clarify things for you?

Comment 37

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP to Silk Documentation

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Thanks, this explanation helps a lot. The part i don't understand is how the nsBaseWidget destructor runs in the first place if the CompositorVsyncObserver still is holding a nsRefPtr to it. That should not happen.

I noticed that there is another call site to nsBaseWidget::DestroyCompositor from the shutdown listener in nsBaseWidget. I can see how this would work if that is the thing that is triggering this whole chain of events. However that will only work during shutdown and not when a window is closed. For example if you open a second window on OSX and then close it, there will be a reference cycle between that nsBaseWidget -> CompositorParent -> CompositorVsyncObserver -> nsBaseWidget. Either we need to let the cycle collector know about this or the entire set of objects will be leaked. At least that's my understanding. Thoughts?

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 8533239 [details] [diff] [review]
Part 2: Access VsyncDispatcher through nsIWidget interface

Review of attachment 8533239 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +202,4 @@
>    , mCurrentCompositeTaskMonitor("CurrentCompositeTaskMonitor")
>    , mCurrentCompositeTask(nullptr)
>  {
> +  MOZ_ASSERT(mWidget != nullptr);

Can you assert that this is on the main thread? I don't think nsIWidget ref counting is thread safe.

@@ +207,5 @@
>  
>  CompositorVsyncObserver::~CompositorVsyncObserver()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  // The VsyncDispatcher is cleaned up before this in the nsBaseWidget, which stops vsync listeners

This comment explains the interdiff but it wont be as clear in the tree without the removal of UnobserveVsync. If we're assuming that we're not observing vsync can we assert this here?

Comment 40

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Create VsyncDispatcher in nsBaseWidget

Carrying r+, Updated with feedback from comment 30.

Comment 41

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Access VsyncDispatcher through nsIWidget interface

Carrying r+, updated with feedback from comment 39.

Comment 42

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create the VsyncSource and Display Framework / Objects

Carrying r+, updated with feedback from comment 31. 

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +548,5 @@
> >      }
> >  
> >      RegisterStrongMemoryReporter(new GfxMemoryImageReporter());
> >  
> > +    if (XRE_IsParentProcess() && gfxPrefs::HardwareVsyncEnabled()) {
> 
> Do we not need the VsyncAlignedCompositor pref check any more?

That was probably a bug. The compositor is already behind a pref, and we have a separate pref just for hardware vsync enabled/disabled. We should probably consolidate this once we enable silk by default.

Comment 43

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

After thinking about this for a while, I think the proper thing is to have the CompositorVsyncObserver have an nsRefPtr<VsyncDispatcher> mVsyncDispatcher rather than a reference to the widget. At first I thought it'd be better to have a single place to always access the VsyncDispatcher through the mWidget, but the shutdown procedure starts at ~nsBaseWidget and the multi-threaded deferred destroy of the Compositor process makes shutting vsync down during the deferred process tricky.

I came around to having the CompositorVsyncObserver having a ref to the VsyncDispatcher since the CompositorParent does this too - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#1158. It takes a ref to the widget http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#354 - and nulls it later. I think it's better to follow the CompositorParent's behavior and not hold a ref to the widget. 

This also solves the problem of the cycle collector. The nsBaseWidget shuts down since we don't have any references from Vsync stuff. When the nsBaseWidget shuts down, it cleans up the VsyncDispatcher on the main thread, releasing the reference to the CompositorVsyncObserver. Then the compositor is destroyed and releases the last reference to the CompositorVsyncObserver, which can then get cleaned up. Thoughts?

Comment 44

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8535684 [details] [diff] [review]
Part 5: Refactor VsyncDispatcher to use the new architecture

Review of attachment 8535684 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +295,5 @@
>  void
>  CompositorVsyncObserver::ObserveVsync()
>  {
>    MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> +  mVsyncDispatcher->SetCompositorVsyncObserver(this);

We call ObserveVsync on compositor thread, right? Both vsync thread and compositor thread will access the same member. why we remove the mutex?

Comment 46

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #43)
> This also solves the problem of the cycle collector. The nsBaseWidget shuts
> down since we don't have any references from Vsync stuff. When the
> nsBaseWidget shuts down, it cleans up the VsyncDispatcher on the main
> thread, releasing the reference to the CompositorVsyncObserver. Then the
> compositor is destroyed and releases the last reference to the
> CompositorVsyncObserver, which can then get cleaned up. Thoughts?

Conceptually this sounds good. The part 2 patch on this bug still introduces the refptr to the widget though, are you planning on updating that? (Also I noticed the part 2 patch says r=kats when it was BenWa who reviewed it, please update that).

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8535684 [details] [diff] [review]
Part 5: Refactor VsyncDispatcher to use the new architecture

Review of attachment 8535684 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +47,5 @@
>  
> +  if (gfxPrefs::VsyncAlignedCompositor() && mCompositorVsyncObserver) {
> +    mCompositorVsyncObserver->NotifyVsync(aVsyncTimestamp);
> +  } else {
> +    DispatchTouchEvents(aVsyncTimestamp);

This bit still bugs me somewhat as bad design. I feel like there shouldn't be two different paths to the DispatchTouchEvents function. Here's what I'm thinking, correct me if I'm wrong:
1) The reason we have the DispatchTouchEvents here is for the case where the compositor isn't "running" and so the DispatchTouchEvents call inside the compositor doesn't get triggered
2) If the compositor isn't running, it unregisters for vsync. This presumably is an optimization which allows us to turn off hardware vsync notifications when we don't want them.
2a) If we do turn off hardware vsync notifications, then this function (VsyncDispatcher::NotifyVsync) won't get called at all, and so DispatchTouchEvents will not get called either.
2b) If we don't actually turn off hardware vsync notifications, then I think it would be better to just never unregister the compositor, but inside CompositorVsyncObserver::NotifyVsync, skip the code to post a composite task if it's not needed. Instead, that function can just call DispatchTouchEvents directly. I would argue this is better because it doesn't split the logic between the CompositorVsyncObserver and the VsyncDispatcher and have this implicit undocumented-but-required behaviour where VsyncDispatcher has to dispatch the touch events sometimes but not other times.

So unless I'm missing something, it seems like there's either a bug here or there's a better way to do this.

There's also the problem that GeckoTouchDispatcher::NotifyVsync will get called from each VsyncDispatcher now, instead of just once per vsync.

::: widget/VsyncDispatcher.h
@@ +36,5 @@
>  
>  // VsyncDispatcher is used to dispatch vsync events to the registered observers.
>  class VsyncDispatcher
>  {
> +   NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(VsyncDispatcher)

Do you need main thread destruction here? The destructor is empty anyway so I don't see the point. And we're not going to be having subclasses of this anyway.

Comment 48

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #47)
> Comment on attachment 8535684 [details] [diff] [review]
> Part 5: Refactor VsyncDispatcher to use the new architecture
> 
> Review of attachment 8535684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.cpp
> @@ +47,5 @@
> >  
> > +  if (gfxPrefs::VsyncAlignedCompositor() && mCompositorVsyncObserver) {
> > +    mCompositorVsyncObserver->NotifyVsync(aVsyncTimestamp);
> > +  } else {
> > +    DispatchTouchEvents(aVsyncTimestamp);
> 
> This bit still bugs me somewhat as bad design. I feel like there shouldn't
> be two different paths to the DispatchTouchEvents function. Here's what I'm
> thinking, correct me if I'm wrong:
> 1) The reason we have the DispatchTouchEvents here is for the case where the
> compositor isn't "running" and so the DispatchTouchEvents call inside the
> compositor doesn't get triggered
> 2) If the compositor isn't running, it unregisters for vsync. This
> presumably is an optimization which allows us to turn off hardware vsync
> notifications when we don't want them.
> 2a) If we do turn off hardware vsync notifications, then this function
> (VsyncDispatcher::NotifyVsync) won't get called at all, and so
> DispatchTouchEvents will not get called either.
> 2b) If we don't actually turn off hardware vsync notifications, then I think
> it would be better to just never unregister the compositor, but inside
> CompositorVsyncObserver::NotifyVsync, skip the code to post a composite task
> if it's not needed. Instead, that function can just call DispatchTouchEvents
> directly. I would argue this is better because it doesn't split the logic
> between the CompositorVsyncObserver and the VsyncDispatcher and have this
> implicit undocumented-but-required behaviour where VsyncDispatcher has to
> dispatch the touch events sometimes but not other times.
> 
> So unless I'm missing something, it seems like there's either a bug here or
> there's a better way to do this.

The logic you described is correct. But we do wish to turn off hardware vsync notifications for when the phone is idle to save battery. It also means that when we get a touch event but vsync is off, we have to modify GeckoTouchDispatcher to enable hardware vsync on touch starts. If we don't have to turn off hardware vsync, I agree that it would be better to just skip events in the CompositorVsyncObserver and dispatch touch events if need be.

If we really want to only have 1 path to DispatchTouchEvents, we could notify the Compositor in GeckoTouchDispatcher to start listening to vsync on touch starts. The logic being that since we got a touch event, we will probably have to composite soon to respond to user input. What do you think about this approach?

> 
> There's also the problem that GeckoTouchDispatcher::NotifyVsync will get
> called from each VsyncDispatcher now, instead of just once per vsync.
> 

Hmm, that shouldn't be the case. By design, b2g only has 1 CompositorParent and thus only 1 VsyncDispatcher. If b2g ever gets more than 1 CompositorParent, this would be an issue, but I assume lots of things would have to be rewritten for this case.

> ::: widget/VsyncDispatcher.h
> @@ +36,5 @@
> >  
> >  // VsyncDispatcher is used to dispatch vsync events to the registered observers.
> >  class VsyncDispatcher
> >  {
> > +   NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(VsyncDispatcher)
> 
> Do you need main thread destruction here? The destructor is empty anyway so
> I don't see the point. And we're not going to be having subclasses of this
> anyway.

We are going to have Subclasses of this with the RefreshDriver. We will have the CompositorVsyncDispatcher and the RefreshTimerVsyncDispatcher. Both the CompositorParent and RefreshTimer are destroyed on the main thread along with the nsBaseWidget, which is why we should destroy the VsyncDispatcher on the same thread as well.

Comment 49

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #48)
> The logic you described is correct. But we do wish to turn off hardware
> vsync notifications for when the phone is idle to save battery. It also
> means that when we get a touch event but vsync is off, we have to modify
> GeckoTouchDispatcher to enable hardware vsync on touch starts.

Hm, so now there are three interaction points between touch events and vsync. One is in the no-vsync case, one is in the vsync-but-no-compositor case and one is in the vsync-and-compositor case.

> If we really want to only have 1 path to DispatchTouchEvents, we could
> notify the Compositor in GeckoTouchDispatcher to start listening to vsync on
> touch starts. The logic being that since we got a touch event, we will
> probably have to composite soon to respond to user input. What do you think
> about this approach?

Yeah I prefer this. If we need to turn on vsync notifications once we get a touchstart anyway, the additional cost to send the notification to the compositor as well is very little, and as you say will we will probably be compositing soon anyway.

> 
> > 
> > There's also the problem that GeckoTouchDispatcher::NotifyVsync will get
> > called from each VsyncDispatcher now, instead of just once per vsync.
> > 
> 
> Hmm, that shouldn't be the case. By design, b2g only has 1 CompositorParent
> and thus only 1 VsyncDispatcher. If b2g ever gets more than 1
> CompositorParent, this would be an issue, but I assume lots of things would
> have to be rewritten for this case.

Ok, fair enough. Might be an issue for other platforms but GeckoTouchDispatcher is specific to B2G so we don't need to worry about this right now.

> > ::: widget/VsyncDispatcher.h
> > @@ +36,5 @@
> > >  
> > >  // VsyncDispatcher is used to dispatch vsync events to the registered observers.
> > >  class VsyncDispatcher
> > >  {
> > > +   NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(VsyncDispatcher)
> > 
> > Do you need main thread destruction here? The destructor is empty anyway so
> > I don't see the point. And we're not going to be having subclasses of this
> > anyway.
> 
> We are going to have Subclasses of this with the RefreshDriver. We will have
> the CompositorVsyncDispatcher and the RefreshTimerVsyncDispatcher. Both the
> CompositorParent and RefreshTimer are destroyed on the main thread along
> with the nsBaseWidget, which is why we should destroy the VsyncDispatcher on
> the same thread as well.

It's not clear to me why we "should" destroy the VsyncDispatcher on main thread. What bad stuff happens if we don't? Also if the CompositorParent, RefreshTimer, and nsBaseWidget are all already being destroyed on the main thread, most likely that's when the last refptr to the VsyncDispatcher will get nulled out, and that's when the VsyncDispatcher will get destroyed anyway. So even without the _WITH_MAIN_THREAD_DESTRUCTION the VsyncDispatcher should get destroyed on the main thread.

Comment 50

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #45)
> Comment on attachment 8535684 [details] [diff] [review]
> Part 5: Refactor VsyncDispatcher to use the new architecture
> 
> Review of attachment 8535684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +295,5 @@
> >  void
> >  CompositorVsyncObserver::ObserveVsync()
> >  {
> >    MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> > +  mVsyncDispatcher->SetCompositorVsyncObserver(this);
> 
> We call ObserveVsync on compositor thread, right? Both vsync thread and
> compositor thread will access the same member. why we remove the mutex?

Yes you're right, I'll add it back in.

Comment 51

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Access VsyncDispatcher through nsIWidget interface

Carrying r+. CompositorVsyncObserver holds a reference to mVsyncDispatcher rather than the widget as discussed in comment 43.

Comment 52

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

Updated with review feedback from comment 49. A couple of things:

1) Remove all references from NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION and use NS_INLINE_DECL_THREADSAFE_REFCOUNTING for Vsyncdispatcher/VsyncObservers. Add assertions on which thread they should be destroyed on.
2) Tell the Compositor to observe vsync from GeckoTouchDispatcher on touch starts.
3) Remove http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#279. There are some timing issues on b2g where we unobserve vsync too fast so when a touch event occurs, we still don't dispatch the touch events. I will follow up on bug 1095242 to properly enable/disable vsync.

This means right now, the Compositor essentially never unobserves vsync until shutdown, but I'd like to fix this in a follow up bug.

Comment 53

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 6: Create VsyncSource on b2g

Create a VsyncSource object on Gonk in GfxAndroidPlatform.

Comment 54

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8537313 [details] [diff] [review]
Part 2: Access VsyncDispatcher through nsIWidget interface

Review of attachment 8537313 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +204,5 @@
>    , mCurrentCompositeTask(nullptr)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mWidget != nullptr);
> +  mVsyncDispatcher = mWidget->GetVsyncDispatcher();

This shouldn't compile, there's no mWidget here. s/mWidget/aWidget/

Comment 56

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8535064 [details] [diff] [review]
Part 3: Create the VsyncSource and Display Framework / Objects

Review of attachment 8535064 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.h
@@ +23,5 @@
> +  class Display {
> +    public:
> +      Display();
> +      virtual ~Display();
> +      // Should only be called on the main thread

Would be better to enforce these thread assertions using MOZ_ASSERT inside the functions.

MOZ_ASSERT(NS_IsMainThread()) for the main thread
Compositor::AssertOnCompositorThread() for the compositor thread

::: gfx/thebes/gfxPlatform.h
@@ +605,5 @@
>      /**
>       * Initialized hardware vsync based on each platform.
>       */
> +    virtual already_AddRefed<mozilla::gfx::VsyncSource> CreateHardwareVsyncSource() {
> +      MOZ_CRASH("Hardware vsync not supported on platform yet");

Might be nice to remove this crash before landing the code, because if somebody flips the pref on an unsupported platform they won't be able to start the browser up again to fix it.

Comment 57

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8533340 [details] [diff] [review]
Part 4: Refactor VsyncSource in OS X

Review of attachment 8533340 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatformMac.cpp
@@ +465,5 @@
>      }
>  
> +    virtual void EnableVsync() MOZ_OVERRIDE
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());

The part 3 patch said that EnableVsync() and DisableVsync() should only be called on the compositor thread, but here (and in DisableVsync) you're asserting the main thread. Something's wrong. You should probably run a debug build to exercise these assertions before landing.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8537329 [details] [diff] [review]
Part 5: Refactor VsyncDispatcher to use the new architecture

Review of attachment 8537329 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +207,5 @@
>    MOZ_ASSERT(aWidget != nullptr);
>    mVsyncDispatcher = aWidget->GetVsyncDispatcher();
> +#ifdef MOZ_WIDGET_GONK
> +  if (gfxPrefs::TouchResampling()) {
> +    GeckoTouchDispatcher::SetCompositorVsyncObserver(this);

I'd prefer to move the pref check inside the function, so that the pref is encapsulated inside GeckoTouchDispatcher.

@@ +435,5 @@
>    }
>    sIndirectLayerTrees.erase(mRootLayerTreeID);
> +  if (mCompositorVsyncObserver) {
> +    mCompositorVsyncObserver->UnobserveVsync();
> +    mCompositorVsyncObserver = nullptr;

So technically the mCompositorVsyncObserver will get leaked because GeckoTouchDispatcher::sTouchDispatcher is still holding a nsRefPtr to it that never gets cleared. I don't really care too much since this only happens on B2G with touch-resampling enabled and the leak is a shutdown leak so it doesn't matter. The obvious fix is to call GeckoTouchDispatcher::SetCompositorVsyncObserver(nullptr) here but it won't be that simple because this code is running on the compositor thread and that function assumes main-thread. If you can find a clean way to do that go for it but otherwise don't worry about it. Do add a comment here though that says that we might leak this on B2G.

::: gfx/layers/ipc/CompositorParent.h
@@ -104,5 @@
>    virtual bool NotifyVsync(TimeStamp aVsyncTimestamp) MOZ_OVERRIDE;
>    void SetNeedsComposite(bool aSchedule);
>    bool NeedsComposite();
>    void CancelCurrentCompositeTask();
> - 

spurious whitespace change

::: widget/VsyncDispatcher.cpp
@@ +57,2 @@
>  {
> +  MOZ_ASSERT(CompositorParent::IsInCompositorThread());

this works too, no need to change it to the other variant I posted in a previous comment

::: widget/VsyncDispatcher.h
@@ +36,5 @@
>  
>  // VsyncDispatcher is used to dispatch vsync events to the registered observers.
>  class VsyncDispatcher
>  {
> +   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VsyncDispatcher)

nit: indent

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +120,5 @@
> +GeckoTouchDispatcher::SetCompositorVsyncObserver(mozilla::layers::CompositorVsyncObserver *aObserver)
> +{
> +  MOZ_ASSERT(sTouchDispatcher != nullptr);
> +  MOZ_ASSERT(NS_IsMainThread());
> +  sTouchDispatcher->mCompositorVsyncObserver = aObserver;

also MOZ_ASSERT(sTouchDispatcher->mCompositorVsyncObserver == nullptr) here with a comment that we are assuming there will only be one compositor on b2g.

@@ +150,5 @@
>  void
>  GeckoTouchDispatcher::NotifyTouch(MultiTouchInput& aTouch, TimeStamp aEventTime)
>  {
> +  if (aTouch.mType == MultiTouchInput::MULTITOUCH_START && mResamplingEnabled) {
> +    MutexAutoLock lock(mTouchQueueLock);

What is the purpose of this lock? If it's to protect access to mCompositorVsyncObserver then you need to pick up the lock in SetCompositorVsyncObserver as well. But it doesn't make sense to be for that because you're not guarding against a null mCompositorVsyncObserver anyway, so you might as well remove this lock.

I do think it would be a good idea to add a "&& mCompositorVsyncObserver" to the if condition though so that we're not assuming the compositor vsync observer is registered before we get the first touch event. (I tried looking through the widget code to see what order things are initialized in and it wasn't clear to me, so probably better not to rely on a particular ordering).

@@ +153,5 @@
> +  if (aTouch.mType == MultiTouchInput::MULTITOUCH_START && mResamplingEnabled) {
> +    MutexAutoLock lock(mTouchQueueLock);
> +    // If we get a touch start, make sure that the compositor starts listening
> +    // to vsync as well.
> +    sTouchDispatcher->mCompositorVsyncObserver->SetNeedsComposite(true);

You don't need the sTouchDispatcher-> here

@@ +155,5 @@
> +    // If we get a touch start, make sure that the compositor starts listening
> +    // to vsync as well.
> +    sTouchDispatcher->mCompositorVsyncObserver->SetNeedsComposite(true);
> +  }
> +  

nit: trailing ws

Comment 59

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8537318 [details] [diff] [review]
Part 6: Create VsyncSource on b2g

Review of attachment 8537318 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing because it looks like this patch isn't the right version, maybe. See comments below.

::: gfx/thebes/gfxAndroidPlatform.cpp
@@ +452,5 @@
> +    }
> +
> +    virtual void EnableVsync() MOZ_OVERRIDE
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());

Again the VsyncSource interface says this should only ever get called on the compositor thread. At this point I'm assuming the VsyncSource interface documentation is wrong and the code is fine.

@@ +453,5 @@
> +
> +    virtual void EnableVsync() MOZ_OVERRIDE
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +      mVsyncEnabled = HwcComposer2D::GetInstance()->EnableVsync(true);

EnableVsync returns void, you need to set mVsyncEnabled = true explicitly. Does this even compile?

@@ +459,5 @@
> +
> +    virtual void DisableVsync() MOZ_OVERRIDE
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +      mVsyncEnabled = HwcComposer2D::GetInstance()->EnableVsync(false);

mVsyncEnabled = false;

@@ +465,5 @@
> +
> +    virtual bool IsVsyncEnabled() MOZ_OVERRIDE
> +    {
> +        MOZ_ASSERT(NS_IsMainThread());
> +        return mVsyncEnabled;

nit: indentation

@@ +476,5 @@
> +  virtual ~GonkVsyncSource()
> +  {
> +  }
> +
> +  GonkDisplay mGlobalDisplay;

Where is mGlobalDisplay instantiated?

@@ +487,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    nsRefPtr<VsyncSource> vsyncSource = new GonkVsyncSource();
> +    return vsyncSource.forget();
> +#else
> +    MOZ_CRASH("Hardware vsync not supported on android yet");

ditto my previous comment on not crashing.

::: widget/gonk/nsAppShell.cpp
@@ +72,5 @@
>  #include "GeckoProfiler.h"
>  
>  // Defines kKeyMapping and GetKeyNameIndex()
>  #include "GonkKeyMapping.h"
> +#include "mozilla/layers/CompositorParent.h"

This doesn't belong in this patch

::: widget/gonk/nsWindow.cpp
@@ +212,5 @@
>          listener->DidPaintWindow();
>      }
>  }
>  
> +void

/*static*/ void

@@ +221,5 @@
> +    }
> +
> +    VsyncDispatcher* vsyncDispatcher = gFocusedWindow->GetVsyncDispatcher();
> +    // During bootup, there is a delay between when the nsWindow is created
> +    // and when the Compositor is created, but vsync is already turned on

thanks for the comment!

Comment 60

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #57)
> Comment on attachment 8533340 [details] [diff] [review]
> Part 4: Refactor VsyncSource in OS X
> 
> Review of attachment 8533340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxPlatformMac.cpp
> @@ +465,5 @@
> >      }
> >  
> > +    virtual void EnableVsync() MOZ_OVERRIDE
> > +    {
> > +      MOZ_ASSERT(NS_IsMainThread());
> 
> The part 3 patch said that EnableVsync() and DisableVsync() should only be
> called on the compositor thread, but here (and in DisableVsync) you're
> asserting the main thread. Something's wrong. You should probably run a
> debug build to exercise these assertions before landing.

Ahh bad comment. Yeah I've been running debug builds, all the code should be asserting they are on the main thread.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #58)
> Comment on attachment 8537329 [details] [diff] [review]
> Part 5: Refactor VsyncDispatcher to use the new architecture
> 
> Review of attachment 8537329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +207,5 @@
> >    MOZ_ASSERT(aWidget != nullptr);
> >    mVsyncDispatcher = aWidget->GetVsyncDispatcher();
> > +#ifdef MOZ_WIDGET_GONK
> > +  if (gfxPrefs::TouchResampling()) {
> > +    GeckoTouchDispatcher::SetCompositorVsyncObserver(this);
> 
> I'd prefer to move the pref check inside the function, so that the pref is
> encapsulated inside GeckoTouchDispatcher.

We already check the pref here - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp&case=true#316
So it's not really encapsulated.

> 
> @@ +435,5 @@
> >    }
> >    sIndirectLayerTrees.erase(mRootLayerTreeID);
> > +  if (mCompositorVsyncObserver) {
> > +    mCompositorVsyncObserver->UnobserveVsync();
> > +    mCompositorVsyncObserver = nullptr;
> 
> So technically the mCompositorVsyncObserver will get leaked because
> GeckoTouchDispatcher::sTouchDispatcher is still holding a nsRefPtr to it
> that never gets cleared. I don't really care too much since this only
> happens on B2G with touch-resampling enabled and the leak is a shutdown leak
> so it doesn't matter. The obvious fix is to call
> GeckoTouchDispatcher::SetCompositorVsyncObserver(nullptr) here but it won't
> be that simple because this code is running on the compositor thread and
> that function assumes main-thread. If you can find a clean way to do that go
> for it but otherwise don't worry about it. Do add a comment here though that
> says that we might leak this on B2G.

Is this the case? We the GeckoTouchDispatcher on shutdown:
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/GeckoTouchDispatcher.cpp?from=GeckoTouchDispatcher.cpp&case=true#73
Which should release the last reference?

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #59)
> Comment on attachment 8537318 [details] [diff] [review]
> Part 6: Create VsyncSource on b2g
> @@ +453,5 @@
> > +
> > +    virtual void EnableVsync() MOZ_OVERRIDE
> > +    {
> > +      MOZ_ASSERT(NS_IsMainThread());
> > +      mVsyncEnabled = HwcComposer2D::GetInstance()->EnableVsync(true);
> 
> EnableVsync returns void, you need to set mVsyncEnabled = true explicitly.
> Does this even compile?

This relies on bug 1102453, which has EnableVsync return true/false if successful in HwcComposer on b2g.
 
> @@ +476,5 @@
> > +  virtual ~GonkVsyncSource()
> > +  {
> > +  }
> > +
> > +  GonkDisplay mGlobalDisplay;
> 
> Where is mGlobalDisplay instantiated?

It's instantiated when the vsync source is created. It's not a pointer so it lives/dies the same time the VsyncSource is created.
 
> ::: widget/gonk/nsAppShell.cpp
> @@ +72,5 @@
> >  #include "GeckoProfiler.h"
> >  
> >  // Defines kKeyMapping and GetKeyNameIndex()
> >  #include "GonkKeyMapping.h"
> > +#include "mozilla/layers/CompositorParent.h"
> 
> This doesn't belong in this patch

Do we have a policy on what's better? The problem is that GeckoTouchDispatcher needs a definition for the CompositorVsyncObserver, which is in CompositorParent.h. I wasn't sure if I should just include the CompositorParent.h in GeckoTouchDispatcher.h or in the users who include GeckoTouchDispatcher.h

I'll fix the other issues.

Comment 61

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mason Chang [:mchang] from comment #60)
> Ahh bad comment. Yeah I've been running debug builds, all the code should be
> asserting they are on the main thread.

Ok, cool.

> > I'd prefer to move the pref check inside the function, so that the pref is
> > encapsulated inside GeckoTouchDispatcher.
> 
> We already check the pref here -
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp?from=CompositorParent.cpp&case=true#316
> So it's not really encapsulated.

Can we move that one in as well?

> Is this the case? We the GeckoTouchDispatcher on shutdown:
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/
> GeckoTouchDispatcher.cpp?from=GeckoTouchDispatcher.cpp&case=true#73
> Which should release the last reference?

Ah, good point. Yeah that should work.

> > 
> > EnableVsync returns void, you need to set mVsyncEnabled = true explicitly.
> > Does this even compile?
> 
> This relies on bug 1102453, which has EnableVsync return true/false if
> successful in HwcComposer on b2g.

Tricksy! Ok, that works.

> > > +  GonkDisplay mGlobalDisplay;
> > 
> > Where is mGlobalDisplay instantiated?
> 
> It's instantiated when the vsync source is created. It's not a pointer so it
> lives/dies the same time the VsyncSource is created.

I should stop reviewing code at stupidly early hours when I can't sleep :) Sorry, I should've noticed that.

> > > +#include "mozilla/layers/CompositorParent.h"
> > 
> > This doesn't belong in this patch
> 
> Do we have a policy on what's better? The problem is that
> GeckoTouchDispatcher needs a definition for the CompositorVsyncObserver,
> which is in CompositorParent.h. I wasn't sure if I should just include the
> CompositorParent.h in GeckoTouchDispatcher.h or in the users who include
> GeckoTouchDispatcher.h

But you already have a forward-declaration of CompositorVsyncObserver in GeckoTouchDispatcher.h, that should be sufficient. You need to include CompositorParent.h in GeckoTouchDispatcher.cpp so that it can find the full declaration and you can call methods on it, but you already did that in part 5. You didn't add anything to nsAppShell.cpp in part 6 so I'm not sure why you needed to add this include. Was there something in nsAppShell.cpp that required the CompositorVsyncObserver declaration?

Comment 62

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8537318 [details] [diff] [review]
Part 6: Create VsyncSource on b2g

Review of attachment 8537318 [details] [diff] [review]:
-----------------------------------------------------------------

Changing to + since the big things I was concerned about were just me not reading the code right and stuff.

Comment 63

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #61)
> 
> But you already have a forward-declaration of CompositorVsyncObserver in
> GeckoTouchDispatcher.h, that should be sufficient. You need to include
> CompositorParent.h in GeckoTouchDispatcher.cpp so that it can find the full
> declaration and you can call methods on it, but you already did that in part
> 5. You didn't add anything to nsAppShell.cpp in part 6 so I'm not sure why
> you needed to add this include. Was there something in nsAppShell.cpp that
> required the CompositorVsyncObserver declaration?

nsAppShell actually creates the GeckoTouchDispatcher - http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#502, so it still needs the full definition of CompositorVsyncObserver to instantiate the GeckoTouchDispatcher. Unless there is some C++ jutsu I'm missing?

Comment 64

[Kartikaya Gupta (email:[e-mail])]

What's the actual compiler error you get if you remove that include?

Comment 65

[Mason Chang [Inactive] [:mchang]]

Attachment: Compiler Error

Compiler error from not including "mozilla/layers/CompositorParent.h"

Comment 66

[Kartikaya Gupta (email:[e-mail])]

(As mentioned on IRC this error doesn't make sense to me, but let's not hold up landing this just because of that. I can take a look at removing the include after it's landed).

Comment 67

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Create VsyncDispatcher in nsBaseWidget. r=roc

Carrying r+, rebased on master.

Comment 68

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Access VsyncDispatcher through nsIWidget interface. r=benwa

Carrying r+, rebased on master.

Comment 69

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create the VsyncSource and Display Framework / Objects. r=kats

Carrying r+, rebased on master.

Comment 70

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Refactor VsyncSource in OS X. r=mstange

Carrying r+, rebased on master.

Comment 71

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Refactor VsyncDispatcher to use the new architecture. r=kats

Carrying r+, rebased on master.

Comment 72

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 6: Create VsyncSource on b2g. r=kats

Carrying r+, rebased on master and with feedback from comment 61.

Comment 73

[Mason Chang [Inactive] [:mchang]]

Attachment: All Parts Combined

In case anyone needs it as one.

Comment 74

[Mason Chang [Inactive] [:mchang]]

Try looks mostly good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb2faa2d4a39

However, mochitest-3 on e10s on Linux debug looks pretty intermittent. I looked at the starred bugs and see that it occurs somewhat regularly, but not sure if this changeset makes the intermittent occur more often or I got unlucky.

Trying just mochitest-3 on e10s after updating to m-c today - https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c72aa9d465

Comment 75

[Mason Chang [Inactive] [:mchang]]

Spoke with Ryan, try looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4177b97a946c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0c7dd4925f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b07cb2752e86
https://hg.mozilla.org/integration/mozilla-inbound/rev/5faeb9df6fc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88dedd047ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/26dd6674c459

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4177b97a946c
https://hg.mozilla.org/mozilla-central/rev/dc0c7dd4925f
https://hg.mozilla.org/mozilla-central/rev/b07cb2752e86
https://hg.mozilla.org/mozilla-central/rev/5faeb9df6fc2
https://hg.mozilla.org/mozilla-central/rev/f88dedd047ae
https://hg.mozilla.org/mozilla-central/rev/26dd6674c459