896822 - Generation of tab thumbnails is extremely slow

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Kitching [:ckitching]]

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]]

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]]

Attachment: Convert thumbnails to RGBA before sending to Java. r=bnicholson

Comment 3

[Chris Kitching [:ckitching]]

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]]

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

Attachment: Convert thumbnails to RGBA before sending to Java. V2 (Commit msg change)

Comment 6

[Brian Nicholson (:bnicholson)]

Incidentally, I think this will also fix bug 890590.

Comment 7

[Chris Kitching [:ckitching]]

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

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

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

Shouldn't the call to the swizzling code be inside an if (is24bit) guard?

Comment 10

[Chris Kitching [:ckitching]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

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

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

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

Err 24-bit.

Comment 13

[Chris Kitching [:ckitching]]

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

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

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]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?)]

(In reply to Chris Kitching [:ckitching] from comment #15)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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]]

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]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4cdffa1784

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e4cdffa1784