Last Comment Bug 724210 - don't use canvas to take screenshots for tab thumbnails
: don't use canvas to take screenshots for tab thumbnails
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 13
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 22:54 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-03-05 14:24 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
+


Attachments
patch (9.40 KB, patch)
2012-02-03 22:54 PST, Brad Lassey [:blassey] (use needinfo?)
bugmail: review-
Details | Diff | Splinter Review
patch (9.40 KB, patch)
2012-02-06 20:06 PST, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review-
Details | Diff | Splinter Review
patch (9.64 KB, patch)
2012-02-08 10:48 PST, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review-
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2012-02-03 22:54:50 PST
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.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-04 13:09:21 PST
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) 2012-02-05 10:57:57 PST
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:kats@mozilla.com) 2012-02-05 16:09:44 PST
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?) 2012-02-06 20:06:17 PST
Created attachment 594894 [details] [diff] [review]
patch
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-07 07:14:55 PST
New patch is same as old patch.
Comment 6 Doug Turner (:dougt) 2012-02-08 10:44:42 PST
Comment on attachment 594894 [details] [diff] [review]
patch

what kats said
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-02-08 10:48:31 PST
Created attachment 595454 [details] [diff] [review]
patch
Comment 8 Doug Turner (:dougt) 2012-02-08 11:36:32 PST
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?) 2012-02-08 11:43:22 PST
(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:kats@mozilla.com) 2012-02-08 12:43:15 PST
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) 2012-02-09 09:16:02 PST
kats, blassey is out.  can you push today?
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-09 10:43:48 PST
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] 2012-02-09 13:10:40 PST
Comment on attachment 595454 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-09 19:46:55 PST
Try was green, landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e11bcec31a
Comment 15 Phil Ringnalda (:philor) 2012-02-09 22:27:59 PST
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 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-10 08:30:33 PST
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:kats@mozilla.com) 2012-02-10 11:17:01 PST
Try was green for updated patch, so landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3852b3874127
Comment 19 Ed Morley [:emorley] 2012-02-10 19:21:09 PST
https://hg.mozilla.org/mozilla-central/rev/3852b3874127
Comment 20 Justin Wood (:Callek) 2012-02-27 16:16:08 PST
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:kats@mozilla.com) 2012-02-27 21:30:42 PST
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) 2012-03-05 14:24:36 PST
Comment on attachment 595454 [details] [diff] [review]
patch

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

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