990832 - Build a tool to Visualize Frame Uniformity with ScrollGraph Output

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

Have a front end to automatically graph touch event input move velocity to the scrollable layer's transform to measure frame uniformity.

Comment 1

[Vincent Lin[:vilin]]

Cool~
I wanna know if it could make a loop test like this one I hacked in InputReaderThread.
https://drive.google.com/file/d/0B4nXGzdjFCO7Sl9NNkxXX1A4T2c/edit?usp=sharing

The test cases should at least cover(hit the scenarios) like horizontal move, vertical move and APZC.

Comment 2

[Jinho Hwang [:jeffhwang]]

Attachment: Patches for Frame Uniformity tool

http://htmlpreview.github.io/?https://github.com/JinhoHwang/FxOSFrameUniformity/blob/master/FrameUniformity.html

This is a tool which draws graphs related with frame uniformity, touch events and layer movement.

How to use this
      Step1 : apply patches attached on this
      Step2 : build your gecko and download it to your device
      Step3 : add a preference to user preference file 
              ex) echo "pref('layers.scroll-graph',true);"  >> /system/b2g/defaults/pref/user.js 
      Step4 : do some dragging or scrolling on device 
              if you use Orangutan, it allows us to make constant movement 
      Step5 : collect scrollgraph log data
              ex) adb log cat | grep ScrollGraph
      step5 : open the html file in your browser and put the log data into the textarea
              and click the "Reload data" button.

Comment 3

[Benoit Girard (:BenWa)]

Cool! Let's get this checked in, the frame uniformatity.html too if it's standalone.

Comment 4

[Jinho Hwang [:jeffhwang]]

Attachment: screenshot : scrolling on Homescreen

Comment 6

[Jinho Hwang [:jeffhwang]]

(In reply to Benoit Girard (:BenWa) from comment #3)
> Cool! Let's get this checked in, the frame uniformatity.html too if it's
> standalone.

Yes, It's standalone.

I reuse the code of bgirard's Rendertrace(which is greatly useful to understand behavior of APZ).
but I'm not a JS guy, any help or feedback welcome.

Since this is using ScrollGraph log data, this is dependent on Bug 982534.

Comment 7

[Mason Chang [Inactive] [:mchang]]

I'll get this hosted on people.mozilla.org. Thanks for the tool! 

Where should we check in frame uniformaity.html? Mozilla-central?

Comment 8

[Mason Chang [Inactive] [:mchang]]

Just a note to myself to host this on people.moz.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8403817 [details] [diff] [review]
Patches for Frame Uniformity tool

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

::: widget/gonk/nsAppShell.cpp
@@ +713,4 @@
>              break;
>          case AMOTION_EVENT_ACTION_POINTER_DOWN:
>          case AMOTION_EVENT_ACTION_POINTER_UP:
> +            return;

What's this change?  Should it be inside #ifdef SCROLL_GRAPH, or is it a fix for some other issue unrelated to this?

Comment 10

[Jinho Hwang [:jeffhwang]]

(In reply to Milan Sreckovic [:milan] from comment #9)
> Comment on attachment 8403817 [details] [diff] [review]
> Patches for Frame Uniformity tool
> 
> Review of attachment 8403817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nsAppShell.cpp
> @@ +713,4 @@
> >              break;
> >          case AMOTION_EVENT_ACTION_POINTER_DOWN:
> >          case AMOTION_EVENT_ACTION_POINTER_UP:
> > +            return;
> 
> What's this change?  Should it be inside #ifdef SCROLL_GRAPH, or is it a fix
> for some other issue unrelated to this?

Thanks you for the review. 
You're right. I made a mistake. 
I changed the code because of a Orangutan bug. 
So, I should open a bug for it and remove it from here.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Mason Chang [:mchang] from comment #7)
> I'll get this hosted on people.mozilla.org. Thanks for the tool! 
> 
> Where should we check in frame uniformaity.html? Mozilla-central?

We could check it in to gfx/layers but it hotlinks several JS libraries so it's not good style to have files in mozilla-central that hotlink resources. Maybe for now hosting it on people.mozilla.org is the right thing to do.

Comment 12

[Mason Chang [Inactive] [:mchang]]

Hosted on people.mozilla.org - http://people.mozilla.org/~mchang/FrameUniformity.html

Comment 13

[Mason Chang [Inactive] [:mchang]]

Jeff, any news on this patch? Is it possible to please get this reviewed and landed? Thanks!

Comment 14

[Jinho Hwang [:jeffhwang]]

Attachment: UniformityInfo.patch

Sorry for the delay in updates.

Patch for review.
Mason could you please update http://people.mozilla.org/~mchang/FrameUniformity.html this html file?

Comment 15

[Benoit Girard (:BenWa)]

I've been rolling some of this into the profiler to make this easier to use. But I don't track input events:

http://benoitgirard.wordpress.com/2014/05/05/profiler-frame-performance-and-debugging/
(The layer movement uniformity was added after this blog posted)

Comment 16

[Jinho Hwang [:jeffhwang]]

Attachment: Uniformity.patch

(In reply to Benoit Girard (:BenWa) from comment #15)
> I've been rolling some of this into the profiler to make this easier to use.
> But I don't track input events:

Wow, great ~ :) it's nice to have frame uniformity tool inside of profiler.

One question about the tool, the graph on your blog looks like representing FPS. 
If the dots are near 16ms, that means 60FPS.
Is there any extra information for movements of layers?
If it provides that informations, it will be much useful thought..

Anyway, I'll just keep this patch here for the project Slik in progress.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8420791 [details] [diff] [review]
Uniformity.patch

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

Mostly looks good. Please fix the nits and ask for review again.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +247,5 @@
> +      aLayer->GetEffectiveVisibleRegion().GetBounds().height < 300) {
> +    return;
> +  }
> +
> +  FrameMetrics fm = aLayer->AsContainerLayer()->GetFrameMetrics();

nit: Please rename to frameMetrics. Short variable names are difficult to read.

::: widget/gonk/nsAppShell.cpp
@@ +67,5 @@
>  #ifdef MOZ_NUWA_PROCESS
>  #include "ipc/Nuwa.h"
>  #endif
>  
> +#ifdef UNIFORMITY_INFO

I generally don't think we need to #ifdef this. Can we remove all the #ifdef UNIFORMITY_INFO.

@@ +232,5 @@
>                         touch.coords.getAxisValue(AMOTION_EVENT_AXIS_PRESSURE))
>      );
>  }
>  
> +#ifdef UNIFORMITY_INFO

