don't use canvas to take screenshots for tab thumbnails

RESOLVED FIXED in Firefox 13

Status

()

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

People

(Reporter: blassey, Assigned: kats)

Tracking

unspecified
Firefox 13
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 wontfix, firefox12 wontfix, firefox13 fixed, fennec+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 594395 [details] [diff] [review]
patch

This seems a bit cleaner, and avoids compressing and decompressing the images each time. It might be nice not to involve js at all, but I don't see how else to map tabIds onto nsIDOMWindows. It may be cleaner to have an js implement an xpcom service to resolve those mappings, which would eliminate going through the observer system.
Attachment #594395 - Flags: review?(doug.turner)
Comment on attachment 594395 [details] [diff] [review]
patch

>     void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
>-        if (Tabs.getInstance().isSelectedTab(thumbnailTab))
>+        if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
>+            if (compressed == null) {
>+                ByteArrayOutputStream bos = new ByteArrayOutputStream();
>+                bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
>+            }
>             mLastScreen = compressed;

Drive-by comment: the compressed PNG isn't saved into "compressed" here.

Comment 2

6 years ago
Comment on attachment 594395 [details] [diff] [review]
patch

Review of attachment 594395 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +637,5 @@
>      void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
> +        if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
> +            if (compressed == null) {
> +                ByteArrayOutputStream bos = new ByteArrayOutputStream();
> +                bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);

also - does this need to be on a separate thread
Attachment #594395 - Flags: review?(doug.turner) → review?(bugmail.mozilla)
Comment on attachment 594395 [details] [diff] [review]
patch

>     void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
>-        if (Tabs.getInstance().isSelectedTab(thumbnailTab))
>+        if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
>+            if (compressed == null) {
>+                ByteArrayOutputStream bos = new ByteArrayOutputStream();
>+                bitmap.compress(Bitmap.CompressFormat.PNG, 0, bos);
>+            }
>             mLastScreen = compressed;
>+        }

As mentioned in above comments, this should actually fill in compressed, and be bumped off to a different thread (right now it looks like it's running straight on the Gecko thread).

>+    __android_log_print(ANDROID_LOG_INFO, "GeckoBridge" , "success");
>+    return NS_OK;
>+}
>+
>+void AndroidBridge::NotifyScreenshot(unsigned char* data, int size, int tabId, int width, int height)
>+{
>+    
>+    JNIEnv* jenv = AndroidBridge::GetJNIEnv();

Don't need the AndroidBridge:: prefix on the GetJNIEnv() call here, and need to check for !jenv.

>+    AutoLocalJNIFrame jniFrame(jenv, 1);
>+    jobject buffer = jenv->NewDirectByteBuffer(data, size);

Should check for a NULL return value and bail out here. Maybe return false for error conditions so that we don't print "success" back in TakeScreenshot.
Attachment #594395 - Flags: review?(bugmail.mozilla) → review-
Created attachment 594894 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #594395 - Attachment is obsolete: true
Attachment #594894 - Flags: review?(doug.turner)
New patch is same as old patch.

Comment 6

6 years ago
Comment on attachment 594894 [details] [diff] [review]
patch

what kats said
Attachment #594894 - Flags: review?(doug.turner) → review-
Created attachment 595454 [details] [diff] [review]
patch
Attachment #594894 - Attachment is obsolete: true
Attachment #595454 - Flags: review?(doug.turner)

Comment 8

6 years ago
Comment on attachment 595454 [details] [diff] [review]
patch

Review of attachment 595454 [details] [diff] [review]:
-----------------------------------------------------------------

kats should also review.

::: mobile/android/base/GeckoApp.java
@@ +635,5 @@
>      }
>      
>      void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
> +        if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
> +            if (compressed == null) {

add timing to this. log it.  follow up bug okay.

::: mobile/android/base/Tabs.java
@@ -290,5 @@
>                  selectTab(message.getInt("tabID"));
> -            } else if (event.equals("Tab:ScreenshotData")) {
> -                Tab tab = getTab(message.getInt("tabID"));
> -                String data = message.getString("data");
> -                if (data.length() < 22)

why 22? magic numbers need comments.

::: widget/android/AndroidBridge.cpp
@@ +1969,5 @@
> +             nsPresContext::CSSPixelsToAppUnits(srcY),
> +             nsPresContext::CSSPixelsToAppUnits(srcW),
> +             nsPresContext::CSSPixelsToAppUnits(srcH));
> +
> +    nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(nsIntSize(dstW, dstH), gfxASurface::ImageFormatRGB16_565);

