[cairo] Status of ps-pdf-dpi branch

[Carl Worth]

On Sat, 13 May 2006 23:32:05 +0200, Emmanuel Pacaud wrote:
> I've done some additionnal work on ps-pdf-dpi branch, which consist of:

Hi Emmmanuel,

Thanks for picking this up again.

>   - suppression of cairo_foo_surface_set_dpi functions.
>   - new cairo_surface_set_fallback_resolution
>   - fix for svg backend

Those sound like good things.

>   - glyph are incorrectly scaled when resolution != 72.0 ppi.
>   - create_similar for ps and pdf backend returns an image with wrong size

I had hoped to be able to look at these two issues today, but I ran
into other problems earlier in the ps-pdf-dpi branch. What I'd like
the branch to look like is a clean progression of the code
development. Ideally, the series might look something like this:

	Replace device_{x,y}_offset with device_transform matrix
	Rename device_transform to surface_to_backend
	Replace cairo_<foo>_surface_set_dpi with cairo_surface_set_fallback_resolution
	Add testcase for set_fallback resolution
	Fix fallback resolution bug in SVG backend

And at that point it would be ready for the new work I would do on
"Fix fallback resolution bug in {PS,PDF} backend".

The current branch doesn't look much like that. I was hoping I could
cherry-pick the first two items, then simply git diff from HEAD to
branch (ignoring SVG-specific stuff) to get the third.

But I got stuck on the first step. Here's the patch that would be
"Replace device_{x,y}_offset with device_transform matrix". It's
obviously a big patch, since there's a bunch of renaming. But it
should be a big, obvious patch, and instead it's a big, scary
patch. That's because sprinkled in among the obvious renaming of
things there is some other stuff going on as well. It's all of this
other stuff that really needs to be moved to separate patches, (where
it's easier to review and reject if needed).

I'll pretend I'm quoting this from an email message to make it easier
to read, and I'll only quote the "scary" bits, (that is, ignoring the
straightforward renaming):

> commit 08c36b29f7ec609b90234a9790f0e5b629439e01
> Author: Christian Biesinger <cbiesinger at web.de>
> Date:   Sat Apr 29 18:29:51 2006 +0200
> 
>     Make fallback images for PS/PDF draw in the correct size
>     
>     ...by readding device scales in the form of a generic device_transform
>     
>     Renames _cairo_surface_has_device_offset_or_scale to
>     _cairo_surface_has_device_transform, adds
>     _cairo_path_transform, and changes all surface->backend
>     fixups to use this new matrix.
> 
> diff --git a/src/cairo-clip.c b/src/cairo-clip.c
> index 06c2abd..556dc56 100644
> --- a/src/cairo-clip.c
> +++ b/src/cairo-clip.c
...
> diff --git a/src/cairo-paginated-surface.c b/src/cairo-paginated-surface.c
> index 6f5d210..f5e7b90 100644
> --- a/src/cairo-paginated-surface.c
> +++ b/src/cairo-paginated-surface.c
> @@ -251,6 +251,10 @@ _paint_page (cairo_paginated_surface_t *
>  							  width,
>  							  height);
>  
> +	cairo_matrix_init_scale (&image->device_transform,
> +				 surface->x_dpi / 72.0,
> +				 surface->y_dpi / 72.0);
> +
>  	_cairo_meta_surface_replay (surface->meta, image);

So that one's not actually so scary, but it definitely doesn't belong
here with this simple renaming. This looks like a real fix, perhaps?
So it deserves its own commit with its own message.

