Fix bad scaling in TiledLayer

Fix bad scaling in TiledLayer

On the windows side (and likely chromeos for some devices) we can end
up in a situation where the pixel bounds is not an even multiple of the
scale factor. This means when TiledLayer calculates the scale it ends
up off.

BUG=416655, 416775
TEST=added test coverage
R=danakj@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/37c078080a860d121443e657eb4089b784469162

[sky, 2014-09-11 21:01:07]

I'm not sure this is the right fix. It fixes the bug I was seeing, but then some
tests break. Specifically:

    TiledLayerTest.NonIntegerContentsScaleIsNotDistortedDuringInvalidation
    TiledLayerTest.NonIntegerContentsScaleIsNotDistortedDuringPaint
    TiledLayerTest.VerifyUpdateRectWhenContentBoundsAreScaled

I am not familiar with this code, so you'll have to tell me what the right thing
is here.
Thanks!

[danakj, 2014-09-13 13:04:50]

https://codereview.chromium.org/567743003/diff/1/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/567743003/diff/1/cc/layers/tiled_layer.cc#new...
cc/layers/tiled_layer.cc:466: float width_scale = 1 /
draw_properties().contents_scale_x;
Ok ya, this would be correct, but you need to fill that last row/column of
pixels with something. Otherwise if the content doesn't fill the entire bitmap,
it'll be blending that last pixel with transparent or uninitialized memory.

This is where we do the same thing in impl-side painting:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...

Previously we dealt with that by scaling to fill the entire bitmap instead of
using the exact scale, and we wrote tests for that, so the tests will need to be
updated.

[sky, 2014-09-16 21:00:46]

Hopefully I'm on the right track here. I haven't updated tests yet, I'm waiting
to make sure the other changes look good before doing that.

[danakj, 2014-09-19 22:02:58]

