Implement HiDPI and pinch-zoom scaling of filter params

Implement HiDPI and pinch-zoom scaling of filter params

This change implements HiDPI scaling of filter params in CC. It also fixes content scaling of filter params (e.g.,
with -webkit-transform: scale(X)) as well as pinch-zoom scaling.

This code is mostly from http://crrev.com/191123002
   "Implement hidpi and pinch-zoom scaling of filter params in cc"
and replaces http://crrev.com/317663005
   "Scale the ALPHA_THRESHOLD filter's region based on the device scale factor"
because this fixes all filters rather than just the AlphaThresholdFilter.

BUG=376532, 281516, 281518, 349493
R=brettw@chromium.org, danakj@chromium.org, enne@chromium.org, jschuh@chromium.org

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

[garykac, 2014-07-17 00:52:53]

I spent some looking into making a useful RenderSurfaceLayerImplTest to verify
that AppendQuads was doing the right thing but encountered:

(1) Setting the device scale factor on the LayerTreeHostImpl didn't result in
the LayerImpl's contents_scale being set.

(2) The contents_scale and the draw_properties().target_space_transform were
always Inverses of each other when I tested with --open-ash and HiDPI. So the
RenderPass's quad's content_to_target_transform was always the Identity matrix.

I can verify that the transform.IsScale2d(), but that doesn't seem very useful
as a test.

Any thoughts on making a useful unittest?

[Stephen White, 2014-07-17 12:57:02]

On 2014/07/17 00:52:53, garykac wrote:
> I spent some looking into making a useful RenderSurfaceLayerImplTest to verify
> that AppendQuads was doing the right thing but encountered:
> 
> (1) Setting the device scale factor on the LayerTreeHostImpl didn't result in
> the LayerImpl's contents_scale being set.
> 
> (2) The contents_scale and the draw_properties().target_space_transform were
> always Inverses of each other when I tested with --open-ash and HiDPI. So the
> RenderPass's quad's content_to_target_transform was always the Identity
matrix.
> 
> I can verify that the transform.IsScale2d(), but that doesn't seem very useful
> as a test.
> 
> Any thoughts on making a useful unittest?

Dana, any ideas?

BTW, you'll need one of a reviewer from base/OWNERS for the base/ changes.

[danakj, 2014-07-17 15:04:26]

On 2014/07/17 12:57:02, Stephen White wrote:
> On 2014/07/17 00:52:53, garykac wrote:
> > I spent some looking into making a useful RenderSurfaceLayerImplTest to
verify
> > that AppendQuads was doing the right thing but encountered:
> > 
> > (1) Setting the device scale factor on the LayerTreeHostImpl didn't result
in
> > the LayerImpl's contents_scale being set.
> > 
> > (2) The contents_scale and the draw_properties().target_space_transform were
> > always Inverses of each other when I tested with --open-ash and HiDPI. So
the
> > RenderPass's quad's content_to_target_transform was always the Identity
> matrix.
> > 
> > I can verify that the transform.IsScale2d(), but that doesn't seem very
useful
> > as a test.
> > 
> > Any thoughts on making a useful unittest?
> 
> Dana, any ideas?

What I think you want is to write a test similar to those in
layer_tree_host_pixeltest_filters.cc, but..

(1) LayerImpl doesn't scale contents, so its contents_scale is always 1. In fact
contents_scale() is somewhat on its way out. You'll have to use a ContentLayer
to get a contents_scale() != 1. There are some tests in
layer_tree_host_pixeltest_readback.cc that do this so you can copy them.

(2) Ideally the contents_scale and target_space_transform are in fact inverses
of each other for layers that do scale their contents_scale() as that means they
have content 1:1 with the screen and it won't be blurry. For layers that do not
scale their contents_scale(), however, the draw_transform will scale. This is
because the draw_transform (ie target_space_transform) goes from content space
to the target, not from layer space. Factoring in the contents_scale() takes you
from layer->content->target space.

If you do one test with a ContentLayer it will have a non-1 contents_scale().
And if you do another test with a PictureLayer instead - they should look
similar otherwise - it will have a 1 contents_scale(). So that would cover both
types of scenarios.

I hope this is enough to get you some nice tests :)

> BTW, you'll need one of a reviewer from base/OWNERS for the base/ changes.

That's cc/base/ not base/ :)

[Stephen White, 2014-07-17 15:19:04]

