Closed Bug 800459 Opened 12 years ago Closed 12 years ago

Gaia work for bug 800170 - Pass max-width, max-height parameters to getScreenshot()

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set
normal

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: justin.lebar+bug, Assigned: timdream)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(1 file, 1 obsolete file)

As part of bug 800170, you'll now need to pass two parameters to the browser-api's getScreenshot: max-width and max-height.

The screenshot which is returned has the following properties:

 - It is no wider than max-width and no taller than max-height
 - It is no wider than the screenshotted window's innerWidth and no taller than the window's innerHeight.
 - If the image is shrunk, it is not deformed -- the width and height are scaled down by the same factor.
 - The resultant image doesn't have any added blank space.  That is, if you specify max-width=100, max-height=100 and the screenshotted window's inner dimensions are (50, 100), the resultant screenshot will have dimensions (50, 100).

I don't actually know how large you want these screenshots to be, but it appears that for the app-loading-animation-screenshot, you /tile/ the app's screenshot, which means the screenshot must either be equal to the screen size, or you need to stop tiling.

I'll attach a patch which touches all the relevant getScreenshot() calls and sets them to (200, 200).  This isn't quite right, but it at least gives you the right idea.
I /think/ you can land these changes before bug 800170; if you pass extra arguments to getScreenshot as-is, I think they'll be ignored.
Blocks: 800170
Justin, I've always thought getScreenshot() simply returns the screenshot of the current viewpoint, which is exactly what I needed in the System app (cards view, window opening sprite). I wonder what's the reason to introduce max-height/max-width to the function?
> I've always thought getScreenshot() simply returns the screenshot of the current viewpoint,

It returns the screenshot of the current /viewport/, I believe; the current width/height is window.innerWidth/innerHeight.

For apps, that may be the same as what's currently on-screen.  But for browsers, it's not the same thing; the viewport might be much larger than the screen.  That was causing the browser's screenshots to take up hundreds of KB.

If you want unchanged behavior, you can pass a large max-width and max-height.  I made the args required because I don't want us to accidentally ask for large screenshots -- if you really want a big screenshot, you need to write code which explicitly asks for it.
Blocks a blocker.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Comment on attachment 670466 [details] [diff] [review]
Bogus patch (set max-height = max-width = 200).

Justin, what made you pick 200x200px as the size? This seems too large for the browser app and too small for the window manager.

I'd prefer a pull request :) Or just explain the changes (just looks like two extra arguments need adding) and Tim and I can implement the Gaia side.
> Justin, what made you pick 200x200px as the size? This seems too large for the browser 
> app and too small for the window manager.

Indeed, that's the "bogus" part of the patch, per comment 0.

> Or just explain the changes

Do you have a specific question I can answer here?
I don't think I understand what these arguments specify.

