tab screenshots unnecessarily big for background tabs

RESOLVED FIXED in Firefox 11

Status

()

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

People

(Reporter: blassey, Assigned: blassey)

Tracking

unspecified
Firefox 12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Created attachment 591654 [details] [diff] [review]
patch

this depends on some methods added in bug 721032
Assignee: nobody → blassey.bugs
Attachment #591654 - Flags: review?(mark.finkle)
Also, I should note that this is a huge speed improvement over painting the entire tile size
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
Attachment #591654 - Flags: review?(mark.finkle) → review-
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.
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
Created attachment 591703 [details] [diff] [review]
patch
Attachment #591654 - Attachment is obsolete: true
Attachment #591703 - Flags: review?(mark.finkle)
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
Attachment #591703 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/965d183e0f3d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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
Attachment #591703 - Flags: approval-mozilla-aurora?
Comment on attachment 591703 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #591703 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6eb8b265cdfa
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
status-firefox12: --- → fixed
You need to log in before you can comment on or make changes to this bug.