Bug 6868 – make TransformationMatrix platform independent

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Dave asked me to do this.

[Darin Adler]

Created attachment 6019 [details]
first cut -- after review I plan to re-read and do final refinements

[Dave Hyatt]

Comment on attachment 6019 [details]
first cut -- after review I plan to re-read and do final refinements

I would prefer it if x() and y() remained on the rect class.  Much of the code that uses rects operates in terms of x and y, and I think it creates an inconsistency to eliminate these methods on rect while leaving all the calling code still using x/y variable names and conventions.

Philosophically you have also mutated what was essentially a PixelRect class into a PointRect class.  There have been assertions that old methods like right/bottom or contains were confusing but they did have well-understood semantics for what was essentially a rect operating on pixels and not on points.  My biggest concern here is just making sure all the call sites that used right/bottom   have been patched properly, since 1 pixel errors will be really hard to spot with any of our regression tests.

This reworking of IntRect to be point-based and not pixel-based has also led to methods like contains being a little strange, since contains is a question being asked of a pixel.  
I don't really like the name containsPixel, and feel like this clarification was almost only needed because the rect has been reworked to operate on points instead of pixels.

That said, my main objection is the x/y removal, since even a point-based rect is constructed with points that have x/y coordinates, and I think it's pretty clear to clients of the rect that the x/y of a rect would refer to the top left point's x/y.

[Dave Hyatt]

I also see some places where code had bottom() + 1, and you patched it to be bottom() - 1 ... + 1, so it would be nice to clean that up.

[Dave Hyatt]

I should say too that if we have a float-based rect, then clearly in order for the two rect classes to be consistent, it's better to think of them as point-based and not pixel-based.  So just fix the x/y thing and post a new patch and I'll scrutinize all the bottom/right call sites and such to help double-check.  containsPixel is ok for now too.

[Dave Hyatt]

I should say too that if we have a float-based rect, then clearly in order for the two rect classes to be consistent, it's better to think of them as point-based and not pixel-based.  So just fix the x/y thing and post a new patch and I'll scrutinize all the bottom/right call sites and such to help double-check.  containsPixel is ok for now too.

[Dave Hyatt]

Wow check out this weird double-post bug on TOT.

[Darin Adler]

I did handle the bottom and right cases very carefull. What I did was:

    1) change every call site to bottomMinusOne() where it seemed that was right
    2) change every call site to sensibleBottom() where it seemed that classic "bottom" worked
    3) go visit all the bottomMinusOne() ones to see if they could be made to make sense, if not changed them to bottom() - 1
    4) global replace sensibleBottom() with bottom()

Originally, in fact, I had intended to keep bottomMinusOne() on the class for an interim period, but I found few cases actually needed it.

I'll post a new patch as soon as I can.

[Darin Adler]

Created attachment 6054 [details]
patch revised based on Hyatt's review

[Darin Adler]

Created attachment 6080 [details]
newer patch, merged with TOT

[Eric Seidel (no email)]

Created attachment 6086 [details]
Approximate -uw patch made from my tree

[Darin Adler]

Comment on attachment 6080 [details]
newer patch, merged with TOT

Eric reviewed this, but it's not passing layout tests yet, so review-.

[Eric Seidel (no email)]

This has been done since... 

[Darin Adler]

No, this was never done. Instead we have three platform-specific AffineTransform implementations, calling through to Core Graphics, Qt, and Cairo. The concept here was to have a single platform-independent one instead. I still think it's a good idea.

[Eric Seidel (no email)]

Ah, my misunderstanding.

[Chris Marrin]

We still need to get rid of the platform specific versions of things like TransformationMatrix::multiply(). That is the job I will do...

[Chris Marrin]

I've added a dependency on this bug to bug 23689. I will be adding the 3D support at the same time as getting rid of the platform dependencies. 

[Chris Marrin]

