image-size

image-size

[Lars Magne Ingebrigtsen]

If I remember correctly from previous discussions, if you call
`image-size', Emacs will go through all the normal stuff it does before
displaying it, including pushing it to the X server, and then ask it how
big it was.  (I'm having a bit of a problem following the logic of
lookup_image, but it looks complicated.  :-)

If that's what's happening, it tallies with my experience when using shr
on big images.  shr (by default) scales down images that are too big, so
that they'll fit on the page.  But since it first "semi-displays" the
huge picture, then rescales, then displays the small picture, this is
uncomfortably slow.  At least under X.  And under ssh, it's unbearable.

So:

1) Is this what's going on?

and

2) If so, would someone mind very much if I alter `image-size' to do a
"fast path" iff a) we're asking for the pixel size, and b) we have
imagemagick compiled it?  If those two things are true, I could add some
code to just ask imagemagick how big the image is without involving the
display engine at all, and things would be a lot faster, I imagine.

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

Re: image-size

[Lars Magne Ingebrigtsen]

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

Er, whaddayou know.  I implemented this (patch below), and reading 9gag
via ssh is now a lot faster.

Does the patch look ok?


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

=== modified file 'src/image.c'
--- src/image.c	2013-05-12 19:17:04 +0000
+++ src/image.c	2013-06-20 12:17:20 +0000
@@ -876,6 +876,33 @@
 }
 
 
+#if defined (HAVE_IMAGEMAGICK)
+void
+imagemagick_image_size (unsigned char *contents, size_t size,
+			char *filename,
+			size_t *width, size_t *height);
+
+static void
+fast_image_size (Lisp_Object spec, size_t *width, size_t *height)
+{
+  Lisp_Object filename, data;
+
+  *width = -1;
+  *height = -1;
+
+  filename = Fplist_get (Fcdr (spec), QCfile);
+  if (! NILP (filename)) {
+    imagemagick_image_size (NULL, 0, SSDATA (x_find_image_file (filename)),
+			    width, height);
+    return;
+  }
+
+  data = Fplist_get (Fcdr (spec), QCdata);
+  if (! NILP (data))
+    imagemagick_image_size (SDATA (data), SBYTES (data), NULL, width, height);
+}
+#endif
+
 DEFUN ("image-size", Fimage_size, Simage_size, 1, 3, 0,
        doc: /* Return the size of image SPEC as pair (WIDTH . HEIGHT).
 PIXELS non-nil means return the size in pixels, otherwise return the
@@ -884,26 +911,33 @@
 or omitted means use the selected frame.  */)
   (Lisp_Object spec, Lisp_Object pixels, Lisp_Object frame)
 {
-  Lisp_Object size;
+  Lisp_Object size = Qnil;
 
-  size = Qnil;
-  if (valid_image_p (spec))
-    {
-      struct frame *f = decode_window_system_frame (frame);
-      ptrdiff_t id = lookup_image (f, spec);
-      struct image *img = IMAGE_FROM_ID (f, id);
-      int width = img->width + 2 * img->hmargin;
-      int height = img->height + 2 * img->vmargin;
-
-      if (NILP (pixels))
-	size = Fcons (make_float ((double) width / FRAME_COLUMN_WIDTH (f)),
-		      make_float ((double) height / FRAME_LINE_HEIGHT (f)));
-      else
-	size = Fcons (make_number (width), make_number (height));
-    }
-  else
+  if (! valid_image_p (spec))
     error ("Invalid image specification");
 
+#if defined (HAVE_IMAGEMAGICK)
+  if (! NILP (pixels)) {
+    size_t width, height;
+    fast_image_size (spec, &width, &height);
+    return Fcons (make_number (width), make_number (height));
+  }
+#endif
+
+  {
+    struct frame *f = decode_window_system_frame (frame);
+    ptrdiff_t id = lookup_image (f, spec);
+    struct image *img = IMAGE_FROM_ID (f, id);
+    size_t width = img->width + 2 * img->hmargin;
+    size_t height = img->height + 2 * img->vmargin;
+
+    if (NILP (pixels))
+      size = Fcons (make_float ((double) width / FRAME_COLUMN_WIDTH (f)),
+		    make_float ((double) height / FRAME_LINE_HEIGHT (f)));
+    else
+      size = Fcons (make_number (width), make_number (height));
+  }
+
   return size;
 }
 
@@ -7616,6 +7650,52 @@
   description = (char *) MagickRelinquishMemory (description);
 }
 
