Multiple screenshots get queued up and processed unnecessarily

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
Firefox 17
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

I'm seeing often upwards of 4 or 5 full-page screenshot slices get queued and then processed, which is basically useless because they only get processed on idle (i.e. when the page has stopped changing, we re-render the page 5 times for screenshot purposes). These events should be coalesced somehow.
Created attachment 634991 [details] [diff] [review]
Drop pending events for the tab if doing a whole-page screenshot
Attachment #634991 - Flags: review?(blassey.bugs)
Attachment #634991 - Flags: review?(blassey.bugs) → review+
Comment on attachment 634991 [details] [diff] [review]
Drop pending events for the tab if doing a whole-page screenshot

I realized this patch has a problem - it doesn't clear out sAcumulatedRect. This could result in the wrong rect being sent to setCheckerboardBitmap if some pending events are dropped. I'll try to figure out a fix for that.
Attachment #634991 - Flags: review+ → review-
Also I see sometimes that the "partial update" rect (i.e. the union of all the addRectToRepaint calls) is the whole page, and we end up re-screenshotting everything just because it goes through a different code path. The fact that this is on a 5-second delay with no coalescing again results in a lot of extra screenshotting work that should be unnecessary.
Created attachment 635506 [details] [diff] [review]
Do better coalescing of screenshots

Here's a more significant overhaul of the code that tries to minimize spurious screenshotting activity. I modified the PendingScreenshot class to encapsulate a full screenshot and keep a list of the events corresponding to the slices, and then discard these PendingScreenshot objects if we start doing a new full-page screenshot. There's also some robustification so that if we switch tabs we don't keep processing and updating screenshots from the previous tabs, and it also accounts for rounding errors in the slicing code. Still needs more testing, but feedback is welcome.
Attachment #634991 - Attachment is obsolete: true
Attachment #635506 - Flags: feedback?(blassey.bugs)
Comment on attachment 635506 [details] [diff] [review]
Do better coalescing of screenshots

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

if you're going to move this code out of GeckoAppShell, preserve the history by doing hg cp GeckoAppShell.java ScreenshotHandler.java and then deleting stuff from the two files
Attachment #635506 - Flags: feedback?(blassey.bugs) → feedback-
Just another data point: loading planet.mozilla.org on a Galaxy Nexus triggers 7 full-page screenshots, for a total of 56697 slices of screenshot. That backlog takes 98 seconds to clear after the page is done loading.
Created attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

Rebased to tip and put ScreenshotLayer back in GeckoAppShell for now. I can do the "hg cp" in a follow up.
Attachment #635506 - Attachment is obsolete: true
Attachment #642973 - Flags: review?(blassey.bugs)
Attachment #642973 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3955b96bcd
Duplicate of this bug: 775030
Duplicate of this bug: 753987
https://hg.mozilla.org/mozilla-central/rev/ae3955b96bcd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Updated

5 years ago
Depends on: 775976
Request approval for uplift?
Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

[Approval Request Comment]
Bug caused by (feature/regressing bug #): low-res screenshot
User impact if declined: a lot of time is wasted taking redundant screenshots, specially just after page load. this *should* happen only when gecko is idle, but still burns up a lot of CPU time
Testing completed (on m-c, etc.): on m-c, but should probably bake for another couple of days
Risk to taking this patch (and alternatives if risky): mobile-only, but is a fairly significant change and has a risk of regressions. for example bug 775976 is a crasher that was introduced by this patch
String or UUID changes made by this patch: none
Attachment #642973 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Blocks: 765952
Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

[Triage Comment]
I don't think we can take a patch with possibility of regression unless we've got data of the improvements we'd be taking. Please re-nominate if/when we get that info. For now, let's maintain current state in 16.
Attachment #642973 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Comment 15

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #14)
> I don't think we can take a patch with possibility of regression unless
> we've got data of the improvements we'd be taking.
This bug blocks bug 765952 that causes hangs and crashes in 16.0a2.
This may also have fixed bug 774616

Updated

5 years ago
Blocks: 774616
Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

Re-requesting aurora approval. This has been baking on m-c for almost a week with no word of any regressions (other than the NPE in bug 775976). This patch definitely fixes bug 765952 and possibly also fixes 774616, so I think it's worth uplifting to aurora.
Attachment #642973 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
Attachment #642973 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You're right to re-nom, definitely want fixes to Aurora mobile crash rate right now.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bec50c0a33f
status-firefox16: --- → fixed
status-firefox17: --- → fixed
Duplicate of this bug: 765952

Updated

5 years ago
No longer blocks: 765952
You need to log in before you can comment on or make changes to this bug.