713874 - Black thumbnails are produced by GeckoSoftwareLayerClient.getBitmap()

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Carla Nadastean]

Attachment: 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 2

[Brad Lassey [:blassey] (use needinfo?)]

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) ([e-mail]) (he/him)]

(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?)]

Attachment: patch

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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)]

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?)]

(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 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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) ([e-mail]) (he/him)]

(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]]

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]]

Aurora nom?

Comment 14

[Aaron Train [:aaronmt]]

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?)]

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]]

Comment on attachment 588210 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for aurora.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3c8c43d7147