> @@ -561,10 +553,22 @@ cairo_surface_mark_dirty_rectangle (cair
>  
>      if (surface->backend->mark_dirty_rectangle) {
>  	cairo_status_t status;
> +	double backend_x, backend_y, backend_width, backend_height;
> +
> +	/* Convert to backend coordinates */
> +	backend_x = x;
> +	backend_y = y;
> +	backend_width = width;
> +	backend_height = height;
> +	_cairo_matrix_transform_bounding_box (&surface->device_transform,
> +					      &backend_x, &backend_y,
> +					      &backend_width,
> +					      &backend_height);
> +
>  	
>  	status = surface->backend->mark_dirty_rectangle (surface,
> -                                                         SURFACE_TO_BACKEND_X(surface, x),
> -                                                         SURFACE_TO_BACKEND_Y(surface, y),
> +                                                         backend_x,
> +                                                         backend_y,
>  							 width, height);

The above might actually be fine, and it's perhaps even appropriately
"non-scary" in that it is preserving the exact same behavior as before,
(though only because we "know" that the device_transform matrix is only
standing in for a translation right now, and will never have
rotate/shear anyway so _cairo_matrix_transform_bounding_box won't ever
do anything "funny").

But anything that touches _mark_dirt_rectangle sets off a red flag in
my head. This is because we don't currently test this function at all,
yet I know that there's code out there that's relying on it working
right. So we do need to be careful to not accidentally change the
semantics of this function (such as changing the coordinate-space in
which the arguments are interpreted).

It would probably be a good idea to write a test case for this
function, (need a reliable way to force a fallback I suppose...).

>  cairo_bool_t
> -_cairo_surface_has_device_offset_or_scale (cairo_surface_t *surface)
> +_cairo_surface_has_device_transform (cairo_surface_t *surface)
>  {
> -    return (surface->device_x_offset != 0.0 ||
> -	    surface->device_y_offset != 0.0);
> +    int itx, ity;
> +    return (!_cairo_matrix_is_integer_translation(&surface->device_transform,
> +						  &itx, &ity) ||
> +	    itx != 0 || ity != 0);
>  }

So that's definitely a bunch more than a renaming, (where a renaming
would have worked just fine). And the above is just plain hard to read
too. Is this trying to do something like:

	return (! _cairo_matrix_is_identity (&surface->device_transform))

And if so, is there any reason we don't just write that function
instead of making a surface function for "has device transform" (which
doesn't make much sense to me as a name anyway).
 
> @@ -1542,10 +1543,22 @@ _cairo_surface_get_extents (cairo_surfac
>  
>      status = surface->backend->get_extents (surface, rectangle);
>  
> -    rectangle->x = SURFACE_TO_BACKEND_X(surface, rectangle->x);
> -    rectangle->y = SURFACE_TO_BACKEND_Y(surface, rectangle->y);
> -    rectangle->width = rectangle->width - surface->device_x_offset;
> -    rectangle->height = rectangle->height - surface->device_y_offset;
> +    /* get_extents returns coordinates in backend coordinates.
> +     * Therefore, we apply the device transformation instead of the inverse
> +     * transform.
> +     */
> +    tx = rectangle->x;
> +    ty = rectangle->y;
> +    cairo_matrix_transform_point (&surface->device_transform, &tx, &ty);
> +
> +    /* All drawing is clipped to the backend size, so width and height may
> +     * need to be adjusted. Width and height are always the difference between
> +     * the backend dimensions and the topleft corner after transformation.
> +     */
> +    rectangle->x = (short)tx;
> +    rectangle->y = (short)ty;
> +    rectangle->width = rectangle->width - rectangle->x;
> +    rectangle->height = rectangle->height - rectangle->y;
>  
>      return status;
>  }

_cairo_surface_get_extents is another red-flag function. We've had (or
still have?) a class of bugs from functions calling and implementing
this function with different semantics. I think we're going to finally
need to break down and implement multiple functions with more
precisely defined semantics to fix the bugs.

Anyway, back to the patch at hand, perhaps I should retract my
suggestion that this first patch should be a renaming-only patch. It
would be possible to do something that introduced the matrix and only
ever used the translation components. But then subsequent patches
would just start using the scaling components and in the review we
could easily miss parts that should have been updated but aren't.

For example, why does the above transform the origin of the rectangle
but not also adjust the scale of width/height by the matrix? The
addition of scale is new here, so the behavior should at least be
noted in the documentation of this function, but the patch as is
doesn't touch that.

---

Anyway, that wraps up a bit of review from me.

Emmanuel, I know that this isn't your code originally, but if you are
interested in/available for reworking the beginnings of this branch
while I'm off playing with line widths, then that would be great.

Otherwise, I'll get back to this next.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20060516/8f1e959b/attachment.pgp