[3/5] Add translated rasterization support for RasterBuffer & below

[3/5] Add translated rasterization support for RasterBuffer & below

This CL implements the muscle of transformed rasterization. RasterBuffer now
accepts a translation in addition to a scale factor when rasterizing into a
backing.

Arbitrary raster transformation is not planned for near future. Thus only
implementing scale & translate which already helps many common cases.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2563743004
Cr-Commit-Position: refs/heads/master@{#461673}
Committed: https://chromium.googlesource.com/chromium/src/+/178ac91a501f976d584a845962fc1afd8a927362

[trchen, 2016-12-09 02:56:28]

Description was changed from

==========
Add translated rasterization support for RasterBuffer & below
==========

to

==========
Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[trchen, 2016-12-09 02:58:08]

Description was changed from

==========
Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[3/6] Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2017-01-03 22:53:15]

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

[enne (OOO), 2017-01-03 22:53:16]

https://codereview.chromium.org/2563743004/diff/1/cc/debug/rasterize_and_reco...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/2563743004/diff/1/cc/debug/rasterize_and_reco...
cc/debug/rasterize_and_record_benchmark_impl.cc:57:
DCHECK_EQ(raster_scales.width(), raster_scales.height());
I think raster_scales was a failed experiment.  You should be able to change
this to just have a single value.

https://codereview.chromium.org/2563743004/diff/1/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2563743004/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:130: DCHECK_EQ(raster_scales_.width(),
raster_scales_.height());
Similarly here, I think you should change this to be a singular scale in all
cases.

[trchen, 2017-01-14 01:06:37]

https://codereview.chromium.org/2563743004/diff/1/cc/debug/rasterize_and_reco...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/2563743004/diff/1/cc/debug/rasterize_and_reco...
cc/debug/rasterize_and_record_benchmark_impl.cc:57:
DCHECK_EQ(raster_scales.width(), raster_scales.height());
On 2017/01/03 22:53:16, enne wrote:
> I think raster_scales was a failed experiment.  You should be able to change
> this to just have a single value.

Done. (Thanks vmpstr!)

https://codereview.chromium.org/2563743004/diff/1/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2563743004/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:130: DCHECK_EQ(raster_scales_.width(),
raster_scales_.height());
On 2017/01/03 22:53:16, enne wrote:
> Similarly here, I think you should change this to be a singular scale in all
> cases.

Done.

[trchen, 2017-03-28 21:41:13]

Description was changed from

==========
[3/6] Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[3/5] Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[trchen, 2017-03-28 22:53:44]

Description was changed from

==========
[3/5] Add translated rasterization support for RasterBuffer & below
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[3/5] Add translated rasterization support for RasterBuffer & below

This CL implements the muscle of transformed rasterization. RasterBuffer now
accepts a translation in addition to a scale factor when rasterizing into a
backing.

