Re: Linking to ImageMagick by default

[Alan Third]

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

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

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

The docstring for image-flush looks like this:

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

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

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

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

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

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

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

You’re right. Thanks.

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

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

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

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

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

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

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