No support for ImageMagick 7 in emacs-26

No support for ImageMagick 7 in emacs-26

[Ulrich Mueller]

Given that ImageMagick 7 was released in April 2016, wouldn't it make
sense to support that version in the upcoming Emacs 26.2?

AFAICS, commits 5729486951, bf1b147b55, 42ed35c68b, and 3cc42bb600 would
have to be backported for that, which doesn't seem too intrusive.

(In fact, Gentoo has a consolidated patch for 26.1:
https://gitweb.gentoo.org/proj/emacs-patches.git/tree/emacs/26.1/02_all_imagemagick-7.patch)

Re: No support for ImageMagick 7 in emacs-26

[Eli Zaretskii]

> From: Ulrich Mueller <ulm@gentoo.org>
> Date: Sun, 25 Nov 2018 09:26:56 +0100
> 
> Given that ImageMagick 7 was released in April 2016, wouldn't it make
> sense to support that version in the upcoming Emacs 26.2?
> 
> AFAICS, commits 5729486951, bf1b147b55, 42ed35c68b, and 3cc42bb600 would
> have to be backported for that, which doesn't seem too intrusive.
> 
> (In fact, Gentoo has a consolidated patch for 26.1:
> https://gitweb.gentoo.org/proj/emacs-patches.git/tree/emacs/26.1/02_all_imagemagick-7.patch)

It doesn't seem entirely trivial to me.  ImageMagick caused quitea few
problems to Emacs, so at this point I can only accept patches to
support v7 on the release branch if the code used by v6.x is
completely unchanged.

Thanks.

Re: No support for ImageMagick 7 in emacs-26

[Ulrich Mueller]

>>>>> On Sun, 25 Nov 2018, Eli Zaretskii wrote:

>> From: Ulrich Mueller <ulm@gentoo.org>
>> Date: Sun, 25 Nov 2018 09:26:56 +0100
>> 
>> Given that ImageMagick 7 was released in April 2016, wouldn't it make
>> sense to support that version in the upcoming Emacs 26.2?
>> 
>> AFAICS, commits 5729486951, bf1b147b55, 42ed35c68b, and 3cc42bb600
>> would have to be backported for that, which doesn't seem too
>> intrusive.
>> 
>> (In fact, Gentoo has a consolidated patch for 26.1:
>> https://gitweb.gentoo.org/proj/emacs-patches.git/tree/emacs/26.1/02_all_imagemagick-7.patch)

> It doesn't seem entirely trivial to me.  ImageMagick caused quitea few
> problems to Emacs, so at this point I can only accept patches to
> support v7 on the release branch if the code used by v6.x is
> completely unchanged.

Sorry, but where do you see a change that could affect behaviour of
v6.x? Effectively, the only code change in the consolidated patch is the
following in image.c:

-      MagickRealType color_scale = 65535.0 / QuantumRange;
+      double quantum_range = QuantumRange;
+      MagickRealType color_scale = 65535.0 / quantum_range;

Also, I noticed that configure will silently disable imagemagick if v6
is not found. This is o.k. for the default, but when I explicitly
specify --with-imagemagick, then I would expect it to error out when it
cannot enable the feature.

Re: No support for ImageMagick 7 in emacs-26

[Eli Zaretskii]

> From: Ulrich Mueller <ulm@gentoo.org>
> Cc: emacs-devel@gnu.org
> Date: Mon, 26 Nov 2018 08:38:21 +0100
> 
> > It doesn't seem entirely trivial to me.  ImageMagick caused quitea few
> > problems to Emacs, so at this point I can only accept patches to
> > support v7 on the release branch if the code used by v6.x is
> > completely unchanged.
> 
> Sorry, but where do you see a change that could affect behaviour of
> v6.x? Effectively, the only code change in the consolidated patch is the
> following in image.c:
> 
> -      MagickRealType color_scale = 65535.0 / QuantumRange;
> +      double quantum_range = QuantumRange;
> +      MagickRealType color_scale = 65535.0 / quantum_range;

This is one part that I'd rather not do on the release branch (why is
it needed, anyway?).  The other one is that inclusion of a header file
was moved to a different place for some reason.  (Yes, I'm being
paranoid ;-)

> Also, I noticed that configure will silently disable imagemagick if v6
> is not found. This is o.k. for the default, but when I explicitly
> specify --with-imagemagick, then I would expect it to error out when it
> cannot enable the feature.

Patches to that effect are welcome, but that part should definitely
not go to the release branch.

Thanks.

Re: No support for ImageMagick 7 in emacs-26

[Ulrich Mueller]

>>>>> On Mon, 26 Nov 2018, Eli Zaretskii wrote:

>> Sorry, but where do you see a change that could affect behaviour of
>> v6.x? Effectively, the only code change in the consolidated patch is the
>> following in image.c:
>> 
>> -      MagickRealType color_scale = 65535.0 / QuantumRange;
>> +      double quantum_range = QuantumRange;
>> +      MagickRealType color_scale = 65535.0 / quantum_range;

> This is one part that I'd rather not do on the release branch

We could put a conditional around it (but it looks silly):

#ifdef HAVE_IMAGEMAGICK7
      double quantum_range = QuantumRange;
      MagickRealType color_scale = 65535.0 / quantum_range;
#else
      MagickRealType color_scale = 65535.0 / QuantumRange;
#fi

> (why is it needed, anyway?).

According to the commit message (commit 42ed35c68b):
    
    * src/image.c (imagemagick_load_image): Use double division, and
    eliminate a cast.  This avoids a -Wdouble-promotion warning with
    GCC 7.3 on Ubuntu 18.04.

> The other one is that inclusion of a header file was moved to a
> different place for some reason.  (Yes, I'm being paranoid ;-)

For v6 it still includes <wand/MagickWand.h> and <magick/version.h> at
the same place and in the same order. So unless I am missing something,
nothing was moved at all.

IMHO it would be shortsighted to support only a version that ImageMagick
upstream considers legacy. (And yes, it can be fixed at the distro
level, but that just means duplicate work.)

Re: No support for ImageMagick 7 in emacs-26

[Andreas Schwab]

On Nov 26 2018, Ulrich Mueller <ulm@gentoo.org> wrote:

> According to the commit message (commit 42ed35c68b):
>     
>     * src/image.c (imagemagick_load_image): Use double division, and
>     eliminate a cast.  This avoids a -Wdouble-promotion warning with
>     GCC 7.3 on Ubuntu 18.04.

With other words, not needed anyway.

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

Re: No support for ImageMagick 7 in emacs-26

[Paul Eggert]

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

On 11/25/18 11:38 PM, Ulrich Mueller wrote:
>>> (In fact, Gentoo has a consolidated patch for 26.1:
>>> https://gitweb.gentoo.org/proj/emacs-patches.git/tree/emacs/26.1/02_all_imagemagick-7.patch)
> Effectively, the only code change in the consolidated patch is the
> following in image.c:
>
> -      MagickRealType color_scale = 65535.0 / QuantumRange;
> +      double quantum_range = QuantumRange;
> +      MagickRealType color_scale = 65535.0 / quantum_range;

We don't need to apply this to the emacs-26 branch, as it does not 
affect behavior and is used only to pacify a GCC warning. I suppose 
Gentoo enables warnings somehow, and doesn't want that warning. But in 
general we no longer worry about --enable-gcc-warnings diagnostics in 
the emacs-26 branch.

Anyway, the business part of the Gentoo patch is everything else in that 
patch.

On 11/26/18 9:38 AM, Eli Zaretskii wrote:
 > The other one is that inclusion of a header file
 > was moved to a different place for some reason.  (Yes, I'm being
 > paranoid

This is OK since exactly the same include files are included, in exactly 
the same order, when HAVE_IMAGEMAGICK7 is not defined.

So how about the attached patch for the emacs-26 branch?


[-- Attachment #2: 0001-Support-ImageMagick-version-7.patch --]
[-- Type: text/x-patch, Size: 3462 bytes --]

From 9648f9e622471b36111847c0ed352303c8c09504 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 26 Nov 2018 17:37:16 -0800
Subject: [PATCH] Support ImageMagick version 7

Backport from master.
* configure.ac (HAVE_IMAGEMAGICK7): New macro.
(HAVE_IMAGEMAGICK): Also define if using ImageMagick 7 or later.
* src/image.c [HAVE_IMAGEMAGICK7]: Include
<MagickWand/MagickWand.h> and <MagickCore/version.h> instead of
<wand/MagickWand.h> and <magick/version.h>.
(PixelSetMagickColor, MagickPixelPacket) [HAVE_IMAGEMAGICK7]:
New compatibility definitions.
---
 configure.ac | 15 +++++++++------
 src/image.c  | 13 +++++++++++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index dc6d776d45..a4d0feaad5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2521,11 +2521,14 @@ AC_DEFUN
 HAVE_IMAGEMAGICK=no
 if test "${HAVE_X11}" = "yes" || test "${HAVE_NS}" = "yes" || test "${HAVE_W32}" = "yes"; then
   if test "${with_imagemagick}" != "no"; then
-    ## 6.3.5 is the earliest version known to work; see Bug#17339.
-    ## 6.8.2 makes Emacs crash; see Bug#13867.
-    ## 7 and later have not been ported to; See Bug#25967.
-    IMAGEMAGICK_MODULE="Wand >= 6.3.5 Wand != 6.8.2 Wand < 7"
-    EMACS_CHECK_MODULES([IMAGEMAGICK], [$IMAGEMAGICK_MODULE])
+    EMACS_CHECK_MODULES([IMAGEMAGICK], [MagickWand >= 7])
+    if test $HAVE_IMAGEMAGICK = yes; then
+       AC_DEFINE([HAVE_IMAGEMAGICK7], 1, [Define to 1 if using ImageMagick7.])
+    else
+       ## 6.3.5 is the earliest version known to work; see Bug#17339.
+       ## 6.8.2 makes Emacs crash; see Bug#13867.
+       EMACS_CHECK_MODULES([IMAGEMAGICK], [Wand >= 6.3.5 Wand != 6.8.2])
+    fi
 
     if test $HAVE_IMAGEMAGICK = yes; then
       OLD_CFLAGS=$CFLAGS
@@ -5404,7 +5407,7 @@ AC_DEFUN
   Does Emacs use -lrsvg-2?                                ${HAVE_RSVG}
   Does Emacs use cairo?                                   ${HAVE_CAIRO}
   Does Emacs use -llcms2?                                 ${HAVE_LCMS2}
-  Does Emacs use imagemagick (version 6)?                 ${HAVE_IMAGEMAGICK}
+  Does Emacs use imagemagick?                             ${HAVE_IMAGEMAGICK}
   Does Emacs support sound?                               ${HAVE_SOUND}
   Does Emacs use -lgpm?                                   ${HAVE_GPM}
   Does Emacs use -ldbus?                                  ${HAVE_DBUS}
diff --git a/src/image.c b/src/image.c
index a6b2d9060b..cc99f33d5f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8272,11 +8272,20 @@ imagemagick_image_p (Lisp_Object object)
 /* The GIF library also defines DrawRectangle, but its never used in Emacs.
    Therefore rename the function so it doesn't collide with ImageMagick.  */
 #define DrawRectangle DrawRectangleGif
-#include <wand/MagickWand.h>
+
+#ifdef HAVE_IMAGEMAGICK7
+# include <MagickWand/MagickWand.h>
+# include <MagickCore/version.h>
+/* ImageMagick 7 compatibility definitions.  */
+# define PixelSetMagickColor PixelSetPixelColor
+typedef PixelInfo MagickPixelPacket;
+#else
+# include <wand/MagickWand.h>
+# include <magick/version.h>
+#endif
 
 /* ImageMagick 6.5.3 through 6.6.5 hid PixelGetMagickColor for some reason.
    Emacs seems to work fine with the hidden version, so unhide it.  */
-#include <magick/version.h>
 #if 0x653 <= MagickLibVersion && MagickLibVersion <= 0x665
 extern WandExport void PixelGetMagickColor (const PixelWand *,
 					    MagickPixelPacket *);
-- 
2.19.1

Re: No support for ImageMagick 7 in emacs-26

[Eli Zaretskii]

> From: Ulrich Mueller <ulm@gentoo.org>
> Cc: emacs-devel@gnu.org
> Date: Mon, 26 Nov 2018 23:00:48 +0100
> 
> We could put a conditional around it (but it looks silly):
> 
> #ifdef HAVE_IMAGEMAGICK7
>       double quantum_range = QuantumRange;
>       MagickRealType color_scale = 65535.0 / quantum_range;
> #else
>       MagickRealType color_scale = 65535.0 / QuantumRange;
> #fi
> 
> > (why is it needed, anyway?).
> 
> According to the commit message (commit 42ed35c68b):
>     
>     * src/image.c (imagemagick_load_image): Use double division, and
>     eliminate a cast.  This avoids a -Wdouble-promotion warning with
>     GCC 7.3 on Ubuntu 18.04.

So it's not really related to ImageMagick 7 support, right?

> IMHO it would be shortsighted to support only a version that ImageMagick
> upstream considers legacy.

I may be shortsighted, but I'm not against adding v7 support, if it is
done safely, as Emacs 26.2 is supposed to be more stable than 26.1,
not less stable.

Thanks.

Re: No support for ImageMagick 7 in emacs-26

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 26 Nov 2018 17:38:28 -0800
> 
> > -      MagickRealType color_scale = 65535.0 / QuantumRange;
> > +      double quantum_range = QuantumRange;
> > +      MagickRealType color_scale = 65535.0 / quantum_range;
> 
> We don't need to apply this to the emacs-26 branch, as it does not 
> affect behavior and is used only to pacify a GCC warning. I suppose 
> Gentoo enables warnings somehow, and doesn't want that warning. But in 
> general we no longer worry about --enable-gcc-warnings diagnostics in 
> the emacs-26 branch.

Agreed.

> So how about the attached patch for the emacs-26 branch?

Fine with me, except:

> --- a/src/image.c
> +++ b/src/image.c
> @@ -8272,11 +8272,20 @@ imagemagick_image_p (Lisp_Object object)
>  /* The GIF library also defines DrawRectangle, but its never used in Emacs.
>     Therefore rename the function so it doesn't collide with ImageMagick.  */
>  #define DrawRectangle DrawRectangleGif
> -#include <wand/MagickWand.h>
> +
> +#ifdef HAVE_IMAGEMAGICK7
> +# include <MagickWand/MagickWand.h>
> +# include <MagickCore/version.h>
> +/* ImageMagick 7 compatibility definitions.  */
> +# define PixelSetMagickColor PixelSetPixelColor
> +typedef PixelInfo MagickPixelPacket;
> +#else
> +# include <wand/MagickWand.h>
> +# include <magick/version.h>
> +#endif
>  
>  /* ImageMagick 6.5.3 through 6.6.5 hid PixelGetMagickColor for some reason.
>     Emacs seems to work fine with the hidden version, so unhide it.  */
> -#include <magick/version.h>
>  #if 0x653 <= MagickLibVersion && MagickLibVersion <= 0x665
>  extern WandExport void PixelGetMagickColor (const PixelWand *,
>  					    MagickPixelPacket *);

It makes little sense to me to move the inclusion of magick/version.h
into the v6 conditional, but leave the "#if 0x653" conditional
fragment and its comment outside that v6 conditional.  AFAIU, we are
including version.h _because_ we want that second #if, right?  So we
should keep them close together, and keep the comment explaining why
we do that before the code it explains.  (Which begs the question
whether we need to include MagickCore/version.h for v7, but I'm not
going to bother about that.)

Thanks.

Re: No support for ImageMagick 7 in emacs-26

[Paul Eggert]

Eli Zaretskii wrote:

> (Which begs the question
> whether we need to include MagickCore/version.h for v7, but I'm not
> going to bother about that.)

We include MagickCore/version.h because it is the v7 counterpart to v6's 
magick/version.h, e.g., it defines the MagickLibVersion macro that the following 
code uses. I could add a comment to that effect, though to be honest this seems 
overkill to me.