Re: Linking to ImageMagick by default

[Eli Zaretskii]

> Date: Fri, 28 Dec 2018 21:21:15 +0000
> From: Alan Third <address@hidden>
> Cc: address@hidden
> 
> 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?

I'm okay with saying that this causes a complete redisplay of FRAME
(or all frames if FRAME is t), but the bit about making the frames
garbaged is an implementation detail, so I don't think it belongs to
the doc string.

> > > +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 think with my suggestion you will still hold only one copy: the last
one whose size was calculated.  AFAIU, the same happens with your
implementation, you just hold the last size separately.

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

No, this is not a good idea, so you were right not to do that.

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

I see nothing wrong with a series of equality checks, we do that
elsewhere, albeit not in a loop.

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

To my eyes, consing a list that will be used only once is less
elegant, and will also cause more GC.  So maybe try the alternative,
and see if it works well.

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

Maybe I'm misreading the code, but it looks to me you only need
:scale, :width, and :height.  Am I missing something?

> At the moment it’s avoided simply by not using :type imagemagick.

If the caller might know whether resizing is needed, perhaps we should
pass an extra flag argument to that effect.

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

Definitely.

Thanks.