970618 - Screenshots taken from CLI are scaled on Retina displays

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) On a Mac with a Retina display, open the devtools CLI
2) Type:

screenshot scaled.png

and press Enter

3) Find the generated screenshot named scaled.png, and open it


ER: The screenshot should match the contents and resolution of whatever was in the browser content frame at the time the command was fired.

AR: While the contents are correct, the resolution is not - the image generated by the screenshot command appears to be scaled down.

Comment 1

[Steven Garrity [:sgarrity]]

This is also true of the screenshot function that is exposed in the UI in the dev tools now.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This uses canvas (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js#100), which it seems Firefox doesn't set a value for the backing device pixel ratio. Otherwise I think this might *just work*.

But since it doesn't, you can probably get the window from the document, get the devicePixelRatio (presumably ensures the right value for the monitor the provided window is actually on, which may or may not be HiDPI). Then you can just scale the canvas to match.

http://www.html5rocks.com/en/tutorials/canvas/hidpi/ has some code which should be a good guide. The demo page looks right in Nightly so presumably it should be pretty simple.

Comment 4

[M Sikma]

As of today, this is still not working. I tried it on the recently released FirefoxDeveloperEdition (35.0a2).

Retina displays are still quite rare on desktop/laptops, but since many mobile devices now have retina pixel density, this is gradually becoming more important.

Comment 5

[Johan K. Jensen :johankj]

Attachment: bug970618_screenshot_retina__johankj.patch

This patch should scale screenshots so the result is crispier on a retina-device.
It only scales the canvas accordingly to the devicePixelRatio.

Screenshots taking by Mac OS X will save DPI-information in the pHYs-section of the png (see the png specification) with values 0x00001625 meters per unit corresponding to 144 DPI.
The resulting screenshot in Firefox (when saved to a png) doesn’t contain information regarding DPI/PPI, as I couldn’t find a way to save this information via the saveURL/internalSave-functions.

As most image viewers will default to 72 DPI, the resulting screenshot will look larger unless you change the DPI in the image viewer or view it at e.g. 50%.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8630466 [details] [diff] [review]
bug970618_screenshot_retina__johankj.patch

Needs review, I suppose I can check this out.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8630466 [details] [diff] [review]
bug970618_screenshot_retina__johankj.patch

Review of attachment 8630466 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this seems great, thanks for working on it!

Just a small tweak I'd like to see.  You can flag me for review on the updated patch.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +287,3 @@
>    const ctx = canvas.getContext("2d");
> +  const devicePixelRatio = window.devicePixelRatio || 1;
> +  const backingStoreRatio = ctx.backingStorePixelRatio || ctx.mozBackingStorePixelRatio || 1;

I can't find any evidence that Gecko intends to implement backingStorePixelRatio, so I think we can just skip this step.

So, effectively:

const ratio = window.devicePixelRatio;

|devicePixelRatio| should always be available, so no need to || 1.

Comment 8

[Johan K. Jensen :johankj]

Attachment: bug970618_screenshot_retina_2__johankj.patch

Thanks, jryans, for the review.
I was unsure about those changes, but found some references to the backing store on the web. On further inspection it doesn’t seem to be moving forward.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8631520 [details] [diff] [review]
bug970618_screenshot_retina_2__johankj.patch

Great, this looks good to me!

I've pushed this to Try, our testing environment:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=499bec5a3b3f

Assuming tests pass, we can mark this bug for landing by setting the keyword "checkin-needed" on the bug.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hooray, the tests passed!  A sheriff will land this shortly.  Thanks again!

Feel free to look around for other DevTools bugs you'd like to work on.  The new "Find DevTools bugs" site[1] is a good way to find easy and mentored bugs you may want to try.

[1]: http://firefox-dev.tools/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f25ab8909c71

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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