980241 - Vsync-triggered RefreshDriver

Status: RESOLVED INVALID

(Core :: Layout, defect, P2)

Comment 1

[Vincent Lin[:vilin]]

Attachment: ProjectButterWIP0306.patch

Comment 2

[u459114]

Here is our simple idea:
1. Instead of using a system timer, we trigger reflesh by vsync event. (This is what  ProjectButterWIP0306.patch did)
2. Then, handle touch event after vsync recieve, and trigger paint/ layout transaction accordingly. 
                     VSYNC        VSYNC
                     |              |
Timeline        --------------------------------
Content Process      |H|P|T         |H|P|T

T= Handle touch event. Include touch event normalization. (Bug 970751)
P= PreShell::Paint
T= Layer Transaction 

is there anyone has already working on this?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

By Layer Transaction, do you mean the work in CompositeParent::CompositeToTarget()?

I can see the benefit of running the compositor off of the vsync as its work is directly related, but can you describe why we would want touch events and PresShell::Paint in the same category?

In particular, it seems that PresShell::Paint() (of potentially things not on screen yet) could take longer than our frame budget and we wouldn't want to delay composition of existing content.

Sorry if I'm just confused about the idea here.

Also, if we do want to drive composition like this then bug 980027 and bug 980321 might help?

Comment 4

[Vincent Lin[:vilin]]

