Bug 18694 - [CAIRO] Problem with rotation in a given matrix in SVG
Summary: [CAIRO] Problem with rotation in a given matrix in SVG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Cairo
Depends on:
Blocks:
 
Reported: 2008-04-23 02:24 PDT by Dirk Schulze
Modified: 2008-08-01 12:26 PDT (History)
1 user (show)

See Also:


Attachments
SVGCairoRotation (1.21 KB, patch)
2008-04-23 02:47 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
SVGCairoRotation (1.63 KB, patch)
2008-06-13 15:13 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVGCairoRotation (2.11 KB, patch)
2008-06-13 23:55 PDT, Dirk Schulze
eric: review+
Details | Formatted Diff | Diff
LayoutTest (461 bytes, image/svg+xml)
2008-06-14 07:00 PDT, Dirk Schulze
eric: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2008-04-23 02:24:16 PDT
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.
Comment 1 Dirk Schulze 2008-04-23 02:47:29 PDT
Created attachment 20768 [details]
SVGCairoRotation

see comment above
Comment 2 Dirk Schulze 2008-04-24 12:53:04 PDT
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);

Comment 3 Dirk Schulze 2008-04-27 23:08:06 PDT
(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.
Comment 4 Dirk Schulze 2008-04-27 23:19:51 PDT
> have to change b to a and a to b in AffineTransform.

I mean b to c and c to b.
Comment 5 Eric Seidel (no email) 2008-04-30 23:49:56 PDT
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!
Comment 6 David Jaša 2008-05-03 07:02:02 PDT
Maybe WK could use this test as layout test: http://www.carto.net/papers/svg/samples/matrix.shtml
Comment 7 Dirk Schulze 2008-05-03 13:02:20 PDT
(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).
Comment 8 Dirk Schulze 2008-06-13 15:13:45 PDT
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).
Comment 9 Dirk Schulze 2008-06-13 23:55:47 PDT
Created attachment 21694 [details]
SVGCairoRotation

forgot b(), setB() and c(), setC()
Comment 10 Dirk Schulze 2008-06-14 07:00:43 PDT
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.
Comment 11 Dirk Schulze 2008-06-19 11:17:55 PDT
The patch solves problems with transformed images on http://paulbakaus.com/lab/js/coverflow/ too.
Comment 12 Eric Seidel (no email) 2008-06-26 12:01:08 PDT
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.  :(
Comment 13 Mark Rowe (bdash) 2008-07-26 23:02:48 PDT
Landed in r35399.
Comment 14 Eric Seidel (no email) 2008-08-01 12:15:16 PDT
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.)
Comment 15 Eric Seidel (no email) 2008-08-01 12:26:57 PDT
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.