Linking to ImageMagick by default

Linking to ImageMagick by default

[Glenn Morris]

There seems to be a fair number of Emacs crashes that are due to
ImageMagick.

The next version of Red Hat Enterprise Linux will drop ImageMagick
(though I would expect it to be available from an "add-on" repository
like EPEL). In https://lwn.net/Articles/772315/ there is speculation
that this is due to the poor stability and security record of
ImageMagick. (AFAICS from the RHEL8 release notes, it is _not_ replaced
by GraphicsMagick, but rather nothing.)

Is it worth reconsidering whether Emacs should link to ImageMagick by
default? The pros are support for more formats and rescaling, the cons
are crashes.

(I don't have anything substantive to add, and the lwn discussion is
hardly rigorous.)

Re: Linking to ImageMagick by default

[Paul Eggert]

Glenn Morris wrote:
> Is it worth reconsidering whether Emacs should link to ImageMagick by
> default? The pros are support for more formats and rescaling, the cons
> are crashes.

My .emacs now has (setq imagemagick-types-inhibit t) due to security and 
stability concerns. Even so, I'd prefer it if 'configure' disabled ImageMagick 
by default because then I wouldn't have to worry about emacs -Q in the normal 
case (or about passing --without-imagemagick to 'configure' all the time).

It would be easy to change the default, and I submitted a patch proposing this 
as Bug#33587.

Re: Linking to ImageMagick by default

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Is it worth reconsidering whether Emacs should link to ImageMagick by
> default? The pros are support for more formats and rescaling, the cons
> are crashes.

Without image scaling, modes that display images become pretty
worthless.

I think the long term solution here is to implement image scaling in
Emacs (probably not very hard) and then drop ImageMagick-by-default
after that's been done.

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

Re: Linking to ImageMagick by default

[Stefan Monnier]

> I think the long term solution here is to implement image scaling in
> Emacs

Hasn't this wheel already been invented?


        Stefan

Re: Linking to ImageMagick by default

[Glenn Morris]

Lars Ingebrigtsen wrote:

> Without image scaling, modes that display images become pretty
> worthless.

I think that is an issue that will vary from person to person.

> I think the long term solution here is to implement image scaling in
> Emacs (probably not very hard) and then drop ImageMagick-by-default
> after that's been done.

