cc: Refactor content scale/bounds into draw properties

cc: Refactor content scale/bounds into draw properties

This change allows layer impls to manipulate their content scale.  This will
allow PictureLayerImpl to pick some contents scale based on the scales of their
tilings, rather than being stuck at the contents scale of its PictureLayer.

This also de-virtualizes all of the content scale/bounds functions and instead
allows a layer to manipulate its draw properties in response to a bounds or
contents scale change.

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

[enne (OOO), 2012-12-10 02:04:12]

Here's a patch based on a previous discussion about refactoring content scales
and bounds.

These are somewhat different than other draw properties, in that a layer tends
to need to know its content bounds prior to calcDrawProperties.  This may be
cleaned up with other refactorings, but I wanted to punt on that here.

That may mean that maybe content scales and bounds don't belong in draw
properties at this point, and I think the patch would still work if
Layer/LayerImpl stored content scales and bounds on themselves.

[shawnsingh, 2012-12-11 21:54:05]

So I think I'm beginning to understand what's going on here.  In my own words,
here's what I understand:

1. User gives us: bounds and contentsScale. Implicitly, some layer types already
know their contentBounds, too.

2. There is an obvious formula that has been our guiding principle:
  contentBounds = bounds * contentsScale.

3. However, if we are given all 3 pieces of information and it's inconsistent,
we have to override something to make it consistent.  For image_layer, we
override the contentsScale based on the value of contentBounds.  For
contents_scaling_layer, we override the contentBounds based on the value of
contentsScale.

----
I think it would be possible to use the same paradigm as we did for opacity: The
layer has "given" opacity, and after calcDrawProperties() we also have
drawProperties.opacity, which we understand may be different.  Yes, there are
two different floats named "opacity", but one is in drawProperties, which we
assume is only known after calcDrawProps, while the other is in the layer
itself, which is assumed to be given by the user of the layer API.   The impl
thread would have only "given properties" pushed to it, so it recomputes the
drawProperties opacity again.

Comments I've added below reflect this paradigm.  Hopefully I'm not missing some
fundamental reason this isn't possible =)

https://codereview.chromium.org/11503005/diff/1/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11503005/diff/1/cc/draw_properties.h#newcode74
cc/draw_properties.h:74: float ideal_contents_scale;
Conceptually, it seems like ideal_contents_scale is merely a temporary value
that is passed to setIdealContentsScale() for the purposes of computing
contents_scale_x and contents_scale_y.

If it's really needed for contents_scaling_layer and possibly image_layer,
perhaps we should make it a private variable unique to those classes, instead of
keeping it here.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode87
cc/layer.h:87: virtual void didUpdateBounds();
I think we can do without this.  Instead, I think it might be better to have a
one-way flow of logic from "given properties" --> "computed drawProperties".

So, when layer's given properties change, we could just note to ourselves that
"calcDrawProperties will need to be run" and accept the change.  Then
calcDrawProperties can call setIdealContentsScale as appropriate, where the
layer updates its draw properties.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode231
cc/layer.h:231: float contentsScaleY() const { return
m_drawProperties.contents_scale_y; }
maybe to avoid confusion with the previous state of the code, we should remove
these accessors, and require people to use drawProperties().contents_scale_x
instead.

That way we can be more explicitly aware of any accidental cycles in our data
flow?  For example, I was briefly confused why image_layer used contentsScaleX()
to compute contentBounds, until I realized that image_layer implemented their
own contentsScaleX() accessor.  removing it I think would be much clearer.

Eventually, I'm thinking we should remove all accessors that return
m_drawProperties values, for the same reason.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode233
cc/layer.h:233: virtual void setIdealContentsScale(float ideal_contents_scale);
instead of "set" which makes it feel like it's part of the user's public API,
how about we say "calculateActualContentsScale(float ideal_contents_scale)"?   I
personally think that helps separate this function from the settors that are
intended to be a sort of public layer API