+/* Return the size of an image that's either contained in contents or
+   filename.  If we couldn't read the image, return -1 as the
+   width/height. */
+
+void
+imagemagick_image_size (unsigned char *contents, size_t size,
+			char *filename,
+			size_t *width, size_t *height)
+{
+  MagickWand *image_wand, *ping_wand;
+
+  *width = -1;
+  *height = -1;
+
+  /* Initialize the imagemagick environment.  */
+  MagickWandGenesis ();
+  ping_wand = NewMagickWand ();
+
+  if ((filename
+       ? MagickPingImage (ping_wand, filename)
+       : MagickPingImageBlob (ping_wand, contents, size))
+      == MagickFalse)
+    {
+      imagemagick_error (ping_wand);
+      DestroyMagickWand (ping_wand);
+      return;
+    }
+
+  DestroyMagickWand (ping_wand);
+  image_wand = NewMagickWand ();
+
+  if ((filename
+       ? MagickReadImage (image_wand, filename)
+       : MagickReadImageBlob (image_wand, contents, size))
+      == MagickFalse)
+    {
+      imagemagick_error (image_wand);
+    } else {
+      *height = MagickGetImageHeight (image_wand);
+      *width = MagickGetImageWidth (image_wand);
+    }
+
+  DestroyMagickWand (image_wand);
+  MagickWandTerminus ();
+}
+
 /* 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


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



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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2013 12:04:52 +0200
> 
> If I remember correctly from previous discussions, if you call
> `image-size', Emacs will go through all the normal stuff it does before
> displaying it, including pushing it to the X server, and then ask it how
> big it was.

I don't think so.  Where do you see X calls that "push the image to
the X server"?

I see no display stuff in image.c, only loading of the image and
storing all kinds of metrics of it in an object.  Display happens
later.

> (I'm having a bit of a problem following the logic of lookup_image,
> but it looks complicated.  :-)

The only relevant part is this:

      img->load_failed_p = ! img->type->load (f, img);

This invokes the 'load' method of the relevant image type.

> 2) If so, would someone mind very much if I alter `image-size' to do a
> "fast path" iff a) we're asking for the pixel size, and b) we have
> imagemagick compiled it?  If those two things are true, I could add some
> code to just ask imagemagick how big the image is without involving the
> display engine at all, and things would be a lot faster, I imagine.

I see no reason for relying on imagemagick.  You will see in each
'load' method a short fragment of code that finds the image size.  If
you want a "fast path", just add another flag argument to the 'load'
method that tells it to return as soon as it computed the size, and
that's it.

Restricting this to Emacs built with imagemagick would be a
regrettable limitation, IMO.

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think so.  Where do you see X calls that "push the image to
> the X server"?

I don't, really, but I think somebody said that the last time I asked
"why is `image-size' so slow"?

> I see no reason for relying on imagemagick.  You will see in each
> 'load' method a short fragment of code that finds the image size.  If
> you want a "fast path", just add another flag argument to the 'load'
> method that tells it to return as soon as it computed the size, and
> that's it.

If Emacs doesn't do anything weird in lookup_image, then why does it
take so long?  Like I said, when running Emacs over ssh, `image-size' is
very slow.  Calling it on a medium-size image typically takes almost a
second from home to work.  If I just call the ImageMagick function size
function directly, it takes 0.01s on the same image.

> Restricting this to Emacs built with imagemagick would be a
> regrettable limitation, IMO.

Well, it's an optimisation.  Which I think 99% of the binary-distributed
Emacs copies has, so I didn't really see the need.

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2013 14:18:34 +0200
> 
> Er, whaddayou know.  I implemented this (patch below), and reading 9gag
> via ssh is now a lot faster.
> 
> Does the patch look ok?

I'd rather not require imagemagick for such a simple job.

Re: image-size

[Lars Magne Ingebrigtsen]

Here's some benchmarking.

This is via ssh, using `lookup-image':

(benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
=> 2.695448783

The next call is a lot faster, meaning that something has been pushed to
X already:

(benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
=> 0.000274576

The "fast" version is consistently fast, but not as fast as the cached
`lookup-image' version:

(benchmark-elapse (image-size (create-image "/tmp/0.jpg") t))
=> 0.045494903

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2013 18:55:56 +0200
> 
> Here's some benchmarking.
> 
> This is via ssh, using `lookup-image':

What exactly goes "via ssh"?

> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
> => 2.695448783

Where can I find this file to try that?

> The next call is a lot faster, meaning that something has been pushed to
> X already:
> 
> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
> => 0.000274576

It's not pushed to X, it is cached internally by Emacs in its image
cache.

> The "fast" version is consistently fast, but not as fast as the cached
> `lookup-image' version:
> 
> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") t))
> => 0.045494903

I suggest to try my suggestion, it might be fast enough.  With most
formats, finding just the size of the image is very simple, you can
see it yourself in image.c (search for "_load", which will find the
various 'load' methods).

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What exactly goes "via ssh"?

Emacs is opened via X tunnelling over ssh.

>> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
>> => 2.695448783
>
> Where can I find this file to try that?

Just use any medium-sized image -- it doesn't matter much.  That one was
from 9gag somewhere.

>> The next call is a lot faster, meaning that something has been pushed to
>> X already:
>> 
>> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
>> => 0.000274576
>
> It's not pushed to X, it is cached internally by Emacs in its image
> cache.

If it's not transferred via X, why is it slow when running Emacs over
ssh, but not locally?

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 19:54:38 +0200
> 
> >> The next call is a lot faster, meaning that something has been pushed to
> >> X already:
> >> 
> >> (benchmark-elapse (image-size (create-image "/tmp/0.jpg") nil))
> >> => 0.000274576
> >
> > It's not pushed to X, it is cached internally by Emacs in its image
> > cache.
> 
> If it's not transferred via X, why is it slow when running Emacs over
> ssh, but not locally?

I don't know.  Please profile image-size using the built-in profiler,
in a loop that clears the image cache after each call to image-size
(using clear-image-cache or image-flush).

Re: image-size

[Eli Zaretskii]

> Date: Thu, 20 Jun 2013 21:15:44 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > If it's not transferred via X, why is it slow when running Emacs over
> > ssh, but not locally?
> 
> I don't know.

Actually, I have a guess: the time it takes is needed to decompress
the JPEG data.  On my machine, even with local files of 3MB size it
takes about a second.

If I use other types of images, like PNG, it is much faster, and with
XPM it is instantaneous.

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > If it's not transferred via X, why is it slow when running Emacs over
>> > ssh, but not locally?
>> 
>> I don't know.
>
> Actually, I have a guess: the time it takes is needed to decompress
> the JPEG data.  On my machine, even with local files of 3MB size it
> takes about a second.

[larsi@stories /tmp]$ time djpeg /tmp/0.jpg > /dev/null

real    0m0.028s
user    0m0.025s
sys     0m0.003s

> If I use other types of images, like PNG, it is much faster, and with
> XPM it is instantaneous.

And, like I said, if X is local, it's virtually instantaneous.  It's
network traffic related.

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 20:46:20 +0200
> 
> > Actually, I have a guess: the time it takes is needed to decompress
> > the JPEG data.  On my machine, even with local files of 3MB size it
> > takes about a second.
> 
> [larsi@stories /tmp]$ time djpeg /tmp/0.jpg > /dev/null
> 
> real    0m0.028s
> user    0m0.025s
> sys     0m0.003s

djpeg is not Emacs.  I just stepped through jpeg_load_body, and this
line takes about 1.5 sec for a 3 MB JPEG image:

  fn_jpeg_start_decompress (&mgr->cinfo);

Why don't you do the same (step with a debugger through that function)
and see where that time goes in your case?

> And, like I said, if X is local, it's virtually instantaneous.  It's
> network traffic related.

Not here.  And I don't even have X.

Anyway, if in your case it's network related, prove it.

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> djpeg is not Emacs.  I just stepped through jpeg_load_body, and this
> line takes about 1.5 sec for a 3 MB JPEG image:

Yeah, but my image isn't that big:

[larsi@stories /tmp]$ ls -lh /tmp/0.jpg 
-rw-r--r-- 1 larsi larsi 220K Jun 20 18:52 /tmp/0.jpg

>   fn_jpeg_start_decompress (&mgr->cinfo);

And it uses imagemagick, not that function.  :-)

>> And, like I said, if X is local, it's virtually instantaneous.  It's
>> network traffic related.
>
> Not here.  And I don't even have X.
>
> Anyway, if in your case it's network related, prove it.

I was hoping that someone who knows something about Emacs and images
under X would pipe up so that I won't have to.  :-)

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

Re: image-size

[Lars Magne Ingebrigtsen]

With this image:

http://www.evo.org/html/scans/BAD0013/BAD0013-1-l.jpg

[larsi@stories ~/mgnus]$ ls -l /home/larsi/Catalogue/html/scans/BAD0013/BAD0011-l.jpg
-rw-r--r-- 1 larsi users 2399848 Feb 25  2007 /home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg

(benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") nil))
=> 46.614988405

46 seconds to query the size.

With a local X, this is the result:

(benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") nil))
=> 0.572555233


strace shows that this is what Emacs does during that time:

pid 16301]      0.000055 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\265\6\0\30 \4", 24}, {"\"\"\34\0\"\"\34\0\"\"\34\0\"\"\34\0\r\10\v\0\32\27\25\0\32\27\25\0\32\27\25\0"..., 248608}, {"", 0}], 3) = 248632
[pid 16301]      0.000055 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\306\6\0\30 \4", 24}, {"\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0"..., 248608}, {"", 0}], 3) = 248632
[pid 16301]      0.000191 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\327\6\0\30 \4", 24}, {"\32\27\25\0\"\"\34\0\32\27\25\0#$&\0\"\"\34\0\"\"\34\0\32\27\25\0#$&\0"..., 248608}, {"", 0}], 3) = 248632

fd 4 is the X-over-ssh socket.

[larsi@stories ~/mgnus]$ grep 248632 /tmp/s | wc -l
214

(* 214 248608)
=> 53202112

So it transferred about 53MB over the net, which is a bit more than
1MB/s, which is about half the size of the connection I have to work.
Which makes sense, since I'm ssh-ing first to work, and then back again.

Proof!!!1!

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 21:18:33 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > djpeg is not Emacs.  I just stepped through jpeg_load_body, and this
> > line takes about 1.5 sec for a 3 MB JPEG image:
> 
> Yeah, but my image isn't that big:
> 
> [larsi@stories /tmp]$ ls -lh /tmp/0.jpg 
> -rw-r--r-- 1 larsi larsi 220K Jun 20 18:52 /tmp/0.jpg
> 
> >   fn_jpeg_start_decompress (&mgr->cinfo);
> 
> And it uses imagemagick, not that function.  :-)

So we are comparing apples with oranges.  And if there's any network
effect, it is within imagemagick in your case, right?  Maybe
imagemagick does involve X, I have no idea about its internals.

> I was hoping that someone who knows something about Emacs and images
> under X would pipe up so that I won't have to.  :-)

I do know about that.  I told you what I know and see: Emacs does not
invoke any display related code inside image-size, but you don't
believe me for some reason.

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 21:34:10 +0200
> 
> With this image:
> 
> http://www.evo.org/html/scans/BAD0013/BAD0013-1-l.jpg
> 
> [larsi@stories ~/mgnus]$ ls -l /home/larsi/Catalogue/html/scans/BAD0013/BAD0011-l.jpg
> -rw-r--r-- 1 larsi users 2399848 Feb 25  2007 /home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg
> 
> (benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") nil))
> => 46.614988405
> 
> 46 seconds to query the size.
> 
> With a local X, this is the result:
> 
> (benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") nil))
> => 0.572555233
> 
> 
> strace shows that this is what Emacs does during that time:
> 
> pid 16301]      0.000055 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\265\6\0\30 \4", 24}, {"\"\"\34\0\"\"\34\0\"\"\34\0\"\"\34\0\r\10\v\0\32\27\25\0\32\27\25\0\32\27\25\0"..., 248608}, {"", 0}], 3) = 248632
> [pid 16301]      0.000055 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\306\6\0\30 \4", 24}, {"\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0\32\27\25\0"..., 248608}, {"", 0}], 3) = 248632
> [pid 16301]      0.000191 writev(4, [{"H\2\316\362^\0 \4_\0 \4H\16\21\0\0\0\327\6\0\30 \4", 24}, {"\32\27\25\0\"\"\34\0\32\27\25\0#$&\0\"\"\34\0\"\"\34\0\32\27\25\0#$&\0"..., 248608}, {"", 0}], 3) = 248632
> 
> fd 4 is the X-over-ssh socket.

Is this using imagemagick or using libjpeg?

If the former, perhaps imagemagick is the only library that needs your
"fast path".  The other image libraries don't, as they clearly don't
write to X (otherwise I couldn't have used them on Windows).

Re: image-size

[David Engster]

Eli Zaretskii writes:
>> I was hoping that someone who knows something about Emacs and images
>> under X would pipe up so that I won't have to.  :-)
>
> I do know about that.  I told you what I know and see: Emacs does not
> invoke any display related code inside image-size, but you don't
> believe me for some reason.

This issue was discussed in 2010. The beginning of the full thread is
here:

http://thread.gmane.org/gmane.emacs.devel/130714

If you don't want to read it all: It seems that the call to
'lookup-image' is the offender. I guess this is a good summary by
Yidong with a pointer on how to fix this:

http://article.gmane.org/gmane.emacs.devel/130785

-David

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Is this using imagemagick or using libjpeg?

imagemagick.

> If the former, perhaps imagemagick is the only library that needs your
> "fast path".  The other image libraries don't, as they clearly don't
> write to X (otherwise I couldn't have used them on Windows).

I can't see anything in the imagemagick code that talks to X, so I think
it's a general Emacs image thingie.

Anybody?

Anyway, this has led me to consider this a bit more.  Sure, this is
faster than 46 seconds:

(benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") t))
=> 0.226372938

But on the other hand, the next time we open the file to scale it down,
we'll have to decode the image yet again, which will take 0.23 seconds
once again.

Wouldn't it be nice if we could just pass in :max-width and :max-height
to `create-image', and `put-image' would then respect this when actually
creating the image?

Then the image would only have to be decoded once, and we'd have the
best of both worlds.  All other uses of `image-size' that I can see in
shr are for images that are or will definitely be inserted, so they'll
already have been (or will be) transferred over X.

So it fits my use case, and will make displaying sensibly-sized images
fast and convenient in general.  (If you have imagemagick, that is.
Most of the other image renderers don't support resizing, and ignore
:width and :height, so they should also ignore :max-width and
:max-height.)

Opinions?

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

Re: image-size

[Eli Zaretskii]

> From: David Engster <deng@randomsample.de>
> Cc: Lars Magne Ingebrigtsen <larsi@gnus.org>,  emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 21:52:19 +0200
> 
> This issue was discussed in 2010. The beginning of the full thread is
> here:
> 
> http://thread.gmane.org/gmane.emacs.devel/130714
> 
> If you don't want to read it all: It seems that the call to
> 'lookup-image' is the offender. I guess this is a good summary by
> Yidong with a pointer on how to fix this:
> 
> http://article.gmane.org/gmane.emacs.devel/130785

That is exactly what I suggested in

  http://lists.gnu.org/archive/html/emacs-devel/2013-06/msg00792.html

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 20 Jun 2013 21:52:29 +0200
> 
> > If the former, perhaps imagemagick is the only library that needs your
> > "fast path".  The other image libraries don't, as they clearly don't
> > write to X (otherwise I couldn't have used them on Windows).
> 
> I can't see anything in the imagemagick code that talks to X, so I think
> it's a general Emacs image thingie.

IT CANNOT BE TRUE!!!!  IF IT WERE TRUE, I COULD NOT HAVE USED IMAGES
ON WINDOWS, where there's no X.

> Anyway, this has led me to consider this a bit more.  Sure, this is
> faster than 46 seconds:
> 
> (benchmark-elapse (image-size (create-image "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") t))
> => 0.226372938
> 
> But on the other hand, the next time we open the file to scale it down,
> we'll have to decode the image yet again, which will take 0.23 seconds
> once again.

If it is in the cache, Emacs will fetch it from there.

Re: image-size

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I can't see anything in the imagemagick code that talks to X, so I think
>> it's a general Emacs image thingie.
>
> IT CANNOT BE TRUE!!!!  IF IT WERE TRUE, I COULD NOT HAVE USED IMAGES
> ON WINDOWS, where there's no X.

Of course there is no X stuff on Windows.  But if you're running over X,
lookup_image pushes data over X, for some reason or other.  I have no
idea what the reason might be, but perhaps somebody else does.

>> Anyway, this has led me to consider this a bit more.  Sure, this is
>> faster than 46 seconds:
>> 
>> (benchmark-elapse (image-size (create-image
>> "/home/larsi/Catalogue/html/scans/BAD0013/BAD0013-1-l.jpg") t))
>> => 0.226372938
>> 
>> But on the other hand, the next time we open the file to scale it down,
>> we'll have to decode the image yet again, which will take 0.23 seconds
>> once again.
>
> If it is in the cache, Emacs will fetch it from there.

It would decode twice with the two-part `image-size'/`put-image' code,
since `image-size' would then not put things into the cache.

As Chong said:

> You have to replace the call to lookup_image in `image-size' with a
> new function that avoids loading the image.

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

Re: image-size

[Lars Magne Ingebrigtsen]

I think I found the culprit:

      /* 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))

way down in imagemagick_load_image.  Hm.  Also in xpm_load_image.  And
pbm_load.  Or does it do something else?  Hm...  I know nothing about X,
but it looks pretty X-ey:

  *ximg = XCreateImage (display, DefaultVisualOfScreen (screen),
			depth, ZPixmap, 0, NULL, width, height,
			depth > 16 ? 32 : depth > 8 ? 16 : 8, 0);


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

Re: image-size

[Jan Djärv]

Hello.

20 jun 2013 kl. 22:57 skrev Lars Magne Ingebrigtsen <larsi@gnus.org>:

> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> I can't see anything in the imagemagick code that talks to X, so I think
>>> it's a general Emacs image thingie.
>> 
>> IT CANNOT BE TRUE!!!!  IF IT WERE TRUE, I COULD NOT HAVE USED IMAGES
>> ON WINDOWS, where there's no X.
> 
> Of course there is no X stuff on Windows.  But if you're running over X,
> lookup_image pushes data over X, for some reason or other.  I have no
> idea what the reason might be, but perhaps somebody else does.


lookup_image calls img->type->load, and jpeg_load creates an X pixmap (x_create_x_image_and_pixmap), i.e. on the X server, and copies the image into the pixmap, i.e. transfers it to the server (XPutImage).

The functions are totally different for X, W32 and NS, so how it works on W32 is totally irrelevant for X and vice versa.

Loading the image just to determine the size is a huge overhead.  The image loading routines already know the image size before sending it to the server (or the equivalent for W32 and NS), otherwise they could not allocate the right size for the Pixmap.  No Imagemagic is needed to figure out the image size.

	Jan D.

Re: image-size

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 20 Jun 2013 23:42:28 +0200, Jan Djärv <jan.h.d@swipnet.se> said:

>> Of course there is no X stuff on Windows.  But if you're running
>> over X, lookup_image pushes data over X, for some reason or other.
>> I have no idea what the reason might be, but perhaps somebody else
>> does.

> lookup_image calls img->type->load, and jpeg_load creates an X
> pixmap (x_create_x_image_and_pixmap), i.e. on the X server, and
> copies the image into the pixmap, i.e. transfers it to the server
> (XPutImage).

> The functions are totally different for X, W32 and NS, so how it
> works on W32 is totally irrelevant for X and vice versa.

> Loading the image just to determine the size is a huge overhead.
> The image loading routines already know the image size before
> sending it to the server (or the equivalent for W32 and NS),
> otherwise they could not allocate the right size for the Pixmap.  No
> Imagemagic is needed to figure out the image size.

Right.  And as I mentioned in (*), a possible way to solve this
problem is to defer the creation of Pixmap (server side resource) from
XImage (client side resource) until the actual display happens (i.e.,
at prepare_image_for_display rather than lookup_image), so as to avoid
unnecessary round trip of image data between the X11 server and
client.

*: http://lists.gnu.org/archive/html/emacs-devel/2010-09/msg01285.html

On the Carbon port and its descendants, deferring the creation of
image data for display (CGImage on OS X) from the raw pixel data
(struct _XImage, a custom data structure made after X11) as above has
been done for a long time.  And I've never heard of a problem about
that.

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

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2013 23:06:45 +0200
> Cc: emacs-devel@gnu.org
> 
> I think I found the culprit:
> 
>       /* 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))
> 
> way down in imagemagick_load_image.  Hm.  Also in xpm_load_image.  And
> pbm_load.  Or does it do something else?  Hm...  I know nothing about X,
> but it looks pretty X-ey:
> 
>   *ximg = XCreateImage (display, DefaultVisualOfScreen (screen),
> 			depth, ZPixmap, 0, NULL, width, height,
> 			depth > 16 ? 32 : depth > 8 ? 16 : 8, 0);

That's _after_ we already know the size of the image, so the
suggestion I made earlier, which is ask each 'load' method to return
right after computing the size, given a flag parameter in a call,
would solve this problem.

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 20 Jun 2013 22:57:10 +0200
> Cc: emacs-devel@gnu.org
> 
> As Chong said:
> 
> > You have to replace the call to lookup_image in `image-size' with a
> > new function that avoids loading the image.

What do you mean by "loading the image"?  You cannot have the size
without reading the image file.

Re: image-size

[Eli Zaretskii]

> From: Jan Djärv <jan.h.d@swipnet.se>
> Date: Thu, 20 Jun 2013 23:42:28 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel@gnu.org
> 
> Loading the image just to determine the size is a huge overhead.  The image loading routines already know the image size before sending it to the server (or the equivalent for W32 and NS), otherwise they could not allocate the right size for the Pixmap.  No Imagemagic is needed to figure out the image size.

Yes.  Thus my suggestion to have a flag in the 'load' method that
would cause it return right after computing the size.

Re: image-size

[Lars Magne Ingebrigtsen]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> Right.  And as I mentioned in (*), a possible way to solve this
> problem is to defer the creation of Pixmap (server side resource) from
> XImage (client side resource) until the actual display happens (i.e.,
> at prepare_image_for_display rather than lookup_image), so as to avoid
> unnecessary round trip of image data between the X11 server and
> client.

I think that sounds like a brilliant idea.  You wouldn't have a patch
that does that, by any chance?  :-)

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

Re: image-size

[Jan Djärv]

Hello.

21 jun 2013 kl. 08:15 skrev Eli Zaretskii <eliz@gnu.org>:

>> From: Jan Djärv <jan.h.d@swipnet.se>
>> Date: Thu, 20 Jun 2013 23:42:28 +0200
>> Cc: Eli Zaretskii <eliz@gnu.org>,
>> emacs-devel@gnu.org
>> 
>> Loading the image just to determine the size is a huge overhead.  The image loading routines already know the image size before sending it to the server (or the equivalent for W32 and NS), otherwise they could not allocate the right size for the Pixmap.  No Imagemagic is needed to figure out the image size.
> 
> Yes.  Thus my suggestion to have a flag in the 'load' method that
> would cause it return right after computing the size.
> 

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> Right.  And as I mentioned in (*), a possible way to solve this
> problem is to defer the creation of Pixmap (server side resource) from
> XImage (client side resource) until the actual display happens (i.e.,
> at prepare_image_for_display rather than lookup_image), so as to avoid
> unnecessary round trip of image data between the X11 server and
> client.

Only prepare_image_for_display have to set a different value for the suggested flag, as it does load if the image is not loaded yet.

	Jan D.

Re: image-size

[Eli Zaretskii]

> From: Jan Djärv <jan.h.d@swipnet.se>
> Date: Fri, 21 Jun 2013 09:34:31 +0200
> Cc: larsi@gnus.org,
>  emacs-devel@gnu.org
> 
> > Yes.  Thus my suggestion to have a flag in the 'load' method that
> > would cause it return right after computing the size.
> > 
> 
> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
> 
> > Right.  And as I mentioned in (*), a possible way to solve this
> > problem is to defer the creation of Pixmap (server side resource) from
> > XImage (client side resource) until the actual display happens (i.e.,
> > at prepare_image_for_display rather than lookup_image), so as to avoid
> > unnecessary round trip of image data between the X11 server and
> > client.
> 
> Only prepare_image_for_display have to set a different value for the suggested flag, as it does load if the image is not loaded yet.

Right.  prepare_image_for_display is called by the display engine when
it is about to display the image.

Re: image-size

[Jan Djärv]

Hello.

Some image types needs a bit more coding for this to work.  I.e. Xpm uses XpmReadFileToPixmap which reads the file, returns the sizes and puts it on the server in one go.
This would have to be reworked so it uses XpmReadFileToImage or similar.
Ditto for XpmReadFileToPixmap.

	Jan D.

21 jun 2013 kl. 10:27 skrev Eli Zaretskii <eliz@gnu.org>:

>> From: Jan Djärv <jan.h.d@swipnet.se>
>> Date: Fri, 21 Jun 2013 09:34:31 +0200
>> Cc: larsi@gnus.org,
>> emacs-devel@gnu.org
>> 
>>> Yes.  Thus my suggestion to have a flag in the 'load' method that
>>> would cause it return right after computing the size.
>>> 
>> 
>> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>> 
>>> Right.  And as I mentioned in (*), a possible way to solve this
>>> problem is to defer the creation of Pixmap (server side resource) from
>>> XImage (client side resource) until the actual display happens (i.e.,
>>> at prepare_image_for_display rather than lookup_image), so as to avoid
>>> unnecessary round trip of image data between the X11 server and
>>> client.
>> 
>> Only prepare_image_for_display have to set a different value for the suggested flag, as it does load if the image is not loaded yet.
> 
> Right.  prepare_image_for_display is called by the display engine when
> it is about to display the image.
> 

Re: image-size

[Lars Magne Ingebrigtsen]

Jan Djärv <jan.h.d@swipnet.se> writes:

> Some image types needs a bit more coding for this to work.  I.e. Xpm
> uses XpmReadFileToPixmap which reads the file, returns the sizes and
> puts it on the server in one go.
> This would have to be reworked so it uses XpmReadFileToImage or similar.
> Ditto for XpmReadFileToPixmap.

While it would be good to make all the loaders postpone pushing the data
to the X server, it's probably not necessary to convert them all (at
once).  XPM images are usually used for (smal) icons and the like.  The
ones that really hurt performance day-to-day are
jpeg/png/gif/imagemagick images.

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

Re: image-size

[Eli Zaretskii]

> From: Jan Djärv <jan.h.d@swipnet.se>
> Date: Fri, 21 Jun 2013 11:03:23 +0200
> Cc: larsi@gnus.org,
>  emacs-devel@gnu.org
> 
> Some image types needs a bit more coding for this to work.  I.e. Xpm uses XpmReadFileToPixmap which reads the file, returns the sizes and puts it on the server in one go.
> This would have to be reworked so it uses XpmReadFileToImage or similar.
> Ditto for XpmReadFileToPixmap.

Right.  The MS-Windows port already does that, so this boils down to
converting a compile-time condition into a run-time condition.

Re: image-size

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 21 Jun 2013 11:07:34 +0200
> 
> While it would be good to make all the loaders postpone pushing the data
> to the X server, it's probably not necessary to convert them all (at
> once).  XPM images are usually used for (smal) icons and the like.  The
> ones that really hurt performance day-to-day are
> jpeg/png/gif/imagemagick images.

That's true, but it's easy to do that for all of them, and the API
needs to be identical anyway.

Re: image-size

[Willem Rein Oudshoorn]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
>> Date: Thu, 20 Jun 2013 22:57:10 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> As Chong said:
>> 
>> > You have to replace the call to lookup_image in `image-size' with a
>> > new function that avoids loading the image.
>
> What do you mean by "loading the image"?  You cannot have the size
> without reading the image file.

Well, I do not know for jpeg etc. But the png file format has the image
size in pixels right at the start of the file, so you only need to read
in a tiny part of the file.

For png the width is big-endian integer starting at byte 16, and
the height starts at 20.

Wim Oudshoorn.

Re: image-size

[YAMAMOTO Mitsuharu]

>>>>> On Fri, 21 Jun 2013 08:27:58 +0200, Lars Magne Ingebrigtsen <larsi@gnus.org> said:

>> Right.  And as I mentioned in (*), a possible way to solve this
>> problem is to defer the creation of Pixmap (server side resource)
>> from XImage (client side resource) until the actual display happens
>> (i.e., at prepare_image_for_display rather than lookup_image), so
>> as to avoid unnecessary round trip of image data between the X11
>> server and client.

> I think that sounds like a brilliant idea.  You wouldn't have a
> patch that does that, by any chance?  :-)

Could you try this?  It doesn't defer the allocation of Pixmap
actually, but defers image data transfer by XPutImage (and also by
XGetImage in the case of postprocessing) until the actual display
happens.

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

=== modified file 'src/image.c'
*** src/image.c	2013-06-23 19:24:27 +0000
--- src/image.c	2013-06-26 08:17:12 +0000
***************
*** 106,113 ****
  #define GET_PIXEL(ximg, x, y) XGetPixel (ximg, x, y)
  #define NO_PIXMAP 0
  
- #define ZPixmap 0
- 
  #define PIX_MASK_RETAIN	0
  #define PIX_MASK_DRAW	1
  
--- 106,111 ----
***************
*** 146,161 ****
     data more than once will not be caught.  */
  
  #ifdef HAVE_NS
