Change the scale factor of texture_size from the layer's one to an estimation.

Change the scale factor of texture_size from the layer's one to an estimation.

The phantom window appearing in a different DPI device could be wrongly scaled
without doing this, since the original texture is calculated in the original
DPI but rendered in another display.

BUG=145968
TEST=run chromeos-chrome with --aura-host-window-size='0+0-500x500,600+0-500x500*2' --force-compositing-mode on linux, open a web page, and drag the window across the screens.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158099

[oshima, 2012-09-18 21:20:45]

piman should review this.

(thank you for fixing this anyway!)

http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
ui/compositor/layer.cc:760: // estimate the scale factor from the size of the
texture and the layer.
can you update the comment to be more generic (without referring to phantom
windows) as this layer has no dependency to ash/chromeos.

[oshima, 2012-09-19 01:54:17]

mukai-san, can you add unit test for this? (layer_unittest.cc)


On 2012/09/18 21:20:45, oshima wrote:
> piman should review this.
> 
> (thank you for fixing this anyway!)
> 
> http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
> ui/compositor/layer.cc:760: // estimate the scale factor from the size of the
> texture and the layer.
> can you update the comment to be more generic (without referring to phantom
> windows) as this layer has no dependency to ash/chromeos.

[Jun Mukai, 2012-09-19 19:26:56]

http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
ui/compositor/layer.cc:760: // estimate the scale factor from the size of the
texture and the layer.
On 2012/09/18 21:20:45, oshima wrote:
> can you update the comment to be more generic (without referring to phantom
> windows) as this layer has no dependency to ash/chromeos.

Removed that part.

[Jun Mukai, 2012-09-19 19:29:54]

On 2012/09/19 01:54:17, oshima wrote:
> mukai-san, can you add unit test for this? (layer_unittest.cc)

Can we do that in another CL?  layer_unittest.cc doesn't have such cases
(drawing/scaling with external textures), and I couldn't find good fields to
verify the correctness from here.

> 
> 
> On 2012/09/18 21:20:45, oshima wrote:
> > piman should review this.
> > 
> > (thank you for fixing this anyway!)
> > 
> > http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
> > File ui/compositor/layer.cc (right):
> > 
> >
>
http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
> > ui/compositor/layer.cc:760: // estimate the scale factor from the size of
the
> > texture and the layer.
> > can you update the comment to be more generic (without referring to phantom
> > windows) as this layer has no dependency to ash/chromeos.

[oshima, 2012-09-19 19:55:53]

On 2012/09/19 19:29:54, Jun Mukai wrote:
> On 2012/09/19 01:54:17, oshima wrote:
> > mukai-san, can you add unit test for this? (layer_unittest.cc)
> 
> Can we do that in another CL?  layer_unittest.cc doesn't have such cases
> (drawing/scaling with external textures), and I couldn't find good fields to
> verify the correctness from here.

Ok I'll leave it to piman then. LGTM

> 
> > 
> > 
> > On 2012/09/18 21:20:45, oshima wrote:
> > > piman should review this.
> > > 
> > > (thank you for fixing this anyway!)
> > > 
> > > http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
> > > File ui/compositor/layer.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
> > > ui/compositor/layer.cc:760: // estimate the scale factor from the size of
> the
> > > texture and the layer.
> > > can you update the comment to be more generic (without referring to
phantom
> > > windows) as this layer has no dependency to ash/chromeos.

[oshima, 2012-09-19 19:56:37]

On 2012/09/19 19:55:53, oshima wrote:
> On 2012/09/19 19:29:54, Jun Mukai wrote:
> > On 2012/09/19 01:54:17, oshima wrote:
> > > mukai-san, can you add unit test for this? (layer_unittest.cc)
> > 
> > Can we do that in another CL?  layer_unittest.cc doesn't have such cases
> > (drawing/scaling with external textures), and I couldn't find good fields to
> > verify the correctness from here.
> 
> Ok I'll leave it to piman then. LGTM

i meant "leave the decision to piman", in case it's not clear.

> 
> > 
> > > 
> > > 
> > > On 2012/09/18 21:20:45, oshima wrote:
> > > > piman should review this.
> > > > 
> > > > (thank you for fixing this anyway!)
> > > > 
> > > > http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc
> > > > File ui/compositor/layer.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10941017/diff/1/ui/compositor/layer.cc#newcode760
> > > > ui/compositor/layer.cc:760: // estimate the scale factor from the size
of
> > the
> > > > texture and the layer.
> > > > can you update the comment to be more generic (without referring to
> phantom
> > > > windows) as this layer has no dependency to ash/chromeos.

[piman, 2012-09-19 23:08:48]

