Closed
Bug 990832
Opened 11 years ago
Closed 10 years ago
Build a tool to Visualize Frame Uniformity with ScrollGraph Output
Categories
(Core :: Graphics, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mchang, Assigned: jeffhwang)
References
Details
(Keywords: perf, Whiteboard: [c=devtools,uniformity p= s= u=])
Attachments
(2 files, 4 obsolete files)
468.14 KB,
image/jpeg
|
Details | |
4.38 KB,
patch
|
Details | Diff | Splinter Review |
Have a front end to automatically graph touch event input move velocity to the scrollable layer's transform to measure frame uniformity.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → faraojh
Status: NEW → ASSIGNED
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Cool! Let's get this checked in, the frame uniformatity.html too if it's standalone.
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8403832 -
Attachment description: example : scrolling on Homescreen → screenshot : scrolling on Homescreen
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Depends on: 982534
Reporter | ||
Comment 7•11 years ago
|
||
I'll get this hosted on people.mozilla.org. Thanks for the tool!
Where should we check in frame uniformaity.html? Mozilla-central?
Flags: needinfo?(bgirard)
Reporter | ||
Comment 8•11 years ago
|
||
Just a note to myself to host this on people.moz.
Flags: needinfo?(mchang)
Comment 9•11 years ago
|
||
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?
Updated•11 years ago
|
Whiteboard: [c=automation p= s= u=] → [c=devtools p= s= u=uniformity]
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [c=devtools p= s= u=uniformity] → [c=devtools,uniformity p= s= u=]
Comment 11•11 years ago
|
||
(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.
Flags: needinfo?(bgirard)
Reporter | ||
Comment 12•11 years ago
|
||
Hosted on people.mozilla.org - http://people.mozilla.org/~mchang/FrameUniformity.html
Flags: needinfo?(mchang)
Reporter | ||
Comment 13•11 years ago
|
||
Jeff, any news on this patch? Is it possible to please get this reviewed and landed? Thanks!
Flags: needinfo?(faraojh)
Assignee | ||
Comment 14•11 years ago
|
||
Sorry for the delay in updates.
Patch for review.
Mason could you please update http://people.mozilla.org/~mchang/FrameUniformity.html this html file?
Attachment #8403817 -
Attachment is obsolete: true
Attachment #8420717 -
Flags: review?(mchang)
Flags: needinfo?(faraojh) → needinfo?(mchang)
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Attachment #8420717 -
Attachment is obsolete: true
Attachment #8420717 -
Flags: review?(mchang)
Attachment #8420791 -
Flags: review?(mchang)
Reporter | ||
Comment 17•11 years ago
|
||
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!
Attachment #8420791 -
Flags: review?(mchang)
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(mchang)
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
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?
Attachment #8420791 -
Attachment is obsolete: true
Attachment #8423742 -
Flags: review?(mchang)
Comment 20•11 years ago
|
||
(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.
Reporter | ||
Comment 21•11 years ago
|
||
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.
Attachment #8423742 -
Flags: review?(mchang)
Attachment #8423742 -
Flags: review?(bgirard)
Attachment #8423742 -
Flags: review+
Updated•11 years ago
|
Attachment #8423742 -
Flags: review?(bgirard) → review+
Reporter | ||
Comment 23•10 years ago
|
||
I have to rebase for mozilla-central. Sorry for letting it bitrot.
Reporter | ||
Comment 24•10 years ago
|
||
Rebased for mozilla-central tip. Try build: https://tbpl.mozilla.org/?tree=Try&rev=cec658b2a513
Attachment #8423742 -
Attachment is obsolete: true
Comment 26•10 years ago
|
||
Keywords: checkin-needed
Comment 27•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(mchang)
You need to log in
before you can comment on or make changes to this bug.
Description
•