Black thumbnails are produced by GeckoSoftwareLayerClient.getBitmap()

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: Carla Nadastean, Assigned: blassey)

Tracking

12 Branch
Firefox 12
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 verified, fennec11+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 711543
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?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

5 years ago
Status: REOPENED → NEW
(Assignee)

Updated

5 years ago
Assignee: nobody → snorp
Priority: -- → P1
(Assignee)

Updated

5 years ago
tracking-fennec: --- → 11+
(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...
Created attachment 587604 [details] [diff] [review]
patch
Assignee: snorp → blassey.bugs
Attachment #587604 - Flags: review?(mark.finkle)
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.
Attachment #587604 - Flags: review?(mark.finkle) → review-
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?
(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
Summary: Black thumbnails are displayed in tabs menu → Black thumbnails are produced by GeckoSoftwareLayerClient.getBitmap()
(Assignee)

Updated

5 years ago
Duplicate of this bug: 715269
Created attachment 588210 [details] [diff] [review]
patch
Attachment #587604 - Attachment is obsolete: true
Attachment #588210 - Flags: review?(mark.finkle)
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!
Attachment #588210 - Flags: review?(mark.finkle) → review+
(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.
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → Firefox 12
Aurora nom?
Verified on M-C
Samsung Nexus S (Android 4.0.3)
20120116083307
http://hg.mozilla.org/mozilla-central/rev/b3b8d62e0d92
Status: RESOLVED → VERIFIED
status-firefox11: --- → affected
status-firefox12: --- → verified
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
Attachment #588210 - Flags: approval-mozilla-aurora?
Comment on attachment 588210 [details] [diff] [review]
patch

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