801676 - [BrowserAPI] Make getScreenshot() use JPEG instead of PNG

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gabriele Svelto [:gsvelto]]

getScreenshot() is currently a very expensive operation on ARM because of its use of PNG encoding. When the source image is in ARGB format the PNG encoding code will trigger a color-conversion step that requires dividing every color channel of every pixel by the pixel's alpha channel. ARM processors do not have a hardware division instruction which causes every division to be done by a very slow replacement function provided by GCC (__udivsi3).

Switching image encoding to JPEG makes the operation significantly cheaper on ARM devices, for example taking a screenshot of the gallery application on an Otoro device yields the following durations for getScreenshot() (also see the attached traces):

PNG encoding   820ms
JPEG encoding  150ms

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Gaia gallery application trace on Otoro using PNG encoding

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: Gaia gallery application trace on Otoro using JPEG encoding

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Use JPEG encoding instead of PNG in getScreenshot()

Comment 4

[Gabriele Svelto [:gsvelto]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=77304d9ee78c

Comment 5

[Justin Lebar (not reading bugmail)]

FYI, we're trying to conserve our limited CPU resources, so we're encouraging people to think twice before pushing to try with -p all -u all for simple patches.

If you really want a -u all build, you could push again with -p linux64 -u all.

In this case, -u mochitest-2 (the suite which happens to contain the browser-element tests) would likely be sufficient.  If it were me, I'd probably do -b d instead of -b o, because debug builds are more likely to die with an assertion, but that's not a big deal.

That said, I've been bitten many times by insufficient testing after I tried to conserve resources, so I totally understand if you prefer to be safe here.

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #5)
> FYI, we're trying to conserve our limited CPU resources, so we're
> encouraging people to think twice before pushing to try with -p all -u all
> for simple patches.
> 
> If you really want a -u all build, you could push again with -p linux64 -u
> all.
> 
> In this case, -u mochitest-2 (the suite which happens to contain the
> browser-element tests) would likely be sufficient.  If it were me, I'd
> probably do -b d instead of -b o, because debug builds are more likely to
> die with an assertion, but that's not a big deal.
> 
> That said, I've been bitten many times by insufficient testing after I tried
> to conserve resources, so I totally understand if you prefer to be safe here.

Thanks for the tips. I went all out on this one (as well as on my previous patch) mostly because I'm still unfamiliar with how the test harness works and which unit tests are where, so I decided that I would be better safe than sorry. I'll keep your suggestions in mind next time I push to try, especially if it's a small patch like this one.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 671591 [details] [diff] [review]
Use JPEG encoding instead of PNG in getScreenshot()

Tests are looking good except for two errors that looked sporadic, re-running them succeeded in both cases:

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-77304d9ee78c/try-android-armv6/try_tegra_android-armv6_test-reftest-1-bm20-tests1-tegra-build208.txt.gz
http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-77304d9ee78c/try-android-armv6/try_tegra_android-armv6_test-crashtest-3-bm22-tests1-tegra-build242.txt.gz

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 671591 [details] [diff] [review]
Use JPEG encoding instead of PNG in getScreenshot()

r=me

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 671591 [details] [diff] [review]
Use JPEG encoding instead of PNG in getScreenshot()

[Approval Request Comment]
b2g-only change.  Or you could mark this bug as blocking.  :)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

This should really have a comment about why JPEG is being used because JPEG is not the obvious choice.

Comment 11

[Justin Lebar (not reading bugmail)]

Too late.  :)  https://hg.mozilla.org/integration/mozilla-inbound/rev/8bef5de35c74

rs=me to add a comment, Jeff.  Or Gabriele can attach another patch for checkin.

Comment 12

[Justin Lebar (not reading bugmail)]

(btw, I expect we have libjpeg-turbo to thank for the fast jpeg encoding here.)

Comment 13

[Gabriele Svelto [:gsvelto]]

Attachment: Comment-only patch for the fix

I've added a patch with a short description of the rationale behind choosing JPEG over PNG

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 671845 [details] [diff] [review]
Comment-only patch for the fix

I think Jeff meant a comment in the code, not a comment in the commit history.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

The comment should probably be something like:
"Hack around the fact that we can't specify opaque png, which requires us to unpremultiply the alpha which is expensive on arm which lacks hardware division."

Comment 16

