cc: Implement and use FilterOperations::MapRectReverse for mapping backdrop bounding box.

cc: Implement and use FilterOperations::MapRectReverse for mapping backdrop bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ca9b612516efc2af1143c09a4d67f5b99e566c79
Cr-Commit-Position: refs/heads/master@{#402974}

[jbroman, 2016-05-20 14:49:18]

Description was changed from

==========
Implement and use cc::FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

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

to

==========
Implement and use cc::FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

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

[jbroman, 2016-05-26 15:18:10]

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

[jbroman, 2016-05-26 15:18:11]

PTAL.

[jbroman, 2016-06-01 16:07:31]

Description was changed from

==========
Implement and use cc::FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

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

to

==========
cc: Implement and use FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

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

[jbroman, 2016-06-01 16:07:41]

ping

[Stephen White, 2016-06-01 17:42:46]

Perhaps a backdrop-filter layout test with hiDPI might be useful here.

https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:800:
quad->background_filters.MapRectReverse(backdrop_rect, SkMatrix::I());
If these are device-space rects, shouldn't we be using filters_scale here?

https://codereview.chromium.org/1998233002/diff/20001/cc/output/software_rend...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/1998233002/diff/20001/cc/output/software_rend...
cc/output/software_renderer.cc:703:
quad->background_filters.MapRectReverse(backdrop_rect, SkMatrix::I());
And here?

[jbroman, 2016-06-01 19:41:20]

https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:800:
quad->background_filters.MapRectReverse(backdrop_rect, SkMatrix::I());
On 2016/06/01 at 17:42:46, Stephen White wrote:
> If these are device-space rects, shouldn't we be using filters_scale here?

I believe you're correct, which would make that an existing bug, as the
GetOutsets code didn't account for it either. Would you like that addressed in
this CL, or would a followup be good? (Or would you like such a fix to land
first?)

[Stephen White, 2016-06-01 22:14:08]

On 2016/06/01 19:41:20, jbroman wrote:
> https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/1998233002/diff/20001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:800:
> quad->background_filters.MapRectReverse(backdrop_rect, SkMatrix::I());
> On 2016/06/01 at 17:42:46, Stephen White wrote:
> > If these are device-space rects, shouldn't we be using filters_scale here?
> 
> I believe you're correct, which would make that an existing bug, as the
> GetOutsets code didn't account for it either. Would you like that addressed in
> this CL, or would a followup be good? (Or would you like such a fix to land
> first?)

It'd be nice to get it in this CL if you don't mind (with a test), since I'm
pretty certain that it's a bug.

[jbroman, 2016-06-07 14:30:43]

OK, PTAL (required fixing up a few other things).

I'm particularly uneasy about dealing with framebuffer flipping in GLRenderer.

I suspect there's a Y translation that's also required to be correct in general,
but I don't know what it is, and also I suspect this breaks the vector-mapping
approach for the gaussian blur radius further (because translating that vector
will wreak havok if we try to expand it; maybe we should instead be reading out
the scale components of the matrix or similar?).

[Stephen White, 2016-06-07 16:00:34]

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

https://codereview.chromium.org/1998233002/diff/80001/cc/output/filter_operat...
cc/output/filter_operation.cc:359: gfx::Rect
FilterOperation::MapRectReverse(const gfx::Rect& rect,
This looks awfully similar to the existing flavour above. Could we instead have
MapRect() take an (optional) MapDirection, and just have this one call it?

https://codereview.chromium.org/1998233002/diff/80001/cc/output/filter_operat...
cc/output/filter_operation.cc:364: float spreadX = std::abs(spread.x());
If this needs to be abs'ed to handle negative scale, chances we'll need to check
the SkBlurImageFilter's version too (for reference filters). Might want to add a
reference filter FEGaussianBlur + reflection test.

https://codereview.chromium.org/1998233002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1998233002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:804: matrix.postScale(1, -1);
I'm not sure about this either.

[jbroman, 2016-06-08 15:17:37]

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

https://codereview.chromium.org/1998233002/diff/80001/cc/output/filter_operat...
cc/output/filter_operation.cc:359: gfx::Rect
FilterOperation::MapRectReverse(const gfx::Rect& rect,
On 2016/06/07 at 16:00:34, Stephen White wrote:
> This looks awfully similar to the existing flavour above. Could we instead
have MapRect() take an (optional) MapDirection, and just have this one call it?

Done.

https://codereview.chromium.org/1998233002/diff/80001/cc/output/filter_operat...
cc/output/filter_operation.cc:364: float spreadX = std::abs(spread.x());
On 2016/06/07 at 16:00:34, Stephen White wrote:
> If this needs to be abs'ed to handle negative scale, chances we'll need to
check the SkBlurImageFilter's version too (for reference filters). Might want to
add a reference filter FEGaussianBlur + reflection test.

SkBlurImageFilter does apply abs:

sigma.fX = SkMinScalar(SkScalarAbs(sigma.fX), MAX_SIGMA);
sigma.fY = SkMinScalar(SkScalarAbs(sigma.fY), MAX_SIGMA);

[Stephen White, 2016-06-08 22:05:23]

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

https://codereview.chromium.org/1998233002/diff/80001/cc/output/filter_operat...
cc/output/filter_operation.cc:364: float spreadX = std::abs(spread.x());
On 2016/06/08 15:17:37, jbroman wrote:
> On 2016/06/07 at 16:00:34, Stephen White wrote:
> > If this needs to be abs'ed to handle negative scale, chances we'll need to
> check the SkBlurImageFilter's version too (for reference filters). Might want
to
> add a reference filter FEGaussianBlur + reflection test.
> 
> SkBlurImageFilter does apply abs:
> 
> sigma.fX = SkMinScalar(SkScalarAbs(sigma.fX), MAX_SIGMA);
> sigma.fY = SkMinScalar(SkScalarAbs(sigma.fY), MAX_SIGMA);

So it does. Thanks!

https://codereview.chromium.org/1998233002/diff/100001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1998233002/diff/100001/cc/output/filter_opera...
cc/output/filter_operation.cc:320: enum class MapDirection { FORWARD, REVERSE };
Why not just use SkImageFilter::MapDirection?

[jbroman, 2016-06-13 14:57:33]

https://codereview.chromium.org/1998233002/diff/100001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1998233002/diff/100001/cc/output/filter_opera...
cc/output/filter_operation.cc:320: enum class MapDirection { FORWARD, REVERSE };
On 2016/06/08 at 22:05:23, Stephen White away til July 11 wrote:
> Why not just use SkImageFilter::MapDirection?

Sure, done. (No real reason, other than "it doesn't always call into Skia". It's
totally not a big deal because this is all in an anonymous namespace anyhow.)

[jbroman, 2016-06-27 19:59:16]

jbroman@chromium.org changed reviewers:
+ enne@chromium.org

[jbroman, 2016-06-27 19:59:17]

senorblanco seems to be OOO; enne, do you have time to have a look?

[enne (OOO), 2016-06-27 20:59:33]

Looks good in general.  Can you convince me that you're accumulating
transformations in the right order? Aren't some filter operations (box reflect?)
not commutative with respect to how they change outsets?

[jbroman, 2016-06-29 20:38:29]

added unit tests for a combination of filters which do not geometrically commute

(Used a simpler matrix filter because it's easier to reason about than the full
box reflect filter, which uses a matrix filter for this anyhow.)

[enne (OOO), 2016-06-29 20:43:13]

lgtm, thanks for the extra test!

[jbroman, 2016-06-29 20:47:56]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-06-29 20:49:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 21:59:16]

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

[commit-bot: I haz the power, 2016-06-29 21:59:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2016-06-29 22:04:44]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-06-29 22:11:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 23:04:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-29 23:05:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 23:06:21]

Description was changed from

==========
cc: Implement and use FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

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

to

==========
cc: Implement and use FilterOperations::MapRectReverse for mapping backdrop
bounding box.

This is more precise than GetFilterOutsets (e.g. in the case of an SVG
<feOffset> filter), and more correct if a filter contains a scale, even by
-1.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ca9b612516efc2af1143c09a4d67f5b99e566c79
Cr-Commit-Position: refs/heads/master@{#402974}
==========

[commit-bot: I haz the power, 2016-06-29 23:06:22]

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