724210 - don't use canvas to take screenshots for tab thumbnails

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

[Doug Turner (:dougt)]

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

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

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

Attachment: patch

Comment 5

[Kartikaya Gupta (email:[e-mail])]

New patch is same as old patch.

Comment 6

[Doug Turner (:dougt)]

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

what kats said

Comment 7

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

Attachment: patch

Comment 8

[Doug Turner (:dougt)]

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.

Comment 9

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

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

[Kartikaya Gupta (email:[e-mail])]

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

Patch looks fine to me. The extra line dougt pointed out can be removed on landing.

Comment 11

[Doug Turner (:dougt)]

kats, blassey is out.  can you push today?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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 13

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Try was green, landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e11bcec31a

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/363fa033d483 - -p android,android-xul is probably a better bet for try pushes ;)

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/c7e11bcec31a
https://hg.mozilla.org/mozilla-central/rev/363fa033d483

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Try was green for updated patch, so landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3852b3874127

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3852b3874127

Comment 20

[Justin Wood (:Callek)]

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

Comment 21

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 22

[Justin Wood (:Callek)]

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

clearing beta approval per akeybl on IRC, No Fennec Native Shipping for Gecko 11