cc: Use the size of the source frame along with the source frame's renderpasses

cc: Use the size of the source frame along with the source frame's renderpasses

Scale the contents of the source frame to fit exactly in the bounds of the
delegated renderer layer.

When the host window is being resized, it can keep the delegated renderer layer
at the size it wants the output displayed, and clip it with a containing layer
until the resize is complete and it has a new frame to show at the right size.

Tests:
DelegatedRendererLayerImplTestSharedData.SharedData
DelegatedRendererLayerImplTestSharedData.DisplaySize

BUG=123444
R=enne
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177878

[enne (OOO), 2013-01-09 02:48:20]

https://codereview.chromium.org/11821015/diff/1/cc/delegated_renderer_layer_i...
File cc/delegated_renderer_layer_impl.h (right):

https://codereview.chromium.org/11821015/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.h:48: gfx::Size m_frameSize;
Is this redundant with the root render pass's output rect?

https://codereview.chromium.org/11821015/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.h:48: gfx::Size m_frameSize;
Is this redundant with the root render pass's output rect?

[danakj, 2013-01-09 02:55:49]

https://codereview.chromium.org/11821015/diff/1/cc/delegated_renderer_layer_i...
File cc/delegated_renderer_layer_impl.h (right):

https://codereview.chromium.org/11821015/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.h:48: gfx::Size m_frameSize;
On 2013/01/09 02:48:20, enne wrote:
> Is this redundant with the root render pass's output rect?

Ya I totally think so. We don't need the size on the compositorframe struct
either.

[danakj, 2013-01-09 03:04:44]

PTAL

[enne (OOO), 2013-01-09 03:28:32]

lgtm

[piman, 2013-01-09 18:17:08]

Relying on an extra layer to do clipping during resize is unfortunate, and will
make things super awkward at the ui/compositor level (we need to introduce an
extra cc::Layer in ui::Layer and all the logic to do sizing and parenting will
need to deal with the fact that there may be 2).
Can't we do what we discussed last night, having a separate frame size vs
desired display size (to deal with DSF mismatch), take the scale out of there
(or maybe pass it explicitly?), but not scale from desired display size to
bounds(), only clip?

[danakj, 2013-01-09 21:43:24]

On 2013/01/09 18:17:08, piman wrote:
> Relying on an extra layer to do clipping during resize is unfortunate, and
will
> make things super awkward at the ui/compositor level (we need to introduce an
> extra cc::Layer in ui::Layer and all the logic to do sizing and parenting will
> need to deal with the fact that there may be 2).
> Can't we do what we discussed last night, having a separate frame size vs
> desired display size (to deal with DSF mismatch), take the scale out of there
> (or maybe pass it explicitly?), but not scale from desired display size to
> bounds(), only clip?

I see, so my thinking went..

We could stick a size in here and render at that size, but it doesn't clip. The
only mechanism for clipping we have in the compositor now is "masks to bounds"
which would require a parent cc::Layer to do the clipping anyhow, so why add an
extra value here and not just use the bounds.

When we talked I was assuming the parent, but that both layers were always the
same size, so needing the extra value.

We can introduce a new clipping method for the layer, but it'll be a bigger,
separate, CL. I would kinda prefer to keep a clipping layer in ui::Layer,
similar to how GraphicsLayerChromium sometimes has to deal with multiple layers,
if we can..

Unless I'm forgetting some other clip mechanism right now, enne?

[enne (OOO), 2013-01-09 21:45:12]

On 2013/01/09 21:43:24, danakj wrote:
> Unless I'm forgetting some other clip mechanism right now, enne?

Nope, you can only mask to bounds right now.  So if you want to clip
arbitrarily, you need an extra layer.

[danakj, 2013-01-09 21:47:28]

On Wed, Jan 9, 2013 at 4:45 PM, <enne@chromium.org> wrote:

> On 2013/01/09 21:43:24, danakj wrote:
>
>> Unless I'm forgetting some other clip mechanism right now, enne?
>>
>
> Nope, you can only mask to bounds right now.  So if you want to clip
> arbitrarily, you need an extra layer.
>

