Last Comment Bug 713874 - Black thumbnails are produced by GeckoSoftwareLayerClient.getBitmap()
: Black thumbnails are produced by GeckoSoftwareLayerClient.getBitmap()
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 12 Branch
: ARM Android
: P1 normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
: 715269 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-28 08:39 PST by Carla Nadastean
Modified: 2012-01-19 09:04 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
screenshot (95.44 KB, image/png)
2011-12-28 08:39 PST, Carla Nadastean
no flags Details
patch (8.28 KB, patch)
2012-01-10 23:08 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review-
Details | Diff | Splinter Review
patch (8.48 KB, patch)
2012-01-12 15:31 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Carla Nadastean 2011-12-28 08:39:27 PST
Created attachment 584579 [details]
screenshot

Mozilla /5.0 (Android;Linux armv7l;rv:12.0a1) Gecko/20111228 Firefox/12.0a1 Fennec/12.0a1 
Device: HTC Desire Z (Android 2.3)

Steps to reproduce:
1.Open Fennec app.
2.Navigate to a webpage. 
3.Tap on "+" button and open a new tab(page).
4.Tap on tab button and verify page thumbnails are displayed properly.

Expected result:
Page thumbnails are displayed properly.

Actual results:
Page thumbnails are black. (screenshot attached)
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-12-28 09:45:55 PST

*** This bug has been marked as a duplicate of bug 711543 ***
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-01-04 22:59:36 PST
Not a dupe given:

