Closed Bug 705171 Opened 8 years ago Closed 8 years ago

Use RENDERMODE_WHEN_DIRTY instead of RENDERMODE_CONTINUOUSLY

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: pcwalton, Assigned: cwiiis)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Once this is resolved, what is the best way to test this patch?
Assignee: nobody → pwalton
Priority: -- → P1
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)
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".
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+
Comment on attachment 577984 [details] [diff] [review]
Draw on demand instead of continuously (address review comments)

Agreed.
Attachment #577984 - Flags: review?(pwalton)
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+
Let's get this landed ASAP, this is a big win in all sorts of ways -- thanks for taking this on.
http://hg.mozilla.org/projects/birch/rev/b747973fdd09
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
You need to log in before you can comment on or make changes to this bug.