Last Comment Bug 705171 - Use RENDERMODE_WHEN_DIRTY instead of RENDERMODE_CONTINUOUSLY
: Use RENDERMODE_WHEN_DIRTY instead of RENDERMODE_CONTINUOUSLY
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 major (vote)
: ---
Assigned To: Chris Lord [:cwiiis]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: native_droid_panning
  Show dependency treegraph
 
Reported: 2011-11-24 10:58 PST by Patrick Walton (:pcwalton)
Modified: 2012-01-09 11:14 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Draw on demand instead of continuously (12.80 KB, patch)
2011-11-29 08:18 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Draw on demand instead of continuously (address review comments) (13.93 KB, patch)
2011-11-30 08:57 PST, Chris Lord [:cwiiis]
bugmail: review+
pwalton: review+
Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2011-11-24 10:58:19 PST
Currently, we constantly render even when the screen hasn't changed. This is a waste in many ways: battery life, most importantly, but also stealing CPU time from Gecko and other apps for no reason. We should call setRenderMode(RENDERMODE_WHEN_DIRTY) when setting up the LayerView and insert requestRender() calls as needed instead.

This is a high priority bug IMHO; we can't ship if we kill the battery.
Comment 1 John Hammink 2011-11-28 10:00:43 PST
Once this is resolved, what is the best way to test this patch?
Comment 2 Chris Lord [:cwiiis] 2011-11-29 08:18:59 PST
Created attachment 577628 [details] [diff] [review]
Draw on demand instead of continuously

This does as suggested. I've not been able to get the FPS counter quite quite, but I wanted to get the patch up for review and we can work that bit out.
Comment 3 Chris Lord [:cwiiis] 2011-11-29 08:20:44 PST
On second thoughts, I think the frame counter is sort-of right, but doesn't take into account the time from render to vblank. Suggestions welcome.
Comment 4 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-30 07:00:34 PST
Comment on attachment 577628 [details] [diff] [review]
Draw on demand instead of continuously

Overall looks pretty good to me. I can't see any issues with the patch. Just one minor nit below.

>     private void checkFPS() {
>+        mFrameTime += mView.getRenderTime();
>+        mFrameCount ++;
>+
>         if (System.currentTimeMillis() >= mFrameCountTimestamp + 1000) {
>             mFrameCountTimestamp = System.currentTimeMillis();
> 
>+            mFrameTime = 1000000000L / (mFrameTime / mFrameCount);

I'd prefer this line read more like "mFrameCount = (int)(mFramecount * 1000000000L / mFrameTime); // extrapolate FPS based on time taken by frames drawn" ...

>-                mFPSLayer.setText(mFrameCount + " FPS");
>+                mFPSLayer.setText(mFrameTime + " FPS");

... and keep using mFrameCount in this line. It just seems odd to have a time variable being used as "frames per second".
Comment 5 Chris Lord [:cwiiis] 2011-11-30 08:57:56 PST
Created attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

Addressed the comment and fixed that I was using the incorrect beginTransaction function, which caused page updates without viewport updates to miss triggering a redraw.
Comment 6 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-30 09:09:37 PST
Comment on attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

r+, but you should probably still get feedback from pcwalton on this before landing.
Comment 7 Chris Lord [:cwiiis] 2011-11-30 09:11:24 PST
Comment on attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

Agreed.
Comment 8 Patrick Walton (:pcwalton) 2011-11-30 09:22:04 PST
Comment on attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

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

r=me
Comment 9 Patrick Walton (:pcwalton) 2011-11-30 09:22:34 PST
Let's get this landed ASAP, this is a big win in all sorts of ways -- thanks for taking this on.
Comment 10 Chris Lord [:cwiiis] 2011-11-30 09:28:07 PST
http://hg.mozilla.org/projects/birch/rev/b747973fdd09

Note You need to log in before you can comment on or make changes to this bug.