Make Chromoting client plugin always render at device DPI.

Make Chromoting client plugin always render at device DPI.

This addresses image quality issues when to a high-resolution device from a high-DPI but lower resolution client.  In this case Chromoting would down-scale to the plugin's DIP dimensions, only for Chrome to up-scale to device dimensions.

Follow-up CLs will add DPI information to the FrameConsumer interface, and try to optimize the plugin's up-scaling mode.

BUG=133349, 135089
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148012

[Wez, 2012-07-17 18:15:31]

PTAL

[Sergey Ulanov, 2012-07-17 19:36:06]

couple drive-by questions.

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:331:
mouse_input_filter_->set_input_size(view_->get_view_size_dips());
Do we always send mouse position in DIPs?

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
File remoting/client/plugin/pepper_view.cc (right):

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_view.cc:177: pp::Graphics2DDev graphics2d =
pp::Graphics2D(instance_, pp_size, true);
why can't set graphics2d_ directly?

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_view.cc:193: pp_clip.width() * view_scale_,
ceil(pp_clip.width() * view_scale_)?

[Wez, 2012-07-17 20:11:43]

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:331:
mouse_input_filter_->set_input_size(view_->get_view_size_dips());
On 2012/07/17 19:36:06, sergeyu wrote:
> Do we always send mouse position in DIPs?

We will always receive them from Pepper in DIPs, yes, although in future they
will be floats with sub-DIP resolution.

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
File remoting/client/plugin/pepper_view.cc (right):

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_view.cc:177: pp::Graphics2DDev graphics2d =
pp::Graphics2D(instance_, pp_size, true);
On 2012/07/17 19:36:06, sergeyu wrote:
> why can't set graphics2d_ directly?

The |graphics2d_| member in the class is still a Graphics2D - I chose just to
use a temporary Graphics2DDev here to access the SetScale() dev API.

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_view.cc:193: pp_clip.width() * view_scale_,
On 2012/07/17 19:36:06, sergeyu wrote:
> ceil(pp_clip.width() * view_scale_)?

That shouldn't be necessary for the scaling factors we'll see in practice (1.0
or 2.0).

We should really use ceil(...) but with SkIRect::MakeLTRB(), to be correct.

https://chromiumcodereview.appspot.com/10785041/diff/1/remoting/client/plugin...
remoting/client/plugin/pepper_view.cc:193: pp_clip.width() * view_scale_,
On 2012/07/17 19:36:06, sergeyu wrote:
> ceil(pp_clip.width() * view_scale_)?

Done.

[Jamie, 2012-07-17 20:34:28]

I don't think I'm a great choice of reviewer for this. I've never modified any
of this code, and don't know it at all well. Sergey, since you've made some
comments already, would you be comfortable reviewing it?

[Wez, 2012-07-17 20:54:12]

Josh, can you take a look and sanity-check the use of Graphics2D::SetScale()?

[Josh Horwich, 2012-07-17 21:05:23]

On 2012/07/17 20:54:12, Wez wrote:
> Josh, can you take a look and sanity-check the use of Graphics2D::SetScale()?

Your use of SetScale (and creating a graphics2d at device-pixel dimensions by
using the view's scale) is correct.

[Wez, 2012-07-18 23:06:17]

sergeyu: Can you please review?

[Sergey Ulanov, 2012-07-19 17:22:56]

On 2012/07/18 23:06:17, Wez wrote:
> sergeyu: Can you please review?

looking now

[Sergey Ulanov, 2012-07-19 17:50:23]

LGTM, but see my nits.

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_view.cc:171: view_scale_ =
view_dev.GetDeviceScale();
nit: maybe call GetDeviceScale only once and save result? similar to how
view.GetRect() is called only once.

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_view.cc:177: pp::Graphics2DDev graphics2d =
pp::Graphics2D(instance_, pp_size, true);
nit: might be better if you assign graphics2d_ first and add _def suffix for
this value, e.g.:
  graphics2d_ = pp::Graphics2D(instance_, pp_size, true);
  pp::Graphics2DDev graphics2d_dev(graphics2d_);
  graphics2d_dev.SetScale(1.0f / view_scale_);

[Wez, 2012-07-20 00:01:59]

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_view.cc:171: view_scale_ =
view_dev.GetDeviceScale();
On 2012/07/19 17:50:23, sergeyu wrote:
> nit: maybe call GetDeviceScale only once and save result? similar to how
> view.GetRect() is called only once.

Done.

http://codereview.chromium.org/10785041/diff/9001/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_view.cc:177: pp::Graphics2DDev graphics2d =
pp::Graphics2D(instance_, pp_size, true);
On 2012/07/19 17:50:23, sergeyu wrote:
> nit: might be better if you assign graphics2d_ first and add _def suffix for
> this value, e.g.:
>   graphics2d_ = pp::Graphics2D(instance_, pp_size, true);
>   pp::Graphics2DDev graphics2d_dev(graphics2d_);
>   graphics2d_dev.SetScale(1.0f / view_scale_);

Done.

[commit-bot: I haz the power, 2012-07-23 21:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10785041/14001

[commit-bot: I haz the power, 2012-07-23 21:44:55]

Failed to apply patch for remoting/client/plugin/pepper_view.cc:
While running patch -p1 --forward --force;
patching file remoting/client/plugin/pepper_view.cc
Hunk #1 succeeded at 11 with fuzz 1 (offset 1 line).
Hunk #2 FAILED at 78.
Hunk #3 succeeded at 67 with fuzz 1 (offset -88 lines).
1 out of 3 hunks FAILED -- saving rejects to file
remoting/client/plugin/pepper_view.cc.rej

[commit-bot: I haz the power, 2012-07-23 23:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10785041/13002

[commit-bot: I haz the power, 2012-07-24 00:42:06]

Change committed as 148012