Last Comment Bug 896822 - Generation of tab thumbnails is extremely slow
: Generation of tab thumbnails is extremely slow
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 25
Assigned To: Chris Kitching [:ckitching]
:
Mentors:
Depends on: 898028
Blocks: 890590
  Show dependency treegraph
 
Reported: 2013-07-22 18:26 PDT by Chris Kitching [:ckitching]
Modified: 2013-07-26 06:54 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
25+


Attachments
Convert thumbnails to RGBA before sending to Java. r=bnicholson (3.20 KB, patch)
2013-07-22 18:36 PDT, Chris Kitching [:ckitching]
no flags Details | Diff | Review
Convert thumbnails to RGBA before sending to Java. V2 (Commit msg change) (3.19 KB, patch)
2013-07-22 18:51 PDT, Chris Kitching [:ckitching]
roc: review+
Details | Diff | Review
Convert thumbnails to RGBA before sending to Java. V3 Adding 24-bit check. (3.21 KB, patch)
2013-07-23 11:12 PDT, Chris Kitching [:ckitching]
bugmail.mozilla: review+
Details | Diff | Review
Convert thumbnails to RGBA before sending to Java. V4 Style nit (3.22 KB, patch)
2013-07-23 15:14 PDT, Chris Kitching [:ckitching]
bugmail.mozilla: review+
Details | Diff | Review

Description Chris Kitching [:ckitching] 2013-07-22 18:26:59 PDT
Profiling page loads on a Nexus 4 it seems 3 seconds of CPU time were being used converting the tab thumbnail image from BGRA to ARGB for rendering after it had been received by Java.

