705171 - Use RENDERMODE_WHEN_DIRTY instead of RENDERMODE_CONTINUOUSLY

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

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]

Once this is resolved, what is the best way to test this patch?

Comment 2

[Chris Lord [:cwiiis]]

Attachment: 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]]

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

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

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]]

Attachment: 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

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

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]]

Comment on attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

Agreed.

Comment 8

[Patrick Walton (:pcwalton)]

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)]

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]]

http://hg.mozilla.org/projects/birch/rev/b747973fdd09