- XImagePtr
- XGetImage (Display *display, Pixmap pixmap, int x, int y,
-            unsigned int width, unsigned int height,
-            unsigned long plane_mask, int format)
- {
-   /* TODO: not sure what this function is supposed to do.. */
-   ns_retain_object (pixmap);
-   return pixmap;
- }
- 
  /* Use with images created by ns_image_for_XPM.  */
  unsigned long
  XGetPixel (XImagePtr ximage, int x, int y)
--- 144,149 ----
***************
*** 435,442 ****
--- 423,445 ----
  					 XImagePtr *, Pixmap *);
  static void x_destroy_x_image (XImagePtr ximg);
  
+ #ifdef HAVE_NTGUI
+ static XImagePtr_or_DC image_get_x_image_or_dc (struct frame *, struct image *,
+ 						bool, HGDIOBJ *);
+ static void image_unget_x_image_or_dc (XImagePtr_or_DC, HGDIOBJ);
+ #else
+ static XImagePtr image_get_x_image (struct frame *, struct image *, bool);
+ static void image_unget_x_image (XImagePtr);
+ #define image_get_x_image_or_dc(f, img, mask_p, dummy)	\
+   image_get_x_image (f, img, mask_p)
+ #define image_unget_x_image_or_dc(ximg, dummy)	\
+   image_unget_x_image (ximg)
+ #endif
+ 
  #ifdef HAVE_X_WINDOWS
  
