987529 - Integrate vsync dispatcher on B2G

Status: RESOLVED DUPLICATE of bug 1092978

(Core Graveyard :: Widget: Gonk, defect, P1)

Description

[Vincent Lin[:vilin]]

A part of Project Butter on B2G. It's going to implement an observer manager for vsync in gecko/hal. It provides IPC for register, unregister and get Vsync timestamp.

Comment 1

[Sotaro Ikeda [:sotaro]]

Can we have good enough performance by using gecko IPC? In android, socket(wrapped by BitTube) is used to deliver vsync to an application.

Comment 2

[Vincent Lin[:vilin]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Can we have good enough performance by using gecko IPC? In android,
> socket(wrapped by BitTube) is used to deliver vsync to an application.

Very good concern.
I've found that.
Gecko IPC is indeed not as good as Android BitTube which impacts not only vsync delivery, but also touch events. But better than do nothing.

I think the IPC performance is not the critical issue while threading issue is.
Vsync delivery is running on main thread. In the case of system app, it's seriously blocked by reflow and painting. Making it be off main thread is my next plan, but I would expect to have current one being landed first.

Comment 3

[Vincent Lin[:vilin]]

Attachment: bug-987529-fix.patch

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 8398388 [details] [diff] [review]
bug-987529-fix.patch

>diff --git a/hal/HalVsyncInfo.h b/hal/HalVsyncInfo.h
>new file mode 100644
>--- /dev/null
>+++ b/hal/HalVsyncInfo.h
>@@ -0,0 +1,20 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et ft=cpp : */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_HalVsyncInfo_h
>+#define mozilla_HalVsyncInfo_h
>+
>+#include "mozilla/Observer.h"
>+
>+namespace mozilla {
>+namespace hal {
>+struct VsyncInfo;
>+typedef Observer<VsyncInfo> VsyncObserver;
>+} // namespace hal
>+} // namespace mozilla
>+
>+#endif  // mozilla_HalVsyncInfo_h
>+

Maybe you want to put this in HalTypes.h instead of creating a new
file?

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+struct VsyncInfo {
>+  int64_t timestamp;
>+};
>+

The mechanics of this patch look perfectly fine to me (except for
comment above).  However, I'm not sure about the use of this
notification mechanism.  IPC messages can have pretty much arbtirary
delay before being delivered (for multiple reasons), so a vsync
notification might not be delivered until seconds after it's sent.
And PHal operates from master-process-main-thread <-->
content-process-main-thread.  I would have expected vsync'ing to be
managed by PCompositor.

So I can definitely say this code looks OK, but I don't know enough
about the bigger project to r+ :).  Sorry.  Please ask someone who's
familiar with the bigger picture for review :).

Comment 5

[Vincent Lin[:vilin]]

(In reply to Chris Jones [:cjones] mostly inactive; ni?/f?/r? if you need me from comment #4)
> Comment on attachment 8398388 [details] [diff] [review]
> bug-987529-fix.patch
> 
> >diff --git a/hal/HalVsyncInfo.h b/hal/HalVsyncInfo.h
> >new file mode 100644
> >--- /dev/null
> >+++ b/hal/HalVsyncInfo.h
> >@@ -0,0 +1,20 @@
> >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: set sw=2 ts=8 et ft=cpp : */
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+#ifndef mozilla_HalVsyncInfo_h
> >+#define mozilla_HalVsyncInfo_h
> >+
> >+#include "mozilla/Observer.h"
> >+
> >+namespace mozilla {
> >+namespace hal {
> >+struct VsyncInfo;
> >+typedef Observer<VsyncInfo> VsyncObserver;
> >+} // namespace hal
> >+} // namespace mozilla
> >+
> >+#endif  // mozilla_HalVsyncInfo_h
> >+
> 
> Maybe you want to put this in HalTypes.h instead of creating a new
> file?
> 
> >diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
> 
> >+struct VsyncInfo {
> >+  int64_t timestamp;
> >+};
> >+
> 
> The mechanics of this patch look perfectly fine to me (except for
> comment above).  However, I'm not sure about the use of this
> notification mechanism.  IPC messages can have pretty much arbtirary
> delay before being delivered (for multiple reasons), so a vsync
> notification might not be delivered until seconds after it's sent.
> And PHal operates from master-process-main-thread <-->
> content-process-main-thread.  I would have expected vsync'ing to be
> managed by PCompositor.
> 
> So I can definitely say this code looks OK, but I don't know enough
> about the bigger project to r+ :).  Sorry.  Please ask someone who's
> familiar with the bigger picture for review :).

The bigger project is Bug 987532. Not only compositor, but also RefreshDriver and InputDispatcher will observe it. I knew your concern as I've mentioned in Comment 2. Currently, I think this way can fulfill most scenarios. I will discuss this issue in gfx work week this week. To see if PHal solution for vsync can be the short-tern solution.

Comment 6

[Vincent Lin[:vilin]]

Comment on attachment 8398388 [details] [diff] [review]
bug-987529-fix.patch

As we've discussed in "design document", here is the VsyncObserverManager implemented for vsync notification. It's gaurenteed to notify InputDispatcher, Compositor and RefreshDriver in order due to these observers register to it in the same order. Currently, VsyncObserverManager relies on PHAL which runs in main thread for some reasons. I prefer to land this first and move it to another thread for better performance in the future.

https://etherpad.mozilla.org/vsync-butter

Please also see Comment 4 of this bug.

Comment 7

[StevenLee[:slee]]

Comment on attachment 8398388 [details] [diff] [review]
bug-987529-fix.patch

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

You should implement fallback path for the platforms do not have Vsync events.

::: hal/gonk/GonkHal.cpp
@@ +868,5 @@
>  
> +void
> +EnableVsyncNotifications()
> +{
> +}

Please add comments why these 2 functions are empty.

Comment 8

[Vincent Lin[:vilin]]

Attachment: bug-987529-fix.patch

Update to assure the notification order.
1. Input. 2. Compositor. 3. Refresh.

Comment 9

[Vincent Lin[:vilin]]

Attachment: bug-987529-fix.patch

Update to assure the notification order.
1. Input. 2. Compositor. 3. Refresh.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8405288 [details] [diff] [review]
bug-987529-fix.patch

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

::: hal/Hal.cpp
@@ +749,5 @@
>    AssertMainThread();
>    PROXY_IF_SANDBOXED(UnlockScreenOrientation());
>  }
>  
> +typedef mozilla::ObserverList<VsyncInfo> VsyncObserverList;

We should only have one Vsync observer so we should not need this list. This patch looks like it still tries to support multiple vsync observers which we do not want to do.

::: hal/sandbox/PHal.ipdl
@@ +110,5 @@
>      NotifyBatteryChange(BatteryInformation aBatteryInfo);
>      NotifyNetworkChange(NetworkInformation aNetworkInfo);
>      NotifyWakeLockChange(WakeLockInformation aWakeLockInfo);
>      NotifyScreenConfigurationChange(ScreenConfiguration aScreenOrientation);
> +    NotifyVsyncChange(VsyncInfo aVsyncInfo);

We need to notify of vsync using the same protocol as we send input so that we can ensure that the input always arrives first.

@@ +174,5 @@
>   
> +    EnableVsyncNotifications();
> +    DisableVsyncNotifications();
> +    sync GetCurrentVsyncInfo()
> +      returns (VsyncInfo aVsyncInfo);

What is GetCurrentVsyncInfo() used for? i.e what are the callers of this?

Comment 11

[Vincent Lin[:vilin]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 8405288 [details] [diff] [review]
> bug-987529-fix.patch
> 
> Review of attachment 8405288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/Hal.cpp
> @@ +749,5 @@
> >    AssertMainThread();
> >    PROXY_IF_SANDBOXED(UnlockScreenOrientation());
> >  }
> >  
> > +typedef mozilla::ObserverList<VsyncInfo> VsyncObserverList;
> 
> We should only have one Vsync observer so we should not need this list. This
> patch looks like it still tries to support multiple vsync observers which we
> do not want to do.

I can't understand why we should have only one observer ? May we have more discussion on this ?
I thought your concern is just if input can be dispatched first or not, right ?
In my current WIP, it can guarantee the order of notification and Compositor definitely can update the up-to-date APZC information.

In the design document(line85~93), is that the conclusion Jerry discussed with you on the final day of gfx work week ?
https://etherpad.mozilla.org/vsync-butter
Then I think it's better to make CompositorParent and RefreshDriver being observers. That provides more flexible design for these observers to run-timely switch on/off observation.

> 
> ::: hal/sandbox/PHal.ipdl
> @@ +110,5 @@
> >      NotifyBatteryChange(BatteryInformation aBatteryInfo);
> >      NotifyNetworkChange(NetworkInformation aNetworkInfo);
> >      NotifyWakeLockChange(WakeLockInformation aWakeLockInfo);
> >      NotifyScreenConfigurationChange(ScreenConfiguration aScreenOrientation);
> > +    NotifyVsyncChange(VsyncInfo aVsyncInfo);
> 
> We need to notify of vsync using the same protocol as we send input so that
> we can ensure that the input always arrives first.

This bug is just for observer manager implementation.

3 kinds of observers being implemented in different bugs.
Bug 980241 - Vsync-triggered RefreshDriver
Bug 987523 - Vsync-triggered CompositorParent
Bug 970751 - Deliver input events more consistently per composite to make scrolling smoother

They just need to call hal::RegisterVsyncObserver() and hal::UnregisterVsyncObserver() to switch on/off the observation.

> 
> @@ +174,5 @@
> >   
> > +    EnableVsyncNotifications();
> > +    DisableVsyncNotifications();
> > +    sync GetCurrentVsyncInfo()
> > +      returns (VsyncInfo aVsyncInfo);
> 
> What is GetCurrentVsyncInfo() used for? i.e what are the callers of this?

