Bug 18694 – [CAIRO] Problem with rotation in a given matrix in SVG

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Dirk Schulze]

There is a problem with rotation of objects, if you use matrix() instead of rotate(). I opened the bug just to be sure not to forget anything.

You can fix the problem for svg with:

AffineTransform::AffineTransform(double a, double b, double c, double d, double tx, double ty)
{
    cairo_matrix_init(&m_transform, a, b, c, d, tx, ty);
}

Is there anything else to do? Perhaps setB(), b() and setC(), c() ?

In cairo a matrix is initialised with:
cairo_matrix_init(cairo_matrix_t *matrix, xx, yx, xy, yy, x0, y0);
So setB(), b() and setC() and c() could be wrong initialised.

[Dirk Schulze]

Created attachment 20768 [details]
SVGCairoRotation

see comment above

[Dirk Schulze]

The values are given correctly to AffineTransform (in SVG-specification):
like matrix(a,b,c,d,tx,ty)
If you transform an SVG-object with Cairo, b and c are interchanged. That means:
matrix(a,c,b,d,tx,ty) is the same on cairo like the matrix above on SVG. Thats why the interchange of c and b in AffineTransform should be correct. But the SVG-object is displayed wrong with it.

I searched in GraphicsContestCairo and found the method
void GraphicsContext::concatCTM(const AffineTransform& transform)
In this method "transform" is changed back to a cairo_matrix_t with reinterpret_cast. 

I asked in IRC and it COULD be that this transformation interchange b and c in the background while transforming "transform" to cairo_matrix_t.
Perhaps, thats why cairo_matrix_init(&m_transform, a, c, b, d, tx, ty); doesn't work correctly.

So you should use a other possibility to change the type to cairo_matrix_t or you use cairo_matrix_init(&m_transform, a, b, c, d, tx, ty);

[Dirk Schulze]

(In reply to comment #2)
> I searched in GraphicsContestCairo and found the method
> void GraphicsContext::concatCTM(const AffineTransform& transform)
> In this method "transform" is changed back to a cairo_matrix_t with
> reinterpret_cast.

You can replace reinterpret_cast:

    const cairo_matrix_t* matrix = reinterpret_cast<const cairo_matrix_t*>(&transform);
    cairo_transform(cr, matrix);

to

    cairo_matrix_t matrix = cairo_matrix_t(transform);
    cairo_transform(cr, &matrix);

The code don't use reinterpret_cast but it doesn't solve the problem. You still have to change b to a and a to b in AffineTransform.
I searched through the source-code of webkit with grep and SVG is the only one that use this method. I shoulnd't be a problem to fix it that way.

[Dirk Schulze]

> have to change b to a and a to b in AffineTransform.

I mean b to c and c to b.

[Eric Seidel (no email)]

Comment on attachment 20768 [details]
SVGCairoRotation

This needs a layout test.  Doesn't it already affect some layout test result?  Also, your email is missing from your changelog.  Otherwise the patch looks great!

[David Jaša]

Maybe WK could use this test as layout test: http://www.carto.net/papers/svg/samples/matrix.shtml

[Dirk Schulze]

(In reply to comment #6)
> Maybe WK could use this test as layout test:
> http://www.carto.net/papers/svg/samples/matrix.shtml
> 

But not a good test in this case, since WebKitGtk doesn't support the <!ENTITY ...> -element (or at least partial).

[Dirk Schulze]

Created attachment 21690 [details]
SVGCairoRotation

I misunderstood the documentation of CGAffineTransform. The CGAffineTransform is the transpose of cairo_matrix. That means

a is xx on cairo, [1,1] on CGAffineTransform
b is yx on cairo, [1,2] on CGAffineTransform
c is xy on cairo, [2,1] on CGAffineTransform
d is yy on cairo, [2,2] on CGAffineTransform

and so on.

That means the handover of the values have to be AffineTransform(a, b, c, d, e, f).

[Dirk Schulze]

Created attachment 21694 [details]
SVGCairoRotation

forgot b(), setB() and c(), setC()

[Dirk Schulze]

Created attachment 21700 [details]
LayoutTest

I created a test because I haven't found a test case for rotation with a matrix in layouttests. This one draws two boxes. The red box is rotated with the attribute 'rotate' and the green box with a matrix (both with the same angle). You should only see the green box.

[Dirk Schulze]

The patch solves problems with transformed images on http://paulbakaus.com/lab/js/coverflow/ too.

[Eric Seidel (no email)]

Comment on attachment 21694 [details]
SVGCairoRotation

The patch looks fine.  Of course Dirk informed me over IRC that GTK doesn't actually have DumpRenderTree yet... so run-webkit-tests (And thus layout tests) don't work.  :(

[Mark Rowe (bdash)]

Landed in r35399.

[Eric Seidel (no email)]

Comment on attachment 21700 [details]
LayoutTest

this test could be much better.  If you want to test that certain rotations work as expected, you should put a second red box behind the green one, the green one would only ever cover all of the red if your bug was fixed (i.e. the rotations for those particular matrices were calculated correctly.)

[Eric Seidel (no email)]

Comment on attachment 21700 [details]
LayoutTest

My bad.  Looks fine.  Even better would be to just show the green box at 0,0 unrotated (no text below it).  That would require these to be in a <g> with another rotate(-10) applied.