https://codereview.chromium.org/11503005/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11503005/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:367: static inline void
updateLayerContentsScale(LayerImpl* layer, const gfx::Transform&
combinedTransform, float deviceScaleFactor, float pageScaleFactor, bool
animatingTransformToScreen)
"update" was actually a word that really made me fumble at first.   It suddenly
makes the code feel not-so-one-way flow of data.

Could we please rename this (and the main thread version of this function) to
something like "calculateLayerContentsScale"?

[enne (OOO), 2012-12-11 23:23:44]

On 2012/12/11 21:54:05, shawnsingh wrote:
> So I think I'm beginning to understand what's going on here.  In my own words,
> here's what I understand:
> 
> 1. User gives us: bounds and contentsScale. Implicitly, some layer types
already
> know their contentBounds, too.
> 
> 2. There is an obvious formula that has been our guiding principle:
>   contentBounds = bounds * contentsScale.
> 
> 3. However, if we are given all 3 pieces of information and it's inconsistent,
> we have to override something to make it consistent.  For image_layer, we
> override the contentsScale based on the value of contentBounds.  For
> contents_scaling_layer, we override the contentBounds based on the value of
> contentsScale.

I think this is not quite complete, and I think you are privileging the "guiding
principle".

There are three types of layers that we support:

(1) "Normal" layers have bounds() == contentBounds() and content scale 1
(2) Content scaling layers have contentBounds = ceil(bounds * contentsScale)
(3) Image layers have contentBounds = image size, and then back-calculate the
contents scale for anybody who needs it.

The data flow is that you give the layer a contents scale and then the layer,
using its internal knowledge of bounds() and depending on what type it is, can
figure out its contentBounds and contentScale.

Content bounds is an important piece of information here, as it gets baked into
draw transforms and is the space that a layer is expected to generate quads in.

> I think it would be possible to use the same paradigm as we did for opacity:
The
> layer has "given" opacity, and after calcDrawProperties() we also have
> drawProperties.opacity, which we understand may be different.  Yes, there are
> two different floats named "opacity", but one is in drawProperties, which we
> assume is only known after calcDrawProps, while the other is in the layer
> itself, which is assumed to be given by the user of the layer API.   The impl
> thread would have only "given properties" pushed to it, so it recomputes the
> drawProperties opacity again.

I don't really agree with this, for several reasons.

(1) Draw opacity (and all other current draw properties) can be calculated from
Layer(Impl) properties directly.  Content scale and content bounds require
special knowledge about the layer type.

(2) ...because of that special knowledge, I don't really want to have to
replicate "contents scaling layer impl" on the impl hierarchy just so that these
values can be calculated instead of pushed.

(3) Draw opacity is accessed with different semantics than content bounds.  As
mentioned in a comment below, unlike draw opacity we use content bounds a lot
and expect it to be a fresh value (reflecting bounds) in a way that we don't
expect draw opacity to reflect the tree's opacity other than at draw time.

I think these comments lead me to believe that these values are not really a
good fit for draw properties.  They should probably be moved back to
Layer/LayerImpl itself.

https://codereview.chromium.org/11503005/diff/1/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11503005/diff/1/cc/draw_properties.h#newcode74
cc/draw_properties.h:74: float ideal_contents_scale;
On 2012/12/11 21:54:05, shawnsingh wrote:
> Conceptually, it seems like ideal_contents_scale is merely a temporary value
> that is passed to setIdealContentsScale() for the purposes of computing
> contents_scale_x and contents_scale_y.
> 
> If it's really needed for contents_scaling_layer and possibly image_layer,
> perhaps we should make it a private variable unique to those classes, instead
of
> keeping it here.

Ok, sure.  I can store it elsewhere.  I just thought it might be useful to store
here in one place since it was something that all layers calculated.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode87
cc/layer.h:87: virtual void didUpdateBounds();
On 2012/12/11 21:54:05, shawnsingh wrote:
> I think we can do without this.  Instead, I think it might be better to have a
> one-way flow of logic from "given properties" --> "computed drawProperties".
> 
> So, when layer's given properties change, we could just note to ourselves that
> "calcDrawProperties will need to be run" and accept the change.  Then
> calcDrawProperties can call setIdealContentsScale as appropriate, where the
> layer updates its draw properties.

