800170 - [BrowserAPI] Modify getScreenshot() so it accepts max-width, max-height

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Lebar (not reading bugmail)]

In bug 798002, we discovered that getScreenshot() uses a lot of memory.

The problem is that the browser compartment seems to hang on to the screenshot data:URI until a full GC, and that GC doesn't seem to happen...basically ever.

There are a couple of changes I want to try making here:

a) Send the screenshot as binary data instead as a base-64-encoded string.

In a base-64 representation, we get 6 bits per char (2^6 = 64).  Each char in JS takes up 16 bits (because JS strings are UCS2).  So base-64 is 16/6 = 2.67 times larger than the binary representation.

b) Use lossy compression on the screenshot.  Either PNG or JPEG; I'm not sure which will be better yet.

c) Reduce the resolution of the screenshots.  Right now, we capture a 980x1118 screenshot of nytimes.com.  I'm not sure how we decide upon that resolution, but it's enormous!  We can fix this by specifying (max-width, max-height) to getScreenshot(), and having the platform size the screenshot so it fits within those given dimensions.

Comment 1

[Justin Lebar (not reading bugmail)]

It unfortunately doesn't look like we can get the contents of a canvas as a compressed binary string -- it either must be an uncompressed bit string or a compressed base64-encoded-string.

Let's see whether simply reducing the resolution solves our problem.  If not, I'll work on getting a compressed byte stream out of the canvas.

Comment 2

[Justin Lebar (not reading bugmail)]

FWIW it also seems that somewhere/somehow we're making multiple copies of this string.  I only see the browser taking one screenshot, but we saw something like 5 copies of it in the strings-dump.

Comment 3

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

(In reply to Justin Lebar [:jlebar] from comment #1)
> It unfortunately doesn't look like we can get the contents of a canvas as a
> compressed binary string -- it either must be an uncompressed bit string or
> a compressed base64-encoded-string.
> 

Is there an internal XPCOM interface that lets us get at that?  If not, we can certainly add it.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #0)
> b) Use lossy compression on the screenshot.  Either PNG or JPEG; I'm not
> sure which will be better yet.

I think for that kind of images, you will have smaller JPEG files than PNG files.

Comment 5

[Ben Francis [:benfrancis]]

Please file bugs against the Gaia::Browser component of Boot2Gecko if any API changes require changes in Gaia's browser app (e.g. new maximum dimension arguments or a different return type) and we'll try to synchronise our changes so we don't cause any regressions. Thanks.

Comment 6

[Justin Lebar (not reading bugmail)]

> Is there an internal XPCOM interface that lets us get at that?

Doesn't look like it.  All the canvas stuff is using webIDL, which means it's easy to see the full chrome + web interface.

> If not, we can certainly add it.

Indeed, and we may want to.  But note that once the child gets the image-blob, it eventually needs to display it.  At the moment, the only way we can do /that/ is to convert it to base-64!

If we are in fact copying the string passed into the mm many times (comment 2), then waiting until the eleventh hour to do the base-64 conversion may still be a win.  But I'm a bit hopeful that reducing the resolution plus perhaps JPEG encoding will make this small enough to ignore.

Comment 7

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