On 2014/07/17 15:04:26, danakj wrote:
> On 2014/07/17 12:57:02, Stephen White wrote:
> > On 2014/07/17 00:52:53, garykac wrote:
> > > I spent some looking into making a useful RenderSurfaceLayerImplTest to
> verify
> > > that AppendQuads was doing the right thing but encountered:
> > > 
> > > (1) Setting the device scale factor on the LayerTreeHostImpl didn't result
> in
> > > the LayerImpl's contents_scale being set.
> > > 
> > > (2) The contents_scale and the draw_properties().target_space_transform
were
> > > always Inverses of each other when I tested with --open-ash and HiDPI. So
> the
> > > RenderPass's quad's content_to_target_transform was always the Identity
> > matrix.
> > > 
> > > I can verify that the transform.IsScale2d(), but that doesn't seem very
> useful
> > > as a test.
> > > 
> > > Any thoughts on making a useful unittest?
> > 
> > Dana, any ideas?
> 
> What I think you want is to write a test similar to those in
> layer_tree_host_pixeltest_filters.cc, but..
> 
> (1) LayerImpl doesn't scale contents, so its contents_scale is always 1. In
fact
> contents_scale() is somewhat on its way out. You'll have to use a ContentLayer
> to get a contents_scale() != 1. There are some tests in
> layer_tree_host_pixeltest_readback.cc that do this so you can copy them.
> 
> (2) Ideally the contents_scale and target_space_transform are in fact inverses
> of each other for layers that do scale their contents_scale() as that means
they
> have content 1:1 with the screen and it won't be blurry. For layers that do
not
> scale their contents_scale(), however, the draw_transform will scale. This is
> because the draw_transform (ie target_space_transform) goes from content space
> to the target, not from layer space. Factoring in the contents_scale() takes
you
> from layer->content->target space.
> 
> If you do one test with a ContentLayer it will have a non-1 contents_scale().
> And if you do another test with a PictureLayer instead - they should look
> similar otherwise - it will have a 1 contents_scale(). So that would cover
both
> types of scenarios.
> 
> I hope this is enough to get you some nice tests :)
> 
> > BTW, you'll need one of a reviewer from base/OWNERS for the base/ changes.
> 
> That's cc/base/ not base/ :)

So it is! These are not the droids you are looking for. You are free to go about
your business.

[garykac, 2014-07-17 17:01:54]

On 2014/07/17 15:19:04, Stephen White wrote:
> On 2014/07/17 15:04:26, danakj wrote:
> > On 2014/07/17 12:57:02, Stephen White wrote:
> > > On 2014/07/17 00:52:53, garykac wrote:
> > > > I spent some looking into making a useful RenderSurfaceLayerImplTest to
> > verify
> > > > that AppendQuads was doing the right thing but encountered:
> > > > 
> > > > (1) Setting the device scale factor on the LayerTreeHostImpl didn't
result
> > in
> > > > the LayerImpl's contents_scale being set.
> > > > 
> > > > (2) The contents_scale and the draw_properties().target_space_transform
> were
> > > > always Inverses of each other when I tested with --open-ash and HiDPI.
So
> > the
> > > > RenderPass's quad's content_to_target_transform was always the Identity
> > > matrix.
> > > > 
> > > > I can verify that the transform.IsScale2d(), but that doesn't seem very
> > useful
> > > > as a test.
> > > > 
> > > > Any thoughts on making a useful unittest?
> > > 
> > > Dana, any ideas?
> > 
> > What I think you want is to write a test similar to those in
> > layer_tree_host_pixeltest_filters.cc, but..
> > 
> > (1) LayerImpl doesn't scale contents, so its contents_scale is always 1. In
> fact
> > contents_scale() is somewhat on its way out. You'll have to use a
ContentLayer
> > to get a contents_scale() != 1. There are some tests in
> > layer_tree_host_pixeltest_readback.cc that do this so you can copy them.
> > 
> > (2) Ideally the contents_scale and target_space_transform are in fact
inverses
> > of each other for layers that do scale their contents_scale() as that means
> they
> > have content 1:1 with the screen and it won't be blurry. For layers that do
> not
> > scale their contents_scale(), however, the draw_transform will scale. This
is
> > because the draw_transform (ie target_space_transform) goes from content
space
> > to the target, not from layer space. Factoring in the contents_scale() takes
> you
> > from layer->content->target space.
> > 
> > If you do one test with a ContentLayer it will have a non-1
contents_scale().
> > And if you do another test with a PictureLayer instead - they should look
> > similar otherwise - it will have a 1 contents_scale(). So that would cover
> both
> > types of scenarios.
> > 
> > I hope this is enough to get you some nice tests :)
> > 
> > > BTW, you'll need one of a reviewer from base/OWNERS for the base/ changes.
> > 
> > That's cc/base/ not base/ :)
> 
> So it is! These are not the droids you are looking for. You are free to go
about
> your business.

content/common/cc_messages.h needs someone else to signoff, but I was going to
wait until any other issues were resolved to avoid spamming them.

[danakj, 2014-07-17 17:03:59]

On Thu, Jul 17, 2014 at 1:01 PM, <garykac@chromium.org> wrote:

> On 2014/07/17 15:19:04, Stephen White wrote:
>
>> On 2014/07/17 15:04:26, danakj wrote:
>> > On 2014/07/17 12:57:02, Stephen White wrote:
>> > > On 2014/07/17 00:52:53, garykac wrote:
>> > > > I spent some looking into making a useful
>> RenderSurfaceLayerImplTest to
>> > verify
>> > > > that AppendQuads was doing the right thing but encountered:
>> > > >
>> > > > (1) Setting the device scale factor on the LayerTreeHostImpl didn't
>>
> result
>
>> > in
>> > > > the LayerImpl's contents_scale being set.
>> > > >
>> > > > (2) The contents_scale and the draw_properties().target_
>> space_transform
>> were
>> > > > always Inverses of each other when I tested with --open-ash and
>> HiDPI.
>>
> So
>
>> > the
>> > > > RenderPass's quad's content_to_target_transform was always the
>> Identity
>> > > matrix.
>> > > >
>> > > > I can verify that the transform.IsScale2d(), but that doesn't seem
>> very
>> > useful
>> > > > as a test.
>> > > >
>> > > > Any thoughts on making a useful unittest?
>> > >
>> > > Dana, any ideas?
>> >
>> > What I think you want is to write a test similar to those in
>> > layer_tree_host_pixeltest_filters.cc, but..
>> >
>> > (1) LayerImpl doesn't scale contents, so its contents_scale is always
>> 1. In
>> fact
>> > contents_scale() is somewhat on its way out. You'll have to use a
>>
> ContentLayer
>
>> > to get a contents_scale() != 1. There are some tests in
>> > layer_tree_host_pixeltest_readback.cc that do this so you can copy
>> them.
>> >
>> > (2) Ideally the contents_scale and target_space_transform are in fact
>>
> inverses
>
>> > of each other for layers that do scale their contents_scale() as that
>> means
>> they
>> > have content 1:1 with the screen and it won't be blurry. For layers
>> that do
>> not
>> > scale their contents_scale(), however, the draw_transform will scale.
>> This
>>
> is
>
>> > because the draw_transform (ie target_space_transform) goes from content
>>
> space
>
>> > to the target, not from layer space. Factoring in the contents_scale()
>> takes
>> you
>> > from layer->content->target space.
>> >
>> > If you do one test with a ContentLayer it will have a non-1
>>
> contents_scale().
>
>> > And if you do another test with a PictureLayer instead - they should
>> look
>> > similar otherwise - it will have a 1 contents_scale(). So that would
>> cover
>> both
>> > types of scenarios.
>> >
>> > I hope this is enough to get you some nice tests :)
>> >
>> > > BTW, you'll need one of a reviewer from base/OWNERS for the base/
>> changes.
>> >
>> > That's cc/base/ not base/ :)
>>
>
>  So it is! These are not the droids you are looking for. You are free to go
>>
> about
>
>> your business.
>>
>
> content/common/cc_messages.h needs someone else to signoff, but I was
> going to
> wait until any other issues were resolved to avoid spamming them.
>

Yes that needs security team review.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-07-25 17:58:17]

+enne since dana is on vacation.

Basic unittests have been added.

[enne (OOO), 2014-07-25 22:45:51]

https://codereview.chromium.org/394193003/diff/20001/cc/layers/render_surface...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/394193003/diff/20001/cc/layers/render_surface...
cc/layers/render_surface_impl.cc:232:
owning_layer_to_target_scale.PreconcatTransform(
Do you really need the full transform multiplication here? It seems like you
could do target_space_transform.Scale2d() and then scale by contents_scale_x/y?

[garykac, 2014-07-29 17:10:01]

https://codereview.chromium.org/394193003/diff/20001/cc/layers/render_surface...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/394193003/diff/20001/cc/layers/render_surface...
cc/layers/render_surface_impl.cc:232:
owning_layer_to_target_scale.PreconcatTransform(
On 2014/07/25 22:45:51, enne wrote:
> Do you really need the full transform multiplication here? It seems like you
> could do target_space_transform.Scale2d() and then scale by
contents_scale_x/y?

Done.

[enne (OOO), 2014-07-29 17:20:58]

lgtm, thanks!

[garykac, 2014-07-29 21:47:50]

+jln for content/common/cc_messages.h

[jln (very slow on Chromium), 2014-07-29 21:57:19]

On 2014/07/29 21:47:50, garykac wrote:
> +jln for content/common/cc_messages.h

To speed the review, could you point me to the code that does deserialize
RenderPassDrawQuad and use it?

We should simply make sure that filters_scale is properly checked for sanity
(for instance to make sure that it doesn't allow an int overflow somewhere down
the stack).

[Stephen White, 2014-07-29 22:01:49]

Unfortunately, css3/filters/effect-reference-subregion-hidpi-hw.html (on
linux_blink_rel) is likely an actual failure.

Fortunately, this is a fairly niche use case, so you can just mark it as failing
in skia/skia_test_expectations.txt, and I'll take care of fixing it (I've logged
crbug.com/398618 for this). Something like the following should work:

# START OVERRIDES HERE

crbug.com/398618 css3/filters/effect-reference-subregion-hidpi-hw.html [ Failure
]

# END OVERRIDES HERE (this line ensures that the file is newline-terminated)

[garykac, 2014-08-04 23:08:27]

+cevans for cc_messages since jln is OOO

On 2014/07/29 21:57:19, jln (OOO til Aug 12) wrote:
> On 2014/07/29 21:47:50, garykac wrote:
> > +jln for content/common/cc_messages.h
> 
> To speed the review, could you point me to the code that does deserialize
> RenderPassDrawQuad and use it?

Unfortunately, I don't know enough about how these messages get passed to point
you to anything more than what codesearch reveals. It's possible that someone on
the cc team can provide more insight here.

> We should simply make sure that filters_scale is properly checked for sanity
> (for instance to make sure that it doesn't allow an int overflow somewhere
down
> the stack).

filters_scale is a Vector2dF, so it is 2 floats (therefore no int overflow). 
AFAICT via codesearch, these sort of sanity checks are not performed on any of
the other members.  I'm fine with adding them if necessary, but I'm not sure
where they would go.

On 2014/07/29 22:01:49, Stephen White wrote:
> Unfortunately, css3/filters/effect-reference-subregion-hidpi-hw.html (on
> linux_blink_rel) is likely an actual failure.
> 
> Fortunately, this is a fairly niche use case, so you can just mark it as
failing
> in skia/skia_test_expectations.txt, and I'll take care of fixing it (I've
logged
> crbug.com/398618 for this). Something like the following should work:
> 
> # START OVERRIDES HERE
> 
> crbug.com/398618 css3/filters/effect-reference-subregion-hidpi-hw.html [
Failure
> ]
> 
> # END OVERRIDES HERE (this line ensures that the file is newline-terminated)

I added that, but it is still being reported as a failure.  Could you
double-check my test-expectations.txt entry to see if I'm missing something?

[garykac, 2014-08-04 23:36:02]

On 2014/08/04 23:08:27, garykac wrote:
> On 2014/07/29 22:01:49, Stephen White wrote:
> > Unfortunately, css3/filters/effect-reference-subregion-hidpi-hw.html (on
> > linux_blink_rel) is likely an actual failure.
> 
> I added that, but it is still being reported as a failure.  Could you
> double-check my test-expectations.txt entry to see if I'm missing something?

Also, could the fix for this be as simple as removing the
BLINK_SCALE_FILTERS_AT_RECORD_TIME flag from common.gypi?

If that sounds reasonable (I don't fully understand the implications of that
flag), I'm game to try it out on the bots.

[danakj, 2014-08-05 13:38:48]

On 2014/07/29 21:57:19, jln (OOO til Aug 12) wrote:
> On 2014/07/29 21:47:50, garykac wrote:
> > +jln for content/common/cc_messages.h
> 
> To speed the review, could you point me to the code that does deserialize
> RenderPassDrawQuad and use it?

It's deserialized automatically via a STRUCT_TRAITS macro:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/cc_...

Which is used
here:https://code.google.com/p/chromium/codesearch#chromium/src/content/common/cc_messages.cc&l=399

It's used by this CL on line 704 of gl_renderer.cc to scale the SkCanvas:
https://codereview.chromium.org/394193003/diff/80001/cc/output/gl_renderer.cc

> We should simply make sure that filters_scale is properly checked for sanity
> (for instance to make sure that it doesn't allow an int overflow somewhere
down
> the stack).

[Stephen White, 2014-08-05 13:51:30]