Sadly, no.  Unfortunately, we do so much in content space that we pretty much
need to immediately update content bounds to reflect changes in bounds.  I don't
think this patch is possible without didUpdateBounds().  This is the equivalent
of the previous virtual contentBounds() function.

There may be some later refactoring possible to make this cleaner, but I don't
see any way to do this easily right now.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode231
cc/layer.h:231: float contentsScaleY() const { return
m_drawProperties.contents_scale_y; }
On 2012/12/11 21:54:05, shawnsingh wrote:
> maybe to avoid confusion with the previous state of the code, we should remove
> these accessors, and require people to use drawProperties().contents_scale_x
> instead.
> 
> That way we can be more explicitly aware of any accidental cycles in our data
> flow?  For example, I was briefly confused why image_layer used
contentsScaleX()
> to compute contentBounds, until I realized that image_layer implemented their
> own contentsScaleX() accessor.  removing it I think would be much clearer.
> 
> Eventually, I'm thinking we should remove all accessors that return
> m_drawProperties values, for the same reason.

I think that's just an issue with ImageLayer shadowing these functions.  I
should rename them there.  There's no cycle, there's just an accidental
shadowing.

Callers should really not care about whether these values are lazily computed or
computed and stored.  If you want to change things to call drawProperties() more
explicitly, then that can be a separate patch.

https://codereview.chromium.org/11503005/diff/1/cc/layer.h#newcode233
cc/layer.h:233: virtual void setIdealContentsScale(float ideal_contents_scale);
On 2012/12/11 21:54:05, shawnsingh wrote:
> instead of "set" which makes it feel like it's part of the user's public API,
> how about we say "calculateActualContentsScale(float ideal_contents_scale)"?  
I
> personally think that helps separate this function from the settors that are
> intended to be a sort of public layer API

Sure.  I can rename this for clarity.

https://codereview.chromium.org/11503005/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11503005/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:367: static inline void
updateLayerContentsScale(LayerImpl* layer, const gfx::Transform&
combinedTransform, float deviceScaleFactor, float pageScaleFactor, bool
animatingTransformToScreen)
On 2012/12/11 21:54:05, shawnsingh wrote:
> "update" was actually a word that really made me fumble at first.   It
suddenly
> makes the code feel not-so-one-way flow of data.
> 
> Could we please rename this (and the main thread version of this function) to
> something like "calculateLayerContentsScale"?

Sure, I can rename this.  Why is the word update problematic for you?

[shawnsingh, 2012-12-12 00:40:31]

> 
> I think this is not quite complete, and I think you are privileging the
"guiding
> principle".
> 
> There are three types of layers that we support:
> 
> (1) "Normal" layers have bounds() == contentBounds() and content scale 1
> (2) Content scaling layers have contentBounds = ceil(bounds * contentsScale)
> (3) Image layers have contentBounds = image size, and then back-calculate the
> contents scale for anybody who needs it.
> 
> The data flow is that you give the layer a contents scale and then the layer,
> using its internal knowledge of bounds() and depending on what type it is, can
> figure out its contentBounds and contentScale.
> 
> Content bounds is an important piece of information here, as it gets baked
into
> draw transforms and is the space that a layer is expected to generate quads
in.

Actually I think we're saying the same thing in different ways.


> 
> > I think it would be possible to use the same paradigm as we did for opacity:
> The
> > layer has "given" opacity, and after calcDrawProperties() we also have
> > drawProperties.opacity, which we understand may be different.  Yes, there
are
> > two different floats named "opacity", but one is in drawProperties, which we
> > assume is only known after calcDrawProps, while the other is in the layer
> > itself, which is assumed to be given by the user of the layer API.   The
impl
> > thread would have only "given properties" pushed to it, so it recomputes the
> > drawProperties opacity again.
> 
> I don't really agree with this, for several reasons.
> 
> (1) Draw opacity (and all other current draw properties) can be calculated
from
> Layer(Impl) properties directly.  Content scale and content bounds require
> special knowledge about the layer type.