I smell a bug where we need to apply the clip rect on the layer to the
quads its generating.

So.. it might be possible to just modify the quads. Let's make a bug for it.


>
>
https://codereview.chromium.**org/11821015/<https://codereview.chromium.org/1...
>

[piman, 2013-01-09 22:48:29]

On Wed, Jan 9, 2013 at 1:43 PM, <danakj@chromium.org> wrote:

> On 2013/01/09 18:17:08, piman wrote:
>
>> Relying on an extra layer to do clipping during resize is unfortunate, and
>>
> will
>
>> make things super awkward at the ui/compositor level (we need to
>> introduce an
>> extra cc::Layer in ui::Layer and all the logic to do sizing and parenting
>> will
>> need to deal with the fact that there may be 2).
>> Can't we do what we discussed last night, having a separate frame size vs
>> desired display size (to deal with DSF mismatch), take the scale out of
>> there
>> (or maybe pass it explicitly?), but not scale from desired display size to
>> bounds(), only clip?
>>
>
> I see, so my thinking went..
>
> We could stick a size in here and render at that size, but it doesn't
> clip. The
> only mechanism for clipping we have in the compositor now is "masks to
> bounds"
> which would require a parent cc::Layer to do the clipping anyhow, so why
> add an
> extra value here and not just use the bounds.
>

In ui::Layer we do size the cc::Layer appropriately based on the renderer's
frame size:
- if renderer smaller than ui::Layer, size it to renderer size.
- if renderer bigger than ui::Layer, size it to ui::Layer size.

We don't need an extra layer, but we do need the good mapping from the
renderer's frame to the layer, i.e. for the TextureLayer, we need the uv
rect - see ui::Layer::RecomputeDrawsContentAndUVRect.

I'm ok requiring masks to bounds == true on the DRL to achieve the
guarantee that we won't draw outside the layer, but we just need to be able
to have frame size != bounds() with a scaling of 1 (i.e. decouple the
scaling from the ratio frame size / bounds() - similar to the uv rect
concept).

Antoine


>
> When we talked I was assuming the parent, but that both layers were always
> the
> same size, so needing the extra value.
>
> We can introduce a new clipping method for the layer, but it'll be a
> bigger,
> separate, CL. I would kinda prefer to keep a clipping layer in ui::Layer,
> similar to how GraphicsLayerChromium sometimes has to deal with multiple
> layers,
> if we can..
>
> Unless I'm forgetting some other clip mechanism right now, enne?
>
>
https://codereview.chromium.**org/11821015/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-19 00:17:35]

On 2013/01/09 22:48:29, piman wrote:
> In ui::Layer we do size the cc::Layer appropriately based on the renderer's
> frame size:
> - if renderer smaller than ui::Layer, size it to renderer size.
> - if renderer bigger than ui::Layer, size it to ui::Layer size.
> 
> We don't need an extra layer, but we do need the good mapping from the
> renderer's frame to the layer, i.e. for the TextureLayer, we need the uv
> rect - see ui::Layer::RecomputeDrawsContentAndUVRect.

In the texture layer case, the bounds of the layer are tied to the bounds of the
texture right?

In the DRLayer case, you can set the bounds to anything arbitrarily. I *think* I
see to functionally equivalent options:

1) Set bounds to the clipping size, provide a size to which the contents are
scaled.

2) Set bounds to the size to which the contents are scaled, provide a rect in
the space of the layer to which things should be clipped.

I'd prefer 2 if possible. I'm not sure why but it smells better to me.. I like
things being inside the bounds() always, and clipping is a common thing in cc/.

[piman, 2013-01-19 00:36:17]

On Fri, Jan 18, 2013 at 4:17 PM, <danakj@chromium.org> wrote:

