imagemmagick patch 5

imagemmagick patch 5

[joakim]

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

The Imagemagick patch allows Emacs to use the Imagemagick libraries to
load images. This support can be used in parallell with the existing
image loading support. There is support for asking your imagemagick
installation which image types it supports, and registering them in
Emacs selectively.

Apart from bugfixes, this version has support for selecting between two
different render methods, one true and tried but somewhat slow(not
slower than the render method used by some existing image libraries
support in emacs) and a new a bit optimized method.

I use this patch a lot for viewing scanned djvu packages. I also have a
emacs scanner frontend called "emsane" I'm using together with this
patch. 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: imagemagick5.patch --]
[-- Type: text/x-patch, Size: 24982 bytes --]

diff --git a/.gitignore b/.gitignore
index 760c452..f10d4e2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,5 +4,7 @@ makefile
 
 /bin/
 /site-lisp/
+*.o
+Makefile
 
 # arch-tag: 75cd3c50-1875-4814-8360-190f7de38302
diff --git a/configure.in b/configure.in
index 32208c7..bee4eb7 100644
--- a/configure.in
+++ b/configure.in
@@ -132,6 +132,8 @@ OPTION_DEFAULT_ON([tiff],[don't compile with TIFF image support])
 OPTION_DEFAULT_ON([gif],[don't compile with GIF image support])
 OPTION_DEFAULT_ON([png],[don't compile with PNG image support])
 OPTION_DEFAULT_ON([rsvg],[don't compile with SVG image support])
+OPTION_DEFAULT_OFF([imagemagick],[compile with ImageMagick image support])
+
 
 OPTION_DEFAULT_ON([xft],[don't use XFT for anti aliased fonts])
 OPTION_DEFAULT_ON([libotf],[don't use libotf for OpenType font support])
@@ -142,6 +144,8 @@ OPTION_DEFAULT_ON([xaw3d],[don't use Xaw3d])
 OPTION_DEFAULT_ON([xim],[don't use X11 XIM])
 OPTION_DEFAULT_OFF([ns],[use nextstep (Cocoa or GNUstep) windowing system])
 
+
+
 OPTION_DEFAULT_ON([gpm],[don't use -lgpm for mouse support on a GNU/Linux console])
 OPTION_DEFAULT_ON([dbus],[don't compile with D-Bus support])
 OPTION_DEFAULT_ON([gconf],[don't compile with GConf support])
@@ -1628,6 +1632,24 @@ if test "${HAVE_X11}" = "yes" || test "${NS_IMPL_GNUSTEP}" = "yes"; then
   fi
 fi
 
+HAVE_IMAGEMAGICK=no
+if test "${HAVE_X11}" = "yes" ; then
+  if test "${with_imagemagick}" != "no"; then
+    IMAGEMAGICK_REQUIRED=1
+    IMAGEMAGICK_MODULE="MagickWand >= $IMAGEMAGICK_REQUIRED"
+
+    PKG_CHECK_MODULES(IMAGEMAGICK, $IMAGEMAGICK_MODULE, :, :)
+    AC_SUBST(IMAGEMAGICK_CFLAGS)
+    AC_SUBST(IMAGEMAGICK_LIBS)
+
+    if test ".${IMAGEMAGICK_CFLAGS}" != "."; then
+      HAVE_IMAGEMAGICK=yes
+      AC_DEFINE(HAVE_IMAGEMAGICK, 1, [Define to 1 if using imagemagick.])
+      CFLAGS="$CFLAGS $IMAGEMAGICK_CFLAGS"
+      LIBS="$IMAGEMAGICK_LIBS $LIBS"
+    fi    
+  fi
+fi
 
 HAVE_GTK=no
 if test "${with_gtk}" = "yes" || test "$USE_X_TOOLKIT" = "maybe"; then
@@ -3007,6 +3029,7 @@ echo "  Does Emacs use -ltiff?                                  ${HAVE_TIFF}"
 echo "  Does Emacs use a gif library?                           ${HAVE_GIF} $ac_gif_lib_name"
 echo "  Does Emacs use -lpng?                                   ${HAVE_PNG}"
 echo "  Does Emacs use -lrsvg-2?                                ${HAVE_RSVG}"
+echo "  Does Emacs use imagemagick?                             ${HAVE_IMAGEMAGICK}"
 echo "  Does Emacs use -lgpm?                                   ${HAVE_GPM}"
 echo "  Does Emacs use -ldbus?                                  ${HAVE_DBUS}"
 echo "  Does Emacs use -lgconf?                                 ${HAVE_GCONF}"
diff --git a/lisp/dired.el b/lisp/dired.el
index 6b2299b..708a971 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3517,7 +3517,7 @@ Ask means pop up a menu for the user to select one of copy, move or link."
 ;;;;;;  dired-run-shell-command dired-do-shell-command dired-do-async-shell-command
 ;;;;;;  dired-clean-directory dired-do-print dired-do-touch dired-do-chown
 ;;;;;;  dired-do-chgrp dired-do-chmod dired-compare-directories dired-backup-diff
-;;;;;;  dired-diff) "dired-aux" "dired-aux.el" "083026f814e1a734adb593f7034ecb7e")
+;;;;;;  dired-diff) "dired-aux" "dired-aux.el" "c7d3d0354ca849c90d3aaf33fa06341d")
 ;;; Generated autoloads from dired-aux.el
 
 (autoload 'dired-diff "dired-aux" "\
diff --git a/lisp/image.el b/lisp/image.el
index e2f4977..d6cdc68 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -70,6 +70,8 @@ a non-nil value, TYPE is the image's type.")
     ("\\.ps\\'" . postscript)
     ("\\.tiff?\\'" . tiff)
     ("\\.svgz?\\'" . svg)
+    ("\\.xcf?\\'" . imagemagick)
+    ("\\.djvu?\\'" . imagemagick)
     )
   "Alist of (REGEXP . IMAGE-TYPE) pairs used to identify image files.
 When the name of an image file match REGEXP, it is assumed to
@@ -585,6 +587,30 @@ Example:
   `(defvar ,symbol (find-image ',specs) ,doc))
 
 
+(defconst imagemagick-types-inhibit
+  '(C HTML HTM TXT)
+  "Types the imagemagic loader should not handle.")
+
+;;;###autoload
+(defun imagemagic-register-types ()
+  "Register file types that imagemagick is able to handle."
+  (let ((im-types (imagemagick-types))
+        (im-inhibit imagemagick-types-inhibit))
+    (while im-inhibit
+      (setq im-types (remove (car im-inhibit) im-types))
+      (setq im-inhibit (cdr im-inhibit)))
+    (while im-types
+      (let
+	  ((extension (downcase (symbol-name (car im-types)))))
+	(push
+	 (cons  (concat "\\." extension "\\'") 'image-mode)
+	 auto-mode-alist)
+	(push
+	 (cons  (concat "\\." extension "\\'") 'imagemagick)
+	 image-type-file-name-regexps)
+	(setq im-types (cdr im-types))))))
+
+
 (provide 'image)
 
 ;; arch-tag: 8e76a07b-eb48-4f3e-a7a0-1a7ba9f096b3
diff --git a/src/Makefile.in b/src/Makefile.in
index 1e956ae..636f2c7 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -266,7 +266,9 @@ GCONF_LIBS = @GCONF_LIBS@
 
 /* C_SWITCH_X_SITE must come before C_SWITCH_X_MACHINE and C_SWITCH_X_SYSTEM
    since it may have -I options that should override those two.  */
-ALL_CFLAGS=-Demacs -DHAVE_CONFIG_H $(MYCPPFLAGS) -I. -I${srcdir} C_SWITCH_MACHINE C_SWITCH_SYSTEM C_SWITCH_X_SITE C_SWITCH_X_MACHINE C_SWITCH_X_SYSTEM C_SWITCH_SYSTEM_TEMACS ${CFLAGS_SOUND} ${RSVG_CFLAGS} ${DBUS_CFLAGS} ${GCONF_CFLAGS} ${CFLAGS} @FREETYPE_CFLAGS@ @FONTCONFIG_CFLAGS@ @LIBOTF_CFLAGS@ @M17N_FLT_CFLAGS@ ${DEPFLAGS}
+
+ALL_CFLAGS=-Demacs -DHAVE_CONFIG_H $(MYCPPFLAGS) -I. -I${srcdir} C_SWITCH_MACHINE C_SWITCH_SYSTEM C_SWITCH_X_SITE C_SWITCH_X_MACHINE C_SWITCH_X_SYSTEM C_SWITCH_SYSTEM_TEMACS ${CFLAGS_SOUND} ${RSVG_CFLAGS} ${IMAGEMAGICK_CFLAGS} ${DBUS_CFLAGS} ${GCONF_CFLAGS} ${CFLAGS} @FREETYPE_CFLAGS@ @FONTCONFIG_CFLAGS@ @LIBOTF_CFLAGS@ @M17N_FLT_CFLAGS@ ${DEPFLAGS}
+
 ALL_OBJC_CFLAGS=$(ALL_CFLAGS) @GNU_OBJC_CFLAGS@
 
 .SUFFIXES: .m
@@ -445,6 +447,9 @@ CFLAGS_SOUND= @CFLAGS_SOUND@
 RSVG_LIBS= @RSVG_LIBS@
 RSVG_CFLAGS= @RSVG_CFLAGS@
 
+IMAGEMAGICK_LIBS= @IMAGEMAGICK_LIBS@
+IMAGEMAGICK_CFLAGS= @IMAGEMAGICK_CFLAGS@
+
 #ifndef ORDINARY_LINK
 /* Fix linking if compiled with GCC.  */
 #ifdef __GNUC__
@@ -909,7 +914,7 @@ SOME_MACHINE_LISP = ../lisp/mouse.elc \
    duplicated symbols.  If the standard libraries were compiled
    with GCC, we might need gnulib again after them.  */
 
-LIBES = $(LOADLIBES) $(LIBS) $(LIBX) $(LIBSOUND) $(RSVG_LIBS) $(DBUS_LIBS) \
+LIBES = $(LOADLIBES) $(LIBS) $(LIBX) $(LIBSOUND) $(RSVG_LIBS) ${IMAGEMAGICK_LIBS} $(DBUS_LIBS) \
    LIBGPM LIBRESOLV LIBS_SYSTEM LIBS_MACHINE LIBS_TERMCAP \
    LIBS_DEBUG $(GETLOADAVG_LIBS) ${GCONF_LIBS} \
    @FREETYPE_LIBS@ @FONTCONFIG_LIBS@ @LIBOTF_LIBS@ @M17N_FLT_LIBS@ \
diff --git a/src/dbusbind.c b/src/dbusbind.c
index 7c0be49..7a47730 100644
--- a/src/dbusbind.c
+++ b/src/dbusbind.c
@@ -773,6 +773,7 @@ xd_add_watch (watch, data)
       if (fd == -1)
 	return FALSE;
 
+
       /* Add the file descriptor to input_wait_mask.  */
       add_keyboard_wait_descriptor (fd);
     }
diff --git a/src/image.c b/src/image.c
index 8cd8355..9f3deae 100644
--- a/src/image.c
+++ b/src/image.c
@@ -606,7 +606,7 @@ extern Lisp_Object QCdata, QCtype;
 extern Lisp_Object Qcenter;
 Lisp_Object QCascent, QCmargin, QCrelief, Qcount;
 Lisp_Object QCconversion, QCcolor_symbols, QCheuristic_mask;
-Lisp_Object QCindex, QCmatrix, QCcolor_adjustment, QCmask;
+Lisp_Object QCindex, QCmatrix, QCcolor_adjustment, QCmask, QCgeometry, QCcrop, QCrotation;
 
 /* Other symbols.  */
 
@@ -7536,6 +7536,451 @@ gif_load (struct frame *f, struct image *img)
 #endif /* HAVE_GIF */
 
 
+/***********************************************************************
+				 imagemagick
+ ***********************************************************************/
+#if defined (HAVE_IMAGEMAGICK)
+Lisp_Object Vimagemagick_render_type;
+/* Function prototypes.  */
+
+static int imagemagick_image_p P_ ((Lisp_Object object));
+static int imagemagick_load P_ ((struct frame *f, struct image *img));
+
+static int imagemagick_load_image P_ ((struct frame *, struct image *,
+                                       unsigned char *, unsigned int, unsigned char *));
+
+/* The symbol `imagemagick' identifying images of this type. */
+
+Lisp_Object Qimagemagick;
+Lisp_Object Vimagemagick_render_type;
+
+/* Indices of image specification fields in imagemagick_format, below.  */
+
+enum imagemagick_keyword_index
+{
+  IMAGEMAGICK_TYPE,
+  IMAGEMAGICK_DATA,
+  IMAGEMAGICK_FILE,
+  IMAGEMAGICK_ASCENT,
+  IMAGEMAGICK_MARGIN,
+  IMAGEMAGICK_RELIEF,
+  IMAGEMAGICK_ALGORITHM,
+  IMAGEMAGICK_HEURISTIC_MASK,
+  IMAGEMAGICK_MASK,
+  IMAGEMAGICK_BACKGROUND,
+  IMAGEMAGICK_LAST
+};
+
+/* Vector of image_keyword structures describing the format
+   of valid user-defined image specifications.  */
+
+static struct image_keyword imagemagick_format[IMAGEMAGICK_LAST] =
+{
+  {":type",		IMAGE_SYMBOL_VALUE,			1},
+  {":data",		IMAGE_STRING_VALUE,			0},
+  {":file",		IMAGE_STRING_VALUE,			0},
+  {":ascent",		IMAGE_ASCENT_VALUE,			0},
+  {":margin",		IMAGE_POSITIVE_INTEGER_VALUE_OR_PAIR,	0},
+  {":relief",		IMAGE_INTEGER_VALUE,			0},
+  {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":background",	IMAGE_STRING_OR_NIL_VALUE,		0}
+};
+/* Free X resources of imagemagick image IMG which is used on frame F.  */
+
+static void
+imagemagick_clear_image (f, img)
+     struct frame *f;
+     struct image *img;
+{
+  //debug since i seem to be leaking somewhere
+  printf("clearing imagemagick image\n");
+  x_clear_image (f, img);
+}
+
+/* Structure describing the image type `imagemagick'.  Its the same type of
+   structure defined for all image formats, handled by emacs image
+   functions.  See struct image_type in dispextern.h.  */
+
+static struct image_type imagemagick_type =
+{
+  /* An identifier showing that this is an image structure for the IMAGEMAGICK format.  */
+  &Qimagemagick,
+  /* Handle to a function that can be used to identify a IMAGEMAGICK file.  */
+  imagemagick_image_p,
+  /* Handle to function used to load a IMAGEMAGICK file.  */
+  imagemagick_load,
+  /* Handle to function to free resources for IMAGEMAGICK.  */
+  imagemagick_clear_image,
+  /* An internal field to link to the next image type in a list of
+     image types, will be filled in when registering the format.  */
+  NULL
+};
+
+
+/* Return non-zero if OBJECT is a valid IMAGEMAGICK image specification.  Do
+   this by calling parse_image_spec and supplying the keywords that
+   identify the IMAGEMAGICK format.   */
+
+static int
+imagemagick_image_p (object)
+     Lisp_Object object;
+{
+  struct image_keyword fmt[IMAGEMAGICK_LAST];
+  bcopy (imagemagick_format, fmt, sizeof fmt);
+
+  if (!parse_image_spec (object, fmt, IMAGEMAGICK_LAST, Qimagemagick))
+    return 0;
+
+  /* Must specify either the :data or :file keyword.  */
+  return fmt[IMAGEMAGICK_FILE].count + fmt[IMAGEMAGICK_DATA].count == 1;
+}
+
+/* The GIF library also defines DrawRectangle, but its never used in Emacs.
+   Therefore rename the function so it doesnt collide with ImageMagick */
+#define DrawRectangle DrawRectangleGif
+#include <wand/MagickWand.h>
+
+//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"
+/* Load IMAGEMAGICK image IMG for use on frame F.  Value is non-zero if
+   successful. this function will go into the imagemagick_type structure, and
+   the prototype thus needs to be compatible with that structure.  */
+
+static int
+imagemagick_load (f, img)
+     struct frame *f;
+     struct image *img;
+{
+  int success_p = 0;
+  Lisp_Object file_name;
+
+  /* If IMG->spec specifies a file name, create a non-file spec from it.  */
+  file_name = image_spec_value (img->spec, QCfile, NULL);
+  if (STRINGP (file_name))
+    {
+      Lisp_Object file;
+      unsigned char *contents;
+      int size;
+      struct gcpro gcpro1;
+
+      file = x_find_image_file (file_name);
+      GCPRO1 (file);
+      if (!STRINGP (file))
+	{
+	  image_error ("Cannot find image file `%s'", file_name, Qnil);
+	  UNGCPRO;
+	  return 0;
+	}
+
+      /* Read the entire file into memory.  */
+      /* contents = slurp_file (SDATA (file), &size); */
+      /* if (contents == NULL) */
+      /*   { */
+      /*     image_error ("Error loading IMAGEMAGICK image `%s'", img->spec, Qnil); */
+      /*     UNGCPRO; */
+      /*     return 0; */
+      /*   } */
+      /* If the file was slurped into memory properly, parse it.  */
+      success_p = imagemagick_load_image (f, img, 0, 0, SDATA(file_name));
+      UNGCPRO;
+    }
+  /* Else its not a file, its a lisp object.  Load the image from a
+     lisp object rather than a file.  */
+  else
+    {
+      Lisp_Object data;
+
+      data = image_spec_value (img->spec, QCdata, NULL);
+      success_p = imagemagick_load_image (f, img, SDATA (data), SBYTES (data),NULL);
+    }
+
+  return success_p;
+}
+
+/* imagemagick_load_image is a helper function for imagemagick_load, which does the
+   actual loading given contents and size, apart from frame and image
+   structures, passed from imagemagick_load.
+
+   Uses librimagemagick to do most of the image processing.
+
+   Returns non-zero when successful.
+*/
+
+static int
+imagemagick_load_image (f, img, contents, size, filename)
+    /* Pointer to emacs frame structure.  */
+     struct frame *f;
+     /* Pointer to emacs image structure.  */
+     struct image *img;
+     /* String containing the IMAGEMAGICK  data to be parsed.  */
+     unsigned char *contents;
+     /* Size of data in bytes.  */
+     unsigned int size;
+     /* filename, either pass filename or contents/size */
+     unsigned char *filename;
+{
+  long unsigned int width;
+  long unsigned int height;
+
+  MagickBooleanType
+    status;
+
+  XImagePtr ximg;
+  Lisp_Object specified_bg;
+  XColor background;
+  int x;
+  int y;
+
+  MagickWand  *image_wand;
+  PixelIterator *iterator;
+  PixelWand  **pixels;
+  MagickPixelPacket  pixel;
+
+  
+  /* image_wand will contain the image  */
+  image_wand = NewMagickWand();  
+
+  /* Parse the contents argument and initialize image_wand.  */
+  if(filename!=NULL)
+    status=MagickReadImage(image_wand, filename);
+  else
+    status=MagickReadImageBlob(image_wand, contents, size);
+  image_error ("im read failed", Qnil, Qnil);
+  if (status == MagickFalse) goto imagemagick_error;
+
+  /* handle image index for image types who can contain more than one image.
+   interface :index is same as for GIF */
+  Lisp_Object image;
+  long ino;
+  image = image_spec_value (img->spec, QCindex, NULL);
+  ino = INTEGERP (image) ? XFASTINT (image) : 0;
+  
+  /* if (ino >= ) */
+  /*   { */
+  /*     image_error ("Invalid image number `%s' in image `%s'", */
+  /*       	   image, img->spec); */
+  /*     UNGCPRO; */
+  /*     return 0; */
+  /*   } */
+
+  if(ino==0)
+    MagickSetFirstIterator(image_wand);
+  else
+    MagickSetIteratorIndex(image_wand, ino);
+
+  /*
+    if width and/or height is set in the display spec
+    assume we want to scale to those */
+
+  int desired_width, desired_height;
+  Lisp_Object value;  
+  value = image_spec_value (img->spec, QCwidth, NULL);
+  desired_width = (INTEGERP (value)  ? XFASTINT (value) : -1);
+  value = image_spec_value (img->spec, QCheight, NULL);
+  desired_height = (INTEGERP (value) ? XFASTINT (value) : -1);
+  if(desired_width != -1 && desired_height != -1){
+    printf("MagickScaleImage %d %d\n",desired_width, desired_height);
+    status=MagickScaleImage(image_wand, desired_width, desired_height);
+    if (status == MagickFalse) {
+      image_error ("Imagemagick scale failed", Qnil, Qnil);
+      goto imagemagick_error;
+    }
+    
+  }
+
+  /* also support :geometry and :crop which are imagemagick specific descriptors */
+
+  Lisp_Object crop, geometry;
+  crop     = image_spec_value (img->spec, QCcrop, NULL);
+  geometry = image_spec_value (img->spec, QCgeometry, NULL);
+  if (STRINGP (crop) && STRINGP (geometry)){
+    printf("MagickTransformImage %s %s\n",SDATA(crop), SDATA(geometry));
+    image_wand = MagickTransformImage (image_wand, SDATA (crop), SDATA (geometry));
+    /*TODO differ between image_wand and transform_wand*/
+  }
+
+  /* furthermore :rotation. we need background color and angle for rotation */
+  /*
+    TODO background handling for rotation
+    specified_bg = image_spec_value (img->spec, QCbackground, NULL);
+  if (!STRINGP (specified_bg)
+  */
+    double rotation;
+    value = image_spec_value (img->spec, QCrotation, NULL);
+    if (FLOATP (value)){
+      PixelWand* background = NewPixelWand();
+      PixelSetColor (background, "#ffffff");/*TODO remove hardcode*/
+
+      rotation = extract_float (value);
+      printf ("MagickRotateImage %f\n",rotation);
+      
+      status=MagickRotateImage (image_wand, background,rotation);
+      DestroyPixelWand (background);
+      if (status == MagickFalse) {
+        image_error ("Imagemagick image rotate failed", Qnil, Qnil);
+        goto imagemagick_error;
+      }
+    }
+  
+  /* finaly we are done manipulating the image,
+     figure resulting width, height, and then transfer ownerwship to emacs
+   */
+  height=MagickGetImageHeight (image_wand);
+  width=MagickGetImageWidth (image_wand);
+  if (status == MagickFalse) {
+      image_error ("Imagemagick image get size failed", Qnil, Qnil);  
+      goto imagemagick_error;
+  }
+    
+  if (! check_image_size (f, width, height))
+    {
+      image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
+      goto imagemagick_error;
+    }
+  
+  /* We can now get a valid pixel buffer from the imagemagick file, if all
+     went ok.  */
+  
+
+  init_color_table ();
+  int imagemagick_rendermethod=(INTEGERP (Vimagemagick_render_type) ? XFASTINT (Vimagemagick_render_type) : 0);
+  if (imagemagick_rendermethod==0){
+    /* Try to create a x pixmap to hold the imagemagick pixmap.  */
+    if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg, &img->pixmap)){
+      image_error("Imagemagick X bitmap allocation failure",Qnil,Qnil);
+      goto imagemagick_error;
+    }
+    
+    /* copy imagegmagick image to x with primitive yet robust pixel
+       pusher loop.  This has been tested a lot with many different
+       images, it doesnt work too well with image archive formats though!
+
+       also seems slow.
+    */
+  
+    /* copy pixels from the imagemagick image structure to the x image map */
+    iterator = NewPixelIterator (image_wand);
+    if ((iterator == (PixelIterator *) NULL)) {
+          image_error ("Imagemagick pixel iterator creation failed", Qnil, Qnil);
+          goto imagemagick_error;
+    }
+
+    for (y=0; y < (long) MagickGetImageHeight(image_wand); y++)
+      {
+        pixels = PixelGetNextIteratorRow (iterator, &width);
+        if ((pixels == (PixelWand **) NULL))
+          break;
+        for (x=0; x < (long) width; x++)
+          {
+            PixelGetMagickColor (pixels[x], &pixel);
+            XPutPixel (ximg, x, y, lookup_rgb_color (f, pixel.red, pixel.green, pixel.blue));      
+          }
+      }
+    DestroyPixelIterator (iterator);
+  }
+
+  if (imagemagick_rendermethod==1){
+    //try if magicexportimage is any faster than pixelpushing
+    /* printf("ximg: bitmap_unit:%d format:%d byte_order:%d depth:%d bits_per_pixel:%d\n", */
+    /*        ximg->bitmap_unit,ximg->format,ximg->byte_order,ximg->depth,ximg->bits_per_pixel); */
+    int imagedepth=24;//MagickGetImageDepth(image_wand);
+    char* exportdepth= imagedepth <= 8 ? "I" : "BGRP";//"RGBP";
+    /* Try to create a x pixmap to hold the imagemagick pixmap.  */
+    printf("imagedepth:%d exportdepth:%s\n", imagedepth, exportdepth);
+    if (!x_create_x_image_and_pixmap (f, width, height, imagedepth, &ximg, &img->pixmap)){
+      image_error("Imagemagick X bitmap allocation failure",Qnil,Qnil);
+      goto imagemagick_error;
+    }
+
+    
+    //oddly, the below code doesnt seem to work:
+    int pixelwidth; 
+    /* switch(ximg->bitmap_unit){ */
+    /* case 8: */
+    /*   pixelwidth=CharPixel; */
+    /*   break; */
+    /* case   16: */
+    /*   pixelwidth=ShortPixel; */
+    /*   break; */
+    /* case   32: */
+    /*   pixelwidth=LongPixel; */
+    /*   break; */
+    /* } */
+    /*
+      here im just guessing the format of the bitmap.
+      happens to work fine for:
+      - bw djvu images
+      on rgb display.
+      seems about 3 times as fast as pixel pushing(not carefully measured)
+      with color djvu, the bitplanes are mapped to wrong color.
+
+    */
+    pixelwidth=CharPixel;//??? TODO figure out
+    MagickExportImagePixels(image_wand,
+                            0,0,
+                            width,height,
+                            exportdepth,
+                            pixelwidth, 
+                            //&(img->pixmap));
+                            ximg->data);
+  }
+  
+#ifdef COLOR_TABLE_SUPPORT
+  /* Remember colors allocated for this image.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
+#endif /* COLOR_TABLE_SUPPORT */
+
+
+  img->width  = width;
+  img->height = height;
+
+  /* Maybe fill in the background field while we have ximg handy.
+     Casting avoids a GCC warning.  */
+  //  IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
+
+  /* Put the image into the pixmap, then free the X image and its
+     buffer.  */
+  x_put_x_image (f, ximg, img->pixmap, width, height);
+
+  x_destroy_x_image (ximg);
+
+
+  /*JAVE TODO more cleanup*/
+  DestroyMagickWand (image_wand);
+
+  return 1;
+
+ imagemagick_error:
+  //TODO more cleanup
+  image_error ("Error parsing IMAGEMAGICK image `%s'", img->spec, Qnil);
+  printf("Imagemagick error, see *Messages*\n");
+  return 0;
+}
+
+DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0,0,0,
+       doc: /* Return image file types supported by ImageMagick.
+             Since ImageMagic recognizes a lot of file-types that clash with Emacs,
+            such as .c, we want to be able to alter the list at the lisp level.*/)
+  ()
+{
+  Lisp_Object typelist = Qnil;
+  unsigned long numf;
+  ExceptionInfo ex;
+  char** imtypes = GetMagickList ("*", &numf, &ex);
+  int i;
+  Lisp_Object Qimagemagicktype;
+  for (i = 0; i < numf; i++) {
+    Qimagemagicktype = intern (*( imtypes + i));
+    typelist = Fcons (Qimagemagicktype, typelist);
+  }
+  return typelist;
+}
+  
+#endif	/* defined (HAVE_IMAGEMAGICK) */
+
+
 \f
 /***********************************************************************
 				 SVG
@@ -8333,6 +8778,14 @@ of `image-library-alist', which see).  */)
     return CHECK_LIB_AVAILABLE (&svg_type, init_svg_functions, libraries);
 #endif
 
+#if defined (HAVE_IMAGEMAGICK)
+  if (EQ (type, Qimagemagick)){
+    /*   MagickWandGenesis() initalizes the imagemagick library */
+    MagickWandGenesis(); 
+    return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions, libraries);
+  }
+#endif
+
 #ifdef HAVE_GHOSTSCRIPT
   if (EQ (type, Qpostscript))
     return CHECK_LIB_AVAILABLE (&gs_type, init_gs_functions, libraries);
@@ -8343,6 +8796,7 @@ of `image-library-alist', which see).  */)
   return Qnil;
 }
 
+
 void
 syms_of_image ()
 {
@@ -8418,7 +8872,14 @@ non-numeric, there is no explicit limit on the size of images.  */);
   staticpro (&QCheuristic_mask);
   QCindex = intern_c_string (":index");
   staticpro (&QCindex);
+  QCgeometry = intern (":geometry");
+  staticpro (&QCgeometry);
+  QCcrop = intern (":crop");
+  staticpro (&QCcrop);
+  QCrotation = intern (":rotation");
+  staticpro (&QCrotation);
   QCmatrix = intern_c_string (":matrix");
+
   staticpro (&QCmatrix);
   QCcolor_adjustment = intern_c_string (":color-adjustment");
   staticpro (&QCcolor_adjustment);
@@ -8478,6 +8939,12 @@ non-numeric, there is no explicit limit on the size of images.  */);
   ADD_IMAGE_TYPE (Qpng);
 #endif
 
+#if defined (HAVE_IMAGEMAGICK)
+  Qimagemagick = intern ("imagemagick");
+  staticpro (&Qimagemagick);
+  ADD_IMAGE_TYPE (Qimagemagick);
+#endif
+  
 #if defined (HAVE_RSVG)
   Qsvg = intern_c_string ("svg");
   staticpro (&Qsvg);
@@ -8494,6 +8961,9 @@ non-numeric, there is no explicit limit on the size of images.  */);
 #endif /* HAVE_RSVG  */
 
   defsubr (&Sinit_image_library);
+#ifdef HAVE_IMAGEMAGICK  
+  defsubr (&Simagemagick_types);
+#endif  
   defsubr (&Sclear_image_cache);
   defsubr (&Simage_refresh);
   defsubr (&Simage_size);
@@ -8521,12 +8991,20 @@ When an image has not been displayed this many seconds, remove it
 from the image cache.  Value must be an integer or nil with nil
 meaning don't clear the cache.  */);
   Vimage_cache_eviction_delay = make_number (30 * 60);
+
+#ifdef HAVE_IMAGEMAGICK  
+  DEFVAR_LISP ("imagemagick-render-type", &Vimagemagick_render_type,
+    doc: /*   */);
+#endif    
 }
 
 void
 init_image ()
 {
+
 }
 
+
+
 /* arch-tag: 123c2a5e-14a8-4c53-ab95-af47d7db49b9
    (do not change this comment) */

[-- Attachment #3: Type: text/plain, Size: 20 bytes --]



-- 
Joakim Verona

Re: imagemmagick patch 5

[Juri Linkov]

> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
> load images. This support can be used in parallell with the existing
> image loading support. There is support for asking your imagemagick
> installation which image types it supports, and registering them in
> Emacs selectively.

Great!

> I use this patch a lot for viewing scanned djvu packages. I also have a
> emacs scanner frontend called "emsane" I'm using together with this
> patch.

I wonder how do you plan to handle multi-page djvu?  Is it something
that doc-view has to do when ImageMagick support will be incorporated
into the core?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[joakim]

Juri Linkov <juri@jurta.org> writes:

>> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
>> load images. This support can be used in parallell with the existing
>> image loading support. There is support for asking your imagemagick
>> installation which image types it supports, and registering them in
>> Emacs selectively.
>
> Great!
>
>> I use this patch a lot for viewing scanned djvu packages. I also have a
>> emacs scanner frontend called "emsane" I'm using together with this
>> patch.
>
> I wonder how do you plan to handle multi-page djvu?  Is it something
> that doc-view has to do when ImageMagick support will be incorporated
> into the core?

I've added some interfaces in the display spec for various imagemagick
operations like scaling and rotation of the image. I've also added an
index interface so a particular subimage inside a multipage djvu can be
viewed. This is the same interface already present for the gif support
in emacs, since gifs also can be image bundles. Imagemagick support a
number of image bundle formats such as pdf, djvu, tif, etc.

My experience is, though, that imagemagick handles image bundles
oddly. It seems to render the entire bundle in ram before rendering the
single indexed page. Most image bundle formats are not designed for that
to be necessary, so I'm not sure why imagemagick does that.

Until this area of imagemagick improves, I would say its doc-views
responsibility to handle image bundles, in the same way it already
does. If this problem is in someway solved, doc-view can take advantage
of the imagemagick library. I'm planning at having a go at this for
the case of directories with image files.

-- 
Joakim Verona

Re: imagemmagick patch 5

[Stefan Monnier]

> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
> load images. This support can be used in parallell with the existing
> image loading support. There is support for asking your imagemagick
> installation which image types it supports, and registering them in
> Emacs selectively.

Thanks.  Looks pretty good.  Feel free to install it in the `pending'
branch, but please see the comments below first, and don't forget to add
a good NEWS entry when you install this change.

> +HAVE_IMAGEMAGICK=no
> +if test "${HAVE_X11}" = "yes" ; then

Do I understand it right that this X11-only restriction could be lifted
at some point in the future?
 
> diff --git a/src/dbusbind.c b/src/dbusbind.c
> index 7c0be49..7a47730 100644
> --- a/src/dbusbind.c
> +++ b/src/dbusbind.c
> @@ -773,6 +773,7 @@ xd_add_watch (watch, data)
>        if (fd == -1)
>  	return FALSE;
>  
> +
>        /* Add the file descriptor to input_wait_mask.  */
>        add_keyboard_wait_descriptor (fd);
>      }

Please drop this gratuitous change.

> +/***********************************************************************
> +				 imagemagick
> + ***********************************************************************/
> +#if defined (HAVE_IMAGEMAGICK)
> +Lisp_Object Vimagemagick_render_type;
> +/* Function prototypes.  */
> +
> +static int imagemagick_image_p P_ ((Lisp_Object object));
> +static int imagemagick_load P_ ((struct frame *f, struct image *img));
> +
> +static int imagemagick_load_image P_ ((struct frame *, struct image *,
> +                                       unsigned char *, unsigned int, unsigned char *));

Please don't use the P_ macro in new code: just use prototypes.

> +//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"

This should be removed as well.

> +    //try if magicexportimage is any faster than pixelpushing
[...]
> +    //oddly, the below code doesnt seem to work:

We currently only use /*...*/ comments, so I'd rather we don't introduce
// comments for now.


        Stefan

Re: imagemmagick patch 5

[joakim]

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

>> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
>> load images. This support can be used in parallell with the existing
>> image loading support. There is support for asking your imagemagick
>> installation which image types it supports, and registering them in
>> Emacs selectively.
>
> Thanks.  Looks pretty good.  Feel free to install it in the `pending'
> branch, but please see the comments below first, and don't forget to add
> a good NEWS entry when you install this change.

Ok. I will need to read up on bzr before committing. 

Could you possibly also comment on the interface I chose for scaling and
rotation? If they are ok, I will document them in the NEWS entry. I
assume the "index" spec is ok since that is also used for gifs already.

Also please comment if the interface to select which image types
imagemagick gets to handle is ok.

>
>> +HAVE_IMAGEMAGICK=no
>> +if test "${HAVE_X11}" = "yes" ; then
>
> Do I understand it right that this X11-only restriction could be lifted
> at some point in the future?

Yes. Theres nothing completely X specific in the patch I think, just the
same type of bitmap manipulations that goes on for the other image
libraries.

>> diff --git a/src/dbusbind.c b/src/dbusbind.c
>> index 7c0be49..7a47730 100644
>> --- a/src/dbusbind.c
>> +++ b/src/dbusbind.c
>> @@ -773,6 +773,7 @@ xd_add_watch (watch, data)
>>        if (fd == -1)
>>  	return FALSE;
>>  
>> +
>>        /* Add the file descriptor to input_wait_mask.  */
>>        add_keyboard_wait_descriptor (fd);
>>      }
>
> Please drop this gratuitous change.

Hmm, I have no idea how this crept in. Odd.


>> +/***********************************************************************
>> +				 imagemagick
>> + ***********************************************************************/
>> +#if defined (HAVE_IMAGEMAGICK)
>> +Lisp_Object Vimagemagick_render_type;
>> +/* Function prototypes.  */
>> +
>> +static int imagemagick_image_p P_ ((Lisp_Object object));
>> +static int imagemagick_load P_ ((struct frame *f, struct image *img));
>> +
>> +static int imagemagick_load_image P_ ((struct frame *, struct image *,
>> +                                       unsigned char *, unsigned int, unsigned char *));
>
> Please don't use the P_ macro in new code: just use prototypes.

Ok. Like this?

static int imagemagick_image_p (Lisp_Object object);
static int imagemagick_load (struct frame *f, struct image *img);

static int imagemagick_load_image (struct frame *, struct image *,
                                       unsigned char *, unsigned int, unsigned char *);


>> +//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"
>
> This should be removed as well.

Ok.

>> +    //try if magicexportimage is any faster than pixelpushing
> [...]
>> +    //oddly, the below code doesnt seem to work:
>
> We currently only use /*...*/ comments, so I'd rather we don't introduce
> // comments for now.
>

Sorry, I keep forgetting this, it seems to be in my muscle memory. 

>         Stefan
>
-- 
Joakim Verona

Re: imagemmagick patch 5

[Stefan Monnier]

> Could you possibly also comment on the interface I chose for scaling and
> rotation?  If they are ok, I will document them in the NEWS entry.

I don't have much experience with such features, so I can't really
usefully comment on them (in mpc.el I do scaling (externally, obviously)
for album-covers, and indeed, I do it by specifying the target size, so
it looks acceptable).

But even if it's not 100% resolved, it can be installed, since it's the
branch for Emacs-24, so there'll be time to adjust the API.

> I assume the "index" spec is ok since that is also used for
> gifs already.

> Also please comment if the interface to select which image types
> imagemagick gets to handle is ok.

>>> +HAVE_IMAGEMAGICK=no
>>> +if test "${HAVE_X11}" = "yes" ; then
>> Do I understand it right that this X11-only restriction could be lifted
>> at some point in the future?
> Yes.  There's nothing completely X specific in the patch I think, just
> the same type of bitmap manipulations that goes on for the other
> image libraries.

So is the test in configure.in necessary?

>> Please don't use the P_ macro in new code: just use prototypes.
> Ok.  Like this?
> static int imagemagick_image_p (Lisp_Object object);

Yes.  Similarly the function definition shouldn't use K&R style any more
(we didn't change all the existing code, but we try and do that for new
code).

>> We currently only use /*...*/ comments, so I'd rather we don't introduce
>> // comments for now.
> Sorry, I keep forgetting this, it seems to be in my muscle memory. 

It's no big problem.  Now that we accept and use prototypes, there
probably aren't any compiler that can compile Emacs but does not accept
// comments.


        Stefan


PS: A few more things:

+  /* furthermore :rotation. we need background color and angle for rotation */

Please capitalize your comments, and use double-spaces at the end of
sentences (including at the end of a comment, where we also want
a full-stop):

  /* Furthermore :rotation.  We need background color and angle for rotation.  */

+    if (FLOATP (value)){

Please put all "{" on their own line.

+      PixelWand* background = NewPixelWand();

Add a space before every open paren.

+      PixelSetColor (background, "#ffffff");/*TODO remove hardcode*/

Please use something like

       PixelSetColor (background, "#ffffff"); /* TODO remove hardcode.  */

I.e. notice the "full stop plus 2 spaces".

+      status=MagickRotateImage (image_wand, background,rotation);

Please add spaces around operators like "=".
Also, please drop the "add empty lines" part of the hunk below as well.

@@ -8521,12 +8991,20 @@ When an image has not been displayed this many seconds, remove it
 from the image cache.  Value must be an integer or nil with nil
 meaning don't clear the cache.  */);
   Vimage_cache_eviction_delay = make_number (30 * 60);
+
+#ifdef HAVE_IMAGEMAGICK  
+  DEFVAR_LISP ("imagemagick-render-type", &Vimagemagick_render_type,
+    doc: /*   */);
+#endif    
 }
 
 void
 init_image ()
 {
+
 }
 
+
+
 /* arch-tag: 123c2a5e-14a8-4c53-ab95-af47d7db49b9
    (do not change this comment) */

Re: imagemmagick patch 5

[Jan Djärv]

Stefan Monnier skrev 2010-03-05 23.30:

>
>>> Please don't use the P_ macro in new code: just use prototypes.
>> Ok.  Like this?
>> static int imagemagick_image_p (Lisp_Object object);
>
> Yes.  Similarly the function definition shouldn't use K&R style any more
> (we didn't change all the existing code, but we try and do that for new
> code).
>

Is this documented somewhere?  It is the first time I've heard about it.  The 
GNU coding standards doesn't seem to agree, at least the version updated in 
February 17, 2010.

	Jan D.

Re: imagemmagick patch 5

[Stephen J. Turnbull]

Stefan Monnier writes:

 > It's no big problem.  Now that we accept and use prototypes, there
 > probably aren't any compiler that can compile Emacs but does not accept
 > // comments.

IIRC, C89 compilers accept prototypes but not // comments.  In any
case, XEmacs has required prototype support for a decade but some
compilers that can compile XEmacs blow up on // comments.  I'm not
even sure when/if GCC started accepting them by default.

I think you need to require C99 (or possibly GNU extensions) to get //
comments.

Re: imagemmagick patch 5

[Juri Linkov]

> I've also added an index interface so a particular subimage inside
> a multipage djvu can be viewed. This is the same interface already
> present for the gif support in emacs, since gifs also can be image
> bundles.

`:index' won't be useful without the ability to get the total
number of images in the file.  `gif' provides this information
in the property `count' (from gif->ImageCount) in the list returned
by `image-extension-data' (img->data.lisp_val).

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[Stefan Monnier]

> number of images in the file.  `gif' provides this information
> in the property `count' (from gif->ImageCount) in the list returned
> by `image-extension-data' (img->data.lisp_val).

BTW: what is meant by "extension" in that function?


        Stefan

Re: imagemmagick patch 5

[Juri Linkov]

>> number of images in the file.  `gif' provides this information
>> in the property `count' (from gif->ImageCount) in the list returned
>> by `image-extension-data' (img->data.lisp_val).
>
> BTW: what is meant by "extension" in that function?

According to Kim's explanations in
http://lists.gnu.org/archive/html/emacs-devel/2006-04/msg00936.html
it returns a raw representation of the extension data of a given (sub)image.

Unfortunately, adding animage.el was postponed due to pre-22.1 feature
freeze.  I think after the current feature freeze it should be added
to image.el as Kim suggested.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[Stefan Monnier]

> it returns a raw representation of the extension data of a given (sub)image.

Again, what is meant by "extension" here?

> Unfortunately, adding animage.el was postponed due to pre-22.1 feature
> freeze.  I think after the current feature freeze it should be added
> to image.el as Kim suggested.

Then please install it in the `pending' branch.


        Stefan

Re: imagemmagick patch 5

[Juri Linkov]

>> it returns a raw representation of the extension data of a given (sub)image.
>
> Again, what is meant by "extension" here?

It seems the term "extension" is GIF-specific.  The GIF specification says
that extension blocks "extend" the 87a definition to allow extensions like
the Graphic Control Extension that specifies the animation delay time, etc.

The parameter `count' is used in animation, but unrelated to "extension".
Perhaps a more common term for `count' would be "metadata".

>> Unfortunately, adding animage.el was postponed due to pre-22.1 feature
>> freeze.  I think after the current feature freeze it should be added
>> to image.el as Kim suggested.
>
> Then please install it in the `pending' branch.

Done.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[Stefan Monnier]

>>> it returns a raw representation of the extension data of a given (sub)image.
>> Again, what is meant by "extension" here?
> It seems the term "extension" is GIF-specific.  The GIF specification says
> that extension blocks "extend" the 87a definition to allow extensions like
> the Graphic Control Extension that specifies the animation delay time, etc.

> The parameter `count' is used in animation, but unrelated to "extension".
> Perhaps a more common term for `count' would be "metadata".

Then we should rename image-extension-data to image-metadata, I think.


        Stefan

Re: imagemmagick patch 5

[Kim F. Storm]

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

> Then we should rename image-extension-data to image-metadata, I think.

That would be a good change!

Juri - Thanks for installing animage.el 

-- 
Kim F. Storm  http://www.cua.dk

Image metadata (was: imagemmagick patch 5)

[Juri Linkov]

>>>> it returns a raw representation of the extension data of a given (sub)image.
>>> Again, what is meant by "extension" here?
>> It seems the term "extension" is GIF-specific.  The GIF specification says
>> that extension blocks "extend" the 87a definition to allow extensions like
>> the Graphic Control Extension that specifies the animation delay time, etc.
>
>> The parameter `count' is used in animation, but unrelated to "extension".
>> Perhaps a more common term for `count' would be "metadata".
>
> Then we should rename image-extension-data to image-metadata, I think.

The problem is that one part of the current `image-extension-data'
is metadata, and another part is extension data.  For instance, in

  (count 44
   255 "NETSCAPE2.0"
     0 "^A^@^@"
   249 "^D^M^@\377"
  )

`count 44' is metadata, and the rest (with non-descriptive numeric keys)
is extension data.

Maybe the list returned by the new function `image-metadata' should be like:

  (count 44
   extension-data (255 "NETSCAPE2.0"
                     0 "^A^@^@"
                   249 "^D^M^@\377")
  )

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image metadata

[Stefan Monnier]

> The problem is that one part of the current `image-extension-data'
> is metadata, and another part is extension data.  For instance, in

>   (count 44
>    255 "NETSCAPE2.0"
>      0 "^A^@^@"
>    249 "^D^M^@\377"
>   )

> `count 44' is metadata, and the rest (with non-descriptive numeric keys)
> is extension data.

Not knowing what is "extension data", I don't know if I agree.


        Stefan

Re: Image metadata

[Juri Linkov]

>> The problem is that one part of the current `image-extension-data'
>> is metadata, and another part is extension data.  For instance, in
>
>>   (count 44
>>    255 "NETSCAPE2.0"
>>      0 "^A^@^@"
>>    249 "^D^M^@\377"
>>   )
>
>> `count 44' is metadata, and the rest (with non-descriptive numeric keys)
>> is extension data.
>
> Not knowing what is "extension data", I don't know if I agree.

From the API's point of view, "extension data" is just raw data fetched
from the `ExtensionBlocks' slot of the GIF metadata.  So really it's not
important what it is as long as slot names basically coincide in the GIF
structure and in its Lisp image metadata.

I believe this patch cleans up the naming mess:

=== modified file 'lisp/image.el'
--- lisp/image.el	2010-03-11 20:01:33 +0000
+++ lisp/image.el	2010-03-30 22:28:18 +0000
@@ -681,8 +681,9 @@ (defun image-animated-p (image)
 shall be displayed."
   (cond
    ((eq (plist-get (cdr image) :type) 'gif)
-    (let* ((extdata (image-extension-data image))
-	   (images (plist-get extdata 'count))
+    (let* ((metadata (image-metadata image))
+	   (images (plist-get metadata 'count))
+	   (extdata (plist-get metadata 'extension-data))
 	   (anim (plist-get extdata #xF9)))
       (and (integerp images) (> images 1)
 	   (stringp anim) (>= (length anim) 4)

=== modified file 'src/image.c'
--- src/image.c	2010-01-24 23:03:13 +0000
+++ src/image.c	2010-03-30 22:31:57 +0000
@@ -604,7 +604,7 @@ (at your option) any later version.
 extern Lisp_Object QCwidth, QCheight, QCforeground, QCbackground, QCfile;
 extern Lisp_Object QCdata, QCtype;
 extern Lisp_Object Qcenter;
-Lisp_Object QCascent, QCmargin, QCrelief, Qcount;
+Lisp_Object QCascent, QCmargin, QCrelief, Qcount, Qextension_data;
 Lisp_Object QCconversion, QCcolor_symbols, QCheuristic_mask;
 Lisp_Object QCindex, QCmatrix, QCcolor_adjustment, QCmask;
 
@@ -1011,8 +1011,8 @@ (at your option) any later version.
   return mask;
 }
 
-DEFUN ("image-extension-data", Fimage_extension_data, Simage_extension_data, 1, 2, 0,
-       doc: /* Return extension data for image SPEC.
+DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
+       doc: /* Return metadata for image SPEC.
 FRAME is the frame on which the image will be displayed.  FRAME nil
 or omitted means use the selected frame.  */)
      (spec, frame)
@@ -7169,7 +7169,7 @@ (at your option) any later version.
      struct frame *f;
      struct image *img;
 {
-  /* IMG->data.ptr_val may contain extension data.  */
+  /* IMG->data.ptr_val may contain metadata with extension data.  */
   img->data.lisp_val = Qnil;
   x_clear_image (f, img);
 }
@@ -7488,8 +7488,8 @@ (at your option) any later version.
 	  }
     }
 
-  /* Save GIF image extension data for `image-extension-data'.
-     Format is (count IMAGES FUNCTION "BYTES" ...).  */
+  /* Save GIF image extension data for `image-metadata'.
+     Format is (count IMAGES extension-data (FUNCTION "BYTES" ...)).  */
   img->data.lisp_val = Qnil;
   if (gif->SavedImages[ino].ExtensionBlockCount > 0)
     {
@@ -7499,7 +7499,9 @@ (at your option) any later version.
 	img->data.lisp_val = Fcons (make_unibyte_string (ext->Bytes, ext->ByteCount),
 				    Fcons (make_number (ext->Function),
 					   img->data.lisp_val));
-      img->data.lisp_val = Fnreverse (img->data.lisp_val);
+      img->data.lisp_val = Fcons (Qextension_data,
+				  Fcons (Fnreverse (img->data.lisp_val),
+					 Qnil));
     }
   if (gif->ImageCount > 1)
     img->data.lisp_val = Fcons (Qcount,
@@ -8403,6 +8405,8 @@ (at your option) any later version.
 
   Qcount = intern_c_string ("count");
   staticpro (&Qcount);
+  Qextension_data = intern_c_string ("extension-data");
+  staticpro (&Qextension_data);
 
   QCascent = intern_c_string (":ascent");
   staticpro (&QCascent);
@@ -8498,7 +8502,7 @@ (at your option) any later version.
   defsubr (&Simage_refresh);
   defsubr (&Simage_size);
   defsubr (&Simage_mask_p);
-  defsubr (&Simage_extension_data);
+  defsubr (&Simage_metadata);
 
 #if GLYPH_DEBUG
   defsubr (&Simagep);

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Image metadata

[Stefan Monnier]

>>> The problem is that one part of the current `image-extension-data'
>>> is metadata, and another part is extension data.  For instance, in
>> 
>>> (count 44
>>> 255 "NETSCAPE2.0"
>>> 0 "^A^@^@"
>>> 249 "^D^M^@\377"
>>> )
>> 
>>> `count 44' is metadata, and the rest (with non-descriptive numeric keys)
>>> is extension data.
>> 
>> Not knowing what is "extension data", I don't know if I agree.

> From the API's point of view, "extension data" is just raw data fetched
> from the `ExtensionBlocks' slot of the GIF metadata.  So really it's not
> important what it is as long as slot names basically coincide in the GIF
> structure and in its Lisp image metadata.

That still doesn't tell me what it is, other than that it's Gif-specific
and that it's a kind of metadata, so the name "extension-data" shouldn't
appear in the generic code.
I guess your patch is good, thanks, feel free to install it.


        Stefan

Re: Image metadata

[joakim]

Juri Linkov <juri@jurta.org> writes:

>>> The problem is that one part of the current `image-extension-data'
>>> is metadata, and another part is extension data.  For instance, in
>>
>>>   (count 44
>>>    255 "NETSCAPE2.0"
>>>      0 "^A^@^@"
>>>    249 "^D^M^@\377"
>>>   )
>>
>>> `count 44' is metadata, and the rest (with non-descriptive numeric keys)
>>> is extension data.
>>
>> Not knowing what is "extension data", I don't know if I agree.
>
>>From the API's point of view, "extension data" is just raw data fetched
> from the `ExtensionBlocks' slot of the GIF metadata.  So really it's not
> important what it is as long as slot names basically coincide in the GIF
> structure and in its Lisp image metadata.
>
> I believe this patch cleans up the naming mess:

This does indeed seem clearer. Will this be commited?

If so I would try to use this api in the imagemagick patch.
For instance, it seems I can use MagickGetNumberImages() to 
get the 'count property for multifile image archives.
>
> === modified file 'lisp/image.el'
> --- lisp/image.el	2010-03-11 20:01:33 +0000
> +++ lisp/image.el	2010-03-30 22:28:18 +0000
> @@ -681,8 +681,9 @@ (defun image-animated-p (image)
>  shall be displayed."
>    (cond
>     ((eq (plist-get (cdr image) :type) 'gif)
> -    (let* ((extdata (image-extension-data image))
> -	   (images (plist-get extdata 'count))
> +    (let* ((metadata (image-metadata image))
> +	   (images (plist-get metadata 'count))
> +	   (extdata (plist-get metadata 'extension-data))
>  	   (anim (plist-get extdata #xF9)))
>        (and (integerp images) (> images 1)
>  	   (stringp anim) (>= (length anim) 4)
>
> === modified file 'src/image.c'
> --- src/image.c	2010-01-24 23:03:13 +0000
> +++ src/image.c	2010-03-30 22:31:57 +0000
> @@ -604,7 +604,7 @@ (at your option) any later version.
>  extern Lisp_Object QCwidth, QCheight, QCforeground, QCbackground, QCfile;
>  extern Lisp_Object QCdata, QCtype;
>  extern Lisp_Object Qcenter;
> -Lisp_Object QCascent, QCmargin, QCrelief, Qcount;
> +Lisp_Object QCascent, QCmargin, QCrelief, Qcount, Qextension_data;
>  Lisp_Object QCconversion, QCcolor_symbols, QCheuristic_mask;
>  Lisp_Object QCindex, QCmatrix, QCcolor_adjustment, QCmask;
>  
> @@ -1011,8 +1011,8 @@ (at your option) any later version.
>    return mask;
>  }
>  
> -DEFUN ("image-extension-data", Fimage_extension_data, Simage_extension_data, 1, 2, 0,
> -       doc: /* Return extension data for image SPEC.
> +DEFUN ("image-metadata", Fimage_metadata, Simage_metadata, 1, 2, 0,
> +       doc: /* Return metadata for image SPEC.
>  FRAME is the frame on which the image will be displayed.  FRAME nil
>  or omitted means use the selected frame.  */)
>       (spec, frame)
> @@ -7169,7 +7169,7 @@ (at your option) any later version.
>       struct frame *f;
>       struct image *img;
>  {
> -  /* IMG->data.ptr_val may contain extension data.  */
> +  /* IMG->data.ptr_val may contain metadata with extension data.  */
>    img->data.lisp_val = Qnil;
>    x_clear_image (f, img);
>  }
> @@ -7488,8 +7488,8 @@ (at your option) any later version.
>  	  }
>      }
>  
> -  /* Save GIF image extension data for `image-extension-data'.
> -     Format is (count IMAGES FUNCTION "BYTES" ...).  */
> +  /* Save GIF image extension data for `image-metadata'.
> +     Format is (count IMAGES extension-data (FUNCTION "BYTES" ...)).  */
>    img->data.lisp_val = Qnil;
>    if (gif->SavedImages[ino].ExtensionBlockCount > 0)
>      {
> @@ -7499,7 +7499,9 @@ (at your option) any later version.
>  	img->data.lisp_val = Fcons (make_unibyte_string (ext->Bytes, ext->ByteCount),
>  				    Fcons (make_number (ext->Function),
>  					   img->data.lisp_val));
> -      img->data.lisp_val = Fnreverse (img->data.lisp_val);
> +      img->data.lisp_val = Fcons (Qextension_data,
> +				  Fcons (Fnreverse (img->data.lisp_val),
> +					 Qnil));
>      }
>    if (gif->ImageCount > 1)
>      img->data.lisp_val = Fcons (Qcount,
> @@ -8403,6 +8405,8 @@ (at your option) any later version.
>  
>    Qcount = intern_c_string ("count");
>    staticpro (&Qcount);
> +  Qextension_data = intern_c_string ("extension-data");
> +  staticpro (&Qextension_data);
>  
>    QCascent = intern_c_string (":ascent");
>    staticpro (&QCascent);
> @@ -8498,7 +8502,7 @@ (at your option) any later version.
>    defsubr (&Simage_refresh);
>    defsubr (&Simage_size);
>    defsubr (&Simage_mask_p);
> -  defsubr (&Simage_extension_data);
> +  defsubr (&Simage_metadata);
>  
>  #if GLYPH_DEBUG
>    defsubr (&Simagep);
-- 
Joakim Verona

Re: Image metadata

[Juri Linkov]

> This does indeed seem clearer. Will this be commited?
>
> If so I would try to use this api in the imagemagick patch.
> For instance, it seems I can use MagickGetNumberImages() to
> get the 'count property for multifile image archives.

This is committed now, so you can continue with your
imagemagick patch.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[joakim]

joakim@verona.se writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
>>> load images. This support can be used in parallell with the existing
>>> image loading support. There is support for asking your imagemagick
>>> installation which image types it supports, and registering them in
>>> Emacs selectively.
>>
>> Thanks.  Looks pretty good.  Feel free to install it in the `pending'
>> branch, but please see the comments below first, and don't forget to add
>> a good NEWS entry when you install this change.
>
> Ok. I will need to read up on bzr before committing. 

I've done a local bzr branch now, called "imagemagick". I would now like
to publish this branch.

http://www.emacswiki.org/emacs/BzrForEmacsDevs
mentions ways of publishing a branch to launchpad etc, but I cant find
how to publish it on savannah. Could some kind soul enlighten me?




> Could you possibly also comment on the interface I chose for scaling and
> rotation? If they are ok, I will document them in the NEWS entry. I
> assume the "index" spec is ok since that is also used for gifs already.
>
> Also please comment if the interface to select which image types
> imagemagick gets to handle is ok.
>
>>
>>> +HAVE_IMAGEMAGICK=no
>>> +if test "${HAVE_X11}" = "yes" ; then
>>
>> Do I understand it right that this X11-only restriction could be lifted
>> at some point in the future?
>
> Yes. Theres nothing completely X specific in the patch I think, just the
> same type of bitmap manipulations that goes on for the other image
> libraries.
>
>>> diff --git a/src/dbusbind.c b/src/dbusbind.c
>>> index 7c0be49..7a47730 100644
>>> --- a/src/dbusbind.c
>>> +++ b/src/dbusbind.c
>>> @@ -773,6 +773,7 @@ xd_add_watch (watch, data)
>>>        if (fd == -1)
>>>  	return FALSE;
>>>  
>>> +
>>>        /* Add the file descriptor to input_wait_mask.  */
>>>        add_keyboard_wait_descriptor (fd);
>>>      }
>>
>> Please drop this gratuitous change.
>
> Hmm, I have no idea how this crept in. Odd.
>
>
>>> +/***********************************************************************
>>> +				 imagemagick
>>> + ***********************************************************************/
>>> +#if defined (HAVE_IMAGEMAGICK)
>>> +Lisp_Object Vimagemagick_render_type;
>>> +/* Function prototypes.  */
>>> +
>>> +static int imagemagick_image_p P_ ((Lisp_Object object));
>>> +static int imagemagick_load P_ ((struct frame *f, struct image *img));
>>> +
>>> +static int imagemagick_load_image P_ ((struct frame *, struct image *,
>>> +                                       unsigned char *, unsigned int, unsigned char *));
>>
>> Please don't use the P_ macro in new code: just use prototypes.
>
> Ok. Like this?
>
> static int imagemagick_image_p (Lisp_Object object);
> static int imagemagick_load (struct frame *f, struct image *img);
>
> static int imagemagick_load_image (struct frame *, struct image *,
>                                        unsigned char *, unsigned int, unsigned char *);
>
>
>>> +//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"
>>
>> This should be removed as well.
>
> Ok.
>
>>> +    //try if magicexportimage is any faster than pixelpushing
>> [...]
>>> +    //oddly, the below code doesnt seem to work:
>>
>> We currently only use /*...*/ comments, so I'd rather we don't introduce
>> // comments for now.
>>
>
> Sorry, I keep forgetting this, it seems to be in my muscle memory. 
>
>>         Stefan
>>
-- 
Joakim Verona

Re: imagemmagick patch 5

[joakim]

joakim@verona.se writes:

> joakim@verona.se writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
>>>> load images. This support can be used in parallell with the existing
>>>> image loading support. There is support for asking your imagemagick
>>>> installation which image types it supports, and registering them in
>>>> Emacs selectively.
>>>
>>> Thanks.  Looks pretty good.  Feel free to install it in the `pending'
>>> branch, but please see the comments below first, and don't forget to add
>>> a good NEWS entry when you install this change.
>>
>> Ok. I will need to read up on bzr before committing. 
>
> I've done a local bzr branch now, called "imagemagick". I would now like
> to publish this branch.
>
> http://www.emacswiki.org/emacs/BzrForEmacsDevsmentions ways of publishing a branch to launchpad etc, but I cant find
> how to publish it on savannah. Could some kind soul enlighten me?


I tried this, which seemingly works:

bzr push sftp://jave@bzr.savannah.gnu.org/srv/bzr/emacs/imagemagick



>
>
>
>> Could you possibly also comment on the interface I chose for scaling and
>> rotation? If they are ok, I will document them in the NEWS entry. I
>> assume the "index" spec is ok since that is also used for gifs already.
>>
>> Also please comment if the interface to select which image types
>> imagemagick gets to handle is ok.
>>
>>>
>>>> +HAVE_IMAGEMAGICK=no
>>>> +if test "${HAVE_X11}" = "yes" ; then
>>>
>>> Do I understand it right that this X11-only restriction could be lifted
>>> at some point in the future?
>>
>> Yes. Theres nothing completely X specific in the patch I think, just the
>> same type of bitmap manipulations that goes on for the other image
>> libraries.
>>
>>>> diff --git a/src/dbusbind.c b/src/dbusbind.c
>>>> index 7c0be49..7a47730 100644
>>>> --- a/src/dbusbind.c
>>>> +++ b/src/dbusbind.c
>>>> @@ -773,6 +773,7 @@ xd_add_watch (watch, data)
>>>>        if (fd == -1)
>>>>  	return FALSE;
>>>>  
>>>> +
>>>>        /* Add the file descriptor to input_wait_mask.  */
>>>>        add_keyboard_wait_descriptor (fd);
>>>>      }
>>>
>>> Please drop this gratuitous change.
>>
>> Hmm, I have no idea how this crept in. Odd.
>>
>>
>>>> +/***********************************************************************
>>>> +				 imagemagick
>>>> + ***********************************************************************/
>>>> +#if defined (HAVE_IMAGEMAGICK)
>>>> +Lisp_Object Vimagemagick_render_type;
>>>> +/* Function prototypes.  */
>>>> +
>>>> +static int imagemagick_image_p P_ ((Lisp_Object object));
>>>> +static int imagemagick_load P_ ((struct frame *f, struct image *img));
>>>> +
>>>> +static int imagemagick_load_image P_ ((struct frame *, struct image *,
>>>> +                                       unsigned char *, unsigned int, unsigned char *));
>>>
>>> Please don't use the P_ macro in new code: just use prototypes.
>>
>> Ok. Like this?
>>
>> static int imagemagick_image_p (Lisp_Object object);
>> static int imagemagick_load (struct frame *f, struct image *img);
>>
>> static int imagemagick_load_image (struct frame *, struct image *,
>>                                        unsigned char *, unsigned int, unsigned char *);
>>
>>
>>>> +//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"
>>>
>>> This should be removed as well.
>>
>> Ok.
>>
>>>> +    //try if magicexportimage is any faster than pixelpushing
>>> [...]
>>>> +    //oddly, the below code doesnt seem to work:
>>>
>>> We currently only use /*...*/ comments, so I'd rather we don't introduce
>>> // comments for now.
>>>
>>
>> Sorry, I keep forgetting this, it seems to be in my muscle memory. 
>>
>>>         Stefan
>>>
-- 
Joakim Verona

Re: imagemmagick patch 5

[Juri Linkov]

>> I've done a local bzr branch now, called "imagemagick".

Thanks.  I tried it, but it seems you don't support older libraries.
In configure you check for libMagickWand-1, but I have only libWand-10.
Maybe I'll try to build the ImageMagick library from sources.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: imagemmagick patch 5

[joakim]

Juri Linkov <juri@jurta.org> writes:

>>> I've done a local bzr branch now, called "imagemagick".
>
> Thanks.  I tried it, but it seems you don't support older libraries.
> In configure you check for libMagickWand-1, but I have only libWand-10.
> Maybe I'll try to build the ImageMagick library from sources.

Since I only use a small subset of the magickwand api, it ought to be
possible to use older versions of the api. I havent checked if the api
has changed much though.

I'm developing on Fedora 12 where I get this newer version. Which
development system do you use? Maybe I could install it in a virtualbox
and try it out.

-- 
Joakim Verona