946596 - Hi-res phone does not take hi-res screenshots

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. Take a screenshot on a hi-res phone like Peak (by pressing Home and Power)
2. Inspect the screenshot taken in Gallery app

Expected:

1. The screenshot is pixel sharp, with pixel dimension equal to device pixel dimension

Actual:
1. The screenshot taken not pixel sharp, and it's pixel dimension dimension is equal to css screen dimension, w/o taken device pixel ratio into account.


The code lives there:
https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1110
it's not a good first bug but probably a good mentor bug.

Wayne, is this a product requirement for Helix device? If so we need this bug hd+

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I quickly tried

diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
index 1ed21d5..b678b59 100644
--- a/b2g/chrome/content/shell.js
+++ b/b2g/chrome/content/shell.js
@@ -1121,8 +1121,8 @@ window.addEventListener('ContentStart', function ss_onCont
     try {
       var canvas = document.createElementNS('http://www.w3.org/1999/xhtml',
                                             'canvas');
-      var width = window.innerWidth;
-      var height = window.innerHeight;
+      var width = window.innerWidth * window.devicePixelRatio;
+      var height = window.innerHeight * window.devicePixelRatio;
       canvas.setAttribute('width', width);
       canvas.setAttribute('height', height);

But drawWindow() does not actually draw the content window with specified size. This is in consistent with what described here:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D#drawWindow%28%29

The actual function locates here:
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#3310

Comment 2

[Wayne Chang [:wchang]]

After discussing with bhuang this shouldn't block the HD release/helix, however nom'ing for v1.3 to complete the hi-res support

Comment 3

[Preeti Raghunath(:Preeti)]

Blocking for completing hi-res support in 1.3.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We'll need to patch drawWindow() for this, maybe add a flag for drawing device pixels instead of CSS pixels.

CJ, is there any good candidate in Taipei could do it?

Comment 5

[u459114]

Peter, Canvas::drawWindow

Comment 6

[Peter Chang[:pchang]]

Checking the behavior on Nexus 4

Comment 7

[Peter Chang[:pchang]]

Taking snapshot is also related to GetImageBuffer func.
Need to hack the size in GetImageBuffer too.

http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#1059

Comment 8

[Peter Chang[:pchang]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #1)
> I quickly tried
> 
> diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
> index 1ed21d5..b678b59 100644
> --- a/b2g/chrome/content/shell.js
> +++ b/b2g/chrome/content/shell.js
> @@ -1121,8 +1121,8 @@ window.addEventListener('ContentStart', function
> ss_onCont
>      try {
>        var canvas = document.createElementNS('http://www.w3.org/1999/xhtml',
>                                              'canvas');
> -      var width = window.innerWidth;
> -      var height = window.innerHeight;
> +      var width = window.innerWidth * window.devicePixelRatio;
> +      var height = window.innerHeight * window.devicePixelRatio;
>        canvas.setAttribute('width', width);
>        canvas.setAttribute('height', height);
> 
> But drawWindow() does not actually draw the content window with specified
> size. This is in consistent with what described here:
> https://developer.mozilla.org/en-US/docs/Web/API/
> CanvasRenderingContext2D#drawWindow%28%29
> 
> The actual function locates here:
> http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> CanvasRenderingContext2D.cpp#3310

With above hack, it also needs to change the following "scale" as 1 to save 1280*720 snapshot in Nexus 4.

http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4554

Comment 9

[Peter Chang[:pchang]]

Attachment: patch v1

Comment 10

[Peter Chang[:pchang]]

Comment on attachment 8349631 [details] [diff] [review]
patch v1

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

Consider the device ratio when generate the device snapshot

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8349631 [details] [diff] [review]
patch v1

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

r- not because the code is bad but because I'm not certain that this is the right way to do it, and because I'd like you to replace the deprecated mozGetAsFile() at the same time.

::: b2g/chrome/content/shell.js
@@ +1132,5 @@
>        var flags =
>          context.DRAWWINDOW_DRAW_CARET |
>          context.DRAWWINDOW_DRAW_VIEW |
>          context.DRAWWINDOW_USE_WIDGET_LAYERS;
> +      context.scale(scale, scale);

I can see that this will produce a full-size screenshot. But does it actually give a pefectly sharp screenshot, or is it fuzzy because it was just scaled up?

While you're fixing this, please also change the deprecated mozGetAsFile() call to use the supported toBlob() method.  (We're getting deprecation warnings in logcat about mozGetAsFile()).

Also, please try taking out the scale adjustments about and just using toBlobHD().

I don't really understand how canvas works when devicePixelRatio !== 1, but I belive that toBlobHD() is intended for this case, and I think it might be possible that you can just use toBlobHD() and not need any of the multiplication by devicePixelRatio. If that works, it seems like a better solution.

Also, feel free to find a better reviewer than me for this. If you can find someone who actually understands how this stuff works, and someone who builds gecko frequently, they would be a better reviewer than me.  But if not, I can review this patch again.

Is this the right way to fix it, or could you just change the toBlob() call to

Comment 12

[Peter Chang[:pchang]]

(In reply to David Flanagan [:djf] from comment #11)
> Comment on attachment 8349631 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8349631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- not because the code is bad but because I'm not certain that this is the
> right way to do it, and because I'd like you to replace the deprecated
> mozGetAsFile() at the same time.
> 
> ::: b2g/chrome/content/shell.js
> @@ +1132,5 @@
> >        var flags =
> >          context.DRAWWINDOW_DRAW_CARET |
> >          context.DRAWWINDOW_DRAW_VIEW |
> >          context.DRAWWINDOW_USE_WIDGET_LAYERS;
> > +      context.scale(scale, scale);
> 
> I can see that this will produce a full-size screenshot. But does it
> actually give a pefectly sharp screenshot, or is it fuzzy because it was
> just scaled up?
> 
I could get the sharp screenshot with my patch.

> While you're fixing this, please also change the deprecated mozGetAsFile()
> call to use the supported toBlob() method.  (We're getting deprecation
> warnings in logcat about mozGetAsFile()).
> 
Sure, I will look into it.

> Also, please try taking out the scale adjustments about and just using
> toBlobHD().
> 
I'm curious that what is the canvas size we need to set if using toBlobHD()?
If we are still using canvas size without considering devicePixelRatio, then the source generated by drawwindow won't be high quality.

> I don't really understand how canvas works when devicePixelRatio !== 1, but
> I belive that toBlobHD() is intended for this case, and I think it might be
> possible that you can just use toBlobHD() and not need any of the
> multiplication by devicePixelRatio. If that works, it seems like a better
> solution.
> 
For canvas scale call, it will scale the draw region for drawwindow().
Since we got the high resolution screen, then the scaled draw region just cover the whole screen for snapshot.

> Also, feel free to find a better reviewer than me for this. If you can find
> someone who actually understands how this stuff works, and someone who
> builds gecko frequently, they would be a better reviewer than me.  But if
> not, I can review this patch again.
> 
> Is this the right way to fix it, or could you just change the toBlob() call
> to

Comment 13

[Peter Chang[:pchang]]

I just checked the toBlobHD and it looks like not supported yet.
https://developer.mozilla.org/en/docs/Web/API/HTMLCanvasElement

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I think we should accept this patch for the sake of 1.3+

(In reply to peter chang[:pchang] from comment #12)
> (In reply to David Flanagan [:djf] from comment #11)
> > Comment on attachment 8349631 [details] [diff] [review]
> > patch v1
> > 
> > Review of attachment 8349631 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- not because the code is bad but because I'm not certain that this is the
> > right way to do it, and because I'd like you to replace the deprecated
> > mozGetAsFile() at the same time.
> > 
> > ::: b2g/chrome/content/shell.js
> > @@ +1132,5 @@
> > >        var flags =
> > >          context.DRAWWINDOW_DRAW_CARET |
> > >          context.DRAWWINDOW_DRAW_VIEW |
> > >          context.DRAWWINDOW_USE_WIDGET_LAYERS;
> > > +      context.scale(scale, scale);
> > 
> > I can see that this will produce a full-size screenshot. But does it
> > actually give a pefectly sharp screenshot, or is it fuzzy because it was
> > just scaled up?
> > 
> I could get the sharp screenshot with my patch.

By scaling the canvas, you resized the canvas pixels to be the device pixels so you get a sharp image. I've failed to figure out this, so thank you :)

> 
> > While you're fixing this, please also change the deprecated mozGetAsFile()
> > call to use the supported toBlob() method.  (We're getting deprecation
> > warnings in logcat about mozGetAsFile()).
> > 
> Sure, I will look into it.
> 

It's might be worthy to do this in another bug instead, if the switch is non-trivial.

> > Also, please try taking out the scale adjustments about and just using
> > toBlobHD().
> > 
> I'm curious that what is the canvas size we need to set if using toBlobHD()?
> If we are still using canvas size without considering devicePixelRatio, then
> the source generated by drawwindow won't be high quality.
> 

The description of *HD() methods mentions "native solution of canvas". AFAIK this is something Safari-specific (not even WebKit-specific). There isn't such thing for Gecko and I would imaging implementing that for Gecko would be non-trivial.

> > I don't really understand how canvas works when devicePixelRatio !== 1, but
> > I belive that toBlobHD() is intended for this case, and I think it might be
> > possible that you can just use toBlobHD() and not need any of the
> > multiplication by devicePixelRatio. If that works, it seems like a better
> > solution.
> > 
> For canvas scale call, it will scale the draw region for drawwindow().
> Since we got the high resolution screen, then the scaled draw region just
> cover the whole screen for snapshot.
> 
> > Also, feel free to find a better reviewer than me for this. If you can find
> > someone who actually understands how this stuff works, and someone who
> > builds gecko frequently, they would be a better reviewer than me.  But if
> > not, I can review this patch again.
> > 

I can review the patch if :djf agrees with me on the above comments.

> > Is this the right way to fix it, or could you just change the toBlob() call
> > to

I believe this is being cuted off...

Comment 15

[Peter Chang[:pchang]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #14)
> I think we should accept this patch for the sake of 1.3+
> 
> (In reply to peter chang[:pchang] from comment #12)
> > (In reply to David Flanagan [:djf] from comment #11)
> > > Comment on attachment 8349631 [details] [diff] [review]
> > > patch v1
> > > 
> > > Review of attachment 8349631 [details] [diff] [review]:
> > > While you're fixing this, please also change the deprecated mozGetAsFile()
> > > call to use the supported toBlob() method.  (We're getting deprecation
> > > warnings in logcat about mozGetAsFile()).
> > > 
> > Sure, I will look into it.
> > 
> 
> It's might be worthy to do this in another bug instead, if the switch is
> non-trivial.
> 
It is better to create another bug for mozGetAsFile() because this bug is a blocker for v1.3.
> > > Also, please try taking out the scale adjustments about and just using
> > > toBlobHD().
> > > 
> > I'm curious that what is the canvas size we need to set if using toBlobHD()?
> > If we are still using canvas size without considering devicePixelRatio, then
> > the source generated by drawwindow won't be high quality.
> > 
> 
> The description of *HD() methods mentions "native solution of canvas". AFAIK
> this is something Safari-specific (not even WebKit-specific). There isn't
> such thing for Gecko and I would imaging implementing that for Gecko would
> be non-trivial.
> 
Agree, I think it will require a lots of works from gecko side.

Comment 16

[Peter Chang[:pchang]]

Actually there is another bug for mozGetAsFile, bug 946402.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8349631 [details] [diff] [review]
patch v1

I'm surprised that it just works with a scale, but happy to hear that it does.  And if there is another bug filed for the toBlob() change, then this seems good to me.

Comment 18

[Peter Chang[:pchang]]

Attachment: patch v2

update reviewers into patch

Comment 19

[Peter Chang[:pchang]]

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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