Let AreVisibleResourcesReady return correct value for PictureImageLayerImpl

Let AreVisibleResourcesReady return correct value for PictureImageLayerImpl

AreVisibleResourcesReady didn't return correct value because
PictureLayerImpl::raster_source_scale_ didn't reflect the actual value for
PictureImageLayerImpl. In this CL raster_contents_scale_ and
low_res_raster_contents_scale_ are stored in PictureLayerImpl.

BTW extracted the code to determine if raster scale needs to be adjusted
into ShouldAdjustRasterScale().

BUG=223201
TEST=PictureImageLayerImplTest.*


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

[enne (OOO), 2013-03-23 00:47:32]

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_image_la...
File cc/layers/picture_image_layer_impl_unittest.cc (right):

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_image_la...
cc/layers/picture_image_layer_impl_unittest.cc:50:
TEST_F(PictureImageLayerImplTest, CalcualateContentsScale) {
Typo.

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
cc/layers/picture_layer_impl.cc:589: if (!raster_page_scale_ ||
!raster_device_scale_ || !raster_contents_scale_)
I think this is wrong.  The previous check was if the raster source scale
changed which is not the same as raster contents scale.

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
cc/layers/picture_layer_impl.cc:624:
CalculateRasterContentsScale(animating_transform_to_screen);
I like the previous version where rather you had a functional-style function
that computed values rather than a function with side effects.  The former makes
it much obvious here in the caller that raster_contents_scale and
low_res_raster_contents_scale are affected by this function.

[Xianzhu, 2013-03-25 16:05:07]

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
cc/layers/picture_layer_impl.cc:589: if (!raster_page_scale_ ||
!raster_device_scale_ || !raster_contents_scale_)
On 2013/03/23 00:47:32, enne wrote:
> I think this is wrong.  The previous check was if the raster source scale
> changed which is not the same as raster contents scale.

I'm confused. How did the previous check deal with changes of raster source
scale?

[danakj, 2013-03-25 16:44:44]

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
cc/layers/picture_layer_impl.cc:589: if (!raster_page_scale_ ||
!raster_device_scale_ || !raster_contents_scale_)
On 2013/03/25 16:05:07, Xianzhu wrote:
> On 2013/03/23 00:47:32, enne wrote:
> > I think this is wrong.  The previous check was if the raster source scale
> > changed which is not the same as raster contents scale.
> 
> I'm confused. How did the previous check deal with changes of raster source
> scale?

It seems like you're just getting rid of raster source (i.e. CSS) scale here?

The "contents scale" is meant to be the product of the source*page*device
scales.

[Xianzhu, 2013-03-25 16:51:00]

On 2013/03/25 16:44:44, danakj wrote:
>
https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_im...
> cc/layers/picture_layer_impl.cc:589: if (!raster_page_scale_ ||
> !raster_device_scale_ || !raster_contents_scale_)
> On 2013/03/25 16:05:07, Xianzhu wrote:
> > On 2013/03/23 00:47:32, enne wrote:
> > > I think this is wrong.  The previous check was if the raster source scale
> > > changed which is not the same as raster contents scale.
> > 
> > I'm confused. How did the previous check deal with changes of raster source
> > scale?
> 
> It seems like you're just getting rid of raster source (i.e. CSS) scale here?
> 
> The "contents scale" is meant to be the product of the source*page*device
> scales.

Yes. Because I think the change can save some computations of
raster_contents_scale from source*page*device. It seems unnecessary to store
both contents scale and source scale.

Previously the raster_contents_scale computed by CalculateRasterContentsScale is
sometimes inconsistent with raster_source_scale, causing the incorrect value
returned from AreVisibleResourcesReady for PictureImageLayerImpl.

[danakj, 2013-03-25 16:55:50]

On Mon, Mar 25, 2013 at 12:51 PM, <wangxianzhu@chromium.org> wrote:

> On 2013/03/25 16:44:44, danakj wrote:
>
> https://codereview.chromium.**org/12954006/diff/2001/cc/**
>
layers/picture_layer_impl.cc<https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_impl.cc>
>
>> File cc/layers/picture_layer_impl.**cc (right):
>>
>
>
> https://codereview.chromium.**org/12954006/diff/2001/cc/**
>
layers/picture_layer_impl.cc#**newcode589<https://codereview.chromium.org/12954006/diff/2001/cc/layers/picture_layer_impl.cc#newcode589>
>
>> cc/layers/picture_layer_impl.**cc:589: if (!raster_page_scale_ ||
>> !raster_device_scale_ || !raster_contents_scale_)
>> On 2013/03/25 16:05:07, Xianzhu wrote:
>> > On 2013/03/23 00:47:32, enne wrote:
>> > > I think this is wrong.  The previous check was if the raster source
>> scale
>> > > changed which is not the same as raster contents scale.
>> >
>> > I'm confused. How did the previous check deal with changes of raster
>> source
>> > scale?
>>
>
>  It seems like you're just getting rid of raster source (i.e. CSS) scale
>> here?
>>
>
>  The "contents scale" is meant to be the product of the source*page*device
>> scales.
>>
>
> Yes. Because I think the change can save some computations of
> raster_contents_scale from source*page*device. It seems unnecessary to
> store
> both contents scale and source scale.
>
> Previously the raster_contents_scale computed by
> CalculateRasterContentsScale is
> sometimes inconsistent with raster_source_scale, causing the incorrect
> value
> returned from AreVisibleResourcesReady for PictureImageLayerImpl.
>

The code currently doesn't do much with the source scale, but it will be.
How will you track and adjust the contents scale based on the source scale
changing over time without keeping track of the source scale's value?

The indended model when I wrote this was: For each of {device, page,
source} scale, watch the current (ideal) value and compare to the  value
we're using (raster). When they diverge and some conditions are met to
allow changing, then reset all of the "raster" scales to equal the current
"ideal" scales. Rinse and repeat.

If source scale goes away, how will this work? (I'm also not totally sure
that CalculateRasterContentsScale should diverge - maybe that just means we
were computing the ideal source/page/device scale's wrong).


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

[Xianzhu, 2013-03-25 17:20:42]

> The code currently doesn't do much with the source scale, but it will be.
> How will you track and adjust the contents scale based on the source scale
> changing over time without keeping track of the source scale's value?
> 
> The indended model when I wrote this was: For each of {device, page,
> source} scale, watch the current (ideal) value and compare to the  value
> we're using (raster). When they diverge and some conditions are met to
> allow changing, then reset all of the "raster" scales to equal the current
> "ideal" scales. Rinse and repeat.
> 
> If source scale goes away, how will this work? (I'm also not totally sure
> that CalculateRasterContentsScale should diverge - maybe that just means we
> were computing the ideal source/page/device scale's wrong).
> 

I think we can calculate ideal_source_scale and raster_source_scale when needed
with contents/page/device, just like previously we can calculate
raster_contents_scale with source*page*device.

By storing raster_contents_scale instead of raster_source_scale, 4
source*page*device computations and 2 contents/page/device computations are
reduced. If future raster_source_scale would need more computations (>6), I'd
choose store raster_source_scale instead of raster_contents_scale. However, IMHO
we only need 2 such computations in ShouldAdjustRasterSourceScale().

Previously CalculateRasterSourceScale generated raster_contents_scale !=
raster_source_scale_ * page * device in the following cases:

1. when animating_transform_to_screen and ideal_contents_scale < 1
2. for scaled PictureImageLayerImpl which doesn't respect ideal_contents_scale
(so that image layers are scaled by GPU not skia)

Other versions of fixing the issue might be:
1) to ensure raster_source_scale_ be consistent with the raster_contents_scale
returned from CalculateRasterContentsScale by adding the following statement:
  raster_source_scale_ = raster_contents_scale / raster_page_scale_ /
ideal_device_scale_.
However, the low-res contents_scale is still inconsistent in
CleanUpTilingsOnActiveLayer.

2) call CalculateRasterContentsScale everytime when we need
raster_contents_scale (in AreVisibleResourcesAvailable and
CleanUpTilingsOnActiveLayer) to ensure the consistency. The problem is that the
animating_transform_to_screen parameter is not available in the cases.

[danakj, 2013-03-25 17:30:11]

On Mon, Mar 25, 2013 at 1:20 PM, <wangxianzhu@chromium.org> wrote:

> The code currently doesn't do much with the source scale, but it will be.
>> How will you track and adjust the contents scale based on the source scale
>> changing over time without keeping track of the source scale's value?
>>
>
>  The indended model when I wrote this was: For each of {device, page,
>> source} scale, watch the current (ideal) value and compare to the  value
>> we're using (raster). When they diverge and some conditions are met to
>> allow changing, then reset all of the "raster" scales to equal the current
>> "ideal" scales. Rinse and repeat.
>>
>
>  If source scale goes away, how will this work? (I'm also not totally sure
>> that CalculateRasterContentsScale should diverge - maybe that just means
>> we
>> were computing the ideal source/page/device scale's wrong).
>>
>
>
> I think we can calculate ideal_source_scale and raster_source_scale when
> needed
> with contents/page/device, just like previously we can calculate
> raster_contents_scale with source*page*device.
>
> By storing raster_contents_scale instead of raster_source_scale, 4
> source*page*device computations and 2 contents/page/device computations are
> reduced. If future raster_source_scale would need more computations (>6),
> I'd
> choose store raster_source_scale instead of raster_contents_scale.
> However, IMHO
> we only need 2 such computations in ShouldAdjustRasterSourceScale(**).
>
> Previously CalculateRasterSourceScale generated raster_contents_scale !=
> raster_source_scale_ * page * device in the following cases:
>
> 1. when animating_transform_to_screen and ideal_contents_scale < 1
> 2. for scaled PictureImageLayerImpl which doesn't respect
> ideal_contents_scale
> (so that image layers are scaled by GPU not skia)
>
> Other versions of fixing the issue might be:
> 1) to ensure raster_source_scale_ be consistent with the
> raster_contents_scale
> returned from CalculateRasterContentsScale by adding the following
> statement:
>   raster_source_scale_ = raster_contents_scale / raster_page_scale_ /
> ideal_device_scale_.
> However, the low-res contents_scale is still inconsistent in
> CleanUpTilingsOnActiveLayer.
>
> 2) call CalculateRasterContentsScale everytime when we need
> raster_contents_scale (in AreVisibleResourcesAvailable and
> CleanUpTilingsOnActiveLayer) to ensure the consistency. The problem is
> that the
> animating_transform_to_screen parameter is not available in the cases.
>

