Image transformations

Image transformations

[Eli Zaretskii]

Recent addition of native image transformations is a very welcome feature, but it's lacking some documentation and has a couple of other problems:

  . 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.
  . 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?).
 . 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

Could someone please address these deficiencies ASAP?  Without that, the feature seems incomplete and frankly not very clean.

TIA

Re: Image transformations

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

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.

>   . 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?

>  . 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. :)

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.

I’ll look into it further.

-- 
Alan Third

[-- Attachment #2: 0001-Document-image-transforms.patch --]
[-- Type: text/plain, Size: 3127 bytes --]

From bb51fbf18d8e9fcb7f65afef49d802b9fc768149 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Tue, 11 Jun 2019 20:31:24 +0100
Subject: [PATCH] Document image transforms

* doc/lispref/display.texi (Image Descriptors): Document :crop and
update :rotation.
* src/image.c: Describe the image transform matrix layout.
---
 doc/lispref/display.texi | 23 ++++++++++++++++++++++-
 src/image.c              | 16 ++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 93c5217c36..d29e98a8fd 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5181,8 +5181,29 @@ Image Descriptors
 specified, the height/width will be adjusted by the specified scaling
 factor.
 
+@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.
+
+If the crop area is outside or overlaps the edge of the image it will
+be reduced to exclude any areas outside of the image.  This means it
+is not possible to use @code{:crop} to increase the size of the image
+by entering large @var{width} or @var{height} values.
+
+Cropping is performed after scaling but before rotation.
+
 @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.  Rotation is
+performed after scaling and cropping.
 
 @item :index @var{frame}
 @xref{Multi-Frame Images}.
diff --git a/src/image.c b/src/image.c
index 86f8e8f4bb..12e35e4527 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1967,6 +1967,22 @@ compute_image_size (size_t width, size_t height,
 }
 #endif /* HAVE_IMAGEMAGICK || HAVE_NATIVE_TRANSFORMS */
 
+/* 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
+
+   tx and ty represent translations, m11 and m22 represent scaling
+   transforms and m21 and m12 represent shear transforms. Some
+   graphics toolkits don't require the third row, however it is
+   necessary for multiplication.  */
+
 typedef double matrix3x3[3][3];
 
 static void
-- 
2.21.0

Re: Image transformations

[Eli Zaretskii]

> Date: Tue, 11 Jun 2019 21:02:33 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> 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.

Re: Image transformations

[Alan Third]

On Wed, Jun 12, 2019 at 06:30:11PM +0300, Eli Zaretskii wrote:
> > >   . 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.

That’s what I imagined, but I didn’t know about the existing manual
tests. Good to know they’re there. Thanks!

> > 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?

Indeed.

> 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.

To be frank the only reason I added this function is because having
XRender doesn’t guarantee every frame will be able to perform these
transforms. I imagine that most modern hardware (from the last 15
years or so) will be able to handle it, but older hardware may be
using graphics cards that don’t support transforms. I think this means
you can end up in the situation where a frame on monitor one, running
on graphics card one, will work, but a frame on monitor two on
graphics card two won’t.

I think this is potentially an even worse situation than on Windows,
but probably very rare.

However extending image-transforms-p, or coming up with another
implementation, makes a lot of sense.

> > 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.

Thanks, I’ll have a look.

> > +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.

I agree, but this is how our imagemagick code does it and I didn’t
want to make my code behave differently, even though I think it makes
no sense.

It’s easy enough to re‐order the events. In the native transforms code
you simply reorder these function calls in lookup_image:

    image_set_size (img, transform_matrix);
    image_set_crop (img, transform_matrix);
    image_set_rotation (img, transform_matrix);

and reorder some of the code in imagemagick_load_image.

IMO the best order is probably crop, rotate and resize. I think
there’s an argument for putting resize before rotate, but rotating by
90 degrees after setting the size would mean :max-width and
:max-height affecting the wrong dimensions, as they do at the moment.

Alternatively we could split resizing so :max-width and :max-height
always operate last. Probably not worth it, though. Just put resizing
last.

> > +/* 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.

You’re right.

> > +   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.

I’ll add some further explanations of how to use the affine
transformation matrices, but I don’t know that I’ll be able to do a
very good job of explaining exactly how they work. I would suggest
that if someone is interested they look it up elsewhere, however I
also don’t think it’s necessary to fully understand the maths to be
able to use them.

I’ll provide an updated patch soon.

> 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.

Using this code at the top of image_set_transform:

  fprintf(stderr, "matrix:\n");
  fprintf(stderr, "%f %f %f\n",matrix[0][0], matrix[1][0], matrix[2][0]);
  fprintf(stderr, "%f %f %f\n",matrix[0][1], matrix[1][1], matrix[2][1]);
  fprintf(stderr, "%f %f %f\n",matrix[0][2], matrix[1][2], matrix[2][2]);

I get this printed (on macOS, but it should be the same everywhere):

matrix:
0.000000 1.000000 0.000000
-1.000000 0.000000 232.000000
0.000000 0.000000 1.000000

I don’t know exactly what that means. The 1 and -1 are shearing the
image in the x and y dimensions. The 232 is moving the image in the y
dimension. It’s actually moving the origin, so a positive value moves
the origin downwards.

The full process of rotating an image is to move the origin to the
centre of the image (width/2, height/2); perform the rotation, which
is some combination of shearing and resizing, but appears in this case
to not involve any actual resizing; and finally move the origin back
to the top left of the image, which may now be a different corner.

This is done by creating a matrix for each of those actions, then
multiplying the transformation matrix (the one we pass into each
function) by each of those matrices in order (matrix multiplication is
not commutative). After we’ve done that we can use our modified
transformation matrix to transform points. I believe we take the x and
y coordinates and convert them into a 3x1 matrix and multiply that by
the transformation matrix and it gives us a new set of coordinates.

    [x]   [m11 m12 m13]
    [y] X [m21 m22 m23] = [x’ y’ 0]
    [0]   [m31 m32 m33]

Luckily we don’t have to worry about the last step as the graphics
toolkit will do it for us.

Cropping is easier as we just move the origin to the top left of where
we want to crop and set the width and height accordingly. The matrices
don’t know anything about width and height.

Scaling is also simple as you can set m11 to scale x, m22 to scale y
and, I think, m33 to scale both by the same value. My code ignores
m33.

And of course you have to set the width and height accordingly again.

It’s possible to pre‐calculate the matrix multiplications and just
generate one transform matrix that will do everything we need in a
single step, but the maths for each element is much more complex and I
thought it was better to lay out the steps separately.

(perhaps I should just put the above into the comment in image.c)
-- 
Alan Third

Re: Image transformations

[Alan Third]

On Wed, Jun 12, 2019 at 11:07:46PM +0100, Alan Third wrote:
> > > +   tx and ty represent translations, m11 and m22 represent scaling
> > > +   transforms and m21 and m12 represent shear transforms.

Having written a huge tract and messed around with outputting various
transform matrices, it seems clear to me now that the above is not
quite as straightforward as I was thinking. A 90 degree rotation
followed by a resize doesn’t actually put anything in m11 and m22, so
clearly m11, m12, m21 and m22 interact in mysterious ways.

My feeble excuse is that I’m not a mathematician.
-- 
Alan Third

Re: Image transformations

[Alp Aker]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On Wed, Jun 12, 2019 at 6:09 PM Alan Third <alan@idiocy.org> wrote:
>
> matrix:
> 0.000000 1.000000 0.000000
> -1.000000 0.000000 232.000000
> 0.000000 0.000000 1.000000
>
> I don’t know exactly what that means.  The 1 and -1 are shearing the
> image in the x and y dimensions. The 232 is moving the image in the y
> dimension

This is a transformation matrix using so-called homogenous coordinates:

https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations

It's a clockwise 90 degree rotation followed by a translation along the y
axis.  In general you can't assign a geometric meaning to m11, m12, m21, m22
taken individually; whether they represent rotation, shearing, or scaling
depends on their relative values.

> I believe we take the x and y coordinates and convert them into a 3x1
matrix
> and multiply that by the transformation matrix and it gives us a new set
of
> coordinates.
>
>    [x]   [m11 m12 m13]
>    [y] X [m21 m22 m23] = [x’ y’ 0]
>    [0]   [m31 m32 m33]

You need to use 1 instead of 0 when translating from Cartesian to homogenous
coordinates.  That is, given a point (x, y), you find (x', y') as
follows.  Multiply (x, y, 1) by the transformation matrix.  Let the result
be
(a, b, c).  Then the new point (x', y') in Cartesian coordinates is (a/c,
b/c).

When dealing only with affine transformations the procedure is simpler.
Such
transformations can always be described by a matrix where m31 == m32 == 0
and
m33 == 1.  In that case, the result of multiplication will have the form
(a, b,
1), so x' == a and y' == b.

[-- Attachment #2: Type: text/html, Size: 1933 bytes --]

Re: Image transformations

[Eli Zaretskii]

> From: Alp Aker <alptekin.aker@gmail.com>
> Date: Thu, 13 Jun 2019 00:16:36 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs devel <emacs-devel@gnu.org>
> 
> > matrix:
> > 0.000000 1.000000 0.000000
> > -1.000000 0.000000 232.000000
> > 0.000000 0.000000 1.000000
> >
> > I don’t know exactly what that means.  The 1 and -1 are shearing the
> > image in the x and y dimensions. The 232 is moving the image in the y
> > dimension
> 
> This is a transformation matrix using so-called homogenous coordinates:
> 
> https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations

Right, I got that far, it's the details that somewhat confuse me, see
below.

> It's a clockwise 90 degree rotation followed by a translation along the y
> axis.

This already goes contrary to my geometric intuition, please bear with
me.  The rotation is around the (0,0) origin, i.e. around the top-left
corner of the original image, right?  If so, the rotation should have
been followed by a translation along the X axis, not Y, because
rotating a 333-pixel wide, 233-pixel high image 90 deg clockwise
produces a 233-pixel wide, 333-pixel high image that is entirely to
the LEFT of the Y axis.  Here's ASCII-art representation of that:

    +------------------+> X        +----------+-------------------> X
    |                  |           |          |
    |                  |           |          | 
    |                  |           |          |
    |                  |   ===>    |          |
    +------------------+           |          |
    |                              |          |
    |                              |          |
    |                              |          |
    |                              +----------+
    |                                         |
    V                                         V
    Y                                         Y

The above is just after the rotation around (0,0).  Is that correct,
or am I missing something?

I also tried to approach this from the matrix notation aspect.  Is the
following the correct equations of computing (x',y'), the new
coordinates of any pixel of the image, from its original coordinates
(x,y)?

  x' = m11 * x + m12 * y + tx
  y' = m21 * x + m22 * y + ty

where the factors are related to the matrix as follows:

   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 the above is correct, then the transformation of the top-left
corner of the original image, whose original coordinates are (0,0),
yield

  x' = 0 * x + -1 * y +   0 = 0
  y' = 1 * x +  0 * y + 232 = 232

which is incorrect, since the correct coordinates should be (233,0),
not (0,232).  (The 232 vs 233 is some kind of round-off, but let's
ignore that for now.)

What am I missing here?

There's also the issue of cropping, which the current code in image.c
seems to represent as some kind of translation with change in image
size.  But my, perhaps incorrect, interpretation of translation is
that the entire image is shifted along X and Y axes, which is not what
cropping is about, AFAIU.  Again, I'm probably missing something very
fundamental here.

> In general you can't assign a geometric meaning to m11, m12, m21, m22
> taken individually; whether they represent rotation, shearing, or scaling
> depends on their relative values.
> 
> > I believe we take the x and y coordinates and convert them into a 3x1
> matrix
> > and multiply that by the transformation matrix and it gives us a new set
> of
> > coordinates.
> >
> >    [x]   [m11 m12 m13]
> >    [y] X [m21 m22 m23] = [x’ y’ 0]
> >    [0]   [m31 m32 m33]
> 
> You need to use 1 instead of 0 when translating from Cartesian to homogenous
> coordinates.  That is, given a point (x, y), you find (x', y') as
> follows.  Multiply (x, y, 1) by the transformation matrix.  Let the result
> be
> (a, b, c).  Then the new point (x', y') in Cartesian coordinates is (a/c,
> b/c).
> 
> When dealing only with affine transformations the procedure is
> simpler.  Such transformations can always be described by a matrix
> where m31 == m32 == 0 and m33 == 1.  In that case, the result of
> multiplication will have the form (a, b, 1), so x' == a and y' == b.

Sorry, now I'm even more confused: aren't we dealing with affine
transformations?  Then how are homogeneous coordinates related to
this?  And does that mean the formulae for calculating (x',y') I show
above are incorrect?

Thanks.

Re: Image transformations

[Eli Zaretskii]

> Date: Wed, 12 Jun 2019 23:07:46 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > > +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.
> 
> I agree, but this is how our imagemagick code does it and I didn’t
> want to make my code behave differently, even though I think it makes
> no sense.

OK, but what about the question regarding the units of :crop
parameters -- should they be interpreted as before or after the
scaling?

> > 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.
> 
> I’ll add some further explanations of how to use the affine
> transformation matrices, but I don’t know that I’ll be able to do a
> very good job of explaining exactly how they work. I would suggest
> that if someone is interested they look it up elsewhere, however I
> also don’t think it’s necessary to fully understand the maths to be
> able to use them.

I have shown my interpretation of the equations.  Trouble is, I cannot
find what XRender does anywhere.  Does someone know where to look for
that?

> I’ll provide an updated patch soon.

Thanks.

> >   (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.
> 
> Using this code at the top of image_set_transform:
> 
>   fprintf(stderr, "matrix:\n");
>   fprintf(stderr, "%f %f %f\n",matrix[0][0], matrix[1][0], matrix[2][0]);
>   fprintf(stderr, "%f %f %f\n",matrix[0][1], matrix[1][1], matrix[2][1]);
>   fprintf(stderr, "%f %f %f\n",matrix[0][2], matrix[1][2], matrix[2][2]);
> 
> I get this printed (on macOS, but it should be the same everywhere):
> 
> matrix:
> 0.000000 1.000000 0.000000
> -1.000000 0.000000 232.000000
> 0.000000 0.000000 1.000000

That's what I get, except that you've printed the matrix in
column-major order.  I described my conceptual problems with these
values in another message.

> Luckily we don’t have to worry about the last step as the graphics
> toolkit will do it for us.

Unfortunately, I do have to worry about all of the steps, because I
need to figure out how to map all this to the equivalent Windows APIs.
Thus my questions, for which I apologize.  I'd prefer that someone
more knowledgeable about graphics programming did the changes on
Windows, but no one stepped forward yet.

> (perhaps I should just put the above into the comment in image.c)

Yes, please.

Thanks.

Re: Image transformations

[Alp Aker]

[-- Attachment #1: Type: text/plain, Size: 4312 bytes --]

On Thu, Jun 13, 2019 at 1:41 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> This already goes contrary to my geometric intuition, please bear with
> me.  The rotation is around the (0,0) origin, i.e. around the top-left
> corner of the original image, right?  If so, the rotation should have
> been followed by a translation along the X axis, not Y

The last sentence is problematic.  When a transformation matrix describes a
rotation followed by a translation, the translation is specified relative
to the
fixed coordinate axes.  The rotation doesn't affect the direction of the
translation.  It's perhaps unintuitive, but that's the way the formalism is
defined.  We could interpret "rotate 90 then translate" in the way you
describe,
but then the translation into algebra would be different.

> rotating a 333-pixel wide, 233-pixel high image 90 deg clockwise
> produces a 233-pixel wide, 333-pixel high image that is entirely to
> the LEFT of the Y axis.  Here's ASCII-art representation of that:
>
>     +------------------+> X        +----------+-------------------> X
>     |                  |           |          |
>     |                  |           |          |
>     |                  |           |          |
>     |                  |   ===>    |          |
>     +------------------+           |          |
>     |                              |          |
>     |                              |          |
>     |                              |          |
>     |                              +----------+
>     |                                         |
>     V                                         V
>     Y                                         Y
>
> The above is just after the rotation around (0,0).  Is that correct,
> or am I missing something?

That's correct.

> I also tried to approach this from the matrix notation aspect.  Is the
> following the correct equations of computing (x',y'), the new
> coordinates of any pixel of the image, from its original coordinates
> (x,y)?
>
>   x' = m11 * x + m12 * y + tx
>   y' = m21 * x + m22 * y + ty
>
> where the factors are related to the matrix as follows:
>
>    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

I confess I'm not sure how to interpret that matrix.  I just looked through
image_set_rotation and found it somewhat confusing, as it seems to use
column-major representation where I'd expect row-major.  E.g., the above
matrix
looks odd to me, because tx and ty would normally be in m[0][2] and m[1][2]
(and
I'd expect m[2][0] == m[2][1] == 0).  Similarly, the rotation matrix used in
image_set_rotation:

  [0][0] = cos_r, [0][1] = -sin_r
  [1][0] = sin_r, [1][1] = cos_r

would normally describe a counter-clockwise rotation by r, not a clockwise
rotation.

That said, if I correctly understand the layout of the data, the equations
should be:

   x' = m11 * x + m21 * y + tx
   y' = m12 * x + m22 * y + ty

> If the above is correct, then the transformation of the top-left
> corner of the original image, whose original coordinates are (0,0),
> [...]
> the correct coordinates should be (233,0), not (0,232).
> What am I missing here?

The transformation described by the matrix is: rotate 90 degrees around the
origin, then translate by 232 along the y axis.  The first operation leaves
(0,
0) unmoved, then the second operation moves it to (0, 232).

> Sorry, now I'm even more confused: aren't we dealing with affine
> transformations?  Then how are homogeneous coordinates related to
> this?  And does that mean the formulae for calculating (x',y') I show
> above are incorrect?

I was being unnecessarily general, which caused confusion; my
apologies.  Probably best for present purposes to ignore what I said
there.  (What I meant: Transformation matrices can be used for both affine
and
non-affine transformations.  I first described the general case, then
described
how the calculations work when we restrict to affine transformations.  It's
homogeneous coordinates in both cases, though.  I also wrote this last part
before noticing the transposition issue I mentioned above, which probably
adds
to the confusion.)

[-- Attachment #2: Type: text/html, Size: 5206 bytes --]

Re: Image transformations

[Eli Zaretskii]

> From: Alp Aker <alptekin.aker@gmail.com>
> Date: Thu, 13 Jun 2019 05:19:52 -0400
> Cc: Alan Third <alan@idiocy.org>, Emacs devel <emacs-devel@gnu.org>
> 
> > This already goes contrary to my geometric intuition, please bear with
> > me.  The rotation is around the (0,0) origin, i.e. around the top-left
> > corner of the original image, right?  If so, the rotation should have
> > been followed by a translation along the X axis, not Y
> 
> The last sentence is problematic.  When a transformation matrix describes a
> rotation followed by a translation, the translation is specified relative
> to the fixed coordinate axes.  The rotation doesn't affect the
> direction of the translation.

That's right, but this is exactly what I was trying to describe.  When
I wrote "translation along the X axis", I meant the original X axis,
which is unaffected by the rotation.  Are you saying that my
expectations are incorrect in that interpretation of "X axis"?

> >     +------------------+> X        +----------+-------------------> X
> >     |                  |           |          |
> >     |                  |           |          |
> >     |                  |           |          |
> >     |                  |   ===>    |          |
> >     +------------------+           |          |
> >     |                              |          |
> >     |                              |          |
> >     |                              |          |
> >     |                              +----------+
> >     |                                         |
> >     V                                         V
> >     Y                                         Y
> >
> > The above is just after the rotation around (0,0).  Is that correct,
> > or am I missing something?
> 
> That's correct.
> 
> > I also tried to approach this from the matrix notation aspect.  Is the
> > following the correct equations of computing (x',y'), the new
> > coordinates of any pixel of the image, from its original coordinates
> > (x,y)?
> >
> >   x' = m11 * x + m12 * y + tx
> >   y' = m21 * x + m22 * y + ty
> >
> > where the factors are related to the matrix as follows:
> >
> >    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
> 
> I confess I'm not sure how to interpret that matrix.  I just looked through
> image_set_rotation and found it somewhat confusing, as it seems to use
> column-major representation where I'd expect row-major.  E.g., the above
> matrix
> looks odd to me, because tx and ty would normally be in m[0][2] and m[1][2]
> (and
> I'd expect m[2][0] == m[2][1] == 0).  Similarly, the rotation matrix used in
> image_set_rotation:
> 
>   [0][0] = cos_r, [0][1] = -sin_r
>   [1][0] = sin_r, [1][1] = cos_r
> 
> would normally describe a counter-clockwise rotation by r, not a clockwise
> rotation.

Maybe that's the problem: if the rotation is counter-clockwise, then
the translation should indeed be along the Y axis.

> That said, if I correctly understand the layout of the data, the equations
> should be:
> 
>    x' = m11 * x + m21 * y + tx
>    y' = m12 * x + m22 * y + ty

AFAIU, this indeed describes a counter-clockwise rotation, not a
clockwise rotation.

> > the correct coordinates should be (233,0), not (0,232).
> > What am I missing here?
> 
> The transformation described by the matrix is: rotate 90 degrees
> around the origin, then translate by 232 along the y axis.  The
> first operation leaves (0, 0) unmoved, then the second operation
> moves it to (0, 232).

But if the rotation is clockwise, the result should be (233,0), right?

Thank you for helping me figure out this stuff.

Re: Image transformations

[Alp Aker]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Thu, Jun 13, 2019 at 9:05 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> That's right, but this is exactly what I was trying to describe.  When
> I wrote "translation along the X axis", I meant the original X axis,
> which is unaffected by the rotation.  Are you saying that my
> expectations are incorrect in that interpretation of "X axis"?
> [...]
> Maybe that's the problem: if the rotation is counter-clockwise, then
> the translation should indeed be along the Y axis.

The translation doesn't depend on the rotation in any way.  The general
form of
an affine transformation is F(x) + b, where F is a linear transformation
(rotation, shear, scaling) and addition by b is translation. In the example
we're discussing, F is rotation by 90 degrees and b is (0, 232) in some
coordinates.  F does not act on b; the rotation does not affect what the
translation does; whether F is clockwise or counter-clockwise rotation, the
function displaces the result of the rotation along the (original) y axis.
 (The
single-matrix form used by the transformation code in image.c is a
computational
convenience; the function it expresses still has the form F(x) + b.)

[-- Attachment #2: Type: text/html, Size: 1341 bytes --]

Re: Image transformations

[Alan Third]

On Thu, Jun 13, 2019 at 08:41:02AM +0300, Eli Zaretskii wrote:
> > From: Alp Aker <alptekin.aker@gmail.com>
> > Date: Thu, 13 Jun 2019 00:16:36 -0400
> > Cc: Eli Zaretskii <eliz@gnu.org>, Emacs devel <emacs-devel@gnu.org>
> > 
> > > matrix:
> > > 0.000000 1.000000 0.000000
> > > -1.000000 0.000000 232.000000
> > > 0.000000 0.000000 1.000000
> > >
> > > I don’t know exactly what that means.  The 1 and -1 are shearing the
> > > image in the x and y dimensions. The 232 is moving the image in the y
> > > dimension
> > 
> > This is a transformation matrix using so-called homogenous coordinates:
> > 
> > https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations
> 
> Right, I got that far, it's the details that somewhat confuse me, see
> below.
> 
> > It's a clockwise 90 degree rotation followed by a translation along the y
> > axis.
> 
> This already goes contrary to my geometric intuition, please bear with
> me.  The rotation is around the (0,0) origin, i.e. around the top-left
> corner of the original image, right?  If so, the rotation should have
> been followed by a translation along the X axis, not Y, because
> rotating a 333-pixel wide, 233-pixel high image 90 deg clockwise
> produces a 233-pixel wide, 333-pixel high image that is entirely to
> the LEFT of the Y axis.  Here's ASCII-art representation of that:
> 
>     +------------------+> X        +----------+-------------------> X
>     |                  |           |          |
>     |                  |           |          | 
>     |                  |           |          |
>     |                  |   ===>    |          |
>     +------------------+           |          |
>     |                              |          |
>     |                              |          |
>     |                              |          |
>     |                              +----------+
>     |                                         |
>     V                                         V
>     Y                                         Y
> 
> The above is just after the rotation around (0,0).  Is that correct,
> or am I missing something?

I think I confused things by saying ‘followed by’, as I think they
probably happen simultaneously.

It works if you consider it as moving the origin from the top left
corner, 232 pixels down the Y axis to the bottom left corner, then
rotating. I don’t really know how to think about this that deeply,
especially since this matrix is the result of two translations and a
rotation multiplied together.

One thing that may also be confusing is that there are two different
approaches to this. XRender applies the transforms to the image,
whereas NS applies the tranforms to the surface the image is to be
drawn to. I have a suspicion, having read some Windows API
documentation (but not much) that Windows works the same way as NS.
This is unfortunate as it’s harder to understand what’s going on.

A way to visualise the difference is that XRender is like taking a
photo and rotating and moving it around before putting it on a bit of
paper. The NS method is like holding a photo still and moving the bit
of paper under it.

The key difference is that for NS I have to invert the transformation
matrix. I also have to take more care with where I place the origin
before applying the transformation matrix.

I could be wrong, but it may explain why things aren’t doing what
you’re expecting. Or it could simply be down to the fact I transposed
the rows and columns. I think it’s probably a good idea for us to deal
with the transposition first before making any definite statements on
this.
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> From: Alp Aker <alptekin.aker@gmail.com>
> Date: Thu, 13 Jun 2019 11:57:34 -0400
> Cc: Alan Third <alan@idiocy.org>, Emacs devel <emacs-devel@gnu.org>
> 
> > Maybe that's the problem: if the rotation is counter-clockwise, then
> > the translation should indeed be along the Y axis.
> 
> The translation doesn't depend on the rotation in any way.

I think the final value of translation does depend on the rotation,
because the center of the image's position after the rotation depends
on the rotation angle.

> The general form of an affine transformation is F(x) + b, where F is
> a linear transformation (rotation, shear, scaling) and addition by b
> is translation.

Yes, but our transformation includes translation, rotation, and
another translation.  Not just one translation.  IOW, it isn't the
transformation matrix that is given; it's the operation on the image.
The transformation matrix surely depends on whether the rotation is
clockwise or counter-clockwise.

Anyway, until we agree on the equations that convert (x,y) into
(x',y'), it is IMO pointless to argue about details.  So can anyone
tell where the meaning of the matrix passed to XRender is described?

Re: Image transformations

[Alan Third]

On Thu, Jun 13, 2019 at 08:48:48AM +0300, Eli Zaretskii wrote:
> > Date: Wed, 12 Jun 2019 23:07:46 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > > > +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.
> > 
> > I agree, but this is how our imagemagick code does it and I didn’t
> > want to make my code behave differently, even though I think it makes
> > no sense.
> 
> OK, but what about the question regarding the units of :crop
> parameters -- should they be interpreted as before or after the
> scaling?

After the scaling.

> > > 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.
> > 
> > I’ll add some further explanations of how to use the affine
> > transformation matrices, but I don’t know that I’ll be able to do a
> > very good job of explaining exactly how they work. I would suggest
> > that if someone is interested they look it up elsewhere, however I
> > also don’t think it’s necessary to fully understand the maths to be
> > able to use them.
> 
> I have shown my interpretation of the equations.  Trouble is, I cannot
> find what XRender does anywhere.  Does someone know where to look for
> that?

I suspect we’d have to dive into the code to see exactly what XRender
is doing, however from my own testing I believe the transform matrix
passed into XRender works exactly as described here:

https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations

I can’t find a great explanation of how exactly transformation matrices
work, but there are a lot of explanations available.

> > Luckily we don’t have to worry about the last step as the graphics
> > toolkit will do it for us.
> 
> Unfortunately, I do have to worry about all of the steps, because I
> need to figure out how to map all this to the equivalent Windows APIs.
> Thus my questions, for which I apologize.  I'd prefer that someone
> more knowledgeable about graphics programming did the changes on
> Windows, but no one stepped forward yet.

You shouldn’t really have to fully understand the maths to implement
this, so clearly we’re going wrong somewhere.

Can you point me to the Windows API documentation and perhaps I can
work out how exactly we need to approach this?
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 17:12:15 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Alp Aker <alptekin.aker@gmail.com>, emacs-devel@gnu.org
> 
> It works if you consider it as moving the origin from the top left
> corner, 232 pixels down the Y axis to the bottom left corner, then
> rotating. I don’t really know how to think about this that deeply,
> especially since this matrix is the result of two translations and a
> rotation multiplied together.

OK, but how to be sure this is the correct interpretation?  The code
which implements the rotations does a translation, followed by
rotation, followed by another translation, and multiplies all the 3
matrices to produce the result.  If we are not sure about what these
transformations mean, how do we know the result is correct?  How did
_you_ know to write that code? is there some XRender elated
documentation that describes the meaning of each matrix element?  For
example, do the translations describe how the pixels are moved or how
the origin of the coordinate system is moved?

> One thing that may also be confusing is that there are two different
> approaches to this. XRender applies the transforms to the image,
> whereas NS applies the tranforms to the surface the image is to be
> drawn to. I have a suspicion, having read some Windows API
> documentation (but not much) that Windows works the same way as NS.
> This is unfortunate as it’s harder to understand what’s going on.

If everybody and their dog work differently, why are we doing this
according to XRender, and not the other way around?

> The key difference is that for NS I have to invert the transformation
> matrix.

What do you mean by "invert", and where is that NS code?

And if you figured out how to map what XRender does to what NS does,
you probably understand well what the XRender matrix means, right?

> I could be wrong, but it may explain why things aren’t doing what
> you’re expecting. Or it could simply be down to the fact I transposed
> the rows and columns. I think it’s probably a good idea for us to deal
> with the transposition first before making any definite statements on
> this.

Not sure what you mean by "deal with transposition".

Thanks.

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 17:58:04 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> Can you point me to the Windows API documentation and perhaps I can
> work out how exactly we need to approach this?

I want to use PlgBlt, see

  https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/nf-wingdi-plgblt

For that, I need to compute, for each of the original image's
vertices, the coordinates of the corresponding vertex of the
transformed image.  That's why I was asking how to interpret the
matrix elements for transforming pixel coordinates.

I also want to know when no rotation is involved, so that I could use
the existing code (which only supports scaling and will be modified to
support cropping) on older Windows versions that cannot support
rotations (PlgBlt is not available on those platforms).

Thanks.

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 17:58:04 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> Can you point me to the Windows API documentation and perhaps I can
> work out how exactly we need to approach this?

I of course appreciate the offer, and responded with the information.
But really, this kind of effort on your part is not necessary.  If you
can add a test for these capabilities, it shouldn't take too long to
figure out what the matrix means by comparing the effect to the
expected results.

So maybe working on the tests will be the most effective use of your
time in this matter.

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 4398 bytes --]

On Thu, 13 Jun 2019 at 18:57, Eli Zaretskii <eliz@gnu.org> wrote:

    > From: Alp Aker <alptekin.aker@gmail.com>
    > Date: Thu, 13 Jun 2019 11:57:34 -0400
    > Cc: Alan Third <alan@idiocy.org>, Emacs devel <emacs-devel@gnu.org>
    >
    > > Maybe that's the problem: if the rotation is counter-clockwise, then
    > > the translation should indeed be along the Y axis.
    >
    > The translation doesn't depend on the rotation in any way.

    I think the final value of translation does depend on the rotation,
    because the center of the image's position after the rotation depends
    on the rotation angle.

    > The general form of an affine transformation is F(x) + b, where F is
    > a linear transformation (rotation, shear, scaling) and addition by b
    > is translation.

    Yes, but our transformation includes translation, rotation, and
    another translation.  Not just one translation.  IOW, it isn't the
    transformation matrix that is given; it's the operation on the image.

XRender uses homogeneous matrices. In this system a 3x3 projective matrix
can
represent any affine transformation as one matrix. The matrix representing a
translation, then a rotation, then a translation, can then be calculated by
multiplying three matrices. (It is in fact again a rotation followed by a
single
translation.)

The 9-coordinate homogeneous matrix can represent any projective
transformation.
Simplifying to the case of affine transformations, the matrix is of the form

[Fxx  Fxy  Tx]
[Fyx  Fyy  Ty]
[  0    0   1]

Fij represent the scale/rotation/shear and Ti represent the translation.
Multiplying two matrices of this form gives another matrix of the same form.

To transform coordinates (X Y) you multiply the matrix (on the left) by
a column vector (on the right) as follows.

[Fxx  Fxy  Tx] [X]     [Fxx * X + Fxy * Y + Tz]
[Fyx  Fyy  Ty] [Y]  =  [Fyx * X + Fyy * Y + Ty]
[  0    0   1] [1]     [                     1]

A pure translation goes like this:

[1   0  Tx] [X]   [X + Tx]
[0   1  Ty] [Y] = [Y + Ty]
[0   0   1] [1]   [     1]

    The transformation matrix surely depends on whether the rotation is
    clockwise or counter-clockwise.

The origin is at the top left so a pure rotation clockwise about the origin
through angle a goes like this:

[cos(a)  -sin(a)  0] [X]   [cos(a) * X - sin(a) * Y]
[sin(a)   cos(a)  0] [Y] = [sin(a) * X + cos(a) * Y]
[     0        0  1] [1]   [                      1]

To combine several transformations (e.g., to get a matrix that does a
transformation, then a rotation, then a transformation) you multiply the
matrices of the transformations together. The matrix for the last
transformation
goes furthest left.

    Anyway, until we agree on the equations that convert (x,y) into
    (x',y'), it is IMO pointless to argue about details.  So can anyone
    tell where the meaning of the matrix passed to XRender is described?

The documentation is here:
<https://www.x.org/releases/X11R7.7/doc/libXrender/libXrender.txt>

What is not specified is the memory layout of the matrix.
Some more details are here:
<http://www.talisman.org/~erlkonig/misc/x11-composite-tutorial/>.

From the tutorial, I guess the memory layout is

XTransform xform = {{
  { XDoubleToFixed(Fxx), XDoubleToFixed(Fyx), XDoubleToFixed(0) },
  { XDoubleToFixed(Fxy), XDoubleToFixed(Fyy), XDoubleToFixed(0) },
  { XDoubleToFixed(Tx),  XDoubleToFixed(Ty),  XDoubleToFixed(1) }
}};

It could equally be

XTransform xform = {{
  { XDoubleToFixed(Fxx), XDoubleToFixed(Fxy), XDoubleToFixed(Tx) },
  { XDoubleToFixed(Fyx), XDoubleToFixed(Fyy), XDoubleToFixed(Ty) },
  { XDoubleToFixed(0),   XDoubleToFixed(0),   XDoubleToFixed(1) }
}};

Someone can work out which through experimenation.

Here I have fixed three of the matrix entries to 0, 0, 1; this is the
simplification to the case of affine transformations only that I mentioned.
The full 3x3 matrix represents an arbitrary projective transformation.
We don't need that.

For example, the tutorial mentions representing a scale factor W as

[1   0   0 ]
[0   1   0 ]
[0   0  1/W]

which implies that there is a perspective-division step, the X and Y
coordinates
being divided by the Z (depth) coordinate. I don't know at what stage of the
pipeline this happens. We can sidestep the issue by setting W = Z = 1 and
representing a scale factor of W using the matrix

[W   0   0]
[0   W   0]
[0   0   1]

[-- Attachment #2: Type: text/html, Size: 5325 bytes --]

Re: Image transformations

[Alan Third]

On Thu, Jun 13, 2019 at 08:11:21PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 13 Jun 2019 17:58:04 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > Can you point me to the Windows API documentation and perhaps I can
> > work out how exactly we need to approach this?
> 
> I want to use PlgBlt, see
> 
>   https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/nf-wingdi-plgblt
> 
> For that, I need to compute, for each of the original image's
> vertices, the coordinates of the corresponding vertex of the
> transformed image.  That's why I was asking how to interpret the
> matrix elements for transforming pixel coordinates.

I think this should be what you need:

   x’ = tm[0][0] * x + tm[0][1] * y + tm[0][2] * 1
   y’ = tm[1][0] * x + tm[1][1] * y + tm[1][2] * 1

where tm is the completed transformation matrix.

BTW, are you aware that you can use the XFORM struct:

https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/ns-wingdi-xform

That maps exactly to the matrices, which is one of the reasons I went
down this route originally. Almost everything supports them.

> I also want to know when no rotation is involved, so that I could use
> the existing code (which only supports scaling and will be modified to
> support cropping) on older Windows versions that cannot support
> rotations (PlgBlt is not available on those platforms).

I’d be inclined to just skip image_set_rotation when on a platform
that doesn’t support it. Or storing the basic crop and scaling
information separately so you don’t have to worry about the matrices
at all.

Now I see why you want to be able to distinguish between the
availability of the different types of transform too.
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 13 Jun 2019 20:00:49 +0100
> Cc: Alp Aker <alptekin.aker@gmail.com>, alan@idiocy.org, 
> 	Emacs Development <emacs-devel@gnu.org>
> 
> XRender uses homogeneous matrices. In this system a 3x3 projective matrix can
> represent any affine transformation as one matrix. The matrix representing a
> translation, then a rotation, then a translation, can then be calculated by
> multiplying three matrices. (It is in fact again a rotation followed by a single
> translation.)

Thanks for a useful and detailed description.

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On Thu, 13 Jun 2019 at 20:01, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Thu, 13 Jun 2019 17:12:15 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: Alp Aker <alptekin.aker@gmail.com>, emacs-devel@gnu.org
>
> > One thing that may also be confusing is that there are two different
> > approaches to this. XRender applies the transforms to the image,
> > whereas NS applies the tranforms to the surface the image is to be
> > drawn to. I have a suspicion, having read some Windows API
> > documentation (but not much) that Windows works the same way as NS.
> > This is unfortunate as it’s harder to understand what’s going on.
>
> If everybody and their dog work differently, why are we doing this
> according to XRender, and not the other way around?
>
> > The key difference is that for NS I have to invert the transformation
> > matrix.
>
> What do you mean by "invert", and where is that NS code?
>
> And if you figured out how to map what XRender does to what NS does,
> you probably understand well what the XRender matrix means, right?
>
> > I could be wrong, but it may explain why things aren’t doing what
> > you’re expecting. Or it could simply be down to the fact I transposed
> > the rows and columns. I think it’s probably a good idea for us to deal
> > with the transposition first before making any definite statements on
> > this.
>
> Not sure what you mean by "deal with transposition".


I doubt it will end up being important to understand the matrix inverse
and transpose operations.

The difference between NS and XRender is that one stores the matrix in
row-major format and the other in column-major format.

[-- Attachment #2: Type: text/html, Size: 2293 bytes --]

Re: Image transformations

[Alan Third]

On Thu, Jun 13, 2019 at 08:27:24PM +0100, Alan Third wrote:
> 
> I think this should be what you need:
> 
>    x’ = tm[0][0] * x + tm[0][1] * y + tm[0][2] * 1
>    y’ = tm[1][0] * x + tm[1][1] * y + tm[1][2] * 1
> 
> where tm is the completed transformation matrix.

Now I’m getting confused by the fact I transposed the rows and columns
as compared to standard matrix notation.

   x’ = tm[0][0] * x + tm[1][0] * y + tm[2][0] * 1
   y’ = tm[0][1] * x + tm[1][1] * y + tm[2][1] * 1

Should be correct.
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 20:27:24 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I think this should be what you need:
> 
>    x’ = tm[0][0] * x + tm[0][1] * y + tm[0][2] * 1
>    y’ = tm[1][0] * x + tm[1][1] * y + tm[1][2] * 1
> 
> where tm is the completed transformation matrix.

Thanks.

> BTW, are you aware that you can use the XFORM struct:
> 
> https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/ns-wingdi-xform

Yes, I'm using it.

> > I also want to know when no rotation is involved, so that I could use
> > the existing code (which only supports scaling and will be modified to
> > support cropping) on older Windows versions that cannot support
> > rotations (PlgBlt is not available on those platforms).
> 
> I’d be inclined to just skip image_set_rotation when on a platform
> that doesn’t support it.

That'd mean putting too much w32-specific code in image.c, something
I'd like to avoid if possible.

> Or storing the basic crop and scaling information separately so you
> don’t have to worry about the matrices at all.

I'm beginning to think we should do this on all platforms, and leave
the matrix calculation, if needed, to the terminal-specific back-ends
(xterm.c, w32term.c, etc.).  It sounds like we gain nothing by
spilling XRender-specific stuff in image.c and letting all the other
platforms adapt.

Re: Image transformations

[Alan Third]

On Thu, Jun 13, 2019 at 10:47:38PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 13 Jun 2019 20:27:24 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > Or storing the basic crop and scaling information separately so you
> > don’t have to worry about the matrices at all.
> 
> I'm beginning to think we should do this on all platforms, and leave
> the matrix calculation, if needed, to the terminal-specific back-ends
> (xterm.c, w32term.c, etc.).  It sounds like we gain nothing by
> spilling XRender-specific stuff in image.c and letting all the other
> platforms adapt.

I think you have a fundamental misunderstanding of how the
transformation matrix is being used. It’s not specific to XRender.
Aside from a few short‐circuits the only XRender specific code is in
image_set_transform, and XRender, NS and Cairo all have their own code
in there, and use the same transformation matrix.

We could even use the same matrix with ImageMagick with a bit of work.

If we were to throw out that shared matrix code and put it into each
backend we would be simply duplicating the code. Even using a
toolkit’s built‐in crop/rotate/resize functions is probably recreating
the exact same transformation matrix in the background.
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 23:26:26 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > I'm beginning to think we should do this on all platforms, and leave
> > the matrix calculation, if needed, to the terminal-specific back-ends
> > (xterm.c, w32term.c, etc.).  It sounds like we gain nothing by
> > spilling XRender-specific stuff in image.c and letting all the other
> > platforms adapt.
> 
> I think you have a fundamental misunderstanding of how the
> transformation matrix is being used.

That's always possible, although I stared at that code and stepped
through it in a debugger long enough to assume it's not the case.

> It’s not specific to XRender.  Aside from a few short‐circuits the
> only XRender specific code is in image_set_transform, and XRender,
> NS and Cairo all have their own code in there, and use the same
> transformation matrix.

The matrix is defined according to XRender, AFAIU.  NS and Cairo need
to transpose/invert/etc. the matrix to use it, and so will the
MS-Windows code, IIUC.  The matrix fuses the actual primitive
transformations into a construct which we currently don't seem to
understand well, whose only "documentation" is in a tutorial that is
not part of XRender's official docs.  From where I'm standing, this is
a scary situation.  I believe both Cairo and MS-Windows have a much
clearer official documentation of the related functionality (didn't
try looking for NS docs, but I'd expect to see good documentation
there as well), so clean code could be relatively easily written for
these platforms using the transformation spec's parameters instead of
the matrix.

> We could even use the same matrix with ImageMagick with a bit of work.

I think this would be a wasted effort.  There's no need to make all
the implementations use the same matrix, unless they all assign the
same semantics to the matrix -- which doesn't appear to be the case,
at least not as far as this discussion shows.

> If we were to throw out that shared matrix code and put it into each
> backend we would be simply duplicating the code.

I'm not convinced we will end up with a duplication.  Certainly, for
rotations by multiples of 90 deg no matrix multiplication is necessary
(though possible) to construct the transform.  And in the Windows case
having the original parameters of the transform will make it much
easier to detect the transforms that cannot be supported on older
platforms than it will be by analyzing the matrix.

What I'm suggesting in practice is to move the entire calculation of
the matrix to image_set_transform, instead of communicating the
individual transformations via the matrix that's calculated piecemeal.
Then each port will be able to either use that matrix or do something
different with the transform parameters.  Since image_set_transform is
the only caller of image_set_size, image_set_crop, and
image_set_rotation, I see no inherent reasons to return the result of
those primitive transformations in the form of a matrix.

WDYT?

Re: Image transformations

[Stefan Monnier]

> I think this would be a wasted effort.  There's no need to make all
> the implementations use the same matrix, unless they all assign the
> same semantics to the matrix -- which doesn't appear to be the case,
> at least not as far as this discussion shows.

There's one case where this would make sense: if we decide to let Elisp
provide such a matrix.


        Stefan

Re: Image transformations

[Alp Aker]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

On Thu, Jun 13, 2019 at 3:01 PM Richard Copley <rcopley@gmail.com> wrote:

>     Yes, but our transformation includes translation, rotation, and
>     another translation.  Not just one translation.  IOW, it isn't the
>     transformation matrix that is given; it's the operation on the image.

There's a miscommunication here.  I was speaking to Alan's question about
how
to geometrically interpret the components of a transformation matrix.

If the question is what translations to use in order to generate a rotation
around an arbitrary point p, then there's no question: the sequence of
operations is translation by -p, then rotation, then translation by p.

> The origin is at the top left so a pure rotation clockwise about the
origin
> through angle a goes like this:
>
> [cos(a)  -sin(a)  0] [X]   [cos(a) * X - sin(a) * Y]
> [sin(a)   cos(a)  0] [Y] = [sin(a) * X + cos(a) * Y]
> [     0        0  1] [1]   [                      1]

That is a counter-clockwise rotation.

[-- Attachment #2: Type: text/html, Size: 1193 bytes --]

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Fri, 14 Jun 2019 at 11:45, Alp Aker <alptekin.aker@gmail.com> wrote:

> On Thu, Jun 13, 2019 at 3:01 PM Richard Copley <rcopley@gmail.com> wrote:
>
> >     Yes, but our transformation includes translation, rotation, and
> >     another translation.  Not just one translation.  IOW, it isn't the
> >     transformation matrix that is given; it's the operation on the image.
>
> There's a miscommunication here.  I was speaking to Alan's question about
> how
> to geometrically interpret the components of a transformation matrix.
>
> If the question is what translations to use in order to generate a rotation
> around an arbitrary point p, then there's no question: the sequence of
> operations is translation by -p, then rotation, then translation by p.
>
> > The origin is at the top left so a pure rotation clockwise about the
> origin
> > through angle a goes like this:
> >
> > [cos(a)  -sin(a)  0] [X]   [cos(a) * X - sin(a) * Y]
> > [sin(a)   cos(a)  0] [Y] = [sin(a) * X + cos(a) * Y]
> > [     0        0  1] [1]   [                      1]
>
> That is a counter-clockwise rotation.
>

Are you sure?
This matrix takes (1, 0) to (cos(a), sin(a)), which is roughly (1, a) for
small a.
I think that's clockwise, if a is positive. (Remember the y axis points
down.)

[-- Attachment #2: Type: text/html, Size: 1814 bytes --]

Re: Image transformations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 14 Jun 2019 05:57:21 -0400
> 
> > I think this would be a wasted effort.  There's no need to make all
> > the implementations use the same matrix, unless they all assign the
> > same semantics to the matrix -- which doesn't appear to be the case,
> > at least not as far as this discussion shows.
> 
> There's one case where this would make sense: if we decide to let Elisp
> provide such a matrix.

Why would providing a matrix interface be a good idea?  I think an
interface based on attributes (:crop, :rotation, etc.) is much cleaner
and easier to use.  We can always add more attributes (like :shear) if
we want.

What am I missing?

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On Fri, 14 Jun 2019 at 11:59, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Date: Fri, 14 Jun 2019 05:57:21 -0400
> >
> > > I think this would be a wasted effort.  There's no need to make all
> > > the implementations use the same matrix, unless they all assign the
> > > same semantics to the matrix -- which doesn't appear to be the case,
> > > at least not as far as this discussion shows.
> >
> > There's one case where this would make sense: if we decide to let Elisp
> > provide such a matrix.
>
> Why would providing a matrix interface be a good idea?  I think an
> interface based on attributes (:crop, :rotation, etc.) is much cleaner
> and easier to use.  We can always add more attributes (like :shear) if
> we want.
>
> What am I missing?
>

Firstly, all the 2D graphics libraries (XRender and NS, as we have seen,
GDI,
Direct2D, HTML5 Canvas, Cairo) specify the transformation with a homogeneous
matrix. (The same goes for 3D graphics libraries.) It's fundamental to
graphics
programming. Why make up our own, inferior, system?

(The Windows API documentation is no longer very good for finding out about
previous versions of Windows. Is GM_ADVANCED not supported on some GDI
versions we're interested in? If so, is it important to provide image
support there?)

Secondly, for users who want to specify a scale, translation and rotation,
it's
easy to construct the matrix from that information. (All the graphics
libraries
provide helper functions for this, and so should we.) On the other hand for
users who want to specify the homogeneous matrix, it's not easy to
decompose the
matrix into scale, shear, rotation and translation and rotation.

Thirdly, the homogeneous matrix is compact and uniform, and makes
it easy to compose and invert (undo) transformations.

[-- Attachment #2: Type: text/html, Size: 2495 bytes --]

Re: Image transformations

[YAMAMOTO Mitsuharu]

On Fri, 14 Jun 2019 19:55:49 +0900,
Richard Copley wrote:
> 
> On Fri, 14 Jun 2019 at 11:45, Alp Aker <alptekin.aker@gmail.com> wrote:
> 
> > On Thu, Jun 13, 2019 at 3:01 PM Richard Copley <rcopley@gmail.com> wrote:
> >
> > >     Yes, but our transformation includes translation, rotation, and
> > >     another translation.  Not just one translation.  IOW, it isn't the
> > >     transformation matrix that is given; it's the operation on the image.
> >
> > There's a miscommunication here.  I was speaking to Alan's question about
> > how
> > to geometrically interpret the components of a transformation matrix.
> >
> > If the question is what translations to use in order to generate a rotation
> > around an arbitrary point p, then there's no question: the sequence of
> > operations is translation by -p, then rotation, then translation by p.
> >
> > > The origin is at the top left so a pure rotation clockwise about the
> > origin
> > > through angle a goes like this:
> > >
> > > [cos(a)  -sin(a)  0] [X]   [cos(a) * X - sin(a) * Y]
> > > [sin(a)   cos(a)  0] [Y] = [sin(a) * X + cos(a) * Y]
> > > [     0        0  1] [1]   [                      1]
> >
> > That is a counter-clockwise rotation.
> >
> 
> Are you sure?
> This matrix takes (1, 0) to (cos(a), sin(a)), which is roughly (1, a) for
> small a.
> I think that's clockwise, if a is positive. (Remember the y axis points
> down.)

Other than the direction of the Y axis, there are several sources of
confusion:

1. Some people (mathematicians?) prefer operating on column vectors
   with matrix multiplication from left, others (computer graphics
   people?) prefer operating on row vectors with matrix multiplication
   from right.  Conversion between them involves transposition.

2. At least on cairo, there are "current transformation matrix", which
   is from user space to device space, and "pattern's transformation
   matrix" which is from user space to pattern space.  Here's a quote
   from the document of cairo_pattern_set_matrix telling caveats about
   the latter one:

     Important: Please note that the direction of this transformation
     matrix is from user space to pattern space. This means that if
     you imagine the flow from a pattern to user space (and on to
     device space), then coordinates in that flow will be transformed
     by the inverse of the pattern matrix.

   The matrix constructed in image.c is of the latter type, which is
   more-or-less directly usable on XRender and cairo.  But NS only has
   the former type of transformations.  That's why the NS code
   involves matrix inversion (not transposition).

One must clarify the context then talking about transformation matrices.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Image transformations

[Alp Aker]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]

On Fri, Jun 14, 2019 at 6:56 AM Richard Copley <rcopley@gmail.com> wrote:
>
> Remember the y axis points down.

Oh, I was working with the opposite orientation.  Comment withdrawn!

[-- Attachment #2: Type: text/html, Size: 289 bytes --]

Re: Image transformations

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Fri, 14 Jun 2019 12:21:19 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Emacs Development <emacs-devel@gnu.org>
> 
> Firstly, all the 2D graphics libraries (XRender and NS, as we have
> seen, GDI, Direct2D, HTML5 Canvas, Cairo) specify the transformation
> with a homogeneous matrix. (The same goes for 3D graphics
> libraries.) It's fundamental to graphics programming. Why make up
> our own, inferior, system?

Two reasons: (a) Emacs is not a system for graphics programming, and
doesn't target programmers in that domain; (2) an API based on
transformation matrices is IMO too low-level, unlike anything else in
Emacs.

> (The Windows API documentation is no longer very good for finding out about
> previous versions of Windows. Is GM_ADVANCED not supported on some GDI
> versions we're interested in? If so, is it important to provide image
> support there?)

I'm using this site when I need information about older versions of
Windows:

  http://winapi.freetechsecrets.com/win32

According to it, GM_ADVANCED is not supported on Windows 9X.

> Secondly, for users who want to specify a scale, translation and
> rotation, it's easy to construct the matrix from that
> information.

"Easy"?  For those with background in graphics programming, sure.  For
others, not so sure.

> (All the graphics libraries provide helper functions for this, and
> so should we.)

So you are saying that we should accept the attributes and convert
them to a matrix in Lisp?  That might work, and doesn't really
contradict what I say above, but we should first make sure that all
the toolkits we support interpret the matrix the same.  We are still
arguing about the meaning we ourselves assign to the matrix (see the
recent clockwise vs counter-clockwise rotation issue), which somehow
doesn't increase my confidence that exposing the transformation
through a Lisp API would be a good/useful idea.

> On the other hand for users who want to specify the homogeneous
> matrix, it's not easy to decompose the matrix into scale, shear,
> rotation and translation and rotation.

I still don't understand why users would want to specify
transformations via a matrix.

> Thirdly, the homogeneous matrix is compact and uniform, and makes
> it easy to compose and invert (undo) transformations.

So are/do the attributes.  Which representation is more compact is
arguable: for example, the matrix representation of a rotation is much
more wasteful than just a single angle of rotation parameter.

But I think this is a tangent, because my proposal doesn't preclude
providing a matrix-based API in the future.

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 4625 bytes --]

On Fri, 14 Jun 2019 at 13:07, Eli Zaretskii <eliz@gnu.org> wrote:

    > From: Richard Copley <rcopley@gmail.com>
    > Date: Fri, 14 Jun 2019 12:21:19 +0100
    > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Emacs Development <
emacs-devel@gnu.org>
    >
    > Firstly, all the 2D graphics libraries (XRender and NS, as we have
    > seen, GDI, Direct2D, HTML5 Canvas, Cairo) specify the transformation
    > with a homogeneous matrix. (The same goes for 3D graphics
    > libraries.) It's fundamental to graphics programming. Why make up
    > our own, inferior, system?

    Two reasons: (a) Emacs is not a system for graphics programming, and
    doesn't target programmers in that domain; (2) an API based on
    transformation matrices is IMO too low-level, unlike anything else in
    Emacs.

    > (The Windows API documentation is no longer very good for finding out
about
    > previous versions of Windows. Is GM_ADVANCED not supported on some GDI
    > versions we're interested in? If so, is it important to provide image
    > support there?)

    I'm using this site when I need information about older versions of
    Windows:

      http://winapi.freetechsecrets.com/win32

    According to it, GM_ADVANCED is not supported on Windows 9X.

Is it important to support such advanced features there?

    > Secondly, for users who want to specify a scale, translation and
    > rotation, it's easy to construct the matrix from that
    > information.

    "Easy"?  For those with background in graphics programming, sure.  For
    others, not so sure.

The point I was making is that it is easy compared with going in the
opposite
direction.

    > (All the graphics libraries provide helper functions for this, and
    > so should we.)

    So you are saying that we should accept the attributes and convert
    them to a matrix in Lisp?

Sure. And we shouldn't allow users to access the attributes afterwards.

    That might work, and doesn't really
    contradict what I say above, but we should first make sure that all
    the toolkits we support interpret the matrix the same.

They don't, but the differences are small platform-specific fixups (possibly
inverting and/or transposing the matrix, as well as converting the values
to the
appropriate types). The mathematical fundamentals are the same for all the
platforms.

    We are still
    arguing about the meaning we ourselves assign to the matrix (see the
    recent clockwise vs counter-clockwise rotation issue), which somehow
    doesn't increase my confidence that exposing the transformation
    through a Lisp API would be a good/useful idea.

It's not so important that everybody gets everything correct every time in
this
discussion. The important thing is that the implementation is correct in the
end.

    > On the other hand for users who want to specify the homogeneous
    > matrix, it's not easy to decompose the matrix into scale, shear,
    > rotation and translation and rotation.

    I still don't understand why users would want to specify
    transformations via a matrix.

Most won't.

    > Thirdly, the homogeneous matrix is compact and uniform, and makes
    > it easy to compose and invert (undo) transformations.

    So are/do the attributes.

I think you're underestimating the difficulty of that part. Here's a
concrete
example. Suppose the user wants to apply two transformations,

1. Stretch by factor 2 in the x direction, then rotate clockwise by 45
degrees,
   then translate to the right by 100 pixels.

2. Stretch by factor 0.5 in the x direction, then rotate clockwise by 90
degrees,
   then translate left by 100 pixels.

Calculating the resulting matrix might be a little tricky to get right, but
with
some experimentation you'll get there. Calculating the resulting shear,
rotation, scale and translation is much more difficult to conceptualize. You
would first have to calculate the matrix in any case. Then you would need
to do
Singular Value Decomposition. I can help you with the equations, if it
comes to
it, but I don't think it's needed for this. We just shouldn't expose the
individual parameters to the users in the first place.

    Which representation is more compact is
    arguable: for example, the matrix representation of a rotation is much
    more wasteful than just a single angle of rotation parameter.

I didn't say "more compact".

    But I think this is a tangent, because my proposal doesn't preclude
    providing a matrix-based API in the future.

It will make it difficult to preserve backward compatibility. Constructing
the :crop, :rotate, etc., from the matrix, is hard.

[-- Attachment #2: Type: text/html, Size: 5501 bytes --]

Re: Image transformations

[Yuri Khan]

On Fri, Jun 14, 2019 at 7:53 PM Richard Copley <rcopley@gmail.com> wrote:

>     > Thirdly, the homogeneous matrix is compact and uniform, and makes
>     > it easy to compose and invert (undo) transformations.
>
>     So are/do the attributes.
>
> I think you're underestimating the difficulty of that part. Here's a concrete
> example. Suppose the user wants to apply two transformations,
>
> 1. Stretch by factor 2 in the x direction, then rotate clockwise by 45 degrees,
>    then translate to the right by 100 pixels.
>
> 2. Stretch by factor 0.5 in the x direction, then rotate clockwise by 90 degrees,
>    then translate left by 100 pixels.
>
> Calculating the resulting matrix might be a little tricky to get right, but with
> some experimentation you'll get there. Calculating the resulting shear,
> rotation, scale and translation is much more difficult to conceptualize. You
> would first have to calculate the matrix in any case. Then you would need to do
> Singular Value Decomposition.

If I were to propose a high-level format for describing
transformations in homogeneous coordinates, it wouldn’t be a flat
structure of rotation, shear, scale and translation. I’d suggest a
list of elementary transformations, taken in reverse order of
application, where each element can be one of:

* ('rotate phi)

  {{ cos(phi)  -sin(phi)  0 },
   { sin(phi)   cos(phi)  0 },
   {        0          0  1 }}

* ('scale s)  or ('scale sx sy)

  {{ s 0 0 },    {{ sx  0 0 },
   { 0 s 0 },     {  0 sy 0 },
   { 0 0 1 }}     {  0  0 1 }}

* ('skew ax ay)              or  ('skew ax)

  {{ 1        tan(ax)  0 },      {{ 1  tan(ax)  0 },
   { tan(ay)        1  0 },       { 0        1  0 },
   {       0        0  1 }}       { 0        0  1 }}

* ('translate dx dy)  or  ('translate dx)

  {{ 1 0 dx },            {{ 1 0 dx },
   { 0 1 dy },             { 0 1  0 },
   { 0 0 1  }}             { 0 0  1 }}

* and maybe, for users with advanced needs and a good understanding of maths,
  ('matrix a b c d tx ty)

  {{ a c tx },
   { b d ty },
   { 0 0  1 }}

(An attentive reader will notice that the above data model is lifted
almost verbatim from CSS Transforms.)

Re: Image transformations

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Fri, 14 Jun 2019 13:49:52 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Emacs Development <emacs-devel@gnu.org>
> 
>       http://winapi.freetechsecrets.com/win32
> 
>     According to it, GM_ADVANCED is not supported on Windows 9X.
> 
> Is it important to support such advanced features there?

I'd like to support there what can be supported.  Which means
everything except rotations.

>     That might work, and doesn't really
>     contradict what I say above, but we should first make sure that all
>     the toolkits we support interpret the matrix the same.
> 
> They don't, but the differences are small platform-specific fixups
> (possibly inverting and/or transposing the matrix, as well as
> converting the values to the appropriate types). The mathematical
> fundamentals are the same for all the platforms.

The mathematical fundamentals are the same, but if we interpret the
Lisp-level matrix as only some platforms do, Lisp programmers whose
background is from the rest of the platforms will likely become
confused and will make mistakes.

IOW, I consider complex platform-specific interfaces generally
unsuitable for being exposed on the Lisp level.

>     > Thirdly, the homogeneous matrix is compact and uniform, and makes
>     > it easy to compose and invert (undo) transformations.
> 
>     So are/do the attributes.
> 
> I think you're underestimating the difficulty of that part. Here's a
> concrete example. Suppose the user wants to apply two
> transformations,
> 
> 1. Stretch by factor 2 in the x direction, then rotate clockwise by
> 45 degrees, then translate to the right by 100 pixels.
> 
> 2. Stretch by factor 0.5 in the x direction, then rotate clockwise
> by 90 degrees, then translate left by 100 pixels.
> 
> Calculating the resulting matrix might be a little tricky to get
> right, but with some experimentation you'll get there. Calculating
> the resulting shear, rotation, scale and translation is much more
> difficult to conceptualize.

I don't see why.  We have :width and :height for stretching in a
single direction, and :scale for stretching in both.  Translation is
impossible anyway, because we can only place images by adjusting
buffer contents and using various layout features like :align-to.

>     Which representation is more compact is
>     arguable: for example, the matrix representation of a rotation is much
>     more wasteful than just a single angle of rotation parameter.
> 
> I didn't say "more compact".

You said "compact and uniform".

>     But I think this is a tangent, because my proposal doesn't preclude
>     providing a matrix-based API in the future.
> 
> It will make it difficult to preserve backward compatibility. Constructing
> the :crop, :rotate, etc., from the matrix, is hard.

I don't see why someone will need to deconstruct a matrix, so I don't
think this problem should bother us.

Re: Image transformations

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 4948 bytes --]

On Fri, 14 Jun 2019 at 15:44, Eli Zaretskii <eliz@gnu.org> wrote:

    > From: Richard Copley <rcopley@gmail.com>
    > Date: Fri, 14 Jun 2019 13:49:52 +0100
    > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Emacs Development <
emacs-devel@gnu.org>
    >
    >       http://winapi.freetechsecrets.com/win32
    >
    >     According to it, GM_ADVANCED is not supported on Windows 9X.
    >
    > Is it important to support such advanced features there?

    I'd like to support there what can be supported.  Which means
    everything except rotations.

OK. (I meant to thank you for the link, in my previous message. It got lost
in
editing. Thanks!) The matrix representation wouldn't cause a problem here.
Since
we would disallow applying a rotation or shear, we would be sure that the
matrix
had no rotation or shear component, and it would not be too hard to derive
the
rectangular transformation from the matrix components.

    >     That might work, and doesn't really
    >     contradict what I say above, but we should first make sure that
all
    >     the toolkits we support interpret the matrix the same.
    >
    > They don't, but the differences are small platform-specific fixups
    > (possibly inverting and/or transposing the matrix, as well as
    > converting the values to the appropriate types). The mathematical
    > fundamentals are the same for all the platforms.

    The mathematical fundamentals are the same, but if we interpret the
    Lisp-level matrix as only some platforms do, Lisp programmers whose
    background is from the rest of the platforms will likely become
    confused and will make mistakes.

OK. We would have the advantage of good documentation, as always.

    IOW, I consider complex platform-specific interfaces generally
    unsuitable for being exposed on the Lisp level.

OK, but I don't think homogeneous coordinates are platform specific,
or any more complex than necessary.

    >     > Thirdly, the homogeneous matrix is compact and uniform, and
makes
    >     > it easy to compose and invert (undo) transformations.
    >
    >     So are/do the attributes.
    >
    > I think you're underestimating the difficulty of that part. Here's a
    > concrete example. Suppose the user wants to apply two
    > transformations,
    >
    > 1. Stretch by factor 2 in the x direction, then rotate clockwise by
    > 45 degrees, then translate to the right by 100 pixels.
    >
    > 2. Stretch by factor 0.5 in the x direction, then rotate clockwise
    > by 90 degrees, then translate left by 100 pixels.
    >
    > Calculating the resulting matrix might be a little tricky to get
    > right, but with some experimentation you'll get there. Calculating
    > the resulting shear, rotation, scale and translation is much more
    > difficult to conceptualize.

    I don't see why.  We have :width and :height for stretching in a
    single direction, and :scale for stretching in both.  Translation is
    impossible anyway, because we can only place images by adjusting
    buffer contents and using various layout features like :align-to.

OK. And :rotation? And :shear, since combining stretches and rotations will
allow arbitrary shearing transformations?

I'm imagining allowing these transformations to be composed in any order,
which would be nice for interactive use (a key for rotating, a key for
stretching horizontally, and so on). If we don't want to support that
then yes, you're right, we can calculate the resulting attributes
from one stretch followed by one rotation (or whatever), easily enough.

    >     Which representation is more compact is
    >     arguable: for example, the matrix representation of a rotation is
much
    >     more wasteful than just a single angle of rotation parameter.
    >
    > I didn't say "more compact".

    You said "compact and uniform".

Not "more compact".

    >     But I think this is a tangent, because my proposal doesn't
preclude
    >     providing a matrix-based API in the future.
    >
    > It will make it difficult to preserve backward compatibility.
Constructing
    > the :crop, :rotate, etc., from the matrix, is hard.

    I don't see why someone will need to deconstruct a matrix, so I don't
    think this problem should bother us.

OK. As long as you don't propose allowing composing the simple
transformations,
including rotations, in any order, then you won't need to do that, indeed.

I'm not writing this just to be a thorn in your side. I worry the model
you're
presenting might lead to difficulties that have already been overcome
elsewhere.
But it's probable I have overestimated how much generality you're proposing
to
support.

I like the idea Yuri described. It's as powerful and easier to use. As far
as I
can see there would still be a homogeneous matrix on the implementation
side,
though, with the accompanying variations for each platform, so perhaps it
doesn't overcome your reservations.

[-- Attachment #2: Type: text/html, Size: 5984 bytes --]

Re: Image transformations

[Alan Third]

On Fri, Jun 14, 2019 at 10:05:46AM +0300, Eli Zaretskii wrote:
> 
> The matrix is defined according to XRender, AFAIU.  NS and Cairo need
> to transpose/invert/etc. the matrix to use it, and so will the
> MS-Windows code, IIUC.  The matrix fuses the actual primitive
> transformations into a construct which we currently don't seem to
> understand well, whose only "documentation" is in a tutorial that is
> not part of XRender's official docs.

I was first taught affine transformation matrices in a Mathematics for
Engineers course at university. The lecturer used these exact same
matrices to perform rotations on diagrams drawn on a blackboard. This
was before XRender existed and I can find references to them in a
maths book originally published in 1963. I’m sure they’re even older
than that.

Here’s some more documentation lifted from a google search for ‘affine
transformation matrix’:

https://en.wikipedia.org/wiki/Affine_transformation#Image_transformation
https://www.mathworks.com/discovery/affine-transformation.html
http://mathworld.wolfram.com/AffineTransformation.html
https://eli.thegreenplace.net/2018/affine-transformations/

Transposition of a matrix is literally using m[b][a] instead of
m[a][b].

Inversion of a matrix is a bit more complex but still well within the
capabilities of a high school student:

https://www.mathsisfun.com/algebra/matrix-inverse.html

Besides, the NS API provides a one liner for that:

  [transform invert];
  
We need to do this whether we use the transformation matrix functions
in image.c or use the NS API, as they do the same thing.

FWIW, I wrote this code for NS first. The fact it appears to match the
XRender requirements so well is simply that XRender’s API has been
written to match the existing maths.

> From where I'm standing, this is a scary situation. I believe both
> Cairo and MS-Windows have a much clearer official documentation of
> the related functionality (didn't try looking for NS docs, but I'd
> expect to see good documentation there as well), so clean code could
> be relatively easily written for these platforms using the
> transformation spec's parameters instead of the matrix.

We certainly can, I just feel it’s a waste of everyone’s time.

> > If we were to throw out that shared matrix code and put it into each
> > backend we would be simply duplicating the code.
> 
> I'm not convinced we will end up with a duplication.  Certainly, for
> rotations by multiples of 90 deg no matrix multiplication is necessary
> (though possible) to construct the transform.  And in the Windows case
> having the original parameters of the transform will make it much
> easier to detect the transforms that cannot be supported on older
> platforms than it will be by analyzing the matrix.
> 
> What I'm suggesting in practice is to move the entire calculation of
> the matrix to image_set_transform, instead of communicating the
> individual transformations via the matrix that's calculated piecemeal.
> Then each port will be able to either use that matrix or do something
> different with the transform parameters.  Since image_set_transform is
> the only caller of image_set_size, image_set_crop, and
> image_set_rotation, I see no inherent reasons to return the result of
> those primitive transformations in the form of a matrix.
> 
> WDYT?

I think it’s doable, the only caveats I have is that I feel it adds
complexity and each step needs to know the result of the previous
step. For example we can’t calculate the scale factor without knowing
the width and height calculated by the crop, which in turn relies on
knowing the width and height after the rotation. The matrix
calculations need to know this information too, so we’ll need to hang
onto it.

BTW, are you really keen to get rid of the matrix multiplications?
Anything we replace them with for XRender will simply be matrix
multiplications written out in long form, keeping track of each
element separately.
-- 
Alan Third

Re: Image transformations

[Alan Third]

On Fri, Jun 14, 2019 at 04:55:39PM +0100, Richard Copley wrote:
> 
> I like the idea Yuri described. It's as powerful and easier to use.
> As far as I can see there would still be a homogeneous matrix on the
> implementation side, though, with the accompanying variations for
> each platform, so perhaps it doesn't overcome your reservations.

FWIW I like Yuri’s suggestion too, but even though it provides a
simple API for doing transforms, and could even replace the transform
code in image.c almost entirely (including for ImageMagick) while
still providing some level of backwards compatibility, it introduces
some other questions.

For example how would we handle cropping? With the code in Emacs at
the moment I cheated: I just reset the origin, and set the width and
height; the rest of the image is still there, we just don’t display
it. If we allow arbitrary translations and rotations after the crop,
however, then it’s reasonable for the user to expect the cropped area
to translate and rotate, without reintroducing areas previously
cropped out. I don’t know how to get round this without, perhaps,
modifying the mask, which isn’t supported on NS at least, or drawing
the image to a new surface at the moment of cropping. I want to avoid
that if possible, but it may be the only solution.

-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Sat, 15 Jun 2019 11:42:42 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > The matrix is defined according to XRender, AFAIU.  NS and Cairo need
> > to transpose/invert/etc. the matrix to use it, and so will the
> > MS-Windows code, IIUC.  The matrix fuses the actual primitive
> > transformations into a construct which we currently don't seem to
> > understand well, whose only "documentation" is in a tutorial that is
> > not part of XRender's official docs.
> 
> I was first taught affine transformation matrices in a Mathematics for
> Engineers course at university.

We are mis-communicating.  I know what affine transformations are, and
I have no problems with matrix multiplication and vector algebra in
general.  I majored in physics, so even tensors of General Relativity
aren't a problem for me.

The issue I'm worried about is the geometrical meaning of each element
of the matrix, as applied to image transformations in Emacs: whether
it needs to be transposed before multiplying vectors by it, whether
vectors should be left-multiplied or right-multiplied, whether the Y
axis is assumed to go up or down, whether the [1][2] member is the
sine of the rotation angle or its negative, etc.

Even Richard and Alp were confused for a moment about this stuff.
Which is a clear sign that these aspects are not as trivial as it
might sound.  IMO, we need clear documentation of that, to allow
people make changes in the code without making mistakes.  But maybe
I'm the only one to be bothered by that.

> Inversion of a matrix is a bit more complex but still well within the
> capabilities of a high school student:

So it is well within your and my capabilities, and those of all the
rest here.  Please let's not worry about this level, it's not the
problem I'm talking about.

> BTW, are you really keen to get rid of the matrix multiplications?
> Anything we replace them with for XRender will simply be matrix
> multiplications written out in long form, keeping track of each
> element separately.

This is again a misunderstanding: I was merely suggesting to make all
of those multiplications in image_set_transform, that's all.

Maybe I should stop talking about this and just go ahead and write the
best code I can.  Because instead of improving things, this discussion
seems to just proliferate misunderstandings and bad feelings.  Most
probably, my fault, sorry.

So please push your documentation changes, and let's move on.

Re: Image transformations

[Eli Zaretskii]

> Date: Sat, 15 Jun 2019 12:00:49 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>,
> 	Emacs Development <emacs-devel@gnu.org>
> 
> For example how would we handle cropping? With the code in Emacs at
> the moment I cheated: I just reset the origin, and set the width and
> height; the rest of the image is still there, we just don’t display
> it. If we allow arbitrary translations and rotations after the crop,
> however, then it’s reasonable for the user to expect the cropped area
> to translate and rotate, without reintroducing areas previously
> cropped out. I don’t know how to get round this without, perhaps,
> modifying the mask, which isn’t supported on NS at least, or drawing
> the image to a new surface at the moment of cropping. I want to avoid
> that if possible, but it may be the only solution.

Another possible solution is not to support cropping, except with
ImageMagick.  Would that really be much of a loss, given that Emacs
can display a slice of an image since Emacs 21?

Re: Image transformations

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

On Sat, Jun 15, 2019 at 02:31:41PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 15 Jun 2019 11:42:42 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > > The matrix is defined according to XRender, AFAIU.  NS and Cairo need
> > > to transpose/invert/etc. the matrix to use it, and so will the
> > > MS-Windows code, IIUC.  The matrix fuses the actual primitive
> > > transformations into a construct which we currently don't seem to
> > > understand well, whose only "documentation" is in a tutorial that is
> > > not part of XRender's official docs.
> > 
> > I was first taught affine transformation matrices in a Mathematics for
> > Engineers course at university.
> 
> We are mis-communicating.  I know what affine transformations are, and
> I have no problems with matrix multiplication and vector algebra in
> general.  I majored in physics, so even tensors of General Relativity
> aren't a problem for me.

I know nothing of your background other than what I’ve read on here,
so I apologise for being patronising. I’ve clearly misunderstood what
you were asking for repeatedly.

> The issue I'm worried about is the geometrical meaning of each element
> of the matrix, as applied to image transformations in Emacs: whether
> it needs to be transposed before multiplying vectors by it, whether
> vectors should be left-multiplied or right-multiplied, whether the Y
> axis is assumed to go up or down, whether the [1][2] member is the
> sine of the rotation angle or its negative, etc.
> 
> Even Richard and Alp were confused for a moment about this stuff.
> Which is a clear sign that these aspects are not as trivial as it
> might sound.  IMO, we need clear documentation of that, to allow
> people make changes in the code without making mistakes.  But maybe
> I'm the only one to be bothered by that.

I think as long as we’re consistent with our own representation then
we can treat the question of whether the toolkits have transposed the
matrix terms as implementation details. The code IS consistent,
unfortunately my first attempt at documenting it WASN’T, which is
entirely on me. That caused at least some of the confusion.

The attached patch has more, hopefully better, documentation.

> > BTW, are you really keen to get rid of the matrix multiplications?
> > Anything we replace them with for XRender will simply be matrix
> > multiplications written out in long form, keeping track of each
> > element separately.
> 
> This is again a misunderstanding: I was merely suggesting to make all
> of those multiplications in image_set_transform, that's all.

OK. Do you want me to go ahead and do that? I don’t want to start
making big changes if you’re working on the Windows side. I have a
suggestion that will simplify this job though.

In another part of this thread you pointed out that crop and image
slices are doing basically the same thing. I hadn’t realised that.
Given that that’s the case, I think we should just get rid of the crop
function.

Getting rid of it will also get rid of that difficult to understand
resize ‐> crop ‐> rotate process where crop parameters have to take
the effects of the resize into account.

> Maybe I should stop talking about this and just go ahead and write the
> best code I can.  Because instead of improving things, this discussion
> seems to just proliferate misunderstandings and bad feelings.  Most
> probably, my fault, sorry.

No bad feelings on my part, just confusion, which was largely my own
fault anyway. I hope we can finish this off without any more
misunderstandings.
-- 
Alan Third

[-- Attachment #2: 0001-Document-image-transforms.patch --]
[-- Type: text/plain, Size: 14075 bytes --]

From bcd6c010f5fe78d4254d9c5e8181d37415f9744c Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Tue, 11 Jun 2019 20:31:24 +0100
Subject: [PATCH] Document image transforms

* doc/lispref/display.texi (Image Descriptors): Document :crop and
update :rotation.
* src/image.c: Describe the image transform matrix layout.
* test/manual/image-transforms-tests.el: New file.
---
 doc/lispref/display.texi              |  23 +++-
 src/image.c                           |  80 ++++++++++++
 test/manual/image-transforms-tests.el | 176 ++++++++++++++++++++++++++
 3 files changed, 278 insertions(+), 1 deletion(-)
 create mode 100644 test/manual/image-transforms-tests.el

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 93c5217c36..5a5b77e709 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5181,8 +5181,29 @@ Image Descriptors
 specified, the height/width will be adjusted by the specified scaling
 factor.
 
+@item :crop @var{geometry}
+This should be a list of the form @code{(@var{width} @var{height}
+@var{x} @var{y})}.  @var{width} and @var{height} 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.
+
+If the crop area is outside or overlaps the edge of the image it will
+be reduced to exclude any areas outside of the image.  This means it
+is not possible to use @code{:crop} to increase the size of the image
+by entering large @var{width} or @var{height} values.
+
+Cropping is performed after scaling but before rotation.
+
 @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 counter-clockwise.  Rotation
+is performed after scaling and cropping.
 
 @item :index @var{frame}
 @xref{Multi-Frame Images}.
diff --git a/src/image.c b/src/image.c
index 86f8e8f4bb..6dc14db880 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1967,6 +1967,86 @@ compute_image_size (size_t width, size_t height,
 }
 #endif /* HAVE_IMAGEMAGICK || HAVE_NATIVE_TRANSFORMS */
 
+/* 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[1][0] = m12 | m[2][0] = tx
+   --------------+---------------+-------------
+   m[0][1] = m21 | m[1][1] = m22 | m[2][1] = ty
+   --------------+---------------+-------------
+   m[0][2] = 0   | m[1][2] = 0   | m[2][2] = 1
+
+   tx and ty represent translations, m11 and m22 represent scaling
+   transforms and m21 and m12 represent shear transforms.  Most
+   graphics toolkits don't require the third row, however it is
+   necessary for multiplication.
+
+   Transforms are done by creating a matrix for each action we wish to
+   take, then multiplying the transformation matrix by each of those
+   matrices in order (matrix multiplication is not commutative).
+   After we’ve done that we can use our modified transformation matrix
+   to transform points.  We take the x and y coordinates and convert
+   them into a 3x1 matrix and multiply that by the transformation
+   matrix and it gives us a new set of coordinates:
+
+       [x]   [m11 m12 tx]   [x']
+       [y] X [m21 m22 ty] = [y']
+       [1]   [  0   0  1]   [1 ]
+
+   Luckily we don’t have to worry about the last step as the graphics
+   toolkit will do it for us.
+
+   The three transforms we are concerned with are translation, scaling
+   and rotation.  The translation matrix looks like this:
+
+       [1 0 tx]
+       [0 1 ty]
+       [0 0  1]
+
+   Where tx and ty are the amount to translate the origin in the x and
+   y coordinates, respectively.  Since we are translating the origin
+   and not the image data itself, it can appear backwards in use, for
+   example to move the image 10 pixels to the right, you would set tx
+   to -10.
+
+   To scale we use:
+
+       [x 0 0]
+       [0 y 0]
+       [0 0 1]
+
+   Where x and y are the amounts to scale in the x and y dimensions.
+   Values larger than 1 make the image larger, values smaller than 1
+   make it smaller.  Negative values flip the image.
+
+   To rotate we use:
+
+       [ cos(r) sin(r) 0]
+       [-sin(r) cos(r) 0]
+       [      0      0 1]
+
+   Where r is the angle of rotation required.  Rotation occurs around
+   the origin, not the centre of the image.  Note that mathematically
+   this is considered a counter-clockwise rotation, however because
+   our y axis is reversed, (0, 0) at the top left, it works as a
+   clockwise rotation.
+
+   The full process of rotating an image is to move the origin to the
+   centre of the image (width/2, height/2), perform the rotation, and
+   finally move the origin back to the top left of the image, which
+   may now be a different corner.
+
+   Cropping is easier as we just move the origin to the top left of
+   where we want to crop and set the width and height accordingly.
+   The matrices don’t know anything about width and height.
+
+   It's possible to pre-calculate the matrix multiplications and just
+   generate one transform matrix that will do everything we need in a
+   single step, but the maths for each element is much more complex
+   and I thought it was better to perform the steps separately.  */
+
 typedef double matrix3x3[3][3];
 
 static void
diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
new file mode 100644
index 0000000000..d601b9397e
--- /dev/null
+++ b/test/manual/image-transforms-tests.el
@@ -0,0 +1,176 @@
+;;; image-transform-tests.el --- Test suite for image transforms.
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Alan Third <alan@idiocy.org>
+;; Keywords:       internal
+;; Human-Keywords: internal
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Type M-x test-transforms RET to generate the test buffer.
+
+;;; Code:
+
+(defun test-rotation ()
+  (let ((up "<svg height='9' width='9'><polygon points='0,8 4,0 8,8'/></svg>")
+        (down "<svg height='9' width='9'><polygon points='0,0 4,8 8,0'/></svg>")
+        (left "<svg height='9' width='9'><polygon points='8,0 0,4 8,8'/></svg>")
+        (right "<svg height='9' width='9'><polygon points='0,0 8,4 0,8'/></svg>"))
+    (insert-header "Test Rotation: rotating an image")
+    (insert-test "0" up up '(:rotation 0))
+    (insert-test "360" up up '(:rotation 360))
+    (insert-test "180" down up '(:rotation 180))
+    (insert-test "-90" left up '(:rotation -90))
+    (insert-test "90" right up '(:rotation 90))
+    (insert-test "90.0" right up '(:rotation 90.0))
+
+    ;; This should log a message and display the unrotated image.
+    (insert-test "45" up up '(:rotation 45)))
+  (insert "\n\n"))
+
+(defun test-cropping ()
+  (let ((image "<svg height='30' width='30'>
+                  <rect x='0' y='0' width='10' height='10'/>
+                  <rect x='10' y='10' width='10' height='10'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                  <line x1='10' y1='10' x2='20' y2='20' style='stroke:#000'/>
+                  <line x1='20' y1='10' x2='10' y2='20' style='stroke:#000'/>
+                  <rect x='20' y='20' width='10' height='10'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                </svg>")
+        (top-left "<svg height='10' width='10'>
+                     <rect x='0' y='0' width='10' height='10'/>
+                   </svg>")
+        (middle "<svg height='10' width='10'>
+                   <rect x='0' y='0' width='10' height='10'
+                         style='fill:none;stroke-width:1;stroke:#000'/>
+                   <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
+                   <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
+                 </svg>")
+        (bottom-right "<svg height='10' width='10'>
+                         <rect x='0' y='0' width='10' height='10'
+                               style='fill:none;stroke-width:1;stroke:#000'/>
+                       </svg>"))
+    (insert-header "Test Crop: cropping an image")
+    (insert-test "all params" top-left image '(:crop (10 10 0 0)))
+    (insert-test "width/height only" middle image '(:crop (10 10)))
+    (insert-test "negative x y" middle image '(:crop (10 10 -10 -10)))
+    (insert-test "all params" bottom-right image '(:crop (10 10 20 20))))
+  (insert "\n\n"))
+
+(defun test-scaling ()
+  (let ((image "<svg height='10' width='10'>
+                  <rect x='0' y='0' width='10' height='10'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                  <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
+                  <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
+                </svg>")
+        (large "<svg height='20' width='20'>
+                  <rect x='0' y='0' width='20' height='20'
+                        style='fill:none;stroke-width:2;stroke:#000'/>
+                  <line x1='0' y1='0' x2='20' y2='20'
+                        style='stroke-width:2;stroke:#000'/>
+                  <line x1='20' y1='0' x2='0' y2='20'
+                        style='stroke-width:2;stroke:#000'/>
+                </svg>")
+        (small "<svg height='5' width='5'>
+                  <rect x='0' y='0' width='4' height='4'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                  <line x1='0' y1='0' x2='4' y2='4' style='stroke:#000'/>
+                  <line x1='4' y1='0' x2='0' y2='4' style='stroke:#000'/>
+                </svg>"))
+    (insert-header "Test Scaling: resize an image (pixelization may occur)")
+    (insert-test "1x" image image '(:scale 1))
+    (insert-test "2x" large image '(:scale 2))
+    (insert-test "0.5x" image large '(:scale 0.5))
+    (insert-test ":max-width" image large '(:max-width 10))
+    (insert-test ":max-height" image large '(:max-height 10))
+    (insert-test "width, height" image large '(:width 10 :height 10)))
+  (insert "\n\n"))
+
+(defun test-scaling-rotation ()
+  (let ((image "<svg height='20' width='20'>
+                  <rect x='0' y='0' width='20' height='20'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                  <rect x='0' y='0' width='10' height='10'
+                        style='fill:#000'/>
+                </svg>")
+        (x2-90 "<svg height='40' width='40'>
+                  <rect x='0' y='0' width='40' height='40'
+                        style='fill:none;stroke-width:1;stroke:#000'/>
+                  <rect x='20' y='0' width='20' height='20'
+                        style='fill:#000'/>
+                </svg>")
+        (x2--90 "<svg height='40' width='40'>
+                   <rect x='0' y='0' width='40' height='40'
+                         style='fill:none;stroke-width:1;stroke:#000'/>
+                   <rect x='0' y='20' width='20' height='20'
+                         style='fill:#000'/>
+                 </svg>")
+        (x0.5-180 "<svg height='10' width='10'>
+                     <rect x='0' y='0' width='10' height='10'
+                           style='fill:none;stroke-width:1;stroke:#000'/>
+                     <rect x='5' y='5' width='5' height='5'
+                           style='fill:#000'/>
+                   </svg>"))
+    (insert-header "Test Scaling and Rotation: resize and rotate an image (pixelization may occur)")
+    (insert-test "1x, 0 degrees" image image '(:scale 1 :rotation 0))
+    (insert-test "2x, 90 degrees" x2-90 image '(:scale 2 :rotation 90.0))
+    (insert-test "2x, -90 degrees" x2--90 image '(:scale 2 :rotation -90.0))
+    (insert-test "0.5x, 180 degrees" x0.5-180 image '(:scale 0.5 :rotation 180.0)))
+  (insert "\n\n"))
+
+(defun insert-header (description)
+  (insert description)
+  (insert "\n")
+  (indent-to 38)
+  (insert "expected")
+  (indent-to 48)
+  (insert "result")
+  (when (fboundp #'imagemagick-types)
+    (indent-to 58)
+    (insert "ImageMagick"))
+  (insert "\n"))
+
+(defun insert-test (description expected image params)
+  (indent-to 2)
+  (insert description)
+  (indent-to 40)
+  (insert-image (create-image expected 'svg t))
+  (indent-to 50)
+  (insert-image (apply #'create-image image 'svg t params))
+  (when (fboundp #'imagemagick-types)
+    (indent-to 60)
+    (insert-image (apply #'create-image image 'imagemagick t params)))
+  (insert "\n"))
+
+(defun test-transforms ()
+  (interactive)
+  (let ((buf (get-buffer "*Image Transform Test*")))
+    (if buf
+	(kill-buffer buf))
+    (switch-to-buffer (get-buffer-create "*Image Transform Test*"))
+    (erase-buffer)
+    (unless #'imagemagick-types
+      (insert "ImageMagick not detected.  ImageMagick tests will be skipped.\n\n"))
+    (test-rotation)
+    (test-cropping)
+    (test-scaling)
+    (test-scaling-rotation)
+    (goto-char (point-min))))
-- 
2.21.0

Re: Image transformations

[Eli Zaretskii]

> Date: Sun, 16 Jun 2019 16:22:59 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> The attached patch has more, hopefully better, documentation.

Thanks, see a few comments below.  Please push this regardless, it's a
huge improvement.

> > This is again a misunderstanding: I was merely suggesting to make all
> > of those multiplications in image_set_transform, that's all.
> 
> OK. Do you want me to go ahead and do that? I don’t want to start
> making big changes if you’re working on the Windows side. I have a
> suggestion that will simplify this job though.

Let me finish my attempts to use the matrix as-is, I have one more
attempt in queue.  After that, if I fail, moving the matrix
calculations into image_set_transform would be a necessity, as all the
alternatives I could think of are much uglier.

> In another part of this thread you pointed out that crop and image
> slices are doing basically the same thing. I hadn’t realised that.
> Given that that’s the case, I think we should just get rid of the crop
> function.

I agree, but please make sure the slices are indeed a 100% replacement
(I cannot yet do that because I don't have a working :crop support to
try it).

> I hope we can finish this off without any more misunderstandings.

Thanks, same here.

> +@item :crop @var{geometry}
> +This should be a list of the form @code{(@var{width} @var{height}
> +@var{x} @var{y})}.  @var{width} and @var{height} 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
                                         ^^
One more space there.

> +       [x]   [m11 m12 tx]   [x']
> +       [y] X [m21 m22 ty] = [y']
> +       [1]   [  0   0  1]   [1 ]

Either we use a column vector, and it should be on the right of the
matrix; or we use a row vector, in which case it should be on the
left.  Which of these is the case?

> diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
> new file mode 100644
> index 0000000000..d601b9397e
> --- /dev/null
> +++ b/test/manual/image-transforms-tests.el

This is great, but Windows only supports displaying images from files,
so we will have to write these specs to files, and then display them.

Thanks again for documenting this stuff.

Re: Image transformations

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Sun, Jun 16, 2019 at 07:34:53PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 16 Jun 2019 16:22:59 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > In another part of this thread you pointed out that crop and image
> > slices are doing basically the same thing. I hadn’t realised that.
> > Given that that’s the case, I think we should just get rid of the crop
> > function.
> 
> I agree, but please make sure the slices are indeed a 100% replacement
> (I cannot yet do that because I don't have a working :crop support to
> try it).

As far as I can tell they are perhaps a 90% replacement, the only real
difference is that the interactive keybinds from image.el don’t work
(r for rotate, + for scale up, etc.). They can probably be made to
work, but to be honest I’m not sure it matters. If someone is cropping
an image I can’t think of any use cases where they’d want to then
provide the user the ability to play with that image interactively.

Perhaps I’m just not imaginative enough.

> > +       [x]   [m11 m12 tx]   [x']
> > +       [y] X [m21 m22 ty] = [y']
> > +       [1]   [  0   0  1]   [1 ]
> 
> Either we use a column vector, and it should be on the right of the
> matrix; or we use a row vector, in which case it should be on the
> left.  Which of these is the case?

It is the former. I fixed it before pushing.

> > diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
> > new file mode 100644
> > index 0000000000..d601b9397e
> > --- /dev/null
> > +++ b/test/manual/image-transforms-tests.el
> 
> This is great, but Windows only supports displaying images from files,
> so we will have to write these specs to files, and then display them.

That’s unfortunate. Patch attached.
-- 
Alan Third

[-- Attachment #2: 0001-Move-inline-SVG-images-to-separate-files.patch --]
[-- Type: text/plain, Size: 16821 bytes --]

From a9b7f48e7a1b4c1541226467919e263c0b3c8cea Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 17 Jun 2019 22:07:44 +0100
Subject: [PATCH] Move inline SVG images to separate files

* test/data/image/transform-crop-base.svg: New file.
* test/data/image/transform-crop-bottom-right.svg: New file.
* test/data/image/transform-crop-middle.svg: New file.
* test/data/image/transform-crop-top-left.svg: New file.
* test/data/image/transform-rotation-down.svg: New file.
* test/data/image/transform-rotation-left.svg: New file.
* test/data/image/transform-rotation-right.svg: New file.
* test/data/image/transform-rotation-up.svg: New file.
* test/data/image/transform-scaling-base.svg: New file.
* test/data/image/transform-scaling-large.svg: New file.
* test/data/image/transform-scaling-rotation-base.svg: New file.
* test/data/image/transform-scaling-rotation-x0.5-180.svg: New file.
* test/data/image/transform-scaling-rotation-x2--90.svg: New file.
* test/data/image/transform-scaling-rotation-x2-90.svg: New file.
* test/data/image/transform-scaling-small.svg: New file.
* test/manual/image-transforms-tests.el (test-rotation):
(test-cropping):
(test-transforms): Remove inline SVG image data and replace with new
image files.
(insert-test): Construct paths to new files and use them.
---
 test/data/image/transform-crop-base.svg       |   9 ++
 .../image/transform-crop-bottom-right.svg     |   4 +
 test/data/image/transform-crop-middle.svg     |   6 +
 test/data/image/transform-crop-top-left.svg   |   3 +
 test/data/image/transform-rotation-down.svg   |   3 +
 test/data/image/transform-rotation-left.svg   |   3 +
 test/data/image/transform-rotation-right.svg  |   3 +
 test/data/image/transform-rotation-up.svg     |   3 +
 test/data/image/transform-scaling-base.svg    |   6 +
 test/data/image/transform-scaling-large.svg   |   8 ++
 .../image/transform-scaling-rotation-base.svg |   6 +
 .../transform-scaling-rotation-x0.5-180.svg   |   6 +
 .../transform-scaling-rotation-x2--90.svg     |   6 +
 .../transform-scaling-rotation-x2-90.svg      |   6 +
 test/data/image/transform-scaling-small.svg   |   6 +
 test/manual/image-transforms-tests.el         | 108 +++++-------------
 16 files changed, 106 insertions(+), 80 deletions(-)
 create mode 100644 test/data/image/transform-crop-base.svg
 create mode 100644 test/data/image/transform-crop-bottom-right.svg
 create mode 100644 test/data/image/transform-crop-middle.svg
 create mode 100644 test/data/image/transform-crop-top-left.svg
 create mode 100644 test/data/image/transform-rotation-down.svg
 create mode 100644 test/data/image/transform-rotation-left.svg
 create mode 100644 test/data/image/transform-rotation-right.svg
 create mode 100644 test/data/image/transform-rotation-up.svg
 create mode 100644 test/data/image/transform-scaling-base.svg
 create mode 100644 test/data/image/transform-scaling-large.svg
 create mode 100644 test/data/image/transform-scaling-rotation-base.svg
 create mode 100644 test/data/image/transform-scaling-rotation-x0.5-180.svg
 create mode 100644 test/data/image/transform-scaling-rotation-x2--90.svg
 create mode 100644 test/data/image/transform-scaling-rotation-x2-90.svg
 create mode 100644 test/data/image/transform-scaling-small.svg

diff --git a/test/data/image/transform-crop-base.svg b/test/data/image/transform-crop-base.svg
new file mode 100644
index 0000000000..188f47ac29
--- /dev/null
+++ b/test/data/image/transform-crop-base.svg
@@ -0,0 +1,9 @@
+<svg height="30" width="30">
+  <rect x="0" y="0" width="10" height="10"/>
+  <rect x="10" y="10" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <line x1="10" y1="10" x2="20" y2="20" style="stroke:#000"/>
+  <line x1="20" y1="10" x2="10" y2="20" style="stroke:#000"/>
+  <rect x="20" y="20" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+</svg>
diff --git a/test/data/image/transform-crop-bottom-right.svg b/test/data/image/transform-crop-bottom-right.svg
new file mode 100644
index 0000000000..74e09f03da
--- /dev/null
+++ b/test/data/image/transform-crop-bottom-right.svg
@@ -0,0 +1,4 @@
+<svg height="10" width="10">
+  <rect x="0" y="0" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+</svg>
diff --git a/test/data/image/transform-crop-middle.svg b/test/data/image/transform-crop-middle.svg
new file mode 100644
index 0000000000..e707df1e55
--- /dev/null
+++ b/test/data/image/transform-crop-middle.svg
@@ -0,0 +1,6 @@
+<svg height="10" width="10">
+  <rect x="0" y="0" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <line x1="0" y1="0" x2="10" y2="10" style="stroke:#000"/>
+  <line x1="10" y1="0" x2="0" y2="10" style="stroke:#000"/>
+</svg>
diff --git a/test/data/image/transform-crop-top-left.svg b/test/data/image/transform-crop-top-left.svg
new file mode 100644
index 0000000000..0f62f89a05
--- /dev/null
+++ b/test/data/image/transform-crop-top-left.svg
@@ -0,0 +1,3 @@
+<svg height="10" width="10">
+  <rect x="0" y="0" width="10" height="10"/>
+</svg>
diff --git a/test/data/image/transform-rotation-down.svg b/test/data/image/transform-rotation-down.svg
new file mode 100644
index 0000000000..67f3e11e64
--- /dev/null
+++ b/test/data/image/transform-rotation-down.svg
@@ -0,0 +1,3 @@
+<svg height="9" width="9">
+  <polygon points="0,0 4,8 8,0"/>
+</svg>
diff --git a/test/data/image/transform-rotation-left.svg b/test/data/image/transform-rotation-left.svg
new file mode 100644
index 0000000000..33cfab6264
--- /dev/null
+++ b/test/data/image/transform-rotation-left.svg
@@ -0,0 +1,3 @@
+<svg height="9" width="9">
+  <polygon points="8,0 0,4 8,8"/>
+</svg>
diff --git a/test/data/image/transform-rotation-right.svg b/test/data/image/transform-rotation-right.svg
new file mode 100644
index 0000000000..60f247cf42
--- /dev/null
+++ b/test/data/image/transform-rotation-right.svg
@@ -0,0 +1,3 @@
+<svg height="9" width="9">
+  <polygon points="0,0 8,4 0,8"/>
+</svg>
diff --git a/test/data/image/transform-rotation-up.svg b/test/data/image/transform-rotation-up.svg
new file mode 100644
index 0000000000..57d545a99d
--- /dev/null
+++ b/test/data/image/transform-rotation-up.svg
@@ -0,0 +1,3 @@
+<svg height="9" width="9">
+  <polygon points="0,8 4,0 8,8"/>
+</svg>
diff --git a/test/data/image/transform-scaling-base.svg b/test/data/image/transform-scaling-base.svg
new file mode 100644
index 0000000000..e707df1e55
--- /dev/null
+++ b/test/data/image/transform-scaling-base.svg
@@ -0,0 +1,6 @@
+<svg height="10" width="10">
+  <rect x="0" y="0" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <line x1="0" y1="0" x2="10" y2="10" style="stroke:#000"/>
+  <line x1="10" y1="0" x2="0" y2="10" style="stroke:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-large.svg b/test/data/image/transform-scaling-large.svg
new file mode 100644
index 0000000000..0803f073dc
--- /dev/null
+++ b/test/data/image/transform-scaling-large.svg
@@ -0,0 +1,8 @@
+<svg height="20" width="20">
+  <rect x="0" y="0" width="20" height="20"
+        style="fill:none;stroke-width:2;stroke:#000"/>
+  <line x1="0" y1="0" x2="20" y2="20"
+        style="stroke-width:2;stroke:#000"/>
+  <line x1="20" y1="0" x2="0" y2="20"
+        style="stroke-width:2;stroke:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-rotation-base.svg b/test/data/image/transform-scaling-rotation-base.svg
new file mode 100644
index 0000000000..046a0523a2
--- /dev/null
+++ b/test/data/image/transform-scaling-rotation-base.svg
@@ -0,0 +1,6 @@
+<svg height="20" width="20">
+  <rect x="0" y="0" width="20" height="20"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <rect x="0" y="0" width="10" height="10"
+        style="fill:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-rotation-x0.5-180.svg b/test/data/image/transform-scaling-rotation-x0.5-180.svg
new file mode 100644
index 0000000000..8bbbb5ce03
--- /dev/null
+++ b/test/data/image/transform-scaling-rotation-x0.5-180.svg
@@ -0,0 +1,6 @@
+<svg height="10" width="10">
+  <rect x="0" y="0" width="10" height="10"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <rect x="5" y="5" width="5" height="5"
+        style="fill:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-rotation-x2--90.svg b/test/data/image/transform-scaling-rotation-x2--90.svg
new file mode 100644
index 0000000000..9c62df53e3
--- /dev/null
+++ b/test/data/image/transform-scaling-rotation-x2--90.svg
@@ -0,0 +1,6 @@
+<svg height="40" width="40">
+  <rect x="0" y="0" width="40" height="40"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <rect x="0" y="20" width="20" height="20"
+        style="fill:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-rotation-x2-90.svg b/test/data/image/transform-scaling-rotation-x2-90.svg
new file mode 100644
index 0000000000..189d0b1a15
--- /dev/null
+++ b/test/data/image/transform-scaling-rotation-x2-90.svg
@@ -0,0 +1,6 @@
+<svg height="40" width="40">
+  <rect x="0" y="0" width="40" height="40"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <rect x="20" y="0" width="20" height="20"
+        style="fill:#000"/>
+</svg>
diff --git a/test/data/image/transform-scaling-small.svg b/test/data/image/transform-scaling-small.svg
new file mode 100644
index 0000000000..2e51cb2901
--- /dev/null
+++ b/test/data/image/transform-scaling-small.svg
@@ -0,0 +1,6 @@
+<svg height="5" width="5">
+  <rect x="0" y="0" width="4" height="4"
+        style="fill:none;stroke-width:1;stroke:#000"/>
+  <line x1="0" y1="0" x2="4" y2="4" style="stroke:#000"/>
+  <line x1="4" y1="0" x2="0" y2="4" style="stroke:#000"/>
+</svg>
diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
index d601b9397e..739df2f2ac 100644
--- a/test/manual/image-transforms-tests.el
+++ b/test/manual/image-transforms-tests.el
@@ -28,10 +28,10 @@
 ;;; Code:
 
 (defun test-rotation ()
-  (let ((up "<svg height='9' width='9'><polygon points='0,8 4,0 8,8'/></svg>")
-        (down "<svg height='9' width='9'><polygon points='0,0 4,8 8,0'/></svg>")
-        (left "<svg height='9' width='9'><polygon points='8,0 0,4 8,8'/></svg>")
-        (right "<svg height='9' width='9'><polygon points='0,0 8,4 0,8'/></svg>"))
+  (let ((up "transform-rotation-up.svg")
+        (down "transform-rotation-down.svg")
+        (left "transform-rotation-left.svg")
+        (right "transform-rotation-right.svg"))
     (insert-header "Test Rotation: rotating an image")
     (insert-test "0" up up '(:rotation 0))
     (insert-test "360" up up '(:rotation 360))
@@ -45,28 +45,10 @@ test-rotation
   (insert "\n\n"))
 
 (defun test-cropping ()
-  (let ((image "<svg height='30' width='30'>
-                  <rect x='0' y='0' width='10' height='10'/>
-                  <rect x='10' y='10' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='10' y1='10' x2='20' y2='20' style='stroke:#000'/>
-                  <line x1='20' y1='10' x2='10' y2='20' style='stroke:#000'/>
-                  <rect x='20' y='20' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                </svg>")
-        (top-left "<svg height='10' width='10'>
-                     <rect x='0' y='0' width='10' height='10'/>
-                   </svg>")
-        (middle "<svg height='10' width='10'>
-                   <rect x='0' y='0' width='10' height='10'
-                         style='fill:none;stroke-width:1;stroke:#000'/>
-                   <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
-                   <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
-                 </svg>")
-        (bottom-right "<svg height='10' width='10'>
-                         <rect x='0' y='0' width='10' height='10'
-                               style='fill:none;stroke-width:1;stroke:#000'/>
-                       </svg>"))
+  (let ((image "transform-crop-base.svg")
+        (top-left "transform-crop-top-left.svg")
+        (middle "transform-crop-middle.svg")
+        (bottom-right "transform-crop-bottom-right.svg"))
     (insert-header "Test Crop: cropping an image")
     (insert-test "all params" top-left image '(:crop (10 10 0 0)))
     (insert-test "width/height only" middle image '(:crop (10 10)))
@@ -75,26 +57,9 @@ test-cropping
   (insert "\n\n"))
 
 (defun test-scaling ()
-  (let ((image "<svg height='10' width='10'>
-                  <rect x='0' y='0' width='10' height='10'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='0' y1='0' x2='10' y2='10' style='stroke:#000'/>
-                  <line x1='10' y1='0' x2='0' y2='10' style='stroke:#000'/>
-                </svg>")
-        (large "<svg height='20' width='20'>
-                  <rect x='0' y='0' width='20' height='20'
-                        style='fill:none;stroke-width:2;stroke:#000'/>
-                  <line x1='0' y1='0' x2='20' y2='20'
-                        style='stroke-width:2;stroke:#000'/>
-                  <line x1='20' y1='0' x2='0' y2='20'
-                        style='stroke-width:2;stroke:#000'/>
-                </svg>")
-        (small "<svg height='5' width='5'>
-                  <rect x='0' y='0' width='4' height='4'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <line x1='0' y1='0' x2='4' y2='4' style='stroke:#000'/>
-                  <line x1='4' y1='0' x2='0' y2='4' style='stroke:#000'/>
-                </svg>"))
+  (let ((image "transform-scaling-base.svg")
+        (large "transform-scaling-large.svg")
+        (small "transform-scaling-small.svg"))
     (insert-header "Test Scaling: resize an image (pixelization may occur)")
     (insert-test "1x" image image '(:scale 1))
     (insert-test "2x" large image '(:scale 2))
@@ -105,30 +70,10 @@ test-scaling
   (insert "\n\n"))
 
 (defun test-scaling-rotation ()
-  (let ((image "<svg height='20' width='20'>
-                  <rect x='0' y='0' width='20' height='20'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <rect x='0' y='0' width='10' height='10'
-                        style='fill:#000'/>
-                </svg>")
-        (x2-90 "<svg height='40' width='40'>
-                  <rect x='0' y='0' width='40' height='40'
-                        style='fill:none;stroke-width:1;stroke:#000'/>
-                  <rect x='20' y='0' width='20' height='20'
-                        style='fill:#000'/>
-                </svg>")
-        (x2--90 "<svg height='40' width='40'>
-                   <rect x='0' y='0' width='40' height='40'
-                         style='fill:none;stroke-width:1;stroke:#000'/>
-                   <rect x='0' y='20' width='20' height='20'
-                         style='fill:#000'/>
-                 </svg>")
-        (x0.5-180 "<svg height='10' width='10'>
-                     <rect x='0' y='0' width='10' height='10'
-                           style='fill:none;stroke-width:1;stroke:#000'/>
-                     <rect x='5' y='5' width='5' height='5'
-                           style='fill:#000'/>
-                   </svg>"))
+  (let ((image "transform-scaling-rotation-base.svg")
+        (x2-90 "transform-scaling-rotation-x2-90.svg")
+        (x2--90 "transform-scaling-rotation-x2--90.svg")
+        (x0.5-180 "transform-scaling-rotation-x0.5-180.svg"))
     (insert-header "Test Scaling and Rotation: resize and rotate an image (pixelization may occur)")
     (insert-test "1x, 0 degrees" image image '(:scale 1 :rotation 0))
     (insert-test "2x, 90 degrees" x2-90 image '(:scale 2 :rotation 90.0))
@@ -149,16 +94,19 @@ insert-header
   (insert "\n"))
 
 (defun insert-test (description expected image params)
-  (indent-to 2)
-  (insert description)
-  (indent-to 40)
-  (insert-image (create-image expected 'svg t))
-  (indent-to 50)
-  (insert-image (apply #'create-image image 'svg t params))
-  (when (fboundp #'imagemagick-types)
-    (indent-to 60)
-    (insert-image (apply #'create-image image 'imagemagick t params)))
-  (insert "\n"))
+  (let* ((image-dir (concat source-directory "test/data/image"))
+         (expected (expand-file-name expected image-dir))
+        (image (expand-file-name image image-dir)))
+    (indent-to 2)
+    (insert description)
+    (indent-to 40)
+    (insert-image (create-image expected 'svg nil))
+    (indent-to 50)
+    (insert-image (apply #'create-image image 'svg nil params))
+    (when (fboundp #'imagemagick-types)
+      (indent-to 60)
+      (insert-image (apply #'create-image image 'imagemagick nil params)))
+    (insert "\n")))
 
 (defun test-transforms ()
   (interactive)
-- 
2.21.0

Re: Image transformations

[Tak Kunihiro]

I have looked for explanation of Affine transformation but could not
find one with keywords that are slide, gain, distortion, and rotation.
Thus I have created a note in a following link (section BASICS).
 
http://dream.misasa.okayama-u.ac.jp/documentation/AffineTheory/report.pdf

By the conversion, an image will be projected to a space.  To export the
projected image, a window should be set explicitly. After a rotation, a
window with the original dimension or a window with dimension that can
cover the whole projected image can be set. To project and to export are
two different operation.

I hope this message helps.

Re: Image transformations

[Eli Zaretskii]

> Date: Mon, 17 Jun 2019 22:13:32 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > This is great, but Windows only supports displaying images from files,
> > so we will have to write these specs to files, and then display them.
> 
> That’s unfortunate. Patch attached.

I'm terribly sorry, my failing memory tricked me here.  Windows does
support :data for images, the problem with :data is with play-sound,
not with images.  Apologies for making you invest extra effort.

Re: Image transformations

[Eli Zaretskii]

> Date: Mon, 17 Jun 2019 22:13:32 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > > In another part of this thread you pointed out that crop and image
> > > slices are doing basically the same thing. I hadn’t realised that.
> > > Given that that’s the case, I think we should just get rid of the crop
> > > function.
> > 
> > I agree, but please make sure the slices are indeed a 100% replacement
> > (I cannot yet do that because I don't have a working :crop support to
> > try it).
> 
> As far as I can tell they are perhaps a 90% replacement, the only real
> difference is that the interactive keybinds from image.el don’t work
> (r for rotate, + for scale up, etc.). They can probably be made to
> work, but to be honest I’m not sure it matters. If someone is cropping
> an image I can’t think of any use cases where they’d want to then
> provide the user the ability to play with that image interactively.

I finished working on the Windows implementation, and have a working
prototype.  The tests you wrote all work, with the single exception of
cropping (not surprisingly, since I didn't really figure out how is
this supposed to work).  I guess the above means I don't have to dig
into this, as we are losing :crop in the native transforms, is that
right?

So now I'm ready to discuss the restructuring of the transform
matrices code.

In my research I found out that the equivalent Windows graphics APIs
need a matrix that is the inverse of what we calculate in image.c.  I
think that calculating a matrix and then inverting it at draw time is
inefficient at best and silly at worst; I'd rather calculate the
inverse matrix to begin with.  Calculating the inverse matrix involves
reversing all the primitive transformations (translations change sign,
rotations change the sign of the angle), and also changing the
multiplication order from left-multiplication of matrices to
right-multiplication.

Given these findings, what would you prefer to do in image.c?  We
could either (a) make the individual functions (image_set_rotation
etc.) return the transformation parameters, and leave the matrix
calculation to image_set_transform, or we could have the individual
functions accept a flag to tell them whether to generate XRender/Cairo
style matrices or NS/Windows style.  Or maybe you ave yet another
idea?

Finally, there's the issue of what to return from image-transforms-p.

Thanks.

Re: Image transformations

[Stefan Monnier]

> In my research I found out that the equivalent Windows graphics APIs
> need a matrix that is the inverse of what we calculate in image.c.

[ Beware, I know next to nothing about graphics APIs and such, so please
  assume I'm mostly just spewing hot air.  ]

IIRC one matrix is needed when computing "display position from
image position" and the inverse is needed when computing "image position
from display position".  So this difference seems to reflect
a fundamental difference in the way the problem is approached (unless
the API(s) perform(s) matrix inversion internally as well).

Right?

Also, one of the two matrices can't be computed if the transformation
turns the 2D image into a line (or a point).

> I think that calculating a matrix and then inverting it at draw time
> is inefficient

Compared to the cost of handling the image, inverting a 3x3 matrix
is probably negligible, tho.

> Given these findings, what would you prefer to do in image.c?  We
> could either (a) make the individual functions (image_set_rotation
> etc.) return the transformation parameters, and leave the matrix
> calculation to image_set_transform, or we could have the individual
> functions accept a flag to tell them whether to generate XRender/Cairo
> style matrices or NS/Windows style.  Or maybe you ave yet another
> idea?

As long as the matrices aren't made visible to Elisp and only one GUI
can be compiled at a time, we could also use a compile-time flag.


        Stefan

Re: Image transformations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 24 Jun 2019 15:50:14 -0400
> 
> IIRC one matrix is needed when computing "display position from
> image position" and the inverse is needed when computing "image position
> from display position".

I don't think I understand what "display position" means here.  Did
you mean coordinates of a pixel?

What actually happens AFAIU is that in some cases the matrix describes
how to convert the original image to the transformed image, while in
others it describes how to transform the image space into the
transformed space.

> Also, one of the two matrices can't be computed if the transformation
> turns the 2D image into a line (or a point).

We don't support such transformations.

> > I think that calculating a matrix and then inverting it at draw time
> > is inefficient
> 
> Compared to the cost of handling the image, inverting a 3x3 matrix
> is probably negligible, tho.

Maybe so, but it still sounds silly to me.

> > Given these findings, what would you prefer to do in image.c?  We
> > could either (a) make the individual functions (image_set_rotation
> > etc.) return the transformation parameters, and leave the matrix
> > calculation to image_set_transform, or we could have the individual
> > functions accept a flag to tell them whether to generate XRender/Cairo
> > style matrices or NS/Windows style.  Or maybe you ave yet another
> > idea?
> 
> As long as the matrices aren't made visible to Elisp and only one GUI
> can be compiled at a time, we could also use a compile-time flag.

Yes, but #ifdef's uglify the code too much, IMO.

Re: Image transformations

[Stefan Monnier]

> I don't think I understand what "display position" means here.  Did
> you mean coordinates of a pixel?

Yes, I meant the coordinate in the display as opposed to coordinates in
the original image.

> What actually happens AFAIU is that in some cases the matrix describes
> how to convert the original image to the transformed image, while in
> others it describes how to transform the image space into the
> transformed space.

That's what I meant to say, yes.

>> Also, one of the two matrices can't be computed if the transformation
>> turns the 2D image into a line (or a point).
> We don't support such transformations.

There's no option to "magnify" by a factor 0?

>> Compared to the cost of handling the image, inverting a 3x3 matrix
>> is probably negligible, tho.
> Maybe so, but it still sounds silly to me.

Part of the reason for my question above is to figure out if the
implementation of the API is likely to have such matrix inversion
code internally (since it seems quite possible that sometimes one is
preferable and sometimes the other).


        Stefan

Re: Image transformations

[Eli Zaretskii]

On June 25, 2019 6:28:32 AM GMT+03:00, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
> >> Also, one of the two matrices can't be computed if the
> transformation
> >> turns the 2D image into a line (or a point).
> > We don't support such transformations.
> 
> There's no option to "magnify" by a factor 0?

Factor zero will give you zero pixels, which is not what you want.

But more generally: no, we don't currently support that.  Did you try it?

> >> Compared to the cost of handling the image, inverting a 3x3 matrix
> >> is probably negligible, tho.
> > Maybe so, but it still sounds silly to me.
> 
> Part of the reason for my question above is to figure out if the
> implementation of the API is likely to have such matrix inversion
> code internally

How can we possibly know for sure, on all supported platforms?

 > (since it seems quite possible that sometimes one is
> preferable and sometimes the other).

I admit I don't see why we would have different preferences.  We need to comply to public interfaces, which means use what they require.

Re: Image transformations

[Stefan Monnier]

>> Part of the reason for my question above is to figure out if the
>> implementation of the API is likely to have such matrix inversion
>> code internally
> How can we possibly know for sure, on all supported platforms?

I'm not looking for "know for sure", just for a general understanding of
how things usually work.

>> (since it seems quite possible that sometimes one is
>> preferable and sometimes the other).
> I admit I don't see why we would have different preferences.

I'm asking about the other side of the API, i.e. not us but the
implementors of Cairo, etc...

It's really just out of curiosity, not directly related to what our code
should do: does the "inverse matrix" indicate that the Windows guys use
"fundamentally" different algorithms than the Cairo guys, or just that
inverting the matrix is considered "par for the course" because
sometimes you need one and sometimes you need the other.


        Stefan

Re: Image transformations

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Tue, 25 Jun 2019 10:43:22 -0400
> 
> >> Part of the reason for my question above is to figure out if the
> >> implementation of the API is likely to have such matrix inversion
> >> code internally
> > How can we possibly know for sure, on all supported platforms?
> 
> I'm not looking for "know for sure", just for a general understanding of
> how things usually work.
> 
> >> (since it seems quite possible that sometimes one is
> >> preferable and sometimes the other).
> > I admit I don't see why we would have different preferences.
> 
> I'm asking about the other side of the API, i.e. not us but the
> implementors of Cairo, etc...
> 
> It's really just out of curiosity, not directly related to what our code
> should do: does the "inverse matrix" indicate that the Windows guys use
> "fundamentally" different algorithms than the Cairo guys, or just that
> inverting the matrix is considered "par for the course" because
> sometimes you need one and sometimes you need the other.

I'm not the right person to answer these questions, as I don't have
enough background and experience in this area.  I think the differences
reflect the different designs and mindsets of the relevant
environments, but that's just a guess.  Perhaps Yamamoto-san could
comment on the nature of these differences.

Re: Image transformations

[Alan Third]

On Mon, Jun 24, 2019 at 08:54:25PM +0300, Eli Zaretskii wrote:
> > Date: Mon, 17 Jun 2019 22:13:32 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > > > In another part of this thread you pointed out that crop and image
> > > > slices are doing basically the same thing. I hadn’t realised that.
> > > > Given that that’s the case, I think we should just get rid of the crop
> > > > function.
> > > 
> > > I agree, but please make sure the slices are indeed a 100% replacement
> > > (I cannot yet do that because I don't have a working :crop support to
> > > try it).
> > 
> > As far as I can tell they are perhaps a 90% replacement, the only real
> > difference is that the interactive keybinds from image.el don’t work
> > (r for rotate, + for scale up, etc.). They can probably be made to
> > work, but to be honest I’m not sure it matters. If someone is cropping
> > an image I can’t think of any use cases where they’d want to then
> > provide the user the ability to play with that image interactively.
> 
> I finished working on the Windows implementation, and have a working
> prototype.  The tests you wrote all work, with the single exception of
> cropping (not surprisingly, since I didn't really figure out how is
> this supposed to work).  I guess the above means I don't have to dig
> into this, as we are losing :crop in the native transforms, is that
> right?

Yes. I’m happy to drop :crop entirely. Although we might be as well
leaving it for ImageMagick, as it provides a RAM saving by actually
cropping the image, if people care enough about that.

> So now I'm ready to discuss the restructuring of the transform
> matrices code.
> 
> In my research I found out that the equivalent Windows graphics APIs
> need a matrix that is the inverse of what we calculate in image.c.  I
> think that calculating a matrix and then inverting it at draw time is
> inefficient at best and silly at worst; I'd rather calculate the
> inverse matrix to begin with.  Calculating the inverse matrix involves
> reversing all the primitive transformations (translations change sign,
> rotations change the sign of the angle), and also changing the
> multiplication order from left-multiplication of matrices to
> right-multiplication.

Am I right in thinking we need to invert the scaling too? 1/scale?

> Given these findings, what would you prefer to do in image.c?  We
> could either (a) make the individual functions (image_set_rotation
> etc.) return the transformation parameters, and leave the matrix
> calculation to image_set_transform, or we could have the individual
> functions accept a flag to tell them whether to generate XRender/Cairo
> style matrices or NS/Windows style.  Or maybe you ave yet another
> idea?

The image size calculation is already a separate function, shared with
ImageMagick, so I feel we’re halfway towards (a) as it is. I’m happy
to put all the matrix code into image_set_transform: once we remove
cropping the resulting function shouldn’t be obscenely large.

> Finally, there's the issue of what to return from image-transforms-p.

Perhaps we return something like:

  '((:rotation imagemagick)
    (:width png gif svg imagemagick)
    (:height png gif svg imagemagick)
    ...)
    
where png, svg, imagemagick, etc. are types (image backends) that you
can pass to create-image.

Or it might be better to spin it round a bit:

  '((png :width :height ...)
    (imagemagick :rotation :width ...))
    
Whenever I think about this I get a bit carried away and wonder if we
could do it by image type rather than backend so that we could provide
a function, that you pass an image type and spec to, which finds the
first image backend that can handle it:

  (image-get-type "tiff"
    '(:rotation 90 :max-width 20))
  => 'imagemagick

  (image-get-type "jpeg"
    '(:rotation 90 :max-width 20))
  => 'jpeg

I can’t decide if that’s getting a bit silly, though.
-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Tue, 25 Jun 2019 19:33:52 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> Yes. I’m happy to drop :crop entirely. Although we might be as well
> leaving it for ImageMagick, as it provides a RAM saving by actually
> cropping the image, if people care enough about that.

Sure, I didn't mean to remove :crop from ImageMagick images.

> > In my research I found out that the equivalent Windows graphics APIs
> > need a matrix that is the inverse of what we calculate in image.c.  I
> > think that calculating a matrix and then inverting it at draw time is
> > inefficient at best and silly at worst; I'd rather calculate the
> > inverse matrix to begin with.  Calculating the inverse matrix involves
> > reversing all the primitive transformations (translations change sign,
> > rotations change the sign of the angle), and also changing the
> > multiplication order from left-multiplication of matrices to
> > right-multiplication.
> 
> Am I right in thinking we need to invert the scaling too? 1/scale?

Yes, of course.

> > Given these findings, what would you prefer to do in image.c?  We
> > could either (a) make the individual functions (image_set_rotation
> > etc.) return the transformation parameters, and leave the matrix
> > calculation to image_set_transform, or we could have the individual
> > functions accept a flag to tell them whether to generate XRender/Cairo
> > style matrices or NS/Windows style.  Or maybe you ave yet another
> > idea?
> 
> The image size calculation is already a separate function, shared with
> ImageMagick, so I feel we’re halfway towards (a) as it is. I’m happy
> to put all the matrix code into image_set_transform: once we remove
> cropping the resulting function shouldn’t be obscenely large.

OK, I will work on these changes.

>   '((:rotation imagemagick)
>     (:width png gif svg imagemagick)
>     (:height png gif svg imagemagick)
>     ...)
>     
> where png, svg, imagemagick, etc. are types (image backends) that you
> can pass to create-image.
> 
> Or it might be better to spin it round a bit:
> 
>   '((png :width :height ...)
>     (imagemagick :rotation :width ...))
>     
> Whenever I think about this I get a bit carried away and wonder if we
> could do it by image type rather than backend so that we could provide
> a function, that you pass an image type and spec to, which finds the
> first image backend that can handle it:
> 
>   (image-get-type "tiff"
>     '(:rotation 90 :max-width 20))
>   => 'imagemagick
> 
>   (image-get-type "jpeg"
>     '(:rotation 90 :max-width 20))
>   => 'jpeg
> 
> I can’t decide if that’s getting a bit silly, though.

Let me think about this and propose something.

Thanks.

Re: Image transformations

[YAMAMOTO Mitsuharu]

On Wed, 26 Jun 2019 00:35:54 +0900,
Eli Zaretskii wrote:
> 
> > It's really just out of curiosity, not directly related to what our code
> > should do: does the "inverse matrix" indicate that the Windows guys use
> > "fundamentally" different algorithms than the Cairo guys, or just that
> > inverting the matrix is considered "par for the course" because
> > sometimes you need one and sometimes you need the other.
> 
> I'm not the right person to answer these questions, as I don't have
> enough background and experience in this area.  I think the differences
> reflect the different designs and mindsets of the relevant
> environments, but that's just a guess.  Perhaps Yamamoto-san could
> comment on the nature of these differences.

I'm not the right person, either.  So the following is just a guess.
First, I couldn't find any Windows GDI API that deals with affine
transformation matrices directly.  PlgBlt that Eli mentioned takes
(integer-valued) coordinates of three corners of the destination.

  https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/nf-wingdi-plgblt

Probably Eli uses "inverse matrix" (inverse with respect to what
Xrender uses) to calculate these coordinates from the source image
width and height.  If so, "inverse matrix" is considered not inherent
to Window GDI, but usually used indirectly at the user side.

Guessing from its API design, I think GDI does not internally perform
any (real-valued) matrix calculations, but uses some variant of
Bresenham's line algorithm for image transformations to prefer integer
arithmetics.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Image transformations

[Eli Zaretskii]

> Date: Wed, 26 Jun 2019 09:28:17 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel@gnu.org
> 
> First, I couldn't find any Windows GDI API that deals with affine
> transformation matrices directly.

The GDI API which uses the matrices directly is SetWorldTransform, see

  https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/nf-wingdi-setworldtransform

I eventually decided not to use it, because when I tried, it displayed
a slightly stretched image even with a trivial matrix, i.e. no
transform at all.  Also, the more general description of the topic,
which you can find here:

  https://docs.microsoft.com/en-us/windows/desktop/gdi/about-coordinate-spaces-and-transformations

and the examples here:

  https://docs.microsoft.com/en-us/windows/desktop/gdi/using-coordinate-spaces-and-transformations

did not tell enough detail nor show enough examples for me to be sure
I really understand the subject.  So I'm using PlgBlt instead.  But I
still want to keep compatibility with the transform matrices as
defined in the documentation of SetWorldTransform, and I'm using the
equations shown there to produce the coordinates that PlgBlt needs.
This will allow to use SetWorldTransform in the future if we want to
(assuming someone will understand why it didn't work for me to be gin
with).

> Guessing from its API design, I think GDI does not internally perform
> any (real-valued) matrix calculations, but uses some variant of
> Bresenham's line algorithm for image transformations to prefer integer
> arithmetics.

I think the fact that SetWorldTransform exists contradicts this
conclusion.  The matrix is defined as an array of floats.

Re: Image transformations

[YAMAMOTO Mitsuharu]

On Thu, 27 Jun 2019 00:34:00 +0900,
Eli Zaretskii wrote:
> 
> > Date: Wed, 26 Jun 2019 09:28:17 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> > 	emacs-devel@gnu.org
> > 
> > First, I couldn't find any Windows GDI API that deals with affine
> > transformation matrices directly.
> 
> The GDI API which uses the matrices directly is SetWorldTransform, see
> 
>   https://docs.microsoft.com/en-us/windows/desktop/api/wingdi/nf-wingdi-setworldtransform
> 
> I eventually decided not to use it, because when I tried, it displayed
> a slightly stretched image even with a trivial matrix, i.e. no
> transform at all.  Also, the more general description of the topic,
> which you can find here:
> 
>   https://docs.microsoft.com/en-us/windows/desktop/gdi/about-coordinate-spaces-and-transformations
> 
> and the examples here:
> 
>   https://docs.microsoft.com/en-us/windows/desktop/gdi/using-coordinate-spaces-and-transformations
> 
> did not tell enough detail nor show enough examples for me to be sure
> I really understand the subject.  So I'm using PlgBlt instead.  But I
> still want to keep compatibility with the transform matrices as
> defined in the documentation of SetWorldTransform, and I'm using the
> equations shown there to produce the coordinates that PlgBlt needs.
> This will allow to use SetWorldTransform in the future if we want to
> (assuming someone will understand why it didn't work for me to be gin
> with).
> 
> > Guessing from its API design, I think GDI does not internally perform
> > any (real-valued) matrix calculations, but uses some variant of
> > Bresenham's line algorithm for image transformations to prefer integer
> > arithmetics.
> 
> I think the fact that SetWorldTransform exists contradicts this
> conclusion.  The matrix is defined as an array of floats.

It seems that SetWorldTransform is only effective on vector drawings,
but not on bitmap image drawings.  So my reasoning above should be
restricted to bitmap images, but still just a guess, of course.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Image transformations

[Eli Zaretskii]

> Date: Thu, 27 Jun 2019 12:37:15 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: monnier@iro.umontreal.ca,
> 	emacs-devel@gnu.org
> 
> > > Guessing from its API design, I think GDI does not internally perform
> > > any (real-valued) matrix calculations, but uses some variant of
> > > Bresenham's line algorithm for image transformations to prefer integer
> > > arithmetics.
> > 
> > I think the fact that SetWorldTransform exists contradicts this
> > conclusion.  The matrix is defined as an array of floats.
> 
> It seems that SetWorldTransform is only effective on vector drawings,
> but not on bitmap image drawings.

My conclusion was different, because I saw examples of using
SetWorldTransform to rotate bitmap images.  But I don't think it's
important enough to argue about, as we both are just guessing.

Re: Image transformations

[Eli Zaretskii]

> Date: Tue, 25 Jun 2019 21:57:56 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > > Given these findings, what would you prefer to do in image.c?  We
> > > could either (a) make the individual functions (image_set_rotation
> > > etc.) return the transformation parameters, and leave the matrix
> > > calculation to image_set_transform, or we could have the individual
> > > functions accept a flag to tell them whether to generate XRender/Cairo
> > > style matrices or NS/Windows style.  Or maybe you ave yet another
> > > idea?
> > 
> > The image size calculation is already a separate function, shared with
> > ImageMagick, so I feel we’re halfway towards (a) as it is. I’m happy
> > to put all the matrix code into image_set_transform: once we remove
> > cropping the resulting function shouldn’t be obscenely large.
> 
> OK, I will work on these changes.
> 
> >   '((:rotation imagemagick)
> >     (:width png gif svg imagemagick)
> >     (:height png gif svg imagemagick)
> >     ...)
> >     
> > where png, svg, imagemagick, etc. are types (image backends) that you
> > can pass to create-image.
> > 
> > Or it might be better to spin it round a bit:
> > 
> >   '((png :width :height ...)
> >     (imagemagick :rotation :width ...))
> >     
> > Whenever I think about this I get a bit carried away and wonder if we
> > could do it by image type rather than backend so that we could provide
> > a function, that you pass an image type and spec to, which finds the
> > first image backend that can handle it:
> > 
> >   (image-get-type "tiff"
> >     '(:rotation 90 :max-width 20))
> >   => 'imagemagick
> > 
> >   (image-get-type "jpeg"
> >     '(:rotation 90 :max-width 20))
> >   => 'jpeg
> > 
> > I can’t decide if that’s getting a bit silly, though.
> 
> Let me think about this and propose something.

What I have now is below.  This doesn't yet include documentation
changes.

I could only test this on MS-Windows, but I hope I didn't goof on
other platforms.  I left the NS code to use the same matrix as
XRender/Cairo, as I couldn't test the result of letting it use the
inverted matrix like MS-Windows does.  If using that matrix works for
NS, the inversion in nsimage.m could be removed.

There's a FIXME in image_set_rotation, please tell what you think
about it.

Please also comment on image-transforms-p.  Maybe we should return
both 'rotate' and 'rotate90' in the list when ImageMagick is
available?

Any other comments are welcome, of course.  TIA.

diff --git a/src/dispextern.h b/src/dispextern.h
index cc2d963..5d66fd8 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3020,6 +3020,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
   /* Picture versions of pixmap and mask for compositing.  */
   Picture picture, mask_picture;
 # endif
+#endif	/* HAVE_X_WINDOWS */
+#ifdef HAVE_NTGUI
+  XFORM xform;
 #endif
 
   /* Colors allocated for this image, if any.  Allocated via xmalloc.  */
diff --git a/src/image.c b/src/image.c
index 7b648c4..6c240c3 100644
--- a/src/image.c
+++ b/src/image.c
@@ -57,6 +57,10 @@ Copyright (C) 1989, 1992-2019 Free Software Foundation, Inc.
 #include <sys/types.h>
 #endif /* HAVE_SYS_TYPES_H */
 
+#ifdef HAVE_NATIVE_TRANSFORMS
+#include <float.h>	/* for FLT_MIN */
+#endif
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -1965,12 +1969,10 @@ compute_image_size (size_t width, size_t height,
   *d_width = desired_width;
   *d_height = desired_height;
 }
-#endif /* HAVE_IMAGEMAGICK || HAVE_NATIVE_TRANSFORMS */
 
-/* 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:
+/* image_set_rotation 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[1][0] = m12 | m[2][0] = tx
    --------------+---------------+-------------
@@ -2039,14 +2041,15 @@ compute_image_size (size_t width, size_t height,
    finally move the origin back to the top left of the image, which
    may now be a different corner.
 
-   Cropping is easier as we just move the origin to the top left of
-   where we want to crop and set the width and height accordingly.
-   The matrices don't know anything about width and height.
+   Note that different GUI backends (X, Cairo, w32, NS) want the
+   transform matrix defined as transform from the original image to
+   the transformed image, while others want the matrix to describe the
+   transform of the space, which boils down to inverting the matrix.
 
    It's possible to pre-calculate the matrix multiplications and just
    generate one transform matrix that will do everything we need in a
    single step, but the maths for each element is much more complex
-   and I thought it was better to perform the steps separately.  */
+   and performing the steps separately makes for more readable code.  */
 
 typedef double matrix3x3[3][3];
 
@@ -2070,102 +2073,25 @@ matrix3x3_mult (matrix3x3 a, matrix3x3 b, matrix3x3 result)
 }
 
 static void
-image_set_rotation (struct image *img, matrix3x3 tm)
+image_set_rotation (struct image *img, double *rotation)
 {
-#ifdef HAVE_NATIVE_TRANSFORMS
-# ifdef HAVE_IMAGEMAGICK
-  /* ImageMagick images are already rotated.  */
-  if (EQ (image_spec_value (img->spec, QCtype, NULL), Qimagemagick))
-    return;
-# endif
-
-# if !defined USE_CAIRO && defined HAVE_XRENDER
-  if (!img->picture)
-    return;
-# endif
-
-  int rotation, cos_r, sin_r, width, height;
-
   Lisp_Object value = image_spec_value (img->spec, QCrotation, NULL);
+  /* FIXME: should we signal an error here?  */
   if (! NUMBERP (value))
     return;
 
   Lisp_Object reduced_angle = Fmod (value, make_fixnum (360));
-  if (! FLOATP (reduced_angle))
-    rotation = XFIXNUM (reduced_angle);
-  else
-    {
-      rotation = XFLOAT_DATA (reduced_angle);
-      if (rotation != XFLOAT_DATA (reduced_angle))
-	goto not_a_multiple_of_90;
-    }
-
-  if (rotation == 0)
-    return;
-
-  if (rotation == 90)
-    {
-      width = img->height;
-      height = img->width;
-
-      cos_r = 0;
-      sin_r = 1;
-    }
-  else if (rotation == 180)
-    {
-      width = img->width;
-      height = img->height;
-
-      cos_r = -1;
-      sin_r = 0;
-    }
-  else if (rotation == 270)
-    {
-      width = img->height;
-      height = img->width;
-
-      cos_r = 0;
-      sin_r = -1;
-    }
+  if (FLOATP (reduced_angle))
+    *rotation = XFLOAT_DATA (reduced_angle);
   else
-    {
-    not_a_multiple_of_90:
-      image_error ("Native image rotation supports "
-		   "only multiples of 90 degrees");
-      return;
-    }
-
-  /* Translate so (0, 0) is in the center of the image.  */
-  matrix3x3 t
-    = { [0][0] = 1,
-				  [1][1] = 1,
-	[2][0] = img->width * .5, [2][1] = img->height * .5, [2][2] = 1 };
-  matrix3x3 tmp;
-  matrix3x3_mult (t, tm, tmp);
-
-  /* Rotate.  */
-  matrix3x3 rot = { [0][0] = cos_r, [0][1] = -sin_r,
-		    [1][0] = sin_r, [1][1] = cos_r,
-						     [2][2] = 1 };
-  matrix3x3 tmp2;
-  matrix3x3_mult (rot, tmp, tmp2);
-
-  /* Translate back.  */
-  t[2][0] = width * -.5;
-  t[2][1] = height * -.5;
-  matrix3x3_mult (t, tmp2, tm);
-
-  img->width = width;
-  img->height = height;
-#endif
+    *rotation = XFIXNUM (reduced_angle);
 }
 
 static void
-image_set_crop (struct image *img, matrix3x3 tm)
+image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)
 {
-#ifdef HAVE_NATIVE_TRANSFORMS
 # ifdef HAVE_IMAGEMAGICK
-  /* ImageMagick images are already cropped.  */
+  /* ImageMagick images already have the correct transform.  */
   if (EQ (image_spec_value (img->spec, QCtype, NULL), Qimagemagick))
     return;
 # endif
@@ -2175,119 +2101,110 @@ image_set_crop (struct image *img, matrix3x3 tm)
     return;
 # endif
 
-  Lisp_Object crop = image_spec_value (img->spec, QCcrop, NULL);
-
-  if (!CONSP (crop))
-    return;
-
-  Lisp_Object w = XCAR (crop), h = Qnil, x = Qnil, y = Qnil;
-  crop = XCDR (crop);
-  if (CONSP (crop))
-    {
-      h = XCAR (crop);
-      crop = XCDR (crop);
-      if (CONSP (crop))
-	{
-	  x = XCAR (crop);
-	  crop = XCDR (crop);
-	  if (CONSP (crop))
-	    y = XCAR (crop);
-	}
-    }
-
-  int width = img->width;
-  if (FIXNATP (w) && XFIXNAT (w) < img->width)
-    width = XFIXNAT (w);
-  int left;
-  if (TYPE_RANGED_FIXNUMP (int, x))
-    {
-      left = XFIXNUM (x);
-      if (left < 0)
-        left = img->width - width + left;
-    }
-  else
-    left = (img->width - width) >> 1;
-
-  int height = img->height;
-  if (FIXNATP (h) && XFIXNAT (h) < img->height)
-    height = XFIXNAT (h);
-  int top;
-  if (TYPE_RANGED_FIXNUMP (int, y))
-    {
-      top = XFIXNUM (y);
-      if (top < 0)
-        top = img->height - height + top;
-    }
-  else
-    top = (img->height - height) >> 1;
-
-  /* Negative values operate from the right and bottom of the image
-     instead of the left and top.  */
-  if (left < 0)
-    {
-      width = img->width + left;
-      left = 0;
-    }
-
-  if (width + left > img->width)
-    width = img->width - left;
+  /* Determine size.  */
+  int width, height;
+  compute_image_size (img->width, img->height, img->spec, &width, &height);
 
-  if (top < 0)
-    {
-      height = img->height + top;
-      top = 0;
-    }
+  /* Determine rotation.  */
+  double rotation = 0.0;
+  image_set_rotation (img, &rotation);
 
-  if (height + top > img->height)
-    height = img->height - top;
+  /* Perform scale transformation.  */
 
-  matrix3x3 tmp, m = { [0][0] = 1,
-				      [1][1] = 1,
-		       [2][0] = left, [2][1] = top, [2][2] = 1 };
-  matrix3x3_mult (m, tm, tmp);
-  matrix3x3_copy (tmp, tm);
+  double xscale = (double) width / img->width;
+  double yscale =  (double) height / img->height;
 
+# if defined USE_CAIRO || defined HAVE_XRENDER || defined HAVE_NS
+  /* Avoid division by zero.  */
+  xscale = max (xscale, FLT_MIN);
+  matrix3x3 tmp, rm =
+    { [0][0] = 1.0 / xscale, [1][1] = 1.0 / yscale, [2][2] = 1 };
+  matrix3x3_mult (rm, matrix, tmp);
+# elif defined HAVE_NTGUI
+  matrix3x3 tmp, rm = { [0][0] = xscale, [1][1] = yscale, [2][2] = 1 };
+  matrix3x3_mult (matrix, rm, tmp);
+# endif
   img->width = width;
   img->height = height;
-#endif
-}
 
-static void
-image_set_size (struct image *img, matrix3x3 tm)
-{
-#ifdef HAVE_NATIVE_TRANSFORMS
-# ifdef HAVE_IMAGEMAGICK
-  /* ImageMagick images are already the correct size.  */
-  if (EQ (image_spec_value (img->spec, QCtype, NULL), Qimagemagick))
-    return;
-# endif
-
-# if !defined USE_CAIRO && defined HAVE_XRENDER
-  if (!img->picture)
-    return;
-# endif
+  /* Perform rotation transformation.  */
 
-  int width, height;
-
-  compute_image_size (img->width, img->height, img->spec, &width, &height);
+  int cos_r, sin_r;
+  if (rotation == 0)
+    matrix3x3_copy (tmp, matrix);
+  else if (rotation == 90 || rotation == 180 || rotation == 270)
+    {
+      if (rotation == 90)
+	{
+	  width = img->height;
+	  height = img->width;
 
-  double xscale = img->width / (double) width;
-  double yscale = img->height / (double) height;
+	  cos_r = 0;
+	  sin_r = 1;
+	}
+      else if (rotation == 180)
+	{
+	  width = img->width;
+	  height = img->height;
 
-  matrix3x3 tmp, rm = { [0][0] = xscale, [1][1] = yscale, [2][2] = 1 };
-  matrix3x3_mult (rm, tm, tmp);
-  matrix3x3_copy (tmp, tm);
+	  cos_r = -1;
+	  sin_r = 0;
+	}
+      else if (rotation == 270)
+	{
+	  width = img->height;
+	  height = img->width;
 
-  img->width = width;
-  img->height = height;
-#endif
-}
+	  cos_r = 0;
+	  sin_r = -1;
+	}
+# if defined USE_CAIRO || defined HAVE_XRENDER || defined HAVE_NS
+      /* 1. Translate so (0, 0) is in the center of the image.  */
+      matrix3x3 t
+	= { [0][0] = 1,
+				      [1][1] = 1,
+	    [2][0] = img->width * .5, [2][1] = img->height * .5, [2][2] = 1 };
+      matrix3x3 tmp2;
+      matrix3x3_mult (t, tmp, tmp2);
+
+      /* 2. Rotate.  */
+      matrix3x3 rot = { [0][0] = cos_r, [0][1] = -sin_r,
+			[1][0] = sin_r, [1][1] = cos_r,
+							 [2][2] = 1 };
+      matrix3x3 tmp3;
+      matrix3x3_mult (rot, tmp2, tmp3);
+
+      /* 3. Translate back.  */
+      t[2][0] = width * -.5;
+      t[2][1] = height * -.5;
+      matrix3x3_mult (t, tmp3, matrix);
+# elif defined HAVE_NTGUI
+      /* 1. Translate so (0, 0) is in the center of the image.  */
+      matrix3x3 t
+	= { [0][0] = 1,
+				       [1][1] = 1,
+	    [2][0] = -img->width * .5, [2][1] = -img->height * .5, [2][2] = 1 };
+      matrix3x3 tmp2;
+      matrix3x3_mult (tmp, t, tmp2);
+
+      /* 2. Rotate.  */
+      matrix3x3 rot = { [0][0] = cos_r,  [0][1] = sin_r,
+			[1][0] = -sin_r, [1][1] = cos_r,
+							  [2][2] = 1 };
+      matrix3x3 tmp3;
+      matrix3x3_mult (tmp2, rot, tmp3);
+
+      /* 3. Translate back.  */
+      t[2][0] = width * .5;
+      t[2][1] = height * .5;
+      matrix3x3_mult (tmp3, t, matrix);
+# endif
+      img->width = width;
+      img->height = height;
+    }
+  else
+    image_error ("Native image rotation supports only multiples of 90 degrees");
 
-static void
-image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)
-{
-  /* TODO: Add MS Windows support.  */
-#ifdef HAVE_NATIVE_TRANSFORMS
 # if defined (HAVE_NS)
   /* Under NS the transform is applied to the drawing surface at
      drawing time, so store it for later.  */
@@ -2317,10 +2234,19 @@ image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)
 			       0, 0);
       XRenderSetPictureTransform (FRAME_X_DISPLAY (f), img->picture, &tmat);
     }
+# elif defined HAVE_NTGUI
+  /* Store the transform matrix for application at draw time.  */
+  img->xform.eM11 = matrix[0][0];
+  img->xform.eM12 = matrix[0][1];
+  img->xform.eM21 = matrix[1][0];
+  img->xform.eM22 = matrix[1][1];
+  img->xform.eDx  = matrix[2][0];
+  img->xform.eDy  = matrix[2][1];
 # endif
-#endif
 }
 
+#endif /* HAVE_IMAGEMAGICK || HAVE_NATIVE_TRANSFORMS */
+
 /* Return the id of image with Lisp specification SPEC on frame F.
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
 
@@ -2378,9 +2304,6 @@ lookup_image (struct frame *f, Lisp_Object spec)
 
 #ifdef HAVE_NATIVE_TRANSFORMS
           matrix3x3 transform_matrix = { [0][0] = 1, [1][1] = 1, [2][2] = 1 };
-          image_set_size (img, transform_matrix);
-          image_set_crop (img, transform_matrix);
-          image_set_rotation (img, transform_matrix);
           image_set_transform (f, img, transform_matrix);
 #endif
 
@@ -10010,19 +9933,38 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
 
 DEFUN ("image-transforms-p", Fimage_transforms_p, Simage_transforms_p, 0, 1, 0,
        doc: /* Test whether FRAME supports image transformation.
-Return t if FRAME supports native transforms, nil otherwise.  */)
+Return list of capabilities if FRAME supports native transforms, nil otherwise.
+FRAME defaults to the selected frame.
+The list of capabilities can include one or more of the following:
+
+ - the symbol `scale' if FRAME supports image scaling
+ - the symbol `rotate' if FRAME supports image rotation by arbitrary angles
+ - the symbol `rotate90' if FRAME supports image rotation only by angles
+    that are integral multiples of 90 degrees
+ - the symbol `crop' if FRAME supports image cropping.  */)
      (Lisp_Object frame)
 {
-#if defined (USE_CAIRO) || defined (HAVE_NS) || defined (HAVE_NTGUI)
-  return Qt;
-#elif defined (HAVE_X_WINDOWS) && defined (HAVE_XRENDER)
-  int event_basep, error_basep;
-
-  if (XRenderQueryExtension
-      (FRAME_X_DISPLAY (decode_window_system_frame (frame)),
-       &event_basep, &error_basep))
-    return Qt;
+  struct frame *f = decode_live_frame (frame);
+  if (FRAME_WINDOW_P (f))
+    {
+#ifdef HAVE_NATIVE_TRANSFORMS
+# if defined HAVE_IMAGEMAGICK
+      return list3 (Qscale, Qrotate, Qcrop);
+# elif defined (USE_CAIRO) || defined (HAVE_NS)
+      return list2 (Qscale, Qrotate90);
+# elif defined (HAVE_NTGUI)
+      return (w32_image_rotations_p ()
+	      ? list2 (Qscale, Qrotate90)
+	      : list1 (Qscale));
+# elif defined (HAVE_X_WINDOWS) && defined (HAVE_XRENDER)
+      int event_basep, error_basep;
+
+      if (XRenderQueryExtension (FRAME_X_DISPLAY (f),
+				 &event_basep, &error_basep))
+	return list2 (Qscale, Qrotate90);
+# endif
 #endif
+    }
 
   return Qnil;
 }
@@ -10168,6 +10110,14 @@ syms_of_image (void)
   DEFSYM (Qpostscript, "postscript");
   DEFSYM (QCmax_width, ":max-width");
   DEFSYM (QCmax_height, ":max-height");
+
+#ifdef HAVE_NATIVE_TRANSFORMS
+  DEFSYM (Qscale, "scale");
+  DEFSYM (Qrotate, "rotate");
+  DEFSYM (Qrotate90, "rotate90");
+  DEFSYM (Qcrop, "crop");
+#endif
+
 #ifdef HAVE_GHOSTSCRIPT
   add_image_type (Qpostscript);
   DEFSYM (QCloader, ":loader");
diff --git a/src/w32term.c b/src/w32term.c
index 5726124..f392d31 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -117,6 +117,10 @@ #define GET_WHEEL_DELTA_WPARAM(wparam) ((short)HIWORD (wparam))
 /* Dynamic linking to SetLayeredWindowAttribute (only since 2000).  */
 BOOL (WINAPI *pfnSetLayeredWindowAttributes) (HWND, COLORREF, BYTE, DWORD);
 
+/* PlgBlt is available since Windows 2000.  */
+BOOL (WINAPI *pfnPlgBlt) (HDC, const POINT *, HDC, int, int, int, int, HBITMAP, int, int);
+
+
 #ifndef LWA_ALPHA
 #define LWA_ALPHA 0x02
 #endif
@@ -1753,6 +1757,26 @@ w32_draw_glyph_string_box (struct glyph_string *s)
     }
 }
 
+bool
+w32_image_rotations_p (void)
+{
+  return pfnPlgBlt != NULL;
+}
+
+static POINT
+transform (int x0, int y0, int x, int y, XFORM *xform)
+{
+  POINT pt;
+
+  /* See https://docs.microsoft.com/en-us/windows/desktop/api/Wingdi/nf-wingdi-setworldtransform */
+  pt.x =
+    x0 + (x - x0) * xform->eM11 + (y - y0) * xform->eM21 + xform->eDx + 0.5f;
+  pt.y =
+    y0 + (x - x0) * xform->eM12 + (y - y0) * xform->eM22 + xform->eDy + 0.5f;
+
+  return pt;
+}
+
 
 /* Draw foreground of image glyph string S.  */
 
@@ -1798,20 +1822,31 @@ w32_draw_image_foreground (struct glyph_string *s)
 	}
       else
 	{
-	  DebPrint (("w32_draw_image_foreground: GetObject failed!\n"));
+	  DebPrint (("w32_draw_image_foreground: GetObject(pixmap) failed!\n"));
 	  orig_width = s->slice.width;
 	  orig_height = s->slice.height;
 	}
 
       double w_factor = 1.0, h_factor = 1.0;
-      bool scaled = false;
+      bool scaled = false, need_xform = false;
       int orig_slice_width  = s->slice.width,
 	  orig_slice_height = s->slice.height;
       int orig_slice_x = s->slice.x, orig_slice_y = s->slice.y;
-      /* For scaled images we need to restore the original slice's
-	 dimensions and origin coordinates, from before the scaling.  */
-      if (s->img->width != orig_width || s->img->height != orig_height)
+      POINT corner[3];
+      if ((s->img->xform.eM12 != 0 || s->img->xform.eM21 != 0
+	   || s->img->xform.eDx != 0 || s->img->xform.eDy != 0)
+	  /* PlgBlt is not available on Windows 9X.  */
+	  && pfnPlgBlt)
 	{
+	  need_xform = true;
+	  corner[0] = transform (x, y, x, y, &s->img->xform);
+	  corner[1] = transform (x, y, x + orig_width, y, &s->img->xform);
+	  corner[2] = transform (x, y, x, y + orig_height, &s->img->xform);
+	}
+      else if (s->img->width != orig_width || s->img->height != orig_height)
+	{
+	  /* For scaled images we need to restore the original slice's
+	     dimensions and origin coordinates, from before the scaling.  */
 	  scaled = true;
 	  w_factor = (double) orig_width  / (double) s->img->width;
 	  h_factor = (double) orig_height / (double) s->img->height;
@@ -1828,7 +1863,15 @@ w32_draw_image_foreground (struct glyph_string *s)
 
 	  SetTextColor (s->hdc, RGB (255, 255, 255));
 	  SetBkColor (s->hdc, RGB (0, 0, 0));
-	  if (!scaled)
+	  if (need_xform)
+	    {
+	      if (!pfnPlgBlt (s->hdc, corner, compat_hdc,
+			      s->slice.x, s->slice.y,
+			      orig_width, orig_height,
+			      s->img->mask, s->slice.x, s->slice.y))
+		DebPrint (("PlgBlt failed!"));
+	    }
+	  else if (!scaled)
 	    {
 	      BitBlt (s->hdc, x, y, s->slice.width, s->slice.height,
 		      compat_hdc, s->slice.x, s->slice.y, SRCINVERT);
@@ -1865,7 +1908,14 @@ w32_draw_image_foreground (struct glyph_string *s)
 	{
 	  SetTextColor (s->hdc, s->gc->foreground);
 	  SetBkColor (s->hdc, s->gc->background);
-	  if (!scaled)
+	  if (need_xform)
+	    {
+	      if (!pfnPlgBlt (s->hdc, corner, compat_hdc,
+			      s->slice.x, s->slice.y,
+			      orig_width, orig_height, NULL, 0, 0))
+		DebPrint (("PlgBlt failed!"));
+	    }
+	  else if (!scaled)
 	    BitBlt (s->hdc, x, y, s->slice.width, s->slice.height,
 		    compat_hdc, s->slice.x, s->slice.y, SRCCOPY);
 	  else
@@ -7354,6 +7404,11 @@ #define LOAD_PROC(lib, fn) pfn##fn = (void *) GetProcAddress (lib, #fn)
 
     LOAD_PROC (user_lib, SetLayeredWindowAttributes);
 
+    /* PlgBlt is not available on Windows 9X.  */
+    HMODULE hgdi = LoadLibrary ("gdi32.dll");
+    if (hgdi)
+      LOAD_PROC (hgdi, PlgBlt);
+
 #undef LOAD_PROC
 
     /* Ensure scrollbar handles are at least 5 pixels.  */
diff --git a/src/w32term.h b/src/w32term.h
index 729e8d0..6133e10 100644
--- a/src/w32term.h
+++ b/src/w32term.h
@@ -743,6 +743,8 @@ #define FILE_NOTIFICATIONS_SIZE 16384
 extern void w32_initialize_display_info (Lisp_Object);
 extern void initialize_w32_display (struct terminal *, int *, int *);
 
+extern bool w32_image_rotations_p (void);
+
 #ifdef WINDOWSNT
 /* Keyboard hooks.  */
 extern void setup_w32_kbdhook (void);
diff --git a/test/manual/image-transforms-tests.el b/test/manual/image-transforms-tests.el
index d601b93..e8b301e 100644
--- a/test/manual/image-transforms-tests.el
+++ b/test/manual/image-transforms-tests.el
@@ -67,7 +67,7 @@ test-cropping
                          <rect x='0' y='0' width='10' height='10'
                                style='fill:none;stroke-width:1;stroke:#000'/>
                        </svg>"))
-    (insert-header "Test Crop: cropping an image")
+    (insert-header "Test Crop: cropping an image (only works with ImageMagick)")
     (insert-test "all params" top-left image '(:crop (10 10 0 0)))
     (insert-test "width/height only" middle image '(:crop (10 10)))
     (insert-test "negative x y" middle image '(:crop (10 10 -10 -10)))

Re: Image transformations

[Alan Third]

On Thu, Jun 27, 2019 at 04:59:06PM +0300, Eli Zaretskii wrote:
> What I have now is below.  This doesn't yet include documentation
> changes.
> 
> I could only test this on MS-Windows, but I hope I didn't goof on
> other platforms.  I left the NS code to use the same matrix as
> XRender/Cairo, as I couldn't test the result of letting it use the
> inverted matrix like MS-Windows does.  If using that matrix works for
> NS, the inversion in nsimage.m could be removed.

I tested before and after I moved the HAVE_NS checks and removed the
inversion from nsimage.m and it all works, so I’d say you’ve done a
good job. We may as well use the same calculations for W32 and NS.

> There's a FIXME in image_set_rotation, please tell what you think
> about it.

I think you’re probably right that we should throw an error there.

> Please also comment on image-transforms-p.  Maybe we should return
> both 'rotate' and 'rotate90' in the list when ImageMagick is
> available?

Yes, we probably should. My only concern is that there is no simple
way to tell whether you should be using ‘:type imagemagick’ or not.
There’s no automatic fallback between types. I know we want fallback
in principal, but I’d imagined it done at the lisp level, and this
function doesn’t help too much.

This is why I’d been thinking about the capabilities being arranged
around image backends.

> +image_set_rotation (struct image *img, double *rotation)

Should we rename this to compute_image_rotation to mirror
compute_image_size?

> +image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)

I don’t think we need to pass this a matrix any more?

-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Fri, 28 Jun 2019 19:36:04 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I tested before and after I moved the HAVE_NS checks and removed the
> inversion from nsimage.m and it all works, so I’d say you’ve done a
> good job. We may as well use the same calculations for W32 and NS.

Great, thanks for testing.

> > There's a FIXME in image_set_rotation, please tell what you think
> > about it.
> 
> I think you’re probably right that we should throw an error there.

OK, will add that.

> > Please also comment on image-transforms-p.  Maybe we should return
> > both 'rotate' and 'rotate90' in the list when ImageMagick is
> > available?
> 
> Yes, we probably should.

Will add.

> My only concern is that there is no simple way to tell whether you
> should be using ‘:type imagemagick’ or not.  There’s no automatic
> fallback between types. I know we want fallback in principal, but
> I’d imagined it done at the lisp level, and this function doesn’t
> help too much.

This function is about the capabilities of a frame.  AFAIK, in an
Emacs built with ImageMagick support we will use ImageMagick for
everything, unless the user somehow forces us not to, isn't that so?
If so, the function is correct disregarding the image library in use,
since if the caller wants a specific image library, that caller will
have to figure out on their own what transformations are available.

> > +image_set_rotation (struct image *img, double *rotation)
> 
> Should we rename this to compute_image_rotation to mirror
> compute_image_size?
> 
> > +image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)
> 
> I don’t think we need to pass this a matrix any more?

Makes sense on both counts, will do.

Thanks.

Re: Image transformations

[Eli Zaretskii]

> Date: Fri, 28 Jun 2019 22:50:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Date: Fri, 28 Jun 2019 19:36:04 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > I tested before and after I moved the HAVE_NS checks and removed the
> > inversion from nsimage.m and it all works, so I’d say you’ve done a
> > good job. We may as well use the same calculations for W32 and NS.
> 
> Great, thanks for testing.
> 
> > > There's a FIXME in image_set_rotation, please tell what you think
> > > about it.
> > 
> > I think you’re probably right that we should throw an error there.
> 
> OK, will add that.
> 
> > > Please also comment on image-transforms-p.  Maybe we should return
> > > both 'rotate' and 'rotate90' in the list when ImageMagick is
> > > available?
> > 
> > Yes, we probably should.
> 
> Will add.
> 
> > My only concern is that there is no simple way to tell whether you
> > should be using ‘:type imagemagick’ or not.  There’s no automatic
> > fallback between types. I know we want fallback in principal, but
> > I’d imagined it done at the lisp level, and this function doesn’t
> > help too much.
> 
> This function is about the capabilities of a frame.  AFAIK, in an
> Emacs built with ImageMagick support we will use ImageMagick for
> everything, unless the user somehow forces us not to, isn't that so?
> If so, the function is correct disregarding the image library in use,
> since if the caller wants a specific image library, that caller will
> have to figure out on their own what transformations are available.
> 
> > > +image_set_rotation (struct image *img, double *rotation)
> > 
> > Should we rename this to compute_image_rotation to mirror
> > compute_image_size?
> > 
> > > +image_set_transform (struct frame *f, struct image *img, matrix3x3 matrix)
> > 
> > I don’t think we need to pass this a matrix any more?
> 
> Makes sense on both counts, will do.

All done and pushed to the master branch.

Thanks for the feedback.

Re: Image transformations

[Alan Third]

On Fri, Jun 28, 2019 at 10:50:52PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 28 Jun 2019 19:36:04 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > My only concern is that there is no simple way to tell whether you
> > should be using ‘:type imagemagick’ or not.  There’s no automatic
> > fallback between types. I know we want fallback in principal, but
> > I’d imagined it done at the lisp level, and this function doesn’t
> > help too much.
> 
> This function is about the capabilities of a frame.  AFAIK, in an
> Emacs built with ImageMagick support we will use ImageMagick for
> everything, unless the user somehow forces us not to, isn't that so?
> If so, the function is correct disregarding the image library in use,
> since if the caller wants a specific image library, that caller will
> have to figure out on their own what transformations are available.

If I were to do:

  (insert-image (create-image "image.png"))

Emacs will use libpng, whether or not ImageMagick is available. I
would have to force ImageMagick use. You may be thinking of
image-mode, which inserts ‘:type imagemagick’ if you rotate or scale
an image using the key bindings (+, -, r).

If you want to change to defaulting to ImageMagick where available I’m
sure we can.
-- 
Alan Third

Re: Image transformations

[Alan Third]

On Sat, Jun 29, 2019 at 02:55:56PM +0300, Eli Zaretskii wrote:
> 
> All done and pushed to the master branch.

Thank you.
-- 
Alan Third

Re: Image transformations

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> If you want to change to defaulting to ImageMagick where available I’m
> sure we can.

I've resorted to using a function like this in every package that
inserts an image:

(defun foo--image-type ()
  (if (or (and (fboundp 'image-transforms-p)
	       (image-transforms-p))
	  (not (fboundp 'imagemagick-types)))
      nil
    'imagemagick))

Which is pretty annoying.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Stefan Monnier]

> If I were to do:
>
>   (insert-image (create-image "image.png"))
>
> Emacs will use libpng, whether or not ImageMagick is available.

I consider this to be good.
Is there a reason why someone would want to use imagemagick instead?


        Stefan

Re: Image transformations

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Sat, Jun 29, 2019 at 09:53:53PM +0200, Lars Ingebrigtsen wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> > If you want to change to defaulting to ImageMagick where available I’m
> > sure we can.
> 
> I've resorted to using a function like this in every package that
> inserts an image:
> 
> (defun foo--image-type ()
>   (if (or (and (fboundp 'image-transforms-p)
> 	       (image-transforms-p))
> 	  (not (fboundp 'imagemagick-types)))
>       nil
>     'imagemagick))
> 
> Which is pretty annoying.

Something like the attached might help.

Although if someone included ImageMagick to support some obscure image
type it might result in slower image loads than native scaling for png
and jpeg, and so on.

I don’t know if we care much about that. Personally I’ve not noticed
any great speed increase, but anecdotally I’ve heard that native
transforms are far faster.

If it does turn out to be an issue then it should be quite easy to
just use ImageMagick to load the image, then use native transforms
when available.

-- 
Alan Third

[-- Attachment #2: 0001-Default-to-ImageMagick-when-transforms-are-requested.patch --]
[-- Type: text/plain, Size: 1820 bytes --]

From dfc19db48f85538b0d60cd0b1e0787811f8daca2 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sun, 30 Jun 2019 15:24:46 +0100
Subject: [PATCH] Default to ImageMagick when transforms are requested

* lisp/image.el (create-image): Default to ImageMagick when transforms
are requested.
---
 lisp/image.el | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lisp/image.el b/lisp/image.el
index b58b1dc954..70c988857c 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -436,13 +436,24 @@ create-image
 Image file names that are not absolute are searched for in the
 \"images\" sub-directory of `data-directory' and
 `x-bitmap-file-path' (in that order)."
+  (when (not (plist-get props :scale))
+    (let ((scale (image-compute-scaling-factor image-scaling-factor)))
+      (when (/= 1 scale) (setq props (append (list :scale scale) props)))))
+  ;; Default to ImageMagick if a transform is requested.
+  (if (and (not type)
+           (fboundp 'imagemagick-types)
+           (or (plist-get props :scale)
+               (plist-get props :width)
+               (plist-get props :height)
+               (plist-get props :max-width)
+               (plist-get props :max-height)
+               (plist-get props :rotation)
+               (plist-get props :crop)))
+      (setq type 'imagemagick))
   ;; It is x_find_image_file in image.c that sets the search path.
   (setq type (image-type file-or-data type data-p))
   (when (image-type-available-p type)
     (append (list 'image :type type (if data-p :data :file) file-or-data)
-            (and (not (plist-get props :scale))
-                 (list :scale
-                       (image-compute-scaling-factor image-scaling-factor)))
 	    props)))
 
 (defun image--set-property (image property value)
-- 
2.21.0

Re: Image transformations

[Eli Zaretskii]

> Date: Sat, 29 Jun 2019 20:49:59 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> > This function is about the capabilities of a frame.  AFAIK, in an
> > Emacs built with ImageMagick support we will use ImageMagick for
> > everything, unless the user somehow forces us not to, isn't that so?
> > If so, the function is correct disregarding the image library in use,
> > since if the caller wants a specific image library, that caller will
> > have to figure out on their own what transformations are available.
> 
> If I were to do:
> 
>   (insert-image (create-image "image.png"))
> 
> Emacs will use libpng, whether or not ImageMagick is available. I
> would have to force ImageMagick use. You may be thinking of
> image-mode, which inserts ‘:type imagemagick’ if you rotate or scale
> an image using the key bindings (+, -, r).

So you are saying that a Lisp program needs to explicitly request
ImageMagick if it wants to, say, rotate the image by 45 degrees?  If
so, I guess either the ImageMagick capabilities should not be part of
the list we return, or the function should accept an additional
argument which is the image type.  The latter sounds like entering the
area of diminishing returns, since if the caller needs to specify
imagemagick as the type, then the caller already knows what will be
returned as the value, right?

So on balance, I think we should return a list of either 2 values or
(on Windows 9X) 1 value, and that's it.  WDYT?

Re: Image transformations

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> +  ;; Default to ImageMagick if a transform is requested.
> +  (if (and (not type)
> +           (fboundp 'imagemagick-types)
> +           (or (plist-get props :scale)
> +               (plist-get props :width)
> +               (plist-get props :height)
> +               (plist-get props :max-width)
> +               (plist-get props :max-height)
> +               (plist-get props :rotation)
> +               (plist-get props :crop)))
> +      (setq type 'imagemagick))

Well, we don't want 'imagemagick if we have native transforms -- then we
want nil so that we get the native transforms.

We only want imagemagick if we don't have native transforms, or we have
a scaling thing and we have imagemagick support.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Alan Third]

On Sun, Jun 30, 2019 at 06:12:11PM +0300, Eli Zaretskii wrote:
> So you are saying that a Lisp program needs to explicitly request
> ImageMagick if it wants to, say, rotate the image by 45 degrees?

Yes.

> If so, I guess either the ImageMagick capabilities should not be
> part of the list we return, or the function should accept an
> additional argument which is the image type. The latter sounds like
> entering the area of diminishing returns, since if the caller needs
> to specify imagemagick as the type, then the caller already knows
> what will be returned as the value, right?

Yes.

> So on balance, I think we should return a list of either 2 values or
> (on Windows 9X) 1 value, and that's it.  WDYT?

That sounds good to me. We can then provide a fallback to ImageMagick
within create-image, as we’ll be able to tell when native transforms
aren’t available.

-- 
Alan Third

Re: Image transformations

[Eli Zaretskii]

> Date: Sun, 30 Jun 2019 20:10:03 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> On Sun, Jun 30, 2019 at 06:12:11PM +0300, Eli Zaretskii wrote:
> > So you are saying that a Lisp program needs to explicitly request
> > ImageMagick if it wants to, say, rotate the image by 45 degrees?
> 
> Yes.
> 
> > If so, I guess either the ImageMagick capabilities should not be
> > part of the list we return, or the function should accept an
> > additional argument which is the image type. The latter sounds like
> > entering the area of diminishing returns, since if the caller needs
> > to specify imagemagick as the type, then the caller already knows
> > what will be returned as the value, right?
> 
> Yes.
> 
> > So on balance, I think we should return a list of either 2 values or
> > (on Windows 9X) 1 value, and that's it.  WDYT?
> 
> That sounds good to me. We can then provide a fallback to ImageMagick
> within create-image, as we’ll be able to tell when native transforms
> aren’t available.

OK, I've now done so.  Thanks for the feedback and the expolanations.

Re: Image transformations

[Lars Ingebrigtsen]

Nothing further was done here?  So just to recap for new viewers who
have recently joined us:

Currently, anybody who wants to insert an image in Emacs has to say the
equivalent of

(create-image data
	      (if (or (and (fboundp 'image-transforms-p)
			   (image-transforms-p))
		      (not (fboundp 'imagemagick-types)))
		  nil
		'imagemagick))

to be somewhat compatible over current Emacses (because we almost always
want to scale images).  That's rather a mouthful, so Alan's suggestion
was to just do this in `create-image', but better.

I took that idea and worked it a bit more so that we never use
imagemagick unless we absolutely need to, to satisfy the user's wishes.

Does the following look OK to everybody?

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 3c91092906..d9ac2e0411 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5757,10 +5757,14 @@ Defining Images
 a string containing the image data; @var{data-p} should be @code{nil}
 for the former case, non-@code{nil} for the latter case.
 
-The optional argument @var{type} is a symbol specifying the image type.
-If @var{type} is omitted or @code{nil}, @code{create-image} tries to
-determine the image type from the file's first few bytes, or else
-from the file's name.
+The optional argument @var{type} is a symbol specifying the image
+type.  If @var{type} is omitted or @code{nil}, @code{create-image}
+tries to determine the image type from the file's first few bytes, or
+else from the file's name.  If @var{props} specify an image transform
+(for instance, @samp{:scale}, @samp{:max-height} or @samp{rotate}),
+and @var{type} is @code{nil}, and Emacs doesn't have the requisite
+native support for that transform, and Emacs is built with ImageMagick
+support, then @var{type} will default to @var{imagemagick} instead.
 
 The remaining arguments, @var{props}, specify additional image
 properties---for example,
diff --git a/etc/NEWS b/etc/NEWS
index f47cf071bb..1ce7bba786 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2344,6 +2344,12 @@ The new function 'image-transforms-p' can be used to test whether any
 given frame supports these capabilities.
 
 +++
+** If `create-image' is called with a nil TYPE parameter, and a transform
+is specified (:scale, :max-height, :rotate, etc), and Emacs doesn't
+support this natively, and Emacs is built with ImageMagick support,
+then TYPE will default to `imagemagick'.
+
++++
 ** '(locale-info 'paper)' now returns the paper size on systems that support it.
 This is currently supported on GNUish hosts and on modern versions of
 MS-Windows.
diff --git a/lisp/image.el b/lisp/image.el
index c3e28655c3..379fda8a51 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -421,6 +421,11 @@ create-image
 of image data.  If that doesn't work, and FILE-OR-DATA is a file name,
 use its file extension as image type.
 
+If PROPS contain image transforms (like :max-height, :scale,
+:rotate, etc), and we don't support these transforms natively in
+Emacs, and TYPE is nil, and we have ImageMagick support in Emacs,
+then TYPE will default to `imagemagick'.
+
 Optional DATA-P non-nil means FILE-OR-DATA is a string containing image data.
 
 Optional PROPS are additional image attributes to assign to the image,
@@ -436,13 +441,35 @@ create-image
 Image file names that are not absolute are searched for in the
 \"images\" sub-directory of `data-directory' and
 `x-bitmap-file-path' (in that order)."
+  (unless (plist-get props :scale)
+    (let ((scale (image-compute-scaling-factor image-scaling-factor)))
+      (unless (= scale 1)
+        (setq props (append (list :scale scale) props)))))
+  ;; Default to ImageMagick if a transform is requested, and we do not
+  ;; have the appropriate native transform, and the type isn't
+  ;; specified.
+  (if (and (not type)
+           (fboundp 'imagemagick-types)
+           ;; All these props require scaling.
+           (or (and (or (plist-get props :scale)
+                        (plist-get props :width)
+                        (plist-get props :height)
+                        (plist-get props :max-width)
+                        (plist-get props :max-height))
+                    (not (memq 'scale (image-transforms-p))))
+               ;; We can have rotation by 90/180/270 degrees supported
+               ;; natively...
+               (and (plist-get props :rotation)
+                    (if (zerop (mod (plist-get props :rotation) 90))
+                        (not (memq 'rotate90 (image-transforms-p)))
+                      t))
+               ;; We don't currently have native cropping.
+               (plist-get props :crop)))
+      (setq type 'imagemagick))
   ;; It is x_find_image_file in image.c that sets the search path.
   (setq type (image-type file-or-data type data-p))
   (when (image-type-available-p type)
     (append (list 'image :type type (if data-p :data :file) file-or-data)
-            (and (not (plist-get props :scale))
-                 (list :scale
-                       (image-compute-scaling-factor image-scaling-factor)))
 	    props)))
 
 (defun image--set-property (image property value)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 25 Jul 2019 21:40:12 +0200
> 
> Currently, anybody who wants to insert an image in Emacs has to say the
> equivalent of
> 
> (create-image data
> 	      (if (or (and (fboundp 'image-transforms-p)
> 			   (image-transforms-p))
> 		      (not (fboundp 'imagemagick-types)))
> 		  nil
> 		'imagemagick))
> 
> to be somewhat compatible over current Emacses (because we almost always
> want to scale images).  That's rather a mouthful, so Alan's suggestion
> was to just do this in `create-image', but better.
> 
> I took that idea and worked it a bit more so that we never use
> imagemagick unless we absolutely need to, to satisfy the user's wishes.
> 
> Does the following look OK to everybody?

I don't see anything wrong with that, but I don't see how it would
help in older Emacsen, either.  A simpler alternative for the above
would be to look at the Emacs version, and then you don't need most or
all of the other tests, right?

Re: Image transformations

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't see anything wrong with that, but I don't see how it would
> help in older Emacsen, either.

No, if you want to be able to insert images in both older Emacs versions
and Emacs 27+, you need that phrase in your `create-image' calls.  The
change is to avoid having that in Emacs 27+-only code that you expect to
work across all architectures (some of which have native image rescaling
and some that don't).  

> A simpler alternative for the above would be to look at the Emacs
> version, and then you don't need most or all of the other tests,
> right?

Or did I misunderstand and there are now no architectures without native
image scaling?  In that case, this change isn't necessary...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: alan@idiocy.org,  emacs-devel@gnu.org
> Date: Fri, 26 Jul 2019 08:46:24 +0200
> 
> > A simpler alternative for the above would be to look at the Emacs
> > version, and then you don't need most or all of the other tests,
> > right?
> 
> Or did I misunderstand and there are now no architectures without native
> image scaling?  In that case, this change isn't necessary...

If you test the version, the ImageMagick parts of the test are not
needed, you need only test image-transforms-p for Emacs 27+.

Re: Image transformations

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > A simpler alternative for the above would be to look at the Emacs
>> > version, and then you don't need most or all of the other tests,
>> > right?
>> 
>> Or did I misunderstand and there are now no architectures without native
>> image scaling?  In that case, this change isn't necessary...
>
> If you test the version, the ImageMagick parts of the test are not
> needed, you need only test image-transforms-p for Emacs 27+.

I'm not quite sure I follow you...  If there are platforms without
native image transforms, then we want to fall back on ImageMagick (if
it's available), even on Emacs 27+.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm not quite sure I follow you...  If there are platforms without
> native image transforms, then we want to fall back on ImageMagick (if
> it's available), even on Emacs 27+.

(And anyway, if the user says :rotate 91, then we want to do
ImageMagick, because we don't have a native transform for that yet.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: alan@idiocy.org,  emacs-devel@gnu.org
> Date: Fri, 26 Jul 2019 10:23:20 +0200
> 
> > If you test the version, the ImageMagick parts of the test are not
> > needed, you need only test image-transforms-p for Emacs 27+.
> 
> I'm not quite sure I follow you...  If there are platforms without
> native image transforms, then we want to fall back on ImageMagick (if
> it's available), even on Emacs 27+.

Yes.  But you don't have to do that in core, just because the image
type was not explicitly specified, do you?

Re: Image transformations

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: alan@idiocy.org,  emacs-devel@gnu.org
> Date: Fri, 26 Jul 2019 10:24:48 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > I'm not quite sure I follow you...  If there are platforms without
> > native image transforms, then we want to fall back on ImageMagick (if
> > it's available), even on Emacs 27+.
> 
> (And anyway, if the user says :rotate 91, then we want to do
> ImageMagick, because we don't have a native transform for that yet.)

We do?  I thought we wanted to _avoid_ ImageMagick at all costs, more
or less.

Re: Image transformations

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>> 
>> > I'm not quite sure I follow you...  If there are platforms without
>> > native image transforms, then we want to fall back on ImageMagick (if
>> > it's available), even on Emacs 27+.
>> 
>> (And anyway, if the user says :rotate 91, then we want to do
>> ImageMagick, because we don't have a native transform for that yet.)
>
> We do?  I thought we wanted to _avoid_ ImageMagick at all costs, more
> or less.

Not at all costs, surely?  We want to try to insert an image that does
what the user has specified, and that requires ImageMagick in some
cases.  (When Emacs has been built with that support, of course, which I
think anybody who uses Emacs to view images on platforms without native
image transforms will do.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: alan@idiocy.org,  emacs-devel@gnu.org
> Date: Fri, 26 Jul 2019 10:58:47 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> (And anyway, if the user says :rotate 91, then we want to do
> >> ImageMagick, because we don't have a native transform for that yet.)
> >
> > We do?  I thought we wanted to _avoid_ ImageMagick at all costs, more
> > or less.
> 
> Not at all costs, surely?

I meant what I said: at all costs.

For example, let's assume some Lisp says ":rotate 90.001" -- would you
automatically invoke ImageMagick, or would you rather error out (so
the user or the program get their act together and fix the round-off)?

We implemented native support for the transforms that we decided are
necessary for handling images in most situations, the only exception
AFAIU being PhotoShop-like applications whose very purpose is to
manipulate images.  And for the latter we expect the caller to
explicitly request ImageMagick.  So I don't think we want to fall back
to ImageMagick by default.

> We want to try to insert an image that does what the user has
> specified, and that requires ImageMagick in some cases.

The users who want this will have to request ImageMagick explicitly.
It should be a very infrequent situation when they need to do that,
unless we have somehow made a serious mistake in our decision.

> (When Emacs has been built with that support, of course, which I
> think anybody who uses Emacs to view images on platforms without
> native image transforms will do.)

There are currently no platforms without native images, except those
who only support text-mode frames.  OTOH, I believe many downstream
distros build with ImageMagick by default, and will do so for many
years to come.  And we want to avoid as much as we can the instability
that this introduces into Emacs.

Re: Image transformations

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> There are currently no platforms without native images, except those
> who only support text-mode frames. 

Oh, OK, then my assumption was wrong and the issue is moot.  (Assuming
you meant "native image transforms" when you said "native images".  :-))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Image transformations

[Stefan Monnier]

>> We do?  I thought we wanted to _avoid_ ImageMagick at all costs, more
>> or less.
> Not at all costs, surely?

Yes.  AFAIK ImageMagick is mostly unmaintained and comes with a lot of
bugs (those two aspects are related), some/many of them affecting
Emacs's stability or introducing security holes.

For this reason we decided it was important to develop "native image
transforms".  And if some features of the old imagemagick support aren't
available in the new code, then either we decide it's OK to drop support
for those (that's currently our position for non-90-rotations) or we
should add those features to the "native transforms" (which I suspect
is what will happen eventually for the non-90-rotations).


        Stefan