The virtual function on this patch, setIdealContentsScale, seems to be just
perfect for this purpose without breaking the one-way flow of logic i'm
proposing...

> 
> (2) ...because of that special knowledge, I don't really want to have to
> replicate "contents scaling layer impl" on the impl hierarchy just so that
these
> values can be calculated instead of pushed.
> 

If that's a reason to keep the two different templatized versions of
updateLayerContentsScale in layer_tree_host_common, that seems reasonable to me.
 But doing this doesn't seem to break the one-way flow of logic, either.

> (3) Draw opacity is accessed with different semantics than content bounds.  As
> mentioned in a comment below, unlike draw opacity we use content bounds a lot
> and expect it to be a fresh value (reflecting bounds) in a way that we don't
> expect draw opacity to reflect the tree's opacity other than at draw time.
> 
> I think these comments lead me to believe that these values are not really a
> good fit for draw properties.  They should probably be moved back to
> Layer/LayerImpl itself.

As you mention below, if contentBounds are being used all over the place, in
particular before calcDrawProperties, then I totally agree it probably shouldn't
be in drawProperties.



> > Could we please rename this (and the main thread version of this function)
to
> > something like "calculateLayerContentsScale"?
> 
> Sure, I can rename this.  Why is the word update problematic for you?

This rename I was proposing was only if we do decide to put these in
drawProperties with a one-way data flow.  At that time, "update" seemed to imply
it modifies existing values, rather than doing givenProperties -->
drawProperties.  I don't think that applies anymore, the direction we seem to be
going (which seems fine to me), the name "update" does seem appropriate.

However, I do still prefer the "actual" part.   i.e. setIdealContentsScale --> 
updateActualContentsScaleAndBounds or something like that.   I see it both ways,
this is just my preference, not a big deal (yet...) =)

[enne (OOO), 2012-12-13 21:41:50]

danakj: Went back to draw props with a TODO for removing didUpdateBounds.  I'll
take care of it, just not during CY.

PTAL

[danakj, 2012-12-14 20:37:23]