+ static void image_sync_to_pixmaps (struct frame *, struct image *);
+ 
  /* Useful functions defined in the section
     `Image type independent image structures' below. */
  
***************
*** 1050,1055 ****
--- 1053,1066 ----
    if (img->pixmap == NO_PIXMAP && !img->load_failed_p)
      img->load_failed_p = ! img->type->load (f, img);
  
+ #ifdef HAVE_X_WINDOWS
+   if (!img->load_failed_p)
+     {
+       block_input ();
+       image_sync_to_pixmaps (f, img);
+       unblock_input ();
+     }
+ #endif
  }
  
  
***************
*** 1145,1169 ****
  
  #ifdef HAVE_NTGUI
  
- #define Destroy_Image(img_dc, prev) \
-   do { SelectObject (img_dc, prev); DeleteDC (img_dc); } while (0)
- 
  #define Free_Pixmap(display, pixmap) \
    DeleteObject (pixmap)
  
  #elif defined (HAVE_NS)
  
- #define Destroy_Image(ximg, dummy) \
-   ns_release_object (ximg)
- 
  #define Free_Pixmap(display, pixmap) \
    ns_release_object (pixmap)
  
  #else
  
- #define Destroy_Image(ximg, dummy) \
-   XDestroyImage (ximg)
- 
  #define Free_Pixmap(display, pixmap) \
    XFreePixmap (display, pixmap)
  
