Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Thu, 27 Dec 2018 13:11:05 +0000
> From: Alan Third <address@hidden>
> Cc: address@hidden
> 
> I’ve now run into another issue. Now that I’m stripping the image spec
> down to the basics when I load an image in image mode and then use +
> or - to resize, it doesn’t redraw.
> 
> It redraws correctly if you cause a redraw, by scrolling the window,
> or making an animated gif animate, or similar.

Does it redraw if you just type "M-x" and nothing else?

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

> Patch attached.

Thanks, a couple of comments below.

> +/* 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.

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

> +      /* 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?

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?

> +/* 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.