The default bug view has changed. See this FAQ.

Generation of tab thumbnails is extremely slow

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
Graphics, Panning and Zooming
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25 fixed, fennec25+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.)
(Assignee)

Comment 2

4 years ago
Created attachment 779548 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. r=bnicholson
Attachment #779548 - Flags: review?(bnicholson)
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Updated

4 years ago
OS: Linux → Android
Hardware: x86_64 → ARM
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 5

4 years ago
Created attachment 779555 [details] [diff] [review]
Convert thumbnails to RGBA before sending to Java. V2 (Commit msg change)
Attachment #779548 - Attachment is obsolete: true
Attachment #779548 - Flags: review?(bnicholson)
Attachment #779555 - Flags: review?(roc)
Attachment #779555 - Flags: review?(roc) → review+
Incidentally, I think this will also fix bug 890590.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

4 years ago
(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?
(Assignee)

Comment 8

4 years ago
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.
Blocks: 890590
Shouldn't the call to the swizzling code be inside an if (is24bit) guard?
Keywords: checkin-needed
(Assignee)

Comment 10

4 years ago
(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.
Err 24-bit.
(Assignee)

Comment 13

4 years ago
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.
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+
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Comment 18

4 years ago
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.
Attachment #779888 - Attachment is obsolete: true
Attachment #780037 - Flags: review?(bugmail.mozilla)
Attachment #780037 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4cdffa1784
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+
status-firefox24: --- → unaffected
status-firefox25: --- → affected
https://hg.mozilla.org/mozilla-central/rev/4e4cdffa1784
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Updated

4 years ago
Depends on: 898028

Updated

4 years ago
status-firefox25: affected → fixed
You need to log in before you can comment on or make changes to this bug.