> On 2013/01/09 22:48:29, piman wrote:
>
>> In ui::Layer we do size the cc::Layer appropriately based on the
>> renderer's
>> frame size:
>> - if renderer smaller than ui::Layer, size it to renderer size.
>> - if renderer bigger than ui::Layer, size it to ui::Layer size.
>>
>
>  We don't need an extra layer, but we do need the good mapping from the
>> renderer's frame to the layer, i.e. for the TextureLayer, we need the uv
>> rect - see ui::Layer::**RecomputeDrawsContentAndUVRect**.
>>
>
> In the texture layer case, the bounds of the layer are tied to the bounds
> of the
> texture right?
>

Right, though in a non-trivial way (we shrink to the texture, but we won't
expand beyond the ui::Layer's size).


> In the DRLayer case, you can set the bounds to anything arbitrarily. I
> *think* I
> see to functionally equivalent options:
>
> 1) Set bounds to the clipping size, provide a size to which the contents
> are
> scaled.
>
> 2) Set bounds to the size to which the contents are scaled, provide a rect
> in
> the space of the layer to which things should be clipped.
>
> I'd prefer 2 if possible. I'm not sure why but it smells better to me.. I
> like
> things being inside the bounds() always, and clipping is a common thing in
> cc/.
>

I think 1 makes more sense, otherwise:
- if you set bounds larger that what you're going to draw, then your
content is no longer opaque (or you need extra logic to handle that new
special clip rect)
- similarly you'll need special logic to handle that new special clip rect
when sizing render targets, otherwise you'll allocate more than necessary.

The way I view things, it's similar to a content layer: the content space
can be larger than the layer, which provides a "view" into the content.
Similarly here, the delegated content space can be larger than the layer,
which provides a view into it.


>
>
https://codereview.chromium.**org/11821015/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-19 01:48:25]

piman@ PTAL. added displaySize() along with a test.

[piman, 2013-01-19 02:02:47]

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
cc/delegated_renderer_layer_impl.cc:159:
static_cast<double>(displaySize.height()) / frameSize.height());
perf nit: should we factor out drawTransform() *
delegatedFrameToLayerSpaceTransform to avoid doing it 3 times?

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
File cc/delegated_renderer_layer_impl.h (right):

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
cc/delegated_renderer_layer_impl.h:29: void setDisplaySize(gfx::Size
displaySize) { m_displaySize = displaySize; }
You need a way to set this from the main thread :)

[danakj, 2013-01-19 02:10:26]

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
File cc/delegated_renderer_layer_impl.h (right):

https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
cc/delegated_renderer_layer_impl.h:29: void setDisplaySize(gfx::Size
displaySize) { m_displaySize = displaySize; }
On 2013/01/19 02:02:47, piman wrote:
> You need a way to set this from the main thread :)

Ya, setting the frames as well. I thought I'd leave it for now since James has a
huge patch pending that this would require him to update.

[piman, 2013-01-19 02:40:17]

On 2013/01/19 02:10:26, danakj wrote:
>
https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
> File cc/delegated_renderer_layer_impl.h (right):
> 
>
https://codereview.chromium.org/11821015/diff/12002/cc/delegated_renderer_lay...
> cc/delegated_renderer_layer_impl.h:29: void setDisplaySize(gfx::Size
> displaySize) { m_displaySize = displaySize; }
> On 2013/01/19 02:02:47, piman wrote:
> > You need a way to set this from the main thread :)
> 
> Ya, setting the frames as well. I thought I'd leave it for now since James has
a
> huge patch pending that this would require him to update.

Ok, LGTM in the mean time

[commit-bot: I haz the power, 2013-01-19 06:30:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11821015/12002

[commit-bot: I haz the power, 2013-01-19 08:56:00]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-19 16:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11821015/12002

[commit-bot: I haz the power, 2013-01-19 18:24:50]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-20 17:00:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11821015/12002

[commit-bot: I haz the power, 2013-01-20 19:25:29]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-20 19:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11821015/12002

[commit-bot: I haz the power, 2013-01-20 19:55:41]

Change committed as 177878