Created attachment 27399 [details]
Patch

        I have not only made TransformationMatrix platform independent
        but I've also added 3D methods, which will be used when I update
        WebKitCSSMatrix to include 3D (see https://bugs.webkit.org/show_bug.cgi?id=23689).
        I am now keeping a full 4x4 matrix in TransformationMatrix. I'm also doing all
        the math as doubles rather than floats. This makes a TransformationMatrix
        go from 24 bytes to 128 bytes, but I don't think this class is used enough to
        make this overhead will be significant.

        The change from floats to doubles has caused some differences in rounding and
        display (sometimes things that displayed as -0.0 now display as 0.0 or vice versa),
        so I've had to change some LayoutTest results in the SVG tests.

        NOTE: This patch is only tested on Mac. Other platforms will likely have build issues.
        I will fix these on Windows, but I can't test on the other platforms.

[Simon Fraser (smfr)]

Comment on attachment 27399 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================

> +        NOTE: This patch is only tested on Mac. Other platforms will likely have build issues.
> +        I will fix these on Windows, but I can't test on the other platforms.

I don't think you need to say this in the changelog.

> Index: WebCore/platform/graphics/cg/TransformationMatrixCG.cpp
> ===================================================================

> +TransformationMatrix::operator CGAffineTransform() const
>  {
> -    return CGAffineTransformConcat(m_transform, CGAffineTransform(m2));
> +    return CGAffineTransformMake(narrowPrecisionToCGFloat(m_matrix[0][0]),
> +                                 narrowPrecisionToCGFloat(m_matrix[0][1]),
> +                                 narrowPrecisionToCGFloat(m_matrix[1][0]),
> +                                 narrowPrecisionToCGFloat(m_matrix[1][1]),
> +                                 narrowPrecisionToCGFloat(m_matrix[3][0]),
> +                                 narrowPrecisionToCGFloat(m_matrix[3][1]));
>  }

It's a shame that extracting the correct elements from the 4x4 matrix to make
an affine matrix happens in more than one place. Can we define an simple
affine matrix type (typedef of float array), and return that from the shared
code?

> Index: WebCore/platform/graphics/transforms/TransformationMatrix.cpp
> ===================================================================

> +////////////////////////////// Supporting Math Functions ///////////////////////////////////////
> +//
> +// This is a set of function from various places (attributed inline) to do things like
> +// inversion and decomposition of a 4x4 matrix. They are used throughout the code
> +//
> +////////////////////////////////////////////////////////////////////////////////////////////////

This is not standard WebCore commenting style.

> +//
> +// Adapted from Matrix Inversion by Richard Carling, Graphics Gems <http://tog.acm.org/GraphicsGems/index.html>.
> +
> +// EULA: The Graphics Gems code is copyright-protected. In other words, you cannot claim the text of the code 
> +// as your own and resell it. Using the code is permitted in any program, product, or library, non-commercial 
> +// or commercial. Giving credit is not required, though is a nice gesture. The code comes as-is, and if there 
> +// are any flaws or problems with any Gems code, nobody involved with Gems - authors, editors, publishers, or 
> +// webmasters - are to be held responsible. Basically, don't be a jerk, and remember that anything free comes 
> +// with no guarantee.

Someone (Darin?) needs to confirm that it's OK to add this code.

> +typedef double Vector4[4];
> +typedef double Vector3[3];
> +
> +#define SMALL_NUMBER    1.e-8

Use a constant.

> +
> +//    inverse( original_matrix, inverse_matrix )

Nit: no spaces next to the parens.

> +static double det2x2(double a, double b, double c, double d)

For this (and other functions), I think the abbreviations don't help any.
Call it determinant2x2() or computeDeterminant2x2().

>  {
> -    TransformationMatrix m(matrix);
> +    double ans;
> +    ans = a * d - b * c;
> +    return ans;
> +}

No need for the 'ans' variable. Inline it?

> +//  double = det3x3(  a1, a2, a3, b1, b2, b3, c1, c2, c3 )
> +//  
> +//  calculate the determinant of a 3x3 matrix
> +//  in the form
> +// 
> +//      | a1,  b1,  c1 |
> +//      | a2,  b2,  c2 |
> +//      | a3,  b3,  c3 |

Nit: sentence case.

> +static double det4x4(const TransformationMatrix::Matrix4& m)
> +{
> +    double ans;
> +    double a1, a2, a3, a4, b1, b2, b3, b4, c1, c2, c3, c4, d1, d2, d3, d4;
> +
> +    // assign to individual variable names to aid selecting
> +    // correct elements
> +
> +    a1 = m[0][0]; b1 = m[0][1]; 
> +    c1 = m[0][2]; d1 = m[0][3];
> +
> +    a2 = m[1][0]; b2 = m[1][1]; 
> +    c2 = m[1][2]; d2 = m[1][3];
> +
> +    a3 = m[2][0]; b3 = m[2][1]; 
> +    c3 = m[2][2]; d3 = m[2][3];
> +
> +    a4 = m[3][0]; b4 = m[3][1]; 
> +    c4 = m[3][2]; d4 = m[3][3];
> +
> +    ans = a1 * det3x3(b2, b3, b4, c2, c3, c4, d2, d3, d4)
> +        - b1 * det3x3(a2, a3, a4, c2, c3, c4, d2, d3, d4)
> +        + c1 * det3x3(a2, a3, a4, b2, b3, b4, d2, d3, d4)
> +        - d1 * det3x3(a2, a3, a4, b2, b3, b4, c2, c3, c4);
> +    return ans;

No need for the 'ans' variable. Just return the result.

> +static void adjoint(const TransformationMatrix::Matrix4& srcMatrix, TransformationMatrix::Matrix4& outMatrix)

The 'out' prefix is not webkit style, alas. You could call the variable 'result'.

> +// Returns false if the matrix is not invertible
> +static bool inverse(const TransformationMatrix::Matrix4& srcMatrix, TransformationMatrix::Matrix4& outMatrix)

Ditto.

> +// Multiply a homogeneous point by a matrix and return the transformed point
> +static void v4MulPointByMatrix(Vector4 pin, const TransformationMatrix::Matrix4& m, Vector4 pout)

'pout'?

Expand 'Mul' to Multiply. Is the v4 prefix necessary?

> +static double v3Length(Vector3 a) 
>  {
> -    return det() != 0.0;
> +    return sqrt((a[0] * a[0])+(a[1] * a[1])+(a[2] * a[2]));

Spaces around the + please.

> +static void v3Scale(Vector3 v, double newlen) 
>  {

What is 'newlen'? Needs a better name.

> -    return (*this) *= other;
> +    double len = v3Length(v);
> +    if (len != 0.0) {

Just (len != 0)

I don't really like that this function changes the vector in-place, while the others like
v3Length do no. I'd prefer to see 'const' used to makek things clearer.

> +// Make a linear combination of two vectors and return the result.
> +// result = (a * ascl) + (b * bscl)
> +static void v3Combine (Vector3 a, Vector3 b, Vector3 result, double ascl, double bscl) 
> +{
> +    result[0] = (ascl * a[0]) + (bscl * b[0]);
> +    result[1] = (ascl * a[1]) + (bscl * b[1]);
> +    result[2] = (ascl * a[2]) + (bscl * b[2]);
> +}

Ditto; use 'const'?

> +// Return the cross product c = a cross b */
> +static void v3Cross(Vector3 a, Vector3 b, Vector3 c)
> +{
> +    c[0] = (a[1] * b[2]) - (a[2] * b[1]);
> +    c[1] = (a[2] * b[0]) - (a[0] * b[2]);
> +    c[2] = (a[0] * b[1]) - (a[1] * b[0]);
> +}

Ditto.

> +static bool decompose(const TransformationMatrix::Matrix4& mat, TransformationMatrix::DecomposedType& ret)

Use 'result' rather than 'ret' for consistency.

> +{
> +    int i, j;

Declare where used?

> +    TransformationMatrix::Matrix4 locmat;
> +    memcpy(locmat, mat, sizeof(TransformationMatrix::Matrix4));

Can't you just assign? Avoid abbreviated names, use interCaps.

> +    // Normalize the matrix.
> +    if (locmat[3][3] == 0)
> +        return false;
> +
> +    for (i = 0; i < 4; i++)
> +        for (j = 0; j < 4; j++)
> +            locmat[i][j] /= locmat[3][3];
> +
> +    // pmat is used to solve for perspective, but it also provides
> +    // an easy way to test for singularity of the upper 3x3 component.
> +    TransformationMatrix::Matrix4 pmat;
> +    memcpy(pmat, locmat, sizeof(TransformationMatrix::Matrix4));

Just assign, or copy-construct?

> +    for (i = 0; i < 3; i++)
> +        pmat[i][3] = 0;
> +    pmat[3][3] = 1;
> +
> +    if (det4x4(pmat) == 0.0)
> +        return false;

Use == 0
 
> +    // First, isolate perspective.  This is the messiest.
> +    if (locmat[0][3] != 0 || locmat[1][3] != 0 || locmat[2][3] != 0) {
> +        // prhs is the right hand side of the equation.
> +        Vector4 prhs;
> +        prhs[0] = locmat[0][3];
> +        prhs[1] = locmat[1][3];
> +        prhs[2] = locmat[2][3];
> +        prhs[3] = locmat[3][3];
> +
> +        // Solve the equation by inverting pmat and multiplying
> +        // prhs by the inverse.  (This is the easiest way, not
> +        // necessarily the best.)
> +        // inverse function (and det4x4, above) from the Matrix
> +        // Inversion gem in the first volume.

Comment seems out of place.

> +        TransformationMatrix::Matrix4 invpmat, tinvpmat;

Don't like these variable names. Just because it's math code doesn't mean it has
to have short names.

> +        // Stuff the answer away.

No need for this comment.

> +    // Now, get the rotations out, as described in the gem.
> +    
> +    // FIXME - We return rotation values as quaternions because they are easier
> +    // to recompose later. But when we add decomposition to CSSMatrix we will 
> +    // want to return the rotation values as rx, ry, and rz values because they 
> +    // are much easier to deal with for the author. The commented out code below 
> +    // does that. Ultimately we will want to pass in a flag to choose which to
> +    // return.

Don't put statements about the future into comments. They always get out of date.
A FIXME comment is more appropriate; even better if there's a bug to track it.

> +    double T, S, x, y, z, w;

What is the significance of the uppercase?

> +// Copied from Core Animation

Ouch. Need to check if it's OK to put this here.

> +static void slerp(double qa[4], const double qb[4], double t)

Needs a better function name.

> +//////////////////////////// End of Supporting Math Functions //////////////////////////////////

Not the normal comment style.


> +TransformationMatrix TransformationMatrix::affineTransform() const
> +{
> +    // Note that this throws away the z-axis
> +    // FIXME: indicate when this loses data somehow
> +    TransformationMatrix transform(m_matrix[0][0], m_matrix[0][1], m_matrix[1][0], m_matrix[1][1], m_matrix[3][0], m_matrix[3][1]);
> +    return transform;

Maybe it should have a bool* param that is filled with true or false if the caller
requests it?

> +FloatPoint TransformationMatrix::projectPoint(const FloatPoint& p) const
> +    // This is basically raytracing. We have a point in the destination
> +    // plane with z=0, and we cast a ray parallel to the z-axis from that
> +    // point to find the z-position at which it intersects the z=0 plane
> +    // with the transform applied. Once we have that point we apply the
> +    // inverse transform to find the corresponding point in the source
> +    // space.
> +    // 
> +    // Given a plane with normal Pn, and a ray starting at point R0 and
> +    // with direction defined by the vector Rd, we can find the
> +    // intersection point as a distance d from R0 in units of Rd by:
> +    // 
> +    // d = -dot (Pn', R0) / dot (Pn', Rd)
> +    
> +    double x = p.x();
> +    double y = p.y();
> +    double z = -(m13() * x + m23() * y + m43()) / m33();
> +
> +    double outX = x * m11() + y * m21() + z * m31() + m41();
> +    double outY = x * m12() + y * m22() + z * m32() + m42();
> +
> +    double w = x * m14() + y * m24() + z * m34() + m44();
> +    if (w != 1 && w != 0) {
> +        outX /= w;
> +        outY /= w;
> +    }
> +
> +    return FloatPoint(static_cast<float>(outX), static_cast<float>(outY));
>  }

Is it OK to put this code into open source too?

> +FloatPoint TransformationMatrix::mapPoint(const FloatPoint& p) const
> +    double x, y;
> +    multVecMatrix(p.x(), p.y(), x, y);
> +    return FloatPoint(static_cast<float>(x), static_cast<float>(y));
>  }

What does this do if the matrix is 3D?

>  IntPoint TransformationMatrix::mapPoint(const IntPoint& point) const
>  {
> -    double x2, y2;
> -    map(point.x(), point.y(), &x2, &y2);
> +    double x, y;
> +    multVecMatrix(point.x(), point.y(), x, y);
>  
>      // Round the point.
> -    return IntPoint(lround(x2), lround(y2));
> +    return IntPoint(lround(x), lround(y));
>  }

What if the matrix is 3D?

> -FloatPoint TransformationMatrix::mapPoint(const FloatPoint& point) const
> +IntRect TransformationMatrix::mapRect(const IntRect &rect) const
>  {
> -    double x2, y2;
> -    map(point.x(), point.y(), &x2, &y2);
> +    return enclosingIntRect(mapRect(FloatRect(rect)));
> +}
>  
> -    return FloatPoint(static_cast<float>(x2), static_cast<float>(y2));
> +FloatRect TransformationMatrix::mapRect(const FloatRect& r) const
> +{
> +    FloatQuad resultQuad = mapQuad(FloatQuad(r));
> +    return resultQuad.boundingBox();
>  }
>  
> -FloatQuad TransformationMatrix::mapQuad(const FloatQuad& quad) const
> +FloatQuad TransformationMatrix::mapQuad(const FloatQuad& q) const
>  {
> -    // FIXME: avoid 4 seperate library calls. Point mapping really needs
> -    // to be platform-independent code.
> -    return FloatQuad(mapPoint(quad.p1()),
> -                     mapPoint(quad.p2()),
> -                     mapPoint(quad.p3()),
> -                     mapPoint(quad.p4()));
> +    FloatQuad result;
> +    result.setP1(mapPoint(q.p1()));
> +    result.setP2(mapPoint(q.p2()));
> +    result.setP3(mapPoint(q.p3()));
> +    result.setP4(mapPoint(q.p4()));
> +    return result;
>  }

What if the matrix is 3D for all these?

> +TransformationMatrix& TransformationMatrix::rotate3d(double x, double y, double z, double angle)
> +{
> +    // angles are in degrees. Switch to radians
> +    angle = angle / 360.0 * M_PI * 2.0;
> +    
> +    angle /= 2.0f;
> +    double sinA = sin(angle);
> +    double cosA = cos(angle);
> +    double sinA2 = sinA * sinA;
> +    
> +    // normalize
> +    double length = sqrt(x*x+y*y+z*z);

Spaces around the operators please.

> +TransformationMatrix& TransformationMatrix::rotate3d(double rx, double ry, double rz)
> +{
> +    // angles are in degrees. Switch to radians
> +    rx = rx / 360.0 * M_PI * 2.0;
> +    ry = ry / 360.0 * M_PI * 2.0;
> +    rz = rz / 360.0 * M_PI * 2.0;

Now there are 4 of them. Add an inline function for degreesToRadians().

> +TransformationMatrix& TransformationMatrix::skew(double sx, double sy)
> +{
> +    // angles are in degrees. Switch to radians
> +    sx = sx / 360.0 * M_PI * 2.0;
> +    sy = sy / 360.0 * M_PI * 2.0;

Here too.


> +//
> +// multiplies matrix by given matrix on the right
> +//

Comment is potentially confusing. Must clearer would just be to say:

// this = mat * this

> +TransformationMatrix& TransformationMatrix::multLeft(const TransformationMatrix& mat)
> +{
> +    double tmp[4][4];

Just use TransformationMatrix::Matrix4 to avoid the nasty cast below:

> +    setMatrix(reinterpret_cast<double*>(tmp));
> +    return *this;
> +}

> +void TransformationMatrix::multVecMatrix(double inx, double iny, double& outx, double& outy) const

Avoid 'in', 'out' prefixes.

> +void TransformationMatrix::multVecMatrix(double inx, double iny, double inz, double& outx, double& outy, double& outz) const

Ditto.

> +//
> +// returns inverse of matrix
> +//

Comment doesn't add anything.

> +TransformationMatrix TransformationMatrix::inverse() const 
> +{
> +    TransformationMatrix invMat;
> +    
> +    bool inverted = WebCore::inverse(m_matrix, invMat.m_matrix);
> +    if (!inverted) {
> +        // FIXME: indicate failure somehow
> +        return TransformationMatrix();

Address the FIXME?

> +static inline void blendFloat(double& from, double to, double progress)
> +{
> +    if (from != to)
> +        from = from + (to - from) * progress;
> +}
>  

> +void TransformationMatrix::blend(const TransformationMatrix& from, double progress)

Should blend stuff really be in here?

> +void TransformationMatrix::recompose(const DecomposedType& decomp)
> +{
> +    makeIdentity();
> +    
> +    // first apply perspective
> +    m_matrix[0][3] = (float) decomp.perspectiveX;
> +    m_matrix[1][3] = (float) decomp.perspectiveY;
> +    m_matrix[2][3] = (float) decomp.perspectiveZ;
> +    m_matrix[3][3] = (float) decomp.perspectiveW;
> +    
> +    // now translate
> +    translate3d((float) decomp.translateX, (float) decomp.translateY, (float) decomp.translateZ);
> +    
> +    // apply rotation
> +    float xx, xy, xz, xw, yy, yz, yw, zz, zw;
> +
> +    xx = (float) (decomp.quaternionX * decomp.quaternionX);
> +    xy = (float) (decomp.quaternionX * decomp.quaternionY);
> +    xz = (float) (decomp.quaternionX * decomp.quaternionZ);
> +    xw = (float) (decomp.quaternionX * decomp.quaternionW);
> +    yy = (float) (decomp.quaternionY * decomp.quaternionY);
> +    yz = (float) (decomp.quaternionY * decomp.quaternionZ);
> +    yw = (float) (decomp.quaternionY * decomp.quaternionW);
> +    zz = (float) (decomp.quaternionZ * decomp.quaternionZ);
> +    zw = (float) (decomp.quaternionZ * decomp.quaternionW);

Why is this using floats, not doubles?

> +    TransformationMatrix u(1 - 2 * (yy + zz), 2 * (xy - zw), 2 * (xz + yw), 0, 
> +                         2 * (xy + zw), 1 - 2 * (xx + zz), 2 * (yz - xw), 0,
> +                         2 * (xz - yw), 2 * (yz + xw), 1 - 2 * (xx + yy), 0,
> +                         0, 0, 0, 1);

Maybe a comment explaining what the heck that math is doing?

Longer variable name than 'u' please.

> Index: WebCore/platform/graphics/transforms/TransformationMatrix.h
> ===================================================================
> --- WebCore/platform/graphics/transforms/TransformationMatrix.h	(revision 40698)
> +++ WebCore/platform/graphics/transforms/TransformationMatrix.h	(working copy)
> @@ -27,21 +27,7 @@
>  #define TransformationMatrix_h
>  
>  #if PLATFORM(CG)
> -#include <CoreGraphics/CGAffineTransform.h>

> +typedef struct CGAffineTransform CGAffineTransform;

Rather than the typedef, just keep the include?

>  class TransformationMatrix {
>  public:

> +    TransformationMatrix() { makeIdentity(); }
> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }

How about m_matrix = t.m_matrix ?


> +    TransformationMatrix& operator =(const TransformationMatrix &t)
> +    {
> +        setMatrix(reinterpret_cast<const double*>(t.m_matrix));

Nasty cast. Can we have a private setter that takes a TransformationMatrix::Matrix4?

> +    // This form preserves the double math from input to output
> +    void map(double x, double y, double *x2, double *y2) const { multVecMatrix(x, y, *x2, *y2); }

Use references for the result params, I think.

> +    // Map a 2D point through the transform, returning a 2D point.
> +    // Note that this ignores the z component.

What does 'ignores the z component' really mean?

> +    void reset() { makeIdentity(); }

Do we need both reset and makeIdentity?

> +
> +    // this = this * mat
> +    TransformationMatrix& multiply(const TransformationMatrix& t) { return *this *= t; }

Rather than making use of operator*, would be a little easier to grok with use of multLeft.

> +
> +    // this = mat * this
> +    TransformationMatrix& multLeft(const TransformationMatrix& mat);
> +    
>      TransformationMatrix& scale(double);
>      TransformationMatrix& scale(double sx, double sy);
> +    TransformationMatrix& scale3d(double sx, double sy, double sz);
>      TransformationMatrix& scaleNonUniform(double sx, double sy);

How do scale() and scaleNonUniform() differ?

> +    TransformationMatrix& rotate3d(double x, double y, double z, double angle);

Should have a comment about normalizing x,y,z

>      TransformationMatrix& flipX();
>      TransformationMatrix& flipY();

No flipZ()?

>      bool isInvertible() const;
>      TransformationMatrix inverse() const;

Why is this not invert() (non-const), when all the other methods change |this|?

> +    TransformationMatrix& operator*=(const TransformationMatrix& t)
> +    {
> +        *this = *this * t;
> +        return *this;
> +    }

A "this is a multRight" comment might be good here.

> +    
> +    TransformationMatrix operator*(const TransformationMatrix& t)

Should be |const|


> +    void setMatrix(const double* m)
> +    {
> +        if (m && m != reinterpret_cast<double*>(m_matrix))
> +            memcpy(m_matrix, m, 16 * sizeof(double));
> +    }

I think this should go away.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 40718)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,67 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=6868
> +
> +        These tests started failing when I changed TransformationMatrix
> +        to be platform independent. This is due to slight differences
> +        in rounding because I am now using doubles rather than floats
> +        to do the matrix operations.

Rather than the "story-telling" type changelog, I think it's better to just
say "fix the tests because ...."

> Index: LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt
> ===================================================================
> --- LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt	(revision 40698)
> +++ LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt	(working copy)
> @@ -9,7 +9,7 @@ layer at (0,0) size 480x360
>        RenderPath {polyline} at (211.66,216.46) size 49.87x59.99 [transform={m=((0.97,0.26)(-0.26,0.97)) t=(0.00,0.00)}] [stroke={[type=SOLID] [color=#B4B4B4] [stroke width=9.00]}] [data="M200.00,120.00 L200.00,140.00 L220.00,140.00 L220.00,160.00"]
>        RenderPath {line} at (44.26,12.13) size 29.49x53.74 [stroke={[type=SOLID] [color=#B4B4B4] [stroke width=3.00]}] [fill={[type=SOLID] [color=#000000]}] [data="M40.00,50.00 L20.00,10.00"]
>        RenderPath {line} at (123.13,11.26) size 105.74x55.49 [stroke={[type=SOLID] [color=#B4B4B4] [stroke width=3.00]}] [fill={[type=SOLID] [color=#000000]}] [data="M160.00,50.00 L80.00,10.00"]
> -      RenderPath {line} at (59,38.35) size 117.00x1.30 [stroke={[type=SOLID] [color=#B4B4B4]}] [fill={[type=SOLID] [color=#000000]}] [data="M30.00,30.00 L120.00,30.00"]
> +      RenderPath {line} at (59,38.35) size 117x1.30 [stroke={[type=SOLID] [color=#B4B4B4]}] [fill={[type=SOLID] [color=#000000]}] [data="M30.00,30.00 L120.00,30.00"]

Why are we seeing decimal numbers here when we didn't before?

> Index: LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected.txt
> ===================================================================
> --- LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected.txt	(revision 40698)
> +++ LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected.txt	(working copy)
> @@ -29,8 +29,8 @@ layer at (0,0) size 450x500
>                  chunk 1 text run 3 at (85.93,25.89) startOffset 0 endOffset 5 width 65.00: " Good"
>            RenderSVGInlineText {#text} at (0,0) size 0x0
>        RenderPath {line} at (50,129) size 350x2 [stroke={[type=SOLID] [color=#0000FF] [stroke width=2.00]}] [fill={[type=SOLID] [color=#000000]}] [data="M50.00,130.00 L400.00,130.00"]
> -      RenderSVGText {text} at (58,125) size 351x20 contains 1 chunk(s)
> -        RenderSVGInlineText {#text} at (-11,-15) size 351x20
> +      RenderSVGText {text} at (58,125) size 350x20 contains 1 chunk(s)
> +        RenderSVGInlineText {#text} at (-10,-15) size 350x20
>            chunk 1 text run 1 at (58.00,125.00) startOffset 0 endOffset 13 width 142.00: "Batik is Good"
>        RenderSVGContainer {g} at (40,146) size 330x55.52 [transform={m=((1.00,0.00)(0.00,1.00)) t=(30.00,150.00)}]
>          RenderSVGContainer {use} at (49.55,158.48) size 320.45x43.05
> @@ -48,8 +48,8 @@ layer at (0,0) size 450x500
>                  chunk 1 text run 3 at (237.33,22.64) startOffset 0 endOffset 5 width 65.00: " Good"
>            RenderSVGInlineText {#text} at (0,0) size 0x0
>        RenderPath {line} at (50,239) size 150x2 [stroke={[type=SOLID] [color=#0000FF] [stroke width=2.00]}] [fill={[type=SOLID] [color=#000000]}] [data="M50.00,240.00 L200.00,240.00"]
> -      RenderSVGText {text} at (58,240) size 143x29 contains 1 chunk(s)
> -        RenderSVGInlineText {#text} at (-9,-23) size 143x29
> +      RenderSVGText {text} at (58,240) size 142x27 contains 1 chunk(s)
> +        RenderSVGInlineText {#text} at (-8,-22) size 142x27
>            chunk 1 text run 1 at (58.00,240.00) startOffset 0 endOffset 13 width 142.00: "Batik is Good"
>        RenderSVGContainer {g} at (224,204) size 176x70 [transform={m=((1.00,0.00)(0.00,1.00)) t=(220.00,220.00)}]
>          RenderSVGContainer {use} at (239.29,228.42) size 160.71x43.16

I don't think these RenderSVGText size changes are caused by your changes. You should not change these expecteds.
This also applies to lots of other updated results.
> Index: LayoutTests/svg/custom/getTransformToElement.svg
> ===================================================================
> --- LayoutTests/svg/custom/getTransformToElement.svg	(revision 40698)
> +++ LayoutTests/svg/custom/getTransformToElement.svg	(working copy)
> @@ -14,7 +14,7 @@
>              ctm = referenceElement.getTransformToElement(document.getElementById("redRect"));
>              if (ctm.a.toFixed(3) == 0.354 && ctm.b.toFixed(3) == -0.354 &&
>                  ctm.c.toFixed(3) == 0.354 && ctm.d.toFixed(3) == 0.354 &&
> -                 ctm.e.toFixed(3) == -107.071 && ctm.f.toFixed(15) == 0.0) {
> +                 ctm.e.toFixed(3) == -107.071 && ctm.f.toFixed(8) == 0.0) {
>                 try {
>                   var ctm = referenceElement.getTransformToElement(document.getElementById("group0"));
>                 } catch(e) {

Inadvertent change?

[Chris Marrin]

Created attachment 27423 [details]
replacement patch

This patch address most, if not all of Simon's comments. I've reviewed all the changed test results and I believe that all the size differences are due to rounding differences. The on that looks suspicious is LayoutTests/svg/batik/textGlyphOrientationHorizontal.html, which differs by 2 pixels in some sizes. But I've looked at the pixel test results and everything passes and looks good. So even here I think the difference must have to do with the precision.

[Chris Marrin]

This replacement patch also addresses a build problem on the Windows platform. With it, the Windows build works fine.

[Simon Fraser (smfr)]

Comment on attachment 27423 [details]
replacement patch

> +
> +static double v3Length(Vector3 a) 

Extra space at the end.

> +{
> +    return sqrt((a[0] * a[0])+(a[1] * a[1]) + (a[2] * a[2]));

Missing space.

> +static void v3Combine (const Vector3 a, const Vector3 b, Vector3 result, double ascl, double bscl) 

Extra space before the (, and at the end.

> +FloatPoint TransformationMatrix::mapPoint(const FloatPoint& p) const
>  {
> -    return rotate(rad2deg(atan2(y, x)));
> +    // If the matrix has 3D components, the z component of the result is
> +    // dropped, effectively projecting the point into the z=0 plane

There's no need to have the same comment in the header and the implementation.

> +    // If the matrix has 3D components, the z component of the result is
> +    // dropped, effectively projecting the point into the z=0 plane

Ditto (and 2-3 more places.


> +// This method returns the identity matrix if it is not invertible.
> +// Use isInvertible() before calling this if you need to know.

This comment belongs in the header.

> Index: WebCore/platform/graphics/transforms/TransformationMatrix.h
> ===================================================================

> +    void setMatrix(const Matrix4 m)
> +    {
> +        if (m && m != m_matrix)
> +            memcpy(m_matrix, m, 16 * sizeof(double));

sizeof(Matrix4) better, I think.

r=me with those nits addressed.

[Chris Marrin]

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-36-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-80-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-81-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-82-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/coords-trans-01-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/coords-trans-02-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/coords-trans-06-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/coords-viewattr-01-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-elem-01-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-elem-02-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-elem-03-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-elem-04-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-elem-07-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/painting-marker-01-f-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/painting-marker-02-f-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/paths-data-05-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/paths-data-06-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/paths-data-07-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/paths-data-09-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/paths-data-15-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/pservers-grad-09-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/pservers-grad-12-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/pservers-grad-17-b-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/struct-frag-02-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/struct-frag-03-t-expected.txt
Sending        LayoutTests/platform/mac/svg/W3C-SVG-1.1/struct-group-03-t-expected.txt
Sending        LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected.txt
Sending        LayoutTests/platform/mac/svg/batik/text/textProperties-expected.txt
Sending        LayoutTests/platform/mac/svg/batik/text/verticalText-expected.txt
Sending        LayoutTests/platform/mac/svg/batik/text/verticalTextOnPath-expected.txt
Sending        LayoutTests/platform/mac/svg/carto.net/slider-expected.txt
Sending        LayoutTests/platform/mac/svg/carto.net/tabgroup-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/glyph-selection-lang-attribute-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/invalid-css-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/js-late-marker-and-object-creation-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/js-late-marker-creation-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/preserve-aspect-ratio-syntax-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/shapes-supporting-markers-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/use-css-no-effect-on-shadow-tree-expected.txt
Sending        LayoutTests/platform/mac/svg/custom/viewbox-syntax-expected.txt
Sending        LayoutTests/platform/mac/svg/hixie/perf/001-expected.txt
Sending        LayoutTests/platform/mac/svg/hixie/perf/002-expected.txt
Sending        LayoutTests/platform/mac/svg/hixie/perf/007-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-align-01-b-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-align-05-b-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-fonts-01-t-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-text-04-t-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-text-05-t-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-text-06-t-expected.txt
Sending        LayoutTests/platform/mac/svg/text/text-tref-01-b-expected.txt
Sending        LayoutTests/svg/custom/getTransformToElement.svg
Sending        WebCore/ChangeLog
Sending        WebCore/css/WebKitCSSMatrix.cpp
Sending        WebCore/html/CanvasRenderingContext2D.cpp
Sending        WebCore/html/HTMLCanvasElement.cpp
Sending        WebCore/platform/graphics/FloatPoint.cpp
Sending        WebCore/platform/graphics/Image.cpp
Sending        WebCore/platform/graphics/cg/GraphicsContextCG.cpp
Sending        WebCore/platform/graphics/cg/PatternCG.cpp
Sending        WebCore/platform/graphics/cg/TransformationMatrixCG.cpp
Sending        WebCore/platform/graphics/transforms/ScaleTransformOperation.h
Sending        WebCore/platform/graphics/transforms/TransformationMatrix.cpp
Sending        WebCore/platform/graphics/transforms/TransformationMatrix.h
Sending        WebCore/rendering/RenderForeignObject.cpp
Sending        WebCore/rendering/RenderLayer.cpp
Sending        WebCore/rendering/RenderPath.cpp
Sending        WebCore/rendering/RenderSVGImage.cpp
Sending        WebCore/rendering/RenderSVGRoot.cpp
Sending        WebCore/rendering/RenderSVGText.cpp
Sending        WebCore/rendering/RenderSVGViewportContainer.cpp
Sending        WebCore/rendering/SVGCharacterLayoutInfo.cpp
Sending        WebCore/rendering/SVGRootInlineBox.cpp
Sending        WebCore/svg/SVGAnimateMotionElement.cpp
Sending        WebCore/svg/SVGPreserveAspectRatio.cpp
Sending        WebCore/svg/SVGSVGElement.cpp
Sending        WebCore/svg/SVGTransform.cpp
Sending        WebCore/svg/SVGTransformDistance.cpp
Sending        WebCore/svg/graphics/SVGResourceClipper.cpp
Sending        WebCore/svg/graphics/SVGResourceMarker.cpp
Transmitting file data .................................................................................
Committed revision 40761.