It's used for getting the Vsync time stamp so that observers can check if any missing due to latency.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Vincent Lin[:vilin] from comment #11)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> > Comment on attachment 8405288 [details] [diff] [review]
> > bug-987529-fix.patch
> > 
> > Review of attachment 8405288 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: hal/Hal.cpp
> > @@ +749,5 @@
> > >    AssertMainThread();
> > >    PROXY_IF_SANDBOXED(UnlockScreenOrientation());
> > >  }
> > >  
> > > +typedef mozilla::ObserverList<VsyncInfo> VsyncObserverList;
> > 
> > We should only have one Vsync observer so we should not need this list. This
> > patch looks like it still tries to support multiple vsync observers which we
> > do not want to do.
> 
> I can't understand why we should have only one observer ? May we have more
> discussion on this ?
> I thought your concern is just if input can be dispatched first or not,
> right ?
> In my current WIP, it can guarantee the order of notification and Compositor
> definitely can update the up-to-date APZC information.


Sorry about the delay in responding to this. Feel free to pester me by email or irc to about responding sooner.

> 
> In the design document(line85~93), is that the conclusion Jerry discussed
> with you on the final day of gfx work week ?
> https://etherpad.mozilla.org/vsync-butter
> Then I think it's better to make CompositorParent and RefreshDriver being
> observers. That provides more flexible design for these observers to
> run-timely switch on/off observation.

Jerry and I had some more discussion about this on irc and I'm not convinced that flexibility is that valuable. I'd rather start with a simpler model and we can always move to a more flexible model later.


> 
> > 
> > ::: hal/sandbox/PHal.ipdl
> > @@ +110,5 @@
> > >      NotifyBatteryChange(BatteryInformation aBatteryInfo);
> > >      NotifyNetworkChange(NetworkInformation aNetworkInfo);
> > >      NotifyWakeLockChange(WakeLockInformation aWakeLockInfo);
> > >      NotifyScreenConfigurationChange(ScreenConfiguration aScreenOrientation);
> > > +    NotifyVsyncChange(VsyncInfo aVsyncInfo);
> > 
> > We need to notify of vsync using the same protocol as we send input so that
> > we can ensure that the input always arrives first.
> 
> This bug is just for observer manager implementation.

I was confused about how the PHal protocol interacted with the rest of the system. I think it might be fine to use it now.

> 
> > 
> > @@ +174,5 @@
> > >   
> > > +    EnableVsyncNotifications();
> > > +    DisableVsyncNotifications();
> > > +    sync GetCurrentVsyncInfo()
> > > +      returns (VsyncInfo aVsyncInfo);
> > 
> > What is GetCurrentVsyncInfo() used for? i.e what are the callers of this?
> 
> It's used for getting the Vsync time stamp so that observers can check if
> any missing due to latency.

Which observers would need this information and could it not just be sent along with the vsync notification?

Comment 13

[Vincent Lin[:vilin]]

Attachment: bug-987529-fix.patch

make off-main-threading notification to Input and Compositor.

Comment 14

[StevenLee[:slee]]

Comment on attachment 8409586 [details] [diff] [review]
bug-987529-fix.patch

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

::: hal/Hal.cpp
@@ +800,5 @@
> +  VsyncObserverList& observer = GetVsyncObserverList(aType);
> +  observer.AddObserver(aObserver);
> +  if (observer.Length() == 1) {
> +    PROXY_IF_SANDBOXED(EnableVsyncNotifications());
> +  }

You don't need the above if. It is done in http://dxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#196

@@ +817,5 @@
> +  if (!observer.RemoveObserver(aObserver) || observer.Length() > 0) {
> +    return;
> +  }
> +
> +  PROXY_IF_SANDBOXED(DisableVsyncNotifications());

Remove the above code.

@@ +830,5 @@
> +}
> +
> +// Vsync thread
> +void
> +NotifyVsyncChange(const VsyncInfo& atimestamp)

Nit. s/atimestamp/aTimestamp

@@ +833,5 @@
> +void
> +NotifyVsyncChange(const VsyncInfo& atimestamp)
> +{
> +  if (!sVsyncObserverLists)
> +    return;

Always brace controlled statements.

@@ +848,5 @@
> +    case VSYNC_TYPE_REFRESH:
> +      if (observer.Length() > 0 && !NS_IsMainThread())
> +        NS_DispatchToMainThread(new VsyncRunnable(atimestamp, observer));
> +      else
> +        observer.Broadcast(atimestamp);

Always brace controlled statements.
You also need some comments in this block.

@@ +853,5 @@
> +      break;
> +    }
> +  }
> +
> +  sVsyncObservers.CacheInformation(atimestamp);

Here has race condition problem. For example, NotifyVsyncChnage is called and is going to call observer.Broadcaast then main thread is running UnregisterVsyncObserver and remove the observer.

::: hal/moz.build
@@ +21,3 @@
>      'HalWakeLock.h',
>  ]
>  

You should implement fallback version when you're not on gonk, like http://dxr.mozilla.org/mozilla-central/source/hal/moz.build#145

Comment 15

[Vincent Lin[:vilin]]

Comment on attachment 8409586 [details] [diff] [review]
bug-987529-fix.patch

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

::: hal/Hal.cpp
@@ +800,5 @@
> +  VsyncObserverList& observer = GetVsyncObserverList(aType);
> +  observer.AddObserver(aObserver);
> +  if (observer.Length() == 1) {
> +    PROXY_IF_SANDBOXED(EnableVsyncNotifications());
> +  }

I have some design changes compares to the previous one. observer is now an object of VsyncObserverList which doesn't inherit ObserversManager i.e. observer.AddObserver() just calls to the method of ObserverList to append object to the list.

It'll be more like Register/Unregister SwitchObserver.

The data structure of ObserversManager can't fulfill observers with different attributes, types or properties. So now I just create an array of observer list, each element in the array represents different type of observer list.

Comment 16

[u459114]

Got compile error while building firefox browser
27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:821: error: undefined reference to 'mozilla::hal_impl::DisableVsyncNotifications()'
27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:803: error: undefined reference to 'mozilla::hal_impl::EnableVsyncNotifications()'
27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:346: error: undefined reference to 'mozilla::hal_impl::EnableVsyncNotifications()'

Comment 17

[StevenLee[:slee]]