Do you have an opinion of GraphicsMagick?
It claims to be more stable and secure than ImageMagick.
I guess that someone who knows the Emacs ImageMagick code could port it
to GraphicsMagick fairly easily. (I don't, and didn't, bug#14358).

Re: Linking to ImageMagick by default

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Lars Ingebrigtsen wrote:
>
>> Without image scaling, modes that display images become pretty
>> worthless.
>
> I think that is an issue that will vary from person to person.

I think the modes that display images are pretty much unusable for
anybody that uses them if you don't have image scaling.  :-)

But if you don't use them, you won't care.

> Do you have an opinion of GraphicsMagick?
> It claims to be more stable and secure than ImageMagick.
> I guess that someone who knows the Emacs ImageMagick code could port it
> to GraphicsMagick fairly easily. (I don't, and didn't, bug#14358).

No, sorry, I don't know.

ImageMagick stability seems to have gotten a bit better the last few
years (in my experience).  I haven't had an ImageMagick-related segfault
in months now, and back in, say, 2014, it was a weekly occurrence. 

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

Re: Linking to ImageMagick by default

[Daniel Pittman]

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

On Wed, Dec 5, 2018 at 12:26 PM Glenn Morris <rgm@gnu.org> wrote:

> Lars Ingebrigtsen wrote:
>
> > Without image scaling, modes that display images become pretty
> > worthless.
>
> I think that is an issue that will vary from person to person.
>

This is ... mostly true, I guess, but I'm certainly in the "scaling is a
requirement" bucket.


> > I think the long term solution here is to implement image scaling in
> > Emacs (probably not very hard) and then drop ImageMagick-by-default
> > after that's been done.
>

Image scaling is ... a big tent, and depending on your tolerance for
errors, and preference for which artifacts show up, can be reasonably
complex to implement.  For example, the default with ImageMagick is
adaptive; see
https://github.com/ImageMagick/ImageMagick/blob/master/MagickCore/resize.c#L2344

To get the current behaviour you would need to implement the
Mitchell-Netravali Cubic filter [1] and also the Lanczos 3-lobe Sinc-Sinc
filter; those are not necessarily the best choices.  For those, see the
discussion pages for ImageMagick on the subject:

* [1]
https://www.cs.utexas.edu/~fussell/courses/cs384g-fall2013/lectures/mitchell/Mitchell.pdf
* http://www.imagemagick.org/Usage/resize/#techniques
* http://www.imagemagick.org/Usage/filter/nicolas/

The last gives a good explanation of the trade-offs, as does the first.
Personally, I would much rather buy than build that sort of image filtering
logic. :)

Do you have an opinion of GraphicsMagick?

It claims to be more stable and secure than ImageMagick.
>

It isn't wildly better in terms of security than ImageMagick is; for
example, compare:
https://security-tracker.debian.org/tracker/source-package/imagemagick
https://security-tracker.debian.org/tracker/source-package/graphicsmagick

The most recommended "I want an imaging library with an extremely broad set
of features, supporting most image types" option seems to be libvips, which
provides similar features to ImageMagick, but has a better focus on
security – especially recently – and a less troubled history.  Remember
that popularity is a big driver of security vulnerability identification
though, and IM is "the" solution in many popular web languages:
https://security-tracker.debian.org/tracker/source-package/vips

I guess that someone who knows the Emacs ImageMagick code could port it
> to GraphicsMagick fairly easily. (I don't, and didn't, bug#14358).
>

Yes, that should be reasonably easy.  The compatibility of the API is
fairly good, in my past experience.

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

Re: Linking to ImageMagick by default

[Lars Ingebrigtsen]

Daniel Pittman <slippycheeze@google.com> writes:

> The last gives a good explanation of the trade-offs, as does the
> first.  Personally, I would much rather buy than build that sort of
> image filtering logic. :)

Fortunately, we don't have to invent anything here.  :-)  As you point
out, there's lots of excellent research into image scaling algorithms,
and the algorithms themselves aren't mind-bogglingly difficult to
implement.  I think.  At least that was my conclusion the last time I
looked at this.

And if we're really lucky, we can adapt code from other GNU projects,
but I haven't investigated.

But what I meant by "probably not very hard" wasn't that bit, but
fitting image scaling into the Emacs image machinery, which is
surprisingly convoluted.  But I went through all the code paths (the
layering here is "there are layers?") and it looked like it wouldn't
require that much to inject scaling into all the image formats we care
about.  (I think XPM was the biggest problem.)

Unless something's changed since...  2014?  I think that was when I
looked at it.

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

Re: Linking to ImageMagick by default

[joakim]

Glenn Morris <rgm@gnu.org> writes:

> Lars Ingebrigtsen wrote:
>
>> Without image scaling, modes that display images become pretty
>> worthless.
>
> I think that is an issue that will vary from person to person.
>
>> I think the long term solution here is to implement image scaling in
>> Emacs (probably not very hard) and then drop ImageMagick-by-default
>> after that's been done.
>
> Do you have an opinion of GraphicsMagick?
> It claims to be more stable and secure than ImageMagick.
> I guess that someone who knows the Emacs ImageMagick code could port it
> to GraphicsMagick fairly easily. (I don't, and didn't, bug#14358).
>

Porting to GraphicsMagick ought to be fairly straightforward.

Copy pasting a textbook image scaler into the emacs codebase ought also
be straightforward.

If this is done I would humbly suggest that the
ImageMagic/GraphicsMagick code is kept around as an alternative.

This is because Imagemagic has support for some less common file formats
that I use. I can of course keep a patch around or make a module.

As an aside I used the ImageMagick code in Emacs to preview about 130000
images that I scanned using Emacs. I remember it as being very
stable. But I guess times change.

-- 
Joakim Verona
joakim@verona.se

Re: Linking to ImageMagick by default

[Alan Third]

On Wed, Dec 05, 2018 at 02:30:34PM +0100, Lars Ingebrigtsen wrote:
> Glenn Morris <rgm@gnu.org> writes:
> 
> > Is it worth reconsidering whether Emacs should link to ImageMagick by
> > default? The pros are support for more formats and rescaling, the cons
> > are crashes.
> 
> Without image scaling, modes that display images become pretty
> worthless.
> 
> I think the long term solution here is to implement image scaling in
> Emacs (probably not very hard) and then drop ImageMagick-by-default
> after that's been done.

This may be naive, but is it possible to leave scaling to the GUI
toolkit? I know that when we draw images into the frame in NS we can
specify different sized rectangles for source and destination and the
toolkit handles the scaling for us. I think w32 is the same and
presumably X provides something similar.

-- 
Alan Third

Re: Linking to ImageMagick by default

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I think the long term solution here is to implement image scaling in
>> Emacs
>
> Hasn't this wheel already been invented?

But has anybody rolled that wheel into our garage?

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

Re: Linking to ImageMagick by default

[Alan Third]

On Wed, Dec 05, 2018 at 10:39:01PM +0000, Alan Third wrote:
> 
> This may be naive, but is it possible to leave scaling to the GUI
> toolkit? I know that when we draw images into the frame in NS we can
> specify different sized rectangles for source and destination and the
> toolkit handles the scaling for us. I think w32 is the same and
> presumably X provides something similar.

In case anyone cares, I looked into this and XRender would do the
trick. I had a look at how to go about it, but as usual with X stuff
it looks a fair bit more complicated than with NS (and Windows), and I
don’t have a good X environment to play about in.

It may be smarter to resize the images (with XRender) when the pixmaps
are being generated, rather than when they’re being displayed, as
otherwise we’d want to rewrite the image caching code.
-- 
Alan Third

Re: Linking to ImageMagick by default

[Paul Eggert]

Alan Third wrote:
> In case anyone cares, I looked into this and XRender would do the
> trick. I had a look at how to go about it, but as usual with X stuff
> it looks a fair bit more complicated than with NS (and Windows), and I
> don’t have a good X environment to play about in.

I have an X environment (and a Wayland one too); could you please post any code 
you wrote, if you think the idea is worth a try?

Re: Linking to ImageMagick by default

[Elias Mårtenson]

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

On Sun, 9 Dec 2018, 02:39 Alan Third <alan@idiocy.org wrote:

>
> In case anyone cares, I looked into this and XRender would do the
> trick. I had a look at how to go about it, but as usual with X stuff
> it looks a fair bit more complicated than with NS (and Windows), and I
> don’t have a good X environment to play about in.
>
> It may be smarter to resize the images (with XRender) when the pixmaps
> are being generated, rather than when they’re being displayed, as
> otherwise we’d want to rewrite the image caching code.
>

I implemented image scaling (and transformation) support in McCLIM using
Xrender and it was surprisingly simple.

The API is very poorly documented, but it's not very hard to use.

>

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

Re: Linking to ImageMagick by default

[Alan Third]

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

On Sat, Dec 08, 2018 at 01:24:13PM -0800, Paul Eggert wrote:
> Alan Third wrote:
> > In case anyone cares, I looked into this and XRender would do the
> > trick. I had a look at how to go about it, but as usual with X stuff
> > it looks a fair bit more complicated than with NS (and Windows), and I
> > don’t have a good X environment to play about in.
> 
> I have an X environment (and a Wayland one too); could you please post any
> code you wrote, if you think the idea is worth a try?

The attached appears to somewhat work (at least on my incredibly slow
atom laptop). It’s not really workable in its current form. It’s slow
because it has to load the image each time due to the way image
caching works. I think there are a number of other places we may need
to replace XCopyArea. It doesn’t handle the case where XRender isn’t
available.

Nor with animated gifs in X. They’re fine in NS... ¯\_(ツ)_/¯

It also doesn’t play nice with imagemagick, so make sure to build
--without-imagemagick.

If we want to go with this method I’ll have to rewrite some of the
image handling code. At the moment we create one ‘struct image’ per
lisp image spec, and store the image in the image cache, but this
means if the same image is displayed twice at two different sizes we
load it twice and store it twice. OTOH, it’s the resized version
that’s stored, so that might save memory if we assume people are
mostly shrinking images.

What I’ve done in the attached would work better if we only loaded the
image once and didn’t tie it to an image spec, so we’d only keep one
(full size) copy in memory, and just resize it on demand. I’m unsure
how we’d go about detangling the image data from the image spec.

It may be possible to use XRender (and NSImage resizing) when we load
the image, and therefore work in the same way we do at the moment.
-- 
Alan Third

[-- Attachment #2: 0001-Scaling-PoC.patch --]
[-- Type: text/plain, Size: 7953 bytes --]

From e1969beea284022ab721b452a43736b49cf74ce3 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 10 Dec 2018 19:43:08 +0000
Subject: [PATCH] Scaling PoC

---
 lisp/image.el    |  2 --
 src/dispextern.h |  7 +++++++
 src/image.c      | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/nsimage.m    |  2 +-
 src/nsterm.m     |  7 ++++---
 src/xterm.c      | 48 +++++++++++++++++++++++++++++++++++++--------
 6 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/lisp/image.el b/lisp/image.el
index 74a23046e9..8989fceefd 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -982,8 +982,6 @@ image--get-image
     image))
 
 (defun image--get-imagemagick-and-warn ()
-  (unless (or (fboundp 'imagemagick-types) (featurep 'ns))
-    (error "Cannot rescale images without ImageMagick support"))
   (let ((image (image--get-image)))
     (image-flush image)
     (when (fboundp 'imagemagick-types)
diff --git a/src/dispextern.h b/src/dispextern.h
index 4cd017603d..490cc10c6c 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2987,6 +2987,13 @@ struct image
   /* Width and height of the image.  */
   int width, height;
 
+  /* Width and height of the image data.  */
+  int actual_width, actual_height;
+
+  /* The scaling factor between the actual image data and the
+     output.  */
+  double scale;
+
   /* These values are used for the rectangles displayed for images
      that can't be loaded.  */
 #define DEFAULT_IMAGE_WIDTH 30
diff --git a/src/image.c b/src/image.c
index 633d66e7a7..8bda88a6ce 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1748,6 +1748,55 @@ postprocess_image (struct frame *f, struct image *img)
 }
 
 
+static void
+set_image_size (struct image *img, Lisp_Object spec)
+{
+  Lisp_Object value;
+  int width = INT_MAX, height = INT_MAX;
+  double scale = 1;
+
+  img->actual_width = img->width;
+  img->actual_height = img->height;
+
+  value = image_spec_value (spec, QCscale, NULL);
+  if (NUMBERP (value))
+    scale = XFLOATINT (value);
+
+  /* Calculate the maximum width and height.  */
+  value = image_spec_value (spec, QCmax_width, NULL);
+  if (FIXNATP (value))
+    width = XFIXNAT (value);
+
+  value = image_spec_value (spec, QCmax_height, NULL);
+  if (FIXNATP (value))
+    height = XFIXNAT (value);
+
+  value = image_spec_value (spec, QCwidth, NULL);
+  if (FIXNATP (value))
+    width = min (XFIXNAT (value) * scale, width);
+
+  value = image_spec_value (spec, QCheight, NULL);
+  if (FIXNATP (value))
+    height = min (XFIXNAT (value) * scale, height);
+
+  /* Calculate the final scaling factor.  */
+  if (img->width * scale > width)
+    scale = (double) img->width / width;
+
+  if (img->height * scale > height)
+    scale = (double) img->height / height;
+
+  /* Set the final sizes.  We always maintain the aspect ratio.  */
+  if (scale != 1)
+    {
+      img->width *= scale;
+      img->height *= scale;
+    }
+
+  img->scale = scale;
+}
+
+
 /* Return the id of image with Lisp specification SPEC on frame F.
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
 
@@ -1803,6 +1852,8 @@ lookup_image (struct frame *f, Lisp_Object spec)
 	  Lisp_Object ascent, margin, relief, bg;
 	  int relief_bound;
 
+          set_image_size (img, spec);
+
 	  ascent = image_spec_value (spec, QCascent, NULL);
 	  if (FIXNUMP (ascent))
 	    img->ascent = XFIXNAT (ascent);
diff --git a/src/nsimage.m b/src/nsimage.m
index 0ae1b88edd..cc3aceae69 100644
--- a/src/nsimage.m
+++ b/src/nsimage.m
@@ -126,7 +126,7 @@ Updated by Christian Limpach (chris@nice.ch)
       eImg = temp;
     }
 
-  [eImg setSizeFromSpec:XCDR (img->spec)];
+  //[eImg setSizeFromSpec:XCDR (img->spec)];
 
   size = [eImg size];
   img->width = size.width;
diff --git a/src/nsterm.m b/src/nsterm.m
index 6c285f0abb..db357a9623 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3828,6 +3828,7 @@ Function modeled after x_draw_glyph_string_box ().
   NSRect br;
   struct face *face;
   NSColor *tdCol;
+  double scale = s->img->scale;
 
   NSTRACE ("ns_dumpglyphs_image");
 
@@ -3877,9 +3878,9 @@ Function modeled after x_draw_glyph_string_box ().
     {
 #ifdef NS_IMPL_COCOA
       NSRect dr = NSMakeRect (x, y, s->slice.width, s->slice.height);
-      NSRect ir = NSMakeRect (s->slice.x,
-                              s->img->height - s->slice.y - s->slice.height,
-                              s->slice.width, s->slice.height);
+      NSRect ir = NSMakeRect (s->slice.x / scale,
+                              (s->img->height - s->slice.y - s->slice.height) / scale,
+                              s->slice.width / scale , s->slice.height / scale);
       [img drawInRect: dr
              fromRect: ir
              operation: NSCompositingOperationSourceOver
diff --git a/src/xterm.c b/src/xterm.c
index 3a7e31e712..f3d0af4177 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -136,6 +136,9 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/XKBlib.h>
 #endif
 
+#include <X11/extensions/Xrender.h>
+#include <X11/extensions/Xcomposite.h>
+
 /* Default to using XIM if available.  */
 #ifdef USE_XIM
 bool use_xim = true;
@@ -2970,6 +2973,39 @@ x_draw_glyph_string_box (struct glyph_string *s)
 }
 
 
+static void
+x_composite_image (struct glyph_string *s, Pixmap dest,
+                   int srcX, int srcY, int dstX, int dstY,
+                   int width, int height)
+{
+  Picture source, destination;
+  XRenderPictFormat *default_format, *source_format;
+  XRenderPictureAttributes source_attr, dest_attr;
+
+  double scale = s->img->scale;
+  XTransform tmat = {{{XDoubleToFixed (1), XDoubleToFixed (0), XDoubleToFixed (0)},
+                      {XDoubleToFixed (0), XDoubleToFixed (1), XDoubleToFixed (0)},
+                      {XDoubleToFixed (0), XDoubleToFixed (0), XDoubleToFixed (scale)}}};
+
+  default_format = XRenderFindVisualFormat (s->display, DefaultVisual (s->display, 0));
+  source_format = XRenderFindStandardFormat (s->display, PictStandardRGB24);
+
+  source = XRenderCreatePicture (s->display, s->img->pixmap, source_format, 0, &source_attr);
+  destination = XRenderCreatePicture (s->display, dest, default_format, 0, &dest_attr);
+
+  XRenderSetPictureTransform (s->display, source, &tmat);
+
+  XRenderComposite (s->display, PictOpOver, source, None, destination,
+                    srcX*scale, srcY*scale,
+                    0, 0,
+                    dstX, dstY,
+                    width, height);
+
+  XRenderFreePicture (s->display, source);
+  XRenderFreePicture (s->display, destination);
+}
+
+
 /* Draw foreground of image glyph string S.  */
 
 static void
@@ -3018,10 +3054,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 	}
       else
 	{
@@ -3033,10 +3067,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 
 	  /* When the image has a mask, we can expect that at
 	     least part of a mouse highlight or a block cursor will
-- 
2.19.1

Re: Linking to ImageMagick by default

[Alan Third]

On Mon, Dec 10, 2018 at 10:09:44PM +0000, Alan Third wrote:
> 
> If we want to go with this method I’ll have to rewrite some of the
> image handling code. At the moment we create one ‘struct image’ per
> lisp image spec, and store the image in the image cache, but this
> means if the same image is displayed twice at two different sizes we
> load it twice and store it twice. OTOH, it’s the resized version
> that’s stored, so that might save memory if we assume people are
> mostly shrinking images.
> 
> What I’ve done in the attached would work better if we only loaded the
> image once and didn’t tie it to an image spec, so we’d only keep one
> (full size) copy in memory, and just resize it on demand. I’m unsure
> how we’d go about detangling the image data from the image spec.

I’ve continued working on this and have something mostly working, but
I’m rather stuck with the image cache.

The image spec is a lisp list that looks something like this:

    '(image :file "image.png" :format png :height 10 :width 20)

The image cache code stores this directly in the image struct and uses
it to find matching images using Fequal.

I want to strip out the properties that I’m going to use at display
time, so that I can use the same cached image for different output
sizes, while leaving other properties that I don’t know about in
place. So I’d like to store the above as

    '(image :file "image.png" :format png)

however I’m not sure how to go about filtering the list in C. I tried
a few variations on this, but they all seg fault

static Lisp_Object
get_cache_spec (Lisp_Object spec)
{
  Lisp_Object cache_spec, tail;

  cache_spec = Qnil;

  for (tail = XCDR (spec);
       CONSP (tail) && CONSP (XCDR (tail));
       tail = XCDR (XCDR (tail)))
    {
      if (!EQ (XCAR (tail), Qwidth)
          && !EQ (XCAR (tail), QCheight)
          && !EQ (XCAR (tail), QCmax_width)
          && !EQ (XCAR (tail), QCmax_height)
          && !EQ (XCAR (tail), QCscale)
          && !EQ (XCAR (tail), QCmargin)
          && !EQ (XCAR (tail), QCascent)
          && !EQ (XCAR (tail), QCrelief))
        cache_spec = list3 (XCAR (tail), XCAR (XCDR (tail)), cache_spec);
    }
  cache_spec = list2 (XCAR (spec), cache_spec);

  return cache_spec;
}

Any ideas or alternative approaches are appreciated.
-- 
Alan Third

Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Wed, 19 Dec 2018 16:03:08 +0000
> From: Alan Third <alan@idiocy.org>
> 
> I want to strip out the properties that I’m going to use at display
> time, so that I can use the same cached image for different output
> sizes, while leaving other properties that I don’t know about in
> place. So I’d like to store the above as
> 
>     '(image :file "image.png" :format png)
> 
> however I’m not sure how to go about filtering the list in C. I tried
> a few variations on this, but they all seg fault
> 
> static Lisp_Object
> get_cache_spec (Lisp_Object spec)
> {
>   Lisp_Object cache_spec, tail;
> 
>   cache_spec = Qnil;
> 
>   for (tail = XCDR (spec);
>        CONSP (tail) && CONSP (XCDR (tail));
>        tail = XCDR (XCDR (tail)))
>     {
>       if (!EQ (XCAR (tail), Qwidth)
>           && !EQ (XCAR (tail), QCheight)
>           && !EQ (XCAR (tail), QCmax_width)
>           && !EQ (XCAR (tail), QCmax_height)
>           && !EQ (XCAR (tail), QCscale)
>           && !EQ (XCAR (tail), QCmargin)
>           && !EQ (XCAR (tail), QCascent)
>           && !EQ (XCAR (tail), QCrelief))
>         cache_spec = list3 (XCAR (tail), XCAR (XCDR (tail)), cache_spec);
>     }
>   cache_spec = list2 (XCAR (spec), cache_spec);

I think you should use Fcons instead of list3 or list2.

Re: Linking to ImageMagick by default

[Joseph Garvin]

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

What does Mozilla use for Firefox? They should have an even more serious
security outlook.

On Wed, Dec 19, 2018, 10:36 AM Eli Zaretskii <eliz@gnu.org wrote:

> > Date: Wed, 19 Dec 2018 16:03:08 +0000
> > From: Alan Third <alan@idiocy.org>
> >
> > I want to strip out the properties that I’m going to use at display
> > time, so that I can use the same cached image for different output
> > sizes, while leaving other properties that I don’t know about in
> > place. So I’d like to store the above as
> >
> >     '(image :file "image.png" :format png)
> >
> > however I’m not sure how to go about filtering the list in C. I tried
> > a few variations on this, but they all seg fault
> >
> > static Lisp_Object
> > get_cache_spec (Lisp_Object spec)
> > {
> >   Lisp_Object cache_spec, tail;
> >
> >   cache_spec = Qnil;
> >
> >   for (tail = XCDR (spec);
> >        CONSP (tail) && CONSP (XCDR (tail));
> >        tail = XCDR (XCDR (tail)))
> >     {
> >       if (!EQ (XCAR (tail), Qwidth)
> >           && !EQ (XCAR (tail), QCheight)
> >           && !EQ (XCAR (tail), QCmax_width)
> >           && !EQ (XCAR (tail), QCmax_height)
> >           && !EQ (XCAR (tail), QCscale)
> >           && !EQ (XCAR (tail), QCmargin)
> >           && !EQ (XCAR (tail), QCascent)
> >           && !EQ (XCAR (tail), QCrelief))
> >         cache_spec = list3 (XCAR (tail), XCAR (XCDR (tail)), cache_spec);
> >     }
> >   cache_spec = list2 (XCAR (spec), cache_spec);
>
> I think you should use Fcons instead of list3 or list2.
>
>

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

Re: Linking to ImageMagick by default

[Alan Third]

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

On Wed, Dec 19, 2018 at 06:36:16PM +0200, Eli Zaretskii wrote:
> I think you should use Fcons instead of list3 or list2.

Thanks Eli! I got there eventually using Fcons.

I’ve now run into another issue. Now that I’m stripping the image spec
down to the basics when I load an image in image mode and then use +
or - to resize, it doesn’t redraw.

It redraws correctly if you cause a redraw, by scrolling the window,
or making an animated gif animate, or similar.

I think that during redisplay something must be looking at the image
and deciding it hasn’t changed, but I can’t find anything like that.

Patch attached. It probably won’t build on Windows, unfortunately.
I’ve yet to look at that code properly, and I don’t have a Windows
build environment, but I expect it to work on X and NS.
-- 
Alan Third

[-- Attachment #2: 0001-Add-native-image-scaling.patch --]
[-- Type: text/plain, Size: 52631 bytes --]

From 8644aace35436d4e92f5c998003072b4b345f8ab Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 10 Dec 2018 19:43:08 +0000
Subject: [PATCH] Add native image scaling

* configure.ac: Test for XRender outside of xft checks.
* src/Makefile.in (XRENDER_LIBS): List XRender libs separately from
xft libs.
* lisp/image.el (image--get-imagemagick-and-warn): Remove ImageMagick
checks and on't flush the image cache as often.
* src/image.c (get_cache_spec):
(calc_image_spec): New function.
(Fimage_flush): Use the modified image spec.
(image_ascent):
(Fimage_size): Replace struct image with struct image_spec.
(make_image): Remove members that are image independent.
(scale_image_size):
(compute_image_size): Make available when image resizing of any sort
is available.
(lookup_image): Use new cache_spec for lookups and remove image
independent code.
* src/dispextern.h (struct image_spec): New struct.
(struct it):
(struct glyph): Replace an image_id with struct image_spec.
(struct glyph_string): Add struct image_spec.
(struct image): Remove members that are image independent.
(calc_image_spec): New function.
(image_ascent): Replace struct image with struct image_spec.
* src/dispnew.c (buffer_posn_from_coords):
(mode_line_string):
(marginal_area_string): Replace struct image.
* src/nsimage.m (ns_load_image):
([EmacsImage setSizeFromSpec:]): Remove NS specific image resizing.
* src/nsterm.m (ns_draw_fringe_bitmap): Remove GNUstep specific code.
(ns_dumpglyphs_image): Composite image according to sizes in new
image_prop struct.
* src/w32term.c (x_draw_image_foreground):
(x_draw_image_relief):
(w32_draw_image_foreground_1): Replace struct image with struct image_spec.
* src/xdisp.c (handle_single_display_spec):
(push_it):
(pop_it):
(dump_glyph):
(push_prefix_prop):
(calc_pixel_width_or_height):
(fill_image_glyph_string):
(produce_image_glyph):
(get_window_cursor_type): Replace struct image with struct image_spec.
(note_mouse_highlight): Use the image spec stored in the glyph struct
rather than the one attached to the image struct.
* src/xterm.c (x_composite_image): New function.
(x_draw_image_foreground): Replace struct image with struct image_spec
and use new x_composite_image function.
(x_draw_image_relief):
(x_draw_image_foreground_1):
(x_draw_image_glyph_string): Replace struct image with struct image_spec.
---
 configure.ac     |  14 +-
 lisp/image.el    |   3 -
 src/Makefile.in  |   3 +-
 src/dispextern.h |  48 +++---
 src/dispnew.c    |  32 +---
 src/image.c      | 374 +++++++++++++++++++++++++++++------------------
 src/nsimage.m    |  62 --------
 src/nsterm.h     |   1 -
 src/nsterm.m     |  36 ++---
 src/w32term.c    |   6 +-
 src/xdisp.c      |  86 ++++++-----
 src/xterm.c      | 108 +++++++++-----
 12 files changed, 417 insertions(+), 356 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8b34c3b658..2b22e2cbc2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3241,6 +3241,17 @@ AC_DEFUN
   CFLAGS=$late_CFLAGS
 fi
 
+# Check for XRender
+HAVE_XRENDER=no
+if test "${HAVE_X11}" = "yes"; then
+  AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
+  if test $HAVE_XRENDER = yes; then
+    XRENDER_LIBS="-lXrender"
+    AC_SUBST(XRENDER_LIBS)
+    AC_DEFINE([HAVE_XRENDER], 1, [Define to 1 if XRender is available.])
+  fi
+fi
+
 ### Start of font-backend (under any platform) section.
 # (nothing here yet -- this is a placeholder)
 ### End of font-backend (under any platform) section.
@@ -3263,15 +3274,12 @@ AC_DEFUN
       EMACS_CHECK_MODULES([XFT], [xft >= 0.13.0], [], [HAVE_XFT=no])
       ## Because xterm.c uses XRenderQueryExtension when XFT is
       ## enabled, we also need to link to -lXrender.
-      HAVE_XRENDER=no
-      AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
       if test "$HAVE_XFT" != no && test "$HAVE_XRENDER" != no; then
 	OLD_CPPFLAGS="$CPPFLAGS"
 	OLD_CFLAGS="$CFLAGS"
 	OLD_LIBS="$LIBS"
 	CPPFLAGS="$CPPFLAGS $XFT_CFLAGS"
 	CFLAGS="$CFLAGS $XFT_CFLAGS"
-	XFT_LIBS="-lXrender $XFT_LIBS"
 	LIBS="$XFT_LIBS $LIBS"
 	AC_CHECK_HEADER(X11/Xft/Xft.h,
 	  AC_CHECK_LIB(Xft, XftFontOpen, HAVE_XFT=yes, , $XFT_LIBS) , ,
diff --git a/lisp/image.el b/lisp/image.el
index 74a23046e9..4843c2de1f 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -982,10 +982,7 @@ image--get-image
     image))
 
 (defun image--get-imagemagick-and-warn ()
-  (unless (or (fboundp 'imagemagick-types) (featurep 'ns))
-    (error "Cannot rescale images without ImageMagick support"))
   (let ((image (image--get-image)))
-    (image-flush image)
     (when (fboundp 'imagemagick-types)
       (plist-put (cdr image) :type 'imagemagick))
     image))
diff --git a/src/Makefile.in b/src/Makefile.in
index 6b2e54a160..809a484413 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -127,7 +127,8 @@ LIBIMAGE=
 
 XCB_LIBS=@XCB_LIBS@
 XFT_LIBS=@XFT_LIBS@
-LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS)
+XRENDER_LIBS=@XRENDER_LIBS@
+LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS) $(XRENDER_LIBS)
 
 FONTCONFIG_CFLAGS = @FONTCONFIG_CFLAGS@
 FONTCONFIG_LIBS = @FONTCONFIG_LIBS@
diff --git a/src/dispextern.h b/src/dispextern.h
index 4cd017603d..877948ee97 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -172,6 +172,30 @@ extern bool trace_redisplay_p EXTERNALLY_VISIBLE;
 
 
 \f
+/* Struct for use by display functions.  Contains the desired size of
+   the image, derived from the display properties.  */
+
+struct image_spec
+{
+  ptrdiff_t image_id;
+  int width;
+  int height;
+
+  /* Relief to draw around the image.  */
+  int relief;
+
+  /* Optional margins around the image.  This includes the relief.  */
+  int hmargin, vmargin;
+
+  /* Percent of image height used as ascent.  A value of
+     CENTERED_IMAGE_ASCENT means draw the image centered on the
+     line.  */
+  int ascent;
+#define DEFAULT_IMAGE_ASCENT 50
+#define CENTERED_IMAGE_ASCENT -1
+};
+
+\f
 /***********************************************************************
 			    Text positions
  ***********************************************************************/
@@ -505,7 +529,7 @@ struct glyph
     } cmp;
 
     /* Image ID for image glyphs (type == IMAGE_GLYPH).  */
-    int img_id;
+    struct image_spec image;
 
 #ifdef HAVE_XWIDGETS
     /* Xwidget reference (type == XWIDGET_GLYPH).  */
@@ -1361,6 +1385,7 @@ struct glyph_string
 
   /* Image, if any.  */
   struct image *img;
+  struct image_spec image;
 
   /* Xwidget.  */
   struct xwidget *xwidget;
@@ -2373,7 +2398,7 @@ struct it
       struct {
 	Lisp_Object object;
 	struct it_slice slice;
-	ptrdiff_t image_id;
+        struct image_spec image;
       } image;
       /* method == GET_FROM_STRETCH */
       struct {
@@ -2509,7 +2534,7 @@ struct it
   enum glyphless_display_method glyphless_method;
 
   /* If what == IT_IMAGE, the id of the image to display.  */
-  ptrdiff_t image_id;
+  struct image_spec image;
 
   /* If what == IT_XWIDGET.  */
   struct xwidget *xwidget;
@@ -3001,13 +3026,6 @@ struct image
 #define BOT_CORNER 2
 #define RIGHT_CORNER 3
 
-  /* Percent of image height used as ascent.  A value of
-     CENTERED_IMAGE_ASCENT means draw the image centered on the
-     line.  */
-  int ascent;
-#define DEFAULT_IMAGE_ASCENT 50
-#define CENTERED_IMAGE_ASCENT -1
-
   /* Lisp specification of this image.  */
   Lisp_Object spec;
 
@@ -3016,12 +3034,6 @@ struct image
      Used to allow fine-grained cache flushing.  */
   Lisp_Object dependencies;
 
-  /* Relief to draw around the image.  */
-  int relief;
-
-  /* Optional margins around the image.  This includes the relief.  */
-  int hmargin, vmargin;
-
   /* Reference to the type of the image.  */
   struct image_type *type;
 
@@ -3042,7 +3054,6 @@ struct image
   struct image *next, *prev;
 };
 
-
 /* Cache of images.  Each frame has a cache.  X frames with the same
    x_display_info share their caches.  */
 
@@ -3358,6 +3369,7 @@ void clear_image_caches (Lisp_Object);
 void mark_image_cache (struct image_cache *);
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
+struct image_spec calc_image_spec (struct frame *f, Lisp_Object spec);
 ptrdiff_t lookup_image (struct frame *, Lisp_Object);
 
 #if defined (HAVE_X_WINDOWS) ||  defined (HAVE_NS)
@@ -3373,7 +3385,7 @@ RGB_PIXEL_COLOR image_background (struct image *, struct frame *,
 int image_background_transparent (struct image *, struct frame *,
                                   XImagePtr_or_DC mask);
 
-int image_ascent (struct image *, struct face *, struct glyph_slice *);
+int image_ascent (struct image_spec, struct face *, struct glyph_slice *);
 
 #endif
 
diff --git a/src/dispnew.c b/src/dispnew.c
index b628c694e8..4f374928eb 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -5117,9 +5117,6 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p
   struct text_pos startp;
   Lisp_Object string;
   struct glyph_row *row;
-#ifdef HAVE_WINDOW_SYSTEM
-  struct image *img = 0;
-#endif
   int x0, x1, to_x, it_vpos;
   void *itdata = NULL;
 
@@ -5213,17 +5210,8 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p
     }
 
 #ifdef HAVE_WINDOW_SYSTEM
-  if (it.what == IT_IMAGE)
-    {
-      /* Note that this ignores images that are fringe bitmaps,
-	 because their image ID is zero, and so IMAGE_OPT_FROM_ID will
-	 return NULL.  This is okay, since fringe bitmaps are not
-	 displayed in the text area, and so are never the object we
-	 are interested in.  */
-      img = IMAGE_OPT_FROM_ID (it.f, it.image_id);
-      if (img && !NILP (img->spec))
-	*object = img->spec;
-    }
+  if (it.what == IT_IMAGE && !NILP (it.object))
+    *object = it.object;
 #endif
 
   /* IT's vpos counts from the glyph row that includes the window's
@@ -5239,14 +5227,14 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p
 	{
 	  struct glyph *glyph = row->glyphs[TEXT_AREA] + it.hpos;
 #ifdef HAVE_WINDOW_SYSTEM
-	  if (img)
+	  if (it.what == IT_IMAGE)
 	    {
 	      *dy -= row->ascent - glyph->ascent;
 	      *dx += glyph->slice.img.x;
 	      *dy += glyph->slice.img.y;
 	      /* Image slices positions are still relative to the entire image */
-	      *width = img->width;
-	      *height = img->height;
+	      *width = it.image.width;
+	      *height = it.image.height;
 	    }
 	  else
 #endif
@@ -5317,10 +5305,7 @@ mode_line_string (struct window *w, enum window_part part,
 #ifdef HAVE_WINDOW_SYSTEM
 	  if (glyph->type == IMAGE_GLYPH)
 	    {
-	      struct image *img;
-	      img = IMAGE_OPT_FROM_ID (WINDOW_XFRAME (w), glyph->u.img_id);
-	      if (img != NULL)
-		*object = img->spec;
+              *object = glyph->object;
 	      y0 -= row->ascent - glyph->ascent;
 	    }
 #endif
@@ -5404,10 +5389,7 @@ marginal_area_string (struct window *w, enum window_part part,
 #ifdef HAVE_WINDOW_SYSTEM
 	  if (glyph->type == IMAGE_GLYPH)
 	    {
-	      struct image *img;
-	      img = IMAGE_OPT_FROM_ID (WINDOW_XFRAME (w), glyph->u.img_id);
-	      if (img != NULL)
-		*object = img->spec;
+              *object = glyph->object;
 	      y0 -= row->ascent - glyph->ascent;
 	      x0 += glyph->slice.img.x;
 	      y0 += glyph->slice.img.y;
diff --git a/src/image.c b/src/image.c
index 218626f771..7900d3a2bc 100644
--- a/src/image.c
+++ b/src/image.c
@@ -901,11 +901,14 @@ or omitted means use the selected frame.  */)
   size = Qnil;
   if (valid_image_p (spec))
     {
+      struct image_spec image;
+      int width, height;
       struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
-      struct image *img = IMAGE_FROM_ID (f, id);
-      int width = img->width + 2 * img->hmargin;
-      int height = img->height + 2 * img->vmargin;
+
+      image = calc_image_spec (f, spec);
+
+      width = image.width + 2 * image.hmargin;
+      height = image.height + 2 * image.vmargin;
 
       if (NILP (pixels))
 	size = Fcons (make_float ((double) width / FRAME_COLUMN_WIDTH (f)),
@@ -984,7 +987,6 @@ make_image (Lisp_Object spec, EMACS_UINT hash)
   eassert (img->type != NULL);
   img->spec = spec;
   img->lisp_data = Qnil;
-  img->ascent = DEFAULT_IMAGE_ASCENT;
   img->hash = hash;
   img->corners[BOT_CORNER] = -1;  /* Full image */
   return img;
@@ -1082,19 +1084,19 @@ prepare_image_for_display (struct frame *f, struct image *img)
    drawn in face FACE.  */
 
 int
-image_ascent (struct image *img, struct face *face, struct glyph_slice *slice)
+image_ascent (struct image_spec img, struct face *face, struct glyph_slice *slice)
 {
   int height;
   int ascent;
 
-  if (slice->height == img->height)
-    height = img->height + img->vmargin;
+  if (slice->height == img.height)
+    height = img.height + img.vmargin;
   else if (slice->y == 0)
-    height = slice->height + img->vmargin;
+    height = slice->height + img.vmargin;
   else
     height = slice->height;
 
-  if (img->ascent == CENTERED_IMAGE_ASCENT)
+  if (img.ascent == CENTERED_IMAGE_ASCENT)
     {
       if (face->font)
 	{
@@ -1116,7 +1118,7 @@ image_ascent (struct image *img, struct face *face, struct glyph_slice *slice)
 	ascent = height / 2;
     }
   else
-    ascent = height * (img->ascent / 100.0);
+    ascent = height * (img.ascent / 100.0);
 
   return ascent;
 }
@@ -1453,6 +1455,46 @@ make_image_cache (void)
   return c;
 }
 
+/* Filter the SPEC so it can be used to look up images in the cache
+   without worrying about whether they are the same size, etc.  */
+
+static Lisp_Object
+get_cache_spec (Lisp_Object spec)
+{
+  Lisp_Object cache_spec, tail;
+
+  /* If type is imagemagick then the image will already be resized and
+     rotated, etc., so just return the full spec.
+
+     FIXME: I think we still want to strip out the likes of margin, as
+     that is not part of the image.  */
+  if (EQ (image_spec_value (spec, QCtype, NULL), Qimagemagick))
+    return spec;
+
+  cache_spec = Qnil;
+
+  for (tail = XCDR (spec);
+       CONSP (tail) && CONSP (XCDR (tail));
+       tail = XCDR (XCDR (tail)))
+    {
+      Lisp_Object property = XCAR (tail);
+      Lisp_Object value = XCAR (XCDR (tail));
+
+      /* FIXME: Surely there's a better way to do this?  */
+      if (!EQ (property, QCwidth)
+          && !EQ (property, QCheight)
+          && !EQ (property, QCmax_width)
+          && !EQ (property, QCmax_height)
+          && !EQ (property, QCscale)
+          && !EQ (property, QCmargin)
+          && !EQ (property, QCascent)
+          && !EQ (property, QCrelief))
+        cache_spec = Fcons (property, Fcons (value, cache_spec));
+    }
+  cache_spec = Fcons (XCAR (spec), cache_spec);
+
+  return cache_spec;
+}
 
 /* Find an image matching SPEC in the cache, and return it.  If no
    image is found, return NULL.  */
@@ -1661,6 +1703,8 @@ FRAME t means refresh the image on all frames.  */)
   if (!valid_image_p (spec))
     error ("Invalid image specification");
 
+  spec = get_cache_spec (spec);
+
   if (EQ (frame, Qt))
     {
       Lisp_Object tail;
@@ -1747,6 +1791,176 @@ postprocess_image (struct frame *f, struct image *img)
     }
 }
 
+#if defined (HAVE_IMAGEMAGICK) || defined (HAVE_XRENDER) || defined (HAVE_NS)
+/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
+   safely rounded and clipped to int range.  */
+
+static int
+scale_image_size (int size, size_t divisor, size_t multiplier)
+{
+  if (divisor != 0)
+    {
+      double s = size;
+      double scaled = s * multiplier / divisor + 0.5;
+      if (scaled < INT_MAX)
+	return scaled;
+    }
+  return INT_MAX;
+}
+
+/* Compute the desired size of an image with native size WIDTH x HEIGHT.
+   Use SPEC to deduce the size.  Store the desired size into
+   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
+static void
+compute_image_size (size_t width, size_t height,
+		    Lisp_Object spec,
+		    int *d_width, int *d_height)
+{
+  Lisp_Object value;
+  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
+  double scale = 1;
+
+  value = image_spec_value (spec, QCscale, NULL);
+  if (NUMBERP (value))
+    scale = XFLOATINT (value);
+
+  value = image_spec_value (spec, QCmax_width, NULL);
+  if (FIXNATP (value))
+    max_width = min (XFIXNAT (value), INT_MAX);
+
+  value = image_spec_value (spec, QCmax_height, NULL);
+  if (FIXNATP (value))
+    max_height = min (XFIXNAT (value), INT_MAX);
+
+  /* If width and/or height is set in the display spec assume we want
+     to scale to those values.  If either h or w is unspecified, the
+     unspecified should be calculated from the specified to preserve
+     aspect ratio.  */
+  value = image_spec_value (spec, QCwidth, NULL);
+  if (FIXNATP (value))
+    {
+      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :width overrides :max-width. */
+      max_width = -1;
+    }
+
+  value = image_spec_value (spec, QCheight, NULL);
+  if (FIXNATP (value))
+    {
+      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :height overrides :max-height. */
+      max_height = -1;
+    }
+
+  /* If we have both width/height set explicitly, we skip past all the
+     aspect ratio-preserving computations below. */
+  if (desired_width != -1 && desired_height != -1)
+    goto out;
+
+  width = width * scale;
+  height = height * scale;
+
+  if (desired_width != -1)
+    /* Width known, calculate height. */
+    desired_height = scale_image_size (desired_width, width, height);
+  else if (desired_height != -1)
+    /* Height known, calculate width. */
+    desired_width = scale_image_size (desired_height, height, width);
+  else
+    {
+      desired_width = width;
+      desired_height = height;
+    }
+
+  if (max_width != -1 && desired_width > max_width)
+    {
+      /* The image is wider than :max-width. */
+      desired_width = max_width;
+      desired_height = scale_image_size (desired_width, width, height);
+    }
+
+  if (max_height != -1 && desired_height > max_height)
+    {
+      /* The image is higher than :max-height. */
+      desired_height = max_height;
+      desired_width = scale_image_size (desired_height, height, width);
+    }
+
+ out:
+  *d_width = desired_width;
+  *d_height = desired_height;
+}
+#endif /* HAVE_IMAGEMAGICK || HAVE_XRENDER || HAVE_NS  */
+
+struct image_spec
+calc_image_spec (struct frame *f, Lisp_Object spec)
+{
+  struct image_spec img_spec;
+  struct image *img;
+
+  Lisp_Object ascent, margin, relief;
+  int relief_bound;
+
+  img_spec.image_id = lookup_image (f, spec);
+  img = IMAGE_OPT_FROM_ID (f, img_spec.image_id);
+
+#if defined (HAVE_XRENDER) || defined (HAVE_NS)
+  if (!EQ (image_spec_value (spec, QCtype, NULL), Qimagemagick))
+    {
+      int width, height;
+
+      compute_image_size (img->width, img->height, spec, &width, &height);
+
+      /* Set the final sizes.  We always maintain the aspect ratio.  */
+      img_spec.width = width;
+      img_spec.height = height;
+    }
+  else
+#endif
+    {
+      /* We don't have built-in image resizing or we're using
+         ImageMagick which does its own transforms.  */
+      img_spec.width = img->width;
+      img_spec.height = img->height;
+    }
+
+  /* Handle type independent image attributes
+     `:ascent ASCENT', `:margin MARGIN', `:relief RELIEF'.  */
+  ascent = image_spec_value (spec, QCascent, NULL);
+  if (FIXNUMP (ascent))
+    img_spec.ascent = XFIXNAT (ascent);
+  else if (EQ (ascent, Qcenter))
+    img_spec.ascent = CENTERED_IMAGE_ASCENT;
+  else
+    img_spec.ascent = DEFAULT_IMAGE_ASCENT;
+
+  margin = image_spec_value (spec, QCmargin, NULL);
+  if (FIXNUMP (margin))
+    img_spec.vmargin = img_spec.hmargin = XFIXNAT (margin);
+  else if (CONSP (margin))
+    {
+      img_spec.hmargin = XFIXNAT (XCAR (margin));
+      img_spec.vmargin = XFIXNAT (XCDR (margin));
+    }
+  else
+    {
+      img_spec.hmargin = 0;
+      img_spec.vmargin = 0;
+    }
+
+  relief = image_spec_value (spec, QCrelief, NULL);
+  relief_bound = INT_MAX - max (img_spec.hmargin, img_spec.vmargin);
+  if (RANGED_FIXNUMP (- relief_bound, relief, relief_bound))
+    {
+      img_spec.relief = XFIXNUM (relief);
+      img_spec.hmargin += eabs (img_spec.relief);
+      img_spec.vmargin += eabs (img_spec.relief);
+    }
+  else
+    img_spec.relief = 0;
+
+  return img_spec;
+}
 
 /* Return the id of image with Lisp specification SPEC on frame F.
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
@@ -1756,15 +1970,18 @@ lookup_image (struct frame *f, Lisp_Object spec)
 {
   struct image *img;
   EMACS_UINT hash;
+  Lisp_Object cache_spec;
 
   /* F must be a window-system frame, and SPEC must be a valid image
      specification.  */
   eassert (FRAME_WINDOW_P (f));
   eassert (valid_image_p (spec));
 
+  cache_spec = get_cache_spec (spec);
+
   /* Look up SPEC in the hash table of the image cache.  */
-  hash = sxhash (spec, 0);
-  img = search_image_cache (f, spec, hash);
+  hash = sxhash (cache_spec, 0);
+  img = search_image_cache (f, cache_spec, hash);
   if (img && img->load_failed_p)
     {
       free_image (f, img);
@@ -1775,7 +1992,7 @@ lookup_image (struct frame *f, Lisp_Object spec)
   if (img == NULL)
     {
       block_input ();
-      img = make_image (spec, hash);
+      img = make_image (cache_spec, hash);
       cache_image (f, img);
       img->load_failed_p = ! img->type->load (f, img);
       img->frame_foreground = FRAME_FOREGROUND_PIXEL (f);
@@ -1797,35 +2014,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
 	}
       else
 	{
-	  /* Handle image type independent image attributes
-	     `:ascent ASCENT', `:margin MARGIN', `:relief RELIEF',
+	  /* Handle image type independent image attribute
 	     `:background COLOR'.  */
-	  Lisp_Object ascent, margin, relief, bg;
-	  int relief_bound;
-
-	  ascent = image_spec_value (spec, QCascent, NULL);
-	  if (FIXNUMP (ascent))
-	    img->ascent = XFIXNAT (ascent);
-	  else if (EQ (ascent, Qcenter))
-	    img->ascent = CENTERED_IMAGE_ASCENT;
-
-	  margin = image_spec_value (spec, QCmargin, NULL);
-	  if (FIXNUMP (margin))
-	    img->vmargin = img->hmargin = XFIXNAT (margin);
-	  else if (CONSP (margin))
-	    {
-	      img->hmargin = XFIXNAT (XCAR (margin));
-	      img->vmargin = XFIXNAT (XCDR (margin));
-	    }
-
-	  relief = image_spec_value (spec, QCrelief, NULL);
-	  relief_bound = INT_MAX - max (img->hmargin, img->vmargin);
-	  if (RANGED_FIXNUMP (- relief_bound, relief, relief_bound))
-	    {
-	      img->relief = XFIXNUM (relief);
-	      img->hmargin += eabs (img->relief);
-	      img->vmargin += eabs (img->relief);
-	    }
+	  Lisp_Object bg;
 
 	  if (! img->background_valid)
 	    {
@@ -8101,105 +8292,6 @@ gif_load (struct frame *f, struct image *img)
 				 ImageMagick
 ***********************************************************************/
 
-/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
-   safely rounded and clipped to int range.  */
-
-static int
-scale_image_size (int size, size_t divisor, size_t multiplier)
-{
-  if (divisor != 0)
-    {
-      double s = size;
-      double scaled = s * multiplier / divisor + 0.5;
-      if (scaled < INT_MAX)
-	return scaled;
-    }
-  return INT_MAX;
-}
-
-/* Compute the desired size of an image with native size WIDTH x HEIGHT.
-   Use SPEC to deduce the size.  Store the desired size into
-   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
-static void
-compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
-		    int *d_width, int *d_height)
-{
-  Lisp_Object value;
-  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
-  double scale = 1;
-
-  value = image_spec_value (spec, QCscale, NULL);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value);
-
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
-
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
-
-  /* If width and/or height is set in the display spec assume we want
-     to scale to those values.  If either h or w is unspecified, the
-     unspecified should be calculated from the specified to preserve
-     aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
-    {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
-    {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  /* If we have both width/height set explicitly, we skip past all the
-     aspect ratio-preserving computations below. */
-  if (desired_width != -1 && desired_height != -1)
-    goto out;
-
-  width = width * scale;
-  height = height * scale;
-
-  if (desired_width != -1)
-    /* Width known, calculate height. */
-    desired_height = scale_image_size (desired_width, width, height);
-  else if (desired_height != -1)
-    /* Height known, calculate width. */
-    desired_width = scale_image_size (desired_height, height, width);
-  else
-    {
-      desired_width = width;
-      desired_height = height;
-    }
-
-  if (max_width != -1 && desired_width > max_width)
-    {
-      /* The image is wider than :max-width. */
-      desired_width = max_width;
-      desired_height = scale_image_size (desired_width, width, height);
-    }
-
-  if (max_height != -1 && desired_height > max_height)
-    {
-      /* The image is higher than :max-height. */
-      desired_height = max_height;
-      desired_width = scale_image_size (desired_height, height, width);
-    }
-
- out:
-  *d_width = desired_width;
-  *d_height = desired_height;
-}
-
 static bool imagemagick_image_p (Lisp_Object);
 static bool imagemagick_load (struct frame *, struct image *);
 static void imagemagick_clear_image (struct frame *, struct image *);
diff --git a/src/nsimage.m b/src/nsimage.m
index 0ae1b88edd..71da0201d1 100644
--- a/src/nsimage.m
+++ b/src/nsimage.m
@@ -126,8 +126,6 @@ Updated by Christian Limpach (chris@nice.ch)
       eImg = temp;
     }
 
-  [eImg setSizeFromSpec:XCDR (img->spec)];
-
   size = [eImg size];
   img->width = size.width;
   img->height = size.height;
@@ -524,66 +522,6 @@ - (BOOL)setFrame: (unsigned int) index
   return YES;
 }
 
-- (void)setSizeFromSpec: (Lisp_Object) spec
-{
-  NSSize size = [self size];
-  Lisp_Object value;
-  double scale = 1, aspect = size.width / size.height;
-  double width = -1, height = -1, max_width = -1, max_height = -1;
-
-  value = Fplist_get (spec, QCscale);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value) ;
-
-  value = Fplist_get (spec, QCmax_width);
-  if (NUMBERP (value))
-    max_width = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCmax_height);
-  if (NUMBERP (value))
-    max_height = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCwidth);
-  if (NUMBERP (value))
-    {
-      width = XFLOATINT (value) * scale;
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = Fplist_get (spec, QCheight);
-  if (NUMBERP (value))
-    {
-      height = XFLOATINT (value) * scale;
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  if (width <= 0 && height <= 0)
-    {
-      width = size.width * scale;
-      height = size.height * scale;
-    }
-  else if (width > 0 && height <= 0)
-      height = width / aspect;
-  else if (height > 0 && width <= 0)
-      width = height * aspect;
-
-  if (max_width > 0 && width > max_width)
-    {
-      width = max_width;
-      height = max_width / aspect;
-    }
-
-  if (max_height > 0 && height > max_height)
-    {
-      height = max_height;
-      width = max_height * aspect;
-    }
-
-  [self setSize:NSMakeSize(width, height)];
-}
-
 - (instancetype)rotate: (double)rotation
 {
   EmacsImage *new_image;
diff --git a/src/nsterm.h b/src/nsterm.h
index 23460abc65..f64d78bd2b 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -648,7 +648,6 @@ typedef id instancetype;
 - (NSColor *)stippleMask;
 - (Lisp_Object)getMetadata;
 - (BOOL)setFrame: (unsigned int) index;
-- (void)setSizeFromSpec: (Lisp_Object) spec;
 - (instancetype)rotate: (double)rotation;
 @end
 
diff --git a/src/nsterm.m b/src/nsterm.m
index 6c285f0abb..0a2a184830 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3121,7 +3121,6 @@ so some key presses (TAB) are swallowed by the system.  */
             [img setXBMColor: bm_color];
           }
 
-#ifdef NS_IMPL_COCOA
           // Note: For periodic images, the full image height is "h + hd".
           // By using the height h, a suitable part of the image is used.
           NSRect fromRect = NSMakeRect(0, 0, p->wd, p->h);
@@ -3134,13 +3133,6 @@ so some key presses (TAB) are swallowed by the system.  */
                  fraction: 1.0
                respectFlipped: YES
                     hints: nil];
