Re: [PATCH v2] Add native image scaling (bug#33587)

[Alan Third]

On Fri, Jan 04, 2019 at 04:31:41PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 2 Jan 2019 21:12:41 +0000
> > From: Alan Third <address@hidden>
> > Cc: address@hidden
> > 
> > I think this is the final version.
> 
> Thanks.  A few minor gotchas.

I regret touching this now. ;)

Do these look OK?

modified   doc/lispref/display.texi
@@ -5120,33 +5120,35 @@ Image Descriptors
 
 @item :max-width @var{max-width}, :max-height @var{max-height}
 The @code{:max-width} and @code{:max-height} keywords are used for
-scaling if the size of the image of the image exceeds these values.
-If @code{:width} is set it will have precedence over @code{max-width},
-and if @code{:height} is set it will have precedence over
+scaling if the size of the image exceeds these values.  If
address@hidden:width} is set, it will have precedence over @code{max-width},
+and if @code{:height} is set, it will have precedence over
 @code{max-height}, but you can otherwise mix these keywords as you
-wish.  @code{:max-width} and @code{:max-height} will always preserve
-the aspect ratio.
-
-If both @code{:width} and @code{:max-height} has been set (but
address@hidden:height} has not been set), then @code{:max-height} will have
-precedence.  The same is the case for the opposite combination: The
-``max'' keyword has precedence.  That is, if you have a 200x100 image
-and specify that @code{:width} should be 400 and @code{:max-height}
-should be 150, you'll end up with an image that is 300x150: Preserving
-the aspect ratio and not exceeding the ``max'' setting.  This
-combination of parameters is a useful way of saying ``display this
-image as large as possible, but no larger than the available display
-area''.
+wish.
+
+If both @code{:max-width} and @code{:height} are specified, but
address@hidden:width} is not, preserving the aspect ratio might require that
+width exceeds @code{:max-width}.  If this happens, scaling will use a
+smaller value for the height so as to preserve the aspect ratio while
+not exceeding @code{:max-width}.  Similarly when both
address@hidden:max-height} and @code{:width} are specified, but @code{:height}
+is not.  For example, if you have a 200x100 image and specify that
address@hidden:width} should be 400 and @code{:max-height} should be 150,
+you'll end up with an image that is 300x150: Preserving the aspect
+ratio and not exceeding the ``max'' setting.  This combination of
+parameters is a useful way of saying ``display this image as large as
+possible, but no larger than the available display area''.
 
 @item :scale @var{scale}
 This should be a number, where values higher than 1 means to increase
-the size, and lower means to decrease the size.  For instance, a value
-of 0.25 will make the image a quarter size of what it originally was.
-If the scaling makes the image larger than specified by
address@hidden:max-width} or @code{:max-height}, the resulting size will not
-exceed those two values.  If both @code{:scale} and
address@hidden:height}/@code{:width} are specified, the height/width will be
-adjusted by the specified scaling factor.
+the size, and lower means to decrease the size, by multiplying both
+the width and height.  For instance, a value of 0.25 will make the
+image a quarter size of what it originally was.  If the scaling makes
+the image larger than specified by @code{:max-width} or
address@hidden:max-height}, the resulting size will not exceed those two
+values.  If both @code{:scale} and @code{:height}/@code{:width} are
+specified, the height/width will be adjusted by the specified scaling
+factor.
 
 @item :index @var{frame}
 @xref{Multi-Frame Images}.

> > address@hidden:height} has not been set), then @code{:max-height} will have
> > +precedence.  The same is the case for the opposite combination: The
> > +``max'' keyword has precedence.
> 
> This confused me until I've read the example.  having read the
> example, I suggest to describe this differently:
> 
>   If both @code{:max-width} and @code{:height} are specified, but
>   @code{:width} is not, preserving the aspect ratio might require that
>   width exceeds @code{:max-width}.  If this happens, scaling will use a
>   smaller value for the height so as to preserve the aspect ratio while
>   not exceeding @code{:max-width}.  Similarly when both
>   @code{:max-height} and @code{:width} are specified, but @code{:height}
>   is not.

I’ve put that in. FWIW I don’t understand why this behaviour was
chosen, it seems to me that max-width and max-height should be exactly
that, the max width and height. I don’t understand why they can be
over‐ridden.

Too late to do anything about it now, I suppose.

> > address@hidden image-scaling-p &optional frame
> > +This function returns @code{t} if @var{frame} supports image scaling.
> > address@hidden @code{nil} or omitted means to use the selected frame
> > +(@pxref{Input Focus}).
> > +
> > +If image scaling is not supported, @code{:width}, @code{:height},
> > address@hidden:scale}, @code{:max-width} and @code{:max-height} will only be
> > +usable through ImageMagick, if available (@pxref{ImageMagick Images}).
> > address@hidden defun
> 
> Shouldn't image-scaling-p return non-nil if ImageMagick is available?
> I think that would allow a simpler Lisp code.

The difficulty is in knowing whether you need to set :type to
ImageMagick or not.

What we really don’t want to do is just return t for both native and
ImageMagick, as you then have no way of deciding to go with native
over ImageMagick, because you can’t tell the difference between
native‐and‐ImageMagick, or just ImageMagick.

I did think that I could return t if native is available, and if it’s
not, but ImageMagick is, return 'imagemagick.

Or perhaps return a list of scaling capable backends

    '(native imagemagick)

There are other ways of detecting ImageMagick, but no other way of
detecting native scaling, other than just trying it I suppose.

(Another option would be to automatically fall‐back to ImageMagick if
required, but given that we’re doing this because people are concerned
about ImageMagick’s security record, I doubt making it’s use automatic
would appeal.)
-- 
Alan Third