(In reply to C.J. Ku[:CJKu] from comment #16)
> Got compile error while building firefox browser
> 27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:821:
> error: undefined reference to
> 'mozilla::hal_impl::DisableVsyncNotifications()'
> 27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:803:
> error: undefined reference to 'mozilla::hal_impl::EnableVsyncNotifications()'
> 27:34.27 /home/cjcool/workspace/repository/mozilla-central/hal/Hal.cpp:346:
> error: undefined reference to 'mozilla::hal_impl::EnableVsyncNotifications()'
That's because the patch does not implement fallback on other platforms.

Comment 18

[u459114]

Vincent, thank for your great work here. Per discussion, let me handle the following work.

Comment 19

[StevenLee[:slee]]

Attachment: fix fallback error

CJ, you can merge this patch to fix the fallback error problem

Comment 20

[u459114]

Attachment: WIP Vsync thread

Compile-able, but not runnable. Just a prototype

Comment 21

[u459114]

Attachment: WIP Vsync thread V2

Comment 22

[u459114]

Attachment: WIP Vsync thread V3

Add more comment

Comment 23

[u459114]

Attachment: WIP Vsync thread V4

Comment 24

[u459114]

Attachment: Vsync Event Routing

Describe how vsync event route from HWComposer to all receiver.
Number in this diagram means the sequence of event receiving.

Comment 25

[u459114]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> (In reply to Vincent Lin[:vilin] from comment #11)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> > 
> > In the design document(line85~93), is that the conclusion Jerry discussed
> > with you on the final day of gfx work week ?
> > https://etherpad.mozilla.org/vsync-butter
> > Then I think it's better to make CompositorParent and RefreshDriver being
> > observers. That provides more flexible design for these observers to
> > run-timely switch on/off observation.
> 
> Jerry and I had some more discussion about this on irc and I'm not convinced
> that flexibility is that valuable. I'd rather start with a simpler model and
> we can always move to a more flexible model later.

Just want everyone knows, base on this design(explicit call Compositor/InputDispatcher/RefreshDriver's function), 
1. I need to add the following free function in Hal.h
  RegisterInputDispatcher()
  UnRegisterInputDispatcher()
  RegisterCompositor()
  UnRegisterCompositor()
  RegisterRefreshDriver()
  UnRegisterRefreshDriver()
  
  Since we don't want all this class inherit from Observer, we need to add a new register function for each type.
2. I need to include Compositor/InputDispatcher/RefreshDriver headers in Hal.cpp, since I need to explicit call Composite::composite function.
3. In Hal.cpp
   a. Have a list to store registered Compositors.
   b. Have a list to store registered RefresDrivers.
   c. Have a pointer to store register InputDispatcher(or a list?)
   d. Have a list to store HalParents.

If you have any concern, please let me know

Comment 26

[u459114]

Attachment: Vsync Event Routing

Comment 27

[u459114]

Attachment: Vsync Event Routing

Comment 28

[u459114]

Attachment: WIP Vsync thread V5

Comment 29

[u459114]

Attachment: WIP Vsync thread V6

Comment 30

[u459114]

Attachment: WIP Vsync thread V7

Comment 31

[u459114]

Attachment: WIP Vsync thread V8

Trigger composition/ tick/ process event after receive an vsync event.

Next step is to create a simple SW vsync pumper to unit test VsyncDispatcher and VsyncObseverManager

Comment 32

[u459114]

Attachment: WIP Vsync thread V9

1. Receive HW vsync event from HwcCompositor.
2. Enable Vsync driven refresh driver on both content and chrome process

Comment 33

[u459114]

Attachment: WIP Vsync thread V9

1. Receive HW vsync event from HwcCompositor.
2. Enable Vsync driven refresh driver on both content and chrome process

Comment 34

[u459114]

Attachment: WIP Vsync thread V10

Hi Jerry,
Please help to check whether this prototype consist with our design

Comment 35

[u459114]

Attachment: Vsync Event Routing V2

Comment 36

[u459114]

Attachment: WIP Vsync thread V11

Make nsRefershDriver be vsync driven - done

Next step
1. Make CompositorParent be vsync driven
2. Make nsAppShell event handling be vsync driven

Comment 37

[u459114]

Attachment: Part 1: (WIP V1) Dispatch vysnc event from HwcComposer2D to Hal

Comment 38

[u459114]

Attachment: Part 2: (WIP V12) Dispatch Vsync Event to Registered Objects

Comment 39

[u459114]

attachment 8418799 [details] [diff] [review] is copied from Bug 987527. You need to apply this patch to receive HW vsync event from vsync thread.

attachment 8418801 [details] [diff] [review] is what we want to constructed in this bug - vsync routing center

Comment 40

[u459114]

Attachment: Part 2: (WIP V13) Dispatch Vsync Event to Registered Objects

Clear useless change in nsRefreshDriver and correct logic error in Add/Remove a receiver to/from VsyncDispatcher

Comment 41

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

Comment on attachment 8418830 [details] [diff] [review]
Part 2: (WIP V13) Dispatch Vsync Event to Registered Objects

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

::: hal/Hal.cpp
@@ +755,5 @@
> +    // Chrome process receive a vsync notification from vsync event source
> +    else
> +    {
> +      dispatcher->EventDispatchAndCompose(aInfo);
> +      NS_DispatchToMainThread(new NotificationRunnable(*this, aInfo));

Could we move this logic to VsyncDispatcher?
I think that it makes hal more clear and let all vsync related code in the same place.

my pseudo code:
hal.cpp:
VsyncObserverManager::NotifyVsyncChange()
{
  ....
  dispatcher->HandleVsync();
  ....
}

VsyncDispatcher::HandleVsync()
{
  if(InContentProcess()){
    Tick();
  }
  else{ //chrome process
    EventDispatchAndCompose(aInfo);
    PostTaskToMainThread(Tick());
  }
}

::: hal/VsyncDispatcher.cpp
@@ +193,5 @@
> +
> +  // This function should be called in chrome process only.
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +
> +  if (mEnableDispatcherThread)

Should we really need a new thread to handle RegisterXXX task?
We use these RegisterXXX calls rarely.

@@ +277,5 @@
> +  // Base on benchmark data to determine whether prevent allocation here.
> +  // In the first version, do all the work in main thread.
> +  if (mEnableDispatcherThread)
> +  {
> +    mDispatchThread->Dispatch(

Hwc driver will call hal::NotifyVsyncChange at their own driver thread. So we can run EventDispatchAndCompose in this driver thread.
Should we use another thread(the DispatcherThread) to do this?

void HwcComposer2D::vsync(int disp, int64_t timestamp)
{
  ....
  hal::NotifyVsyncChange(VsyncInfo(timestamp));
}

Comment 42

[u459114]

(In reply to Jerry Shih[:jerry] from comment #41)
> Comment on attachment 8418830 [details] [diff] [review]
> Part 2: (WIP V13) Dispatch Vsync Event to Registered Objects
> 
> Review of attachment 8418830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/Hal.cpp
> @@ +755,5 @@
> > +    // Chrome process receive a vsync notification from vsync event source
> > +    else
> > +    {
> > +      dispatcher->EventDispatchAndCompose(aInfo);
> > +      NS_DispatchToMainThread(new NotificationRunnable(*this, aInfo));
> 
> Could we move this logic to VsyncDispatcher?
> I think that it makes hal more clear and let all vsync related code in the
> same place.
> 
> my pseudo code:
> hal.cpp:
> VsyncObserverManager::NotifyVsyncChange()
> {
>   ....
>   dispatcher->HandleVsync();
>   ....
> }
> 
> VsyncDispatcher::HandleVsync()
> {
>   if(InContentProcess()){
>     Tick();
>   }
>   else{ //chrome process
>     EventDispatchAndCompose(aInfo);
>     PostTaskToMainThread(Tick());
>   }
> }
> 
> ::: hal/VsyncDispatcher.cpp
> @@ +193,5 @@
> > +
> > +  // This function should be called in chrome process only.
> > +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> > +
> > +  if (mEnableDispatcherThread)
> 
> Should we really need a new thread to handle RegisterXXX task?
> We use these RegisterXXX calls rarely.
since you need to make thread safe, you have to move the following tasks into one thread
1. add/ remove list items
2. dereference any list item. 
Or, use mutex to guarantee the thing.

If you use the second way(mutes), main thread/ vsync thread might be block in object insertion/ deletion while vsyncdispathcer is accessing that list.
RegistXXX and UnRegistXX be called every 16.67 msend, we use it very often. The basic design is, an vsync observer should unregister vsync receiving after receiving a vsync notification, "and" register again after work done. Please have a discussion with Vincent if it makes you confuse. 

> @@ +277,5 @@
> > +  // Base on benchmark data to determine whether prevent allocation here.
> > +  // In the first version, do all the work in main thread.
> > +  if (mEnableDispatcherThread)
> > +  {
> > +    mDispatchThread->Dispatch(
> 
> Hwc driver will call hal::NotifyVsyncChange at their own driver thread. So
> we can run EventDispatchAndCompose in this driver thread.
> Should we use another thread(the DispatcherThread) to do this?
> 
> void HwcComposer2D::vsync(int disp, int64_t timestamp)
> {
>   ....
>   hal::NotifyVsyncChange(VsyncInfo(timestamp));
> }
That's a good catch.
you can do that, but you shouldn't. Don't block driver thread. Hand over tasks into another dedicated thread and return ASAP.

Comment 43

[u459114]

Jerry, 
Since you have touch the most important thing(threading modal) in this design, I would reveal more about it.

Currently, I dispatch all the tasks into main thread because of input dispatch flow, from nsAppShell to AsyncPanZoomController, need to in main thread. All the following dispatching tasks, dispatches to compositor and refresh drivers, need to be waited until input dispatch done. That's a big problem for this framework since we might be blocked by other tasks in main thread.

As a result, two blockers for this framework are
1. Move input dispatch flow out of main thread
2. Move refresh driver dispatch flow out of PHAL protocol, since this protocol is created in main thread as well.

Comment 44

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

(In reply to C.J. Ku[:cjku] from comment #43)
> As a result, two blockers for this framework are
> 1. Move input dispatch flow out of main thread
> 2. Move refresh driver dispatch flow out of PHAL protocol, since this
> protocol is created in main thread as well.

For item 2, I will take this job. My implementation will be similar to PImageBirdge, the top level protocal.

Comment 45

[StevenLee[:slee]]

Comment on attachment 8418830 [details] [diff] [review]
Part 2: (WIP V13) Dispatch Vsync Event to Registered Objects

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

Please follow the coding rules, ex, opening braces on same line.
For the current implementation, the preference only can enable/disable hw-vsync. If the implementation of "align-vsync" has problem, there is no way to turn it off. I think we may also need to consider how to turn off "align-vsync".

::: hal/Hal.cpp
@@ +755,5 @@
> +    // Chrome process receive a vsync notification from vsync event source
> +    else
> +    {
> +      dispatcher->EventDispatchAndCompose(aInfo);
> +      NS_DispatchToMainThread(new NotificationRunnable(*this, aInfo));

Agree, it makes the concept clearer. And we can also run it on the thread of VsyncDispatcher.

@@ +820,5 @@
> +  else
> +  {
> +    PROXY_IF_SANDBOXED(DisableVsyncNotifications());
> +  }
> +}

You don't need this function. In sVsyncObserverManager, when the length of observers is 1, it calls VsyncObserverManager::EnableNotifications.

::: hal/VsyncDispatcher.cpp
@@ +111,5 @@
> +// Try to find a holder to host VsyncDispatcher.
> +/* static */
> +VsyncDispatcher *VsyncDispatcher::GetInstance()
> +{
> +  if (sVsyncDispatcher.get() == nullptr)

if (!sVsyncDispatcher)

When using RefPtr, you don't need to call the get function to check whether it's null or not.

@@ +126,5 @@
> +void VsyncDispatcher::Shutdown()
> +{
> +  if (sVsyncDispatcher.get())
> +  {
> +    delete sVsyncDispatcher;

You don't need this code.

@@ +172,5 @@
> +    if (!ret)
> +    {
> +      printf_stderr("VDispatcher: dispatch task on wrong thread");
> +    }
> +

You can use NS_IsMainThread()

Comment 46

[u459114]

Attachment: Part 2: (WIP V14) Dispatch Vsync Event to Registered Objects

Enable vsync driven compositor

Comment 47

[u459114]

(In reply to Jerry Shih[:jerry] from comment #44)
> (In reply to C.J. Ku[:cjku] from comment #43)
> > As a result, two blockers for this framework are
> > 1. Move input dispatch flow out of main thread
> > 2. Move refresh driver dispatch flow out of PHAL protocol, since this
> > protocol is created in main thread as well.
> 
> For item 2, I will take this job. My implementation will be similar to
> PImageBirdge, the top level protocal.

Jerry, please check bug 994970

Comment 48

[u459114]

Attachment: Part 2: (WIP V15) Dispatch Vsync Event to Registered Objects

Enable vsync driven input dispatch on B2G

Comment 49

[u459114]

In attachment 8422725 [details] [diff] [review], all vsync recievers change to be vsync driven.
I didn't do any optimization in this patch, since I hopt the code is simple so that a reviewer can put focus on the logic of dispatch conduit design, instread of spending time on figure out how a function work.

Comment 50

[u459114]

Attachment: Part 2: (WIP V16) Dispatch Vsync Event to Registered Objects

1. Pull back touch event normalization from attachment 8418501 [details] [diff] [review]
2. Make sure things work correctly after vsync dispatch disable

Comment 51

[u459114]

Attachment: Vsync Event Routing V3

Update diagram according to discussion with Roc

Comment 52

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

Attachment: [WIP] Add a new top level protocol for vsync

Base on attachment 8418830 [details] [diff] [review], add a new "top level" protocol for vsync notification.

Comment 53

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

Attachment: [WIP] Add a new top level protocol for vsync. v2

move in-process ipc init() into VsyncEventParent

Comment 54

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

Attachment: Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol

Comment 55

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

Attachment: Part 3:[WIP] Add PVsyncEvent protocol with VsyncDispatcher

Comment 56

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

Attachment: Vsync Event Routing V4

Comment 57

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

Right now, vsync event can be passed by a new top level protocol.

Please apply these patches to try.
1. attachment 8418799 [details] [diff] [review]
2. attachment 8426185 [details] [diff] [review]
3. attachment 8426186 [details] [diff] [review]

The attachment 8427560 [details] is the vsync event routing and the thread model of this implementation.

Comment 58

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

Attachment: Vsync Event Routing Doc

Comment 59

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

Attachment: Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v2

VsyncDispatcher will wait the main thread apzc updated and then send the vsync event to other module.

Comment 60

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

Attachment: Part 3:[WIP] Add PVsyncEvent protocol with VsyncDispatcher. v2

rebase for part 2 patch.

Comment 61

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

Attachment: Vsync Event Routing Diagram V5

Use main thread to process PVsyncEventChild ipc event.

Comment 62

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

Attachment: Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v3

Fix creating useless thread in VsyncDispatcher.

Comment 63

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

Attachment: Part 3:[WIP] Add PVsyncEvent protocol with VsyncDispatcher. v3

Use main thread to process PVsyncEventChild ipc event.

Comment 64

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

Attachment: Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v4

Rewrite VsyncDispatcher input monitor mechanism.

Comment 65

[Mason Chang [Inactive] [:mchang]]

Hi Jeff, can you try testing the frame uniformity with the patches in this bug from comment 53?

Thanks!

Comment 66

[u459114]

Comment on attachment 8429233 [details] [diff] [review]
Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v4

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

::: gfx/layers/VsyncDispatcher.h
@@ +63,5 @@
> +  static void StartUpThread();
> +  static void StartUpOnExistedThread(MessageLoop* aMessageLoop);
> +
> +  static VsyncDispatcher* GetInstance();
> +

Only GetInstance need to be static.

@@ +75,5 @@
> +  // notify all registered vsync observer
> +  void NotifyVsync(const VsyncData& aVsyncData);
> +
> +  // dispatch vsync event
> +  void DispatchVsync(const VsyncData& aVsyncData);

this one should be private

Comment 67

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

Attachment: Part 1: [WIP] Dispatch vysnc event from HwcComposer2D to Hal. v2

Comment 68

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

Attachment: Part 2:[WIP] Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v5

Comment 69

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

Attachment: Part 3:[WIP] Add PVsyncEvent protocol with VsyncDispatcher. v4

Comment 70

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

Attachment: Part 4:[WIP, Test] Generate input event automatically for performance test. v1

Comment 71

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

attachment 8436885 [details] [diff] [review], 8436887, 8436888, 8436889

1.rebase to gecko hg changeset:   187525:4ff0524f1698
2.add auto move patch
  a)adb shell "setprob debug.touchevent.x"    //generate x direction move input event
  b)adb shell "setprob debug.touchevent.y"    //generate y direction move input event

Comment 72

[Jinho Hwang [:jeffhwang]]

(In reply to Mason Chang [:mchang] from comment #65)
> Hi Jeff, can you try testing the frame uniformity with the patches in this
> bug from comment 53?
> 
> Thanks!

Sorry Mason, for the late reply.
I'll do the test using High-speed camera.
It will take some time to get the result.
I only have Nexus4 now is it okay testing on Nexus4?

Comment 73

[Mason Chang [Inactive] [:mchang]]

Hi Jeff, sure Nexus 4 is ok. I think the initial target will be nexus 4 / flame. If it improves on Nexus 4 then we should be ok software wise. Thanks for testing Jeff!

Comment 74

[u459114]

Attachment: Thread execution time

A diagram to reveal relation among VsyncThead, MainThread and CompositorThread

Comment 75

[u459114]

Attachment: (I) Thread dependency

Comment 76

[u459114]

Attachment: (II) Main thread latency

Comment 77

[u459114]

Attachment: (III) Context switch

Comment 78

[u459114]

These three diagrams reveal thread dependency and weakness of current design.
(I) Dependency among vsync thread, main thread and compositor thread.
(II) Dispatch latency cause by main thread
(III) Context switch overhead before composition task executed.

Comment 79

[u459114]

(In reply to C.J. Ku[:cjku] from comment #78)
> These three diagrams reveal thread dependency and weakness of current design.
> (I) Dependency among vsync thread, main thread and compositor thread.
> (II) Dispatch latency cause by main thread
> (III) Context switch overhead before composition task executed.

bug 930939 is a solution of (II).
For (III), we may fix it here or file another one to reduce latency cause by multiple context switch

Comment 80

[Mason Chang [Inactive] [:mchang]]

Hey CJ and Jerry. Thanks for the updated diagrams. I was going through the docs and I think there should be a few updates from last week. 

1) bug 930939 - Kats and I were discussing how to get the input events off main thread. This would help with the latency.
2) I think we can tick the nsRefreshDriver via a signal instead of IPC for the foreground content process only. Background content processes can tick via IPC. What do you think?

Thanks!

Comment 81

[u459114]

(In reply to Mason Chang [:mchang] from comment #80)
> 2) I think we can tick the nsRefreshDriver via a signal instead of IPC for
> the foreground content process only. Background content processes can tick
> via IPC. What do you think?
Hi Manson,
In fact, we should not use vsync to trigger background process refresh tick, since the smoothness of background app is not important at all. That's, only foreground app need to be vsync driven. We will do this change in the near future
1. There should be only two refresh driver clients - chrome process and foreground content process
2. All the other background content processes should use local InactiveRefreshDriverTimer to reduce IPC and consume less CPU resource.
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#664

Regarding using signal instead of IPC, could you share a simple pseudo code(just several lines of code to demo how to use it) and brief explain why you think it's better? Since I am not quite familiar with the performance difference between IPC and singal.

Jerry is attending MAE, an trade show in China, may not give response by this week.

Comment 82

[u459114]

(In reply to Mason Chang [:mchang] from comment #80)
> Hey CJ and Jerry. Thanks for the updated diagrams. I was going through the
> docs and I think there should be a few updates from last week. 
> 
> 1) bug 930939 - Kats and I were discussing how to get the input events off
> main thread. This would help with the latency.
attachment 8437591 [details] explains why we need get input event off main thread. it's definitely help to reduce jank. Please keep us in the loop for this enhancement, thanks

Comment 83

[Mason Chang [Inactive] [:mchang]]

Hey CJ,

Sure, there is a lot of data in bug 991420, but essentially with IPC, we have up to a 30-40ms delay in receiving the IPC signal from HalParent -> HalChild. A signal should have very high priority in the kernel and remove that delay. Some PsuedoCode:

Parent Process:
VsyncDispatcher::DispatchVsync {
   // essentially get the tid of the vsync handler on the foreground process
   pid_t vsyncThreadContentProcess = GetForegroundVsyncThread();
   signal(VSYNC_SIGNAL, vsyncThreadContentProcess);
}

Foreground Child Process:
handleVsyncSignal() {
  DispatchToMainThread(FROM_HERE, new Runnable(nsRefreshDriver::Tick));
}

You can read more about signal handling by running 'man signal'. I still want to measure if this is actually faster first. You can track that work in bug 991420.

Comment 84

[Mason Chang [Inactive] [:mchang]]

(In reply to C.J. Ku[:cjku] from comment #81)
> (In reply to Mason Chang [:mchang] from comment #80)
> > 2) I think we can tick the nsRefreshDriver via a signal instead of IPC for
> > the foreground content process only. Background content processes can tick
> > via IPC. What do you think?
> Hi Manson,
> In fact, we should not use vsync to trigger background process refresh tick,
> since the smoothness of background app is not important at all. That's, only
> foreground app need to be vsync driven. We will do this change in the near
> future
> 1. There should be only two refresh driver clients - chrome process and
> foreground content process

Why do we need to tick the refresh driver on the chrome process? If an app is in the foreground, i think the system process only paints the status bar, which isn't super critical. Unless I'm missing something. Thanks!

Comment 85

[u459114]

(In reply to Mason Chang [:mchang] from comment #84)
> (In reply to C.J. Ku[:cjku] from comment #81)
> > (In reply to Mason Chang [:mchang] from comment #80)
> > > 2) I think we can tick the nsRefreshDriver via a signal instead of IPC for
> > > the foreground content process only. Background content processes can tick
> > > via IPC. What do you think?
> > Hi Manson,
> > In fact, we should not use vsync to trigger background process refresh tick,
> > since the smoothness of background app is not important at all. That's, only
> > foreground app need to be vsync driven. We will do this change in the near
> > future
> > 1. There should be only two refresh driver clients - chrome process and
> > foreground content process
> 
> Why do we need to tick the refresh driver on the chrome process? If an app
> is in the foreground, i think the system process only paints the status bar,
> which isn't super critical. Unless I'm missing something. Thanks!
I think it's ok to triggering refresh driver on the chrome process because there is no IPC overhead.

Even if we have only status bar need to be painted, you still want to see a smooth animation while drag down status bar from top.

Unless a refresh driver registry to vsync-dispathcer(VsyncDispathcer::RegisterRefreshDriver) because of content change, it won't receive vsync notification at all. As a result, system app won't keep receive vsync events while content still.

Comment 86

[u459114]

(In reply to Mason Chang [:mchang] from comment #83)
> You can read more about signal handling by running 'man signal'. I still
> want to measure if this is actually faster first. You can track that work in
> bug 991420.
Cool, Manson. I will definitely look into that bug, thank for your help.

Comment 87

[Jinho Hwang [:jeffhwang]]

Attachment: Unifomity.png

Hi Mason and Jerry,

I tested the frame uniformity with the patches here.
This attachment is the result. 
What I'm thinking is that the result doesn't show any difference between them(1.Without Vsync patch 2.With Vsync patch).

So, I review the code for touch event resampling and then found this typo.
Because of the following code the patch doesn't make any improvement on frame uniformity.

Comment on attachment 8436887 [details] [diff] [review] [diff] [review]
Part 2:[WIP] Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v5
-----------------------------------------------------------------
::: gfx/layers/VsyncDispatcher.h
@@ +735,5 @@
> else if (!mTouchMoveSamples.empty()) {
>   UserInputData sample = resample();
>   sendTouchOrMouseEvent(mTouchMoveSamples[0]);

Probably, Typo..
We need to dispatch sample instead of mTouchMoveSamples[0]

I tested frame uniformity again using frame uiformity visualization tool(bug 990832) after I fixed this typo.

Please see the next attachment for the result.
It shows the difference.
After fixing the typo, the result graph is more uniform then before as I expect.
Also, we can see the touch move events are resampled correctly.

Thanks.

Comment 88

[Jinho Hwang [:jeffhwang]]

Attachment: dispatch_TouchUniformity.png

Test using visualization tool.

::: widget/gonk/nsAppShell.cpp
@@ +735,5 @@
> else if (!mTouchMoveSamples.empty()) {
>   UserInputData sample = resample();
>   sendTouchOrMouseEvent(mTouchMoveSamples[0]);

change code : sendTouchOrMouseEvent(sample);

Comment 89

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

Attachment: Part 1: [WIP] Dispatch vysnc event from HwcComposer2D to Hal. v3

Comment 90

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

Attachment: Part 2:[WIP] Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol. v6

Comment 91

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

Attachment: Part 3:[WIP] Add PVsyncEvent protocol with VsyncDispatcher. v5

Comment 92

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

Attachment: Part 4:[WIP, Test] Generate input event automatically for performance test. v2

Comment 93

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

comment 89 to 92:

rebase to hg changeset 190587:a19e0434ea52.

update the auto move step:
a) use "your finger" to move x or y direction first.

at the same time:
b)adb shell "setprop debug.touchevent.x 1"    //auto generate x direction move input event
c)adb shell "setprop debug.touchevent.y 1"    //auto generate y direction move input event

set the property to 0 will disable the auto event.

Comment 94

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

Attachment: Part 1:[WIP] Enable project silk.

Comment 95

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

Attachment: Part 2:[WIP] Enable hwc vsync event

Comment 96

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

Attachment: Part 3:[WIP] Create vsync ipc protocol

Comment 97

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

Attachment: Part 4:[WIP] Modify refresh driver timer to fulfill silk project requirement

Comment 98

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

Attachment: Part 5:[WIP] Vsync event dispatcher

Comment 99

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

Attachment: Part 6:[WIP] compose with vsync event

Comment 100

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

Attachment: Part 7:[WIP] Tick refresh driver with vsync event

Comment 101

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

Attachment: Part 8:[WIP] Dispatch input with vsync event

Comment 102

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

Hi Mason,
You can apply part 1 to part 5 first.
Part 6, 7 and 8 can be applied individually. You also can combine the tree.

Comment 103

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

Attachment: Part 8:[WIP] Dispatch input with vsync event. v2

fix preference value

Comment 104

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

Attachment: Part 8:[WIP] Dispatch input with vsync event. v3

forgot to notify vsync dispatcher after processing input event.

Comment 105

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

Attachment: Part 5:[WIP] Vsync event dispatcher. v2

Use base::TimeTicks as the timestamp instead of using the hwc driver timestamp.
Because we can't get the same clock as the hwc driver does.

Comment 106

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

Attachment: Part 6:[WIP] compose with vsync event. v2

Trigger compose when vsync on.

I forgot to align the composing task with vsync when the task comes from APZC.

Comment 107

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

Attachment: Part 3:[WIP] Create vsync ipc protocol. v2

rebase

Comment 108

[Mason Chang [Inactive] [:mchang]]

Hey Jerry, I've been noticing more crashes with this and just did a debug. I sometimes see crashes in :mozilla::GonkVsyncDispatcher::Tick (this=0xb1086880, aVsyncData=...) at ../../../widget/gonk/GonkVsyncDispatcher.cpp:443

 timer->Tick(aVsyncData.timeStamp(), aVsyncData.frameNumber());


The timer was GCed to 0x5a5a5a5a. Do you know if we're correctly declaring the pointer to make sure it's not garbage collected? Thanks!

Comment 109

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

(In reply to Mason Chang [:mchang] from comment #108)
> Hey Jerry, I've been noticing more crashes with this and just did a debug. I
> sometimes see crashes in :mozilla::GonkVsyncDispatcher::Tick
> (this=0xb1086880, aVsyncData=...) at
> ../../../widget/gonk/GonkVsyncDispatcher.cpp:443
> 
>  timer->Tick(aVsyncData.timeStamp(), aVsyncData.frameNumber());
> 
> 
> The timer was GCed to 0x5a5a5a5a. Do you know if we're correctly declaring
> the pointer to make sure it's not garbage collected? Thanks!

Thanks, Mason.
I will check this crash.

Comment 110

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

I found the problem. The timer list is not thread-safe in this implementation.
I will update a new one to fix it.

Comment 111

[Peter Chang[:pchang]]

This bug is used to integrate vsync dispatcher to related modules, like compositor, APZ, and refresh drivers.

Comment 112

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

Attachment: [WIP] integrate refresh driver into vsync dispatcher. v1

Comment 113

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

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

Base on bug 1048667, integrate the refresh driver into vsync dispatcher.

Comment 114

[u459114]

Client does not really need to know client and host.

class RefreshDriverKicker {
public:
  void RegisterTimer(VsyncObserver *aRefreshDriverTimer) = 0; 
  void UnregisterTimer(VsyncObserver *aRefreshDriverTimer) = 0; 
  void UnregisterTimerSync(VsyncObserver *aRefreshDriverTimer) = 0; 
};

class CompositorKicker {
public:
// TBD	
};

class InputDispatcherKicker{
// TBD
};

class VsyncDispatcher 
{
public:
  virtual RefreshDriverKicker *AsRefreshDriverKicker() {
    return nullptr;
  }
  virtual CompositorKicker *AsCompositorKicker(){
    return nullptr;
  }
  virtual InputDispatcherKicker *AsInputDispatcherKicker() {
	return nullptr;
  }
};

class VysyncDispatcherClient : public VsyncDispatcher, RefreshDriverKicker {
public:
  virtual RefreshDriverKicker *AsRefreshDriverKicker() {
    return static_cast<RefreshDriverKicker *>(this);
  }

  void RegisterTimer(VsyncObserver *aRefreshDriverTimer); 
  void UnregisterTimer(VsyncObserver *aRefreshDriverTimer); 
  void UnregisterTimerSync(VsyncObserver *aRefreshDriverTimer); 
};

class VysyncDispatcherHost : public VsyncDispatcher, RefreshDriverKicker, CompositorKicker, InputDispatcherKicker {
public:
  virtual RefreshDriverKicker *AsRefreshDriverKicker() {
    return static_cast<RefreshDriverKicker *>(this);
  }
  virtual CompositorKicker *AsCompositorKicker(){
    return static_cast<CompositorKicker *>(this);
  }
  virtual InputDispatcherKicker *AsInputDispatcherKicker() {
	return static_cast<InputDispatcherKicker *>(this);
  }

  // RefreshDriverKicker functions.
  void RegisterTimer(VsyncObserver *aRefreshDriverTimer); 
  void UnregisterTimer(VsyncObserver *aRefreshDriverTimer); 
  void UnregisterTimerSync(VsyncObserver *aRefreshDriverTimer); 
};

In nsRefrestDriver

RefreshDriverKicker *kicker = VsyncDispathcer::GetInstance()->AsRefreshDriverKicker();
kicker->RegisterTimer(this);

Comment 115

[u459114]

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: layout/base/nsRefreshDriver.cpp
@@ +232,5 @@
> +      VsyncDispatcherHost::GetInstance()->RegisterRefreshDriverTimer(this);
> +    }
> +    else{
> +      VsyncDispatcherClient::GetInstance()->RegisterRefreshDriverTimer(this);
> +    }

Please refer to my previous comment and try to hide this into VsycnDispatcher

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +37,5 @@
>  
> +uint32_t
> +GonkVsyncDispatcher::GetVsyncPeriodNS() const
> +{
> +  return mVsyncPeriodNS;

Can't we just query this information from HwcComposer2D directly? Why bother to use a data member to keep this value>

@@ +54,5 @@
>    // Use hw event or software vsync event
>    if (gfxPrefs::FrameUniformityHWVsyncEnabled()) {
>      // init hw vsync event
>      if (HwcComposer2D::GetInstance()->RegisterHwcEventCallback()) {
> +      mVsyncPeriodNS = HwcComposer2D::GetInstance()->GetHWVsyncPeroidNS();

Can we change PeroidNS to GetVsycnRate? or GetVsyncFreqence?

::: widget/gonk/HwcComposer2D.cpp
@@ +224,5 @@
>  
> +uint32_t
> +HwcComposer2D::GetHWVsyncPeroidNS() const
> +{
> +    return mVsyncPeriodNS;

MOZ_ASSERT(mVsyncPeriodNS != 0);

::: widget/shared/VsyncDispatcherClient.cpp
@@ +78,5 @@
> +VsyncDispatcherClient::RegisterRefreshDriverTimer(VsyncObserver *aRefreshDriverTimer)
> +{
> +  ObserverListHelper::PostAddObserverTask(this, &mRefreshDriverTimerList, aRefreshDriverTimer);
> +}
> +

Try to prevent duplicate code in VsyncDispatcherClient and VsyncDispatcherHost.

::: widget/shared/VsyncDispatcherHost.cpp
@@ +178,5 @@
> +  int num = 0;
> +  num += mVsyncEventParentList.Length();
> +  num += mRefreshDriverTimerList.Length();
> +
> +  return num;

return (mVsyncEventParentList.Length() + mRefreshDriverTimerList.Length());

::: widget/shared/moz.build
@@ +8,5 @@
>      DIRS += ['x11']
>  
>  EXPORTS.mozilla += [
>      'VsyncDispatcherClient.h',
> +    'VsyncDispatcherDef.h',

Try not to public VsyncDispatcherHost and Client

Comment 116

[u459114]

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: widget/shared/VsyncDispatcherUtils.h
@@ +24,5 @@
>    template <typename DispatcherType, typename Type>
> +  static void PostAddObserverTask(DispatcherType* aDispatcher,
> +                                  nsTArray<Type*>* aList,
> +                                  Type* aItem)
> +  {

template <typename DispatcherT, typename ListenerT>
static void Add(DispathcerT*         aLoop,
		nsTArray<ListenerT>& aList,
		ListenerT&           aItem)

@@ +90,5 @@
> +      printf_stderr("Wait task timeout: %s", aDebugMessage);
> +    }
> +  }
> +
> +  template <typename DispatcherType, typename Type>

make this and following member functions be private
rename to AddInternal

Comment 117

[u459114]

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: widget/shared/VsyncDispatcherClient.cpp
@@ +99,5 @@
>  VsyncDispatcherClient::DispatchVsyncEvent(int64_t aTimestampUS, uint32_t aFrameNumber)
>  {
>    MOZ_ASSERT(IsInVsyncDispatcherClientThread(), "Call DispatchVsyncEvent at wrong thread.");
>  
>    if (!mVsyncEventNeeded) {

Can we just call GetVsyncObserverCount here and ignore this member?

if (GetVsyncObserverCount() == 0) {

}

@@ +136,5 @@
>  }
>  
>  int
>  VsyncDispatcherClient::GetVsyncObserverCount() const
>  {

MOZ_ASSERT(is_on_main_thread)

::: widget/shared/VsyncDispatcherClient.h
@@ +57,4 @@
>    void SetVsyncEventChild(layers::VsyncEventChild* aVsyncEventChild);
>  
>    // Dispatch vsync to observer
>    // This function should run at vsync dispatcher thread

Do we have a dedicate thread at client side?

@@ +78,2 @@
>  
>    bool mVsyncDispatcherClientInited;

mInit

@@ +78,4 @@
>  
>    bool mVsyncDispatcherClientInited;
>  
>    MessageLoop* mVsyncDispatchClientMessageLoop;

Why we need a message loop in client side?

Comment 118

[Peter Chang[:pchang]]

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: widget/gonk/HwcComposer2D.cpp
@@ +210,5 @@
> +    };
> +    int32_t hwcAttributeValues[QUERY_ATTRIBUTE_NUM];
> +
> +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, hwcAttributeValues);
> +    mVsyncPeriodNS=hwcAttributeValues[0];

Add error handling if mVsyncPeriodInNS is zero, for example, disable HW Vsync.

::: widget/gonk/HwcComposer2D.h
@@ +104,5 @@
>  
>      void Vsync(int aDisplay, int64_t aTimestamp);
> +
> +    // Return the hw vsync period(nanosecond).
> +    uint32_t GetHWVsyncPeroidNS() const;

s/GetHWVsyncPeriodNS/GetHWVsyncPeriod

@@ +140,5 @@
>      android::sp<android::Fence> mPrevRetireFence;
>      android::sp<android::Fence> mPrevDisplayFence;
>  
>      HWVsyncCallback         mVsyncCallback;
> +    uint32_t                mVsyncPeriodNS;

s/mVsyncPeriodNS/mVsyncPeriodInNS

::: widget/shared/VsyncDispatcherHost.cpp
@@ +226,5 @@
> +                                 aFrameNumber);
> +    NS_DispatchToMainThread(mainThreadTickTask);
> +  }
> +
> +  mRefreshDriverTimerList.Clear();

why do we clear the refreshdrivertimerlist here?

::: widget/shared/VsyncDispatcherHost.h
@@ +51,5 @@
>    //void RegisterInputDispatcher();
>    //void UnregisterInputDispatcher();
>    //// Register compositor for vsync composing.
>    //void RegisterCompositer(layers::CompositorParent *aCompositorParent);
>    //void UnregisterCompositer(layers::CompositorParent *aCompositorParent);

You can declare above function with empty implementation.

@@ +55,5 @@
>    //void UnregisterCompositer(layers::CompositorParent *aCompositorParent);
> +  // Register refresh driver timer to do tick at vsync.
> +  void RegisterRefreshDriverTimer(VsyncObserver *aRefreshDriverTimer);
> +  void UnregisterRefreshDriverTimer(VsyncObserver *aRefreshDriverTimer);
> +  void UnregisterRefreshDriverTimerSync(VsyncObserver *aRefreshDriverTimer);

How about UnregistedRefreshDriver(VsyncObserver *aVsyncObs, bool aSync)

@@ +75,5 @@
>  
>    bool IsInVsyncDispatcherHostThread();
>  
> +protected:
> +  uint32_t mVsyncPeriodNS;

rename to mVsyncPeriodInNS

::: widget/shared/VsyncDispatcherUtils.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef mozilla_VsyncDispatcherUtils_h
>  #define mozilla_VsyncDispatcherUtils_h

Rename to VsyncDispatcherHelper.h

@@ +45,5 @@
> +                                            aItem));
> +  }
> +
> +  template <typename DispatcherType, typename Type>
> +  static void PostAddObserverTaskSync(const char* aDebugMessage,

Please define VSYNC_DEBUG for the debug purpose, not just add debug msg directly

@@ +61,5 @@
> +                                            aList,
> +                                            aItem,
> +                                            &monitor,
> +                                            &done));
> +    lock.Wait(PR_MillisecondsToInterval(32));

What we can do if timout happened?

Comment 119

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

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +37,5 @@
>  
> +uint32_t
> +GonkVsyncDispatcher::GetVsyncPeriodNS() const
> +{
> +  return mVsyncPeriodNS;

Content side can't access the HwcComposer2D. It should get the value via ipc channel. I would like to cache the value when we init the VsyncDispatcherHost/Client for further using.

::: widget/shared/VsyncDispatcherClient.cpp
@@ +99,5 @@
>  VsyncDispatcherClient::DispatchVsyncEvent(int64_t aTimestampUS, uint32_t aFrameNumber)
>  {
>    MOZ_ASSERT(IsInVsyncDispatcherClientThread(), "Call DispatchVsyncEvent at wrong thread.");
>  
>    if (!mVsyncEventNeeded) {

Please check EnableVsyncNotificationIfhasObserver().
We use mVsyncEventNeeded instead of the GetVsyncObserverCount() value to prevent the redundant EnableVsyncEvent() call.

::: widget/shared/VsyncDispatcherClient.h
@@ +57,4 @@
>    void SetVsyncEventChild(layers::VsyncEventChild* aVsyncEventChild);
>  
>    // Dispatch vsync to observer
>    // This function should run at vsync dispatcher thread

No, the client side uses the main thread.
I will remove all thread related code in VsyncDispatcherClient.

::: widget/shared/moz.build
@@ +8,5 @@
>      DIRS += ['x11']
>  
>  EXPORTS.mozilla += [
>      'VsyncDispatcherClient.h',
> +    'VsyncDispatcherDef.h',

We should export the VsyncDispatcherHost and Client for PVsyncEvent.

Comment 120

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

(In reply to peter chang[:pchang][:peter] from comment #118)
> Comment on attachment 8473588 [details] [diff] [review]
> [WIP] integrate refresh driver into vsync dispatcher. v1
> 
> Review of attachment 8473588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +210,5 @@
> > +    };
> > +    int32_t hwcAttributeValues[QUERY_ATTRIBUTE_NUM];
> > +
> > +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, hwcAttributeValues);
> > +    mVsyncPeriodNS=hwcAttributeValues[0];
> 
> Add error handling if mVsyncPeriodInNS is zero, for example, disable HW
> Vsync.
> 
checked.
> ::: widget/gonk/HwcComposer2D.h
> @@ +104,5 @@
> >  
> >      void Vsync(int aDisplay, int64_t aTimestamp);
> > +
> > +    // Return the hw vsync period(nanosecond).
> > +    uint32_t GetHWVsyncPeroidNS() const;
> 
> s/GetHWVsyncPeriodNS/GetHWVsyncPeriod
> 

rename all related code into GetHWVsyncRate().
It will return the vsync rate per second(uint32_t).

> @@ +140,5 @@
> >      android::sp<android::Fence> mPrevRetireFence;
> >      android::sp<android::Fence> mPrevDisplayFence;
> >  
> >      HWVsyncCallback         mVsyncCallback;
> > +    uint32_t                mVsyncPeriodNS;
> 
> s/mVsyncPeriodNS/mVsyncPeriodInNS
> 
> ::: widget/shared/VsyncDispatcherHost.cpp
> @@ +226,5 @@
> > +                                 aFrameNumber);
> > +    NS_DispatchToMainThread(mainThreadTickTask);
> > +  }
> > +
> > +  mRefreshDriverTimerList.Clear();
> 
> why do we clear the refreshdrivertimerlist here?
>

We only tick once. 
We will not tick the refresh driver unless it registers again in ScheduleNextTick().

> ::: widget/shared/VsyncDispatcherHost.h
> @@ +51,5 @@
> >    //void RegisterInputDispatcher();
> >    //void UnregisterInputDispatcher();
> >    //// Register compositor for vsync composing.
> >    //void RegisterCompositer(layers::CompositorParent *aCompositorParent);
> >    //void UnregisterCompositer(layers::CompositorParent *aCompositorParent);
> 
> You can declare above function with empty implementation.
> 
checked.
> @@ +55,5 @@
> >    //void UnregisterCompositer(layers::CompositorParent *aCompositorParent);
> > +  // Register refresh driver timer to do tick at vsync.
> > +  void RegisterRefreshDriverTimer(VsyncObserver *aRefreshDriverTimer);
> > +  void UnregisterRefreshDriverTimer(VsyncObserver *aRefreshDriverTimer);
> > +  void UnregisterRefreshDriverTimerSync(VsyncObserver *aRefreshDriverTimer);
> 
> How about UnregistedRefreshDriver(VsyncObserver *aVsyncObs, bool aSync)
> 
checked.
> @@ +75,5 @@
> >  
> >    bool IsInVsyncDispatcherHostThread();
> >  
> > +protected:
> > +  uint32_t mVsyncPeriodNS;
> 
> rename to mVsyncPeriodInNS
> 
> ::: widget/shared/VsyncDispatcherUtils.h
> @@ +4,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #ifndef mozilla_VsyncDispatcherUtils_h
> >  #define mozilla_VsyncDispatcherUtils_h
> 
> Rename to VsyncDispatcherHelper.h
> 
checked.
> @@ +45,5 @@
> > +                                            aItem));
> > +  }
> > +
> > +  template <typename DispatcherType, typename Type>
> > +  static void PostAddObserverTaskSync(const char* aDebugMessage,
> 
> Please define VSYNC_DEBUG for the debug purpose, not just add debug msg
> directly
> 

The monitor need a name for constructor, and we need a message for time out case.
I rename the aDebugMessage into aTaskName.

> @@ +61,5 @@
> > +                                            aList,
> > +                                            aItem,
> > +                                            &monitor,
> > +                                            &done));
> > +    lock.Wait(PR_MillisecondsToInterval(32));
> 
> What we can do if timout happened?
We just print a error message, and assume the task is finished.
This function is used at shutdown case. The vsync dispatcher thread should not be blocked for a long time.

Comment 121

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

Attachment: [WIP] integrate refresh driver into vsync dispatcher. v2

Base on bug 1048667 attachment 8474562 [details] [diff] [review].
fix for comment 114 to comment 118

Comment 122

[u459114]

Comment on attachment 8473588 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v1

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +38,5 @@
> +uint32_t
> +GonkVsyncDispatcher::GetVsyncPeriodNS() const
> +{
> +  return mVsyncPeriodNS;
> +}

I mean 
return HwcComposer2D::GetInstance()->GetHWVsyncPeroidNS();

Comment 123

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

Attachment: [WIP] integrate refresh driver into vsync dispatcher. v3

Tick the refresh driver directly in VDClient.

Comment 124

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

Attachment: [WIP] integrate CompositorParent into vsync dispatcher. v1

Comment 125

[u459114]

Comment on attachment 8475196 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v3

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

::: widget/shared/VsyncDispatcherHost.cpp
@@ +309,5 @@
> +    ObserverListHelper::WaitForAsyncRemove("VDHost UnregisterTimerSync",
> +                                           this,
> +                                           &mRefreshDriverTimerList,
> +                                           aTimer);
> +  }

How about divide ObsererListHelper int two classes
1. AyncDispatcherListHelper
public:
template <typename DispatcherType, typename Type>
static void Add(DispatcherType* aDispatcher, nsTArray<Type*>* aList, Type* aItem)
{
  if (current thread != message loop of aDispatcher) {
    aDispatcher->GetMessageLoop()->PostTask(FROM_HERE,
      NewRunnableFunction(&AyncDispatcherListHelper::Add<DispatcherType, Type>,
      aDispatcher,
      aList,
      aItem));
  }

  if (!aList->Contains(aItem)) {
    aList->AppendElement(aItem);
    aDispatcher->EnableVsyncNotificationIfhasObserver();
  }
}
Remove
   
2. SyncDispatcherListHelper
public:
template <typename DispatcherType, typename Type>
static void Add(DispatcherType* aDispatcher, nsTArray<Type*>* aList, Type* aItem)
{
  if (current thread != message loop of aDispatcher) {
    aDispatcher->GetMessageLoop()->PostTask(FROM_HERE,
      NewRunnableFunction(&SyncDispatcherListHelper::AddInternal<DispatcherType, Type>,
      aDispatcher,
      aList,
      aItem));
  }

  lock.Wait(PR_MillisecondsToInterval(32));
}

Comment 126

[u459114]

Comment on attachment 8475821 [details] [diff] [review]
[WIP] integrate CompositorParent into vsync dispatcher. v1

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +248,5 @@
>  
> +void
> +CompositorParent::VsyncTick(int64_t aTimestampUS, uint32_t aFrameNumber)
> +{
> +  if (mVsyncComposeNeeded) {

Is mVsyncComposeNeeded thread safe?

Comment 127

[Peter Chang[:pchang]]

Comment on attachment 8475821 [details] [diff] [review]
[WIP] integrate CompositorParent into vsync dispatcher. v1

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +626,5 @@
> +
> +#ifdef COMPOSITOR_PERFORMANCE_WARNING
> +    uint32_t vsyncRate = VsyncDispatcher::GetInstance()->GetVsyncRate();
> +    mExpectedComposeStartTime = TimeStamp::Now() +
> +        TimeDuration::FromMilliseconds(1000.0 / vsyncRate);

Why update mExpectComposeStartTime here, not inside CompositorParent::VsyncTick?
Because here you only register compositor to vsync dispatcher.

@@ +658,2 @@
>    mCurrentCompositeTask = NewRunnableMethod(this, &CompositorParent::CompositeCallback);
>  

How about change to CompositeCallback task to 'registerCompositor with Vsync' task if FrameUniformity is enabled?

@@ +673,5 @@
>  
>  void
>  CompositorParent::CompositeCallback()
>  {
> +  mVsyncComposeNeeded = false;

Please add explanation why you set mVsyncComposeNeeded as false here.

::: gfx/layers/ipc/CompositorParent.h
@@ +88,5 @@
>  };
>  
>  class CompositorParent : public PCompositorParent,
> +                         public ShadowLayersManager,
> +                         public VsyncObserver

How about let others implement VsyncObserver, like input and refresh driver?

@@ +343,5 @@
>    nsRefPtr<APZCTreeManager> mApzcTreeManager;
>  
>    nsRefPtr<CompositorThreadHolder> mCompositorThreadHolder;
>  
> +  bool mVsyncComposeNeeded;

How about call it as mComposewithVsync?

::: widget/shared/VsyncDispatcherHost.cpp
@@ +197,5 @@
>  VsyncDispatcherHost::Compose(int64_t aTimestampUS, uint32_t aFrameNumber)
>  {
> +  for (CompositorList::size_type i = 0; i < mCompositorList.Length(); ++i) {
> +    VsyncObserver* compositor = mCompositorList[i];
> +

what happened if compositor becomes invalid?

@@ +271,5 @@
>  void
>  VsyncDispatcherHost::RegisterCompositor(VsyncObserver* aCompositor)
>  {
> +  MOZ_ASSERT(mInited);
> +

Check aCompositor before use.

@@ +279,5 @@
>  void
>  VsyncDispatcherHost::UnregisterCompositor(VsyncObserver* aCompositor, bool aSync)
>  {
> +  MOZ_ASSERT(mInited);
> +

Check aCompositor before use.

@@ +281,5 @@
>  {
> +  MOZ_ASSERT(mInited);
> +
> +  if (aSync) {
> +    ObserverListHelper::AsyncAdd(this, &mCompositorList, aCompositor);

why called AsyncAdd here

@@ +283,5 @@
> +
> +  if (aSync) {
> +    ObserverListHelper::AsyncAdd(this, &mCompositorList, aCompositor);
> +  } else {
> +    ObserverListHelper::WaitForAsyncRemove("VDHost UnregisterTimerSync",

The name called 'waitforAsyncRemove' doesn't imply it's a sync call.

::: widget/shared/VsyncDispatcherHost.h
@@ +131,5 @@
>  
>    //TODO: Put other vsync observer list here.
>    // Ex: input, compositor and refresh driver.
>  
>    // IPC parent

Please update more detail about the following variables.

@@ +137,3 @@
>    VsyncEventParentList mVsyncEventParentList;
>  
> +  // Compositor

Please update more detail about the following variables.

Comment 128

[Peter Chang[:pchang]]

Comment on attachment 8475196 [details] [diff] [review]
[WIP] integrate refresh driver into vsync dispatcher. v3

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

::: layout/base/nsRefreshDriver.cpp
@@ +206,5 @@
> +  }
> +
> +  ~VsyncTriggeredRefreshDriverTimer()
> +  {
> +    VsyncDispatcher::GetInstance()->AsRefreshDriverTrigger()->UnregisterTimer(this, true);

If all modules is a derived class from VsyncObserver, are we able to use the same register/unregister functions?

@@ +218,5 @@
> +
> +  virtual void StartTimer() MOZ_OVERRIDE
> +  {
> +    mLastFireEpoch = JS_Now();
> +    mLastFireTime = TimeStamp::Now();

Should be move to VysncTick. Am I right?

@@ +675,5 @@
> +    if (rate > 0){
> +      return 1000.0 / rate;
> +    }
> +    else {  // Use default 60 frame rate
> +      return 1000.0 / 60.0;

Add assertion here.

::: widget/shared/VsyncDispatcherHost.cpp
@@ +306,5 @@
> +  if (!aSync) {
> +    ObserverListHelper::AsyncRemove(this, &mRefreshDriverTimerList, aTimer);
> +  } else {
> +    ObserverListHelper::WaitForAsyncRemove("VDHost UnregisterTimerSync",
> +                                           this,

How about call this function as "WaitUntilRemoved("msg", this, timeout)"?
In my opinion, I prefer to call async and sync in one helper library.

Comment 129

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

(In reply to C.J. Ku[:cjku] from comment #126)
> Comment on attachment 8475821 [details] [diff] [review]
> [WIP] integrate CompositorParent into vsync dispatcher. v1
> 
> Review of attachment 8475821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +248,5 @@
> >  
> > +void
> > +CompositorParent::VsyncTick(int64_t aTimestampUS, uint32_t aFrameNumber)
> > +{
> > +  if (mVsyncComposeNeeded) {
> 
> Is mVsyncComposeNeeded thread safe?

Yes, we run the VsyncTick() func at compositor thread.

Comment 130

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

Comment on attachment 8475821 [details] [diff] [review]
[WIP] integrate CompositorParent into vsync dispatcher. v1

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +626,5 @@
> +
> +#ifdef COMPOSITOR_PERFORMANCE_WARNING
> +    uint32_t vsyncRate = VsyncDispatcher::GetInstance()->GetVsyncRate();
> +    mExpectedComposeStartTime = TimeStamp::Now() +
> +        TimeDuration::FromMilliseconds(1000.0 / vsyncRate);

VsyncTick() will do compose task, so the mExpectedComposeStartTime can't put there. After register, we will get the tick at next vsync event. So we estimate the next triggered time here.

::: widget/shared/VsyncDispatcherHost.cpp
@@ +197,5 @@
>  VsyncDispatcherHost::Compose(int64_t aTimestampUS, uint32_t aFrameNumber)
>  {
> +  for (CompositorList::size_type i = 0; i < mCompositorList.Length(); ++i) {
> +    VsyncObserver* compositor = mCompositorList[i];
> +

I will add a sync unregister call when we call CompositorParent::ActorDestroy

Comment 131

[u459114]

Comment on attachment 8475821 [details] [diff] [review]
[WIP] integrate CompositorParent into vsync dispatcher. v1

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +224,5 @@
>    , mResumeCompositionMonitor("ResumeCompositionMonitor")
>    , mOverrideComposeReadiness(false)
>    , mForceCompositionTask(nullptr)
>    , mCompositorThreadHolder(sCompositorThreadHolder)
> +  , mVsyncComposeNeeded(false)

Jerry,
I do suggest remove this new data member totally.

1. Calls VsyncDispathcer::RegisterCompositor in ScheduleComposition
2. Calls VsyncDispatcher::UnRegisterCompositor(async) in CancelCurrentCompositeTask
3. Calls VsyncDispatcher::UnRegisterCompositor(sync) in ActorDestoy.

Introduce a new stat flag in CompositorParent make flow control here more complex.

::: gfx/layers/ipc/CompositorParent.h
@@ +96,5 @@
>                     bool aUseExternalSurfaceSize = false,
>                     int aSurfaceWidth = -1, int aSurfaceHeight = -1);
>  
> +  virtual void VsyncTick(int64_t aTimestampUS, uint32_t aFrameNumber) MOZ_OVERRIDE;
> +

Why need prefix with vsync?

Comment 132

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: InputDispatcher integration. v1

Integrate GeckoTouchDispatcher into vsync dispatcher.

Comment 133

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v2

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

BTW, the input monitor is still under construction. We have to make sure
the waiting delay of each input event shouldn't longer than 4 ms.

Comment 134

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

Attachment: Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v3

rebase for VD framework.

Comment 135

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v4

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

BTW, the input monitor is still under construction. We have to make sure
the waiting delay of each input event shouldn't longer than 4 ms.

In this patch, add unregister while singleTouch events happen and fix the
assertion expression.

Comment 136

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v5

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

BTW, the input monitor is still under construction. We have to make sure
the waiting delay of each input event shouldn't longer than 4 ms.

Fix defalut value of InputDispatcherRegistryHost::Unregister().

Comment 137

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8489332 [details] [diff] [review]
[WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v5

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +137,5 @@
>    }
>  
>    if (haveTouchData) {
> +    // aTimestampUS is in microseconds, so we have to change its unit
> +    NS_DispatchToMainThread(new DispatchTouchEventsMainThread(this, aTimestampUS*1000));

Where do we define aTimestampUS? Is this a conversion from the HwcComposer on when the vysnc event occurred? Or is this when the VsyncDispatcher ticks?

::: widget/shared/VsyncDispatcherHostImpl.cpp
@@ +570,5 @@
> +      //lock.Wait();
> +      printf_stderr("[Boris] input done: %s",mInputDone?"Finish":"Not Finish");
> +      mInputDone = false;
> +    }
> +  }

I don't think we should do this. Right now, we already have a race condition when where both the touch and composite race. If the main thread on the parent process is busy, which it often is, we won't composite. I think this is the right approach once we have touch off the main thread, but until then, I don't think we should block compositing or ticking the refresh driver.

Comment 138

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Integrate refresh driver into vsync dispatcher, v4

Rebased.

Comment 139

[Boris Chiou [:boris]]

Attachment: [WIP] Part 3: Integrate CompositorParent into vsync dispatcher. v2

Rebased.

Comment 140

[Boris Chiou [:boris]]

Comment on attachment 8489332 [details] [diff] [review]
[WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v5

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +137,5 @@
>    }
>  
>    if (haveTouchData) {
> +    // aTimestampUS is in microseconds, so we have to change its unit
> +    NS_DispatchToMainThread(new DispatchTouchEventsMainThread(this, aTimestampUS*1000));

In Attachment 8484157 [details] [diff] (Part 3 of Bug 1048667), we call "base::TimeTicks::HighResNow().ToInternalValue()" to get the current time in HwcCompser2D::Vsync() when the vsync event occurred, instead of using the timestamp directly from the hardware vsync, so the unit becomes "microsecond" here.

::: widget/shared/VsyncDispatcherHostImpl.cpp
@@ +570,5 @@
> +      //lock.Wait();
> +      printf_stderr("[Boris] input done: %s",mInputDone?"Finish":"Not Finish");
> +      mInputDone = false;
> +    }
> +  }

OK. Maybe we can remove the wait() in the next patch after we have touch off the main thread. Jerry and I will comment out this wait() in our performance tests. Thanks.

Comment 141

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v6

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

Comment 142

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v7

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

Comment 143

[Boris Chiou [:boris]]

Attachment: [WIP] part 3: Integrate CompositorParent into vsync dispatcher. v3

Comment 144

[Boris Chiou [:boris]]

Attachment: [WIP] part 2: Integrate refresh driver into vsync dispatcher. v5

Fix coding style

Comment 145

[Boris Chiou [:boris]]

Comment on attachment 8493312 [details] [diff] [review]
[WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v7

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

Hi, Mason
Could you please take a look at this patch? I'm not sure whether the positions of Register() and Unregister() are correct or not. We want to register input dispatcher if we have move touch events and unregister it if we don't have any touch events and don't need to listen to vsync events anymore.

Comment 146

[Boris Chiou [:boris]]

Comment on attachment 8493312 [details] [diff] [review]
[WIP] Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v7

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +153,5 @@
>          MutexAutoLock lock(mTouchQueueLock);
>          mTouchMoveEvents.push_back(aData);
>          mTouchTimeDiff = aEventTime - mLastTouchTime;
>          mLastTouchTime = aEventTime;
> +        VsyncDispatcher::GetInstance()->GetInputDispatcherRegistry()->Register(this);

We register the InputDispatcher if we have some touch events.

@@ +368,5 @@
>        mTouchDownCount--;
>        if (mTouchDownCount == 0) {
>          MutexAutoLock lock(mTouchQueueLock);
>          mTouchMoveEvents.clear();
> +        VsyncDispatcher::GetInstance()->GetInputDispatcherRegistry()->Unregister(this);

Unregister if any single touch clears the mTouchMoveEvents.

::: widget/shared/VsyncDispatcherHostImpl.cpp
@@ +179,5 @@
>  
> +  bool isTicked = false;
> +  if (mObserver) {
> +    isTicked = mObserver->TickTask(aTimestampUS, aFrameNumber);
> +    mObserver = nullptr;

Unregister if we finish the Tick of InputDispatcher.

Comment 147

[Boris Chiou [:boris]]

Attachment: Part 1: Integrate GeckoTouchDispatcher into vsync dispatcher. v8

Add a new class, InputDispatcherRegistryHost, to handle the ticktask for
GeckoTouchDispatcher. In this patch, we also revise some data members
and member functions of other VsyncEventRegistries.

Comment 148

[Mason Chang [Inactive] [:mchang]]

Refresh driver and vsync dispatcher integration happened in bug 1092978.