cc: Render to destination transform when rendering into a separate surface

cc: Render to destination transform when rendering into a separate surface

The existing code would render the separate surface render pass in source
space, rather than in target space, which means it would render small,
apply the filter, then scale up to target space, which caused scaling
artifacts.

I've updated the surface transforms to respect combined_transform_scales,
which are the x and y axis lengths of combined_transform.

I've also written a test that demonstrates the issue, and is fixed by
this cl.

BUG=411079
Committed: https://crrev.com/c493d268261dfbdda8efdc068b150c25e94905ae
Cr-Commit-Position: refs/heads/master@{#313167}

[hendrikw, 2015-01-16 22:34:01]

hendrikw@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, vollick@chromium.org

[hendrikw, 2015-01-16 22:34:01]

NOT FOR LANDING - yet!

PTAL and let me know if I'm approaching this wrong.

Firstly, I (or vollick) would need to make a similar change for property trees,
since the CHECK in LayerTreeHostCommon::CalculateDrawProperties is being hit.

I've updated two tests, the scale factor is 2, I think that now that we're
respecting rendering at target scale, the results need to be doubled, but maybe
I'm misunderstanding things.



LayerTreeHostCommonTest.ContentsScaleForSurfaces_LayerTransformsDontAffectContentsScale
is the only test failing after these changes.  I'm not quite sure if the test is
now wrong, or I've made an error.  Here's the output.  If you're familiar with
this test, let me know what you think, thanks!

d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5164): error:
Value of: surface_scale->render_surface()->draw_transform().matrix().get(0, 0)
  Actual: 1
Expected: initial_parent_scale * initial_child_scale
Which is: 6
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5167): error:
Value of: surface_scale->render_surface()->draw_transform().matrix().get(1, 1)
  Actual: 1
Expected: initial_parent_scale * initial_child_scale
Which is: 6
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5171): error:
Value of: surface_scale->draw_transform().matrix().get(0, 0)
  Actual: 6
Expected: 1.0
Which is: 1
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5172): error:
Value of: surface_scale->draw_transform().matrix().get(1, 1)
  Actual: 6
Expected: 1.0
Which is: 1
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5178): error:
Value of: surface_scale_child_scale->draw_transform().matrix().get(0, 0)
  Actual: 18
Expected: initial_child_scale
Which is: 3
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5181): error:
Value of: surface_scale_child_scale->draw_transform().matrix().get(1, 1)
  Actual: 18
Expected: initial_child_scale
Which is: 3
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5188): error:
Value of: surface_scale_child_no_scale->draw_transform().matrix().get(0, 0)
  Actual: 630
Expected: device_scale_factor * page_scale_factor * initial_child_scale
Which is: 105
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5191): error:
Value of: surface_scale_child_no_scale->draw_transform().matrix().get(1, 1)
  Actual: 630
Expected: device_scale_factor * page_scale_factor * initial_child_scale
Which is: 105
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5197): error:
Value of: surface_no_scale->render_surface()->draw_transform().matrix().get(0,
0)
  Actual: 1
Expected: initial_parent_scale * initial_child_scale
Which is: 6
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5200): error:
Value of: surface_no_scale->render_surface()->draw_transform().matrix().get(1,
1)
  Actual: 1
Expected: initial_parent_scale * initial_child_scale
Which is: 6
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5207): error:
Value of: surface_no_scale->draw_transform().matrix().get(0, 0)
  Actual: 210
Expected: device_scale_factor * page_scale_factor
Which is: 35
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5209): error:
Value of: surface_no_scale->draw_transform().matrix().get(1, 1)
  Actual: 210
Expected: device_scale_factor * page_scale_factor
Which is: 35
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5216): error:
Value of: surface_no_scale_child_scale->draw_transform().matrix().get(0, 0)
  Actual: 18
Expected: initial_child_scale
Which is: 3
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5219): error:
Value of: surface_no_scale_child_scale->draw_transform().matrix().get(1, 1)
  Actual: 18
Expected: initial_child_scale
Which is: 3
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5226): error:
Value of: surface_no_scale_child_no_scale->draw_transform().matrix().get(0, 0)
  Actual: 630
Expected: device_scale_factor * page_scale_factor * initial_child_scale
Which is: 105
d:\src\chrome\src\cc\trees\layer_tree_host_common_unittest.cc(5229): error:
Value of: surface_no_scale_child_no_scale->draw_transform().matrix().get(1, 1)
  Actual: 630