(In reply to Carla Nadastean from comment #0)
> Steps to reproduce:
> 1.Open Fennec app.
> 2.Navigate to a webpage. 
2a.Wait long enough to make sure the thumbnail gets saved
> 3.Tap on "+" button and open a new tab(page).
> 4.Tap on tab button and verify page thumbnails are displayed properly.

I added some logging to confirm 2a is happening, we're getting all black bitmaps from mSoftwareLayerClient.getBitmap(). Snorp, could this be a gralloc regression?
Comment 3 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-05 09:44:50 PST
(In reply to Brad Lassey [:blassey] from comment #2)
> Not a dupe given:
> 
> (In reply to Carla Nadastean from comment #0)
> > Steps to reproduce:
> > 1.Open Fennec app.
> > 2.Navigate to a webpage. 
> 2a.Wait long enough to make sure the thumbnail gets saved
> > 3.Tap on "+" button and open a new tab(page).
> > 4.Tap on tab button and verify page thumbnails are displayed properly.
> 
> I added some logging to confirm 2a is happening, we're getting all black
> bitmaps from mSoftwareLayerClient.getBitmap(). Snorp, could this be a
> gralloc regression?

Yeah, probably so. We don't copy the bits into the bitmap that GeckoSoftwareLayerClient holds (nor do we want to). We should be able to just pass whatever bitmap we want to use in to nsWindow::DrawTo() for screenshots. I thought that's how it worked, actually...
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-01-10 23:08:49 PST
Created attachment 587604 [details] [diff] [review]
patch
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-11 22:01:07 PST
Comment on attachment 587604 [details] [diff] [review]
patch


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>                 } else {
>                     mLastScreen = null;
>+                    GeckoAppShell.sendEventToGecko(
>+                        new GeckoEvent("Tab:Screenshot", 
>+                                       "{\"uri\": \"" + mLastUri + "\", " +

You send this to JS and send it back to Java but you don't use it in processThumbnail. Let's remove it

>+                                       "\"width\": \"" + mSoftwareLayerClient.getWidth() + "\", " +
>+                                       "\"height\": \"" + mSoftwareLayerClient.getHeight() + "\", " +
>+                                       "\"id\": \"" + tab.getId() + "\" }"));

"id" -> "tabID" to be consistent with other messages

>+    void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
>+        mLastScreen = compressed;
>+        if (bitmap == null)
>+            bitmap = BitmapFactory.decodeByteArray(compressed, 0, compressed.length);
>+        if (thumbnailTab.getURL().equals("about:home"))
>+            thumbnailTab.updateThumbnail(null);

You could check this right away and return, right? BitmapFactory.decodeByteArray is not cheap even for white bitmaps.

>+            } else if (event.equals("Tab:ScreenshotData")) {
>+                int tabId = message.getInt("tabID");
>+                Tab tab = Tabs.getInstance().selectTab(tabId);

You don't want Tabs.getInstance().selectTab(tabId), you want Tabs.getInstance().getTab(tabId)

>+                processThumbnail(tab, null, Base64.decode(message.getString("data").substring(22), Base64.DEFAULT));

I wonder if this should be on the background thread? SessionSnapshotRunnable already calls processThumbnail on the background thread, right?

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java

>+                if (mTileLayer instanceof MultiTileLayer) {
>+                    b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
>                                                CairoUtils.cairoFormatTobitmapConfig(mFormat));
>-
>-                if (mTileLayer instanceof MultiTileLayer)
>                     copyPixelsFromMultiTileLayer(b);
>-                else
>+                } else if (mTileLayer instanceof SingleTileLayer) {
>+                    b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
>+                                               CairoUtils.cairoFormatTobitmapConfig(mFormat));
>+                    copyPixelsFromMultiTileLayer(b);

Do you really want to call copyPixelsFromMultiTileLayer for a SingleTileLayer?

>+                } else {
>+                    Log.w(LOGTAG, "getBitmap() called on a layer (" + mTileLayer + ") we don't know how to get a bitmap from");
>+                }

You mentioned returning null for a gralloc case

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  screenshotTab: function screenshotTab(aData) {
>+      let json = JSON.parse(aData);
>+      let tab = this.getTabForId(parseInt(json.id));
>+      let width = parseInt(json.width);
>+      let height =  parseInt(json.height);

Could you split this function so most of it lives in Tab? Add a method called Tab.screenshot(aWidth, aHeight) and put the lower section of code in it

>+      let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");  
>+      canvas.setAttribute('width',width);  
>+      canvas.setAttribute('height',height);

Use " instead of ' and add a space after the commas

Test for a valid browser and browser.contentWindow first and return if we don't have it:

if (!this.browser || !this.browser.contentWindow)
  return;

>+      let ctx = canvas.getContext("2d");
>+      ctx.drawWindow(tab.browser.contentWindow, 0, 0, width, height, "rgb(255, 255, 255)");
>+      let message = {
>+        gecko: {
>+          type: "Tab:ScreenshotData",
>+          tabID: tab.id,
>+	  width: width,

Got a TAB here
>+          height: height,
>+          data: canvas.toDataURL(),
>+	  url: json.url

And here, but remove this anyway. It's not used in Java

>+    } else if (aTopic == "Tab:Screenshot") {
>+      this.screenshotTab(aData);

You could add the JSON getters here and call Tab.screenshot here, removing BrowserApp.screenshotTab

r- to see a new patch

also, the size of the string being sent over the message could be huge. not sure what ramifications could exist from that.
Comment 6 Peter Retzer (:pretzer) 2012-01-12 12:30:15 PST
I only see black thumbnails on about:home as well. Is this covered with this bug, or should I file a new one?
If it is indeed covered, can someone adjust the summary, please?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-12 12:37:26 PST
(In reply to pretzer from comment #6)
> I only see black thumbnails on about:home as well. Is this covered with this
> bug, or should I file a new one?
> If it is indeed covered, can someone adjust the summary, please?

same bug
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-12 13:08:14 PST
*** Bug 715269 has been marked as a duplicate of this bug. ***
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-01-12 15:31:19 PST
Created attachment 588210 [details] [diff] [review]
patch
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-13 06:49:20 PST
Comment on attachment 588210 [details] [diff] [review]
patch

This might have issues due to the size of the data, but we need something like this for other bugs too. Like getting the thumbnail of a background tab.

Eventually, I'd like all thumbnails to not use the surface layer image.

Let's see how this works!
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-13 06:53:01 PST
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 588210 [details] [diff] [review]
> patch
> 
> This might have issues due to the size of the data, but we need something
> like this for other bugs too. Like getting the thumbnail of a background tab.
> 
> Eventually, I'd like all thumbnails to not use the surface layer image.
> 
> Let's see how this works!

It draws at the requested thumbnail size, no? So the data shouldn't be too big? Though I guess there is some substantial overhead from converting to a data uri (which is base64 or something?). Anyway, seems good to me.
Comment 12 Jonathan Kew (:jfkthame) 2012-01-16 04:49:50 PST
Would've been good to note when this landed on -inbound. Anyway, it's now merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/ba3335f34100
Comment 13 Aaron Train [:aaronmt] 2012-01-16 08:50:23 PST
Aurora nom?
Comment 14 Aaron Train [:aaronmt] 2012-01-16 08:51:32 PST
Verified on M-C
Samsung Nexus S (Android 4.0.3)
20120116083307
http://hg.mozilla.org/mozilla-central/rev/b3b8d62e0d92
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-01-17 05:44:59 PST
Comment on attachment 588210 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Any device with gralloc will have all black thumbnails. The dependent bug for background tab thumbnails will not work.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
very low
Comment 16 Alex Keybl [:akeybl] 2012-01-18 08:27:46 PST
Comment on attachment 588210 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for aurora.
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-01-19 09:04:17 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3c8c43d7147

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