--- 1156,1171 ----
***************
*** 1187,1208 ****
  #endif /* HAVE_NTGUI */
  
        if (free_ximg)
! 	{
! #ifndef HAVE_NTGUI
! 	  ximg = XGetImage (FRAME_X_DISPLAY (f), img->pixmap,
! 			    0, 0, img->width, img->height, ~0, ZPixmap);
! #else
! 	  HDC frame_dc = get_frame_dc (f);
! 	  ximg = CreateCompatibleDC (frame_dc);
! 	  release_frame_dc (f, frame_dc);
! 	  prev = SelectObject (ximg, img->pixmap);
! #endif /* !HAVE_NTGUI */
! 	}
  
        img->background = four_corners_best (ximg, img->corners, img->width, img->height);
  
        if (free_ximg)
! 	Destroy_Image (ximg, prev);
  
        img->background_valid = 1;
      }
--- 1189,1200 ----
  #endif /* HAVE_NTGUI */
  
        if (free_ximg)
! 	ximg = image_get_x_image_or_dc (f, img, 0, &prev);
  
        img->background = four_corners_best (ximg, img->corners, img->width, img->height);
  
        if (free_ximg)
! 	image_unget_x_image_or_dc (ximg, prev);
  
        img->background_valid = 1;
      }
***************
*** 1228,1250 ****
  #endif /* HAVE_NTGUI */
  
  	  if (free_mask)
! 	    {
! #ifndef HAVE_NTGUI
! 	      mask = XGetImage (FRAME_X_DISPLAY (f), img->mask,
! 				0, 0, img->width, img->height, ~0, ZPixmap);
! #else
! 	      HDC frame_dc = get_frame_dc (f);
! 	      mask = CreateCompatibleDC (frame_dc);
! 	      release_frame_dc (f, frame_dc);
! 	      prev = SelectObject (mask, img->mask);
! #endif /* HAVE_NTGUI */
! 	    }
  
  	  img->background_transparent
  	    = (four_corners_best (mask, img->corners, img->width, img->height) == PIX_MASK_RETAIN);
  
  	  if (free_mask)
! 	    Destroy_Image (mask, prev);
  	}
        else
  	img->background_transparent = 0;
--- 1220,1232 ----
  #endif /* HAVE_NTGUI */
  
  	  if (free_mask)
! 	    mask = image_get_x_image_or_dc (f, img, 1, &prev);
  
  	  img->background_transparent
  	    = (four_corners_best (mask, img->corners, img->width, img->height) == PIX_MASK_RETAIN);
  
  	  if (free_mask)
! 	    image_unget_x_image_or_dc (mask, prev);
  	}
        else
  	img->background_transparent = 0;
***************
*** 1260,1289 ****
  		  Helper functions for X image types
   ***********************************************************************/
  
! /* Clear X resources of image IMG on frame F.  PIXMAP_P means free the
!    pixmap if any.  MASK_P means clear the mask pixmap if any.
!    COLORS_P means free colors allocated for the image, if any.  */
  
  static void
! x_clear_image_1 (struct frame *f, struct image *img, bool pixmap_p,
! 		 bool mask_p, bool colors_p)
  {
!   if (pixmap_p && img->pixmap)
      {
!       Free_Pixmap (FRAME_X_DISPLAY (f), img->pixmap);
!       img->pixmap = NO_PIXMAP;
!       /* NOTE (HAVE_NS): background color is NOT an indexed color! */
!       img->background_valid = 0;
      }
  
!   if (mask_p && img->mask)
      {
!       Free_Pixmap (FRAME_X_DISPLAY (f), img->mask);
!       img->mask = NO_PIXMAP;
!       img->background_transparent_valid = 0;
      }
  
!   if (colors_p && img->ncolors)
      {
        /* W32_TODO: color table support.  */
  #ifdef HAVE_X_WINDOWS
--- 1242,1299 ----
  		  Helper functions for X image types
   ***********************************************************************/
  
! /* Clear X resources of image IMG on frame F according to FLAGS.
!    FLAGS is bitwise-or of the following masks:
!    CLEAR_IMAGE_PIXMAP free the pixmap if any.
!    CLEAR_IMAGE_MASK means clear the mask pixmap if any.
!    CLEAR_IMAGE_COLORS means free colors allocated for the image, if
!      any.  */
! 
! #define CLEAR_IMAGE_PIXMAP	(1 << 0)
! #define CLEAR_IMAGE_MASK	(1 << 1)
! #define CLEAR_IMAGE_COLORS	(1 << 2)
  
  static void
! x_clear_image_1 (struct frame *f, struct image *img, int flags)
  {
!   if (flags & CLEAR_IMAGE_PIXMAP)
      {
!       if (img->pixmap)
! 	{
! 	  Free_Pixmap (FRAME_X_DISPLAY (f), img->pixmap);
! 	  img->pixmap = NO_PIXMAP;
! 	  /* NOTE (HAVE_NS): background color is NOT an indexed color! */
! 	  img->background_valid = 0;
! 	}
! #ifdef HAVE_X_WINDOWS
!       if (img->ximg)
! 	{
! 	  x_destroy_x_image (img->ximg);
! 	  img->ximg = NULL;
! 	  img->background_valid = 0;
! 	}
! #endif
      }
  
!   if (flags & CLEAR_IMAGE_MASK)
      {
!       if (img->mask)
! 	{
! 	  Free_Pixmap (FRAME_X_DISPLAY (f), img->mask);
! 	  img->mask = NO_PIXMAP;
! 	  img->background_transparent_valid = 0;
! 	}
! #ifdef HAVE_X_WINDOWS
!       if (img->mask_img)
! 	{
! 	  x_destroy_x_image (img->mask_img);
! 	  img->mask_img = NULL;
! 	  img->background_transparent_valid = 0;
! 	}
! #endif
      }
  
!   if ((flags & CLEAR_IMAGE_COLORS) && img->ncolors)
      {
        /* W32_TODO: color table support.  */
  #ifdef HAVE_X_WINDOWS
***************
*** 1302,1308 ****
  x_clear_image (struct frame *f, struct image *img)
  {
    block_input ();
!   x_clear_image_1 (f, img, 1, 1, 1);
    unblock_input ();
  }
  
--- 1312,1319 ----
  x_clear_image (struct frame *f, struct image *img)
  {
    block_input ();
!   x_clear_image_1 (f, img,
! 		   CLEAR_IMAGE_PIXMAP | CLEAR_IMAGE_MASK | CLEAR_IMAGE_COLORS);
    unblock_input ();
  }
  
***************
*** 1633,1642 ****
  		x_build_heuristic_mask (f, img, XCDR (mask));
  	    }
  	  else if (NILP (mask) && found_p && img->mask)
! 	    {
! 	      Free_Pixmap (FRAME_X_DISPLAY (f), img->mask);
! 	      img->mask = NO_PIXMAP;
! 	    }
  	}
  
  
--- 1644,1650 ----
  		x_build_heuristic_mask (f, img, XCDR (mask));
  	    }
  	  else if (NILP (mask) && found_p && img->mask)
! 	    x_clear_image_1 (f, img, CLEAR_IMAGE_MASK);
  	}
  
  
***************
*** 2094,2099 ****
--- 2102,2222 ----
  #endif
  }
  