Does it specify the x and y co-ordinates across the viewport that will be captured, or will it capture everything and scale it down to that size?
(In reply to Justin Lebar [:jlebar] from comment #0)
> I don't actually know how large you want these screenshots to be, but it
> appears that for the app-loading-animation-screenshot, you /tile/ the app's
> screenshot

Oh, that explains some weird behaviour I've seen. We probably shouldn't do that.
(In reply to Ben Francis [:benfrancis] from comment #8)
> I don't think I understand what these arguments specify.
> 
> Does it specify the x and y co-ordinates across the viewport that will be
> captured, or will it capture everything and scale it down to that size?

It captures everything in the viewport and then scales it down according to the rules in comment 0.
(In reply to Justin Lebar [:jlebar] from comment #7)
> Indeed, that's the "bogus" part of the patch, per comment 0.

(oops, I didn't read your comment :P)
(In reply to Justin Lebar [:jlebar] from comment #10)
> It captures everything in the viewport and then scales it down according to
> the rules in comment 0.

Hmm, so if I wanted enough pixels to fill a 140x100px thumbnail and called getScreenshot(140, 100) but the viewport was set to be very hight but not very wide, I could end up with just a slither which was a lot less than 140pixels wide.

Do you know if the viewport size is effected by the orientation of the device?
(In reply to Ben Francis [:benfrancis] from comment #12)
> Hmm, so if I wanted enough pixels to fill a 140x100px thumbnail and called
> getScreenshot(140, 100) but the viewport was set to be very hight but not
> very wide, I could end up with just a slither which was a lot less than
> 140pixels wide.

True!

I don't know how often such pathological cases appear in practice; we could try this method as-is and tweak it if it's problematic.

Or if you know exactly how you want this method to work, I can change it now.  :)

> Do you know if the viewport size is effected by the orientation of the
> device?

I'm not an expert on this, but if it's true that apps sometimes (usually?  always?) have screen-size == viewport, then yes.
(In reply to Justin Lebar [:jlebar] from comment #13)
> Or if you know exactly how you want this method to work, I can change it
> now.  :)

How feasible do you think it would be to specify the exact width and height of the desired image, then have the width of the image (which includes the full width of the viewport) scaled down to the required width, with the height scaled proportionately? If there isn't enough height to fill the box then pad with transparent pixels.
What would be even better would be if both width and height were optional. By default you get the full size image. If you specify width you get exactly that width with height scaled proportionately. If you specify height you get exactly that height with width scaled proportionately. If you specify both, you use the behaviour described in the last comment.
> How feasible do you think it would be to specify the exact width and height of the desired image, 
> then have the width of the image (which includes the full width of the viewport) scaled down to the 
> required width, with the height scaled proportionately?

I could do that.  And if the viewport isn't wide enough, we basically apply the current algorithm?

Maybe we take the current API and add a third parameter, which indicates whether we

  * never crop the viewport (current behavior with patch)
  * crop height to fill width (what you're asking for)
  * crop width to fill height
  * crop both (that is, never scale the image down)

> By default you get the full size image.

I explicitly do not want any way for us to get accidental run-away memory usage with this API, so which is why I made the width and height to be mandatory.  See the end of comment 4.  If you want the effect of an "optional" width/height, pass a very large integer in for the width/height; then it's obvious that you're asking for trouble.
(In reply to Justin Lebar [:jlebar] from comment #16)
> I could do that.  And if the viewport isn't wide enough, we basically apply
> the current algorithm?

Erm, yes. In as much as you never want to pad both width and height.

> Maybe we take the current API and add a third parameter, which indicates
> whether we
> 
>   * never crop the viewport (current behavior with patch)
>   * crop height to fill width (what you're asking for)
>   * crop width to fill height
>   * crop both (that is, never scale the image down)

That sounds over-complex.

> > By default you get the full size image.
> 
> I explicitly do not want any way for us to get accidental run-away memory
> usage with this API, so which is why I made the width and height to be
> mandatory.  See the end of comment 4.  If you want the effect of an
> "optional" width/height, pass a very large integer in for the width/height;
> then it's obvious that you're asking for trouble.

I understand your intentions, but I think if a developer wants to get a screenshot of the full viewport, they should be able to do that, without picking arbitrary large numbers.

Just to be explicit, this would be my proposal...

only width specified:
* Scale viewport width up or down to requested width and scale height proportionately.

only height specified:
* Scale viewport height up or down to requested height and scale width proportionately.

both width and height specified:
* Scale viewport width up or down to requested width and scale height proportionately, padding or cropping as necessary.

neither specified:
* provide screenshot of full viewport.
> I understand your intentions, but I think if a developer wants to get a screenshot of the full 
> viewport, they should be able to do that, without picking arbitrary large numbers.

The question isn't whether developers should be able to get a screenshot of the full viewport.  With my current patch, you always get a screenshot of the full viewport, full stop.

The question is whether developers should be able to get a /full-resolution/ screenshot of the full viewport.  That is explicitly harmful, as we've seen.  Making that easy is an API footgun that I want to avoid.

In the scheme proposed in comment 17, it is very easy to get a full- or high-resolution screenshot of a page.  In B2G, it would /never be correct/ to leave off the height or width arguments unless we had full control over the screenshotted page's viewport, because doing so would allow us to take an arbitrary-resolution screenshot.
I don't think the current patch allows it, but we could definitely allow passing of Infinity in as a height/width parameter, so you don't have to pick an arbitrarily-large integer, which I agree is dubious.  Using Infinity would make it explicit that you're asking for trouble.
We absolutely need to make clients of this API beg and plead to get full-resolution captures.  We allow <meta viewport> to override the CSS viewport up to 10,000 x 10,000 CSS pixels.  Please don't try to capture at that resolution! :)
Ben, this is currently blocking our number-one b2g memshrink bug.

Can we land this simple Gaia fix and see how it works?  I'll be happy to tweak the API as a follow-up (or ask someone else to do so :), but we really need to measure memory usage with bug 800170 fixed.
The dimensions specified in the patch definitely need to change. I would guess that for the browser we'd need 140 width and 210 height (although infinite height would be the only way to guarantee always filling 140 pixel thumbnail widths). For the system app I would guess 320 width and 480 height, although I'm not convinced this will have the desired result in all cases and it's probably not actually going to save any memory in the system app if that's the viewport size anyway.

You've persuaded me that returning a full resolution screenshot by default is probably a bad idea, but I don't think the current getScreenshot(max_width, max_height) approach is going to fulfil our requirements :(

If you change the dimensions as above I can r+me the patch for now (as it won't actually have any effect), but I am concerned it might cause visual regressions if the API change lands as currently specified, certainly for some edge cases. I would prefer to see it working before that happens.
> The dimensions specified in the patch definitely need to change.

Yes, of course.  When I say "land this change", I mean, fix the args and land that.  Sorry that was unclear.

I expect you don't actually want me to hardcode in these width/heights.  That's why I was hoping a Gaia person could pick up this patch and do it correctly.  There's also the question of what the card view's dimensions should be.
This patch isn't quite right, because

  a) It hardcodes the height/width in pixels, and
  b) It uses 320x480px screenshots for the card view, which is probably wrong.

It's also not quite right because benfrancis would prefer that the API always ensure that we fill the width of the screenshot.  (Right now, a sufficiently tall and skinny viewport will cause us not to fill the screenshot's width.)  We're going to tackle that in a separate bug.
Attachment #670466 - Attachment is obsolete: true
Justin, just to be sure, here is my understanding of the parameters:

The returned image will always be the exact dimensions given by the parameters; if the viewport is bigger than the specified dimensions the resulting image will be a scaled-down screenshot "thumbnail". We will save some memory by getting this smaller image from the Gecko directly, instead of scaling the bigger image with CSS or canvas.

If the above is true,

-- For window opening/closing transitions, we probably can't save any memory since the image we need is of the exact size of the iframe.
-- For cards view, the patch needed will be more than the parameters -- we would need to update the cards view CSS altogether to avoid enlarge the image with background-size and then scale the div down with transform. Right?
> We will save some memory by getting this smaller image from the Gecko directly, instead of scaling 
> the bigger image with CSS or canvas.

Lots of memory, in fact, since the main bug here is that we don't GC these big strings aggressively enough.  Gaia appears to stop holding onto these strings pretty quickly, which is good, but unfortunately not enough.

> -- For window opening/closing transitions, we probably can't save any memory since the image we need 
> is of the exact size of the iframe.

Correct.  The main culprit in my testing is the browser app, which was taking ~1MP screenshots at nytimes.com.

> -- For cards view, the patch needed will be more than the parameters -- we would need to update 
> the cards view CSS altogether to avoid enlarge the image with background-size and then scale the 
> div down with transform. Right?

I don't know what you mean, partially because I have no idea how the cards view CSS works.  Could you rephrase this?

It's not clear to me why you need to take separate screenshots for the cards view and the system app (the fewer screenshots we take, the better, even if we don't /store/ duplicate screenshots), but that's bug 801306, so we can discuss that there, if you like.
(In reply to Justin Lebar [:jlebar] from comment #26)
> > -- For cards view, the patch needed will be more than the parameters -- we would need to update 
> > the cards view CSS altogether to avoid enlarge the image with background-size and then scale the 
> > div down with transform. Right?
> 
> I don't know what you mean, partially because I have no idea how the cards
> view CSS works.  Could you rephrase this?

Currently, Cards view

1. creates |<div class="card">| with the same dimension of the app frame.
2. Call getScreenshot() and assign the data URL as the background image of the div.
3. |background-size: contain| is there to ensure the user can see the whole app in the contained box.
4. |transform: scale(0.7)| is there so card is a card, not something as big as the app frame.

Now, if the fix here is, simply

|getScreenshot(appFrameWidth * 0.7, appFrameHeight * 0.7)|

The background image will be scaled up by rule #3 and scaled down by rule #4. Unless Gecko is smart enough about it, the fix will means more CPU and GPU right?

> 
> It's not clear to me why you need to take separate screenshots for the cards
> view and the system app (the fewer screenshots we take, the better, even if
> we don't /store/ duplicate screenshots), but that's bug 801306, so we can
> discuss that there, if you like.
> The background image will be scaled up by rule #3 and scaled down by rule #4. Unless Gecko is smart 
> enough about it, the fix will means more CPU and GPU right?

I have no idea whether Gecko is smart enough to do this correctly.

Would you mind writing a patch to do this correctly, so we can check it in?  Like I've said, it's hard to continue measuring B2G memory usage while this bug remains unfixed.  We don't have a lot of time to dally here...
(In reply to Justin Lebar [:jlebar] from comment #28)
> > The background image will be scaled up by rule #3 and scaled down by rule #4. Unless Gecko is smart 
> > enough about it, the fix will means more CPU and GPU right?
> 
> I have no idea whether Gecko is smart enough to do this correctly.
> 
> Would you mind writing a patch to do this correctly, so we can check it in? 
> Like I've said, it's hard to continue measuring B2G memory usage while this
> bug remains unfixed.  We don't have a lot of time to dally here...

I'll write a patch do 

|getScreenshot(appFrameWidth * 0.7, appFrameHeight * 0.7)|

tomorrow.
Assignee: nobody → timdream+bugs
I've sent a pull request for the browser part of this https://github.com/mozilla-b2g/gaia/pull/5824
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Not yet resolved, still waiting for https://github.com/mozilla-b2g/gaia/pull/5824 to be reviewed & merged.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: