Implement cc::FilterOperations::MapRect.

Implement cc::FilterOperations::MapRect.

Unlike GetOutsets(), this is flexible enough to represent other kinds of
mapping between filter source and destination rect, like scaling.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/29dab8e7a907cd5db664581f0543d169f2650329
Cr-Commit-Position: refs/heads/master@{#386801}

[jbroman, 2016-04-07 17:56:30]

Description was changed from

==========
Implement cc::FilterOperations::MapRect.

Unlike GetOutsets(), this is flexible enough to represent other kinds of
mapping between filter source and destination rect, like scaling.

BUG=600821
==========

to

==========
Implement cc::FilterOperations::MapRect.

Unlike GetOutsets(), this is flexible enough to represent other kinds of
mapping between filter source and destination rect, like scaling.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbroman, 2016-04-07 18:20:15]

jbroman@chromium.org changed reviewers:
+ senorblanco@chromium.org, weiliangc@chromium.org

[jbroman, 2016-04-07 18:20:16]

This is a first cc CL counterpart to
https://codereview.chromium.org/1855303002/

This implements the forward direction of a "mapRect" method, which
maps from a rectangle in the filter source to the corresponding rectangle
in the filter output which can be affected by those pixels (conservatively).

This is the same way Skia expresses it (SkImageFilter::filterBounds), and
it's more flexible than outsets, in that it can express scales and other
effects.

The ultimate objective is to fully replace FilterOperations::GetOutsets.

[jbroman, 2016-04-08 18:22:22]

On 2016/04/07 at 18:20:16, jbroman wrote:
> This is a first cc CL counterpart to
> https://codereview.chromium.org/1855303002/
> 
> This implements the forward direction of a "mapRect" method, which
> maps from a rectangle in the filter source to the corresponding rectangle
> in the filter output which can be affected by those pixels (conservatively).
> 
> This is the same way Skia expresses it (SkImageFilter::filterBounds), and
> it's more flexible than outsets, in that it can express scales and other
> effects.
> 
> The ultimate objective is to fully replace FilterOperations::GetOutsets.

ping

[weiliangc, 2016-04-08 18:30:32]

LGTM

[jbroman, 2016-04-11 17:35:20]

If senorblanco has no objections, I'll CQ this later today.

[Stephen White, 2016-04-11 18:05:00]

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operation.cc:351: SkIRect out_rect =
image_filter()->filterBounds(in_rect, SkMatrix::I());
SkImageFilter::filterBounds() defaults to the reverse mapping, but your comment
in the header states that this function is intended to do forward mapping. Which
is correct?

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations.h (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations.h:57: void GetOutsets(int* top, int* right, int*
bottom, int* left) const;
Do we still need this?

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:58: // TODO(hendrikw): We need to make
outsets for reference filters be in line
Is this comment still valid? If not, remove it.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:67: EXPECT_EQ(gfx::Rect(-15, -35, 34,
64), ops.MapRect(gfx::Rect(0, 0, 10, 10)));
This does look like a reverse mapping. Maybe the code was wrong?

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:86: EXPECT_EQ(gfx::Rect(0, 0, 10, 10),
ops.MapRect(gfx::Rect(0, 0, 10, 10)));
Could you add a test for a non-null Reference filter, e.g., an
SkOffsetImageFilter?

[jbroman, 2016-04-11 20:21:52]

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operation.cc:351: SkIRect out_rect =
image_filter()->filterBounds(in_rect, SkMatrix::I());
On 2016/04/11 at 18:05:00, Stephen White wrote:
> SkImageFilter::filterBounds() defaults to the reverse mapping, but your
comment in the header states that this function is intended to do forward
mapping. Which is correct?

Good catch. What's troubling to me is that GetOutsets seems to be used
principally for forward mapping, but GetOutsets called
SkImageFilter::filterBounds this way (i.e. with reverse mapping), which is why
my unit test passed. (I only checked that it matched what GetOutsets was doing).

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations.h (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations.h:57: void GetOutsets(int* top, int* right, int*
bottom, int* left) const;
On 2016/04/11 at 18:05:00, Stephen White wrote:
> Do we still need this?

Yes. None of the call sites have been rewritten yet; all of them will need to be
to delete this.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:58: // TODO(hendrikw): We need to make
outsets for reference filters be in line
On 2016/04/11 at 18:05:00, Stephen White wrote:
> Is this comment still valid? If not, remove it.

I'm not entirely clear on what it means, so I figured I'd better not let it get
removed.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:67: EXPECT_EQ(gfx::Rect(-15, -35, 34,
64), ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 at 18:05:00, Stephen White wrote:
> This does look like a reverse mapping. Maybe the code was wrong?

These expectations were taken from GetOutsets (with conversion to a rectangle).
See previous comment.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:86: EXPECT_EQ(gfx::Rect(0, 0, 10, 10),
ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 at 18:05:00, Stephen White wrote:
> Could you add a test for a non-null Reference filter, e.g., an
SkOffsetImageFilter?

Is the drop shadow reference filter (above) insufficient? I've mirrored the
existing tests. I'm happy to add one for SkOffsetImageFilter if you think that's
additionally useful.

[Stephen White, 2016-04-11 20:56:04]

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operation.cc:351: SkIRect out_rect =
image_filter()->filterBounds(in_rect, SkMatrix::I());
On 2016/04/11 20:21:52, jbroman wrote:
> On 2016/04/11 at 18:05:00, Stephen White wrote:
> > SkImageFilter::filterBounds() defaults to the reverse mapping, but your
> comment in the header states that this function is intended to do forward
> mapping. Which is correct?
> 
> Good catch. What's troubling to me is that GetOutsets seems to be used
> principally for forward mapping, but GetOutsets called
> SkImageFilter::filterBounds this way (i.e. with reverse mapping), which is why
> my unit test passed. (I only checked that it matched what GetOutsets was
doing).

Yeah, I think this was an existing bug. IIRC, this flavour of getOutsets() was
mostly used for background filter. I'm guessing there isn't a layout test that
would show the bug. We should probably add one.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:58: // TODO(hendrikw): We need to make
outsets for reference filters be in line
On 2016/04/11 20:21:52, jbroman wrote:
> On 2016/04/11 at 18:05:00, Stephen White wrote:
> > Is this comment still valid? If not, remove it.
> 
> I'm not entirely clear on what it means, so I figured I'd better not let it
get
> removed.

The referenced bug is basically what you're doing here: use real rect mapping,
not outsets (see my comment#2 on that bug). So I think you can remove it from
this flavour of the test.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:67: EXPECT_EQ(gfx::Rect(-15, -35, 34,
64), ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 20:21:52, jbroman wrote:
> On 2016/04/11 at 18:05:00, Stephen White wrote:
> > This does look like a reverse mapping. Maybe the code was wrong?
> 
> These expectations were taken from GetOutsets (with conversion to a
rectangle).
> See previous comment.

Yeah, I think there is/was a bug in GetOutsets().

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:86: EXPECT_EQ(gfx::Rect(0, 0, 10, 10),
ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 20:21:52, jbroman wrote:
> On 2016/04/11 at 18:05:00, Stephen White wrote:
> > Could you add a test for a non-null Reference filter, e.g., an
> SkOffsetImageFilter?
> 
> Is the drop shadow reference filter (above) insufficient? I've mirrored the
> existing tests. I'm happy to add one for SkOffsetImageFilter if you think
that's
> additionally useful.

The advantage of SkOffsetImageFilter is that it doesn't union with the original
primitive, so it tends to show bugs which don't show up in drop shadow (which
does).

[jbroman, 2016-04-12 19:32:37]

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:58: // TODO(hendrikw): We need to make
outsets for reference filters be in line
On 2016/04/11 at 20:56:04, Stephen White wrote:
> On 2016/04/11 20:21:52, jbroman wrote:
> > On 2016/04/11 at 18:05:00, Stephen White wrote:
> > > Is this comment still valid? If not, remove it.
> > 
> > I'm not entirely clear on what it means, so I figured I'd better not let it
get
> > removed.
> 
> The referenced bug is basically what you're doing here: use real rect mapping,
not outsets (see my comment#2 on that bug). So I think you can remove it from
this flavour of the test.

Done.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:67: EXPECT_EQ(gfx::Rect(-15, -35, 34,
64), ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 at 20:56:04, Stephen White wrote:
> On 2016/04/11 20:21:52, jbroman wrote:
> > On 2016/04/11 at 18:05:00, Stephen White wrote:
> > > This does look like a reverse mapping. Maybe the code was wrong?
> > 
> > These expectations were taken from GetOutsets (with conversion to a
rectangle).
> > See previous comment.
> 
> Yeah, I think there is/was a bug in GetOutsets().

Acknowledged.

https://codereview.chromium.org/1867913002/diff/20001/cc/output/filter_operat...
cc/output/filter_operations_unittest.cc:86: EXPECT_EQ(gfx::Rect(0, 0, 10, 10),
ops.MapRect(gfx::Rect(0, 0, 10, 10)));
On 2016/04/11 at 20:56:04, Stephen White wrote:
> On 2016/04/11 20:21:52, jbroman wrote:
> > On 2016/04/11 at 18:05:00, Stephen White wrote:
> > > Could you add a test for a non-null Reference filter, e.g., an
> > SkOffsetImageFilter?
> > 
> > Is the drop shadow reference filter (above) insufficient? I've mirrored the
> > existing tests. I'm happy to add one for SkOffsetImageFilter if you think
that's
> > additionally useful.
> 
> The advantage of SkOffsetImageFilter is that it doesn't union with the
original primitive, so it tends to show bugs which don't show up in drop shadow
(which does).

Done.

[Stephen White, 2016-04-12 19:47:00]

LGTM

[jbroman, 2016-04-12 19:51:01]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-04-12 19:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/1867913002/#ps60001
(title: "senorblanco review")

[commit-bot: I haz the power, 2016-04-12 19:51:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1867913002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1867913002/60001

[commit-bot: I haz the power, 2016-04-12 20:47:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-12 20:48:33]

Description was changed from

==========
Implement cc::FilterOperations::MapRect.

Unlike GetOutsets(), this is flexible enough to represent other kinds of
mapping between filter source and destination rect, like scaling.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::FilterOperations::MapRect.

Unlike GetOutsets(), this is flexible enough to represent other kinds of
mapping between filter source and destination rect, like scaling.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/29dab8e7a907cd5db664581f0543d169f2650329
Cr-Commit-Position: refs/heads/master@{#386801}
==========

[commit-bot: I haz the power, 2016-04-12 20:48:34]

Patchset 4 (id:??) landed as
https://crrev.com/29dab8e7a907cd5db664581f0543d169f2650329
Cr-Commit-Position: refs/heads/master@{#386801}