Re: _cairo_surface_wrapper_get_target_extents() too small

Guillaume Ayoub <[email protected]>
Newsgroups gmane.comp.lib.cairo
Message-ID <1368445620.2072.24.camel@liZe76> 
Le vendredi 29 mars 2013 à 22:55 +0100, Guillaume Ayoub a écrit :
> Le vendredi 29 mars 2013 à 15:43 +0100, Simon Sapin a écrit :
> > Le 27/03/2013 13:12, Simon Sapin a écrit :
> > > Hi,
> > >
> > > When rendering to PDF with WeasyPrint a document containing a SVG image,
> > > SVG elements in the right and bottom of the image are not shown in some
> > > cases.
> > >
> > > git bisect indicates that the bug appeared in this cairo commit:
> > >
> > > http://cgit.freedesktop.org/cairo/commit/?id=09b42c748e9dbcc923560c7d8bf5298fbffe95ef
> > >
> > > Reverting it on top of today’s master fixes the bug.
> > >
> > >
> > > WeasyPrint has CairoSVG render the image to an intermediate cairo SVG
> > > surface, which is then used in surface pattern as a source to paint in
> > > the final PDF surface. The bug appears when the target context has a
> > > scale to make the image smaller than its "intrinsic" size. Making the
> > > SVG surface smaller (with its content scaled accordingly) and
> > > compensating with a scale on the target surface works around the problem.
> > >
> > > Apparently, the rectangle returned by
> > > _cairo_surface_wrapper_get_target_extents() is smaller than it should
> > > be. Something is transformed wrong, but I’m not sure what coordinate
> > > system each rectangle is supposed to be in.
> > 
> > 
> > Based on further research, it appears that the wrapper->transform value 
> > in _cairo_surface_wrapper_get_target_extents() should account for the 
> > target context’s transformation matrix, but doesn’t. wrapper->transform 
> > is always the identity matrix in our tests, even though the context has 
> > a non-identity transform when painting the surface pattern.
> > 
> > Removing any calls to cairo_save() and cairo_restore() also works around 
> > the issue, but for reasons we do not understand.
> 
> I finally think that I have found what's happening.
> 
> In cairo-analysis-surface, the transformation matrix is stored in
> surface->ctm, and this matrix is handled by _add_operation for all the
> operations added in the analysis surface. In
> _analyze_recording_surface_pattern, the ctm surface is set, everything
> is OK for the future operations.
> 
> But _analyze_recording_surface_pattern calls
> _cairo_recording_surface_replay_and_create_regions, calling
> _cairo_recording_surface_replay_internal, calling
> _cairo_surface_wrapper_get_target_extents. As replay_and_create_regions
> calls replay_internal without the surface_transform parameter, the
> original transformation matrix is lost. get_target_extents relies on
> wrapper.transform that is not set, and get_target_extents then uses the
> original size of the target to reduce the extents rectangle, instead of
> using the size multiplied by the transformation matrix.
> 
> A simple solution to fix this is to add a surface_transform parameter to
> _cairo_recording_surface_replay_and_create_regions (as it is done in
> _cairo_recording_surface_replay_with_clip), and to use the invert matrix
> of &tmp->ctm as surface_transform parameter of
> _cairo_recording_surface_replay_and_create_regions.

Here is a simple patch solving this issue.

This patch adds a "surface_transform" parameter to
replay_and_create_regions. It was already done in
surface_replay_with_clip (it is needed for the clip), but it's also
needed by replay_and_create_regions, because get_target_extents clips
the target surface for performance issues (that's what the "culprit"
commit 09b42c7 does).

