Last Comment Bug 766647 - Multiple screenshots get queued up and processed unnecessarily
: Multiple screenshots get queued up and processed unnecessarily
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 17
Assigned To: (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
: 753987 765952 775030 (view as bug list)
Depends on: 775976
Blocks: 766643 774616
  Show dependency treegraph
 
Reported: 2012-06-20 11:31 PDT by (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-08-01 07:15 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Drop pending events for the tab if doing a whole-page screenshot (5.24 KB, patch)
2012-06-20 11:44 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review-
Details | Diff | Review
Do better coalescing of screenshots (29.80 KB, patch)
2012-06-21 15:31 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: feedback-
Details | Diff | Review
Do better coalescing of screenshots (27.48 KB, patch)
2012-07-17 08:48 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-20 11:31:10 PDT
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.
Comment 1 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-20 11:44:30 PDT
Created attachment 634991 [details] [diff] [review]
Drop pending events for the tab if doing a whole-page screenshot
Comment 2 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 08:07:22 PDT
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.
Comment 3 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 08:55:48 PDT
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.
Comment 4 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 15:31:39 PDT
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.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-06-26 12:25:11 PDT
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
Comment 6 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-17 07:58:45 PDT
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.
Comment 7 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-17 08:48:27 PDT
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.
Comment 8 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 08:27:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3955b96bcd
Comment 9 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 08:50:16 PDT
*** Bug 775030 has been marked as a duplicate of this bug. ***
Comment 10 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-18 12:49:59 PDT
*** Bug 753987 has been marked as a duplicate of this bug. ***
Comment 11 Ed Morley [:emorley] 2012-07-19 07:35:26 PDT
https://hg.mozilla.org/mozilla-central/rev/ae3955b96bcd
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-20 08:15:23 PDT
Request approval for uplift?
Comment 13 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-20 08:31:27 PDT
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
Comment 14 Alex Keybl [:akeybl] 2012-07-23 10:55:16 PDT
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.
Comment 15 Scoobidiver (away) 2012-07-23 11:02:45 PDT
(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.
Comment 16 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-23 12:27:47 PDT
This may also have fixed bug 774616
Comment 17 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-25 10:53:37 PDT
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.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-26 16:24:12 PDT
You're right to re-nom, definitely want fixes to Aurora mobile crash rate right now.
Comment 19 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-30 12:16:52 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bec50c0a33f
Comment 20 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-08-01 07:12:53 PDT
*** Bug 765952 has been marked as a duplicate of this bug. ***

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