This seems to be harming performance somewhat hugely. I present a patch which converts the image to ARGB before sending through JNI in the Android Bridge. Since there already exists a utility function for this conversion, the patch is extremely simple. It also makes thumbnail generation around 6000 times faster.
Comment 1 Chris Kitching [:ckitching] 2013-07-22 18:29:50 PDT
Profile without patch, demonstrating the problem:
https://www.dropbox.com/s/r7o6mu6evvgy1na/nytimes0.trace
(Profiling started when the page load started, and halted three seconds after the last draw complete event.)
Comment 2 Chris Kitching [:ckitching] 2013-07-22 18:36:16 PDT
Created attachment 779548 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. r=bnicholson
Comment 3 Chris Kitching [:ckitching] 2013-07-22 18:37:00 PDT
Profile after applying the patch - the huge splodge of getPixel calls at the end have completely vanished:
https://www.dropbox.com/s/rz3jxdavb8d9pii/nytimes1.trace
Comment 4 Aaron Train [:aaronmt] 2013-07-22 18:39:24 PDT
(In reply to Chris Kitching [:ckitching] from comment #0)
> It also makes thumbnail generation around 6000 times faster.

http://i.imgur.com/h1Sd3sf.gif
Comment 5 Chris Kitching [:ckitching] 2013-07-22 18:51:38 PDT
Created attachment 779555 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. V2 (Commit msg change)
Comment 6 Brian Nicholson (:bnicholson) 2013-07-22 19:47:51 PDT
Incidentally, I think this will also fix bug 890590.
Comment 7 Chris Kitching [:ckitching] 2013-07-22 19:49:11 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Incidentally, I think this will also fix bug 890590.

I'm not convined. What makes you say this?
Comment 8 Chris Kitching [:ckitching] 2013-07-22 19:49:37 PDT
Ah(In reply to Chris Kitching [:ckitching] from comment #7)
> (In reply to Brian Nicholson (:bnicholson) from comment #6)
> > Incidentally, I think this will also fix bug 890590.
> 
> I'm not convined. What makes you say this?

Belay that. It helps if I read the thread.
Comment 9 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-07-23 05:01:29 PDT
Shouldn't the call to the swizzling code be inside an if (is24bit) guard?
Comment 10 Chris Kitching [:ckitching] 2013-07-23 10:09:21 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Shouldn't the call to the swizzling code be inside an if (is24bit) guard?

Quite possibly - but what happens about the BGRA-ness of the image if it's not 24 bit? What should we do then? Wouldn't we end up with the channels in the wrong order if we omitted to convert?
Comment 11 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-07-23 11:05:06 PDT
If it's not 25-bit the image is going to be in 565 format and doesn't need swizzling. The code you removed from ThumbnailHelper.java did the same thing.
Comment 12 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-07-23 11:05:26 PDT
Err 24-bit.
Comment 13 Chris Kitching [:ckitching] 2013-07-23 11:12:36 PDT
Created attachment 779888 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. V3 Adding 24-bit check.

Ah. That did look a bit fishy. Here is a revised patch - no more crazy colours on very old devices.
Comment 14 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-07-23 11:32:03 PDT
Comment on attachment 779888 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. V3 Adding 24-bit check.

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

::: widget/android/AndroidBridge.cpp
@@ +2729,5 @@
>      context->Translate(pt);
>      context->Scale(scale * bufW / srcW, scale * bufH / srcH);
>      rv = presShell->RenderDocument(r, renderDocFlags, bgColor, context);
> +    if (is24bit)
> +      gfxUtils::ConvertBGRAtoRGBA(surf);

4-space indent and braces, please.
Comment 15 Chris Kitching [:ckitching] 2013-07-23 11:44:11 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 779888 [details] [diff] [review]
> Convert thumbnails to RGBA before sending to Java. V3 Adding 24-bit check.
> 
> Review of attachment 779888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidBridge.cpp
> @@ +2729,5 @@
> >      context->Translate(pt);
> >      context->Scale(scale * bufW / srcW, scale * bufH / srcH);
> >      rv = presShell->RenderDocument(r, renderDocFlags, bgColor, context);
> > +    if (is24bit)
> > +      gfxUtils::ConvertBGRAtoRGBA(surf);
> 
> 4-space indent and braces, please.

But https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNI.cpp#811 - Brace usage in this file for single-line ifs seems to be consistent. Should I be fixing the linked nit, too?
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2013-07-23 12:11:11 PDT
(In reply to Chris Kitching [:ckitching] from comment #15)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> > Comment on attachment 779888 [details] [diff] [review]
> > Convert thumbnails to RGBA before sending to Java. V3 Adding 24-bit check.
> > 
> > Review of attachment 779888 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/android/AndroidBridge.cpp
> > @@ +2729,5 @@
> > >      context->Translate(pt);
> > >      context->Scale(scale * bufW / srcW, scale * bufH / srcH);
> > >      rv = presShell->RenderDocument(r, renderDocFlags, bgColor, context);
> > > +    if (is24bit)
> > > +      gfxUtils::ConvertBGRAtoRGBA(surf);
> > 
> > 4-space indent and braces, please.
> 
> But
> https://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNI.
> cpp#811 - Brace usage in this file for single-line ifs seems to be
> consistent. Should I be fixing the linked nit, too?

Sadly, the file is not consistent. In that case, the predominate current policy can over-rule the file. So using the braces with a single line IF is preferred.

Also, avoid doing nit changes like that to non-effected code in a file with mixed usage. If it was the last brace to fix in the file, then OK. But in this case, it's not.
Comment 17 Chris Kitching [:ckitching] 2013-07-23 12:46:44 PDT
It occurs to me that I wrote "consistent" when I wanted to write "Not consistent". Grand.

Thanks for the info - I've always been a little unsure how to deal with this sort of sitation. I'll get a revised patch out shortly.
Comment 18 Chris Kitching [:ckitching] 2013-07-23 15:14:50 PDT
Created attachment 780037 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. V4 Style nit

Okay, style sorted out. Good job spotting that bug, kats.
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-07-24 06:41:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4cdffa1784
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2013-07-24 07:15:30 PDT
Tracking for Fx25 since this appears to be a followup to the 24bit color work, and that is not on Fx24.
Comment 21 Wes Kocher (:KWierso) 2013-07-24 20:00:20 PDT
https://hg.mozilla.org/mozilla-central/rev/4e4cdffa1784

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