We already talked about this all, but my thoughts :)

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer.cc
File cc/contents_scaling_layer.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
cc/contents_scaling_layer.cc:20: void
ContentsScalingLayer::updateContentsScale(float ideal_contents_scale) {
What do you think of making this method just return the actual scale/bounds it
wants to use, and have calcDrawProps set the m_drawProperties itself?

I think I'd like it if all the m_drawProps mutating happened in calcDrawProps or
helper static methods in lthcommon, and everything else treated it as const.

The didUpdateBounds() would be an exception for now.

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
File cc/contents_scaling_layer_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
cc/contents_scaling_layer_unittest.cc:49: testLayer->updateContentsScale(2.0f);
If you make this method return the scale/bounds it wants to use then you could
check those values instead of contentBounds() on the layer itself.

https://codereview.chromium.org/11503005/diff/11020/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11503005/diff/11020/cc/draw_properties.h#newc...
cc/draw_properties.h:80: float contents_scale_x;
Comment please, maybe this?

// The scale used to move between layer space and content space, and bounds of
the space. One is always a function of the other, but which one depends on the
layer type. For picture layers, this is an ideal scale, and not always the one
used.

https://codereview.chromium.org/11503005/diff/11020/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11503005/diff/11020/cc/layer.h#newcode315
cc/layer.h:315: DrawProperties<Layer, RenderSurface> m_drawProperties;
This can stay private I think.

If the subclass doesn't set this directly it doesn't need to access it, and
calcDraw and continue using it however it has been already.

https://codereview.chromium.org/11503005/diff/11020/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/layer_impl.cc#newcode660
cc/layer_impl.cc:660: void LayerImpl::setContentBounds(const gfx::Size&
contentBounds)
Random thought: What do you think of a setDrawProperties() method instead (or
maybe pushDrawProperties()?), and it only picks out the things it cares about
with a // why it chose them?

https://codereview.chromium.org/11503005/diff/11020/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:376: static inline void
updateLayerContentsScale(LayerImpl* layer, const gfx::Transform&
combinedTransform, float deviceScaleFactor, float pageScaleFactor, bool
animatingTransformToScreen)
Prefer this as a separate change where it'll be used.

[enne (OOO), 2012-12-14 22:06:05]

PTAL

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer.cc
File cc/contents_scaling_layer.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
cc/contents_scaling_layer.cc:20: void
ContentsScalingLayer::updateContentsScale(float ideal_contents_scale) {
On 2012/12/14 20:37:23, danakj wrote:
> What do you think of making this method just return the actual scale/bounds it
> wants to use, and have calcDrawProps set the m_drawProperties itself?
> 
> I think I'd like it if all the m_drawProps mutating happened in calcDrawProps
or
> helper static methods in lthcommon, and everything else treated it as const.
> 
> The didUpdateBounds() would be an exception for now.

Done.

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
File cc/contents_scaling_layer_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/contents_scaling_layer...
cc/contents_scaling_layer_unittest.cc:49: testLayer->updateContentsScale(2.0f);
On 2012/12/14 20:37:23, danakj wrote:
> If you make this method return the scale/bounds it wants to use then you could
> check those values instead of contentBounds() on the layer itself.

Added updateContentsScale to FakeTiledLayer as a helper.

https://codereview.chromium.org/11503005/diff/11020/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/layer_impl.cc#newcode660
cc/layer_impl.cc:660: void LayerImpl::setContentBounds(const gfx::Size&
contentBounds)
On 2012/12/14 20:37:23, danakj wrote:
> Random thought: What do you think of a setDrawProperties() method instead (or
> maybe pushDrawProperties()?), and it only picks out the things it cares about
> with a // why it chose them?

I'm not sure.  I think I'd prefer to punt on a pushDrawProperties and sort it
out later.

https://codereview.chromium.org/11503005/diff/11020/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11503005/diff/11020/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:376: static inline void
updateLayerContentsScale(LayerImpl* layer, const gfx::Transform&
combinedTransform, float deviceScaleFactor, float pageScaleFactor, bool
animatingTransformToScreen)
On 2012/12/14 20:37:23, danakj wrote:
> Prefer this as a separate change where it'll be used.

Done.

[danakj, 2012-12-14 22:58:40]

LGTM!

https://codereview.chromium.org/11503005/diff/19001/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11503005/diff/19001/cc/draw_properties.h#newc...
cc/draw_properties.h:80: float contents_scale_x;
descriptive comment please like the others :)

i wrote some prose in the previous review round if you like it, or something
else if you don't :P

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_common...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:3975: virtual void didUpdateBounds()
OVERRIDE
You probably don't need this, as the contentScaling layer will still use a
multiplier of 1 right?

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:1224: virtual void didUpdateBounds() OVERRIDE
Same here?

https://codereview.chromium.org/11503005/diff/19001/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11503005/diff/19001/cc/scrollbar_layer.cc#new...
cc/scrollbar_layer.cc:78: clampScaleToMaxTextureSize(idealContentsScale),
I like how this function turns out.

[enne (OOO), 2012-12-14 23:06:01]

Thanks.  :)

https://codereview.chromium.org/11503005/diff/19001/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11503005/diff/19001/cc/draw_properties.h#newc...
cc/draw_properties.h:80: float contents_scale_x;
On 2012/12/14 22:58:41, danakj wrote:
> descriptive comment please like the others :)
> 
> i wrote some prose in the previous review round if you like it, or something
> else if you don't :P

Done.  Sorry I missed that.  :)

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_common...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:3975: virtual void didUpdateBounds()
OVERRIDE
On 2012/12/14 22:58:41, danakj wrote:
> You probably don't need this, as the contentScaling layer will still use a
> multiplier of 1 right?

Done.

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11503005/diff/19001/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:1224: virtual void didUpdateBounds() OVERRIDE
On 2012/12/14 22:58:41, danakj wrote:
> Same here?

Done.

[commit-bot: I haz the power, 2012-12-14 23:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11503005/18022

[commit-bot: I haz the power, 2012-12-15 02:25:38]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-16 00:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11503005/24001

[commit-bot: I haz the power, 2012-12-16 04:52:55]

Change committed as 173365