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.

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

(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 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 :).

(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 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 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.

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

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

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?

(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.

(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?

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

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 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.

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()'

(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.

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

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

Compile-able, but not runnable. Just a prototype

Add more comment

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

(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

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

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

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

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

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

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

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

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)); }

(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.

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.

(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 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()

Enable vsync driven compositor

(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

Enable vsync driven input dispatch on B2G

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.

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

Update diagram according to discussion with Roc

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

move in-process ipc init() into VsyncEventParent

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.

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

rebase for part 2 patch.

Use main thread to process PVsyncEventChild ipc event.

Fix creating useless thread in VsyncDispatcher.

Use main thread to process PVsyncEventChild ipc event.

Rewrite VsyncDispatcher input monitor mechanism.

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

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

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

(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?

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!

A diagram to reveal relation among VsyncThead, MainThread and CompositorThread

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.

(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

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!

(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.

(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

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.

(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!

(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.

(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.

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.

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 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.

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.

fix preference value

forgot to notify vsync dispatcher after processing input event.

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.

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

rebase

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!

(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.

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

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

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.

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 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 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 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 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 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.

(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.

Base on bug 1048667 attachment 8474562 [details] [diff] [review]. fix for comment 114 to 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/GonkVsyncDispatcher.cpp @@ +38,5 @@ > +uint32_t > +GonkVsyncDispatcher::GetVsyncPeriodNS() const > +{ > + return mVsyncPeriodNS; > +} I mean return HwcComposer2D::GetInstance()->GetHWVsyncPeroidNS();

Tick the refresh driver directly in VDClient.

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 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 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 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.

(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 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 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?

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().