Closed
Bug 987529
Opened 11 years ago
Closed 10 years ago
Integrate vsync dispatcher on B2G
Categories
(Core Graveyard :: Widget: Gonk, defect, P1)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1092978
2.1 S4 (12sep)
People
(Reporter: vlin, Assigned: jerry)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [c=uniformity p= s= u=])
Attachments
(10 files, 73 obsolete files)
98 bytes,
text/plain
|
Details | |
77.79 KB,
image/png
|
Details | |
45.13 KB,
image/jpeg
|
Details | |
35.82 KB,
image/jpeg
|
Details | |
45.61 KB,
image/jpeg
|
Details | |
58.75 KB,
image/png
|
Details | |
414.27 KB,
image/png
|
Details | |
13.00 KB,
patch
|
Details | Diff | Splinter Review | |
9.94 KB,
patch
|
Details | Diff | Splinter Review | |
10.67 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Component: Performance → General
Reporter | ||
Updated•11 years ago
|
Component: General → Widget: Gonk
Product: Firefox OS → Core
Comment 1•11 years ago
|
||
Can we have good enough performance by using gecko IPC? In android, socket(wrapped by BitTube) is used to deliver vsync to an application.
Reporter | ||
Comment 2•11 years ago
|
||
(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.
Reporter | ||
Comment 3•11 years ago
|
||
Attachment #8398388 -
Flags: review?(cjones.bugs)
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 :).
Attachment #8398388 -
Flags: review?(cjones.bugs)
Reporter | ||
Comment 5•11 years ago
|
||
(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.
Reporter | ||
Comment 6•11 years ago
|
||
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.
Attachment #8398388 -
Flags: feedback?(jmuizelaar)
Comment 7•11 years ago
|
||
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.
Reporter | ||
Comment 8•11 years ago
|
||
Update to assure the notification order.
1. Input. 2. Compositor. 3. Refresh.
Attachment #8398388 -
Attachment is obsolete: true
Attachment #8398388 -
Flags: feedback?(jmuizelaar)
Reporter | ||
Comment 9•11 years ago
|
||
Update to assure the notification order.
1. Input. 2. Compositor. 3. Refresh.
Attachment #8405287 -
Attachment is obsolete: true
Attachment #8405288 -
Flags: feedback?(jmuizelaar)
Comment 10•11 years ago
|
||
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?
Attachment #8405288 -
Flags: feedback?(jmuizelaar) → feedback-
Reporter | ||
Comment 11•11 years ago
|
||
(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.
Flags: needinfo?(jmuizelaar)
Comment 12•11 years ago
|
||
(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?
Flags: needinfo?(jmuizelaar)
Reporter | ||
Comment 13•11 years ago
|
||
make off-main-threading notification to Input and Compositor.
Attachment #8405288 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8409586 -
Flags: feedback?(slee)
Attachment #8409586 -
Flags: feedback?(cku)
Comment 14•11 years ago
|
||
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
Attachment #8409586 -
Flags: feedback?(slee)
Reporter | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
Vincent, thank for your great work here. Per discussion, let me handle the following work.
Assignee: vlin → cku
Attachment #8409586 -
Flags: feedback?(cku)
Comment 19•11 years ago
|
||
CJ, you can merge this patch to fix the fallback error problem
Comment 20•11 years ago
|
||
Compile-able, but not runnable. Just a prototype
Comment 21•11 years ago
|
||
Attachment #8412451 -
Attachment is obsolete: true
Comment 23•11 years ago
|
||
Attachment #8412516 -
Attachment is obsolete: true
Attachment #8412366 -
Attachment is obsolete: true
Comment 24•11 years ago
|
||
Describe how vsync event route from HWComposer to all receiver.
Number in this diagram means the sequence of event receiving.
Comment 25•11 years ago
|
||
(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
Flags: needinfo?(vlin)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(hshih)
Comment 26•11 years ago
|
||
Attachment #8412852 -
Attachment is obsolete: true
Flags: needinfo?(vlin)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(hshih)
Comment 27•11 years ago
|
||
Attachment #8413498 -
Attachment is obsolete: true
Comment 28•11 years ago
|
||
Attachment #8412543 -
Attachment is obsolete: true
Comment 29•11 years ago
|
||
Attachment #8413555 -
Attachment is obsolete: true
Comment 30•11 years ago
|
||
Attachment #8413656 -
Attachment is obsolete: true
Comment 31•11 years ago
|
||
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
Attachment #8413831 -
Attachment is obsolete: true
Comment 32•11 years ago
|
||
1. Receive HW vsync event from HwcCompositor.
2. Enable Vsync driven refresh driver on both content and chrome process
Attachment #8413852 -
Attachment is obsolete: true
Comment 33•11 years ago
|
||
1. Receive HW vsync event from HwcCompositor.
2. Enable Vsync driven refresh driver on both content and chrome process
Attachment #8418146 -
Attachment is obsolete: true
Comment 34•11 years ago
|
||
Hi Jerry,
Please help to check whether this prototype consist with our design
Attachment #8418148 -
Attachment is obsolete: true
Flags: needinfo?(hshih)
Comment 35•11 years ago
|
||
Attachment #8413532 -
Attachment is obsolete: true
Comment 36•11 years ago
|
||
Make nsRefershDriver be vsync driven - done
Next step
1. Make CompositorParent be vsync driven
2. Make nsAppShell event handling be vsync driven
Attachment #8418172 -
Attachment is obsolete: true
Comment 37•11 years ago
|
||
Comment 38•11 years ago
|
||
Attachment #8418654 -
Attachment is obsolete: true
Comment 39•11 years ago
|
||
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•11 years ago
|
||
Clear useless change in nsRefreshDriver and correct logic error in Add/Remove a receiver to/from VsyncDispatcher
Attachment #8418801 -
Attachment is obsolete: true
Attachment #8418830 -
Flags: feedback?(slee)
Attachment #8418830 -
Flags: feedback?(hshih)
Summary: Implement an observer manager for vsync in gecko/hal → Implement Vsync dispatch pipeline
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(hshih)
Assignee | ||
Comment 41•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 44•11 years ago
|
||
(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•11 years ago
|
||
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()
Attachment #8418830 -
Flags: feedback?(slee)
Comment 46•11 years ago
|
||
Enable vsync driven compositor
Attachment #8418830 -
Attachment is obsolete: true
Attachment #8418830 -
Flags: feedback?(hshih)
Comment 47•11 years ago
|
||
(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•11 years ago
|
||
Enable vsync driven input dispatch on B2G
Attachment #8422584 -
Attachment is obsolete: true
Comment 49•11 years ago
|
||
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•11 years ago
|
||
1. Pull back touch event normalization from attachment 8418501 [details] [diff] [review]
2. Make sure things work correctly after vsync dispatch disable
Attachment #8422725 -
Attachment is obsolete: true
Comment 51•11 years ago
|
||
Update diagram according to discussion with Roc
Attachment #8418187 -
Attachment is obsolete: true
Assignee | ||
Comment 52•11 years ago
|
||
Base on attachment 8418830 [details] [diff] [review], add a new "top level" protocol for vsync notification.
Assignee | ||
Comment 53•11 years ago
|
||
move in-process ipc init() into VsyncEventParent
Attachment #8423119 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: cku → hshih
Assignee | ||
Comment 54•11 years ago
|
||
Attachment #8422792 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8426185 -
Attachment description: Part 2: Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol → Part 2:[WIP]Dispatch Vsync Event to Registered Objects using PVsyncEvent protocol
Assignee | ||
Comment 55•11 years ago
|
||
Attachment #8423823 -
Attachment is obsolete: true
Assignee | ||
Comment 56•11 years ago
|
||
Attachment #8422803 -
Attachment is obsolete: true
Assignee | ||
Comment 57•11 years ago
|
||
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.
Assignee | ||
Comment 58•11 years ago
|
||
Assignee | ||
Comment 59•11 years ago
|
||
VsyncDispatcher will wait the main thread apzc updated and then send the vsync event to other module.
Attachment #8426185 -
Attachment is obsolete: true
Assignee | ||
Comment 60•11 years ago
|
||
rebase for part 2 patch.
Attachment #8426186 -
Attachment is obsolete: true
Assignee | ||
Comment 61•11 years ago
|
||
Use main thread to process PVsyncEventChild ipc event.
Attachment #8427560 -
Attachment is obsolete: true
Assignee | ||
Comment 62•11 years ago
|
||
Fix creating useless thread in VsyncDispatcher.
Attachment #8428620 -
Attachment is obsolete: true
Assignee | ||
Comment 63•11 years ago
|
||
Use main thread to process PVsyncEventChild ipc event.
Attachment #8428622 -
Attachment is obsolete: true
Assignee | ||
Comment 64•11 years ago
|
||
Rewrite VsyncDispatcher input monitor mechanism.
Attachment #8429110 -
Attachment is obsolete: true
Comment 65•11 years ago
|
||
Hi Jeff, can you try testing the frame uniformity with the patches in this bug from comment 53?
Thanks!
Flags: needinfo?(faraojh)
Comment 66•11 years ago
|
||
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
Assignee | ||
Comment 67•10 years ago
|
||
Attachment #8418799 -
Attachment is obsolete: true
Assignee | ||
Comment 68•10 years ago
|
||
Attachment #8429233 -
Attachment is obsolete: true
Assignee | ||
Comment 69•10 years ago
|
||
Attachment #8429112 -
Attachment is obsolete: true
Assignee | ||
Comment 70•10 years ago
|
||
Assignee | ||
Comment 71•10 years ago
|
||
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•10 years ago
|
||
(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?
Flags: needinfo?(faraojh) → needinfo?(mchang)
Comment 73•10 years ago
|
||
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!
Flags: needinfo?(mchang)
Comment 74•10 years ago
|
||
A diagram to reveal relation among VsyncThead, MainThread and CompositorThread
Comment 75•10 years ago
|
||
Attachment #8437567 -
Attachment is obsolete: true
Attachment #8437571 -
Attachment description: Thread execution time (V2) → (I) Thread dependency
Comment 76•10 years ago
|
||
Comment 77•10 years ago
|
||
Comment 78•10 years ago
|
||
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•10 years ago
|
||
(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
Attachment #8409586 -
Attachment is obsolete: true
Attachment #8429109 -
Attachment description: Vsync Event Routing V5 → Vsync Event Routing Diagram V5
Attachment #8427639 -
Attachment description: Vsync Event Routing(google doc) → Vsync Event Routing Doc
Comment 80•10 years ago
|
||
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!
Flags: needinfo?(cku)
Comment 81•10 years ago
|
||
(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.
Flags: needinfo?(cku) → needinfo?(mchang)
Comment 82•10 years ago
|
||
(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•10 years ago
|
||
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.
Flags: needinfo?(mchang)
Comment 84•10 years ago
|
||
(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!
Flags: needinfo?(cku)
Comment 85•10 years ago
|
||
(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.
Flags: needinfo?(cku)
Comment 86•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
Test using visualization tool.
::: widget/gonk/nsAppShell.cpp
@@ +735,5 @@
> else if (!mTouchMoveSamples.empty()) {
> UserInputData sample = resample();
> sendTouchOrMouseEvent(mTouchMoveSamples[0]);
change code : sendTouchOrMouseEvent(sample);
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 89•10 years ago
|
||
Attachment #8436885 -
Attachment is obsolete: true
Assignee | ||
Comment 90•10 years ago
|
||
Attachment #8436887 -
Attachment is obsolete: true
Assignee | ||
Comment 91•10 years ago
|
||
Attachment #8436888 -
Attachment is obsolete: true
Assignee | ||
Comment 92•10 years ago
|
||
Attachment #8436889 -
Attachment is obsolete: true
Assignee | ||
Comment 93•10 years ago
|
||
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.
Updated•10 years ago
|
Blocks: input-thread
Assignee | ||
Comment 94•10 years ago
|
||
Attachment #8445682 -
Attachment is obsolete: true
Attachment #8445684 -
Attachment is obsolete: true
Attachment #8445685 -
Attachment is obsolete: true
Attachment #8445687 -
Attachment is obsolete: true
Assignee | ||
Comment 95•10 years ago
|
||
Assignee | ||
Comment 96•10 years ago
|
||
Assignee | ||
Comment 97•10 years ago
|
||
Assignee | ||
Comment 98•10 years ago
|
||
Assignee | ||
Comment 99•10 years ago
|
||
Assignee | ||
Comment 100•10 years ago
|
||
Assignee | ||
Comment 101•10 years ago
|
||
Assignee | ||
Comment 102•10 years ago
|
||
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.
Assignee | ||
Comment 103•10 years ago
|
||
fix preference value
Attachment #8454509 -
Attachment is obsolete: true
Assignee | ||
Comment 104•10 years ago
|
||
forgot to notify vsync dispatcher after processing input event.
Assignee | ||
Updated•10 years ago
|
Attachment #8454521 -
Attachment is obsolete: true
Assignee | ||
Comment 105•10 years ago
|
||
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.
Attachment #8454500 -
Attachment is obsolete: true
Assignee | ||
Comment 106•10 years ago
|
||
Trigger compose when vsync on.
I forgot to align the composing task with vsync when the task comes from APZC.
Attachment #8454504 -
Attachment is obsolete: true
Assignee | ||
Comment 107•10 years ago
|
||
rebase
Assignee | ||
Updated•10 years ago
|
Attachment #8454496 -
Attachment is obsolete: true
Comment 108•10 years ago
|
||
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!
Assignee | ||
Comment 109•10 years ago
|
||
(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.
Assignee | ||
Comment 110•10 years ago
|
||
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•10 years ago
|
||
This bug is used to integrate vsync dispatcher to related modules, like compositor, APZ, and refresh drivers.
Summary: Implement Vsync dispatch pipeline → Integrate vsync dispatcher on B2G
Updated•10 years ago
|
Target Milestone: --- → 2.1 S4 (12sep)
Assignee | ||
Comment 112•10 years ago
|
||
Attachment #8454492 -
Attachment is obsolete: true
Attachment #8454493 -
Attachment is obsolete: true
Attachment #8454499 -
Attachment is obsolete: true
Attachment #8454507 -
Attachment is obsolete: true
Attachment #8454534 -
Attachment is obsolete: true
Attachment #8458385 -
Attachment is obsolete: true
Attachment #8458410 -
Attachment is obsolete: true
Attachment #8459291 -
Attachment is obsolete: true
Assignee | ||
Comment 113•10 years ago
|
||
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.
Attachment #8473588 -
Flags: feedback?(pchang)
Attachment #8473588 -
Flags: feedback?(cku)
Comment 114•10 years ago
|
||
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);
Attachment #8473588 -
Flags: feedback?(cku)
Comment 115•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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?
Assignee | ||
Comment 119•10 years ago
|
||
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.
Assignee | ||
Comment 120•10 years ago
|
||
(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.
Assignee | ||
Comment 121•10 years ago
|
||
Base on bug 1048667 attachment 8474562 [details] [diff] [review].
fix for comment 114 to comment 118
Attachment #8473588 -
Attachment is obsolete: true
Attachment #8473588 -
Flags: feedback?(pchang)
Comment 122•10 years ago
|
||
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();
Assignee | ||
Comment 123•10 years ago
|
||
Tick the refresh driver directly in VDClient.
Attachment #8474572 -
Attachment is obsolete: true
Assignee | ||
Comment 124•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8475196 -
Flags: feedback?(pchang)
Attachment #8475196 -
Flags: feedback?(cku)
Assignee | ||
Updated•10 years ago
|
Attachment #8475821 -
Flags: feedback?(pchang)
Attachment #8475821 -
Flags: feedback?(cku)
Comment 125•10 years ago
|
||
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));
}
Attachment #8475196 -
Flags: feedback?(cku) → feedback+
Comment 126•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8475821 -
Flags: feedback?(pchang) → feedback-
Comment 128•10 years ago
|
||
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.
Attachment #8475196 -
Flags: feedback?(pchang)
Assignee | ||
Comment 129•10 years ago
|
||
(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.
Assignee | ||
Comment 130•10 years ago
|
||
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•10 years ago
|
||
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?
Attachment #8475821 -
Flags: feedback?(cku) → feedback-
Updated•10 years ago
|
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 136•10 years ago
|
||
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().
Attachment #8489321 -
Attachment is obsolete: true
Comment 137•10 years ago
|
||
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 hidden (obsolete) |
Comment hidden (obsolete) |
Comment 140•10 years ago
|
||
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 hidden (obsolete) |
Comment 142•10 years ago
|
||
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.
Attachment #8493308 -
Attachment is obsolete: true
Comment 143•10 years ago
|
||
Attachment #8490534 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Updated•10 years ago
|
Attachment #8493312 -
Flags: feedback?(mchang)
Comment 147•10 years ago
|
||
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.
Attachment #8493312 -
Attachment is obsolete: true
Comment 148•10 years ago
|
||
Refresh driver and vsync dispatcher integration happened in bug 1092978.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•