Expected: device_scale_factor * page_scale_factor * initial_child_scale
Which is: 105

[hendrikw, 2015-01-20 18:06:24]

I removed the asymmetry that vollick was concerned with, surprisingly all of the
tests still pass, even though it's potentially passing a different matrix to
render_surface->SetScreenSpaceTransform().

PTAL, thanks

[danakj, 2015-01-20 19:58:29]

https://codereview.chromium.org/846453005/diff/160001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:774: EXPECT_EQ(gfx::Rect(20,
20, 70, 70).ToString(),
Can you explain why the comments above these changes are still correct?

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1866: // instead of the draw_transform.
However, we do need to add a different
Needs updating

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:363: static float scale_filter =
1.5f;
did you mean static const? tbh just "float scale_filter" seems fine in tests. or
just pass 1.5, 1.5 to Scale() if you're not going to reuse it later in the test.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:368: SkIRect rects[2] =
{SkIRect::MakeLTRB(10, 10, 120, 120),
comment explaining these numbers please, and why this type of filter is used
instead of something simpler?

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:392: // TODO(hendrikw): Enable
test in software as well: crbug.com/432157
Why? This isn't a background filter. This is a foreground filter.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:395:
base::FilePath(FILE_PATH_LITERAL("alpha_filter_on_scaled_layer.png")));
can you add the png to the review?

[hendrikw, 2015-01-21 22:46:03]

Everything is working correctly now, PTAL.

Please note that I needed separate test result PNGs for software and hardware
since there is a very large difference on the AA'd edges.

Thanks!

https://codereview.chromium.org/846453005/diff/160001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl_unittest.cc:774: EXPECT_EQ(gfx::Rect(20,
20, 70, 70).ToString(),
On 2015/01/20 19:58:29, danakj wrote:
> Can you explain why the comments above these changes are still correct?

I've updated the comments.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1866: // instead of the draw_transform.
However, we do need to add a different
On 2015/01/20 19:58:29, danakj wrote:
> Needs updating

hmm, It sounds like this comment correctly sums up what's happening before and
after my change.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:363: static float scale_filter =
1.5f;
On 2015/01/20 19:58:29, danakj wrote:
> did you mean static const? tbh just "float scale_filter" seems fine in tests.
or
> just pass 1.5, 1.5 to Scale() if you're not going to reuse it later in the
test.

yeah, this was debug code so that I could change the value in the debugger
without a recompile.  Thanks for spotting it.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:368: SkIRect rects[2] =
{SkIRect::MakeLTRB(10, 10, 120, 120),
On 2015/01/20 19:58:29, danakj wrote:
> comment explaining these numbers please, and why this type of filter is used
> instead of something simpler?

I've added a comment.

The first test I wrote, I assumed it simulate the issues seen on the pixel
(which was wrong).  In this test I wanted to get as close to the real problem as
possible, so I'm using an alpha threshold filter with an irregularly shape.  It
still might not be the actual problem, but hopefully it is.

https://codereview.chromium.org/846453005/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:392: // TODO(hendrikw): Enable
test in software as well: crbug.com/432157
On 2015/01/20 19:58:29, danakj wrote:
> Why? This isn't a background filter. This is a foreground filter.

Done.

[danakj, 2015-01-22 01:13:45]

LGTM

https://codereview.chromium.org/846453005/diff/220001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/846453005/diff/220001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:376:
filter_layer->SetFilters(filters);
I would weakly prefer making this test a simpler repro by just
filter_layer->SetForceRenderSurface(true) instead of using an alpha transform,
which isn't related to scaling issues directly beyond being a render surface.

[hendrikw, 2015-01-22 22:00:10]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:02:00]

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

[commit-bot: I haz the power, 2015-01-22 22:24:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:24:44]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[hendrikw, 2015-01-22 22:31:36]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:36:23]

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

[commit-bot: I haz the power, 2015-01-23 00:25:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 00:25:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hendrikw, 2015-01-23 22:58:23]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-23 22:59:33]

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

[commit-bot: I haz the power, 2015-01-24 00:50:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:50:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2015-01-26 22:43:13]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-26 22:44:19]

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

[commit-bot: I haz the power, 2015-01-26 23:35:43]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-01-26 23:36:26]

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