I see, thanks for the explanation. I ran into similar issues with the ideal
scales. At first I always recomputed the ideal contents scale from
source*page*device, but I could get slight floating point error, and differ
from the scale I wanted. I guess this is similar, just that the difference
is larger.

If the contents scale is not always source*page*device, because it can be
modified by other values - such as animating - then the source may also not
be content/page/device. What would you think of storing the raster contents
scale along with the source scale, instead of just replacing it?


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

[Xianzhu, 2013-03-25 17:35:44]

On 2013/03/25 17:30:11, danakj wrote:
> 
> I see, thanks for the explanation. I ran into similar issues with the ideal
> scales. At first I always recomputed the ideal contents scale from
> source*page*device, but I could get slight floating point error, and differ
> from the scale I wanted. I guess this is similar, just that the difference
> is larger.
> 
> If the contents scale is not always source*page*device, because it can be
> modified by other values - such as animating - then the source may also not
> be content/page/device. What would you think of storing the raster contents
> scale along with the source scale, instead of just replacing it?
>

So it's intentional that raster_source_scale_ doesn't equal to
raster_contents_scale/page/device during animation, right?

Will create a new version storing both raster_source_scale_ and
raster_contents_scale_.

[danakj, 2013-03-25 17:42:00]

On Mon, Mar 25, 2013 at 1:35 PM, <wangxianzhu@chromium.org> wrote:

