Re: [chromium-dev] Redesign gfx::Transform and share implementation with blink::TransformationMatrix

[Chris Harrelson]

On Fri, Mar 10, 2017 at 4:54 PM, Tien-Ren Chen <trc...@chromium.org> wrote:

Hello chromium-dev,

I'm recently working on Blink layerization and geometry conversion
work, and found the ergonomics of transform classes really paralyzing
to me. I'll summarize a few issues we have today, the proposed
solutions with rationale, and would like to hear feedback from you.

1. A lot of duplicated code between the two classes. gfx::Transform is
a thin wrapper over SkMatrix44, while blink::TransformationMatrix does
its own 4x4 array.

Proposal: Make a templated 4x4 matrix class that can plug any
arbitrary scalar type.
Rationale: We can't all resort to SkMatrix44 because we have different
precision requirement in cc and in Blink. In Blink geometry code the
matrix often needs to map between arbitrary spaces that are can be
distant to each other, and can have a huge dynamic range thus double
precision is required. In cc we only use single precision matrix
currently, but IMO some of the transforms that are more "global"
really should be double precision, while code that are closer to
raster side (thus only involves coordinates in screen space) should
use single precision for performance. IMO the need of both is real.

An alternative is to templatize SkMatrix44, but I'm hesitate to
propose radical changes to Skia because it's a public library. Also in
Skia's context, there is almost no needs for double precision matrix.

2. 2D transform class is absent.

Proposal: Also add a templated 3x3 matrix class for general 2D
homogeneous linear transform.
Rationale: Mathematically speaking, 3D transform and 2D transform
really have different semantics. The former maps a space to another
space, while the latter maps a plane to another plane.

To make a concrete example, in Chromium we often need make such query
like "here are 3D-transformed element A and element B, do they
overlap? what is the enclosing rect of the overlap?", and one will be
very tempting to use their accumulate 4x4 matrix to convert them to
the same space. However that would be the wrong answer because what
that really does is to map the two 3D planes to a common space, and
find intersection of them, which would be a line. What the query
really means is: "we have two 3D-transformed rects, which will be
projected onto the screen plane, what is the overlap of their
projection?", and the right implementation is to look at the 3D
transform, and ask how would it map plane (u, v, 0) in local space to
screen (x, y), boiling it down to a 2D homogeneous transform, i.e. a
3x3 matrix.

In fact, most operations we do in cc/Blink only involves 2D
homogeneous transforms, but we simply use a degenerate 3D transform.
In addition to the potential misuse I outlined above, using a higher
rank matrix is also slower, and determinant computation will be
subject to more numerical issues.

The only use of full 4x4 matrix is for determining face orientation of
a layer, because we want to query "is the front half-space in the
original space mapped to the front half-space of the output space",
and a 3x4 matrix is only needed for depth sorting. Everything else are
just screen projection.

3. Lack of standard operators (v.s. inplace operators)

Proposal: Still offers only inplace operator, but provides safe chaining.
Rationale: Currently gfx::Transform offers no standard operators,
effectively disallowing the use of constant matrix, and can be awkward
to write.

For example, we often see code patterns like this:
class Foo {
  gfx::Transform draw_transform_;
  Foo(const gfx::Transform& layer_transform, float content_scale) {
    draw_transform_ = layer_transform;
    draw_transform_.scale(1 / content_scale);
  }
};
The cleaner way really should be:
class Foo {
  const gfx::Transform draw_transform_;
  Foo(const gfx::Transform& layer_transform, float content_scale)
    : draw_transform_(layer_transform.clone().scale(1 / content_Scale)) {}
};

Another awkwardness:
gfx::Transform content_transform = transform_node->to_screen_;
content_transform.translate(layer_offset_from_transform_node);
content_transform.scale(1 / content_scale);
V.s.
const gfx::Transform content_transform =
transform_node->to_screen_.clone().translate(layer_offset_from_transform_node).scale(1
/ content_scale);

Why inplace chaining v.s. standard operators? Because inplace operator
usually use less memory/registers when not inlined.

What is safe inplace chaining? Here is an example of unsafe chaining:
blink::TransformationMatrix foo =
bar.translate(layer_offset_from_transform_node).scale(1 /
content_scale);
And the author will be surprised to see "bar" also gets mutated. The
idea of safe chaining is that transform objects should be generally
immutable unless explicitly told to. This can be done by C++ type
magic and providing two accessors:
.clone(), which will return a new mutable instance.
.mutate(), which will return mutable reference to the same object.

Here I uploaded a small prototype: https://codereview.chromium.org/2738213005

4. Lack of error checking in many of the operators. For example,
homogeneous linear transforms on Cartesian points can fail by having a
scale factor of 0. The error check is not only omitted, but we even
generate a bogus value of [hx, hy, hz], instead of a more sensible
fallback of [inf, inf, inf].

Proposal: Making error check mandatory by returning base::Optional<T>
type. Also provide no-check version that contains DCHECK for error
checking.
Rationale: The common error check pattern we have today is to return a
boolean and pass output variable by reference. This is less friendly
to compiler optimization, and is error-prone:

gfx::Transform inv;
xfrm.GetInverse(&inv); // Forgot to error check!

In comparison the base::Optional<T> pattern enforces error check and
also makes it easier for aliasing analysis.
auto inv = xfrm.GetInverse();
if (!inv)
  return;
gfx::Transform something = inv->translate(123, 456);

No-check version should also be provided for a zero-cost path when the
developer knew the function can never fail. e.g. transform known to be
invertible / transform known to be affine. And DCHECK should be added
to make sure the contract is actually held.

5. Transform and matrix are implemented by the same class.

Proposal: Split transform class and matrix class.
Rationale: Transforms are often implemented as square matrix, but it
doesn't have to, and details like whether it uses column vector or row
vector should be abstracted from users of the class. Also there are
other uses of the 4x4 matrix, e.g. color space conversion. Some
transform operations just don't quite make sense for colors.

6. Some CSS internals was leaked to the class interface. For example,
interpolation of transforms by decomposition. Unusual notation of
m_col_row instead of m_row_col.

Proposal: Those really should go to some helper class prefixed by CSS something.

--------

These are some of the issues I can remember. I may have missed some. I
would also like to hear what are the other transform-related
ergonomics issues that are concerned by you. Thanks for reading this
long post!

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.