Re: Image transformations

[Eli Zaretskii]

> Date: Tue, 11 Jun 2019 21:02:33 +0100
> From: Alan Third <address@hidden>
> Cc: address@hidden
> 
> On Tue, Jun 11, 2019 at 08:10:41AM +0300, Eli Zaretskii wrote:
> >   . The :crop parameter of image specs doesn't seem to be documented 
> > anywhere.  We previously supported it without documenting only for 
> > ImageMagick, which was already a problem; now it's a much bigger problem.
> >   . The ELisp manual doesn't mention that :rotation is generally supported 
> > only for multiples of 90 deg, except if ImageMagick is available.
> >   . The transformation matrix used by the implementation is not described; 
> > one needs to guess what its components mean, or search the Internet for 
> > docs of XRender and/or Cairo, which are generally not helpful enough, 
> > especially the XRender one.
> 
> I hope the attached patch goes some way to solving these issues.

It's good progress, thanks.  See a few comments below.

> >   . There are no tests, AFAICT.  We should have a simple manual test
> >   which could be used to exercise all of the transformations,
> >   separately and in combinations.  I wonder how did the people who
> >   worked on the implementations for different platforns verify that
> >   the results are consistent, especially given the lack of
> >   documentation (e.g., is rotation of 90 deg clockwise or
> >   counter-clockwise?).
> 
> Is there some example of how to write a manual test like this?

See test/manual/redisplay-testsuite.el, for example.

The idea is to show some instructions, and then let the user/tester
invoke operations being tested and observe the results.  The epected
results should be described as part of the instructions.

> >  . I question the wisdom of the current definition of
> >  image-transforms-p. First, it returns a simple yes/no value, under
> >  the assumption that either all of the transformations are supported
> >  or none of them; IMO, it is better to return a list of supported
> >  capabilities instead. Second, it doesn't distinguish between
> >  capability to rotate by arbitrary angles and by 90 deg multiples
> >  only, which will require Lisp programs to test for ImageMagick,
> >  something that undermines the very raison d'etre of this function
> 
> My hope was that MS Windows support for affine transform matrices
> would be forthcoming quite quickly in which case there would be no
> need to differentiate between the different types of transform (if one
> is supported, they all are), but perhaps that’s hoping for too much. :)

Well, they don't really queue up for the job of making this work on
Windows, do they?

Part of the reason for my message was that I tried to figure out what
would it take to provide these capabilities on Windows, and quickly
got lost.  I have no background in graphics programming, neither on
Windows nor on any other platform, so for me good documentation is
critical.

Having this function be a simple boolean, on the assumption that all
GUI platforms provide all the sub-features, would actually mean that
this function is just a fancy alias for display-graphic-p.  I don't
think this is the best we can do.

In addition, my research indicates that the equivalent features on
Windows will have to use APIs that aren't available on Windows 9X
(unless we decide to transform on pixel level by our own code, which I
think is way too gross).  So there will be cases where rotations will
not be supported, even after the code to do that will have been
written.

Finally, there's the ImageMagick case, where we support rotations by
arbitrary angles, and it would be good to be able to make that
distinction with this single function, instead of making additional
tests.

> I’ve been thinking a bit about the idea of returning some sort of
> capabilities list and it seems quite neat. We could perhaps roll some
> of the imagemagick-types stuff into it.

We have similar availability testing functions in gnutls.c.

> +@item :crop @var{geometry}
> +This should be a list of the form @code{(@var{width} @var{height}
> +@var{x} @var{y})}.  If @var{width} and @var{height} are positive
> +numbers they specify the width and height of the cropped image.  If
> +@var{x} is a positive number it specifies the offset of the cropped
> +area from the left of the original image, and if negative the offset
> +from the right. If @var{y} is a positive number it specifies the
> +offset from the top of the original image, and if negative from the
> +bottom. If @var{x} or @var{y} are @code{nil} or unspecified the crop
> +area will be centred on the original image.

This says "If WIDTH and HEIGHT are positive numbers, ...", but doesn't
say what happens if they are non-positive.

> +Cropping is performed after scaling but before rotation.

This sounds strange to me; are you sure?  I'd expect cropping to be
done either before everything else or after everything else.  Is this
so because that's how XRender does it?  At the very least, it begs the
question whether the parameters of :crop are measured in units before
or after scaling.

>  @item :rotation @var{angle}
> -Specifies a rotation angle in degrees.
> +Specifies a rotation angle in degrees.  Only multiples of 90 degrees
> +are supported, unless the image type is @code{imagemagick}.  Positive
> +values rotate clockwise, negative values anti-clockwise.
                                            ^^^^^^^^^^^^^^
"counter-clockwise" is better, I think.

> +/* image_set_rotation, image_set_crop, image_set_size and
> +   image_set_transform use affine transformation matrices to perform
> +   various transforms on the image.  The matrix is a 2D array of
> +   doubles.  It is laid out like this:
> +
> +   m[0][0] = m11 | m[0][1] = m12 | m[0][2] = tx
> +   --------------+---------------+-------------
> +   m[1][0] = m21 | m[1][1] = m22 | m[1][2] = ty
> +   --------------+---------------+-------------
> +   m[2][0] = 0   | m[2][1] = 0   | m[2][2] = 1

Looking at the code, it seems that the matrix is actually defined like
this:

   m[0][0] = m11 | m[0][1] = m12 | m[0][2] = 0
   --------------+---------------+-------------
   m[1][0] = m21 | m[1][1] = m22 | m[1][2] = 0
   --------------+---------------+-------------
   m[2][0] = tx  | m[2][1] = ty  | m[2][2] = 1

If not, I think the Cairo code takes the wrong components for its
implementation.

> +   tx and ty represent translations, m11 and m22 represent scaling
> +   transforms and m21 and m12 represent shear transforms.

Can you please add the equations used to perform this affine
transformation, i.e. how x' and y' are computed from x and y?  I think
it will go a long way towards clarifying the processing.

Also, as long as I have your attention: could you please tell what you
get in the elements of matrix in image_set_size just before you pass
the values to XRenderSetPictureTransform, when you evaluate this in
*scratch*:

  (insert-image (create-image "splash.svg" 'svg nil :rotation 90))

I stepped through the code trying to figure out how to map these
features to the equivalent Windows APIs, and I saw some results that
confused me.  After you show me the values you get in this use case, I
might have a few follow-up questions about the code, to clarify my
understanding of what the code does and what we expect from the
backend when we hand it the matrix computed here.

Thanks.