Closed Bug 896822 Opened 7 years ago Closed 7 years ago

Generation of tab thumbnails is extremely slow

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
fennec 25+ ---

People

(Reporter: ckitching, Assigned: ckitching)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.)
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
OS: Linux → Android
Hardware: x86_64 → ARM
Assignee: nobody → ckitching
(In reply to Chris Kitching [:ckitching] from comment #0)
> It also makes thumbnail generation around 6000 times faster.

http://i.imgur.com/h1Sd3sf.gif
Attachment #779548 - Attachment is obsolete: true
Attachment #779548 - Flags: review?(bnicholson)
Attachment #779555 - Flags: review?(roc)
Incidentally, I think this will also fix bug 890590.
Keywords: checkin-needed
(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?
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.
Shouldn't the call to the swizzling code be inside an if (is24bit) guard?
Keywords: checkin-needed
(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?
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.
Ah. That did look a bit fishy. Here is a revised patch - no more crazy colours on very old devices.
Attachment #779555 - Attachment is obsolete: true
Attachment #779888 - Flags: review?(bugmail.mozilla)
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.
Attachment #779888 - Flags: review?(bugmail.mozilla) → review+
(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?
(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.
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.
Okay, style sorted out. Good job spotting that bug, kats.
Attachment #779888 - Attachment is obsolete: true
Attachment #780037 - Flags: review?(bugmail.mozilla)
Attachment #780037 - Flags: review?(bugmail.mozilla) → review+
Keywords: checkin-needed
Tracking for Fx25 since this appears to be a followup to the 24bit color work, and that is not on Fx24.
tracking-fennec: --- → 25+
https://hg.mozilla.org/mozilla-central/rev/4e4cdffa1784
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 898028
You need to log in before you can comment on or make changes to this bug.