GNOME Bugzilla – Bug 751357
ImageBrush doesn't support Viewbox or rotation transformations
Last modified: 2015-08-13 09:48:35 UTC
Created attachment 305888 [details] XPS document The attached document renders incorrectly with libgxps because it doesn't use the ImageBrush Viewbox attribute and doesn't support rotation transformations.
Created attachment 305889 [details] Incorrect rendering The current incorrect rendering
Created attachment 305890 [details] Correct rendering Correct rendering
Created attachment 305893 [details] [review] Support ImageBrush Viewbox and rotation/shearing matrices This patch modifies ImageBrush handling to support clipping based on the Viewbox and multiplies the transform matrix to support rotation or shearing transformations. Viewbox is given in units of 1/96 of an inch, which means it depends on the image resolution. This modifies image loading (including using libpng instead of cairo) in order to capture the resolution and wraps it in a GXPSImage struct.
Review of attachment 305893 [details] [review]: Thanks for the patch. I have just a few minor comments ::: libgxps/gxps-images.c @@ +67,3 @@ + + msg = png_get_error_ptr (png_ptr); + *msg = g_strdup (error_msg); Could libpng call this callback more than once? In that case we could be leaking the message. @@ +104,3 @@ + uint8_t red = base[0]; + uint8_t green = base[1]; + uint8_t blue = base[2]; Nit: Extra spaces here between uint8_t and variable name. @@ +195,3 @@ + if (png == NULL) { + fill_png_error (error, image_uri, NULL); + goto error; I'm not a fan of gotos, I prefer to duplicate the cleanup code, or even better refactor the function moving blocks to helper functions that return a boolean, so reduce the amout of duplicated cleanup code. @@ +281,3 @@ + + image = g_slice_new (GXPSImage); + image->surface = NULL; There isn't any early return between this and the surface = cairo_image_surface_create_for_data(), so we don't probably need this. We could also use g_slice_new0() instead. @@ +322,3 @@ + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); + image = NULL; Use gxps_image_free and you don't need to explicitly free the surface here. @@ +490,3 @@ jpeg_destroy_decompress (&cinfo); + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); Ditto. @@ +528,3 @@ + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); Ditto. @@ +563,3 @@ + cairo_status_to_string (cairo_surface_status (image->surface))); + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); Ditto. @@ +826,3 @@ + cairo_status_to_string (cairo_surface_status (image->surface))); + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); Ditto. @@ +839,3 @@ fill_tiff_error (error, image_uri); + cairo_surface_destroy (image->surface); + g_slice_free (GXPSImage, image); Ditto. @@ +939,3 @@ + return; + + cairo_surface_destroy (image->surface); And here we would need to check surface is not NULL before calling destroy.
Created attachment 308279 [details] [review] Support ImageBrush Viewbox and rotation/shearing matrices (In reply to Carlos Garcia Campos from comment #4) > Review of attachment 305893 [details] [review] [review]: > > Thanks for the patch. I have just a few minor comments > > ::: libgxps/gxps-images.c > @@ +67,3 @@ > + > + msg = png_get_error_ptr (png_ptr); > + *msg = g_strdup (error_msg); > > Could libpng call this callback more than once? In that case we could be > leaking the message. There's a longjmp just after this, so no, I don't see how it could be called more than once. > @@ +104,3 @@ > + uint8_t red = base[0]; > + uint8_t green = base[1]; > + uint8_t blue = base[2]; > > Nit: Extra spaces here between uint8_t and variable name. fixed > @@ +195,3 @@ > + if (png == NULL) { > + fill_png_error (error, image_uri, NULL); > + goto error; > > I'm not a fan of gotos, I prefer to duplicate the cleanup code, or even > better refactor the function moving blocks to helper functions that return a > boolean, so reduce the amout of duplicated cleanup code. fixed > @@ +281,3 @@ > + > + image = g_slice_new (GXPSImage); > + image->surface = NULL; > > There isn't any early return between this and the surface = > cairo_image_surface_create_for_data(), so we don't probably need this. We > could also use g_slice_new0() instead. Changed to g_slice_new0. The libpng functions could call png_error_callback and longjmp so it could return before the surface is created. > @@ +322,3 @@ > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > + image = NULL; > > Use gxps_image_free and you don't need to explicitly free the surface here. fixed > @@ +490,3 @@ > jpeg_destroy_decompress (&cinfo); > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > > Ditto. fixed > @@ +528,3 @@ > > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > > Ditto. fixed > @@ +563,3 @@ > + cairo_status_to_string (cairo_surface_status (image->surface))); > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > > Ditto. fixed > @@ +826,3 @@ > + cairo_status_to_string (cairo_surface_status (image->surface))); > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > > Ditto. fixed > @@ +839,3 @@ > fill_tiff_error (error, image_uri); > + cairo_surface_destroy (image->surface); > + g_slice_free (GXPSImage, image); > > Ditto. fixed > @@ +939,3 @@ > + return; > + > + cairo_surface_destroy (image->surface); > > And here we would need to check surface is not NULL before calling destroy. fixed
Comment on attachment 308279 [details] [review] Support ImageBrush Viewbox and rotation/shearing matrices Thank you! pushed to git master.