+ /* Thin wrapper for x_create_x_image_and_pixmap, so that it matches
+    with image_put_x_image.  */
+ 
+ static bool
+ image_create_x_image_and_pixmap (struct frame *f, struct image *img,
+ 				 int width, int height, int depth,
+ 				 XImagePtr *ximg, bool mask_p)
+ {
+   return x_create_x_image_and_pixmap (f, width, height, depth, ximg,
+ 				      !mask_p ? &img->pixmap : &img->mask);
+ }
+ 
+ /* Put X image XIMG into image IMG on frame F, as a mask if and only
+    if MASK_P.  On X, this simply records XIMG on a member of IMG, so
+    it can be put into the pixmap afterwards via image_sync_to_pixmaps.
+    On the other platforms, it puts XIMG into the pixmap, then frees
+    the X image and its buffer.  */
+ 
+ static void
+ image_put_x_image (struct frame *f, struct image *img, XImagePtr ximg,
+ 		   bool mask_p)
+ {
+ #ifdef HAVE_X_WINDOWS
+   if (!mask_p)
+     img->ximg = ximg;
+   else
+     img->mask_img = ximg;
+ #else
+   x_put_x_image (f, ximg, !mask_p ? img->pixmap : img->mask,
+ 		 img->width, img->height);
+   x_destroy_x_image (ximg);
+ #endif
+ }
+ 
+ #ifdef HAVE_X_WINDOWS
+ /* Put the X images recorded in IMG on frame F into pixmaps, then free
+    the X images and their buffer.  */
+ 
+ static void
+ image_sync_to_pixmaps (struct frame *f, struct image *img)
+ {
+   if (img->ximg)
+     {
+       x_put_x_image (f, img->ximg, img->pixmap, img->width, img->height);
+       x_destroy_x_image (img->ximg);
+       img->ximg = NULL;
+     }
+   if (img->mask_img)
+     {
+       x_put_x_image (f, img->mask_img, img->mask, img->width, img->height);
+       x_destroy_x_image (img->mask_img);
+       img->mask_img = NULL;
+     }
+ }
+ #endif
+ 
+ #ifdef HAVE_NTGUI
+ /* Create a memory device context for IMG on frame F.  It stores the
+    currently selected GDI object into *PREV for future restoration by
+    image_unget_x_image_or_dc.  */
+ 
+ static XImagePtr_or_DC
+ image_get_x_image_or_dc (struct frame *f, struct image *img, bool mask_p,
+ 			 HGDIOBJ *prev)
+ {
+   HDC frame_dc = get_frame_dc (f);
+   XImagePtr_or_DC ximg = CreateCompatibleDC (frame_dc);
+ 
+   release_frame_dc (f, frame_dc);
+   *prev = SelectObject (ximg, !mask_p ? img->pixmap : img->mask);
+ }
+ 
+ static void image_unget_x_image_or_dc (XImagePtr_or_DC ximg, HGDIOBJ prev)
+ {
+   SelectObject (ximg, prev);
+   DeleteDC (ximg);
+ }
+ #else  /* !HAVE_NTGUI */
+ /* Get the X image for IMG on frame F.  */
+ 
+ static XImagePtr
+ image_get_x_image (struct frame *f, struct image *img, bool mask_p)
+ {
+ #ifdef HAVE_X_WINDOWS
+   if (!mask_p)
+     {
+       if (img->ximg == NULL)
+ 	img->ximg = XGetImage (FRAME_X_DISPLAY (f), img->pixmap,
+ 			       0, 0, img->width, img->height, ~0, ZPixmap);
+       return img->ximg;
+     }
+   else
+     {
+       if (img->mask_img == NULL)
+ 	img->mask_img = XGetImage (FRAME_X_DISPLAY (f), img->mask,
+ 				   0, 0, img->width, img->height, ~0, ZPixmap);
+       return img->mask_img;
+     }
+ #elif defined (HAVE_NS)
+   XImagePtr pixmap = !mask_p ? img->pixmap : img->mask;
+ 
+   ns_retain_object (pixmap);
+   return pixmap;
+ #endif
+ }
+ 
+ static void image_unget_x_image (XImagePtr ximg)
+ {
+ #ifdef HAVE_X_WINDOWS
+ #elif defined (HAVE_NS)
+   ns_release_object (ximg);
+ #endif
+ }
+ #endif	/* !HAVE_NTGUI */
+ 
  \f
  /***********************************************************************
  			      File Handling
***************
*** 3461,3469 ****
  				  &xpm_image, &xpm_mask,
  				  &attrs);
  #else
!       rc = XpmReadFileToPixmap (FRAME_X_DISPLAY (f), FRAME_X_WINDOW (f),
! 				SSDATA (file), &img->pixmap, &img->mask,
! 				&attrs);
  #endif /* HAVE_NTGUI */
      }
    else
--- 3584,3592 ----
  				  &xpm_image, &xpm_mask,
  				  &attrs);
  #else
!       rc = XpmReadFileToImage (FRAME_X_DISPLAY (f), SSDATA (file),
! 			       &img->ximg, &img->mask_img,
! 			       &attrs);
  #endif /* HAVE_NTGUI */
      }
    else
***************
*** 3484,3496 ****
  					&xpm_image, &xpm_mask,
  					&attrs);
  #else
!       rc = XpmCreatePixmapFromBuffer (FRAME_X_DISPLAY (f), FRAME_X_WINDOW (f),
! 				      SSDATA (buffer),
! 				      &img->pixmap, &img->mask,
! 				      &attrs);
  #endif /* HAVE_NTGUI */
      }
  
    if (rc == XpmSuccess)
      {
  #if defined (COLOR_TABLE_SUPPORT) && defined (ALLOC_XPM_COLORS)
--- 3607,3644 ----
  					&xpm_image, &xpm_mask,
  					&attrs);
  #else
!       rc = XpmCreateImageFromBuffer (FRAME_X_DISPLAY (f), SSDATA (buffer),
! 				     &img->ximg, &img->mask_img,
! 				     &attrs);
  #endif /* HAVE_NTGUI */
      }
  
+ #ifdef HAVE_X_WINDOWS
+   if (rc == XpmSuccess)
+     {
+       img->pixmap = XCreatePixmap (FRAME_X_DISPLAY (f), FRAME_X_WINDOW (f),
+ 				   img->ximg->width, img->ximg->height,
+ 				   img->ximg->depth);
+       if (img->pixmap == NO_PIXMAP)
+ 	{
+ 	  x_clear_image (f, img);
+ 	  rc = XpmNoMemory;
+ 	}
+       else if (img->mask_img)
+ 	{
+ 	  img->mask = XCreatePixmap (FRAME_X_DISPLAY (f), FRAME_X_WINDOW (f),
+ 				     img->mask_img->width,
+ 				     img->mask_img->height,
+ 				     img->mask_img->depth);
+ 	  if (img->mask == NO_PIXMAP)
+ 	    {
+ 	      x_clear_image (f, img);
+ 	      rc = XpmNoMemory;
+ 	    }
+ 	}
+     }
+ #endif
+ 
    if (rc == XpmSuccess)
      {
  #if defined (COLOR_TABLE_SUPPORT) && defined (ALLOC_XPM_COLORS)
***************
*** 3847,3857 ****
        goto failure;
      }
  
!   if (!x_create_x_image_and_pixmap (f, width, height, 0,
! 				    &ximg, &img->pixmap)
  #ifndef HAVE_NS
!       || !x_create_x_image_and_pixmap (f, width, height, 1,
! 				       &mask_img, &img->mask)
  #endif
        )
      {
--- 3995,4004 ----
        goto failure;
      }
  
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0)
  #ifndef HAVE_NS
!       || !image_create_x_image_and_pixmap (f, img, width, height, 1,
! 					   &mask_img, 1)
  #endif
        )
      {
***************
*** 3986,3993 ****
    if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
      IMAGE_BACKGROUND (img, f, ximg);
  
!   x_put_x_image (f, ximg, img->pixmap, width, height);
!   x_destroy_x_image (ximg);
  #ifndef HAVE_NS
    if (have_mask)
      {
--- 4133,4139 ----
    if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
      IMAGE_BACKGROUND (img, f, ximg);
  
!   image_put_x_image (f, img, ximg, 0);
  #ifndef HAVE_NS
    if (have_mask)
      {
***************
*** 3995,4008 ****
  	 mask handy.  */
        image_background_transparent (img, f, mask_img);
  
!       x_put_x_image (f, mask_img, img->mask, width, height);
!       x_destroy_x_image (mask_img);
      }
    else
      {
        x_destroy_x_image (mask_img);
!       Free_Pixmap (FRAME_X_DISPLAY (f), img->mask);
!       img->mask = NO_PIXMAP;
      }
  #endif
    return 1;
--- 4141,4152 ----
  	 mask handy.  */
        image_background_transparent (img, f, mask_img);
  
!       image_put_x_image (f, img, mask_img, 1);
      }
    else
      {
        x_destroy_x_image (mask_img);
!       x_clear_image_1 (f, img, CLEAR_IMAGE_MASK);
      }
  #endif
    return 1;
***************
*** 4400,4416 ****
      memory_full (SIZE_MAX);
    colors = xmalloc (sizeof *colors * img->width * img->height);
  
! #ifndef HAVE_NTGUI
!   /* Get the X image IMG->pixmap.  */
!   ximg = XGetImage (FRAME_X_DISPLAY (f), img->pixmap,
! 		    0, 0, img->width, img->height, ~0, ZPixmap);
! #else
!   /* Load the image into a memory device context.  */
!   hdc = get_frame_dc (f);
!   ximg = CreateCompatibleDC (hdc);
!   release_frame_dc (f, hdc);
!   prev = SelectObject (ximg, img->pixmap);
! #endif /* HAVE_NTGUI */
  
    /* Fill the `pixel' members of the XColor array.  I wished there
       were an easy and portable way to circumvent XGetPixel.  */
--- 4544,4551 ----
      memory_full (SIZE_MAX);
    colors = xmalloc (sizeof *colors * img->width * img->height);
  
!   /* Get the X image or create a memory device context for IMG. */
!   ximg = image_get_x_image_or_dc (f, img, 0, &prev);
  
    /* Fill the `pixel' members of the XColor array.  I wished there
       were an easy and portable way to circumvent XGetPixel.  */
***************
*** 4440,4446 ****
  #endif /* HAVE_X_WINDOWS */
      }
  
!   Destroy_Image (ximg, prev);
  
    return colors;
  }
--- 4575,4581 ----
  #endif /* HAVE_X_WINDOWS */
      }
  
!   image_unget_x_image_or_dc (ximg, prev);
  
    return colors;
  }
***************
*** 4505,4512 ****
  
    init_color_table ();
  
!   x_create_x_image_and_pixmap (f, img->width, img->height, 0,
! 			       &oimg, &pixmap);
    p = colors;
    for (y = 0; y < img->height; ++y)
      for (x = 0; x < img->width; ++x, ++p)