same

@@ +236,5 @@
> +#ifdef UNIFORMITY_INFO
> +static void
> +printUniformityInfo(UserInputData& aData)
> +{
> +    char * touchAction;

nit: char* touchAction.

@@ +602,5 @@
>          , mKeyDownCount(0)
>          , mTouchEventsFiltered(false)
>          , mKeyEventsFiltered(false)
> +    {
> +#ifdef UNIFORMITY_INFO

Same, just make the default false and initialize it later. We can only read preferences on the main thread. Does the GeckoInputDispatcher occur only on the main thread?

@@ +655,5 @@
>      int mKeyDownCount;
>      bool mTouchEventsFiltered;
>      bool mKeyEventsFiltered;
>      BitSet32 mTouchDown;
> +#ifdef UNIFORMITY_INFO

Same, please remove UNIFORMITY_INFO ifdefs.

@@ +770,5 @@
>          nsEventStatus status = nsEventStatus_eIgnore;
>          if (action != AMOTION_EVENT_ACTION_HOVER_MOVE) {
>              bool captured;
>              status = sendTouchEvent(data, &captured);
> +#ifdef UNIFORMITY_INFO

Same

@@ +771,5 @@
>          if (action != AMOTION_EVENT_ACTION_HOVER_MOVE) {
>              bool captured;
>              status = sendTouchEvent(data, &captured);
> +#ifdef UNIFORMITY_INFO
> +            if(mEnabledUniformityInfo)

Please put printUniformity in brackets {}. Thanks!

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Jinho Hwang [:jeffhwang] from comment #16)
> Created attachment 8420791 [details] [diff] [review]
> Uniformity.patch
> 
> (In reply to Benoit Girard (:BenWa) from comment #15)
> > I've been rolling some of this into the profiler to make this easier to use.
> > But I don't track input events:
> 
> Wow, great ~ :) it's nice to have frame uniformity tool inside of profiler.
> 
> One question about the tool, the graph on your blog looks like representing
> FPS. 
> If the dots are near 16ms, that means 60FPS.
> Is there any extra information for movements of layers?
> If it provides that informations, it will be much useful thought..
> 
> Anyway, I'll just keep this patch here for the project Slik in progress.

I haven't blogged about layer position uniformity but it was recently added after the blog post:
https://www.dropbox.com/s/r45t7h1c2qa42i5/Screenshot%202014-05-12%2014.46.10.png

I don't track the layer position so we could have you patch report that information to the profiler to add this feature in as well.

Comment 19

[Jinho Hwang [:jeffhwang]]

Attachment: UniformityInfo.patch

Thanks mason for the review.

(In reply to Mason Chang [:mchang] from comment #17)
> Mostly looks good. Please fix the nits and ask for review again.

Asking you for review again after fixing the nits you point.

> We can only read
> preferences on the main thread. Does the GeckoInputDispatcher occur only on
> the main thread?

Yes, It does running on main thread so far.


(In reply to Benoit Girard (:BenWa) from comment #18)
> I haven't blogged about layer position uniformity but it was recently added > > after the blog post:
> https://www.dropbox.com/s/r45t7h1c2qa42i5/Screenshot%202014-05-12%2014.46.10.png

Thanks BenWa for the detail info.
It looks great :)
wow~ Profiler is getting improved. 

> I don't track the layer position so we could have you patch report that 
> information to the profiler to add this feature in as well.

How about the touch information? are you willing to add the touch information as well?

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Jinho Hwang [:jeffhwang] from comment #19)
> > I don't track the layer position so we could have you patch report that 
> > information to the profiler to add this feature in as well.
> 
> How about the touch information? are you willing to add the touch
> information as well?

I mean I do track layer position but I don't track touch information.

Comment 21

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8423742 [details] [diff] [review]
UniformityInfo.patch

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

Looks good to me, but I'm not a module owner. Asking :benwa for review as well.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +236,5 @@
>  }
>  
> +static void PrintUniformityInfo(Layer* aLayer)
> +{
> +

nit: remove line break.

Comment 22

[Mike Lee [:mlee]]

r+'d. What happens next?

Comment 23

[Mason Chang [Inactive] [:mchang]]

I have to rebase for mozilla-central. Sorry for letting it bitrot.

Comment 24

[Mason Chang [Inactive] [:mchang]]

Attachment: uniformity.patch

Rebased for mozilla-central tip. Try build: https://tbpl.mozilla.org/?tree=Try&rev=cec658b2a513

Comment 25

[Mason Chang [Inactive] [:mchang]]

Looks like a good try build for b2g.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6f7d5a01b3

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae6f7d5a01b3