Closed Bug 800170 Opened 8 years ago Closed 8 years ago
API] Modify get Screenshot() so it accepts max-width, max-height
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.
Assignee: nobody → justin.lebar+bug
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.
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.
(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.
(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.
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.
> 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.
(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.
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)...
(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.
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.
Summary: [BrowserAPI] Modify getScreenshot() so it uses less memory → [BrowserAPI] Modify getScreenshot() so it accepts max-width, max-height
Implementing canvas.toBlob() would probably be easy.
(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.
> 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?
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.
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.
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 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.
Attachment #671231 - Flags: feedback?(ben)
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.
Attachment #671231 - Flags: feedback?(ben) → feedback+
(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.
Attachment #671231 - Flags: review?(bugs)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6c7006403e I can land on Aurora once we land bug 801676 there.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.