--- 4640,4648 ----
  
    init_color_table ();
  
!   x_clear_image_1 (f, img, CLEAR_IMAGE_PIXMAP | CLEAR_IMAGE_COLORS);
!   image_create_x_image_and_pixmap (f, img, img->width, img->height, 0,
! 				   &oimg, 0);
    p = colors;
    for (y = 0; y < img->height; ++y)
      for (x = 0; x < img->width; ++x, ++p)
***************
*** 4517,4527 ****
        }
  
    xfree (colors);
-   x_clear_image_1 (f, img, 1, 0, 1);
  
!   x_put_x_image (f, oimg, pixmap, img->width, img->height);
!   x_destroy_x_image (oimg);
!   img->pixmap = pixmap;
  #ifdef COLOR_TABLE_SUPPORT
    img->colors = colors_in_color_table (&img->ncolors);
    free_color_table ();
--- 4653,4660 ----
        }
  
    xfree (colors);
  
!   image_put_x_image (f, img, oimg, 0);
  #ifdef COLOR_TABLE_SUPPORT
    img->colors = colors_in_color_table (&img->ncolors);
    free_color_table ();
***************
*** 4706,4712 ****
  #define MaskForeground(f)  WHITE_PIX_DEFAULT (f)
  
        Display *dpy = FRAME_X_DISPLAY (f);
!       GC gc = XCreateGC (dpy, img->pixmap, 0, NULL);
        XSetForeground (dpy, gc, BLACK_PIX_DEFAULT (f));
        XDrawLine (dpy, img->pixmap, gc, 0, 0,
  		 img->width - 1, img->height - 1);
--- 4839,4848 ----
  #define MaskForeground(f)  WHITE_PIX_DEFAULT (f)
  
        Display *dpy = FRAME_X_DISPLAY (f);
!       GC gc;
! 
!       image_sync_to_pixmaps (f, img);
!       gc = XCreateGC (dpy, img->pixmap, 0, NULL);
        XSetForeground (dpy, gc, BLACK_PIX_DEFAULT (f));
        XDrawLine (dpy, img->pixmap, gc, 0, 0,
  		 img->width - 1, img->height - 1);
***************
*** 4781,4817 ****
    unsigned long bg = 0;
  
    if (img->mask)
!     {
!       Free_Pixmap (FRAME_X_DISPLAY (f), img->mask);
!       img->mask = NO_PIXMAP;
!       img->background_transparent_valid = 0;
!     }
  
  #ifndef HAVE_NTGUI
  #ifndef HAVE_NS
    /* Create an image and pixmap serving as mask.  */
!   rc = x_create_x_image_and_pixmap (f, img->width, img->height, 1,
! 				    &mask_img, &img->mask);
    if (!rc)
      return;
  #endif /* !HAVE_NS */
- 
-   /* Get the X image of IMG->pixmap.  */
-   ximg = XGetImage (FRAME_X_DISPLAY (f), img->pixmap, 0, 0,
- 		    img->width, img->height,
- 		    ~0, ZPixmap);
  #else
    /* Create the bit array serving as mask.  */
    row_width = (img->width + 7) / 8;
    mask_img = xzalloc (row_width * img->height);
- 
-   /* Create a memory device context for IMG->pixmap.  */
-   frame_dc = get_frame_dc (f);
-   ximg = CreateCompatibleDC (frame_dc);
-   release_frame_dc (f, frame_dc);
-   prev = SelectObject (ximg, img->pixmap);
  #endif /* HAVE_NTGUI */
  
    /* Determine the background color of ximg.  If HOW is `(R G B)'
       take that as color.  Otherwise, use the image's background color. */
    use_img_background = 1;
--- 4917,4941 ----
    unsigned long bg = 0;
  
    if (img->mask)
!     x_clear_image_1 (f, img, CLEAR_IMAGE_MASK);
  
  #ifndef HAVE_NTGUI
  #ifndef HAVE_NS
    /* Create an image and pixmap serving as mask.  */
!   rc = image_create_x_image_and_pixmap (f, img, img->width, img->height, 1,
! 					&mask_img, 1);
    if (!rc)
      return;
  #endif /* !HAVE_NS */
  #else
    /* Create the bit array serving as mask.  */
    row_width = (img->width + 7) / 8;
    mask_img = xzalloc (row_width * img->height);
  #endif /* HAVE_NTGUI */
  
+   /* Get the X image or create a memory device context for IMG.  */
+   ximg = image_get_x_image_or_dc (f, img, 0, &prev);
+ 
    /* Determine the background color of ximg.  If HOW is `(R G B)'
       take that as color.  Otherwise, use the image's background color. */
    use_img_background = 1;
***************
*** 4858,4866 ****
    /* Fill in the background_transparent field while we have the mask handy. */
    image_background_transparent (img, f, mask_img);
  
!   /* Put mask_img into img->mask.  */
!   x_put_x_image (f, mask_img, img->mask, img->width, img->height);
!   x_destroy_x_image (mask_img);
  #endif /* !HAVE_NS */
  #else
    for (y = 0; y < img->height; ++y)
--- 4982,4989 ----
    /* Fill in the background_transparent field while we have the mask handy. */
    image_background_transparent (img, f, mask_img);
  
!   /* Put mask_img into the image.  */
!   image_put_x_image (f, img, mask_img, 1);
  #endif /* !HAVE_NS */
  #else
    for (y = 0; y < img->height; ++y)
***************
*** 4882,4888 ****
    xfree (mask_img);
  #endif /* HAVE_NTGUI */
  
!   Destroy_Image (ximg, prev);
  }
  
  \f
--- 5005,5011 ----
    xfree (mask_img);
  #endif /* HAVE_NTGUI */
  
!   image_unget_x_image_or_dc (ximg, prev);
  }
  
  \f
***************
*** 5110,5117 ****
        goto error;
      }
  
!   if (!x_create_x_image_and_pixmap (f, width, height, 0,
! 				    &ximg, &img->pixmap))
      goto error;
  
    /* Initialize the color hash table.  */
--- 5233,5239 ----
        goto error;
      }
  
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
      goto error;
  
    /* Initialize the color hash table.  */
***************
*** 5248,5256 ****
      /* Casting avoids a GCC warning.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put the image into a pixmap.  */
!   x_put_x_image (f, ximg, img->pixmap, width, height);
!   x_destroy_x_image (ximg);
  
    /* X and W32 versions did it here, MAC version above.  ++kfs
       img->width = width;
--- 5370,5377 ----
      /* Casting avoids a GCC warning.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
  
    /* X and W32 versions did it here, MAC version above.  ++kfs
       img->width = width;
***************
*** 5688,5695 ****
  
    /* Create the X image and pixmap now, so that the work below can be
       omitted if the image is too large for X.  */
!   if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg,
! 				    &img->pixmap))
      goto error;
  
    /* If image contains simply transparency data, we prefer to
--- 5809,5815 ----
  
    /* Create the X image and pixmap now, so that the work below can be
       omitted if the image is too large for X.  */
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
      goto error;
  
    /* If image contains simply transparency data, we prefer to
***************
*** 5801,5812 ****
       contains an alpha channel.  */
    if (channels == 4
        && !transparent_p
!       && !x_create_x_image_and_pixmap (f, width, height, 1,
! 				       &mask_img, &img->mask))
      {
        x_destroy_x_image (ximg);
!       Free_Pixmap (FRAME_X_DISPLAY (f), img->pixmap);
!       img->pixmap = NO_PIXMAP;
        goto error;
      }
  
--- 5921,5931 ----
       contains an alpha channel.  */
    if (channels == 4
        && !transparent_p
!       && !image_create_x_image_and_pixmap (f, img, width, height, 1,
! 					   &mask_img, 1))
      {
        x_destroy_x_image (ximg);
!       x_clear_image_1 (f, img, CLEAR_IMAGE_PIXMAP);
        goto error;
      }
  
***************
*** 5880,5888 ****
       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);
  
    /* Same for the mask.  */
    if (mask_img)
--- 5999,6006 ----
       Casting avoids a GCC warning.  */
    IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
  
    /* Same for the mask.  */
    if (mask_img)
***************
*** 5891,5898 ****
  	 mask handy.  Casting avoids a GCC warning.  */
        image_background_transparent (img, f, (XImagePtr_or_DC)mask_img);
  
!       x_put_x_image (f, mask_img, img->mask, img->width, img->height);
!       x_destroy_x_image (mask_img);
      }
  
    return 1;
--- 6009,6015 ----
  	 mask handy.  Casting avoids a GCC warning.  */
        image_background_transparent (img, f, (XImagePtr_or_DC)mask_img);
  
!       image_put_x_image (f, img, mask_img, 1);
      }
  
    return 1;
***************
*** 6429,6435 ****
      }
  
    /* Create X image and pixmap.  */