> On 2013/03/25 17:30:11, danakj wrote:
>
>  I see, thanks for the explanation. I ran into similar issues with the
>> ideal
>> scales. At first I always recomputed the ideal contents scale from
>> source*page*device, but I could get slight floating point error, and
>> differ
>> from the scale I wanted. I guess this is similar, just that the difference
>> is larger.
>>
>
>  If the contents scale is not always source*page*device, because it can be
>> modified by other values - such as animating - then the source may also
>> not
>> be content/page/device. What would you think of storing the raster
>> contents
>> scale along with the source scale, instead of just replacing it?
>>
>
>
> So it's intentional that raster_source_scale_ doesn't equal to
> raster_contents_scale/page/**device during animation, right?
>

So in the CSS-animation case, that's a bit of a workaround right now. I
don't think that's the long-term behaviour intention there. It's really
just trying to avoid leaving the raster contents scale < 1 even though the
source scale is changing - because a changing source scale never triggers
us to reset the raster contents scale to source*page*device right now -
there's a bug open for that. In lovely future times, we would always set it
to source*page*device, because we'd allow the source scale changing to
trigger change_target_tiling - at a throttled rate.

For images, is the desired behaviour that the source, page, and device
scales all do not contribute to the contents scale?


> Will create a new version storing both raster_source_scale_ and
> raster_contents_scale_.
>
>
https://codereview.chromium.**org/12954006/<https://codereview.chromium.org/1...
>

[Xianzhu, 2013-03-25 18:00:51]

On 2013/03/25 17:42:00, danakj wrote:
> For images, is the desired behaviour that the source, page, and device
> scales all do not contribute to the contents scale?
> 

Yes. This is to avoid unnecessary re-rasterings. Image layers are rastered
strictly at 1:1 scale then GPU will scale them to the desired scale.

[enne (OOO), 2013-03-25 18:03:58]

Here's a suggestion:

Move the code from line 620-636 in PictureLayerImpl into
ShouldChangeRasterScale.  That way, PictureImageLayerImpl can just return false
but PictureLayerImpl can have a more complex decision about when to change its
raster scale.  In both cases you always want to change if page/device/source
scale are un-set.  I think that should fix the bug and leave the code mostly
unchanged.

[Xianzhu, 2013-03-25 18:45:58]

On 2013/03/25 18:03:58, enne wrote:
> Here's a suggestion:
> 
> Move the code from line 620-636 in PictureLayerImpl into
> ShouldChangeRasterScale.  That way, PictureImageLayerImpl can just return
false
> but PictureLayerImpl can have a more complex decision about when to change its
> raster scale.  In both cases you always want to change if page/device/source
> scale are un-set.  I think that should fix the bug and leave the code mostly
> unchanged.

Adopted. PTAL.

[enne (OOO), 2013-03-25 19:45:55]

Thanks!  This lgtm.  danakj, do you want to take another look at this too?

[danakj, 2013-03-25 20:13:03]

LGTM+nits

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_image_l...
File cc/layers/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_image_l...
cc/layers/picture_image_layer_impl.cc:34: bool
PictureImageLayerImpl::ShouldAdjustRasterScale(bool) const {
please give the variable its name

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:610: raster_source_scale_was_animating_ =
animating_transform_to_screen;
Can you add a comment saying that this is setting the value for the next time
ManageTilings is called, since this is used in ShouldAdjustRasterScale()?

Since the place it's used is so far away now, this is less clear.

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.h:101: float ideal_contents_scale_;
nit: can you move this down below ideal_source_scale so it's more clear that we
are/aren't storing the same values for both? I had to look a few times to find
this.

[Xianzhu, 2013-03-25 20:42:23]

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_image_l...
File cc/layers/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_image_l...
cc/layers/picture_image_layer_impl.cc:34: bool
PictureImageLayerImpl::ShouldAdjustRasterScale(bool) const {
On 2013/03/25 20:13:03, danakj wrote:
> please give the variable its name

Done.

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:610: raster_source_scale_was_animating_ =
animating_transform_to_screen;
On 2013/03/25 20:13:03, danakj wrote:
> Can you add a comment saying that this is setting the value for the next time
> ManageTilings is called, since this is used in ShouldAdjustRasterScale()?
> 
> Since the place it's used is so far away now, this is less clear.

Done.

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/12954006/diff/17001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.h:101: float ideal_contents_scale_;
On 2013/03/25 20:13:03, danakj wrote:
> nit: can you move this down below ideal_source_scale so it's more clear that
we
> are/aren't storing the same values for both? I had to look a few times to find
> this.

Done.

[commit-bot: I haz the power, 2013-03-25 20:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12954006/23001

[commit-bot: I haz the power, 2013-03-26 07:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12954006/23001

[commit-bot: I haz the power, 2013-03-26 09:24:46]

Change committed as 190621