Yep this is the right track. A bunch of naming changes to help make this more
clear, and there's some code you can remove.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:41: const gfx::Size& content_size,
layer_content_size

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:56: gfx::Rect layer_rect = paint_rect;
move down to where the rest of the computation of layer_rect happens

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:63: // texel (since the recording won't
cover it) and outside the last texel
s/recording/paint/

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:64: // (due to linear filtering when using
this texture).
this "outside the last texel" isn't needed here, because the bitmap is sized
exactly to the paint rect in BitmapContentLayerUpdater::PrepareToUpdate (see
DCHECKs above). in implside painting the right/bottom-most bitmaps are larger
than the paint rects which is why it does that.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:65: // The final texel of content may only
be partially covered by a
whitespace above to separate

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:68: const gfx::Rect content_rect =
gfx::Rect(content_size);
layer_content_rect

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:69: gfx::Rect deflated_content_rect =
content_rect;
deflated_layer_content_rect

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:70: deflated_content_rect.Inset(0, 0, 1,
1);
whitespace below to separate, make it clear the comment "this rect" is for this
block

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:71: if
(!content_rect.Contains(paint_rect)) {
deflated_content_rect

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:72: // Drawing at most 2 x 2 x (canvas
width + canvas height) texels is 2-3X
s/2/1/

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:73: // faster than clearing, so special
case this.
can you DCHECK_LE(paint_rect.right(), content_rect.right()) and same for bottom
(this is kinda doubling the DCHECKs above but explicitly showing why we don't
need to do the inflation here.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:75: gfx::Rect inflated_content_rect =
content_rect;
remove this just use the content_rect

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
File cc/resources/content_layer_updater.h (right):

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.h:35: // Paints the contents. |content_size|
size of the underlying layer.
layer's content space.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.h:36: // |paint_rect| bounds to paint. Both
|content_size| and |paint_rect| are in
bounds to paint in content space of the layer.

[sky, 2014-09-22 18:00:00]

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:41: const gfx::Size& content_size,
On 2014/09/19 22:02:58, danakj wrote:
> layer_content_size

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:56: gfx::Rect layer_rect = paint_rect;
On 2014/09/19 22:02:57, danakj wrote:
> move down to where the rest of the computation of layer_rect happens

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:63: // texel (since the recording won't
cover it) and outside the last texel
On 2014/09/19 22:02:57, danakj wrote:
> s/recording/paint/

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:64: // (due to linear filtering when using
this texture).
On 2014/09/19 22:02:57, danakj wrote:
> this "outside the last texel" isn't needed here, because the bitmap is sized
> exactly to the paint rect in BitmapContentLayerUpdater::PrepareToUpdate (see
> DCHECKs above). in implside painting the right/bottom-most bitmaps are larger
> than the paint rects which is why it does that.

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:65: // The final texel of content may only
be partially covered by a
On 2014/09/19 22:02:57, danakj wrote:
> whitespace above to separate

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:68: const gfx::Rect content_rect =
gfx::Rect(content_size);
On 2014/09/19 22:02:57, danakj wrote:
> layer_content_rect

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:69: gfx::Rect deflated_content_rect =
content_rect;
On 2014/09/19 22:02:58, danakj wrote:
> deflated_layer_content_rect

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:70: deflated_content_rect.Inset(0, 0, 1,
1);
On 2014/09/19 22:02:57, danakj wrote:
> whitespace below to separate, make it clear the comment "this rect" is for
this
> block

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:71: if
(!content_rect.Contains(paint_rect)) {
On 2014/09/19 22:02:57, danakj wrote:
> deflated_content_rect

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:72: // Drawing at most 2 x 2 x (canvas
width + canvas height) texels is 2-3X
On 2014/09/19 22:02:58, danakj wrote:
> s/2/1/

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:73: // faster than clearing, so special
case this.
On 2014/09/19 22:02:58, danakj wrote:
> can you DCHECK_LE(paint_rect.right(), content_rect.right()) and same for
bottom
> (this is kinda doubling the DCHECKs above but explicitly showing why we don't
> need to do the inflation here.

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.cc:75: gfx::Rect inflated_content_rect =
content_rect;
On 2014/09/19 22:02:57, danakj wrote:
> remove this just use the content_rect

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
File cc/resources/content_layer_updater.h (right):

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.h:35: // Paints the contents. |content_size|
size of the underlying layer.
On 2014/09/19 22:02:58, danakj wrote:
> layer's content space.

Done.

https://codereview.chromium.org/567743003/diff/60001/cc/resources/content_lay...
cc/resources/content_layer_updater.h:36: // |paint_rect| bounds to paint. Both
|content_size| and |paint_rect| are in
On 2014/09/19 22:02:58, danakj wrote:
> bounds to paint in content space of the layer.

Done.

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (left):

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:866: TEST_F(TiledLayerTest,
VerifyUpdateRectWhenContentBoundsAreScaled) {
This test didn't seem worth keeping around.

[danakj, 2014-09-22 19:21:19]

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (left):

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:866: TEST_F(TiledLayerTest,
VerifyUpdateRectWhenContentBoundsAreScaled) {
On 2014/09/22 18:00:00, sky wrote:
> This test didn't seem worth keeping around.

This is testing the Layer::update_rect_ is in layer space, which should still be
true and is kind of orthogonal to this change. It's verifying
InvalidateContentRect gives a scaled-to-layer-space rect up to the base class.

Did this fail with your change? I think this should stick around.

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1669: EXPECT_RECT_NEAR(
Can you instead EXPECT_EQ and just explain in a comment the math for why the
result is what it is? NEAR could go either way and is less clear.

I don't actually see why the rects in layer space would be different. We don't
ask blink to paint anything larger due to this change. We just clear some pixels
in content space.

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1706: EXPECT_RECT_NEAR(
Same comment.

[sky, 2014-09-22 21:23:55]

Updated.

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (left):

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:866: TEST_F(TiledLayerTest,
VerifyUpdateRectWhenContentBoundsAreScaled) {
On 2014/09/22 19:21:18, danakj wrote:
> On 2014/09/22 18:00:00, sky wrote:
> > This test didn't seem worth keeping around.
> 
> This is testing the Layer::update_rect_ is in layer space, which should still
be
> true and is kind of orthogonal to this change. It's verifying
> InvalidateContentRect gives a scaled-to-layer-space rect up to the base class.
> 
> Did this fail with your change? I think this should stick around.

Yes, because it didn't update the scales explicitly. I changed it, and changed
the numbers to be easy to deal with (eg not 2/3).

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/567743003/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1669: EXPECT_RECT_NEAR(
On 2014/09/22 19:21:18, danakj wrote:
> Can you instead EXPECT_EQ and just explain in a comment the math for why the
> result is what it is? NEAR could go either way and is less clear.
> 
> I don't actually see why the rects in layer space would be different. We don't
> ask blink to paint anything larger due to this change. We just clear some
pixels
> in content space.

It's the 1.5 scale. Previously we calculated the scale based on bitmaps which
gave a slightly different scale. Now that we honor the exact scale we get a
different number here.

[danakj, 2014-09-22 21:55:05]

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
cc/resources/content_layer_updater.cc:91: layer_rect =
gfx::ScaleToEnclosingRect(
Oh I see, this layer_rect can be larger than the layer's bounds, which breaks
existing behaviour/tests.

What happens in views if cc asks it to paint a 31px DIP layer with a rect that's
32px tall?

If that sounds concerning you can maintain the existing behaviour by
intersecting with the layer bounds.

FWIW The layer bounds would be gfx::ScaleToEnclosedRect(layer_content_rect, 1.f
/ contents_scales..))

[sky, 2014-09-22 21:56:31]

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
cc/resources/content_layer_updater.cc:91: layer_rect =
gfx::ScaleToEnclosingRect(
On 2014/09/22 21:55:05, danakj wrote:
> Oh I see, this layer_rect can be larger than the layer's bounds, which breaks
> existing behaviour/tests.
> 
> What happens in views if cc asks it to paint a 31px DIP layer with a rect
that's
> 32px tall?
> 
> If that sounds concerning you can maintain the existing behaviour by
> intersecting with the layer bounds.
> 
> FWIW The layer bounds would be gfx::ScaleToEnclosedRect(layer_content_rect,
1.f
> / contents_scales..))

I don't believe it'll impact views at all.

[danakj, 2014-09-22 22:00:09]

LGTM

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
cc/resources/content_layer_updater.cc:91: layer_rect =
gfx::ScaleToEnclosingRect(
On 2014/09/22 21:56:30, sky wrote:
> On 2014/09/22 21:55:05, danakj wrote:
> > Oh I see, this layer_rect can be larger than the layer's bounds, which
breaks
> > existing behaviour/tests.
> > 
> > What happens in views if cc asks it to paint a 31px DIP layer with a rect
> that's
> > 32px tall?
> > 
> > If that sounds concerning you can maintain the existing behaviour by
> > intersecting with the layer bounds.
> > 
> > FWIW The layer bounds would be gfx::ScaleToEnclosedRect(layer_content_rect,
> 1.f
> > / contents_scales..))
> 
> I don't believe it'll impact views at all.

OK, can you please add a comment here that this rect may exceed the bounds of
the layer being painted?

[danakj, 2014-09-22 22:00:34]

Mind filing a BUG and attaching it to this CL so we can track it?

[sky, 2014-09-22 22:15:28]

And I created a bug.

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
File cc/resources/content_layer_updater.cc (right):

https://codereview.chromium.org/567743003/diff/120001/cc/resources/content_la...
cc/resources/content_layer_updater.cc:91: layer_rect =
gfx::ScaleToEnclosingRect(
On 2014/09/22 22:00:08, danakj wrote:
> On 2014/09/22 21:56:30, sky wrote:
> > On 2014/09/22 21:55:05, danakj wrote:
> > > Oh I see, this layer_rect can be larger than the layer's bounds, which
> breaks
> > > existing behaviour/tests.
> > > 
> > > What happens in views if cc asks it to paint a 31px DIP layer with a rect
> > that's
> > > 32px tall?
> > > 
> > > If that sounds concerning you can maintain the existing behaviour by
> > > intersecting with the layer bounds.
> > > 
> > > FWIW The layer bounds would be
gfx::ScaleToEnclosedRect(layer_content_rect,
> > 1.f
> > > / contents_scales..))
> > 
> > I don't believe it'll impact views at all.
> 
> OK, can you please add a comment here that this rect may exceed the bounds of
> the layer being painted?

Done.

[sky, 2014-09-22 22:15:40]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-09-22 22:16:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567743003/140001

[commit-bot: I haz the power, 2014-09-23 00:38:43]

Committed patchset #8 (id:140001) as 26611dacdeafd1d4f1845e50d76e13b3cc2c26ce

[commit-bot: I haz the power, 2014-09-23 00:39:26]

Patchset 8 (id:??) landed as
https://crrev.com/c01746b4f7d811a5fdc75ce6570a9667b9877913
Cr-Commit-Position: refs/heads/master@{#296108}

[Mads Ager (chromium), 2014-09-23 08:28:57]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/590313004/ by ager@chromium.org.

The reason for reverting is: This break Blink layout tests. See
crbug.com/416775..

[sky, 2014-09-23 21:24:33]

I'm going to reland this. The reason this was reverted was because a couple of
layout tests broke. They broke because of differences in the rounding. They need
to be rebaselined.

[danakj, 2014-09-23 21:25:34]

On 2014/09/23 21:24:33, sky wrote:
> I'm going to reland this. The reason this was reverted was because a couple of
> layout tests broke. They broke because of differences in the rounding. They
need
> to be rebaselined.

Can you land a change to TestExpectations with NeedsManualRebaseline before this
lands. Then change them to NeedsRebaseline after?

[commit-bot: I haz the power, 2014-09-23 21:26:05]

Patchset 9 (id:??) landed as
https://crrev.com/37c078080a860d121443e657eb4089b784469162
Cr-Commit-Position: refs/heads/master@{#296252}

[sky, 2014-09-23 21:26:06]

Committed patchset #9 (id:160001) manually as 37c0780 (presubmit successful).