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

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 2 bugs)

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

5 years ago
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.
Whiteboard: [MemShrink]
(Assignee)

Comment 2

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

Comment 6

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

Comment 8

5 years ago
Created attachment 670453 [details] [diff] [review]
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)...
(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.
(Assignee)

Comment 10

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

Updated

5 years ago
Summary: [BrowserAPI] Modify getScreenshot() so it uses less memory → [BrowserAPI] Modify getScreenshot() so it accepts max-width, max-height
(Assignee)

Updated

5 years ago
Depends on: 800459
(Assignee)

Comment 11

5 years ago
Created attachment 670471 [details] [diff] [review]
Patch v1.1
Attachment #670453 - Attachment is obsolete: true
Attachment #670471 - Flags: review?(bugs)

Updated

5 years ago
Attachment #670471 - Flags: review?(bugs) → review+
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?

Updated

5 years ago
blocking-basecamp: ? → +
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.
(Assignee)

Comment 14

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

Comment 16

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

Comment 17

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

Comment 18

5 years ago
Created attachment 671231 [details] [diff] [review]
Part 1a, v1: Tweak how we scale and crop screenshots.
(Assignee)

Comment 19

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

Comment 21

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

Updated

5 years ago
Attachment #671231 - Flags: review?(bugs)

Updated

5 years ago
Attachment #671231 - Flags: review?(bugs) → review+
Whiteboard: [MemShrink] → [MemShrink:P1]
(Assignee)

Comment 22

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

I can land on Aurora once we land bug 801676 there.
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/e11125917b82
(Assignee)

Updated

5 years ago
status-firefox18: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/ab6c7006403e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.