Arbitrary raster transformation is not planned for near future. Thus only
implementing scale & translate which already helps many common cases.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2017-03-29 13:06:13]

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
File cc/base/scale_translate2d.h (right):

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:16: class CC_BASE_EXPORT ScaleTranslate2d {
If you're going to add a geometry type, could you at least add some unit tests
that smoke test all the functions?

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:86: gfx::RectF TransformRectReverse(const
gfx::RectF& r) const {
naming nit: Maybe InverseTransformRect, as it really just seems to be a helper
for Inverse(this).TransformRect?

[danakj, 2017-03-29 16:16:36]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-03-29 16:16:38]

Some geometry feedbacks

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
File cc/base/scale_translate2d.h (right):

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
2017

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:16: class CC_BASE_EXPORT ScaleTranslate2d {
On 2017/03/29 13:06:13, enne wrote:
> If you're going to add a geometry type, could you at least add some unit tests
> that smoke test all the functions?

Please put a class-level comment on it explaining what it is, when it would be
used, any limitations it has etc.

I don't think that we should add geometry types to cc/base/. They should go in
ui/gfx/geometry.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:18: constexpr ScaleTranslate2d() : scale_(1.f) {}
 = default

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:29: static ScaleTranslate2d PreScale(const
ScaleTranslate2d& t, float scale) {
It would be nice to make these static methods into free functions like we do for
Rect/Size/Point/Vector2d/etc..

However then this would be PreScaleScaleTranslate2d, which uh.. doesn't work.

I'm proposing to name this class AxisTransform2d (because its a transform that
keeps things axis-aligned). That would give the class a much more clearly-nouns
name, and we could have PreScaleAxisTransform2d and that'd make sense.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:32: void PreScale(float scale) { *this =
PreScale(*this, scale); }
This is maybe a nit, or maybe nicer if the statics are free functions, but I'd
prefer if these modifying methods we implemented directly, and the free/static
methods used them on a variable that they returned. I think these would read
very clearly when they are modifying their own members, and the free functions
would read more clearly by calling them than by constructing a new object and
doing the math on the arguments to the constructor.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:76: gfx::PointF TransformPoint(const gfx::PointF& p)
const {
I'd say everything should be s/Transform/Map/g cuz that's how we refer to things
in MathUtil

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:90: void ApplyToCanvas(SkCanvas* canvas) const;
This seems like something that should not be part of this class. It could be a
free function somewhere else.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:98: // Scale is applied before translation, i.e.
This type of thing seems like it should be part of the class-level comment,
rather than on the private variables.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:100: float scale_;
= 1.f

[trchen, 2017-03-29 22:59:57]

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
File cc/base/scale_translate2d.h (right):

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:1: // Copyright (c) 2016 The Chromium Authors. All
rights reserved.
On 2017/03/29 16:16:37, danakj wrote:
> 2017

Acknowledged.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:16: class CC_BASE_EXPORT ScaleTranslate2d {
On 2017/03/29 16:16:37, danakj wrote:
> On 2017/03/29 13:06:13, enne wrote:
> > If you're going to add a geometry type, could you at least add some unit
tests
> > that smoke test all the functions?

Acknowledged.

> Please put a class-level comment on it explaining what it is, when it would be
> used, any limitations it has etc.

Acknowledged.

> I don't think that we should add geometry types to cc/base/. They should go in
> ui/gfx/geometry.

Acknowledged. I was thinking the same. I put it in cc only because I wasn't sure
what people prefer. :)

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:18: constexpr ScaleTranslate2d() : scale_(1.f) {}
On 2017/03/29 16:16:37, danakj wrote:
>  = default

Acknowledged.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:29: static ScaleTranslate2d PreScale(const
ScaleTranslate2d& t, float scale) {
On 2017/03/29 16:16:37, danakj wrote:
> It would be nice to make these static methods into free functions like we do
for
> Rect/Size/Point/Vector2d/etc..
> 
> However then this would be PreScaleScaleTranslate2d, which uh.. doesn't work.

Acknowledged.

I will follow the existing convention. But this is one question I always wanted
to ask. Why is this style preferred over the others?

The problem with free function is that it pollutes the namespace. With type
suffix it is mitigated, but then the advantage of not having to type redundant
type name is gone, makes it no better than static class method. And what's so
wrong with member function anyway? Is it because it confuses between in-place
and out-of-place operations?

> I'm proposing to name this class AxisTransform2d (because its a transform that
> keeps things axis-aligned). That would give the class a much more
clearly-nouns
> name, and we could have PreScaleAxisTransform2d and that'd make sense.

Yea I saw the discussion on chat. In mathematical notation it would be
D(2)⨯T(2), but I couldn't find a common name for it either. AxisTransform2d
sounds good enough (AxisPreservingTransform2d sounds too verbose). :/

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:32: void PreScale(float scale) { *this =
PreScale(*this, scale); }
On 2017/03/29 16:16:37, danakj wrote:
> This is maybe a nit, or maybe nicer if the statics are free functions, but I'd
> prefer if these modifying methods we implemented directly, and the free/static
> methods used them on a variable that they returned. I think these would read
> very clearly when they are modifying their own members, and the free functions
> would read more clearly by calling them than by constructing a new object and
> doing the math on the arguments to the constructor.

Acknowledged.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:76: gfx::PointF TransformPoint(const gfx::PointF& p)
const {
On 2017/03/29 16:16:37, danakj wrote:
> I'd say everything should be s/Transform/Map/g cuz that's how we refer to
things
> in MathUtil

Acknowledged. YYYYES! I was following gfx::Transform, but MapPoint sounds way
better!

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:86: gfx::RectF TransformRectReverse(const
gfx::RectF& r) const {
On 2017/03/29 13:06:13, enne wrote:
> naming nit: Maybe InverseTransformRect, as it really just seems to be a helper
> for Inverse(this).TransformRect?

Acknowledged. Was following gfx::Transform. Perhaps InverseMapRect? I personally
preferred to omit the type suffix but oh well.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:90: void ApplyToCanvas(SkCanvas* canvas) const;
On 2017/03/29 16:16:37, danakj wrote:
> This seems like something that should not be part of this class. It could be a
> free function somewhere else.

Agreed. Somewhere in ui/gfx? cc/base? skia/ext?

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:98: // Scale is applied before translation, i.e.
On 2017/03/29 16:16:37, danakj wrote:
> This type of thing seems like it should be part of the class-level comment,
> rather than on the private variables.

I think the opposite. At the class level all we know is that it is a D(2)⨯T(2)
group. The internal representation shouldn't be a concern of the caller. It
could well be pre-translation post-scale while keeping all behavior the same.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:100: float scale_;
On 2017/03/29 16:16:37, danakj wrote:
> = 1.f

Acknowledged.

[trchen, 2017-03-30 20:56:05]

All done.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
File cc/base/scale_translate2d.h (right):

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:90: void ApplyToCanvas(SkCanvas* canvas) const;
It's only called once. Unrolled & removed this helper function for now.

https://codereview.chromium.org/2563743004/diff/60001/cc/base/scale_translate...
cc/base/scale_translate2d.h:98: // Scale is applied before translation, i.e.
Anyway, also added comments at the top about pre-scale post-translate. No harm
to keep both right?

[enne (OOO), 2017-03-31 14:31:15]

Thanks.  This lgtm % danakj's final review.

[danakj, 2017-03-31 15:09:49]

gfx LGTM

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
File ui/gfx/geometry/axis_transform2d.h (right):

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:76: Vector2dF translation() const { return
translation_; }
nit: return const& as this is returning a member

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:125: static inline AxisTransform2d
InverseAxisTransform2d(const AxisTransform2d& t) {
Invert

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:131: }  // namespace gfx
I was going to ask for PrintTo for this for unit tests so they don't have to
ToString() like 

https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.h?rcl=8199d676ec703...
and
https://cs.chromium.org/chromium/src/ui/gfx/test/gfx_util.cc?rcl=16dd65423c40...

But it looks like maybe you don't need it? We have it for RectF even though
there is AssertRectFloatEqual, maybe that's redundant..

[trchen, 2017-03-31 20:56:51]

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
File ui/gfx/geometry/axis_transform2d.h (right):

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:76: Vector2dF translation() const { return
translation_; }
On 2017/03/31 15:09:49, danakj wrote:
> nit: return const& as this is returning a member

Done.

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:125: static inline AxisTransform2d
InverseAxisTransform2d(const AxisTransform2d& t) {
On 2017/03/31 15:09:49, danakj wrote:
> Invert

Done.

https://codereview.chromium.org/2563743004/diff/80001/ui/gfx/geometry/axis_tr...
ui/gfx/geometry/axis_transform2d.h:131: }  // namespace gfx
On 2017/03/31 15:09:49, danakj wrote:
> I was going to ask for PrintTo for this for unit tests so they don't have to
> ToString() like 
> 
>
https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.h?rcl=8199d676ec703...
> and
>
https://cs.chromium.org/chromium/src/ui/gfx/test/gfx_util.cc?rcl=16dd65423c40...
> 
> But it looks like maybe you don't need it? We have it for RectF even though
> there is AssertRectFloatEqual, maybe that's redundant..

PrintTo added.

[trchen, 2017-04-04 08:50:26]

The CQ bit was checked by trchen@chromium.org

[trchen, 2017-04-04 08:50:26]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2563743004/#ps100001
(title: "rebase & fix last nits")

[commit-bot: I haz the power, 2017-04-04 08:50:41]

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, 2017-04-04 10:11:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491295826169940,
"parent_rev": "841eab11c2273bb100fbd927a9e47411838cc6b5", "commit_rev":
"178ac91a501f976d584a845962fc1afd8a927362"}

[commit-bot: I haz the power, 2017-04-04 10:12:09]

Description was changed from

==========
[3/5] Add translated rasterization support for RasterBuffer & below

This CL implements the muscle of transformed rasterization. RasterBuffer now
accepts a translation in addition to a scale factor when rasterizing into a
backing.

Arbitrary raster transformation is not planned for near future. Thus only
implementing scale & translate which already helps many common cases.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[3/5] Add translated rasterization support for RasterBuffer & below

This CL implements the muscle of transformed rasterization. RasterBuffer now
accepts a translation in addition to a scale factor when rasterizing into a
backing.

Arbitrary raster transformation is not planned for near future. Thus only
implementing scale & translate which already helps many common cases.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2563743004
Cr-Commit-Position: refs/heads/master@{#461673}
Committed:
https://chromium.googlesource.com/chromium/src/+/178ac91a501f976d584a845962fc...
==========

[commit-bot: I haz the power, 2017-04-04 10:12:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/178ac91a501f976d584a845962fc...