Last Comment Bug 721209 - tab screenshots unnecessarily big for background tabs
: tab screenshots unnecessarily big for background tabs
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-25 14:22 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-01-30 11:39 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
patch (3.83 KB, patch)
2012-01-25 16:31 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review-
Details | Diff | Splinter Review
patch (4.16 KB, patch)
2012-01-25 22:36 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2012-01-25 14:22:04 PST
for the foreground tab we need to get an entire tile's worth of screenshot data for our OOM restart splash screen. Currently we use that size for all the screenshots which is much larger than is needed for background tabs
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2012-01-25 16:31:11 PST
Created attachment 591654 [details] [diff] [review]
patch

this depends on some methods added in bug 721032
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-01-25 16:35:21 PST
Also, I should note that this is a huge speed improvement over painting the entire tile size
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 19:23:30 PST
Comment on attachment 591654 [details] [diff] [review]
patch

>diff -r 36645d20b354 -r 88ab6b3e6b36 mobile/android/base/GeckoApp.java

>     void getAndProcessThumbnailForTab(final Tab tab) {

>                 new GeckoEvent("Tab:Screenshot", 

>+                               "{\"src_width\": \"" + sw + "\", " +
>+                               "\"src_height\": \"" + sh + "\", " +
>+                               "\"dst_width\": \"" + dw + "\", " +
>+                               "\"dst_height\": \"" + dh + "\", " +

You might be cursing me for this, but "xxx_width" and "xxx_height" are really un-JSON names. I'd really prefer sub-objects here:

>+                               "{" +
>+                                   "\"source\": {" +
>+                                       "\"width\": \"" + sw + "\", " +
>+                                       "\"height\": \"" + sh + "\", " +
>+                                   "}, " +
>+                                   "\"destination\": {" +
>+                                       "\"width\": \"" + dw + "\", " +
>+                                       "\"height\": \"" + dh + "\", " +
>+                                   "}, " +
>                                    "\"tabID\": \"" + tab.getId() + "\"" +
>                                "}"));

(I think I got the syntax right)

But I really think we should use JSONObject to build up the JSON now. It's too hard to manage and error prone:

JSONObject message = new JSONObject();
message.put("tabID", tab.getId());

JSONObject source = new JSONObject();
source.put("width", sw);
source.put("height", sh);
message.put("source", source);

JSONObject destination = new JSONObject();
source.put("width", dw);
source.put("height", dh);
message.put("destination", destination);

String json = message.toString();


>diff -r 36645d20b354 -r 88ab6b3e6b36 mobile/android/chrome/content/browser.js

>   doScreenshotTab: function doScreenshotTab(aData) {
>       let json = JSON.parse(aData);
>       let tab = this.getTabForId(parseInt(json.tabID));

>+      let dst_width = parseInt(json.dst_width);
>+      let dst_height =  parseInt(json.dst_height);
>+      let src_width = parseInt(json.src_width);
>+      let src_height =  parseInt(json.src_height);
>+      tab.screenshot(src_width, src_height, dst_width, dst_width);

If we do things right, you should not need the parseInt calls, and since we are passing objects, you should be able to do:

        tab.screenshot(json.source, json.destination);

>-  screenshot: function(aWidth, aHeight) {
>+  screenshot: function(aSrcWidth, aSrcHeight, aDstWidth, aDstHeight) {

    screenshot: function(aSrcSize, aDstSize) {

r- for the JSON fixup
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 20:54:52 PST
Also, while you are in there can you add an optimization to Tab.screenshot?

>       canvas.setAttribute("width", aWidth);  
>       canvas.setAttribute("height", aHeight);
>+      canvas.setAttribute("moz-opaque", "true");

From bug 430906:
We can get a significant performance win if the canvas can know ahead of time that translucency is not important on the canvas; this is, the entire canvas will be fully painted opaquely.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 21:04:41 PST
Got another canvas optimization:

>-      ctx.drawWindow(this.browser.contentWindow, 0, 0, aWidth, aHeight, "rgb(255, 255, 255)");
>+      ctx.drawWindow(this.browser.contentWindow, 0, 0, aWidth, aHeight, "rgb(255, 255, 255)", ctx.DRAWWINDOW_DO_NOT_FLUSH);

I was thinking of adding ctx.DRAWWINDOW_ASYNC_DECODE_IMAGES in there too. The Windows taskbar tab preview code uses it. It doesn't sync block to decode images. I'm not sure if it would be good for background tabs though. Might be worth a try to see if it renders images in background tabs though.

http://mxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm#394
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-01-25 22:36:54 PST
Created attachment 591703 [details] [diff] [review]
patch
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-26 05:48:32 PST
Comment on attachment 591703 [details] [diff] [review]
patch


>+            } catch(JSONException jsonEx) {
>+                Log.w(LOGTAG, "Constructing the JSON data for Tab:Screenshot event failed, go yell at mfinkle", jsonEx);
>+            }

I'd rather be anonymous please :)

>diff -r 8ef11d3eb030 -r db57e646f40a mobile/android/chrome/content/browser.js

>+  screenshot: function(aSrc, aDst) {
>       if (!this.browser || !this.browser.contentWindow)
>           return;
>       let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>-      canvas.setAttribute("width", aWidth);  
>-      canvas.setAttribute("height", aHeight);
>+      canvas.setAttribute("width", aDst.width);  
>+      canvas.setAttribute("height", aDst.height);
>       let ctx = canvas.getContext("2d");
>-      ctx.drawWindow(this.browser.contentWindow, 0, 0, aWidth, aHeight, "rgb(255, 255, 255)");
>+      ctx.drawWindow(this.browser.contentWindow, 0, 0, aSrc.width, aSrc.height, "rgb(255, 255, 255)");

I can file a new bug for the other canvas optimizations I guess
Comment 8 Marco Bonardo [::mak] 2012-01-26 16:23:33 PST
https://hg.mozilla.org/mozilla-central/rev/965d183e0f3d
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-01-26 20:46:09 PST
Comment on attachment 591703 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
we produce screenshots that are ~200x bigger than needed, wasting time and memory
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
none, we were just cropping the too big thumbnails
Comment 10 Alex Keybl [:akeybl] 2012-01-27 16:17:16 PST
Comment on attachment 591703 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-01-30 11:39:43 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/6eb8b265cdfa

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