-#else
-          {
-            NSPoint pt = imageRect.origin;
-            pt.y += p->h;
-            [img compositeToPoint: pt operation: NSCompositingOperationSourceOver];
-          }
-#endif
         }
       ns_reset_clipping (f);
     }
@@ -3821,13 +3813,15 @@ Function modeled after x_draw_glyph_string_box ().
 {
   EmacsImage *img = s->img->pixmap;
   int box_line_vwidth = max (s->face->box_line_width, 0);
-  int x = s->x, y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  int x = s->x, y = s->ybase - image_ascent (s->image, s->face, &s->slice);
   int bg_x, bg_y, bg_height;
   int th;
   char raised_p;
   NSRect br;
   struct face *face;
   NSColor *tdCol;
+  double xscale = (double)s->img->width/s->image.width;
+  double yscale = (double)s->img->height/s->image.height;
 
   NSTRACE ("ns_dumpglyphs_image");
 
@@ -3841,8 +3835,8 @@ Function modeled after x_draw_glyph_string_box ().
   /* other terms have this, but was causing problems w/tabbar mode */
   /* - 2 * box_line_vwidth; */
 
-  if (s->slice.x == 0) x += s->img->hmargin;
-  if (s->slice.y == 0) y += s->img->vmargin;
+  if (s->slice.x == 0) x += s->image.hmargin;
+  if (s->slice.y == 0) y += s->image.vmargin;
 
   /* Draw BG: if we need larger area than image itself cleared, do that,
      otherwise, since we composite the image under NS (instead of mucking
@@ -3859,7 +3853,7 @@ Function modeled after x_draw_glyph_string_box ().
 
   [ns_lookup_indexed_color (NS_FACE_BACKGROUND (face), s->f) set];
 
-  if (bg_height > s->slice.height || s->img->hmargin || s->img->vmargin
+  if (bg_height > s->slice.height || s->image.hmargin || s->image.vmargin
       || s->img->mask || s->img->pixmap == 0 || s->width != s->background_width)
     {
       br = NSMakeRect (bg_x, bg_y, s->background_width, bg_height);
@@ -3877,9 +3871,9 @@ Function modeled after x_draw_glyph_string_box ().
     {
 #ifdef NS_IMPL_COCOA
       NSRect dr = NSMakeRect (x, y, s->slice.width, s->slice.height);
-      NSRect ir = NSMakeRect (s->slice.x,
-                              s->img->height - s->slice.y - s->slice.height,
-                              s->slice.width, s->slice.height);
+      NSRect ir = NSMakeRect (s->slice.x * xscale,
+                              (s->image.height - s->slice.y - s->slice.height) * yscale,
+                              s->slice.width * xscale , s->slice.height * yscale);
       [img drawInRect: dr
              fromRect: ir
              operation: NSCompositingOperationSourceOver
@@ -3914,7 +3908,7 @@ Function modeled after x_draw_glyph_string_box ().
   ns_draw_text_decoration (s, face, tdCol, br.size.width, br.origin.x);
 
   /* Draw relief, if requested */
-  if (s->img->relief || s->hl ==DRAW_IMAGE_RAISED || s->hl ==DRAW_IMAGE_SUNKEN)
+  if (s->image.relief || s->hl ==DRAW_IMAGE_RAISED || s->hl ==DRAW_IMAGE_SUNKEN)
     {
       if (s->hl == DRAW_IMAGE_SUNKEN || s->hl == DRAW_IMAGE_RAISED)
         {
@@ -3924,8 +3918,8 @@ Function modeled after x_draw_glyph_string_box ().
         }
       else
         {
-          th = abs (s->img->relief);
-          raised_p = (s->img->relief > 0);
+          th = abs (s->image.relief);
+          raised_p = (s->image.relief > 0);
         }
 
       r.origin.x = x - th;
@@ -3934,9 +3928,9 @@ Function modeled after x_draw_glyph_string_box ().
       r.size.height = s->slice.height + 2*th-1;
       ns_draw_relief (r, th, raised_p,
                       s->slice.y == 0,
-                      s->slice.y + s->slice.height == s->img->height,
+                      s->slice.y + s->slice.height == s->image.height,
                       s->slice.x == 0,
-                      s->slice.x + s->slice.width == s->img->width, s);
+                      s->slice.x + s->slice.width == s->image.width, s);
     }
 
   /* If there is no mask, the background won't be seen,
@@ -3944,7 +3938,7 @@ Function modeled after x_draw_glyph_string_box ().
      Do this for all images, getting transparency right is not reliable.  */
   if (s->hl == DRAW_CURSOR)
     {
-      int thickness = abs (s->img->relief);
+      int thickness = abs (s->image.relief);
       if (thickness == 0) thickness = 1;
       ns_draw_box (br, thickness, FRAME_CURSOR_COLOR (s->f), 1, 1);
     }