http://codereview.chromium.org/10941017/diff/6001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/6001/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:762: static_cast<float>(bounds().width()) /
texture_->size().width();
This will entirely defeat the purpose of computing the UV rect below (to avoid
resizing the texture when resizing windows if it doesn't match). And you may
generate non-integer scaling factors which will produce bad artifacts.

What you have here is a texture with a device pixel ratio that doesn't match the
layer's device pixel ratio. So to properly fix it you'd need to override the
layer's pixel ratio for mapping the content.
Alternatively, when creating the texture (RecreateLayer I guess?) you could set
the texture size in DIPs, and set the scale_content_ flag on the layer.

[Jun Mukai, 2012-09-20 22:00:31]

http://codereview.chromium.org/10941017/diff/6001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/6001/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:762: static_cast<float>(bounds().width()) /
texture_->size().width();
On 2012/09/19 23:08:48, piman wrote:
> This will entirely defeat the purpose of computing the UV rect below (to avoid
> resizing the texture when resizing windows if it doesn't match). And you may
> generate non-integer scaling factors which will produce bad artifacts.
> 
> What you have here is a texture with a device pixel ratio that doesn't match
the
> layer's device pixel ratio. So to properly fix it you'd need to override the
> layer's pixel ratio for mapping the content.
> Alternatively, when creating the texture (RecreateLayer I guess?) you could
set
> the texture size in DIPs, and set the scale_content_ flag on the layer.

Thanks for comments.  Then I introduced another field for the scale factor for
the textures.  Please take another look at this.

[piman, 2012-09-20 23:20:06]

http://codereview.chromium.org/10941017/diff/10001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/10001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:383: device_scale_factor_for_texture_ =
device_scale_factor_;
mmh, that may solve the problem in a given case, but not the general problem.
If you have one layer tree that's on screen1 with a device ratio R1, and one
layer tree that's on screen2 with device ratio R2, and you want to put the same
texture in both, this won't work.
What you really want is to have a per-texture device pixel ratio. There's no
reason we can't extend Texture for that - and they're only created in 2 places,
both in ImageTransportFactory, only called from RenderWidgetHostViewAura, which
incidentally is what knows the pixel ratio for the texture.

[Jun Mukai, 2012-09-21 00:53:55]

http://codereview.chromium.org/10941017/diff/10001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10941017/diff/10001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:383: device_scale_factor_for_texture_ =
device_scale_factor_;
On 2012/09/20 23:20:07, piman wrote:
> mmh, that may solve the problem in a given case, but not the general problem.
> If you have one layer tree that's on screen1 with a device ratio R1, and one
> layer tree that's on screen2 with device ratio R2, and you want to put the
same
> texture in both, this won't work.
> What you really want is to have a per-texture device pixel ratio. There's no
> reason we can't extend Texture for that - and they're only created in 2
places,
> both in ImageTransportFactory, only called from RenderWidgetHostViewAura,
which
> incidentally is what knows the pixel ratio for the texture.

hmm, then added the device_scale_factor to ui::Texture itself.  please take
another look, again.

[piman, 2012-09-21 00:59:34]

http://codereview.chromium.org/10941017/diff/11003/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.h (right):

http://codereview.chromium.org/10941017/diff/11003/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.h:93: virtual void
SetDeviceScaleFactor(float device_scale_factor) = 0;
Because ImageTransportFactory is a singleton, that doesn't really work. It's
probably better to pass it to CreateTransportClient/CreateOwnedTexture.

[Jun Mukai, 2012-09-21 02:28:52]

http://codereview.chromium.org/10941017/diff/11003/content/browser/renderer_h...
File content/browser/renderer_host/image_transport_factory.h (right):

http://codereview.chromium.org/10941017/diff/11003/content/browser/renderer_h...
content/browser/renderer_host/image_transport_factory.h:93: virtual void
SetDeviceScaleFactor(float device_scale_factor) = 0;
On 2012/09/21 00:59:34, piman wrote:
> Because ImageTransportFactory is a singleton, that doesn't really work. It's
> probably better to pass it to CreateTransportClient/CreateOwnedTexture.

removed this and added an argument to CreateTransportClient/OwnedTexture.

[piman, 2012-09-21 03:56:55]

lgtm

[oshima, 2012-09-21 04:32:04]

http://codereview.chromium.org/10941017/diff/13002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

http://codereview.chromium.org/10941017/diff/13002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.h:327: // The scale
factor of the device.
nit: .. of the display the renderer is currently on.

[Jun Mukai, 2012-09-21 18:25:47]

http://codereview.chromium.org/10941017/diff/13002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

http://codereview.chromium.org/10941017/diff/13002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.h:327: // The scale
factor of the device.
On 2012/09/21 04:32:04, oshima wrote:
> nit: .. of the display the renderer is currently on.

Done.

[Jun Mukai, 2012-09-21 19:12:01]

sky, can you take a look at this as the OWNER?

[sky, 2012-09-21 20:24:48]

LGTM

[commit-bot: I haz the power, 2012-09-21 20:36:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10941017/19002

[commit-bot: I haz the power, 2012-09-21 22:33:27]

Change committed as 158099