!   if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg, &img->pixmap))
      {
        mgr->failure_code = MY_JPEG_CANNOT_CREATE_X;
        sys_longjmp (mgr->setjmp_buffer, 1);
--- 6546,6552 ----
      }
  
    /* Create X image and pixmap.  */
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
      {
        mgr->failure_code = MY_JPEG_CANNOT_CREATE_X;
        sys_longjmp (mgr->setjmp_buffer, 1);
***************
*** 6496,6504 ****
      /* Casting avoids a GCC warning.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put the image into the pixmap.  */
!   x_put_x_image (f, ximg, img->pixmap, width, height);
!   x_destroy_x_image (ximg);
    return 1;
  }
  
--- 6613,6620 ----
      /* Casting avoids a GCC warning.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
    return 1;
  }
  
***************
*** 6895,6902 ****
  
    /* Create the X image and pixmap.  */
    if (! (height <= min (PTRDIFF_MAX, SIZE_MAX) / sizeof *buf / width
! 	 && x_create_x_image_and_pixmap (f, width, height, 0,
! 					 &ximg, &img->pixmap)))
      {
        fn_TIFFClose (tiff);
        return 0;
--- 7011,7018 ----
  
    /* Create the X image and pixmap.  */
    if (! (height <= min (PTRDIFF_MAX, SIZE_MAX) / sizeof *buf / width
! 	 && image_create_x_image_and_pixmap (f, img, width, height, 0,
! 					     &ximg, 0)))
      {
        fn_TIFFClose (tiff);
        return 0;
***************
*** 6955,6963 ****
      /* Casting avoids a GCC warning on W32.  */
      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);
    xfree (buf);
  
    return 1;
--- 7071,7078 ----
      /* Casting avoids a GCC warning on W32.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
    xfree (buf);
  
    return 1;
***************
*** 7285,7291 ****
      }
  
    /* Create the X image and pixmap.  */
!   if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg, &img->pixmap))
      {
        fn_DGifCloseFile (gif);
        return 0;
--- 7400,7406 ----
      }
  
    /* Create the X image and pixmap.  */
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
      {
        fn_DGifCloseFile (gif);
        return 0;
***************
*** 7469,7477 ****
      /* 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);
  
    return 1;
  }
--- 7584,7591 ----
      /* Casting avoids a GCC warning.  */
      IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
  
    return 1;
  }
***************
*** 7909,7916 ****
        int imagedepth = 24; /*MagickGetImageDepth(image_wand);*/
        const char *exportdepth = imagedepth <= 8 ? "I" : "BGRP"; /*"RGBP";*/
        /* Try to create a x pixmap to hold the imagemagick pixmap.  */
!       if (!x_create_x_image_and_pixmap (f, width, height, imagedepth,
!                                         &ximg, &img->pixmap))
  	{
  #ifdef COLOR_TABLE_SUPPORT
  	  free_color_table ();
--- 8023,8030 ----
        int imagedepth = 24; /*MagickGetImageDepth(image_wand);*/
        const char *exportdepth = imagedepth <= 8 ? "I" : "BGRP"; /*"RGBP";*/
        /* Try to create a x pixmap to hold the imagemagick pixmap.  */
!       if (!image_create_x_image_and_pixmap (f, img, width, height, imagedepth,
! 					    &ximg, 0))
  	{
  #ifdef COLOR_TABLE_SUPPORT
  	  free_color_table ();
***************
*** 7948,7955 ****
        size_t image_height;
  
        /* 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))
          {
  #ifdef COLOR_TABLE_SUPPORT
  	  free_color_table ();
--- 8062,8069 ----
        size_t image_height;
  
        /* Try to create a x pixmap to hold the imagemagick pixmap.  */
!       if (!image_create_x_image_and_pixmap (f, img, width, height, 0,
! 					    &ximg, 0))
          {
  #ifdef COLOR_TABLE_SUPPORT
  	  free_color_table ();
***************
*** 8003,8012 ****
    img->width  = width;
    img->height = height;
  
!   /* 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);
  
    /* Final cleanup. image_wand should be the only resource left. */
    DestroyMagickWand (image_wand);
--- 8117,8124 ----
    img->width  = width;
    img->height = height;
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
  
    /* Final cleanup. image_wand should be the only resource left. */
    DestroyMagickWand (image_wand);
***************
*** 8400,8406 ****
    eassert (fn_gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
  
    /* Try to create a x pixmap to hold the svg pixmap.  */
!   if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg, &img->pixmap))
      {
        fn_g_object_unref (pixbuf);
        return 0;
--- 8512,8518 ----
    eassert (fn_gdk_pixbuf_get_bits_per_sample (pixbuf) == 8);
  
    /* Try to create a x pixmap to hold the svg pixmap.  */
!   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
      {
        fn_g_object_unref (pixbuf);
        return 0;
***************
*** 8475,8484 ****
       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);
  
    return 1;
  
--- 8587,8594 ----
       Casting avoids a GCC warning.  */
    IMAGE_BACKGROUND (img, f, (XImagePtr_or_DC)ximg);
  
!   /* Put ximg into the image.  */
!   image_put_x_image (f, img, ximg, 0);
  
    return 1;
  

Re: image-size

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 26 Jun 2013 17:25:06 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> Could you try this?  It doesn't defer the allocation of Pixmap
> actually, but defers image data transfer by XPutImage (and also by
> XGetImage in the case of postprocessing) until the actual display
> happens.

Sorry.  I forgot to include the dispextern.h part.

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

=== modified file 'src/dispextern.h'
*** src/dispextern.h	2013-06-17 06:03:19 +0000
--- src/dispextern.h	2013-06-26 10:35:52 +0000
***************
*** 2870,2875 ****
--- 2870,2883 ----
    /* Pixmaps of the image.  */
    Pixmap pixmap, mask;
  
+ #ifdef HAVE_X_WINDOWS
+   /* X images of the image, corresponding to the above Pixmaps.
+      Non-NULL means it and its Pixmap counterpart may be out of sync
+      and the latter is outdated.  NULL means the X image has been
+      synchronized to Pixmap.  */
+   XImagePtr ximg, mask_img;
+ #endif
+ 
    /* Colors allocated for this image, if any.  Allocated via xmalloc.  */
    unsigned long *colors;
    int ncolors;

Re: image-size

[Lars Magne Ingebrigtsen]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> Could you try this?  It doesn't defer the allocation of Pixmap
> actually, but defers image data transfer by XPutImage (and also by
> XGetImage in the case of postprocessing) until the actual display
> happens.

Great!  This seems to work perfectly.  Calling `image-size' over ssh is
now as fast as calling it with a local X; the decoded image is cached;
and transferring the image to X is postponed until it's actually
displayed.  

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

Re: image-size

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
>
>> Could you try this?  It doesn't defer the allocation of Pixmap
>> actually, but defers image data transfer by XPutImage (and also by
>> XGetImage in the case of postprocessing) until the actual display
>> happens.
>
> Great!  This seems to work perfectly.  Calling `image-size' over ssh is
> now as fast as calling it with a local X; the decoded image is cached;
> and transferring the image to X is postponed until it's actually
> displayed.  

So I think this should be applied.  This is not a part of the code that
I'm extremely familiar with, but the patch seems fine to me.  Does
anybody have any objections to applying this?

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

Re: image-size

[Stefan Monnier]

> So I think this should be applied.  This is not a part of the code that
> I'm extremely familiar with, but the patch seems fine to me.  Does
> anybody have any objections to applying this?

Yamamoto can install it himself, if/when he thinks it's good enough.
I like the idea, but I don't know enough about these libraries to judge
the quality/reliability of the change.


        Stefan

Re: image-size

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 26 Jun 2013 14:04:57 +0200, Lars Magne Ingebrigtsen <larsi@gnus.org> said:

>> Could you try this?  It doesn't defer the allocation of Pixmap
>> actually, but defers image data transfer by XPutImage (and also by
>> XGetImage in the case of postprocessing) until the actual display
>> happens.

> Great!  This seems to work perfectly.  Calling `image-size' over ssh
> is now as fast as calling it with a local X; the decoded image is
> cached; and transferring the image to X is postponed until it's
> actually displayed.

Thanks for testing.  I've just installed a slightly different version
of the change.

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

Re: image-size

[Juanma Barranquero]

On Fri, Jun 28, 2013 at 4:46 AM, YAMAMOTO Mitsuharu
<mituharu@math.s.chiba-u.ac.jp> wrote:

> Thanks for testing.  I've just installed a slightly different version
> of the change.

image.c: In function 'image_get_x_image_or_dc':
image.c:2184:1: warning: no return statement in function returning
non-void [-Wreturn-type]
image.c: At top level:
image.c:2186:13: error: conflicting types for 'image_unget_x_image_or_dc'
image.c:429:13: note: previous declaration of
'image_unget_x_image_or_dc' was here

Re: image-size

[YAMAMOTO Mitsuharu]

>>>>> On Fri, 28 Jun 2013 05:12:07 +0200, Juanma Barranquero <lekktu@gmail.com> said:

>> Thanks for testing.  I've just installed a slightly different version
>> of the change.

> image.c: In function 'image_get_x_image_or_dc':
> image.c:2184:1: warning: no return statement in function returning
> non-void [-Wreturn-type]
> image.c: At top level:
> image.c:2186:13: error: conflicting types for 'image_unget_x_image_or_dc'
> image.c:429:13: note: previous declaration of
> 'image_unget_x_image_or_dc' was here

Sorry, they were in the W32-specific part I couldn't test myself.  I
think I've fixed them.

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