diff --git a/src/w32term.c b/src/w32term.c
index 8d189ae32c..eb231b57f7 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -1850,7 +1850,7 @@ static void
 x_draw_image_foreground (struct glyph_string *s)
 {
   int x = s->x;
-  int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
@@ -1943,7 +1943,7 @@ x_draw_image_relief (struct glyph_string *s)
   int extra_x, extra_y;
   RECT r;
   int x = s->x;
-  int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
@@ -2015,7 +2015,7 @@ w32_draw_image_foreground_1 (struct glyph_string *s, HBITMAP pixmap)
   HDC hdc = CreateCompatibleDC (s->hdc);
   HGDIOBJ orig_hdc_obj = SelectObject (hdc, pixmap);
   int x = 0;
-  int y = s->ybase - s->y - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - s->y - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 4201bdc4a7..6d17f64b7d 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -5197,7 +5197,7 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
 
 	  it->area = TEXT_AREA;
 	  it->what = IT_IMAGE;
-	  it->image_id = -1; /* no image */
+	  it->image.image_id = -1; /* no image */
 	  it->position = start_pos;
 	  it->object = NILP (object) ? it->w->contents : object;
 	  it->method = GET_FROM_IMAGE;
@@ -5356,7 +5356,7 @@ handle_single_display_spec (struct it *it, Lisp_Object spec, Lisp_Object object,
       else
 	{
 	  it->what = IT_IMAGE;
-	  it->image_id = lookup_image (it->f, value);
+          it->image = calc_image_spec (it->f, value);
 	  it->position = start_pos;
 	  it->object = NILP (object) ? it->w->contents : object;
 	  it->method = GET_FROM_IMAGE;
@@ -6103,7 +6103,7 @@ push_it (struct it *it, struct text_pos *position)
     {
     case GET_FROM_IMAGE:
       p->u.image.object = it->object;
-      p->u.image.image_id = it->image_id;
+      p->u.image.image = it->image;
       p->u.image.slice = it->slice;
       break;
     case GET_FROM_STRETCH:
@@ -6209,7 +6209,7 @@ pop_it (struct it *it)
   switch (it->method)
     {
     case GET_FROM_IMAGE:
-      it->image_id = p->u.image.image_id;
+      it->image = p->u.image.image;
       it->object = p->u.image.object;
       it->slice = p->u.image.slice;
       break;
@@ -19446,7 +19446,7 @@ dump_glyph (struct glyph_row *row, struct glyph *glyph, int area)
 		      ? '0'
 		      : '-'))),
 	       glyph->pixel_width,
-	       (unsigned int) glyph->u.img_id,
+	       (unsigned int) glyph->u.image.image_id,
 	       '.',
 	       glyph->face_id,
 	       glyph->left_box_line_p,
@@ -20775,7 +20775,7 @@ push_prefix_prop (struct it *it, Lisp_Object prop)
   else if (IMAGEP (prop))
     {
       it->what = IT_IMAGE;
-      it->image_id = lookup_image (it->f, prop);
+      it->image = calc_image_spec (it->f, prop);
       it->method = GET_FROM_IMAGE;
     }
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -25596,12 +25596,8 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	  /* '(image PROPS...)': width or height of the specified image.  */
 	  if (FRAME_WINDOW_P (it->f)
 	      && valid_image_p (prop))
-	    {
-	      ptrdiff_t id = lookup_image (it->f, prop);
-	      struct image *img = IMAGE_FROM_ID (it->f, id);
+	      return OK_PIXELS (width_p ? it->image.width : it->image.height);
 
-	      return OK_PIXELS (width_p ? img->width : img->height);
-	    }
 	  /* '(xwidget PROPS...)': dimensions of the specified xwidget.  */
 	  if (FRAME_WINDOW_P (it->f) && valid_xwidget_spec_p (prop))
 	    {
@@ -26104,7 +26100,8 @@ static void
 fill_image_glyph_string (struct glyph_string *s)
 {
   eassert (s->first_glyph->type == IMAGE_GLYPH);
-  s->img = IMAGE_FROM_ID (s->f, s->first_glyph->u.img_id);
+  s->img = IMAGE_FROM_ID (s->f, s->first_glyph->u.image.image_id);
+  s->image = s->first_glyph->u.image;
   eassert (s->img);
   s->slice = s->first_glyph->slice.img;
   s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
@@ -27247,7 +27244,7 @@ produce_image_glyph (struct it *it)
   /* Make sure X resources of the face is loaded.  */
   prepare_face_for_display (it->f, face);
 
-  if (it->image_id < 0)
+  if (it->image.image_id < 0)
     {
       /* Fringe bitmap.  */
       it->ascent = it->phys_ascent = 0;
@@ -27257,60 +27254,60 @@ produce_image_glyph (struct it *it)
       return;
     }
 
-  img = IMAGE_FROM_ID (it->f, it->image_id);
+  img = IMAGE_FROM_ID (it->f, it->image.image_id);
   /* Make sure X resources of the image is loaded.  */
   prepare_image_for_display (it->f, img);
 
   slice.x = slice.y = 0;
-  slice.width = img->width;
-  slice.height = img->height;
+  slice.width = it->image.width;
+  slice.height = it->image.height;
 
   if (FIXNUMP (it->slice.x))
     slice.x = XFIXNUM (it->slice.x);
   else if (FLOATP (it->slice.x))
-    slice.x = XFLOAT_DATA (it->slice.x) * img->width;
+    slice.x = XFLOAT_DATA (it->slice.x) * it->image.width;
 
   if (FIXNUMP (it->slice.y))
     slice.y = XFIXNUM (it->slice.y);
   else if (FLOATP (it->slice.y))
-    slice.y = XFLOAT_DATA (it->slice.y) * img->height;
+    slice.y = XFLOAT_DATA (it->slice.y) * it->image.height;
 
   if (FIXNUMP (it->slice.width))
     slice.width = XFIXNUM (it->slice.width);
   else if (FLOATP (it->slice.width))
-    slice.width = XFLOAT_DATA (it->slice.width) * img->width;
+    slice.width = XFLOAT_DATA (it->slice.width) * it->image.width;
 
   if (FIXNUMP (it->slice.height))
     slice.height = XFIXNUM (it->slice.height);
   else if (FLOATP (it->slice.height))
-    slice.height = XFLOAT_DATA (it->slice.height) * img->height;
+    slice.height = XFLOAT_DATA (it->slice.height) * it->image.height;
 
-  if (slice.x >= img->width)
-    slice.x = img->width;
-  if (slice.y >= img->height)
-    slice.y = img->height;
-  if (slice.x + slice.width >= img->width)
-    slice.width = img->width - slice.x;
-  if (slice.y + slice.height > img->height)
-    slice.height = img->height - slice.y;
+  if (slice.x >= it->image.width)
+    slice.x = it->image.width;
+  if (slice.y >= it->image.height)
+    slice.y = it->image.height;
+  if (slice.x + slice.width >= it->image.width)
+    slice.width = it->image.width - slice.x;
+  if (slice.y + slice.height > it->image.height)
+    slice.height = it->image.height - slice.y;
 
   if (slice.width == 0 || slice.height == 0)
     return;
 
-  it->ascent = it->phys_ascent = glyph_ascent = image_ascent (img, face, &slice);
+  it->ascent = it->phys_ascent = glyph_ascent = image_ascent (it->image, face, &slice);
 
   it->descent = slice.height - glyph_ascent;
   if (slice.y == 0)
-    it->descent += img->vmargin;
-  if (slice.y + slice.height == img->height)
-    it->descent += img->vmargin;
+    it->descent += it->image.vmargin;
+  if (slice.y + slice.height == it->image.height)
+    it->descent += it->image.vmargin;
   it->phys_descent = it->descent;
 
   it->pixel_width = slice.width;
   if (slice.x == 0)
-    it->pixel_width += img->hmargin;
-  if (slice.x + slice.width == img->width)
-    it->pixel_width += img->hmargin;
+    it->pixel_width += it->image.hmargin;
+  if (slice.x + slice.width == it->image.width)
+    it->pixel_width += it->image.hmargin;
 
   /* It's quite possible for images to have an ascent greater than
      their height, so don't get confused in that case.  */
@@ -27325,13 +27322,13 @@ produce_image_glyph (struct it *it)
 	{
 	  if (slice.y == 0)
 	    it->ascent += face->box_line_width;
-	  if (slice.y + slice.height == img->height)
+	  if (slice.y + slice.height == it->image.height)
 	    it->descent += face->box_line_width;
 	}
 
       if (it->start_of_box_run_p && slice.x == 0)
 	it->pixel_width += eabs (face->box_line_width);
-      if (it->end_of_box_run_p && slice.x + slice.width == img->width)
+      if (it->end_of_box_run_p && slice.x + slice.width == it->image.width)
 	it->pixel_width += eabs (face->box_line_width);
     }
 
@@ -27388,7 +27385,7 @@ produce_image_glyph (struct it *it)
           glyph->padding_p = false;
 	  glyph->glyph_not_available_p = false;
 	  glyph->face_id = it->face_id;
-	  glyph->u.img_id = img->id;
+	  glyph->u.image = it->image;
 	  glyph->slice.img = slice;
 	  glyph->font_type = FONT_TYPE_UNKNOWN;
 	  if (it->bidi_p)
@@ -29326,15 +29323,15 @@ get_window_cursor_type (struct window *w, struct glyph *glyph, int *width,
 	      /* Using a block cursor on large images can be very annoying.
 		 So use a hollow cursor for "large" images.
 		 If image is not transparent (no mask), also use hollow cursor.  */
-	      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
+	      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.image.image_id);
 	      if (img != NULL && IMAGEP (img->spec))
 		{
 		  /* Arbitrarily, interpret "Large" as >32x32 and >NxN
 		     where N = size of default frame font size.
 		     This should cover most of the "tiny" icons people may use.  */
 		  if (!img->mask
-		      || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
-		      || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
+		      || glyph->u.image.width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
+		      || glyph->u.image.height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
 		    cursor_type = HOLLOW_BOX_CURSOR;
 		}
 	    }
@@ -31482,11 +31479,10 @@ note_mouse_highlight (struct frame *f, int x, int y)
       /* Look for :pointer property on image.  */
       if (glyph != NULL && glyph->type == IMAGE_GLYPH)
 	{
-	  struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
-	  if (img != NULL && IMAGEP (img->spec))
+	  if (IMAGEP (glyph->object))
 	    {
 	      Lisp_Object image_map, hotspot;
-	      if ((image_map = Fplist_get (XCDR (img->spec), QCmap),
+	      if ((image_map = Fplist_get (XCDR (glyph->object), QCmap),
 		   !NILP (image_map))
 		  && (hotspot = find_hot_spot (image_map,
 					       glyph->slice.img.x + dx,
@@ -31517,7 +31513,7 @@ note_mouse_highlight (struct frame *f, int x, int y)
 		    }
 		}
 	      if (NILP (pointer))
-		pointer = Fplist_get (XCDR (img->spec), QCpointer);
+		pointer = Fplist_get (XCDR (glyph->object), QCpointer);
 	    }
 	}
 #endif	/* HAVE_WINDOW_SYSTEM */
diff --git a/src/xterm.c b/src/xterm.c
index 943f4c3b3d..6a94916d4e 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -38,11 +38,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/extensions/Xfixes.h>
 #endif
 
-/* Using Xft implies that XRender is available.  */
-#ifdef HAVE_XFT
-#include <X11/extensions/Xrender.h>
-#endif
-
 #ifdef HAVE_XDBE
 #include <X11/extensions/Xdbe.h>
 #endif
@@ -136,6 +131,10 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/XKBlib.h>
 #endif
 
+#ifdef HAVE_XRENDER
+#include <X11/extensions/Xrender.h>
+#endif
+
 /* Default to using XIM if available.  */
 #ifdef USE_XIM
 bool use_xim = true;
@@ -2976,13 +2975,60 @@ x_draw_glyph_string_box (struct glyph_string *s)
 }
 
 
+static void
+x_composite_image (struct glyph_string *s, Pixmap dest,
+                   int srcX, int srcY, int dstX, int dstY,
+                   int width, int height)
+{
+#ifdef HAVE_XRENDER
+  Picture source, destination;
+  XRenderPictFormat *default_format, *source_format;
+  XRenderPictureAttributes source_attr, dest_attr;
+  double xscale = (double) s->img->width/s->image.width;
+  double yscale = (double) s->img->height/s->image.height;
+
+  XTransform tmat = {{{XDoubleToFixed (xscale), XDoubleToFixed (0), XDoubleToFixed (0)},
+                      {XDoubleToFixed (0), XDoubleToFixed (yscale), XDoubleToFixed (0)},
+                      {XDoubleToFixed (0), XDoubleToFixed (0), XDoubleToFixed (1)}}};
+
+  default_format = XRenderFindVisualFormat (s->display, DefaultVisual (s->display, 0));
+  /* FIXME: I doubt this is always correct.  There must be a way of
+     working out the format of the pixmap.  */
+  source_format = XRenderFindStandardFormat (s->display, PictStandardRGB24);
+
+  source = XRenderCreatePicture (s->display, s->img->pixmap, source_format,
+                                 0, &source_attr);
+  destination = XRenderCreatePicture (s->display, dest, default_format, 0, &dest_attr);
+
+  XRenderSetPictureFilter (s->display, source, FilterBest, 0, 0);
+  XRenderSetPictureTransform (s->display, source, &tmat);
+
+  /* FIXME: XRenderComposite supports masks, so can we just use the
+     mask in s->img->mask here?  */
+  XRenderComposite (s->display, PictOpOver, source, None, destination,
+                    srcX*xscale, srcY*yscale,
+                    0, 0,
+                    dstX, dstY,
+                    width, height);
+
+  XRenderFreePicture (s->display, source);
+  XRenderFreePicture (s->display, destination);
+#else
+  XCopyArea (s->display, s->img->pixmap,
+             dest, s->gc,
+             srcX, srcY
+             width, height, dstX, dstY);
+#endif
+}
+
+
 /* Draw foreground of image glyph string S.  */
 
 static void
 x_draw_image_foreground (struct glyph_string *s)
 {
   int x = s->x;
-  int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
@@ -2994,9 +3040,9 @@ x_draw_image_foreground (struct glyph_string *s)
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
   if (s->slice.x == 0)
-    x += s->img->hmargin;
+    x += s->image.hmargin;
   if (s->slice.y == 0)
-    y += s->img->vmargin;
+    y += s->image.vmargin;
 
   if (s->img->pixmap)
     {
@@ -3024,10 +3070,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 	}
       else
 	{
@@ -3039,10 +3083,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 
 	  /* When the image has a mask, we can expect that at
 	     least part of a mouse highlight or a block cursor will
@@ -3052,7 +3094,7 @@ x_draw_image_foreground (struct glyph_string *s)
 	     nothing here for mouse-face.  */
 	  if (s->hl == DRAW_CURSOR)
 	    {
-	      int relief = eabs (s->img->relief);
+	      int relief = eabs (s->image.relief);
 	      x_draw_rectangle (s->f, s->gc,
 			      x - relief, y - relief,
 			      s->slice.width + relief*2 - 1,
@@ -3077,7 +3119,7 @@ x_draw_image_relief (struct glyph_string *s)
   int extra_x, extra_y;
   XRectangle r;
   int x = s->x;
-  int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
@@ -3089,9 +3131,9 @@ x_draw_image_relief (struct glyph_string *s)
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
   if (s->slice.x == 0)
-    x += s->img->hmargin;
+    x += s->image.hmargin;
   if (s->slice.y == 0)
-    y += s->img->vmargin;
+    y += s->image.vmargin;
 
   if (s->hl == DRAW_IMAGE_SUNKEN
       || s->hl == DRAW_IMAGE_RAISED)
@@ -3101,8 +3143,8 @@ x_draw_image_relief (struct glyph_string *s)
     }
   else
     {
-      thick = eabs (s->img->relief);
-      raised_p = s->img->relief > 0;
+      thick = eabs (s->image.relief);
+      raised_p = s->image.relief > 0;
     }
 
   x1 = x + s->slice.width - 1;
@@ -3128,9 +3170,9 @@ x_draw_image_relief (struct glyph_string *s)
     x -= thick + extra_x, left_p = true;
   if (s->slice.y == 0)
     y -= thick + extra_y, top_p = true;
-  if (s->slice.x + s->slice.width == s->img->width)
+  if (s->slice.x + s->slice.width == s->image.width)
     x1 += thick + extra_x, right_p = true;
-  if (s->slice.y + s->slice.height == s->img->height)
+  if (s->slice.y + s->slice.height == s->image.height)
     y1 += thick + extra_y, bot_p = true;
 
   x_setup_relief_colors (s);
@@ -3146,7 +3188,7 @@ static void
 x_draw_image_foreground_1 (struct glyph_string *s, Pixmap pixmap)
 {
   int x = 0;
-  int y = s->ybase - s->y - image_ascent (s->img, s->face, &s->slice);
+  int y = s->ybase - s->y - image_ascent (s->image, s->face, &s->slice);
 
   /* If first glyph of S has a left box line, start drawing it to the
      right of that line.  */
@@ -3158,9 +3200,9 @@ x_draw_image_foreground_1 (struct glyph_string *s, Pixmap pixmap)
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
   if (s->slice.x == 0)
-    x += s->img->hmargin;
+    x += s->image.hmargin;
   if (s->slice.y == 0)
-    y += s->img->vmargin;
+    y += s->image.vmargin;
 
   if (s->img->pixmap)
     {
@@ -3200,7 +3242,7 @@ x_draw_image_foreground_1 (struct glyph_string *s, Pixmap pixmap)
 	     nothing here for mouse-face.  */
 	  if (s->hl == DRAW_CURSOR)
 	    {
-	      int r = eabs (s->img->relief);
+	      int r = eabs (s->image.relief);
 	      x_draw_rectangle (s->f, s->gc, x - r, y - r,
 			      s->slice.width + r*2 - 1,
 			      s->slice.height + r*2 - 1);
@@ -3265,8 +3307,8 @@ x_draw_image_glyph_string (struct glyph_string *s)
      flickering.  */
   s->stippled_p = s->face->stipple != 0;
   if (height > s->slice.height
-      || s->img->hmargin
-      || s->img->vmargin
+      || s->image.hmargin
+      || s->image.vmargin
       || s->img->mask
       || s->img->pixmap == 0
       || s->width != s->background_width)
@@ -3338,8 +3380,8 @@ x_draw_image_glyph_string (struct glyph_string *s)
     {
       cairo_t *cr = x_begin_cr_clip (s->f, s->gc);
 
-      int x = s->x + s->img->hmargin;
-      int y = s->y + s->img->vmargin;
+      int x = s->x + s->image.hmargin;
+      int y = s->y + s->image.vmargin;
       int width = s->background_width;
 
       cairo_set_source_surface (cr, s->img->cr_data,
@@ -3363,7 +3405,7 @@ x_draw_image_glyph_string (struct glyph_string *s)
     x_draw_image_foreground (s);
 
   /* If we must draw a relief around the image, do it.  */
-  if (s->img->relief
+  if (s->image.relief
       || s->hl == DRAW_IMAGE_RAISED
       || s->hl == DRAW_IMAGE_SUNKEN)
     x_draw_image_relief (s);
-- 
2.19.1

Re: Linking to ImageMagick by default

[Alan Third]

On Wed, Dec 19, 2018 at 10:45:45AM -0600, Joseph Garvin wrote:
> What does Mozilla use for Firefox? They should have an even more serious
> security outlook.

AFAICT Firefox uses its own ‘imglib’, which is using libpng and
friends in the background. I don’t know if it handles resizing or if
they just resize on display to the screen. I seem to recall that they
have some fancy image caching/compressing technology to save RAM.
-- 
Alan Third

Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Thu, 27 Dec 2018 13:11:05 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I’ve now run into another issue. Now that I’m stripping the image spec
> down to the basics when I load an image in image mode and then use +
> or - to resize, it doesn’t redraw.

And the current code using ImageMagick does redraw in this situation?
If so, can you spot how it accomplishes that?

See also the patch posted by Juri just today, which might help.

(I didn't yet have time to read the patch.)

Thanks.

Re: Linking to ImageMagick by default

[Juri Linkov]

>> I’ve now run into another issue. Now that I’m stripping the image spec
>> down to the basics when I load an image in image mode and then use +
>> or - to resize, it doesn’t redraw.
>
> And the current code using ImageMagick does redraw in this situation?
> If so, can you spot how it accomplishes that?
>
> See also the patch posted by Juri just today, which might help.

Sounds more like bug#33631.

Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Thu, 27 Dec 2018 13:11:05 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I’ve now run into another issue. Now that I’m stripping the image spec
> down to the basics when I load an image in image mode and then use +
> or - to resize, it doesn’t redraw.
> 
> It redraws correctly if you cause a redraw, by scrolling the window,
> or making an animated gif animate, or similar.

Does it redraw if you just type "M-x" and nothing else?

> I think that during redisplay something must be looking at the image
> and deciding it hasn’t changed, but I can’t find anything like that.

My first suspicion is that redisplay doesn't even look at the image,
because nothing tells it that it should.  IOW, it's a side effect of
redisplay optimizations that should be disabled in this case.

> Patch attached.

Thanks, a couple of comments below.

> +/* Struct for use by display functions.  Contains the desired size of
> +   the image, derived from the display properties.  */
> +
> +struct image_spec
> +{
> +  ptrdiff_t image_id;
> +  int width;
> +  int height;
> +
> +  /* Relief to draw around the image.  */
> +  int relief;
> +
> +  /* Optional margins around the image.  This includes the relief.  */
> +  int hmargin, vmargin;
> +
> +  /* Percent of image height used as ascent.  A value of
> +     CENTERED_IMAGE_ASCENT means draw the image centered on the
> +     line.  */
> +  int ascent;
> +#define DEFAULT_IMAGE_ASCENT 50
> +#define CENTERED_IMAGE_ASCENT -1
> +};

I'm not sure I understand the need for introducing this new struct.
For starters, it repeats some of the fields we have already in other
structures.  Can you explain why you needed this?  Why not store the
results of calculating the size (in calc_image_spec) in the original
structure returned by IMAGE_FROM_ID, for example?

One reason to avoid this new struct is that it makes struct glyph,
struct glyph_string, and struct it larger, which will make the display
code slower, especially if the larger structs cause spilling of CPU
caches.  The display engine shuffles these structures to and fro in
many places, so they should be as small as possible.

> @@ -5213,17 +5210,8 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p
>      }
>  
>  #ifdef HAVE_WINDOW_SYSTEM
> -  if (it.what == IT_IMAGE)
> -    {
> -      /* Note that this ignores images that are fringe bitmaps,
> -	 because their image ID is zero, and so IMAGE_OPT_FROM_ID will
> -	 return NULL.  This is okay, since fringe bitmaps are not
> -	 displayed in the text area, and so are never the object we
> -	 are interested in.  */
> -      img = IMAGE_OPT_FROM_ID (it.f, it.image_id);
> -      if (img && !NILP (img->spec))
> -	*object = img->spec;
> -    }
> +  if (it.what == IT_IMAGE && !NILP (it.object))
> +    *object = it.object;
>  #endif

This doesn't look right.  The original code puts in *OBJECT the Lisp
spec of the image, whereas you put there the object being iterated by
struct it, which is a buffer or a Lisp string.  Please make sure you
test the functions that call buffer_posn_from_coords, as part of your
testing, such as (AFAIR) posn-at-x-y, and verify that the info they
return is unchanged, when the coordinates are on an image.

> +      /* FIXME: Surely there's a better way to do this?  */
> +      if (!EQ (property, QCwidth)
> +          && !EQ (property, QCheight)
> +          && !EQ (property, QCmax_width)
> +          && !EQ (property, QCmax_height)
> +          && !EQ (property, QCscale)
> +          && !EQ (property, QCmargin)
> +          && !EQ (property, QCascent)
> +          && !EQ (property, QCrelief))
> +        cache_spec = Fcons (property, Fcons (value, cache_spec));

Why did you think there was something wrong with this code?

One possible alternative would be not to cons a new image spec without
these attributes, but compare specs ignoring these attributes instead
of using EQ.  Did you consider this alternative?

> +/* Compute the desired size of an image with native size WIDTH x HEIGHT.
> +   Use SPEC to deduce the size.  Store the desired size into
> +   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
> +static void
> +compute_image_size (size_t width, size_t height,
> +		    Lisp_Object spec,
> +		    int *d_width, int *d_height)
> +{
> +  Lisp_Object value;
> +  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
> +  double scale = 1;
> +
> +  value = image_spec_value (spec, QCscale, NULL);
> +  if (NUMBERP (value))
> +    scale = XFLOATINT (value);
> +
> +  value = image_spec_value (spec, QCmax_width, NULL);
> +  if (FIXNATP (value))
> +    max_width = min (XFIXNAT (value), INT_MAX);
> +
> +  value = image_spec_value (spec, QCmax_height, NULL);
> +  if (FIXNATP (value))
> +    max_height = min (XFIXNAT (value), INT_MAX);
> +
> +  /* If width and/or height is set in the display spec assume we want
> +     to scale to those values.  If either h or w is unspecified, the
> +     unspecified should be calculated from the specified to preserve
> +     aspect ratio.  */
> +  value = image_spec_value (spec, QCwidth, NULL);
> +  if (FIXNATP (value))
> +    {
> +      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
> +      /* :width overrides :max-width. */
> +      max_width = -1;
> +    }
> +
> +  value = image_spec_value (spec, QCheight, NULL);
> +  if (FIXNATP (value))
> +    {
> +      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
> +      /* :height overrides :max-height. */
> +      max_height = -1;
> +    }
> +
> +  /* If we have both width/height set explicitly, we skip past all the
> +     aspect ratio-preserving computations below. */
> +  if (desired_width != -1 && desired_height != -1)
> +    goto out;

This avoids the unnecessary calculations too late, IMO.  Since I
expect most images to not require resizing, I suggest to make that
frequent case as fast as it is today, which means avoid any
unnecessary lookups in the image spec.  As a minimum, you don't need
to look up :max-width and :max-height.  Bonus points if you avoid
calling this function altogether when the image needs no resizing.

Re: Linking to ImageMagick by default

[Alan Third]

On Fri, Dec 28, 2018 at 10:12:35AM +0200, Eli Zaretskii wrote:
> > Date: Thu, 27 Dec 2018 13:11:05 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> >
> > I think that during redisplay something must be looking at the image
> > and deciding it hasn’t changed, but I can’t find anything like that.
> 
> My first suspicion is that redisplay doesn't even look at the image,
> because nothing tells it that it should.  IOW, it's a side effect of
> redisplay optimizations that should be disabled in this case.

Yes, I finally worked this out today. Like you say, the redisplay is a
side‐effect, but of flushing an image from the cache. I’ve added a
(force-window-update) to image--change-size and that’s fixed it.

I can’t believe I spent so long on that.

The docstring for image-flush looks like this:

       doc: /* Flush the image with specification SPEC on frame FRAME.
This removes the image from the Emacs image cache.  If SPEC specifies
an image file, the next redisplay of this image will read from the
current contents of that file.

FRAME nil or omitted means use the selected frame.
FRAME t means refresh the image on all frames.  */)

Should it mention that it will garbage the frame and therefore cause a
redisplay?

> > +/* Struct for use by display functions.  Contains the desired size of
> > +   the image, derived from the display properties.  */
> > +
> > +struct image_spec
> > +{
> > +  ptrdiff_t image_id;
> > +  int width;
> > +  int height;
> > +
> > +  /* Relief to draw around the image.  */
> > +  int relief;
> > +
> > +  /* Optional margins around the image.  This includes the relief.  */
> > +  int hmargin, vmargin;
> > +
> > +  /* Percent of image height used as ascent.  A value of
> > +     CENTERED_IMAGE_ASCENT means draw the image centered on the
> > +     line.  */
> > +  int ascent;
> > +#define DEFAULT_IMAGE_ASCENT 50
> > +#define CENTERED_IMAGE_ASCENT -1
> > +};
> 
> I'm not sure I understand the need for introducing this new struct.
> For starters, it repeats some of the fields we have already in other
> structures.  Can you explain why you needed this?  Why not store the
> results of calculating the size (in calc_image_spec) in the original
> structure returned by IMAGE_FROM_ID, for example?
> 
> One reason to avoid this new struct is that it makes struct glyph,
> struct glyph_string, and struct it larger, which will make the display
> code slower, especially if the larger structs cause spilling of CPU
> caches.  The display engine shuffles these structures to and fro in
> many places, so they should be as small as possible.

I added this struct to separate the image pixel data from the size
information. The idea is to avoid loading and holding multiple copies
of the same image in RAM when we want to display at different sizes.

I considered making the reference in struct it and others a pointer to
struct image_spec because it is noticeably larger than what it’s
replacing, but I’m unsure of the lifetime and when I would have to
free it and so on.

> > @@ -5213,17 +5210,8 @@ buffer_posn_from_coords (struct window *w, int *x, int *y, struct display_pos *p
> >      }
> >  
> >  #ifdef HAVE_WINDOW_SYSTEM
> > -  if (it.what == IT_IMAGE)
> > -    {
> > -      /* Note that this ignores images that are fringe bitmaps,
> > -	 because their image ID is zero, and so IMAGE_OPT_FROM_ID will
> > -	 return NULL.  This is okay, since fringe bitmaps are not
> > -	 displayed in the text area, and so are never the object we
> > -	 are interested in.  */
> > -      img = IMAGE_OPT_FROM_ID (it.f, it.image_id);
> > -      if (img && !NILP (img->spec))
> > -	*object = img->spec;
> > -    }
> > +  if (it.what == IT_IMAGE && !NILP (it.object))
> > +    *object = it.object;
> >  #endif
> 
> This doesn't look right.  The original code puts in *OBJECT the Lisp
> spec of the image, whereas you put there the object being iterated by
> struct it, which is a buffer or a Lisp string.  Please make sure you
> test the functions that call buffer_posn_from_coords, as part of your
> testing, such as (AFAIR) posn-at-x-y, and verify that the info they
> return is unchanged, when the coordinates are on an image.

You’re right. Thanks.

> > +      /* FIXME: Surely there's a better way to do this?  */
> > +      if (!EQ (property, QCwidth)
> > +          && !EQ (property, QCheight)
> > +          && !EQ (property, QCmax_width)
> > +          && !EQ (property, QCmax_height)
> > +          && !EQ (property, QCscale)
> > +          && !EQ (property, QCmargin)
> > +          && !EQ (property, QCascent)
> > +          && !EQ (property, QCrelief))
> > +        cache_spec = Fcons (property, Fcons (value, cache_spec));
> 
> Why did you think there was something wrong with this code?

I just thought there might be a neater way to do it. To be honest, I
think when I wrote that message the code was full of XCARs which I
subsequently got rid of. But if you’re happy with it then I’m happy
with it.

> One possible alternative would be not to cons a new image spec without
> these attributes, but compare specs ignoring these attributes instead
> of using EQ.  Did you consider this alternative?

It might be more efficient. I’m not sure which option would be better.

> > +/* Compute the desired size of an image with native size WIDTH x HEIGHT.
> > +   Use SPEC to deduce the size.  Store the desired size into
> > +   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
> > +static void
> > +compute_image_size (size_t width, size_t height,
> > +		    Lisp_Object spec,
> > +		    int *d_width, int *d_height)
> > +{
> > +  Lisp_Object value;
> > +  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
> > +  double scale = 1;
> > +
> > +  value = image_spec_value (spec, QCscale, NULL);
> > +  if (NUMBERP (value))
> > +    scale = XFLOATINT (value);
> > +
> > +  value = image_spec_value (spec, QCmax_width, NULL);
> > +  if (FIXNATP (value))
> > +    max_width = min (XFIXNAT (value), INT_MAX);
> > +
> > +  value = image_spec_value (spec, QCmax_height, NULL);
> > +  if (FIXNATP (value))
> > +    max_height = min (XFIXNAT (value), INT_MAX);
> > +
> > +  /* If width and/or height is set in the display spec assume we want
> > +     to scale to those values.  If either h or w is unspecified, the
> > +     unspecified should be calculated from the specified to preserve
> > +     aspect ratio.  */
> > +  value = image_spec_value (spec, QCwidth, NULL);
> > +  if (FIXNATP (value))
> > +    {
> > +      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
> > +      /* :width overrides :max-width. */
> > +      max_width = -1;
> > +    }
> > +
> > +  value = image_spec_value (spec, QCheight, NULL);
> > +  if (FIXNATP (value))
> > +    {
> > +      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
> > +      /* :height overrides :max-height. */
> > +      max_height = -1;
> > +    }
> > +
> > +  /* If we have both width/height set explicitly, we skip past all the
> > +     aspect ratio-preserving computations below. */
> > +  if (desired_width != -1 && desired_height != -1)
> > +    goto out;
> 
> This avoids the unnecessary calculations too late, IMO.  Since I
> expect most images to not require resizing, I suggest to make that
> frequent case as fast as it is today, which means avoid any
> unnecessary lookups in the image spec.  As a minimum, you don't need
> to look up :max-width and :max-height.  Bonus points if you avoid
> calling this function altogether when the image needs no resizing.

I’m not sure how to check whether the image needs resizing without
doing the lookups for :width, :height, :max-width, etc. At the moment
it’s avoided simply by not using :type imagemagick.

It should be possible to avoid all this for fringe bitmaps, though.
The image IDs for them are negative numbers, so there’s no
point doing the calculations if id < 0.
-- 
Alan Third

Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Fri, 28 Dec 2018 21:21:15 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> The docstring for image-flush looks like this:
> 
>        doc: /* Flush the image with specification SPEC on frame FRAME.
> This removes the image from the Emacs image cache.  If SPEC specifies
> an image file, the next redisplay of this image will read from the
> current contents of that file.
> 
> FRAME nil or omitted means use the selected frame.
> FRAME t means refresh the image on all frames.  */)
> 
> Should it mention that it will garbage the frame and therefore cause a
> redisplay?

I'm okay with saying that this causes a complete redisplay of FRAME
(or all frames if FRAME is t), but the bit about making the frames
garbaged is an implementation detail, so I don't think it belongs to
the doc string.

> > > +struct image_spec
> > > +{
> > > +  ptrdiff_t image_id;
> > > +  int width;
> > > +  int height;
> > > +
> > > +  /* Relief to draw around the image.  */
> > > +  int relief;
> > > +
> > > +  /* Optional margins around the image.  This includes the relief.  */
> > > +  int hmargin, vmargin;
> > > +
> > > +  /* Percent of image height used as ascent.  A value of
> > > +     CENTERED_IMAGE_ASCENT means draw the image centered on the
> > > +     line.  */
> > > +  int ascent;
> > > +#define DEFAULT_IMAGE_ASCENT 50
> > > +#define CENTERED_IMAGE_ASCENT -1
> > > +};
> > 
> > I'm not sure I understand the need for introducing this new struct.
> > For starters, it repeats some of the fields we have already in other
> > structures.  Can you explain why you needed this?  Why not store the
> > results of calculating the size (in calc_image_spec) in the original
> > structure returned by IMAGE_FROM_ID, for example?
> > 
> > One reason to avoid this new struct is that it makes struct glyph,
> > struct glyph_string, and struct it larger, which will make the display
> > code slower, especially if the larger structs cause spilling of CPU
> > caches.  The display engine shuffles these structures to and fro in
> > many places, so they should be as small as possible.
> 
> I added this struct to separate the image pixel data from the size
> information. The idea is to avoid loading and holding multiple copies
> of the same image in RAM when we want to display at different sizes.

I think with my suggestion you will still hold only one copy: the last
one whose size was calculated.  AFAIU, the same happens with your
implementation, you just hold the last size separately.

> I considered making the reference in struct it and others a pointer to
> struct image_spec because it is noticeably larger than what it’s
> replacing, but I’m unsure of the lifetime and when I would have to
> free it and so on.

No, this is not a good idea, so you were right not to do that.

> > > +      /* FIXME: Surely there's a better way to do this?  */
> > > +      if (!EQ (property, QCwidth)
> > > +          && !EQ (property, QCheight)
> > > +          && !EQ (property, QCmax_width)
> > > +          && !EQ (property, QCmax_height)
> > > +          && !EQ (property, QCscale)
> > > +          && !EQ (property, QCmargin)
> > > +          && !EQ (property, QCascent)
> > > +          && !EQ (property, QCrelief))
> > > +        cache_spec = Fcons (property, Fcons (value, cache_spec));
> > 
> > Why did you think there was something wrong with this code?
> 
> I just thought there might be a neater way to do it. To be honest, I
> think when I wrote that message the code was full of XCARs which I
> subsequently got rid of. But if you’re happy with it then I’m happy
> with it.

I see nothing wrong with a series of equality checks, we do that
elsewhere, albeit not in a loop.

> > One possible alternative would be not to cons a new image spec without
> > these attributes, but compare specs ignoring these attributes instead
> > of using EQ.  Did you consider this alternative?
> 
> It might be more efficient. I’m not sure which option would be better.

To my eyes, consing a list that will be used only once is less
elegant, and will also cause more GC.  So maybe try the alternative,
and see if it works well.

> 
> > > +/* Compute the desired size of an image with native size WIDTH x HEIGHT.
> > > +   Use SPEC to deduce the size.  Store the desired size into
> > > +   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
> > > +static void
> > > +compute_image_size (size_t width, size_t height,
> > > +		    Lisp_Object spec,
> > > +		    int *d_width, int *d_height)
> > > +{
> > > +  Lisp_Object value;
> > > +  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
> > > +  double scale = 1;
> > > +
> > > +  value = image_spec_value (spec, QCscale, NULL);
> > > +  if (NUMBERP (value))
> > > +    scale = XFLOATINT (value);
> > > +
> > > +  value = image_spec_value (spec, QCmax_width, NULL);
> > > +  if (FIXNATP (value))
> > > +    max_width = min (XFIXNAT (value), INT_MAX);
> > > +
> > > +  value = image_spec_value (spec, QCmax_height, NULL);
> > > +  if (FIXNATP (value))
> > > +    max_height = min (XFIXNAT (value), INT_MAX);
> > > +
> > > +  /* If width and/or height is set in the display spec assume we want
> > > +     to scale to those values.  If either h or w is unspecified, the
> > > +     unspecified should be calculated from the specified to preserve
> > > +     aspect ratio.  */
> > > +  value = image_spec_value (spec, QCwidth, NULL);
> > > +  if (FIXNATP (value))
> > > +    {
> > > +      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
> > > +      /* :width overrides :max-width. */
> > > +      max_width = -1;
> > > +    }
> > > +
> > > +  value = image_spec_value (spec, QCheight, NULL);
> > > +  if (FIXNATP (value))
> > > +    {
> > > +      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
> > > +      /* :height overrides :max-height. */
> > > +      max_height = -1;
> > > +    }
> > > +
> > > +  /* If we have both width/height set explicitly, we skip past all the
> > > +     aspect ratio-preserving computations below. */
> > > +  if (desired_width != -1 && desired_height != -1)
> > > +    goto out;
> > 
> > This avoids the unnecessary calculations too late, IMO.  Since I
> > expect most images to not require resizing, I suggest to make that
> > frequent case as fast as it is today, which means avoid any
> > unnecessary lookups in the image spec.  As a minimum, you don't need
> > to look up :max-width and :max-height.  Bonus points if you avoid
> > calling this function altogether when the image needs no resizing.
> 
> I’m not sure how to check whether the image needs resizing without
> doing the lookups for :width, :height, :max-width, etc.

Maybe I'm misreading the code, but it looks to me you only need
:scale, :width, and :height.  Am I missing something?

> At the moment it’s avoided simply by not using :type imagemagick.

If the caller might know whether resizing is needed, perhaps we should
pass an extra flag argument to that effect.

> It should be possible to avoid all this for fringe bitmaps, though.
> The image IDs for them are negative numbers, so there’s no
> point doing the calculations if id < 0.

Definitely.

Thanks.

Re: Linking to ImageMagick by default

[Juri Linkov]

>> > This avoids the unnecessary calculations too late, IMO.  Since I
>> > expect most images to not require resizing, I suggest to make that
>> > frequent case as fast as it is today, which means avoid any
>> > unnecessary lookups in the image spec.  As a minimum, you don't need
>> > to look up :max-width and :max-height.  Bonus points if you avoid
>> > calling this function altogether when the image needs no resizing.
>>
>> I’m not sure how to check whether the image needs resizing without
>> doing the lookups for :width, :height, :max-width, etc.
>
> Maybe I'm misreading the code, but it looks to me you only need
> :scale, :width, and :height.  Am I missing something?

It's easier in application code just to set :max-width/:max-height
to the window dimensions and let the core to calculate its scale.

Re: Linking to ImageMagick by default

[Alan Third]

On Sat, Dec 29, 2018 at 08:56:29AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 28 Dec 2018 21:21:15 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > > One reason to avoid this new struct is that it makes struct glyph,
> > > struct glyph_string, and struct it larger, which will make the display
> > > code slower, especially if the larger structs cause spilling of CPU
> > > caches.  The display engine shuffles these structures to and fro in
> > > many places, so they should be as small as possible.
> > 
> > I added this struct to separate the image pixel data from the size
> > information. The idea is to avoid loading and holding multiple copies
> > of the same image in RAM when we want to display at different sizes.
> 
> I think with my suggestion you will still hold only one copy: the last
> one whose size was calculated.  AFAIU, the same happens with your
> implementation, you just hold the last size separately.

If someone were to do something like this

  (insert-image (create-image "file.png" nil nil :width 100))
  (insert-image (create-image "file.png" nil nil :width 200))

We need to be able to differentiate between the two images. The
current way is just to load file.png twice, and hold the two different
sized images separately.

My method loads it once, then makes redisplay work out the sizes
before it displays them.

I believe your method loads it once, then updates the image struct
on the fly between the two sizes as and when it needs them. I’m not
sure how that would work.

To be perfectly honest, I’m not entirely sold on the idea that we need
to hold only one copy, it just seems like a waste to load and store
the image multiple times. But, Emacs is not a graphics application and
it seems unlikely that displaying multiple, slightly different copies
of an image is a major use‐case.

I may just scrap all this and go back to the basics and provide a
resizing system that just loads and stores each image separately, like
the current setup. If we want we can revisit the caching and so on later.

(A side note: I believe the NS toolkits provide transparent image
caching anyway, so this doesn’t really matter there.)

> > I’m not sure how to check whether the image needs resizing without
> > doing the lookups for :width, :height, :max-width, etc.
> 
> Maybe I'm misreading the code, but it looks to me you only need
> :scale, :width, and :height.  Am I missing something?

As Juri also pointed out it’s often preferred to set :max-width or
:max-height purely to make sure an image fits in a certain space
without having to calculate the required width and height to maintain
the aspect ratio and so on.

> > At the moment it’s avoided simply by not using :type imagemagick.
> 
> If the caller might know whether resizing is needed, perhaps we should
> pass an extra flag argument to that effect.

Perhaps the best solution is to parse the lisp image spec in a
different way, so it only runs through the plist once and notes down
the settings, instead of the current method of running through it
every time? We’d still have to do some size checking, but I suspect
the time saved on parsing the image spec would be far larger than
that.

FWIW, the time spent throwing around pixel data is, as far as I can
tell, far greater than the time spent parsing the spec and calculating
the sizes.

-- 
Alan Third

[PATCH] Add native image scaling

[Alan Third]

* configure.ac: Test for XRender outside of xft checks.
* src/Makefile.in (XRENDER_LIBS): List XRender libs separately from
xft libs.
* lisp/image.el (image--get-imagemagick-and-warn): Allow resizing if
native scaling is available.
* src/dispextern.h: Add XRender and image scaling stuff.
(struct image): Add XRender Pictures.
* src/image.c (x_create_bitmap_mask):
(image_create_x_image_and_pixmap): Handle XRender Picture.
(scale_image_size):
(compute_image_size): Make available when any form of scaling is
enabled.
(x_set_image_size): New function.
(lookup_image): Set image size.
(x_create_x_image_and_pixmap): Create XRender Picture when necessary.
(x_put_x_image): Handle the case where desired size != actual size.
(free_image): Free XRender Pictures.
(Fimage_scaling_p): New function.
(syms_of_image): Add image-scaling-p.
* src/nsimage.m (ns_load_image): Remove NS specific resizing.
([EmacsImage setSizeFromSpec:]): Remove method.
(ns_image_set_size): New function.
* src/nsterm.m (ns_draw_fringe_bitmap): Cocoa and GNUstep both have
the same compositing functions, so remove unnecessary difference.
* src/xterm.c (x_composite_image): New function.
(x_draw_image_foreground): Use new x_composite_image function.
---

This is a completely rewritten version that doesn't do anything
different with the image cache. I think this is pretty complete as it
is, excepting Windows support. I don't know how to add Windows support
to match the way it's done in NS and X, however if we can't resize the
images up-front, it should be possible to add some size info to the
image struct and do the resize on display.

I'm still unsure about image.el calling image-flush on every image
resize, but I have to assume it's in there for a reason.

 configure.ac     |  14 ++-
 etc/NEWS         |   6 +
 lisp/image.el    |   4 +-
 src/Makefile.in  |   3 +-
 src/dispextern.h |  13 ++
 src/image.c      | 314 ++++++++++++++++++++++++++++++++---------------
 src/nsimage.m    |  68 +---------
 src/nsterm.h     |   2 +-
 src/nsterm.m     |   8 --
 src/xterm.c      |  58 +++++++--
 10 files changed, 298 insertions(+), 192 deletions(-)

diff --git a/configure.ac b/configure.ac
index 91fa417308..0c59fe0619 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3241,6 +3241,17 @@ AC_DEFUN
   CFLAGS=$late_CFLAGS
 fi
 
+# Check for XRender
+HAVE_XRENDER=no
+if test "${HAVE_X11}" = "yes"; then
+  AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
+  if test $HAVE_XRENDER = yes; then
+    XRENDER_LIBS="-lXrender"
+    AC_SUBST(XRENDER_LIBS)
+    AC_DEFINE([HAVE_XRENDER], 1, [Define to 1 if XRender is available.])
+  fi
+fi
+
 ### Start of font-backend (under any platform) section.
 # (nothing here yet -- this is a placeholder)
 ### End of font-backend (under any platform) section.
@@ -3263,15 +3274,12 @@ AC_DEFUN
       EMACS_CHECK_MODULES([XFT], [xft >= 0.13.0], [], [HAVE_XFT=no])
       ## Because xterm.c uses XRenderQueryExtension when XFT is
       ## enabled, we also need to link to -lXrender.
-      HAVE_XRENDER=no
-      AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
       if test "$HAVE_XFT" != no && test "$HAVE_XRENDER" != no; then
 	OLD_CPPFLAGS="$CPPFLAGS"
 	OLD_CFLAGS="$CFLAGS"
 	OLD_LIBS="$LIBS"
 	CPPFLAGS="$CPPFLAGS $XFT_CFLAGS"
 	CFLAGS="$CFLAGS $XFT_CFLAGS"
-	XFT_LIBS="-lXrender $XFT_LIBS"
 	LIBS="$XFT_LIBS $LIBS"
 	AC_CHECK_HEADER(X11/Xft/Xft.h,
 	  AC_CHECK_LIB(Xft, XftFontOpen, HAVE_XFT=yes, , $XFT_LIBS) , ,
diff --git a/etc/NEWS b/etc/NEWS
index 75e2c1bf98..69e14773a7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1442,6 +1442,12 @@ that is non-nil, it will look for a file name handler for the current
 buffer's 'default-directory' and invoke that file name handler to make
 the process.  That way 'make-process' can start remote processes.
 
++++
+** Emacs now supports resizing images without ImageMagick on X window
+systems where the XRender extension is available, and on the NS port.
+The new function 'image-scaling-p' can be used to test whether any
+given frame supports resizing.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/lisp/image.el b/lisp/image.el
index 5727d8fbce..2e84e47b5c 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -982,8 +982,8 @@ image--get-image
     image))
 
 (defun image--get-imagemagick-and-warn ()
-  (unless (or (fboundp 'imagemagick-types) (featurep 'ns))
-    (error "Cannot rescale images without ImageMagick support"))
+  (unless (or (fboundp 'imagemagick-types) (image-scaling-p))
+    (error "Cannot rescale images on this terminal"))
   (let ((image (image--get-image)))
     (image-flush image)
     (when (fboundp 'imagemagick-types)
diff --git a/src/Makefile.in b/src/Makefile.in
index e9831e9299..f409ed4db2 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -127,7 +127,8 @@ LIBIMAGE=
 
 XCB_LIBS=@XCB_LIBS@
 XFT_LIBS=@XFT_LIBS@
-LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS)
+XRENDER_LIBS=@XRENDER_LIBS@
+LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS) $(XRENDER_LIBS)
 
 FONTCONFIG_CFLAGS = @FONTCONFIG_CFLAGS@
 FONTCONFIG_LIBS = @FONTCONFIG_LIBS@
diff --git a/src/dispextern.h b/src/dispextern.h
index 5774e3e951..b064875ac4 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -31,6 +31,9 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/Intrinsic.h>
 #endif /* USE_X_TOOLKIT */
 
+#ifdef HAVE_XRENDER
+#include <X11/extensions/Xrender.h>
+#endif
 #else /* !HAVE_X_WINDOWS */
 
 /* X-related stuff used by non-X gui code.  */
@@ -2935,6 +2938,11 @@ struct redisplay_interface
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+#if defined (HAVE_X_WINDOWS) && defined (HAVE_XRENDER) \
+  || defined (HAVE_NS)
+#define HAVE_NATIVE_SCALING
+#endif
+
 /* Structure describing an image.  Specific image formats like XBM are
    converted into this form, so that display only has to deal with
    this type of image.  */
@@ -2958,6 +2966,11 @@ struct image
      and the latter is outdated.  NULL means the X image has been
      synchronized to Pixmap.  */
   XImagePtr ximg, mask_img;
+
+#ifdef HAVE_NATIVE_SCALING
+  /* Picture versions of pixmap and mask for compositing.  */
+  Picture picture, mask_picture;
+#endif
 #endif
 
   /* Colors allocated for this image, if any.  Allocated via xmalloc.  */
diff --git a/src/image.c b/src/image.c
index 87e0c071ee..9c78755bbc 100644
--- a/src/image.c
+++ b/src/image.c
@@ -408,8 +408,13 @@ x_destroy_all_bitmaps (Display_Info *dpyinfo)
   dpyinfo->bitmaps_last = 0;
 }
 
+#ifdef HAVE_XRENDER
+static bool x_create_x_image_and_pixmap (struct frame *, int, int, int,
+					 XImagePtr *, Pixmap *, Picture *);
+#else
 static bool x_create_x_image_and_pixmap (struct frame *, int, int, int,
 					 XImagePtr *, Pixmap *);
+#endif
 static void x_destroy_x_image (XImagePtr ximg);
 
 #ifdef HAVE_NTGUI
@@ -472,7 +477,11 @@ x_create_bitmap_mask (struct frame *f, ptrdiff_t id)
       return;
     }
 
-  result = x_create_x_image_and_pixmap (f, width, height, 1, &mask_img, &mask);
+  result = x_create_x_image_and_pixmap (f, width, height, 1, &mask_img, &mask
+#ifdef HAVE_XRENDER
+                                        , NULL
+#endif
+                                        );
 
   unblock_input ();
   if (!result)
@@ -1011,6 +1020,13 @@ free_image (struct frame *f, struct image *img)
 
       c->images[img->id] = NULL;
 
+#ifdef HAVE_XRENDER
+      if (img->picture)
+        XRenderFreePicture (FRAME_X_DISPLAY (f), img->picture);
+      if (img->mask_picture)
+        XRenderFreePicture (FRAME_X_DISPLAY (f), img->mask_picture);
+#endif
+
       /* Windows NT redefines 'free', but in this file, we need to
          avoid the redefinition.  */
 #ifdef WINDOWSNT
@@ -1747,6 +1763,147 @@ postprocess_image (struct frame *f, struct image *img)
     }
 }
 
+#if defined (HAVE_IMAGEMAGICK) || defined (HAVE_NATIVE_SCALING)
+/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
+   safely rounded and clipped to int range.  */
+
+static int
+scale_image_size (int size, size_t divisor, size_t multiplier)
+{
+  if (divisor != 0)
+    {
+      double s = size;
+      double scaled = s * multiplier / divisor + 0.5;
+      if (scaled < INT_MAX)
+	return scaled;
+    }
+  return INT_MAX;
+}
+
+/* Compute the desired size of an image with native size WIDTH x HEIGHT.
+   Use SPEC to deduce the size.  Store the desired size into
+   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
+static void
+compute_image_size (size_t width, size_t height,
+		    Lisp_Object spec,
+		    int *d_width, int *d_height)
+{
+  Lisp_Object value;
+  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
+  double scale = 1;
+
+  value = image_spec_value (spec, QCscale, NULL);
+  if (NUMBERP (value))
+    scale = XFLOATINT (value);
+
+  value = image_spec_value (spec, QCmax_width, NULL);
+  if (FIXNATP (value))
+    max_width = min (XFIXNAT (value), INT_MAX);
+
+  value = image_spec_value (spec, QCmax_height, NULL);
+  if (FIXNATP (value))
+    max_height = min (XFIXNAT (value), INT_MAX);
+
+  /* If width and/or height is set in the display spec assume we want
+     to scale to those values.  If either h or w is unspecified, the
+     unspecified should be calculated from the specified to preserve
+     aspect ratio.  */
+  value = image_spec_value (spec, QCwidth, NULL);
+  if (FIXNATP (value))
+    {
+      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :width overrides :max-width. */
+      max_width = -1;
+    }
+
+  value = image_spec_value (spec, QCheight, NULL);
+  if (FIXNATP (value))
+    {
+      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :height overrides :max-height. */
+      max_height = -1;
+    }
+
+  /* If we have both width/height set explicitly, we skip past all the
+     aspect ratio-preserving computations below. */
+  if (desired_width != -1 && desired_height != -1)
+    goto out;
+
+  width = width * scale;
+  height = height * scale;
+
+  if (desired_width != -1)
+    /* Width known, calculate height. */
+    desired_height = scale_image_size (desired_width, width, height);
+  else if (desired_height != -1)
+    /* Height known, calculate width. */
+    desired_width = scale_image_size (desired_height, height, width);
+  else
+    {
+      desired_width = width;
+      desired_height = height;
+    }
+
+  if (max_width != -1 && desired_width > max_width)
+    {
+      /* The image is wider than :max-width. */
+      desired_width = max_width;
+      desired_height = scale_image_size (desired_width, width, height);
+    }
+
+  if (max_height != -1 && desired_height > max_height)
+    {
+      /* The image is higher than :max-height. */
+      desired_height = max_height;
+      desired_width = scale_image_size (desired_height, height, width);
+    }
+
+ out:
+  *d_width = desired_width;
+  *d_height = desired_height;
+}
+
+#ifdef HAVE_NATIVE_SCALING
+static void
+x_set_image_size (struct frame *f, struct image *img)
+{
+#ifdef HAVE_IMAGEMAGICK
+  /* ImageMagick images are already the correct size.  */
+  if (!imagemagick_image_p (img->spec))
+#endif
+    {
+      int width, height;
+
+      compute_image_size (img->width, img->height, img->spec, &width, &height);
+
+#ifdef HAVE_NS
+      ns_image_set_size (img->pixmap, width, height);
+      img->width = width;
+      img->height = height;
+#endif
+
+#ifdef HAVE_XRENDER
+      if (img->picture)
+      {
+        double xscale = (double) img->width/width;
+        double yscale = (double) img->height/height;
+
+        XTransform tmat = {{{XDoubleToFixed (xscale), XDoubleToFixed (0), XDoubleToFixed (0)},
+                            {XDoubleToFixed (0), XDoubleToFixed (yscale), XDoubleToFixed (0)},
+                            {XDoubleToFixed (0), XDoubleToFixed (0), XDoubleToFixed (1)}}};
+
+        XRenderSetPictureFilter (FRAME_X_DISPLAY (f), img->picture, FilterBest, 0, 0);
+        XRenderSetPictureTransform (FRAME_X_DISPLAY (f), img->picture, &tmat);
+
+        img->width = width;
+        img->height = height;
+      }
+#endif
+    }
+}
+#endif
+#endif /* HAVE_IMAGEMAGICK || HAVE_XRENDER || HAVE_NS  */
+
 
 /* Return the id of image with Lisp specification SPEC on frame F.
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
@@ -1802,6 +1959,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
 	     `:background COLOR'.  */
 	  Lisp_Object ascent, margin, relief, bg;
 	  int relief_bound;
+#ifdef HAVE_NATIVE_SCALING
+          x_set_image_size (f, img);
+#endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
 	  if (FIXNUMP (ascent))
@@ -1975,13 +2135,21 @@ x_check_image_size (XImagePtr ximg, int width, int height)
    should indicate the bit depth of the image.  */
 
 static bool
+#ifdef HAVE_XRENDER
+x_create_x_image_and_pixmap (struct frame *f, int width, int height, int depth,
+			     XImagePtr *ximg, Pixmap *pixmap, Picture *picture)
+#else
 x_create_x_image_and_pixmap (struct frame *f, int width, int height, int depth,
 			     XImagePtr *ximg, Pixmap *pixmap)
+#endif
 {
 #ifdef HAVE_X_WINDOWS
   Display *display = FRAME_X_DISPLAY (f);
   Drawable drawable = FRAME_X_DRAWABLE (f);
   Screen *screen = FRAME_X_SCREEN (f);
+#ifdef HAVE_XRENDER
+  int event_basep, error_basep;
+#endif
 
   eassert (input_blocked_p ());
 
@@ -2018,6 +2186,21 @@ x_create_x_image_and_pixmap (struct frame *f, int width, int height, int depth,
       return 0;
     }
 
+#ifdef HAVE_XRENDER
+  if (picture && XRenderQueryExtension (display, &event_basep, &error_basep))
+    {
+      XRenderPictFormat *format;
+      XRenderPictureAttributes attr;
+
+      /* FIXME: Do we need to handle all possible bit depths?  */
+      format = XRenderFindStandardFormat (display,
+                                          depth > 24 ? PictStandardARGB32
+                                          : depth > 8 ? PictStandardRGB24
+                                          : PictStandardA8);
+      *picture = XRenderCreatePicture (display, *pixmap, format, 0, &attr);
+    }
+#endif
+
   return 1;
 #endif /* HAVE_X_WINDOWS */
 
@@ -2163,7 +2346,8 @@ x_put_x_image (struct frame *f, XImagePtr ximg, Pixmap pixmap, int width, int he
 
   eassert (input_blocked_p ());
   gc = XCreateGC (FRAME_X_DISPLAY (f), pixmap, 0, NULL);
-  XPutImage (FRAME_X_DISPLAY (f), pixmap, gc, ximg, 0, 0, 0, 0, width, height);
+  XPutImage (FRAME_X_DISPLAY (f), pixmap, gc, ximg, 0, 0, 0, 0,
+             ximg->width, ximg->height);
   XFreeGC (FRAME_X_DISPLAY (f), gc);
 #endif /* HAVE_X_WINDOWS */
 
@@ -2192,7 +2376,11 @@ image_create_x_image_and_pixmap (struct frame *f, struct image *img,
   eassert ((!mask_p ? img->pixmap : img->mask) == NO_PIXMAP);
 
   return x_create_x_image_and_pixmap (f, width, height, depth, ximg,
-				      !mask_p ? &img->pixmap : &img->mask);
+				      !mask_p ? &img->pixmap : &img->mask
+#ifdef HAVE_XRENDER
+                                      , !mask_p ? &img->picture : &img->mask_picture
+#endif
+                                      );
 }
 
 /* Put X image XIMG into image IMG on frame F, as a mask if and only
@@ -8101,105 +8289,6 @@ gif_load (struct frame *f, struct image *img)
 				 ImageMagick
 ***********************************************************************/
 
-/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
-   safely rounded and clipped to int range.  */
-
-static int
-scale_image_size (int size, size_t divisor, size_t multiplier)
-{
-  if (divisor != 0)
-    {
-      double s = size;
-      double scaled = s * multiplier / divisor + 0.5;
-      if (scaled < INT_MAX)
-	return scaled;
-    }
-  return INT_MAX;
-}
-
-/* Compute the desired size of an image with native size WIDTH x HEIGHT.
-   Use SPEC to deduce the size.  Store the desired size into
-   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
-static void
-compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
-		    int *d_width, int *d_height)
-{
-  Lisp_Object value;
-  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
-  double scale = 1;
-
-  value = image_spec_value (spec, QCscale, NULL);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value);
-
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
-
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
-
-  /* If width and/or height is set in the display spec assume we want
-     to scale to those values.  If either h or w is unspecified, the
-     unspecified should be calculated from the specified to preserve
-     aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
-    {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
-    {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  /* If we have both width/height set explicitly, we skip past all the
-     aspect ratio-preserving computations below. */
-  if (desired_width != -1 && desired_height != -1)
-    goto out;
-
-  width = width * scale;
-  height = height * scale;
-
-  if (desired_width != -1)
-    /* Width known, calculate height. */
-    desired_height = scale_image_size (desired_width, width, height);
-  else if (desired_height != -1)
-    /* Height known, calculate width. */
-    desired_width = scale_image_size (desired_height, height, width);
-  else
-    {
-      desired_width = width;
-      desired_height = height;
-    }
-
-  if (max_width != -1 && desired_width > max_width)
-    {
-      /* The image is wider than :max-width. */
-      desired_width = max_width;
-      desired_height = scale_image_size (desired_width, width, height);
-    }
-
-  if (max_height != -1 && desired_height > max_height)
-    {
-      /* The image is higher than :max-height. */
-      desired_height = max_height;
-      desired_width = scale_image_size (desired_height, height, width);
-    }
-
- out:
-  *d_width = desired_width;
-  *d_height = desired_height;
-}
-
 static bool imagemagick_image_p (Lisp_Object);
 static bool imagemagick_load (struct frame *, struct image *);
 static void imagemagick_clear_image (struct frame *, struct image *);
@@ -9816,6 +9905,25 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
 			    Initialization
  ***********************************************************************/
 
+DEFUN ("image-scaling-p", Fimage_scaling_p, Simage_scaling_p, 0, 1, 0,
+       doc: /* Test whether FRAME supports resizing images.
+Return t if FRAME supports native scaling, nil otherwise.  */)
+     (Lisp_Object frame)
+{
+#ifdef HAVE_NS
+  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;
+#endif
+
+  return Qnil;
+}
+
 DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
        doc: /* Initialize image library implementing image type TYPE.
 Return non-nil if TYPE is a supported image type.
@@ -10058,6 +10166,8 @@ non-numeric, there is no explicit limit on the size of images.  */);
   defsubr (&Slookup_image);
 #endif
 
+  defsubr (&Simage_scaling_p);
+
   DEFVAR_BOOL ("cross-disabled-images", cross_disabled_images,
     doc: /* Non-nil means always draw a cross over disabled images.
 Disabled images are those having a `:conversion disabled' property.
diff --git a/src/nsimage.m b/src/nsimage.m
index 7879c5891d..f16910de08 100644
--- a/src/nsimage.m
+++ b/src/nsimage.m
@@ -126,8 +126,6 @@ Updated by Christian Limpach (chris@nice.ch)
       eImg = temp;
     }
 
-  [eImg setSizeFromSpec:XCDR (img->spec)];
-
   size = [eImg size];
   img->width = size.width;
   img->height = size.height;
@@ -151,6 +149,12 @@ Updated by Christian Limpach (chris@nice.ch)
   return [(id)img size].height;
 }
 
+void
+ns_image_set_size (void *img, int width, int height)
+{
+  [(EmacsImage *)img setSize:NSMakeSize (width, height)];
+}
+
 unsigned long
 ns_get_pixel (void *img, int x, int y)
 {
@@ -524,66 +528,6 @@ - (BOOL)setFrame: (unsigned int) index
   return YES;
 }
 
-- (void)setSizeFromSpec: (Lisp_Object) spec
-{
-  NSSize size = [self size];
-  Lisp_Object value;
-  double scale = 1, aspect = size.width / size.height;
-  double width = -1, height = -1, max_width = -1, max_height = -1;
-
-  value = Fplist_get (spec, QCscale);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value) ;
-
-  value = Fplist_get (spec, QCmax_width);
-  if (NUMBERP (value))
-    max_width = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCmax_height);
-  if (NUMBERP (value))
-    max_height = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCwidth);
-  if (NUMBERP (value))
-    {
-      width = XFLOATINT (value) * scale;
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = Fplist_get (spec, QCheight);
-  if (NUMBERP (value))
-    {
-      height = XFLOATINT (value) * scale;
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  if (width <= 0 && height <= 0)
-    {
-      width = size.width * scale;
-      height = size.height * scale;
-    }
-  else if (width > 0 && height <= 0)
-      height = width / aspect;
-  else if (height > 0 && width <= 0)
-      width = height * aspect;
-
-  if (max_width > 0 && width > max_width)
-    {
-      width = max_width;
-      height = max_width / aspect;
-    }
-
-  if (max_height > 0 && height > max_height)
-    {
-      height = max_height;
-      width = max_height * aspect;
-    }
-
-  [self setSize:NSMakeSize(width, height)];
-}
-
 - (instancetype)rotate: (double)rotation
 {
   EmacsImage *new_image;
diff --git a/src/nsterm.h b/src/nsterm.h
index 089cbccbf0..78ce608554 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -648,7 +648,6 @@ typedef id instancetype;
 - (NSColor *)stippleMask;
 - (Lisp_Object)getMetadata;
 - (BOOL)setFrame: (unsigned int) index;
-- (void)setSizeFromSpec: (Lisp_Object) spec;
 - (instancetype)rotate: (double)rotation;
 @end
 
@@ -1197,6 +1196,7 @@ extern bool ns_load_image (struct frame *f, struct image *img,
 			   Lisp_Object spec_file, Lisp_Object spec_data);
 extern int ns_image_width (void *img);
 extern int ns_image_height (void *img);
+extern void ns_image_set_size (void *img, int width, int height);
 extern unsigned long ns_get_pixel (void *img, int x, int y);
 extern void ns_put_pixel (void *img, int x, int y, unsigned long argb);
 extern void ns_set_alpha (void *img, int x, int y, unsigned char a);
diff --git a/src/nsterm.m b/src/nsterm.m
index 016c044760..cbb2d2a5ce 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3121,7 +3121,6 @@ so some key presses (TAB) are swallowed by the system.  */
             [img setXBMColor: bm_color];
           }
 
-#ifdef NS_IMPL_COCOA
           // Note: For periodic images, the full image height is "h + hd".
           // By using the height h, a suitable part of the image is used.
           NSRect fromRect = NSMakeRect(0, 0, p->wd, p->h);
@@ -3134,13 +3133,6 @@ so some key presses (TAB) are swallowed by the system.  */
                  fraction: 1.0
                respectFlipped: YES
                     hints: nil];
-#else
-          {
-            NSPoint pt = imageRect.origin;
-            pt.y += p->h;
-            [img compositeToPoint: pt operation: NSCompositingOperationSourceOver];
-          }
-#endif
         }
       ns_reset_clipping (f);
     }
diff --git a/src/xterm.c b/src/xterm.c
index e9cebcebba..fbbf61d320 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -38,11 +38,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/extensions/Xfixes.h>
 #endif
 
-/* Using Xft implies that XRender is available.  */
-#ifdef HAVE_XFT
-#include <X11/extensions/Xrender.h>
-#endif
-
 #ifdef HAVE_XDBE
 #include <X11/extensions/Xdbe.h>
 #endif
@@ -2976,6 +2971,46 @@ x_draw_glyph_string_box (struct glyph_string *s)
 }
 
 
+static void
+x_composite_image (struct glyph_string *s, Pixmap dest,
+                   int srcX, int srcY, int dstX, int dstY,
+                   int width, int height)
+{
+#ifdef HAVE_XRENDER
+  if (s->img->picture)
+    {
+      Picture destination;
+      XRenderPictFormat *default_format;
+      XRenderPictureAttributes attr;
+
+      /* FIXME: Should we do this each time or would it make sense to
+         store destination in the frame struct?  */
+      default_format = XRenderFindVisualFormat (s->display,
+                                                DefaultVisual (s->display, 0));
+      destination = XRenderCreatePicture (s->display, dest,
+                                          default_format, 0, &attr);
+
+      /* FIXME: It may make sense to use PictOpSrc instead of
+         PictOpOver, as I don't know if we care about alpha values too
+         much here.  */
+      XRenderComposite (s->display, PictOpOver,
+                        s->img->picture, s->img->mask_picture, destination,
+                        srcX, srcY,
+                        srcX, srcY,
+                        dstX, dstY,
+                        width, height);
+
+      XRenderFreePicture (s->display, destination);
+    }
+  else
+#endif
+    XCopyArea (s->display, s->img->pixmap,
+               dest, s->gc,
+               srcX, srcY,
+               width, height, dstX, dstY);
+}
+
+
 /* Draw foreground of image glyph string S.  */
 
 static void
@@ -3007,6 +3042,7 @@ x_draw_image_foreground (struct glyph_string *s)
 	     trust on the shape extension to be available
 	     (XShapeCombineRegion).  So, compute the rectangle to draw
 	     manually.  */
+          /* FIXME: Do we need to do this when using XRender compositing?  */
 	  unsigned long mask = (GCClipMask | GCClipXOrigin | GCClipYOrigin
 				| GCFunction);
 	  XGCValues xgcv;
@@ -3024,10 +3060,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 	}
       else
 	{
@@ -3039,10 +3073,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 
 	  /* When the image has a mask, we can expect that at
 	     least part of a mouse highlight or a block cursor will
-- 
2.19.1


-- 
Alan Third

Re: [PATCH] Add native image scaling

[Eli Zaretskii]

> Date: Tue, 1 Jan 2019 21:47:19 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> This is a completely rewritten version that doesn't do anything
> different with the image cache. I think this is pretty complete as it
> is, excepting Windows support. I don't know how to add Windows support
> to match the way it's done in NS and X, however if we can't resize the
> images up-front, it should be possible to add some size info to the
> image struct and do the resize on display.

Thanks, I have only minor comments, after which this can be pushed.
Support for MS-Windows can be added later.

> +#ifdef HAVE_XRENDER
> +static bool x_create_x_image_and_pixmap (struct frame *, int, int, int,
> +					 XImagePtr *, Pixmap *, Picture *);
> +#else
>  static bool x_create_x_image_and_pixmap (struct frame *, int, int, int,
>  					 XImagePtr *, Pixmap *);
> +#endif
>  static void x_destroy_x_image (XImagePtr ximg);

Here and elsewhere, I'd prefer not to change the signature of
functions with and without the scaling support.  Instead, let's use
the same signature, where the extra argument(s) are left unused when
scaling is not supported.  If some data types will be undefined
without including headers which are available only when the feature is
supported, we can either use a void pointer, or have a dummy
data type definition for those arguments.

> +DEFUN ("image-scaling-p", Fimage_scaling_p, Simage_scaling_p, 0, 1, 0,
> +       doc: /* Test whether FRAME supports resizing images.
> +Return t if FRAME supports native scaling, nil otherwise.  */)
> +     (Lisp_Object frame)

This primitive should be documented both in NEWS and in the ELisp
manual.

Thanks!

[PATCH v2] Add native image scaling (bug#33587)

[Alan Third]

* configure.ac: Test for XRender outside of xft checks.
* src/Makefile.in (XRENDER_LIBS): List XRender libs separately from
xft libs.
* lisp/image.el (image--get-imagemagick-and-warn): Allow resizing if
native scaling is available.
* src/dispextern.h: Add XRender and image scaling stuff.
(struct image): Add XRender Pictures.
* src/image.c (x_create_bitmap_mask):
(image_create_x_image_and_pixmap): Handle XRender Picture.
(scale_image_size):
(compute_image_size): Make available when any form of scaling is
enabled.
(x_set_image_size): New function.
(lookup_image): Set image size.
(x_create_x_image_and_pixmap): Create XRender Picture when necessary.
(x_put_x_image): Handle the case where desired size != actual size.
(free_image): Free XRender Pictures.
(Fimage_scaling_p): New function.
(syms_of_image): Add image-scaling-p.
* src/nsimage.m (ns_load_image): Remove NS specific resizing.
([EmacsImage setSizeFromSpec:]): Remove method.
(ns_image_set_size): New function.
* src/nsterm.m (ns_draw_fringe_bitmap): Cocoa and GNUstep both have
the same compositing functions, so remove unnecessary difference.
* src/xterm.c (x_composite_image): New function.
(x_draw_image_foreground): Use new x_composite_image function.
* doc/lispref/display.texi (Image Descriptors): Document
image-scaling-p and add resizing descriptors.
(ImageMagick Images): Remove resizing descriptors.
---
I think this is the final version.

I would appreciate if someone who knows their way around image
handling would be able to test it. I’m particularly concerned that
I’ve probably broken masks. I can’t find any examples of how to use
them online, and they don’t work at all in NS, so I don’t know if they
get any use.

 configure.ac             |  14 +-
 doc/lispref/display.texi |  88 ++++++-----
 etc/NEWS                 |   6 +
 lisp/image.el            |   4 +-
 src/Makefile.in          |   3 +-
 src/dispextern.h         |  13 ++
 src/image.c              | 312 ++++++++++++++++++++++++++-------------
 src/nsimage.m            |  68 +--------
 src/nsterm.h             |   2 +-
 src/nsterm.m             |   8 -
 src/xterm.c              |  58 ++++++--
 11 files changed, 343 insertions(+), 233 deletions(-)

diff --git a/configure.ac b/configure.ac
index 91fa417308..0c59fe0619 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3241,6 +3241,17 @@ AC_DEFUN
   CFLAGS=$late_CFLAGS
 fi
 
+# Check for XRender
+HAVE_XRENDER=no
+if test "${HAVE_X11}" = "yes"; then
+  AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
+  if test $HAVE_XRENDER = yes; then
+    XRENDER_LIBS="-lXrender"
+    AC_SUBST(XRENDER_LIBS)
+    AC_DEFINE([HAVE_XRENDER], 1, [Define to 1 if XRender is available.])
+  fi
+fi
+
 ### Start of font-backend (under any platform) section.
 # (nothing here yet -- this is a placeholder)
 ### End of font-backend (under any platform) section.
@@ -3263,15 +3274,12 @@ AC_DEFUN
       EMACS_CHECK_MODULES([XFT], [xft >= 0.13.0], [], [HAVE_XFT=no])
       ## Because xterm.c uses XRenderQueryExtension when XFT is
       ## enabled, we also need to link to -lXrender.
-      HAVE_XRENDER=no
-      AC_CHECK_LIB(Xrender, XRenderQueryExtension, HAVE_XRENDER=yes)
       if test "$HAVE_XFT" != no && test "$HAVE_XRENDER" != no; then
 	OLD_CPPFLAGS="$CPPFLAGS"
 	OLD_CFLAGS="$CFLAGS"
 	OLD_LIBS="$LIBS"
 	CPPFLAGS="$CPPFLAGS $XFT_CFLAGS"
 	CFLAGS="$CFLAGS $XFT_CFLAGS"
-	XFT_LIBS="-lXrender $XFT_LIBS"
 	LIBS="$XFT_LIBS $LIBS"
 	AC_CHECK_HEADER(X11/Xft/Xft.h,
 	  AC_CHECK_LIB(Xft, XftFontOpen, HAVE_XFT=yes, , $XFT_LIBS) , ,
diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 19424ecc7e..f311cd73df 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5112,6 +5112,45 @@ Image Descriptors
 @var{relief} is negative, shadows are drawn so that the image appears
 as a pressed button; otherwise, it appears as an unpressed button.
 
+@item :width @var{width}, :height @var{height}
+The @code{:width} and @code{:height} keywords are used for scaling the
+image.  If only one of them is specified, the other one will be
+calculated so as to preserve the aspect ratio.  If both are specified,
+aspect ratio may not be preserved.
+
+@item :max-width @var{max-width}, :max-height @var{max-height}
+The @code{:max-width} and @code{:max-height} keywords are used for
+scaling if the size of the image of the image exceeds these values.
+If @code{:width} is set it will have precedence over @code{max-width},
+and if @code{:height} is set it will have precedence over
+@code{max-height}, but you can otherwise mix these keywords as you
+wish.  @code{:max-width} and @code{:max-height} will always preserve
+the aspect ratio.
+
+If both @code{:width} and @code{:max-height} has been set (but
+@code{:height} has not been set), then @code{:max-height} will have
+precedence.  The same is the case for the opposite combination: The
+``max'' keyword has precedence.  That is, if you have a 200x100 image
+and specify that @code{:width} should be 400 and @code{:max-height}
+should be 150, you'll end up with an image that is 300x150: Preserving
+the aspect ratio and not exceeding the ``max'' setting.  This
+combination of parameters is a useful way of saying ``display this
+image as large as possible, but no larger than the available display
+area''.
+
+@item :scale @var{scale}
+This should be a number, where values higher than 1 means to increase
+the size, and lower means to decrease the size.  For instance, a value
+of 0.25 will make the image a quarter size of what it originally was.
+If the scaling makes the image larger than specified by
+@code{:max-width} or @code{:max-height}, the resulting size will not
+exceed those two values.  If both @code{:scale} and
+@code{:height}/@code{:width} are specified, the height/width will be
+adjusted by the specified scaling factor.
+
+@item :index @var{frame}
+@xref{Multi-Frame Images}.
+
 @item :conversion @var{algorithm}
 This specifies a conversion algorithm that should be applied to the
 image before it is displayed; the value, @var{algorithm}, specifies
@@ -5251,6 +5290,16 @@ Image Descriptors
 (@pxref{Input Focus}).
 @end defun
 
+@defun image-scaling-p &optional frame
+This function returns @code{t} if @var{frame} supports image scaling.
+@var{frame} @code{nil} or omitted means to use the selected frame
+(@pxref{Input Focus}).
+
+If image scaling is not supported, @code{:width}, @code{:height},
+@code{:scale}, @code{:max-width} and @code{:max-height} will only be
+usable through ImageMagick, if available (@pxref{ImageMagick Images}).
+@end defun
+
 @node XBM Images
 @subsection XBM Images
 @cindex XBM
@@ -5387,42 +5436,6 @@ ImageMagick Images
 supports transparency.  If the value is @code{nil}, it defaults to the
 frame's background color.
 
-@item :width @var{width}, :height @var{height}
-The @code{:width} and @code{:height} keywords are used for scaling the
-image.  If only one of them is specified, the other one will be
-calculated so as to preserve the aspect ratio.  If both are specified,
-aspect ratio may not be preserved.
-
-@item :max-width @var{max-width}, :max-height @var{max-height}
-The @code{:max-width} and @code{:max-height} keywords are used for
-scaling if the size of the image of the image exceeds these values.
-If @code{:width} is set it will have precedence over @code{max-width},
-and if @code{:height} is set it will have precedence over
-@code{max-height}, but you can otherwise mix these keywords as you
-wish.  @code{:max-width} and @code{:max-height} will always preserve
-the aspect ratio.
-
-If both @code{:width} and @code{:max-height} has been set (but
-@code{:height} has not been set), then @code{:max-height} will have
-precedence.  The same is the case for the opposite combination: The
-``max'' keyword has precedence.  That is, if you have a 200x100 image
-and specify that @code{:width} should be 400 and @code{:max-height}
-should be 150, you'll end up with an image that is 300x150: Preserving
-the aspect ratio and not exceeding the ``max'' setting.  This
-combination of parameters is a useful way of saying ``display this
-image as large as possible, but no larger than the available display
-area''.
-
-@item :scale @var{scale}
-This should be a number, where values higher than 1 means to increase
-the size, and lower means to decrease the size.  For instance, a value
-of 0.25 will make the image a quarter size of what it originally was.
-If the scaling makes the image larger than specified by
-@code{:max-width} or @code{:max-height}, the resulting size will not
-exceed those two values.  If both @code{:scale} and
-@code{:height}/@code{:width} are specified, the height/width will be
-adjusted by the specified scaling factor.
-
 @item :format @var{type}
 The value, @var{type}, should be a symbol specifying the type of the
 image data, as found in @code{image-format-suffixes}.  This is used
@@ -5431,9 +5444,6 @@ ImageMagick Images
 
 @item :rotation @var{angle}
 Specifies a rotation angle in degrees.
-
-@item :index @var{frame}
-@xref{Multi-Frame Images}.
 @end table
 
 @node SVG Images
diff --git a/etc/NEWS b/etc/NEWS
index 75e2c1bf98..69e14773a7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1442,6 +1442,12 @@ that is non-nil, it will look for a file name handler for the current
 buffer's 'default-directory' and invoke that file name handler to make
 the process.  That way 'make-process' can start remote processes.
 
++++
+** Emacs now supports resizing images without ImageMagick on X window
+systems where the XRender extension is available, and on the NS port.
+The new function 'image-scaling-p' can be used to test whether any
+given frame supports resizing.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/lisp/image.el b/lisp/image.el
index 5727d8fbce..2e84e47b5c 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -982,8 +982,8 @@ image--get-image
     image))
 
 (defun image--get-imagemagick-and-warn ()
-  (unless (or (fboundp 'imagemagick-types) (featurep 'ns))
-    (error "Cannot rescale images without ImageMagick support"))
+  (unless (or (fboundp 'imagemagick-types) (image-scaling-p))
+    (error "Cannot rescale images on this terminal"))
   (let ((image (image--get-image)))
     (image-flush image)
     (when (fboundp 'imagemagick-types)
diff --git a/src/Makefile.in b/src/Makefile.in
index e9831e9299..f409ed4db2 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -127,7 +127,8 @@ LIBIMAGE=
 
 XCB_LIBS=@XCB_LIBS@
 XFT_LIBS=@XFT_LIBS@
-LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS)
+XRENDER_LIBS=@XRENDER_LIBS@
+LIBX_EXTRA=-lX11 $(XCB_LIBS) $(XFT_LIBS) $(XRENDER_LIBS)
 
 FONTCONFIG_CFLAGS = @FONTCONFIG_CFLAGS@
 FONTCONFIG_LIBS = @FONTCONFIG_LIBS@
diff --git a/src/dispextern.h b/src/dispextern.h
index 5774e3e951..b064875ac4 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -31,6 +31,9 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/Intrinsic.h>
 #endif /* USE_X_TOOLKIT */
 
+#ifdef HAVE_XRENDER
+#include <X11/extensions/Xrender.h>
+#endif
 #else /* !HAVE_X_WINDOWS */
 
 /* X-related stuff used by non-X gui code.  */
@@ -2935,6 +2938,11 @@ struct redisplay_interface
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+#if defined (HAVE_X_WINDOWS) && defined (HAVE_XRENDER) \
+  || defined (HAVE_NS)
+#define HAVE_NATIVE_SCALING
+#endif
+
 /* Structure describing an image.  Specific image formats like XBM are
    converted into this form, so that display only has to deal with
    this type of image.  */
@@ -2958,6 +2966,11 @@ struct image
      and the latter is outdated.  NULL means the X image has been
      synchronized to Pixmap.  */
   XImagePtr ximg, mask_img;
+
+#ifdef HAVE_NATIVE_SCALING
+  /* Picture versions of pixmap and mask for compositing.  */
+  Picture picture, mask_picture;
+#endif
 #endif
 
   /* Colors allocated for this image, if any.  Allocated via xmalloc.  */
diff --git a/src/image.c b/src/image.c
index 87e0c071ee..677689cae2 100644
--- a/src/image.c
+++ b/src/image.c
@@ -408,8 +408,13 @@ x_destroy_all_bitmaps (Display_Info *dpyinfo)
   dpyinfo->bitmaps_last = 0;
 }
 
+#ifndef HAVE_XRENDER
+/* Required for the definition of x_create_x_image_and_pixmap below.  */
+typedef void Picture;
+#endif
+
 static bool x_create_x_image_and_pixmap (struct frame *, int, int, int,
-					 XImagePtr *, Pixmap *);
+					 XImagePtr *, Pixmap *, Picture *);
 static void x_destroy_x_image (XImagePtr ximg);
 
 #ifdef HAVE_NTGUI
@@ -472,7 +477,8 @@ x_create_bitmap_mask (struct frame *f, ptrdiff_t id)
       return;
     }
 
-  result = x_create_x_image_and_pixmap (f, width, height, 1, &mask_img, &mask);
+  result = x_create_x_image_and_pixmap (f, width, height, 1,
+                                        &mask_img, &mask, NULL);
 
   unblock_input ();
   if (!result)
@@ -1011,6 +1017,13 @@ free_image (struct frame *f, struct image *img)
 
       c->images[img->id] = NULL;
 
+#ifdef HAVE_XRENDER
+      if (img->picture)
+        XRenderFreePicture (FRAME_X_DISPLAY (f), img->picture);
+      if (img->mask_picture)
+        XRenderFreePicture (FRAME_X_DISPLAY (f), img->mask_picture);
+#endif
+
       /* Windows NT redefines 'free', but in this file, we need to
          avoid the redefinition.  */
 #ifdef WINDOWSNT
@@ -1747,6 +1760,147 @@ postprocess_image (struct frame *f, struct image *img)
     }
 }
 
+#if defined (HAVE_IMAGEMAGICK) || defined (HAVE_NATIVE_SCALING)
+/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
+   safely rounded and clipped to int range.  */
+
+static int
+scale_image_size (int size, size_t divisor, size_t multiplier)
+{
+  if (divisor != 0)
+    {
+      double s = size;
+      double scaled = s * multiplier / divisor + 0.5;
+      if (scaled < INT_MAX)
+	return scaled;
+    }
+  return INT_MAX;
+}
+
+/* Compute the desired size of an image with native size WIDTH x HEIGHT.
+   Use SPEC to deduce the size.  Store the desired size into
+   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
+static void
+compute_image_size (size_t width, size_t height,
+		    Lisp_Object spec,
+		    int *d_width, int *d_height)
+{
+  Lisp_Object value;
+  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
+  double scale = 1;
+
+  value = image_spec_value (spec, QCscale, NULL);
+  if (NUMBERP (value))
+    scale = XFLOATINT (value);
+
+  value = image_spec_value (spec, QCmax_width, NULL);
+  if (FIXNATP (value))
+    max_width = min (XFIXNAT (value), INT_MAX);
+
+  value = image_spec_value (spec, QCmax_height, NULL);
+  if (FIXNATP (value))
+    max_height = min (XFIXNAT (value), INT_MAX);
+
+  /* If width and/or height is set in the display spec assume we want
+     to scale to those values.  If either h or w is unspecified, the
+     unspecified should be calculated from the specified to preserve
+     aspect ratio.  */
+  value = image_spec_value (spec, QCwidth, NULL);
+  if (FIXNATP (value))
+    {
+      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :width overrides :max-width. */
+      max_width = -1;
+    }
+
+  value = image_spec_value (spec, QCheight, NULL);
+  if (FIXNATP (value))
+    {
+      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      /* :height overrides :max-height. */
+      max_height = -1;
+    }
+
+  /* If we have both width/height set explicitly, we skip past all the
+     aspect ratio-preserving computations below. */
+  if (desired_width != -1 && desired_height != -1)
+    goto out;
+
+  width = width * scale;
+  height = height * scale;
+
+  if (desired_width != -1)
+    /* Width known, calculate height. */
+    desired_height = scale_image_size (desired_width, width, height);
+  else if (desired_height != -1)
+    /* Height known, calculate width. */
+    desired_width = scale_image_size (desired_height, height, width);
+  else
+    {
+      desired_width = width;
+      desired_height = height;
+    }
+
+  if (max_width != -1 && desired_width > max_width)
+    {
+      /* The image is wider than :max-width. */
+      desired_width = max_width;
+      desired_height = scale_image_size (desired_width, width, height);
+    }
+
+  if (max_height != -1 && desired_height > max_height)
+    {
+      /* The image is higher than :max-height. */
+      desired_height = max_height;
+      desired_width = scale_image_size (desired_height, height, width);
+    }
+
+ out:
+  *d_width = desired_width;
+  *d_height = desired_height;
+}
+
+#ifdef HAVE_NATIVE_SCALING
+static void
+x_set_image_size (struct frame *f, struct image *img)
+{
+#ifdef HAVE_IMAGEMAGICK
+  /* ImageMagick images are already the correct size.  */
+  if (!imagemagick_image_p (img->spec))
+#endif
+    {
+      int width, height;
+
+      compute_image_size (img->width, img->height, img->spec, &width, &height);
+
+#ifdef HAVE_NS
+      ns_image_set_size (img->pixmap, width, height);
+      img->width = width;
+      img->height = height;
+#endif
+
+#ifdef HAVE_XRENDER
+      if (img->picture)
+      {
+        double xscale = (double) img->width/width;
+        double yscale = (double) img->height/height;
+
+        XTransform tmat = {{{XDoubleToFixed (xscale), XDoubleToFixed (0), XDoubleToFixed (0)},
+                            {XDoubleToFixed (0), XDoubleToFixed (yscale), XDoubleToFixed (0)},
+                            {XDoubleToFixed (0), XDoubleToFixed (0), XDoubleToFixed (1)}}};
+
+        XRenderSetPictureFilter (FRAME_X_DISPLAY (f), img->picture, FilterBest, 0, 0);
+        XRenderSetPictureTransform (FRAME_X_DISPLAY (f), img->picture, &tmat);
+
+        img->width = width;
+        img->height = height;
+      }
+#endif
+    }
+}
+#endif
+#endif /* HAVE_IMAGEMAGICK || HAVE_XRENDER || HAVE_NS  */
+
 
 /* Return the id of image with Lisp specification SPEC on frame F.
    SPEC must be a valid Lisp image specification (see valid_image_p).  */
@@ -1802,6 +1956,9 @@ lookup_image (struct frame *f, Lisp_Object spec)
 	     `:background COLOR'.  */
 	  Lisp_Object ascent, margin, relief, bg;
 	  int relief_bound;
+#ifdef HAVE_NATIVE_SCALING
+          x_set_image_size (f, img);
+#endif
 
 	  ascent = image_spec_value (spec, QCascent, NULL);
 	  if (FIXNUMP (ascent))
@@ -1976,12 +2133,15 @@ x_check_image_size (XImagePtr ximg, int width, int height)
 
 static bool
 x_create_x_image_and_pixmap (struct frame *f, int width, int height, int depth,
-			     XImagePtr *ximg, Pixmap *pixmap)
+			     XImagePtr *ximg, Pixmap *pixmap, Picture *picture)
 {
 #ifdef HAVE_X_WINDOWS
   Display *display = FRAME_X_DISPLAY (f);
   Drawable drawable = FRAME_X_DRAWABLE (f);
   Screen *screen = FRAME_X_SCREEN (f);
+#ifdef HAVE_XRENDER
+  int event_basep, error_basep;
+#endif
 
   eassert (input_blocked_p ());
 
@@ -2018,6 +2178,21 @@ x_create_x_image_and_pixmap (struct frame *f, int width, int height, int depth,
       return 0;
     }
 
+#ifdef HAVE_XRENDER
+  if (picture && XRenderQueryExtension (display, &event_basep, &error_basep))
+    {
+      XRenderPictFormat *format;
+      XRenderPictureAttributes attr;
+
+      /* FIXME: Do we need to handle all possible bit depths?  */
+      format = XRenderFindStandardFormat (display,
+                                          depth > 24 ? PictStandardARGB32
+                                          : depth > 8 ? PictStandardRGB24
+                                          : PictStandardA8);
+      *picture = XRenderCreatePicture (display, *pixmap, format, 0, &attr);
+    }
+#endif
+
   return 1;
 #endif /* HAVE_X_WINDOWS */
 
@@ -2163,7 +2338,8 @@ x_put_x_image (struct frame *f, XImagePtr ximg, Pixmap pixmap, int width, int he
 
   eassert (input_blocked_p ());
   gc = XCreateGC (FRAME_X_DISPLAY (f), pixmap, 0, NULL);
-  XPutImage (FRAME_X_DISPLAY (f), pixmap, gc, ximg, 0, 0, 0, 0, width, height);
+  XPutImage (FRAME_X_DISPLAY (f), pixmap, gc, ximg, 0, 0, 0, 0,
+             ximg->width, ximg->height);
   XFreeGC (FRAME_X_DISPLAY (f), gc);
 #endif /* HAVE_X_WINDOWS */
 
@@ -2192,7 +2368,13 @@ image_create_x_image_and_pixmap (struct frame *f, struct image *img,
   eassert ((!mask_p ? img->pixmap : img->mask) == NO_PIXMAP);
 
   return x_create_x_image_and_pixmap (f, width, height, depth, ximg,
-				      !mask_p ? &img->pixmap : &img->mask);
+				      !mask_p ? &img->pixmap : &img->mask,
+#ifdef HAVE_XRENDER
+                                      !mask_p ? &img->picture : &img->mask_picture
+#else
+                                      NULL
+#endif
+                                      );
 }
 
 /* Put X image XIMG into image IMG on frame F, as a mask if and only
@@ -8101,105 +8283,6 @@ gif_load (struct frame *f, struct image *img)
 				 ImageMagick
 ***********************************************************************/
 
-/* Scale an image size by returning SIZE / DIVISOR * MULTIPLIER,
-   safely rounded and clipped to int range.  */
-
-static int
-scale_image_size (int size, size_t divisor, size_t multiplier)
-{
-  if (divisor != 0)
-    {
-      double s = size;
-      double scaled = s * multiplier / divisor + 0.5;
-      if (scaled < INT_MAX)
-	return scaled;
-    }
-  return INT_MAX;
-}
-
-/* Compute the desired size of an image with native size WIDTH x HEIGHT.
-   Use SPEC to deduce the size.  Store the desired size into
-   *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
-static void
-compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
-		    int *d_width, int *d_height)
-{
-  Lisp_Object value;
-  int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
-  double scale = 1;
-
-  value = image_spec_value (spec, QCscale, NULL);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value);
-
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
-
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
-
-  /* If width and/or height is set in the display spec assume we want
-     to scale to those values.  If either h or w is unspecified, the
-     unspecified should be calculated from the specified to preserve
-     aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
-    {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
-    {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  /* If we have both width/height set explicitly, we skip past all the
-     aspect ratio-preserving computations below. */
-  if (desired_width != -1 && desired_height != -1)
-    goto out;
-
-  width = width * scale;
-  height = height * scale;
-
-  if (desired_width != -1)
-    /* Width known, calculate height. */
-    desired_height = scale_image_size (desired_width, width, height);
-  else if (desired_height != -1)
-    /* Height known, calculate width. */
-    desired_width = scale_image_size (desired_height, height, width);
-  else
-    {
-      desired_width = width;
-      desired_height = height;
-    }
-
-  if (max_width != -1 && desired_width > max_width)
-    {
-      /* The image is wider than :max-width. */
-      desired_width = max_width;
-      desired_height = scale_image_size (desired_width, width, height);
-    }
-
-  if (max_height != -1 && desired_height > max_height)
-    {
-      /* The image is higher than :max-height. */
-      desired_height = max_height;
-      desired_width = scale_image_size (desired_height, height, width);
-    }
-
- out:
-  *d_width = desired_width;
-  *d_height = desired_height;
-}
-
 static bool imagemagick_image_p (Lisp_Object);
 static bool imagemagick_load (struct frame *, struct image *);
 static void imagemagick_clear_image (struct frame *, struct image *);
@@ -9816,6 +9899,25 @@ DEFUN ("lookup-image", Flookup_image, Slookup_image, 1, 1, 0,
 			    Initialization
  ***********************************************************************/
 
+DEFUN ("image-scaling-p", Fimage_scaling_p, Simage_scaling_p, 0, 1, 0,
+       doc: /* Test whether FRAME supports resizing images.
+Return t if FRAME supports native scaling, nil otherwise.  */)
+     (Lisp_Object frame)
+{
+#ifdef HAVE_NS
+  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;
+#endif
+
+  return Qnil;
+}
+
 DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
        doc: /* Initialize image library implementing image type TYPE.
 Return non-nil if TYPE is a supported image type.
@@ -10058,6 +10160,8 @@ non-numeric, there is no explicit limit on the size of images.  */);
   defsubr (&Slookup_image);
 #endif
 
+  defsubr (&Simage_scaling_p);
+
   DEFVAR_BOOL ("cross-disabled-images", cross_disabled_images,
     doc: /* Non-nil means always draw a cross over disabled images.
 Disabled images are those having a `:conversion disabled' property.
diff --git a/src/nsimage.m b/src/nsimage.m
index 7879c5891d..f16910de08 100644
--- a/src/nsimage.m
+++ b/src/nsimage.m
@@ -126,8 +126,6 @@ Updated by Christian Limpach (chris@nice.ch)
       eImg = temp;
     }
 
-  [eImg setSizeFromSpec:XCDR (img->spec)];
-
   size = [eImg size];
   img->width = size.width;
   img->height = size.height;
@@ -151,6 +149,12 @@ Updated by Christian Limpach (chris@nice.ch)
   return [(id)img size].height;
 }
 
+void
+ns_image_set_size (void *img, int width, int height)
+{
+  [(EmacsImage *)img setSize:NSMakeSize (width, height)];
+}
+
 unsigned long
 ns_get_pixel (void *img, int x, int y)
 {
@@ -524,66 +528,6 @@ - (BOOL)setFrame: (unsigned int) index
   return YES;
 }
 
-- (void)setSizeFromSpec: (Lisp_Object) spec
-{
-  NSSize size = [self size];
-  Lisp_Object value;
-  double scale = 1, aspect = size.width / size.height;
-  double width = -1, height = -1, max_width = -1, max_height = -1;
-
-  value = Fplist_get (spec, QCscale);
-  if (NUMBERP (value))
-    scale = XFLOATINT (value) ;
-
-  value = Fplist_get (spec, QCmax_width);
-  if (NUMBERP (value))
-    max_width = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCmax_height);
-  if (NUMBERP (value))
-    max_height = XFLOATINT (value);
-
-  value = Fplist_get (spec, QCwidth);
-  if (NUMBERP (value))
-    {
-      width = XFLOATINT (value) * scale;
-      /* :width overrides :max-width. */
-      max_width = -1;
-    }
-
-  value = Fplist_get (spec, QCheight);
-  if (NUMBERP (value))
-    {
-      height = XFLOATINT (value) * scale;
-      /* :height overrides :max-height. */
-      max_height = -1;
-    }
-
-  if (width <= 0 && height <= 0)
-    {
-      width = size.width * scale;
-      height = size.height * scale;
-    }
-  else if (width > 0 && height <= 0)
-      height = width / aspect;
-  else if (height > 0 && width <= 0)
-      width = height * aspect;
-
-  if (max_width > 0 && width > max_width)
-    {
-      width = max_width;
-      height = max_width / aspect;
-    }
-
-  if (max_height > 0 && height > max_height)
-    {
-      height = max_height;
-      width = max_height * aspect;
-    }
-
-  [self setSize:NSMakeSize(width, height)];
-}
-
 - (instancetype)rotate: (double)rotation
 {
   EmacsImage *new_image;
diff --git a/src/nsterm.h b/src/nsterm.h
index 089cbccbf0..78ce608554 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -648,7 +648,6 @@ typedef id instancetype;
 - (NSColor *)stippleMask;
 - (Lisp_Object)getMetadata;
 - (BOOL)setFrame: (unsigned int) index;
-- (void)setSizeFromSpec: (Lisp_Object) spec;
 - (instancetype)rotate: (double)rotation;
 @end
 
@@ -1197,6 +1196,7 @@ extern bool ns_load_image (struct frame *f, struct image *img,
 			   Lisp_Object spec_file, Lisp_Object spec_data);
 extern int ns_image_width (void *img);
 extern int ns_image_height (void *img);
+extern void ns_image_set_size (void *img, int width, int height);
 extern unsigned long ns_get_pixel (void *img, int x, int y);
 extern void ns_put_pixel (void *img, int x, int y, unsigned long argb);
 extern void ns_set_alpha (void *img, int x, int y, unsigned char a);
diff --git a/src/nsterm.m b/src/nsterm.m
index 016c044760..cbb2d2a5ce 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3121,7 +3121,6 @@ so some key presses (TAB) are swallowed by the system.  */
             [img setXBMColor: bm_color];
           }
 