I can provide further information if needed.
-- 
Guillaume

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo
cairo-fix-extents.patch (text/x-patch, 3.2 KB) 
diff --git a/src/cairo-analysis-surface.c b/src/cairo-analysis-surface.c
index 8516094..d3afd9f 100644
--- a/src/cairo-analysis-surface.c
+++ b/src/cairo-analysis-surface.c
@@ -144,7 +144,7 @@ _analyze_recording_surface_pattern (cairo_analysis_surface_t *surface,
     const cairo_surface_pattern_t *surface_pattern;
     cairo_analysis_surface_t *tmp;
     cairo_surface_t *source, *proxy;
-    cairo_matrix_t p2d;
+    cairo_matrix_t p2d, surface_transform;
     cairo_status_t status, analysis_status;
 
     assert (pattern->type == CAIRO_PATTERN_TYPE_SURFACE);
@@ -171,9 +171,11 @@ _analyze_recording_surface_pattern (cairo_analysis_surface_t *surface,
     cairo_matrix_multiply (&tmp->ctm, &p2d, &surface->ctm);
     tmp->has_ctm = ! _cairo_matrix_is_identity (&tmp->ctm);
 
+    surface_transform = tmp->ctm;
+    status = cairo_matrix_invert (&surface_transform);
     source = _cairo_surface_get_source (source, NULL);
     status = _cairo_recording_surface_replay_and_create_regions (source,
-								 &tmp->base);
+								 &surface_transform, &tmp->base);
     analysis_status = tmp->has_unsupported ? CAIRO_INT_STATUS_IMAGE_FALLBACK : CAIRO_INT_STATUS_SUCCESS;
     detach_proxy (proxy);
     cairo_surface_destroy (&tmp->base);
diff --git a/src/cairo-paginated-surface.c b/src/cairo-paginated-surface.c
index fe9ccee..8371474 100644
--- a/src/cairo-paginated-surface.c
+++ b/src/cairo-paginated-surface.c
@@ -354,7 +354,7 @@ _paint_page (cairo_paginated_surface_t *surface)
     surface->backend->set_paginated_mode (surface->target,
 	                                  CAIRO_PAGINATED_MODE_ANALYZE);
     status = _cairo_recording_surface_replay_and_create_regions (surface->recording_surface,
-								 analysis);
+								 NULL, analysis);
     if (status)
 	goto FAIL;
 
diff --git a/src/cairo-recording-surface-private.h b/src/cairo-recording-surface-private.h
index 0235b0f..aee7887 100644
--- a/src/cairo-recording-surface-private.h
+++ b/src/cairo-recording-surface-private.h
@@ -167,6 +167,7 @@ _cairo_recording_surface_replay_with_clip (cairo_surface_t *surface,
 
 cairo_private cairo_status_t
 _cairo_recording_surface_replay_and_create_regions (cairo_surface_t *surface,
+						    const cairo_matrix_t *surface_transform,
 						    cairo_surface_t *target);
 cairo_private cairo_status_t
 _cairo_recording_surface_replay_region (cairo_surface_t			*surface,
diff --git a/src/cairo-recording-surface.c b/src/cairo-recording-surface.c
index c6d34c0..0f1a06d 100644
--- a/src/cairo-recording-surface.c
+++ b/src/cairo-recording-surface.c
@@ -1917,9 +1917,10 @@ _cairo_recording_surface_replay_with_clip (cairo_surface_t *surface,
  */
 cairo_status_t
 _cairo_recording_surface_replay_and_create_regions (cairo_surface_t *surface,
+						    const cairo_matrix_t *surface_transform,
 						    cairo_surface_t *target)
 {
-    return _cairo_recording_surface_replay_internal ((cairo_recording_surface_t *) surface, NULL, NULL,
+    return _cairo_recording_surface_replay_internal ((cairo_recording_surface_t *) surface, NULL, surface_transform,
 						     target, NULL,
 						     CAIRO_RECORDING_CREATE_REGIONS,
 						     CAIRO_RECORDING_REGION_ALL);
signature.asc (application/pgp-signature, 490 B) 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQEcBAABAgAGBQJRkNK0AAoJEFDXmmt3d/ODvPwH/Ajcgi26Lq+kcMGgkN1aeW+c
jpH7ZJy9jgVk493y9dPbxHvu7JuUfUcO1xyE2QrOtRjyWE5ZcmeBY+bff8DDwWzr
1XfEG9EI9SzEixOxb8GAmrc4Nf5yVMXJLtIPBKkSA/eycfl6+2SI3XOnngiYStM6
+Umq5QfCgXtTKBJrftzSIY+7WTRdnxQztbB+hVMS494TRmVQZG73LtCQiY0wuJSy
Ah7O283AHEoDM+XhNqoKDX4YQdBBuJOjwIBYLBsg6t3IX4f6z6BI+R8iwz9Il2XT
8SP/jq4hzeqpa5t5fC13xmNof0aLuZf5/v5lfoTmqj6ud1z9g8IUkFPuAI2ezg4=
=rScX
-----END PGP SIGNATURE-----
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.