bug#33587: [PROPOSED] Default to disabling ImageMagick

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Paul Eggert]

From: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>

ImageMagick has continuing stability and security problems, suggesting
that 'configure' should disable it by default.  See Glenn Morris's notes
at: https://lists.gnu.org/r/emacs-devel/2018-12/msg00036.html
* INSTALL, etc/NEWS: Mention this.
* configure.ac (imagemagick): Default to off.
---
 INSTALL      | 4 +++-
 configure.ac | 2 +-
 etc/NEWS     | 4 ++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/INSTALL b/INSTALL
index 0c56fff6d4..9696904dce 100644
--- a/INSTALL
+++ b/INSTALL
@@ -294,7 +294,9 @@ or more of these options:
   --without-gif          for GIF image support
   --without-png          for PNG image support
   --without-rsvg         for SVG image support
-  --without-imagemagick  for Imagemagick support
+
+Although ImageMagick support is disabled by default due to security
+and stability concerns, you can enable it with --with-imagemagick.
 
 Use --without-toolkit-scroll-bars to disable Motif or Xaw3d scroll bars.
 
diff --git a/configure.ac b/configure.ac
index 8b34c3b658..b70393925a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -354,7 +354,7 @@ AC_DEFUN
 OPTION_DEFAULT_ON([libsystemd],[don't compile with libsystemd support])
 OPTION_DEFAULT_OFF([cairo],[compile with Cairo drawing (experimental)])
 OPTION_DEFAULT_ON([xml2],[don't compile with XML parsing support])
-OPTION_DEFAULT_ON([imagemagick],[don't compile with ImageMagick image support])
+OPTION_DEFAULT_OFF([imagemagick],[compile with ImageMagick image support])
 OPTION_DEFAULT_ON([json], [don't compile with native JSON support])
 
 OPTION_DEFAULT_ON([xft],[don't use XFT for anti aliased fonts])
diff --git a/etc/NEWS b/etc/NEWS
index 6297d07879..07c6f74c44 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -37,6 +37,10 @@ functions 'json-serialize', 'json-insert', 'json-parse-string', and
 'json-parse-buffer' are typically much faster than their Lisp
 counterparts from json.el.
 
+** Emacs no longer defaults to using ImageMagick to display images,
+due to security and stability concerns.  To override the default, use
+'configure --with-imagemagick'.
+
 ** The etags program now uses the C library's regular expression matcher
 when possible, and a compatible regex substitute otherwise.  This will
 let developers maintain Emacs's own regex code without having to also
-- 
2.19.2

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun,  2 Dec 2018 10:09:19 -0800
> Cc: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>
> 
> From: Paul Eggert <eggert@Penguin.CS.UCLA.EDU>
> 
> ImageMagick has continuing stability and security problems, suggesting
> that 'configure' should disable it by default.  See Glenn Morris's notes
> at: https://lists.gnu.org/r/emacs-devel/2018-12/msg00036.html
> * INSTALL, etc/NEWS: Mention this.
> * configure.ac (imagemagick): Default to off.

No objections from me, but let's please wait for a week, to let people
chance to voice objections.

Thanks.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Andreas Schwab]

On Dez 02 2018, Paul Eggert <eggert@cs.ucla.edu> wrote:

> +** Emacs no longer defaults to using ImageMagick to display images,
> +due to security and stability concerns.  To override the default, use
> +'configure --with-imagemagick'.

ImageMagick is the only backend that supports scaling.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Paul Eggert]

Andreas Schwab wrote:
> ImageMagick is the only backend that supports scaling.

Good point, and if we make the change, the scaling issue should be mentioned in 
INSTALL. Perhaps something like the following wording:

"Although ImageMagick support is disabled by default due to security
and stability concerns, you can enable it by configuring with
--with-imagemagick.  ImageMagick is the only backend that supports
image scaling."

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Alan Third]

On Sun, Dec 02, 2018 at 03:51:57PM -0800, Paul Eggert wrote:
> Andreas Schwab wrote:
> > ImageMagick is the only backend that supports scaling.
> 
> Good point, and if we make the change, the scaling issue should be mentioned
> in INSTALL. Perhaps something like the following wording:
> 
> "Although ImageMagick support is disabled by default due to security
> and stability concerns, you can enable it by configuring with
> --with-imagemagick.  ImageMagick is the only backend that supports
> image scaling."

FWIW the NS port on master supports scaling through the NS toolkit,
although there is the problem that most lisp code that wants to scale
checks exclusively for imagemagick support.
-- 
Alan Third

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Glenn Morris]

I'm a bit surprised by the lack of objections so far, though it's early
days yet of course. Maybe it's an experiment that needs to be tried out
for the implications to be seen.

A related alternative would be to lower the priority of the ImageMagick
backend. At the moment, visiting eg a png image uses ImageMagick rather
than libpng if both are linked in.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Paul Eggert]

On 12/3/18 11:08 AM, Glenn Morris wrote:
> A related alternative would be to lower the priority of the ImageMagick
> backend. At the moment, visiting eg a png image uses ImageMagick rather
> than libpng if both are linked in.

If this alternative is taken and the user requests scaling, presumably 
the ImageMagick library would need to be used anyway since it can scale 
and libpng can't.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Glenn Morris]

Paul Eggert wrote:

> On 12/3/18 11:08 AM, Glenn Morris wrote:
>> A related alternative would be to lower the priority of the ImageMagick
>> backend. At the moment, visiting eg a png image uses ImageMagick rather
>> than libpng if both are linked in.
>
> If this alternative is taken and the user requests scaling, presumably
> the ImageMagick library would need to be used anyway since it can
> scale and libpng can't.

Sure. I mean, make use of ImageMagick require an explicit request, for
uses that might need those features (eww?), rather than just happening
by default like it does now.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[David Engster]

Glenn Morris writes:
> I'm a bit surprised by the lack of objections so far, though it's early
> days yet of course. Maybe it's an experiment that needs to be tried out
> for the implications to be seen.

Well, I do depend on image scaling, but I (like many others here, I
guess) build Emacs myself, so defaults don't matter much to me.

Question is: will disabling Imagemagick by default also have an impact
on how Emacs is shipped in distributions? I don't think so, at least as
long as they don't drop Imagemagick completely. If for instance Debian
has to take care of Imagemagick security issues anyway, why shouldn't
Emacs link to it?

But that's just my guess...

-David

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Glenn Morris]

David Engster wrote:

> Question is: will disabling Imagemagick by default also have an impact
> on how Emacs is shipped in distributions?

I don't know. It depends whether they go with the default configure
options or not.

> I don't think so, at least as long as they don't drop Imagemagick
> completely.

Note that Red Hat Enterprise Linux 8 _will_ drop ImageMagick completely
(though it will probably be available from an add-on repository),
presumably because they don't feel able to keep up with the security
issues. That's what prompted me to first raise this in

http://lists.gnu.org/r/emacs-devel/2018-12/msg00036.html

> If for instance Debian has to take care of Imagemagick security issues
> anyway, why shouldn't Emacs link to it?

(For reference:
https://security-tracker.debian.org/tracker/source-package/imagemagick )

Because one can never guarantee all security issues are fixed, and if a
project has a history of having a lot of them, it may be considered
likely to be insecure. Also there are the various Emacs crash reports
due to ImageMagick.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[David Engster]

Glenn Morris writes:
> Note that Red Hat Enterprise Linux 8 _will_ drop ImageMagick completely
> (though it will probably be available from an add-on repository),
> presumably because they don't feel able to keep up with the security
> issues. That's what prompted me to first raise this in
>
> http://lists.gnu.org/r/emacs-devel/2018-12/msg00036.html

RHEL can do this because they're supporting way less packages than other
distributions. As you know, enterprise customers have other priorities
than home desktop users. Debian cannot remove Imagemagick because many
other packages depend on it, at least currently.

>> If for instance Debian has to take care of Imagemagick security issues
>> anyway, why shouldn't Emacs link to it?
>
> (For reference:
> https://security-tracker.debian.org/tracker/source-package/imagemagick )
>
> Because one can never guarantee all security issues are fixed, and if a
> project has a history of having a lot of them, it may be considered
> likely to be insecure. Also there are the various Emacs crash reports
> due to ImageMagick.

I understand the reasoning. To me, image scaling is essential for what
I'm doing with Emacs, so I'm willing to take that risk. But that's just
one data point.

Don't get me wrong: I don't object to disable it by default. Let's see
what happens. Maybe distributions will then disable it as well, but they
have their own ways to see how changes like these affect users (by
having an 'unstable' tree or whatever).

-David

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Glenn Morris]

PS GraphicsMagick allegedly has fewer security issues than ImageMagick,
but https://debbugs.gnu.org/14358 saw no interest.

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Paul Eggert]

Elias Mårtenson wrote in 
<http://lists.gnu.org/r/emacs-devel/2018-12/msg00186.html> that image 
scaling via Xrender is surprisingly simple. So perhaps an X11 expert 
could investigate doing that for the X Window System, when ImageMagick 
scaling is not available or not used. My impression is that the Xrender 
extension (introduced in 2000) is reasonably popular among X11 servers 
these days.

Scaling on the server could also be faster (e.g., with hardware 
acceleration) and/or more reliable, so quite possibly it'd be better to 
use Xrender to scale even if ImageMagick is available.

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

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)

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

bug#33587: [PROPOSED] Default to disabling ImageMagick

[Paul Eggert]

Paul Eggert wrote:

> At some point soon I plan to install the patch in Bug#33587#5

It wasn't soon, but I did install the patch just now. Closing the bug report.