(In reply to Justin Lebar [:jlebar] from comment #6)
> Indeed, and we may want to.  But note that once the child gets the
> image-blob, it eventually needs to display it.  At the moment, the only way
> we can do /that/ is to convert it to base-64!

Nope!  We can make a Blob URI and set that on the <img>.  Confirmed with bent yesterday.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1 - Make getScreenshot() take args specifying the max height/width of the resultant image.

This, combined with a patch which causes Gaia to take max-200x200 screenshots, brings our string-chars memory usage down to .70mb in the browser compartment.

Still need to try JPEG and blob URIs (per comment 7, which is cool)...

Comment 9

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

(In reply to Justin Lebar [:jlebar] from comment #8)
> Created attachment 670453 [details] [diff] [review]
> Still need to try JPEG and blob URIs (per comment 7, which is cool)...

I don't think JPEGs are worth pursuing here, TBH.  Lossless screenshots are more valuable if we can get them to meet our perf metrics.

Comment 10

[Justin Lebar (not reading bugmail)]

JPEG doesn't seem to be much of a win at this resolution, afaict.

Blob URIs is going to be more complicated, since we have to modify canvas and whatnot.  So I think we should take this as-is and worry about blobs in a follow-up, if they're necessary.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.1

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Implementing canvas.toBlob() would probably be easy.

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> I don't think JPEGs are worth pursuing here, TBH.  Lossless screenshots are
> more valuable if we can get them to meet our perf metrics.

I did some further performance testing on the Otoro device to gather some repeatable profile data. The test involved launching the gallery application, going back to the homescreen and then invoking the task-switcher, this caused exactly 4 screenshots to be taken every time. I've tested by tweaking _recvGetScreenshot to use both PNG and JPEG and to do full resolution and 200x200 screenshots. This is a summary of the results, with the time spent on a single screenshot under every setup:

PNG,  full resolution   820ms
PNG,  200x200           240ms
JPEG, full resolution   150ms
JPEG, 200x200            65ms

As you can see using JPEG saves quite a bit of CPU time on ARM-based devices so I think we should consider using it.

Comment 14

[Justin Lebar (not reading bugmail)]

> As you can see using JPEG saves quite a bit of CPU time on ARM-based devices so I think we should 
> consider using it.

Fantastic!  (I was measuring only memory usage.)

Can you file a separate bug, please?

Comment 15

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

Thanks for measuring!  Very interesting.

Since we have the ability to take a full-resolution lossless capture of the display framebuffer, I guess modifying the mozbrowser interface to return lossy images is probably OK.  The only clients so far are thumbnails, which aren't too sensitive to compression artifacts.

Even better, we could make this a parameter for the screenshot API.

Comment 16

[Justin Lebar (not reading bugmail)]

Ben Francis has convinced me in bug 800459 that what would be useful for the Gaia folks is slightly different resizing logic.

Right now the screenshot is always of the full viewport (perhaps scaled down).  So for example if the viewport is 100x1000 and we ask for a 100x100 screenshot, the resultant image will be 10x100.

Ben would like instead for us to always fill the width and crop the height if necessary.  So in the case above, the resultant screenshot would be 100x100, showing an unscaled version of the top 100 pixels of the viewport.

I think ideally, the API would let you choose what cropping behavior you want.  But for B2G v1, it seems that nobody wants the current cropping behavior, and everyone wants the behavior Ben suggested.  So I'm going to change this patch to match what he wants, and we can later worry about whether we want to add parameters to this API which let you select a cropping behavior.

Comment 17

[Justin Lebar (not reading bugmail)]

Okay, new algorithm.  This is equivalent to what Ben asked for in the case
that the viewport is much taller than it is wide.  But it also doesn't hit
pathological cases when the viewport is much wider than it is tall.

To run getScreenshot(max-width, max-height), do the following:

- Let scale-height be the factor by which we'd need to downscale the viewport
  so it would fit within max-height.  (If the viewport's height is less than
  max-height, let scale-height be 1.) Compute scale-width the same way.

- Scale the viewport by max(scale-height, scale-width).  Now either the
  viewport's height is no larger than max-height, the viewport's width is
  no larger than max-width, or both.

- Crop the viewport so its height is no larger than max-height and its
  width is no larger than max-width.

- Return a screenshot of the page's viewport cropped and scaled per
  above.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Part 1a, v1: Tweak how we scale and crop screenshots.

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 671231 [details] [diff] [review]
Part 1a, v1: Tweak how we scale and crop screenshots.

Ben, are you happy with this?  See the explanation of this algorithm in this bug.

Comment 20

[Ben Francis [:benfrancis]]

If I understand this correctly, the only circumstances under which you'd get an image which was not of the exact width and height specified would be when the viewport width is smaller than the requested width or the viewport height is smaller than the viewport height.

If this is correct then I think this sounds great.

Comment 21

[Justin Lebar (not reading bugmail)]

(In reply to Ben Francis [:benfrancis] from comment #20)
> If I understand this correctly, the only circumstances under which you'd get
> an image which was not of the exact width and height specified would be when
> the viewport width is smaller than the requested width or the viewport
> height is smaller than the viewport height.
> 
> If this is correct then I think this sounds great.

Yes, correct.

Comment 22

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6c7006403e

I can land on Aurora once we land bug 801676 there.

Comment 23

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e11125917b82

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ab6c7006403e