On 2014/08/04 23:36:02, garykac wrote:
> On 2014/08/04 23:08:27, garykac wrote:
> > On 2014/07/29 22:01:49, Stephen White wrote:
> > > Unfortunately, css3/filters/effect-reference-subregion-hidpi-hw.html (on
> > > linux_blink_rel) is likely an actual failure.
> > 
> > I added that, but it is still being reported as a failure.  Could you
> > double-check my test-expectations.txt entry to see if I'm missing something?
> 
> Also, could the fix for this be as simple as removing the
> BLINK_SCALE_FILTERS_AT_RECORD_TIME flag from common.gypi?
> 
> If that sounds reasonable (I don't fully understand the implications of that
> flag), I'm game to try it out on the bots.

Ah yes, that's exactly it.

I had removed it in my original patch, but it seems that got lost along the way.
Redoing the removal should fix it.

[danakj, 2014-08-05 14:05:44]

LGTM w/ a few comments. Thanks for this!

https://codereview.chromium.org/394193003/diff/80001/cc/layers/render_surface...
File cc/layers/render_surface_impl_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/cc/layers/render_surface...
cc/layers/render_surface_impl_unittest.cc:72: class RenderSurfaceImplHiDpiTest :
public LayerTreePixelTest {
can you move this to one of the _pixeltest.cc suffix files? Probably
layer_tree_host_pixeltest_filters.cc.

https://codereview.chromium.org/394193003/diff/80001/cc/quads/draw_quad_unitt...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/cc/quads/draw_quad_unitt...
cc/quads/draw_quad_unittest.cc:384: #define CREATE_QUAD_9_NEW_RP(Type, a, b, c,
d, e, f, g, h, i, copy_a) \
You can delete the old unused macros that these replace.

https://codereview.chromium.org/394193003/diff/80001/cc/quads/render_pass_dra...
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/394193003/diff/80001/cc/quads/render_pass_dra...
cc/quads/render_pass_draw_quad.h:60: // This scale is applied to the filter
parameters for pixel-moving filters.
nit: don't \n after each .

https://codereview.chromium.org/394193003/diff/80001/content/common/cc_messag...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/content/common/cc_messag...
content/common/cc_messages_unittest.cc:161: EXPECT_EQ(a->filters_scale.x(),
b->filters_scale.x());
You can just compare |filters_scale| instead of each of the x and y components.

[garykac, 2014-08-05 21:17:50]

https://codereview.chromium.org/394193003/diff/80001/cc/layers/render_surface...
File cc/layers/render_surface_impl_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/cc/layers/render_surface...
cc/layers/render_surface_impl_unittest.cc:72: class RenderSurfaceImplHiDpiTest :
public LayerTreePixelTest {
On 2014/08/05 14:05:43, danakj wrote:
> can you move this to one of the _pixeltest.cc suffix files? Probably
> layer_tree_host_pixeltest_filters.cc.

Done.  Do you have a better name for the test than RenderSurfaceImplXxx?  It
seems odd to have the RenderSurfaceImpl test in the LayerTreehost unittest file.

https://codereview.chromium.org/394193003/diff/80001/cc/quads/draw_quad_unitt...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/cc/quads/draw_quad_unitt...
cc/quads/draw_quad_unittest.cc:384: #define CREATE_QUAD_9_NEW_RP(Type, a, b, c,
d, e, f, g, h, i, copy_a) \
On 2014/08/05 14:05:43, danakj wrote:
> You can delete the old unused macros that these replace.

Done.

https://codereview.chromium.org/394193003/diff/80001/cc/quads/render_pass_dra...
File cc/quads/render_pass_draw_quad.h (right):

https://codereview.chromium.org/394193003/diff/80001/cc/quads/render_pass_dra...
cc/quads/render_pass_draw_quad.h:60: // This scale is applied to the filter
parameters for pixel-moving filters.
On 2014/08/05 14:05:44, danakj wrote:
> nit: don't \n after each .

Done.

https://codereview.chromium.org/394193003/diff/80001/content/common/cc_messag...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/394193003/diff/80001/content/common/cc_messag...
content/common/cc_messages_unittest.cc:161: EXPECT_EQ(a->filters_scale.x(),
b->filters_scale.x());
On 2014/08/05 14:05:44, danakj wrote:
> You can just compare |filters_scale| instead of each of the x and y
components.

Done.

[garykac, 2014-08-05 21:20:22]

+brettw for build/config/BUILD.gn

There's also a reference to this flag in native_client/build/config/BUILD.gn,
but it's in the native_client repo so I can't remove it in this cl.

[danakj, 2014-08-05 21:47:25]