-#ifdef NS_IMPL_COCOA
           // Note: For periodic images, the full image height is "h + hd".
           // By using the height h, a suitable part of the image is used.
           NSRect fromRect = NSMakeRect(0, 0, p->wd, p->h);
@@ -3134,13 +3133,6 @@ so some key presses (TAB) are swallowed by the system.  */
                  fraction: 1.0
                respectFlipped: YES
                     hints: nil];
-#else
-          {
-            NSPoint pt = imageRect.origin;
-            pt.y += p->h;
-            [img compositeToPoint: pt operation: NSCompositingOperationSourceOver];
-          }
-#endif
         }
       ns_reset_clipping (f);
     }
diff --git a/src/xterm.c b/src/xterm.c
index e9cebcebba..fbbf61d320 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -38,11 +38,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <X11/extensions/Xfixes.h>
 #endif
 
-/* Using Xft implies that XRender is available.  */
-#ifdef HAVE_XFT
-#include <X11/extensions/Xrender.h>
-#endif
-
 #ifdef HAVE_XDBE
 #include <X11/extensions/Xdbe.h>
 #endif
@@ -2976,6 +2971,46 @@ x_draw_glyph_string_box (struct glyph_string *s)
 }
 
 
+static void
+x_composite_image (struct glyph_string *s, Pixmap dest,
+                   int srcX, int srcY, int dstX, int dstY,
+                   int width, int height)
+{
+#ifdef HAVE_XRENDER
+  if (s->img->picture)
+    {
+      Picture destination;
+      XRenderPictFormat *default_format;
+      XRenderPictureAttributes attr;
+
+      /* FIXME: Should we do this each time or would it make sense to
+         store destination in the frame struct?  */
+      default_format = XRenderFindVisualFormat (s->display,
+                                                DefaultVisual (s->display, 0));
+      destination = XRenderCreatePicture (s->display, dest,
+                                          default_format, 0, &attr);
+
+      /* FIXME: It may make sense to use PictOpSrc instead of
+         PictOpOver, as I don't know if we care about alpha values too
+         much here.  */
+      XRenderComposite (s->display, PictOpOver,
+                        s->img->picture, s->img->mask_picture, destination,
+                        srcX, srcY,
+                        srcX, srcY,
+                        dstX, dstY,
+                        width, height);
+
+      XRenderFreePicture (s->display, destination);
+    }
+  else
+#endif
+    XCopyArea (s->display, s->img->pixmap,
+               dest, s->gc,
+               srcX, srcY,
+               width, height, dstX, dstY);
+}
+
+
 /* Draw foreground of image glyph string S.  */
 
 static void
