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

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

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.

(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 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/gsvelto@mozilla.com-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/gsvelto@mozilla.com-77304d9ee78c/try-android-armv6/try_tegra_android-armv6_test-crashtest-3-bm22-tests1-tegra-build242.txt.gz

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

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. :)

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

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.

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

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

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.

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."

(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?)

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

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

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

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

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

(In reply to Robert Kaiser (:kairo@mozilla.com) 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.

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.

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

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

(In reply to Robert Kaiser (:kairo@mozilla.com) 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.

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

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

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.

(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. ;-)

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 :'(

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

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

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.

(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().

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

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

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.