(In reply to Ben Kelly [:bkelly] from comment #3)
> By Layer Transaction, do you mean the work in
> CompositeParent::CompositeToTarget()?

Layer Transaction means "SendUpdate()" or "SendUpdateNoSwap()" IPC to compositor.
CompositeToTarget() will happen to the next Vsync.

                        VSYNC        VSYNC        VSYNC
                        |              |              |
Timeline           -----------------------------------------
Content Process         |H|P|T         |H|P|T
B2G Process      [TEs]                 |C|            |C|

H= Handle touch event. Include touch event normalization.
P= PreShell::Paint
T= Layer Transaction 
C= Composition
[TEs] = Touch Events

Comment 5

[Vincent Lin[:vilin]]

Attachment: HalVsyncInfo.h

The newly created file has to be appended to the previous WIP. (gecko/hal)

Comment 6

[u459114]

Comment on attachment 8386642 [details] [diff] [review]
ProjectButterWIP0306.patch

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

Feedback of HwcCompiser2D.cpp/h

::: widget/gonk/HwcComposer2D.cpp
@@ +121,5 @@
>          mRBSwapSupport = supported ? true : false;
>      }
> +
> +    LOGI("%s %p", __func__, mHwc->registerProcs);
> +    if (mHwc->registerProcs) {

You can bypass mHwc to mCBContext, and handle all the things in that class

Change class definition of HwcComposer2D::cb_context to
class VSyncHandler {
public:
  VSyncHandler() { /*Initiate data member here*/}
  void Init(HwcDevice *hwc);

  static hook_invalidate() ...
  static hook_vsyn() ...
  static hook_hotplug()...

private:
 struct callbacks : public hwc_procs_t {
 ...
 };
 callbacks procs;
};

And, unless VSyncHandler real need to use HwcComposer2d, VSyncHandler should not aware HwcComposer2D at all.

@@ +187,5 @@
> +
> +void
> +HwcComposer2D::vsync(int disp, int64_t timestamp) {
> +    ATRACE_INT("VSYNC", ++mVSyncCount&1);
> +    nsAppShell::NotifyVsyncReceived(timestamp);

Don't bypass vsync signal into nsAppShell. VSyncHandler should dispatch this signal from hwc to hal in this compile unit

::: widget/gonk/HwcComposer2D.h
@@ +63,4 @@
>  
> +    inline void invalidate();
> +    inline void vsync(int disp, int64_t timestamp);
> +    inline void hotplug(int disp, int connected);

Remove these function declaration.
Move all these function into VSyncHandler in .cpp

Comment 7

[u459114]

Comment on attachment 8386642 [details] [diff] [review]
ProjectButterWIP0306.patch

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

A suggestion. You don't need to do it in WIP patch

::: layout/base/nsRefreshDriver.cpp
@@ +711,5 @@
>        sRegularRateTimer = new PreciseRefreshDriverTimerWindowsDwmVsync(rate, isDefault);
>      }
>  #endif
>      if (!sRegularRateTimer) {
> +      sRegularRateTimer = new HWVsyncRefreshDriverTimer(rate);	 

Have a preference here. By default, use sw timer

Comment 8

[Vincent Lin[:vilin]]

Attachment: ProjectButterWIP0312.patch

Jeremy,

Could you please try this patch ? We don't have equipment to verify the uniformity. I will attach systrace profile later.

The major change is to have a VsyncObserver in gecko/HAL so RefreshDriver and CompositorParent can register to it. The prerequisite is your HWComposer should support vsync callback(Android JB). Or I can design a SW Vsync to support non-JB device.

I also include your patch in Bug 970751. You can see that I ever try to make a VsyncTimer instead for triggering dispatch, but thing gets worse. (I will keep looking into it.)

Comment 9

[Vincent Lin[:vilin]]

Attachment: mynewtrace.html

Comment 10

[Vincent Lin[:vilin]]

Attachment: ProjectButterWIP0313.patch

WIP~
Touch dispatcher aligns with Vsync as well.

Comment 11

[Jinho Hwang [:jeffhwang]]

Attachment: nsAppShell.patch

Comment 12

[Jinho Hwang [:jeffhwang]]

Thanks Vincent for the patch.

It looks good to me using HwcComposer callback for Vsync. 
I guess it's good idea to have SW Vsync for the such devices which don't support HWcomposer, vsync callback or set the "layers.composer2d.enabled" flag to false.

And I'd like you to consider this patch for nsAppShell.cpp. I only change a way to use vsync observer in GonkInputDispatcher.

Additionally, there is long latency (more than 100ms) when I test drag on a contact list.
I'm trying to figure out why it happens.
Applied your patch to Master(HEAD : 7312341e00e2785419fc4746e291646299ba3018) with the nsAppShell patch I attached here.

The high-speed camera test is in progress. After we get the result, Jeremy will share you it.

Comment 13

[Vincent Lin[:vilin]]

(In reply to Jinho Hwang [:jeffhwang] from comment #12)
> Thanks Vincent for the patch.
> 
> It looks good to me using HwcComposer callback for Vsync. 
> I guess it's good idea to have SW Vsync for the such devices which don't
> support HWcomposer, vsync callback or set the "layers.composer2d.enabled"
> flag to false.

Sure. I will make it after we proof the prototype(WIP) is workable.

> 
> And I'd like you to consider this patch for nsAppShell.cpp. I only change a
> way to use vsync observer in GonkInputDispatcher.
> 
> Additionally, there is long latency (more than 100ms) when I test drag on a
> contact list.
> I'm trying to figure out why it happens.
> Applied your patch to Master(HEAD :
> 7312341e00e2785419fc4746e291646299ba3018) with the nsAppShell patch I
> attached here.
> 
> The high-speed camera test is in progress. After we get the result, Jeremy
> will share you it.

Thanks.
If possible, can you have a systrace profile for the test ? I just concerned if the test hits rendering performance issue which is not considered in my current WIP.

Comment 14

[Jinho Hwang [:jeffhwang]]

Attachment: trace_vsync_1.html

Here is one trace file.
I hope it helps your work. 
What I see on the tracefile is that after touch down event is sent, we dispatch more than 5 touch move events but no compositing for those. Possibly, some of those move events are ignored because it didn't exceed the threshold for determine move action.
But, the latency is over 100ms. It's too long.

Comment 15

[Vincent Lin[:vilin]]

(In reply to Jinho Hwang [:jeffhwang] from comment #14)
> Created attachment 8391040 [details]
> trace_vsync_1.html
> 
> Here is one trace file.
> I hope it helps your work. 
> What I see on the tracefile is that after touch down event is sent, we
> dispatch more than 5 touch move events but no compositing for those.
> Possibly, some of those move events are ignored because it didn't exceed the
> threshold for determine move action.
> But, the latency is over 100ms. It's too long.

Is there any Content process running at the same time ?
I think Content process got to receive touch event, reflow and then send update to compositor.
The skipped touch move events may be due to the main thread of Content process is busy on flow and painting the layer. This problem dose not surprise me. What I would like to focus on is the uniformity during dragging i.e. the case without rendering or composition bound.

Comment 16

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

This bug should probably be moved to the Core::Widget:Gonk component considering the code it's touching.

Comment 17

[Vincent Lin[:vilin]]

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) from comment #16)
> This bug should probably be moved to the Core::Widget:Gonk component
> considering the code it's touching.

The major changes include gonk, hal, layout/RefreshDriver and layer/CompositeParent.
Is it still suitable for Core::Widget:Gonk component.

Comment 18

[Vincent Lin[:vilin]]

Attachment: WIP.pdf

Introduction to this Bug.

Comment 19

[Vincent Lin[:vilin]]

Comment on attachment 8391016 [details] [diff] [review]
nsAppShell.patch

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

The current WIP is just a prototype. I'm eager to profile the improvement of uniformity now. All the necessary refactorings will be considered once these patches are gonna to be landed.

Comment 20

[Andre Graziani (:graziani)]

Attachment: Results_for_ProjectButterWIP0313.png

Results for ProjectButterWIP0313.patch:
- Using ScrollGraph + Orangutan in Contact List
- Tested 5 times
- Graph is deltaY[px] x refresh[#] during drag

The pattern is the same observed using high-speed camera: a good uniformity, but sometimes a peak occurs

Comment 21

[leo.bugzilla.gecko]

Cleaning needinfo, by comment 20

Comment 22

[Vincent Lin[:vilin]]

(In reply to Andre Graziani (:graziani) from comment #20)
> Created attachment 8392701 [details]
> Results_for_ProjectButterWIP0313.png
> 
> Results for ProjectButterWIP0313.patch:
> - Using ScrollGraph + Orangutan in Contact List
> - Tested 5 times
> - Graph is deltaY[px] x refresh[#] during drag
> 
> The pattern is the same observed using high-speed camera: a good uniformity,
> but sometimes a peak occurs

Ok. Great.
I think the peak is probably due to the reflow takes more than 16ms. You can make sure by Systrace.
Bug 931668 is going to improve the performance of re-styling. I'm also looking forward to see if it's helpful.

Comment 23

[u459114]

Vincent, please share your profile data(in a png file) here to explain what you wrote in previous comment

Comment 24

[Vincent Lin[:vilin]]

(In reply to C.J. Ku[:CJKu] from comment #23)
> Vincent, please share your profile data(in a png file) here to explain what
> you wrote in previous comment

Here it is.
https://bugzilla.mozilla.org/show_bug.cgi?id=931668#c36

Comment 25

[Vincent Lin[:vilin]]

Attachment: ProjectButterWIP0320.patch

Move Vsync notification to HwcComposer2D.
Add test code(notifyMotionTest) in nsAppShell.cpp.

Comment 26

[Mason Chang [Inactive] [:mchang]]

Project butter is awesome. I finally started understanding a lot of what's actually going on. At least for the GeckoInputDispatcher, is there a reason we can't modify the touch device drivers or kernel to also dispatch events at vsyncs rather than having it at the higher 100ms sample rate? Thanks!

Comment 27

[Vincent Lin[:vilin]]

(In reply to Mason Chang [:mchang] from comment #26)
> Project butter is awesome. I finally started understanding a lot of what's
> actually going on. At least for the GeckoInputDispatcher, is there a reason
> we can't modify the touch device drivers or kernel to also dispatch events
> at vsyncs rather than having it at the higher 100ms sample rate? Thanks!

Good question.
IMHO~
We can't actually define how touch device driver works after all that belongs to partner's implementation. However, do it in device driver is platform-dependent. 

By the way, it's 100Hz(10ms) sample rate, but not 100ms sample rate. Just fix the typo:)

Comment 28

[Mason Chang [Inactive] [:mchang]]

Hmm maybe not for old devices, but what about future devices? Could we make that a requirement so that we don't have to down sample?

Comment 29

[Vincent Lin[:vilin]]

(In reply to Mason Chang [:mchang] from comment #28)
> Hmm maybe not for old devices, but what about future devices? Could we make
> that a requirement so that we don't have to down sample?

So far, B2G was built on top of device driver and even HAL for Android(except for the ongoing TV project). According to my experience working on HWComposer with partner, it's not easy to convince partner to modify driver or HAL for B2G. Furthermore, Android also handle resampling stuff in InputTransport module. If you search "Project Butter" in the internet, you may just see some conceptual article, but not find out this detail. You will do if you trace the code on AOSP.

Comment 30

[Vincent Lin[:vilin]]

Attachment: bug-980241-fix.patch

WIP

Comment 31

[Vincent Lin[:vilin]]

Attachment: bug-980241-fix.patch

WIP

Comment 32

[Vincent Lin[:vilin]]

Attachment: bug-980241-fix.patch

WIP~
Register with info. of observer type.

Comment 33

[u459114]

Comment on attachment 8405953 [details] [diff] [review]
bug-980241-fix.patch

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

To integrate with Bug 987529
1. Change RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH) to VsyncDispatcher::RegisterRefreshTimer(this)
2. Change UnregisterVsyncObserver(this, VSYNC_TYPE_REFRESH) to VsyncDispatcher::UnregisterRefreshTimer(this)

::: layout/base/nsRefreshDriver.cpp
@@ +256,5 @@
> +  virtual void StartTimer()
> +  {
> +    if (!mTimerRunning)
> +      RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH);
> +      mTimerRunning = true;

if (!mTimerRunning)
{
  RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH);
  mTimerRunning = true;
}

@@ +628,5 @@
>  nsRefreshDriver::Shutdown()
>  {
>    // clean up our timers
>    delete sRegularRateTimer;
> +  delete sHWVsyncTimer;

if (sHWVsyncTimer)

@@ +708,5 @@
>                                                             DEFAULT_INACTIVE_TIMER_DISABLE_SECONDS * 1000.0);
>      return sThrottledRateTimer;
>    }
>  
> +  if (Preferences::GetBool("hal.hw-vsync", false)) {

Preferences::GetBool("hal.hw-vsync", false)
Preferences::GetBool("xxx.enablesilk", false)

xxx.enablesilk means we trigger refresh driver by VsyncDispatcher(rename to SilkDispatcher?)
"hal.hw-vsync == true" means we use HW vsync from HWComposer to drive VsyncDispatcher.
"hal.hw-vsync == fasle" means we use SW vsync(a real-time timer) to drive VsyncDispatcher.
"hal.hw-vsync" should be respect in Bug 987527. In this bug, we need only care xxx.enablesilk

Comment 34

[u459114]

BTW, I think this bug is the clearest one among all silk bugs.
Please refurbish it and ask for review after bug 987524 and bug 987527 ready

Comment 35

[Vincent Lin[:vilin]]

Attachment: bug-980241-fix.patch

Include "gfx.silk-enable" preference.

Comment 36

[Vincent Lin[:vilin]]

Attachment: bug-980241-fix.patch

Include "gfx.silk-enable" preference.

Comment 37

[u459114]

Since we decided to explicit call nsRefreshDriver::Tick in VsyncDispatcher
1. nsRefreshDriver needs to know VsyncDispatcher: calls VsyncDispatcher::RegistryRefreshDriver
2. VsyncDispatcher needs to know nsRefreshDriver: calls nsRefreshDriver::Tick

We can't separate change of nsRefreshDriver into another bug(mutual relative). Close this one, let's keep trace progress of SiLK implementation by Bug 987529.