@@ -3007,6 +3042,7 @@ x_draw_image_foreground (struct glyph_string *s)
 	     trust on the shape extension to be available
 	     (XShapeCombineRegion).  So, compute the rectangle to draw
 	     manually.  */
+          /* FIXME: Do we need to do this when using XRender compositing?  */
 	  unsigned long mask = (GCClipMask | GCClipXOrigin | GCClipYOrigin
 				| GCFunction);
 	  XGCValues xgcv;
@@ -3024,10 +3060,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 	}
       else
 	{
@@ -3039,10 +3073,8 @@ x_draw_image_foreground (struct glyph_string *s)
 	  image_rect.width = s->slice.width;
 	  image_rect.height = s->slice.height;
 	  if (x_intersect_rectangles (&clip_rect, &image_rect, &r))
-            XCopyArea (s->display, s->img->pixmap,
-                       FRAME_X_DRAWABLE (s->f), s->gc,
-		       s->slice.x + r.x - x, s->slice.y + r.y - y,
-		       r.width, r.height, r.x, r.y);
+            x_composite_image (s, FRAME_X_DRAWABLE (s->f), s->slice.x + r.x - x, s->slice.y + r.y - y,
+                               r.x, r.y, r.width, r.height);
 
 	  /* When the image has a mask, we can expect that at
 	     least part of a mouse highlight or a block cursor will
-- 
2.19.1


-- 
Alan Third

Re: [PATCH v2] Add native image scaling (bug#33587)

[Eli Zaretskii]

> Date: Wed, 2 Jan 2019 21:12:41 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I think this is the final version.

Thanks.  A few minor gotchas.

> I would appreciate if someone who knows their way around image
> handling would be able to test it. I’m particularly concerned that
> I’ve probably broken masks. I can’t find any examples of how to use
> them online, and they don’t work at all in NS, so I don’t know if they
> get any use.

Seconded.

> +@item :max-width @var{max-width}, :max-height @var{max-height}
> +The @code{:max-width} and @code{:max-height} keywords are used for
> +scaling if the size of the image of the image exceeds these values.
                       ^^^^^^^^^^^^^^^^^^^^^^^^^
Redundant repetition.

> +If @code{:width} is set it will have precedence over @code{max-width},
                          ^
A comma is missing there.

> +and if @code{:height} is set it will have precedence over
                               ^
Likewise here.

> +       @code{:max-width} and @code{:max-height} will always preserve
> +the aspect ratio.

I don't think I understand what that means in practice.  Does this
allude to the issue described below with using :max-width and
:max-height in preference to :with and :height?  If so, I suggest to
describe that only once.

> +If both @code{:width} and @code{:max-height} has been set (but
                                                ^^^
"have"

> +@code{:height} has not been set), then @code{:max-height} will have
> +precedence.  The same is the case for the opposite combination: The
> +``max'' keyword has precedence.

This confused me until I've read the example.  having read the
example, I suggest to describe this differently:

  If both @code{:max-width} and @code{:height} are specified, but
  @code{:width} is not, preserving the aspect ratio might require that
  width exceeds @code{:max-width}.  If this happens, scaling will use a
  smaller value for the height so as to preserve the aspect ratio while
  not exceeding @code{:max-width}.  Similarly when both
  @code{:max-height} and @code{:width} are specified, but @code{:height}
  is not.

> +@item :scale @var{scale}
> +This should be a number, where values higher than 1 means to increase
> +the size, and lower means to decrease the size.  For instance, a value
> +of 0.25 will make the image a quarter size of what it originally was.

I think we should say here explicitly that scale multiplies both
height and width, because people might otherwise think the scaling
applies to the area of the image.

> +@defun image-scaling-p &optional frame
> +This function returns @code{t} if @var{frame} supports image scaling.
> +@var{frame} @code{nil} or omitted means to use the selected frame
> +(@pxref{Input Focus}).
> +
> +If image scaling is not supported, @code{:width}, @code{:height},
> +@code{:scale}, @code{:max-width} and @code{:max-height} will only be
> +usable through ImageMagick, if available (@pxref{ImageMagick Images}).
> +@end defun

Shouldn't image-scaling-p return non-nil if ImageMagick is available?
I think that would allow a simpler Lisp code.

Re: [PATCH v2] Add native image scaling (bug#33587)

[Alan Third]

On Fri, Jan 04, 2019 at 04:31:41PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 2 Jan 2019 21:12:41 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > I think this is the final version.
> 
> Thanks.  A few minor gotchas.

I regret touching this now. ;)

Do these look OK?

modified   doc/lispref/display.texi
@@ -5120,33 +5120,35 @@ Image Descriptors
 
 @item :max-width @var{max-width}, :max-height @var{max-height}
 The @code{:max-width} and @code{:max-height} keywords are used for
-scaling if the size of the image of the image exceeds these values.
-If @code{:width} is set it will have precedence over @code{max-width},
-and if @code{:height} is set it will have precedence over
+scaling if the size of the image exceeds these values.  If
+@code{:width} is set, it will have precedence over @code{max-width},
+and if @code{:height} is set, it will have precedence over
 @code{max-height}, but you can otherwise mix these keywords as you
-wish.  @code{:max-width} and @code{:max-height} will always preserve
-the aspect ratio.
-
-If both @code{:width} and @code{:max-height} has been set (but
-@code{:height} has not been set), then @code{:max-height} will have
-precedence.  The same is the case for the opposite combination: The
-``max'' keyword has precedence.  That is, if you have a 200x100 image
-and specify that @code{:width} should be 400 and @code{:max-height}
-should be 150, you'll end up with an image that is 300x150: Preserving
-the aspect ratio and not exceeding the ``max'' setting.  This
-combination of parameters is a useful way of saying ``display this
-image as large as possible, but no larger than the available display
-area''.
+wish.
+
+If both @code{:max-width} and @code{:height} are specified, but
+@code{:width} is not, preserving the aspect ratio might require that
+width exceeds @code{:max-width}.  If this happens, scaling will use a
+smaller value for the height so as to preserve the aspect ratio while
+not exceeding @code{:max-width}.  Similarly when both
+@code{:max-height} and @code{:width} are specified, but @code{:height}
+is not.  For example, if you have a 200x100 image and specify that
+@code{:width} should be 400 and @code{:max-height} should be 150,
+you'll end up with an image that is 300x150: Preserving the aspect
+ratio and not exceeding the ``max'' setting.  This combination of
+parameters is a useful way of saying ``display this image as large as
+possible, but no larger than the available display area''.
 
 @item :scale @var{scale}
 This should be a number, where values higher than 1 means to increase
-the size, and lower means to decrease the size.  For instance, a value
-of 0.25 will make the image a quarter size of what it originally was.
-If the scaling makes the image larger than specified by
-@code{:max-width} or @code{:max-height}, the resulting size will not
-exceed those two values.  If both @code{:scale} and
-@code{:height}/@code{:width} are specified, the height/width will be
-adjusted by the specified scaling factor.
+the size, and lower means to decrease the size, by multiplying both
+the width and height.  For instance, a value of 0.25 will make the
+image a quarter size of what it originally was.  If the scaling makes
+the image larger than specified by @code{:max-width} or
+@code{:max-height}, the resulting size will not exceed those two
+values.  If both @code{:scale} and @code{:height}/@code{:width} are
+specified, the height/width will be adjusted by the specified scaling
+factor.
 
 @item :index @var{frame}
 @xref{Multi-Frame Images}.

> > +@code{:height} has not been set), then @code{:max-height} will have
> > +precedence.  The same is the case for the opposite combination: The
> > +``max'' keyword has precedence.
> 
> This confused me until I've read the example.  having read the
> example, I suggest to describe this differently:
> 
>   If both @code{:max-width} and @code{:height} are specified, but
>   @code{:width} is not, preserving the aspect ratio might require that
>   width exceeds @code{:max-width}.  If this happens, scaling will use a
>   smaller value for the height so as to preserve the aspect ratio while
>   not exceeding @code{:max-width}.  Similarly when both
>   @code{:max-height} and @code{:width} are specified, but @code{:height}
>   is not.

I’ve put that in. FWIW I don’t understand why this behaviour was
chosen, it seems to me that max-width and max-height should be exactly
that, the max width and height. I don’t understand why they can be
over‐ridden.

Too late to do anything about it now, I suppose.

> > +@defun image-scaling-p &optional frame
> > +This function returns @code{t} if @var{frame} supports image scaling.
> > +@var{frame} @code{nil} or omitted means to use the selected frame
> > +(@pxref{Input Focus}).
> > +
> > +If image scaling is not supported, @code{:width}, @code{:height},
> > +@code{:scale}, @code{:max-width} and @code{:max-height} will only be
> > +usable through ImageMagick, if available (@pxref{ImageMagick Images}).
> > +@end defun
> 
> Shouldn't image-scaling-p return non-nil if ImageMagick is available?
> I think that would allow a simpler Lisp code.

The difficulty is in knowing whether you need to set :type to
ImageMagick or not.

What we really don’t want to do is just return t for both native and
ImageMagick, as you then have no way of deciding to go with native
over ImageMagick, because you can’t tell the difference between
native‐and‐ImageMagick, or just ImageMagick.

I did think that I could return t if native is available, and if it’s
not, but ImageMagick is, return 'imagemagick.

Or perhaps return a list of scaling capable backends

    '(native imagemagick)

There are other ways of detecting ImageMagick, but no other way of
detecting native scaling, other than just trying it I suppose.

(Another option would be to automatically fall‐back to ImageMagick if
required, but given that we’re doing this because people are concerned
about ImageMagick’s security record, I doubt making it’s use automatic
would appeal.)
-- 
Alan Third

Re: [PATCH v2] Add native image scaling (bug#33587)

[Eli Zaretskii]

> Date: Fri, 4 Jan 2019 19:09:14 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> (Another option would be to automatically fall‐back to ImageMagick if
> required, but given that we’re doing this because people are concerned
> about ImageMagick’s security record, I doubt making it’s use automatic
> would appeal.)

I guess I've misread the code, because I thought we did fall back
automatically.  FWIW, I think if the user builds Emacs with
ImageMagick, they already considered the security issues and decided
they are okay with them.  We shouldn't second-guess them.

Re: [PATCH v2] Add native image scaling (bug#33587)

[Alan Third]

On Fri, Jan 04, 2019 at 10:21:17PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 4 Jan 2019 19:09:14 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > (Another option would be to automatically fall‐back to ImageMagick if
> > required, but given that we’re doing this because people are concerned
> > about ImageMagick’s security record, I doubt making it’s use automatic
> > would appeal.)
> 
> I guess I've misread the code, because I thought we did fall back
> automatically.  FWIW, I think if the user builds Emacs with
> ImageMagick, they already considered the security issues and decided
> they are okay with them.  We shouldn't second-guess them.

If you load an image then use image-mode’s + and - bindings it
automatically falls back to ImageMagick, but if you just load and
insert an image like this:

    (insert-image (create-image "image.gif" nil nil :width 25))

it doesn’t. To get :width to do anything you need to specify that you
want to use ImageMagick:

    (insert-image (create-image "image.gif" imagemagick nil :width 25))

I suspect the simplest place to do it would be in create-image, but
someone who understands image.el better than me might have a different
opinion. (In other words: I hope someone else fancies implementing the
fallback, as I’m not very confident digging into lisp.)
-- 
Alan Third

Track mouse drags over an image (was: Add native image scaling)

[Roland Winkler]

Whenever I see the thread on "add native image scaling" it reminds
me of the following (kind of related):

For Djvu mode in GNU Elpa I needed code that tracks mouse drags over
an image, similar to what mouse-drag-track does for ordinary text.
This is useful if one needs to define a rectangular region in an
image via mouse events.  The way I implemented the tracking in elisp
is by converting the coordinates of the mouse events into image
coordinates, inverting the bits of the PPM image for the current
rectangle and redisplay the modified image (see the function
djvu-image-rect).  This code runs many times while dragging over the
image.  I was most surprised that this works sufficiently fast when
implemented in elisp to be usable.  Still I am wondering: would it
make sense to implement such code in a built-in function?  (This
would be beyond my knowledge of hacking Emacs.)  Are there other
situations where such a built-in function would be useful?
(Or am I possibly missing something better that exists already?)

Roland

Re: [PATCH v2] Add native image scaling (bug#33587)

[Eli Zaretskii]

> Date: Fri, 4 Jan 2019 19:09:14 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> On Fri, Jan 04, 2019 at 04:31:41PM +0200, Eli Zaretskii wrote:
> > > Date: Wed, 2 Jan 2019 21:12:41 +0000
> > > From: Alan Third <alan@idiocy.org>
> > > Cc: emacs-devel@gnu.org
> > > 
> > > I think this is the final version.
> > 
> > Thanks.  A few minor gotchas.
> 
> I regret touching this now. ;)
> 
> Do these look OK?

Yes, thanks.

Re: Track mouse drags over an image

[Stefan Monnier]

> implemented in elisp to be usable.  Still I am wondering: would it
> make sense to implement such code in a built-in function?

Not sure what you mean by "such code" nor "built-in".
Providing an Elisp function to help coders like you would be very
welcome, indeed.  If you can try and extract the relevant code from
djvu-mode, that would be great.

Implementing it in C would only be justified if the performance
difference is relevant or if the semantics can be made cleaner.
E.g. make it work for any image rather than only for PPM images of
"depth" 255.  Images are pretty far from my area of expertise, tho, so
someone else should look into this.


        Stefan

Re: [PATCH v2] Add native image scaling (bug#33587)

[Alan Third]

I’ve pushed this to master as‐is. If nobody else picks it up I’ll see
if I can work out an ImageMagick fall‐back system.
-- 
Alan Third

Re: [PATCH v2] Add native image scaling (bug#33587)

[Eli Zaretskii]

> Date: Thu, 10 Jan 2019 19:42:50 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I’ve pushed this to master as‐is.

Thanks.

Re: [PATCH v2] Add native image scaling (bug#33587)

[Paul Eggert]

On 1/10/19 11:42 AM, Alan Third wrote:
> I’ve pushed this to master as‐is. If nobody else picks it up I’ll see
> if I can work out an ImageMagick fall‐back system.

Thanks for doing all that. I tweaked the indentation a bit to fit in 80 
columns etc.

At some point soon I plan to install the patch in Bug#33587#5, which 
would disable ImageMagick by default when configuring Emacs (you can 
still enable it if you like). On my Fedora 29 desktop this patch 
decouples Emacs from libMagickCore, libMagickWand, libfftw3 (the FFT 
library), libgomp (OpenMP), libtldl (libtool), and libXt (X toolkit), 
which shaves 5-10 ms off the Emacs startup time in my little experiments.

Re: Track mouse drags over an image

[Roland Winkler]

On Tue, Jan 08 2019, Stefan Monnier wrote:
>> implemented in elisp to be usable.  Still I am wondering: would it
>> make sense to implement such code in a built-in function?
>
> Not sure what you mean by "such code" nor "built-in".  Providing an
> Elisp function to help coders like you would be very welcome, indeed.
> If you can try and extract the relevant code from djvu-mode, that
> would be great.

My question was indeed different: is there any possible use for such a
feature in other elisp packages that could justify such efforts?
Djvu-mode uses this for making annotations.  Could something like that
be useful for other packages, too (packages I do not know about)?

If the answer is yes, one can go from there.

> Implementing it in C would only be justified if the performance
> difference is relevant or if the semantics can be made cleaner.

The elisp implementation in djvu-mode has a terrible performance, as it
pushes the cpu load on my computer to 100% while doing its job.
Possibly, even elisp might permit a better solution than what djvu-mode
is currently doing.  But all this is not my area of expertise either.

> E.g. make it work for any image rather than only for PPM images of
> "depth" 255.  Images are pretty far from my area of expertise, tho, so
> someone else should look into this.

PPM (or PGM) images of "depth" 255 allow most easily to highlight the
dragged region as they represent each pixel by three (or one) byte that
can easily be addressed and inverted in elisp.  I do not know how to do
the same thing with, say, PBM images that otherwise could probably be
more efficient.

Re: Track mouse drags over an image

[Stefan Monnier]

> My question was indeed different: is there any possible use for such a
> feature in other elisp packages that could justify such efforts?
> Djvu-mode uses this for making annotations.  Could something like that
> be useful for other packages, too (packages I do not know about)?

I think I wanted to use such a drag in doc-view-mode to let the user
"zoom to the selected area", but never got to it (in part because of
the tedium of writing that code to do the highlighting of the rectangle
while tracking mouse movements).

I'd expect pdf-tools to want this as well (and there's a chance it's
already implemented there also).

>> E.g. make it work for any image rather than only for PPM images of
>> "depth" 255.  Images are pretty far from my area of expertise, tho, so
>> someone else should look into this.
> PPM (or PGM) images of "depth" 255 allow most easily to highlight the
> dragged region as they represent each pixel by three (or one) byte that
> can easily be addressed and inverted in elisp.  I do not know how to do
> the same thing with, say, PBM images that otherwise could probably be
> more efficient.

I think there's a chance that the end result can be obtained quite
differently (e.g. first convert the source image into a pixmap and then
use some existing toolkit functionality to do the image manipulation,
just like we recently added code to the image resizing).


        Stefan