Use RENDERMODE_WHEN_DIRTY instead of RENDERMODE_CONTINUOUSLY

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: cwiiis)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Once this is resolved, what is the best way to test this patch?
Assignee: nobody → pwalton
Priority: -- → P1
(Assignee)

Comment 2

6 years ago
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.
Attachment #577628 - Flags: review?(pwalton)
(Assignee)

Comment 3

6 years ago
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 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".
(Assignee)

Comment 5

6 years ago
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.
Assignee: pwalton → chrislord.net
Attachment #577628 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #577628 - Flags: review?(pwalton)
Attachment #577984 - Flags: review?(kgupta)
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.
Attachment #577984 - Flags: review?(kgupta) → review+
(Assignee)

Comment 7

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

Agreed.
Attachment #577984 - Flags: review?(pwalton)
(Reporter)

Comment 8

6 years ago
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
Attachment #577984 - Flags: review?(pwalton) → review+
(Reporter)

Comment 9

6 years ago
Let's get this landed ASAP, this is a big win in all sorts of ways -- thanks for taking this on.
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/projects/birch/rev/b747973fdd09
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.