[Justin Lebar (not reading bugmail)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> The comment should probably be something like:
> "Hack around the fact that we can't specify opaque png, which requires us to
> unpremultiply the alpha which is expensive on arm which lacks hardware
> division."

Do you know that's actually the problem, or is that just a guess?  (I haven't figured out how to open one of these traces; I guess I have to use the built-in profiler?)

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: Comment for the code added in the previous patch

(In reply to Justin Lebar [:jlebar] from comment #16)
> Do you know that's actually the problem, or is that just a guess?  (I
> haven't figured out how to open one of these traces; I guess I have to use
> the built-in profiler?)

Yes, the description given by Jeff is accurate. Originally I had thought of altering the PNG encoding code to fix the issue but in the end this was quicker and probably more effective too as far as memory reduction goes.

I've attached a patch with a slightly revised comment.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 671845 [details] [diff] [review]
Comment-only patch for the fix

(If you were talking about obsoleting /this/ patch, that would be right.  The patch we checked in is the one which shouldn't be obsoleted.)

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 671889 [details] [diff] [review]
Comment for the code added in the previous patch

Sure.

btw, the commit message doesn't have to include the bug summary; something like "[BrowserAPI] Add comment explaining why we use JPEG instead of PNG in getScreenshot()." would be better than "[Browser API] Make getScreenshot() use JPEG instead of PNG, commented code".  I fixed it in this push.

https://hg.mozilla.org/integration/mozilla-inbound/rev/532c0ef6588b

Comment 20

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #19)
> btw, the commit message doesn't have to include the bug summary; something
> like "[BrowserAPI] Add comment explaining why we use JPEG instead of PNG in
> getScreenshot()." would be better than "[Browser API] Make getScreenshot()
> use JPEG instead of PNG, commented code".  I fixed it in this push.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/532c0ef6588b

OK, thank you. I was a bit undecided about having another patch with the exact same comment and wasn't sure about Mozilla's policy regarding commit comments. Just one last question,  approval-mozilla-aurora flag is required for the patch to go in the what-will-become-b2g-v1 branch, correct? I didn't dare set it myself as I'm unsure about the policy for that.

Comment 21

[Justin Lebar (not reading bugmail)]

> Just one last question,  approval-mozilla-aurora flag is required for the patch to go in the 
> what-will-become-b2g-v1 branch, correct? I didn't dare set it myself as I'm unsure about the policy 
> for that.

Either the patch needs approval-aurora, or the bug needs to be basecamp-blocking+.  I personally don't think we should blocking+ bugs that wouldn't actual block shipping (since that messes up other measurements we're doing about the number of blockers and their fix rate), so I nom'ed the patch instead of the bug.  But I can imagine someone else doing it the other way.

I'll probably push both patches to aurora even though I nom'ed only the one.  Maybe that's flying fast and loose, but I don't think anyone will care about pushing a comment-only follow-up to Aurora.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bef5de35c74
https://hg.mozilla.org/mozilla-central/rev/532c0ef6588b

Comment 23

[Robert Kaiser]

Hmm, makes me sad that we need to do this, as usually JPEG artifacts are not what you want in screenshots, esp. as they might distort the point you want to make in some cases, e.g. when you might want to show a subtle color variation that shouldn't be there - esp. because creating a screenshot is a pretty rarely-used and probably non-advertised feature, so it's questionable if a perf win in that is worth it.
I understand why we're doing it given the dimension of the difference, but I still can't help being sad about it...

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to Robert Kaiser (:[e-mail]) from comment #23)
> Hmm, makes me sad that we need to do this, as usually JPEG artifacts are not
> what you want in screenshots, esp. as they might distort the point you want
> to make in some cases, e.g. when you might want to show a subtle color
> variation that shouldn't be there - esp. because creating a screenshot is a
> pretty rarely-used and probably non-advertised feature, so it's questionable
> if a perf win in that is worth it.
> I understand why we're doing it given the dimension of the difference, but I
> still can't help being sad about it...

The feature that shows all opened applications (aka card view) is, I think, using getScreenshot(). Actually getScreenshot() is way more used that you would think of.
It is also used by the browser app to do tabs preview.

Comment 25

[Robert Kaiser]

Ah, right, I totally forgot about us using it all over the place, of course we're also displaying a screenshot while loading an app again and stuff like that. Makes much more sense when thinking about that, of course.

Would be nice if we'd use the faster one for all those implicit "internal" uses and the higher-quality PNG for the ones explicitly generated by a "user" (more likely a dev or tester, as most users probably won't be told about that anyhow). Too much work for v1, though, I guess. But might be worth filing a bug for later.

Comment 26

[Dietrich Ayala (:dietrich)]

Blocks a slim-fast bug (and memshrink p1).

Comment 27

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2e068e438e9
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f623c9fdc52

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to Robert Kaiser (:[e-mail]) from comment #25)
> Would be nice if we'd use the faster one for all those implicit "internal"
> uses and the higher-quality PNG for the ones explicitly generated by a
> "user" (more likely a dev or tester, as most users probably won't be told
> about that anyhow). Too much work for v1, though, I guess. But might be
> worth filing a bug for later.

I do not thing this feature is exposed to users or devs actually.

Comment 29

[Robert Kaiser]

(In reply to Mounir Lamouri (:mounir) from comment #28)
> I do not thing this feature is exposed to users or devs actually.

I meant the Home+Power button mechanism to create screen shots that are saved to the SD card.

Comment 30

[Mounir Lamouri (:mounir)]

I had no idea that was doable and no idea what is used to make it work.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The Home+Power "system screenshot" reads back directly from the hardware framebuffer and *must* use lossless compression because those captures are used to file bugs on graphics artifacts.

That implementation is different from the mozbrowser API "capture", so it won't be affected by this work.

Comment 32

[Robert Kaiser]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> That implementation is different from the mozbrowser API "capture", so it
> won't be affected by this work.

OK, good, I was only concerned about that part - exactly because of that "*must*" you mention in your comment. ;-)

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This patch prevent us from getting a screenshot with alpha channel.

I would like to apply the screenshot technique to home screen, but the white background will block the background image :'(

Comment 34

[Justin Lebar (not reading bugmail)]

> I would like to apply the screenshot technique to home screen, but the white background 
> will block the background image :'(

File a bug and we'll see what we can do.  Maybe we can optimize the part of the PNG encoder that's slow.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

(In reply to Justin Lebar [:jlebar] from comment #34)
> > I would like to apply the screenshot technique to home screen, but the white background 
> > will block the background image :'(
> 
> File a bug and we'll see what we can do.  Maybe we can optimize the part of
> the PNG encoder that's slow.

If we can get the PNG encoder to know that the data we're compressing doesn't have an alpha channel when it doesn't we won't get killed by the divisions.

Comment 36

[Justin Lebar (not reading bugmail)]

Yes, although that doesn't help Tim, who explicitly wants alpha.

I'm happy to let callers specify a MIME type here, but it's not clear the performance is acceptable with PNG.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

(In reply to Justin Lebar [:jlebar] from comment #36)
> Yes, although that doesn't help Tim, who explicitly wants alpha.

If only doing the division when needed still isn't acceptable we could also use the lookup table approach that we use for getImageData().

Comment 38

[Justin Lebar (not reading bugmail)]

Yes, absolutely.  We'd need to benchmark on the particular device, of course.

Comment 39

[Gabriele Svelto [:gsvelto]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #37)
> If only doing the division when needed still isn't acceptable we could also
> use the lookup table approach that we use for getImageData().

When I first started tackling this bug I tried optimizing the PNG encoding code and it is possible but we would need to be careful about the performance impact. The situation is currently the following: we get an ARGB image as input and the alpha-channel is always set, though it's usually always 0xff (i.e. opaque in this case), since we have no way of ignoring it we always take the slow path. The conversion involves dividing every color-channel by the alpha-channel so three divisions per pixel which kills us. To make it faster I would take these three steps:

- Create a path where the alpha-channel is ignored for fully opaque clients and a way to specify it (possibly as an option of toDataURL?). We would also need a way to let the Gaia apps to specify if they have an alpha-channel or not when a screenshot of them is taken).

- Improve the alpha-channel conversion code by taking a fixed-point reciprocal of the alpha-channel and then use a fixed-point multiplication to divide each color channel. This turns three full integer divisions into one integer division (of a constant) and three multiplies, a pretty big win. We might even try floating-point code here, it could be faster for what we know.

- If we're still using integer math then as a final step we could replace the slow integer division replacement provided by GCC with an ARM-optimized version, there's plenty including in more recent versions of GCC. I would do this as a very last step though.

Comment 40

[Justin Lebar (not reading bugmail)]

Can we move this into a separate bug?  Before we spend a lot of time thinking about this, we should evaluate how important Tim's use case from comment 33 is, whether we have more important things to be doing right now, and so on.