why 565 here?  shouldn't you just let the java side convert?

@@ +1980,5 @@
> +
> +void AndroidBridge::NotifyScreenshot(unsigned char* data, int size, int tabId, int width, int height)
> +{
> +    
> +    JNIEnv* jenv = GetJNIEnv();

extra space above this.
Attachment #595454 - Flags: review?(doug.turner)
Attachment #595454 - Flags: review?(bugmail.mozilla)
Attachment #595454 - Flags: review-
(In reply to Doug Turner (:dougt) from comment #8)
> Comment on attachment 595454 [details] [diff] [review]
> patch
> 
> Review of attachment 595454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> kats should also review.
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +635,5 @@
> >      }
> >      
> >      void processThumbnail(Tab thumbnailTab, Bitmap bitmap, byte[] compressed) {
> > +        if (Tabs.getInstance().isSelectedTab(thumbnailTab)) {
> > +            if (compressed == null) {
> 
> add timing to this. log it.  follow up bug okay.
why?
> 
> ::: mobile/android/base/Tabs.java
> @@ -290,5 @@
> >                  selectTab(message.getInt("tabID"));
> > -            } else if (event.equals("Tab:ScreenshotData")) {
> > -                Tab tab = getTab(message.getInt("tabID"));
> > -                String data = message.getString("data");
> > -                if (data.length() < 22)
> 
> why 22? magic numbers need comments.
That's deleted code
> 
> ::: widget/android/AndroidBridge.cpp
> @@ +1969,5 @@
> > +             nsPresContext::CSSPixelsToAppUnits(srcY),
> > +             nsPresContext::CSSPixelsToAppUnits(srcW),
> > +             nsPresContext::CSSPixelsToAppUnits(srcH));
> > +
> > +    nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(nsIntSize(dstW, dstH), gfxASurface::ImageFormatRGB16_565);
> 
> why 565 here?  shouldn't you just let the java side convert?
huh? convert to what?
Comment on attachment 595454 [details] [diff] [review]
patch

Patch looks fine to me. The extra line dougt pointed out can be removed on landing.
Attachment #595454 - Flags: review?(bugmail.mozilla) → review+
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → All

Updated

6 years ago
tracking-fennec: ? → +
kats, blassey is out.  can you push today?
Assignee: blassey.bugs → bugmail.mozilla

Updated

6 years ago
Attachment #595454 - Flags: approval-mozilla-beta?
Attachment #595454 - Flags: approval-mozilla-aurora?
Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=f9d0cde25286 and will land on inbound if that goes green.

I also noticed while rebasing that Tabs.java doesn't need to register for the Tab:ScreenshotData event any more so I took that line out as well.
Comment on attachment 595454 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Attachment #595454 - Flags: approval-mozilla-beta?
Attachment #595454 - Flags: approval-mozilla-beta+
Attachment #595454 - Flags: approval-mozilla-aurora?
Attachment #595454 - Flags: approval-mozilla-aurora+
Try was green, landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e11bcec31a
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → fixed
Target Milestone: --- → Firefox 13
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/363fa033d483 - -p android,android-xul is probably a better bet for try pushes ;)
Target Milestone: Firefox 13 → ---
https://hg.mozilla.org/mozilla-central/rev/c7e11bcec31a
https://hg.mozilla.org/mozilla-central/rev/363fa033d483
Updated patch to (hopefully) not crash XUL fennec, pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=20378c0a4e25 this time for both android and android-xul :)
Try was green for updated patch, so landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3852b3874127
https://hg.mozilla.org/mozilla-central/rev/3852b3874127
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
fyi, fails to apply:

Justin@ORION /d/sources/mozilla-beta
$ hg qpush
applying 724210
unable to find 'widget/android/AndroidBridge.cpp' for patching
3 out of 3 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.cpp.rej
unable to find 'widget/android/AndroidBridge.h' for patching
2 out of 2 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.h.rej
unable to find 'widget/android/nsIAndroidBridge.idl' for patching
2 out of 2 hunks FAILED -- saving rejects to file widget/android/nsIAndroidBridge.idl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 724210
I assume we're not going to bother landing these on 11 and 12 since native fennec is now targeting 13 for the 1.0 release.
status-firefox11: affected → wontfix
status-firefox12: affected → wontfix
Comment on attachment 595454 [details] [diff] [review]
patch

clearing beta approval per akeybl on IRC, No Fennec Native Shipping for Gecko 11
Attachment #595454 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.