Thanks, a couple more things about the tests

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:164: class
RenderSurfaceImplHiDpiTest : public LayerTreePixelTest {
How about LayerTreeHostFiltersScaledPixelTest? Can you subclass from
LayerTreeHostFiltersPixelTest instead? (all tests in the file should)

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:212: void RunDpiTest(int
content_size, float device_scale_factor) {
How about moving the contents of this function that have to do with making
layers into SetupTree? (that's where I'd expect to find it)

then all that's left here is
device_scale_factor_ = device_scale_factor;
impl_side_painting_ = false;
RunPixelTest(...)

And you could just do those things in the TEST_F()'s below and drop this method
from the class.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:234: this->impl_side_painting_ =
false;
why do you do this? non-impl-side-painting is going away, do we need to do this
or was it copied from somewhere?

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:236: SOFTWARE_WITH_DEFAULT,
Can you add some GL variants of these tests? Like ImageFilterClipped_GL and
ImageFilterClipped_Software?

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:248:
TEST_F(RenderSurfaceImplHiDpiTest, TestStandardDpi) {
nit: Drop the "Test", it's cleaner.

[brettw, 2014-08-06 17:41:11]

build/config/BUILD.gn LGTM

[garykac, 2014-08-07 02:04:15]

cevans@ could you please take a look at cc_messages?

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:164: class
RenderSurfaceImplHiDpiTest : public LayerTreePixelTest {
On 2014/08/05 21:47:24, danakj wrote:
> How about LayerTreeHostFiltersScaledPixelTest? Can you subclass from
> LayerTreeHostFiltersPixelTest instead? (all tests in the file should)

Done.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:212: void RunDpiTest(int
content_size, float device_scale_factor) {
On 2014/08/05 21:47:24, danakj wrote:
> How about moving the contents of this function that have to do with making
> layers into SetupTree? (that's where I'd expect to find it)

The other pixel readback tests are set up this way and have a minimal
SetupTree().

Also, the bounds for each layer varies based on the test so it would be awkward
to do that in SetupTree.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:234: this->impl_side_painting_ =
false;
On 2014/08/05 21:47:24, danakj wrote:
> why do you do this? non-impl-side-painting is going away, do we need to do
this
> or was it copied from somewhere?

Modelled after LayerTreeHostReadbackDeviceScalePixelTest, which has this.
Removing it causes MatchesPNGFile() to fail. :-(

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:236: SOFTWARE_WITH_DEFAULT,
On 2014/08/05 21:47:24, danakj wrote:
> Can you add some GL variants of these tests? Like ImageFilterClipped_GL and
> ImageFilterClipped_Software?

Done. I added all 4 variants, which perhaps is overkill.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:248:
TEST_F(RenderSurfaceImplHiDpiTest, TestStandardDpi) {
On 2014/08/05 21:47:24, danakj wrote:
> nit: Drop the "Test", it's cleaner.

Done.

[danakj, 2014-08-07 13:15:30]

LGTM

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:212: void RunDpiTest(int
content_size, float device_scale_factor) {
On 2014/08/07 02:04:15, garykac wrote:
> On 2014/08/05 21:47:24, danakj wrote:
> > How about moving the contents of this function that have to do with making
> > layers into SetupTree? (that's where I'd expect to find it)
> 
> The other pixel readback tests are set up this way and have a minimal
> SetupTree().


Ok, I think that test is an outlier compared to most of our LayerTreeTests in
that way (it doesn't even have a SetupTree so I didn't think of it in that way,
but it's fine either way I guess.

> Also, the bounds for each layer varies based on the test so it would be
awkward
> to do that in SetupTree.

You'd do like:

content_size_ = 100;
device_scale_factor_ = 1.f;
RunPixelTest(SOFTWARE_...);

And then make layers with content_size_ instead of content_size?

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:234: this->impl_side_painting_ =
false;
On 2014/08/07 02:04:15, garykac wrote:
> On 2014/08/05 21:47:24, danakj wrote:
> > why do you do this? non-impl-side-painting is going away, do we need to do
> this
> > or was it copied from somewhere?
> 
> Modelled after LayerTreeHostReadbackDeviceScalePixelTest, which has this.
> Removing it causes MatchesPNGFile() to fail. :-(

Oh, darn. I will have to look into removing all of these, thanks for pointing
out that other test.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:236: SOFTWARE_WITH_DEFAULT,
On 2014/08/07 02:04:15, garykac wrote:
> On 2014/08/05 21:47:24, danakj wrote:
> > Can you add some GL variants of these tests? Like ImageFilterClipped_GL and
> > ImageFilterClipped_Software?
> 
> Done. I added all 4 variants, which perhaps is overkill.

Thanks! SOFTWARE_WITH_DEFAULT and SOFTWARE_WITH_BITMAP are technically the same
codepath, as long as we're hitting both the GL and the Software renderers then
we're good.

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

https://codereview.chromium.org/394193003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:264:
TEST_F(LayerTreeHostFiltersScaledPixelTest, StandardDpi_GlBitmap) {
nit: it's GL not Gl, we never lowercase the l

[danakj, 2014-08-07 14:03:18]

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:234: this->impl_side_painting_ =
false;
On 2014/08/07 13:15:29, danakj wrote:
> On 2014/08/07 02:04:15, garykac wrote:
> > On 2014/08/05 21:47:24, danakj wrote:
> > > why do you do this? non-impl-side-painting is going away, do we need to do
> > this
> > > or was it copied from somewhere?
> > 
> > Modelled after LayerTreeHostReadbackDeviceScalePixelTest, which has this.
> > Removing it causes MatchesPNGFile() to fail. :-(
> 
> Oh, darn. I will have to look into removing all of these, thanks for pointing
> out that other test.

If you use PictureLayer instead of ContentLayer and remove setting this flag to
false, it should "just work". :) https://codereview.chromium.org/443353003/

[garykac, 2014-08-07 19:06:09]

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:234: this->impl_side_painting_ =
false;
On 2014/08/07 14:03:18, danakj wrote:
> On 2014/08/07 13:15:29, danakj wrote:
> > On 2014/08/07 02:04:15, garykac wrote:
> > > On 2014/08/05 21:47:24, danakj wrote:
> > > > why do you do this? non-impl-side-painting is going away, do we need to
do
> > > this
> > > > or was it copied from somewhere?    content_size_ = content_size;

> > > 
> > > Modelled after LayerTreeHostReadbackDeviceScalePixelTest, which has this.
> > > Removing it causes MatchesPNGFile() to fail. :-(
> > 
> > Oh, darn. I will have to look into removing all of these, thanks for
pointing
> > out that other test.
> 
> If you use PictureLayer instead of ContentLayer and remove setting this flag
to
> false, it should "just work". :) https://codereview.chromium.org/443353003/

Ah yes... "just work" :-(

I experimented with making this change, but it resulted in the test failing
because the quad_state->content_bounds aren't adjusted by the DSF. :-(

I looked for another "magic setting" to make this work (something like
layer_transforms_should_scale_layer_contents), but didn't find anything.

I can follow up with a separate cl to update this test.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:236: SOFTWARE_WITH_DEFAULT,
On 2014/08/07 13:15:29, danakj wrote:
> On 2014/08/07 02:04:15, garykac wrote:
> > On 2014/08/05 21:47:24, danakj wrote:
> > > Can you add some GL variants of these tests? Like ImageFilterClipped_GL
and
> > > ImageFilterClipped_Software?
> > 
> > Done. I added all 4 variants, which perhaps is overkill.
> 
> Thanks! SOFTWARE_WITH_DEFAULT and SOFTWARE_WITH_BITMAP are technically the
same
> codepath, as long as we're hitting both the GL and the Software renderers then
> we're good.

CreateCopyOutputRequest (in l_t_h_pixeltest_readback) does CreateBitmapRequest
for *_BITMAP and CreateRequest for *_DEFAULT, so I assumed they might be
different. But I'm happy to remove one of them.

Curiously, in this code the SOFTWARE_WITH_DEFAULT case has:
   CreateRequest(...ReadbackResultAsBitmap...)
whereas I expected either:
   CreateRequest(...ReadbackResultAsTexture...)
or
   CreateBitmapRequest(...ReadbackResultAsBitmap...)

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

https://codereview.chromium.org/394193003/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:264:
TEST_F(LayerTreeHostFiltersScaledPixelTest, StandardDpi_GlBitmap) {
On 2014/08/07 13:15:29, danakj wrote:
> nit: it's GL not Gl, we never lowercase the l

Done.

[danakj, 2014-08-07 19:37:00]

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

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:190: RenderSurfaceImpl*
render_surface = layer_impl->render_surface();
I think I managed to somehow not pay as much attention to this test as I should
have while I've been trying to catch up from vacation, sorry :( This test is
doing some funny stuff and I'm not sure why it is doing so.

What I was thinking was that you had a layer with a Filter set on it and
verifying that it came out both at 1x and 2x scale correctly. But I don't see
you setting any filters on layers down below, and this code here is calling
things that are somewhat private and likely to change and not quite the right
way to be going about it.

If you look at some of the other tests in this file you'll see what I mean,
putting a filter on a layer and seeing what happens with the pixel output. Maybe
you can more or less half the sizes of things and set a DSF of 2 on one of these
existing tests, it might be slightly different pixels, so you can add a new png
if you need to.

https://codereview.chromium.org/394193003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:236: SOFTWARE_WITH_DEFAULT,
On 2014/08/07 19:06:08, garykac wrote:
> On 2014/08/07 13:15:29, danakj wrote:
> > On 2014/08/07 02:04:15, garykac wrote:
> > > On 2014/08/05 21:47:24, danakj wrote:
> > > > Can you add some GL variants of these tests? Like ImageFilterClipped_GL
> and
> > > > ImageFilterClipped_Software?
> > > 
> > > Done. I added all 4 variants, which perhaps is overkill.
> > 
> > Thanks! SOFTWARE_WITH_DEFAULT and SOFTWARE_WITH_BITMAP are technically the
> same
> > codepath, as long as we're hitting both the GL and the Software renderers
then
> > we're good.
> 
> CreateCopyOutputRequest (in l_t_h_pixeltest_readback) does CreateBitmapRequest
> for *_BITMAP and CreateRequest for *_DEFAULT, so I assumed they might be
> different. But I'm happy to remove one of them.
> 
> Curiously, in this code the SOFTWARE_WITH_DEFAULT case has:
>    CreateRequest(...ReadbackResultAsBitmap...)
> whereas I expected either:
>    CreateRequest(...ReadbackResultAsTexture...)
> or
>    CreateBitmapRequest(...ReadbackResultAsBitmap...)

In software compositing there's nothing to get except bitmaps. In GL mode you
could ask for either atm. They hit different paths in GL Renderer, but we have
tests for making sure they both work, for the readback. Otherwise we just want
to make sure both the GL and Software renderer are drawing the right things.

[garykac, 2014-08-07 21:19:57]

On 2014/08/07 19:37:00, danakj wrote:
> What I was thinking was that you had a layer with a Filter set on it and
> verifying that it came out both at 1x and 2x scale correctly. But I don't see
> you setting any filters on layers down below, and this code here is calling
> things that are somewhat private and likely to change and not quite the right
> way to be going about it.

Done!

And the new test is SO much nicer than the previous approach.

[garykac, 2014-08-07 21:36:21]

+jschuh for cc_messages.h

[jschuh, 2014-08-07 21:48:50]

ipc security lgtm (notes: float value that doesn't appear security impacting)

[danakj, 2014-08-07 21:59:19]

Cool this is great thank you very much. One question:

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

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:7: #include
"cc/layers/content_layer.h"
Some (all?) Of these aren't needed anymore

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:195: // everything except the
lower right corner.
Nice!

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:206:
background->SetMasksToBounds(true);
Does the test not fail without your patch without this? I'm not sure how this
would interact with the visible output here since its the whole viewport.

[garykac, 2014-08-07 22:51:52]

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

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:7: #include
"cc/layers/content_layer.h"
On 2014/08/07 21:59:18, danakj wrote:
> Some (all?) Of these aren't needed anymore

Done.

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:206:
background->SetMasksToBounds(true);
On 2014/08/07 21:59:19, danakj wrote:
> Does the test not fail without your patch without this? I'm not sure how this
> would interact with the visible output here since its the whole viewport. 

I hadn't tried it without it before. It's unnecessary, so I removed it (along
with the useless SetBackgroundFilters() call).

[garykac, 2014-08-07 22:52:06]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2014-08-07 23:00:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/garykac@chromium.org/394193003/180001

[danakj, 2014-08-07 23:12:17]

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

https://codereview.chromium.org/394193003/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_filters.cc:206:
background->SetMasksToBounds(true);
On 2014/08/07 22:51:52, garykac wrote:
> On 2014/08/07 21:59:19, danakj wrote:
> > Does the test not fail without your patch without this? I'm not sure how
this
> > would interact with the visible output here since its the whole viewport. 
> 
> I hadn't tried it without it before. It's unnecessary, so I removed it (along
> with the useless SetBackgroundFilters() call).

Great thanks!

[commit-bot: I haz the power, 2014-08-08 05:13:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 06:46:40]

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

[commit-bot: I haz the power, 2014-08-08 06:46:41]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[Stephen White, 2014-08-08 13:18:32]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-08-08 13:19:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/garykac@chromium.org/394193003/180001

[commit-bot: I haz the power, 2014-08-08 17:04:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[garykac, 2014-08-08 21:25:48]

Committed patchset #10 manually as 288436 (presubmit successful).