bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hello,

While working with images, I found what seems an issue to me with
`create-image' which unconditionally set the :scale image property to
'default' when not specified, ignoring the value of the option
`image-scaling-factor'.

Here is an illustration:

(let ((image-scaling-factor 1.0))
    (image-size
     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
     t))
=> (63 . 63)

(let ((image-scaling-factor 2.0))
    (image-size
     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
     t))
=> (63 . 63)

(image-size
   (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
   t)
=> (48 . 48)

(image-size
   (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
   t)
=> (96 . 96)

You can replace `image-size' with `insert-image' and observe the same.

Here is a simple patch which fix the issue for me:

diff --git a/lisp/image.el b/lisp/image.el
index ce97eeb3ca1..2c1e865c336 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -536,7 +536,9 @@ create-image
                           file-or-data)
                     (and (not (plist-get props :scale))
                          ;; Add default scaling.
-                        (list :scale 'default))
+                        (list :scale (if (numberp image-scaling-factor)
+                                         image-scaling-factor
+                                       'default)))
  	           props)))
        ;; Add default smoothing.
        (unless (plist-member props :transform-smoothing)


Thanks!


In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
  3.24.43, cairo version 1.18.0) of 2024-12-02
Repository revision: 8cd4ab7abde87ac04e05442196b4646ab46df9a7
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 40 (KDE Plasma)

Configured using:
  'configure --with-native-compilation=no
  PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
   value of $LC_TIME: fr_FR.utf8
   value of $LANG: fr_FR.UTF-8
   locale-coding-system: utf-8-unix

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hello,

After more investigation I found the issue is not in image.el, but is due
to image caching.

If I flush the image cache when I change the value of `image-scaling-factor'
the result is as expected:

(let* ((image-scaling-factor 2.0)
        (image (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))))
   (image-flush image t)
   (image-size
    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
    t))
(96 . 96)

;; Image is used from cache, size is not updated.
(let* ((image-scaling-factor 3.0)
        (image (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))))
   ;; (image-flush image t)
   (image-size
    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
    t))
(96 . 96)

;; Image not in cache, size is updated.
(let* ((image-scaling-factor 3.0)
        (image (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))))
   (image-flush image t)
   (image-size
    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
    t))
(144 . 144)

Thanks!

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> Date: Sat, 7 Dec 2024 13:13:58 +0100
> From:  David Ponce via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> While working with images, I found what seems an issue to me with
> `create-image' which unconditionally set the :scale image property to
> 'default' when not specified, ignoring the value of the option
> `image-scaling-factor'.
> 
> Here is an illustration:
> 
> (let ((image-scaling-factor 1.0))
>     (image-size
>      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>      t))
> => (63 . 63)
> 
> (let ((image-scaling-factor 2.0))
>     (image-size
>      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>      t))
> => (63 . 63)
> 
> (image-size
>    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
>    t)
> => (48 . 48)
> 
> (image-size
>    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
>    t)
> => (96 . 96)
> 
> You can replace `image-size' with `insert-image' and observe the same.
> 
> Here is a simple patch which fix the issue for me:
> 
> diff --git a/lisp/image.el b/lisp/image.el
> index ce97eeb3ca1..2c1e865c336 100644
> --- a/lisp/image.el
> +++ b/lisp/image.el
> @@ -536,7 +536,9 @@ create-image
>                            file-or-data)
>                      (and (not (plist-get props :scale))
>                           ;; Add default scaling.
> -                        (list :scale 'default))
> +                        (list :scale (if (numberp image-scaling-factor)
> +                                         image-scaling-factor
> +                                       'default)))
>   	           props)))
>         ;; Add default smoothing.
>         (unless (plist-member props :transform-smoothing)

AFAIU, this is supposed to be taken care of in image.c.

Alan, any ideas why this doesn't seem to work?

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Alan Third]

On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 7 Dec 2024 13:13:58 +0100
> > From:  David Ponce via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > While working with images, I found what seems an issue to me with
> > `create-image' which unconditionally set the :scale image property to
> > 'default' when not specified, ignoring the value of the option
> > `image-scaling-factor'.
> > 
> > Here is an illustration:
> > 
> > (let ((image-scaling-factor 1.0))
> >     (image-size
> >      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> >      t))
> > => (63 . 63)
> > 
> > (let ((image-scaling-factor 2.0))
> >     (image-size
> >      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> >      t))
> > => (63 . 63)
> > 
> > (image-size
> >    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
> >    t)
> > => (48 . 48)
> > 
> > (image-size
> >    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
> >    t)
> > => (96 . 96)
> > 
> > You can replace `image-size' with `insert-image' and observe the same.
> > 
> > Here is a simple patch which fix the issue for me:
> > 
> > diff --git a/lisp/image.el b/lisp/image.el
> > index ce97eeb3ca1..2c1e865c336 100644
> > --- a/lisp/image.el
> > +++ b/lisp/image.el
> > @@ -536,7 +536,9 @@ create-image
> >                            file-or-data)
> >                      (and (not (plist-get props :scale))
> >                           ;; Add default scaling.
> > -                        (list :scale 'default))
> > +                        (list :scale (if (numberp image-scaling-factor)
> > +                                         image-scaling-factor
> > +                                       'default)))
> >   	           props)))
> >         ;; Add default smoothing.
> >         (unless (plist-member props :transform-smoothing)
> 
> AFAIU, this is supposed to be taken care of in image.c.
> 
> Alan, any ideas why this doesn't seem to work?

It's because the image spec doesn't change so the image is pulled from
the cache each time.

Flushing the image between calls to image-size fixes it:

    (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))

I'm not sure what the solution is here. My feeling is that
image-scaling-factor isn't intended as something you set for each
image as you load it, it's a set-and-forget setting, so perhaps we
just need to document that in order for it to take effect the image
cache needs to be flushed.

Alternatively we make the image cache aware of it.

Perhaps we can flush the cache automatically when it changes? That
might give unexpected results too.
-- 
Alan Third

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> Date: Sat, 7 Dec 2024 15:49:47 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: David Ponce <da_vid@orange.fr>, 74725@debbugs.gnu.org
> 
> On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
> > > Date: Sat, 7 Dec 2024 13:13:58 +0100
> > > From:  David Ponce via "Bug reports for GNU Emacs,
> > >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > > 
> > > While working with images, I found what seems an issue to me with
> > > `create-image' which unconditionally set the :scale image property to
> > > 'default' when not specified, ignoring the value of the option
> > > `image-scaling-factor'.
> > > 
> > > Here is an illustration:
> > > 
> > > (let ((image-scaling-factor 1.0))
> > >     (image-size
> > >      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> > >      t))
> > > => (63 . 63)
> > > 
> > > (let ((image-scaling-factor 2.0))
> > >     (image-size
> > >      (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> > >      t))
> > > => (63 . 63)
> > > 
> > > (image-size
> > >    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
> > >    t)
> > > => (48 . 48)
> > > 
> > > (image-size
> > >    (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
> > >    t)
> > > => (96 . 96)
> > > 
> > > You can replace `image-size' with `insert-image' and observe the same.
> > > 
> > > Here is a simple patch which fix the issue for me:
> > > 
> > > diff --git a/lisp/image.el b/lisp/image.el
> > > index ce97eeb3ca1..2c1e865c336 100644
> > > --- a/lisp/image.el
> > > +++ b/lisp/image.el
> > > @@ -536,7 +536,9 @@ create-image
> > >                            file-or-data)
> > >                      (and (not (plist-get props :scale))
> > >                           ;; Add default scaling.
> > > -                        (list :scale 'default))
> > > +                        (list :scale (if (numberp image-scaling-factor)
> > > +                                         image-scaling-factor
> > > +                                       'default)))
> > >   	           props)))
> > >         ;; Add default smoothing.
> > >         (unless (plist-member props :transform-smoothing)
> > 
> > AFAIU, this is supposed to be taken care of in image.c.
> > 
> > Alan, any ideas why this doesn't seem to work?
> 
> It's because the image spec doesn't change so the image is pulled from
> the cache each time.
> 
> Flushing the image between calls to image-size fixes it:
> 
>     (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))
> 
> I'm not sure what the solution is here. My feeling is that
> image-scaling-factor isn't intended as something you set for each
> image as you load it, it's a set-and-forget setting, so perhaps we
> just need to document that in order for it to take effect the image
> cache needs to be flushed.
> 
> Alternatively we make the image cache aware of it.
> 
> Perhaps we can flush the cache automatically when it changes? That
> might give unexpected results too.

I think recording the scale in the cache, and rejecting the cached
image if the scale doesn't match, will cause the least surprise.  But
I'm open to other opinions and suggestions.

Thanks.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2024-12-07 17:21, Eli Zaretskii wrote:
>> Date: Sat, 7 Dec 2024 15:49:47 +0000
>> From: Alan Third <alan@idiocy.org>
>> Cc: David Ponce <da_vid@orange.fr>, 74725@debbugs.gnu.org
>>
>> On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
>>>> Date: Sat, 7 Dec 2024 13:13:58 +0100
>>>> From:  David Ponce via "Bug reports for GNU Emacs,
>>>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>>
>>>> While working with images, I found what seems an issue to me with
>>>> `create-image' which unconditionally set the :scale image property to
>>>> 'default' when not specified, ignoring the value of the option
>>>> `image-scaling-factor'.
>>>>
>>>> Here is an illustration:
>>>>
>>>> (let ((image-scaling-factor 1.0))
>>>>      (image-size
>>>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>>>>       t))
>>>> => (63 . 63)
>>>>
>>>> (let ((image-scaling-factor 2.0))
>>>>      (image-size
>>>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>>>>       t))
>>>> => (63 . 63)
>>>>
>>>> (image-size
>>>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
>>>>     t)
>>>> => (48 . 48)
>>>>
>>>> (image-size
>>>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
>>>>     t)
>>>> => (96 . 96)
>>>>
>>>> You can replace `image-size' with `insert-image' and observe the same.
>>>>
>>>> Here is a simple patch which fix the issue for me:
>>>>
>>>> diff --git a/lisp/image.el b/lisp/image.el
>>>> index ce97eeb3ca1..2c1e865c336 100644
>>>> --- a/lisp/image.el
>>>> +++ b/lisp/image.el
>>>> @@ -536,7 +536,9 @@ create-image
>>>>                             file-or-data)
>>>>                       (and (not (plist-get props :scale))
>>>>                            ;; Add default scaling.
>>>> -                        (list :scale 'default))
>>>> +                        (list :scale (if (numberp image-scaling-factor)
>>>> +                                         image-scaling-factor
>>>> +                                       'default)))
>>>>    	           props)))
>>>>          ;; Add default smoothing.
>>>>          (unless (plist-member props :transform-smoothing)
>>>
>>> AFAIU, this is supposed to be taken care of in image.c.
>>>
>>> Alan, any ideas why this doesn't seem to work?
>>
>> It's because the image spec doesn't change so the image is pulled from
>> the cache each time.
>>
>> Flushing the image between calls to image-size fixes it:
>>
>>      (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))
>>
>> I'm not sure what the solution is here. My feeling is that
>> image-scaling-factor isn't intended as something you set for each
>> image as you load it, it's a set-and-forget setting, so perhaps we
>> just need to document that in order for it to take effect the image
>> cache needs to be flushed.
>>
>> Alternatively we make the image cache aware of it.
>>
>> Perhaps we can flush the cache automatically when it changes? That
>> might give unexpected results too.
> 
> I think recording the scale in the cache, and rejecting the cached
> image if the scale doesn't match, will cause the least surprise.  But
> I'm open to other opinions and suggestions.
> 

Seems like a good idea.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2024-12-07 16:49, Alan Third wrote:
> On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
>>> Date: Sat, 7 Dec 2024 13:13:58 +0100
>>> From:  David Ponce via "Bug reports for GNU Emacs,
>>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> While working with images, I found what seems an issue to me with
>>> `create-image' which unconditionally set the :scale image property to
>>> 'default' when not specified, ignoring the value of the option
>>> `image-scaling-factor'.
>>>
>>> Here is an illustration:
>>>
>>> (let ((image-scaling-factor 1.0))
>>>      (image-size
>>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>>>       t))
>>> => (63 . 63)
>>>
>>> (let ((image-scaling-factor 2.0))
>>>      (image-size
>>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>>>       t))
>>> => (63 . 63)
>>>
>>> (image-size
>>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
>>>     t)
>>> => (48 . 48)
>>>
>>> (image-size
>>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
>>>     t)
>>> => (96 . 96)
>>>
>>> You can replace `image-size' with `insert-image' and observe the same.
>>>
>>> Here is a simple patch which fix the issue for me:
>>>
>>> diff --git a/lisp/image.el b/lisp/image.el
>>> index ce97eeb3ca1..2c1e865c336 100644
>>> --- a/lisp/image.el
>>> +++ b/lisp/image.el
>>> @@ -536,7 +536,9 @@ create-image
>>>                             file-or-data)
>>>                       (and (not (plist-get props :scale))
>>>                            ;; Add default scaling.
>>> -                        (list :scale 'default))
>>> +                        (list :scale (if (numberp image-scaling-factor)
>>> +                                         image-scaling-factor
>>> +                                       'default)))
>>>    	           props)))
>>>          ;; Add default smoothing.
>>>          (unless (plist-member props :transform-smoothing)
>>
>> AFAIU, this is supposed to be taken care of in image.c.
>>
>> Alan, any ideas why this doesn't seem to work?
> 
> It's because the image spec doesn't change so the image is pulled from
> the cache each time.
> 
> Flushing the image between calls to image-size fixes it:
> 
>      (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))
> 
> I'm not sure what the solution is here. My feeling is that
> image-scaling-factor isn't intended as something you set for each
> image as you load it, it's a set-and-forget setting, so perhaps we
> just need to document that in order for it to take effect the image
> cache needs to be flushed.
> 
> Alternatively we make the image cache aware of it.
> 
> Perhaps we can flush the cache automatically when it changes? That
> might give unexpected results too.

This is exactly what I also observe.
It seems due to this change:

author	Po Lu <luangruo@yahoo.com>	2024-06-03 16:34:51 +0800
committer	Po Lu <luangruo@yahoo.com>	2024-06-03 16:36:29 +0800
commit	56376585134d627f96c71b7b063ec51548d3ad3f (patch)

Which replaced

-                        (list :scale
-                              (image-compute-scaling-factor
-                               image-scaling-factor)))

By this

+                        (list :scale 'default))

In create-image.

With the side effect that the image spec don't change when the scaling
factor changes, so the same cached image in always used.


Not sure either what a solution could be.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> Date: Sat, 7 Dec 2024 17:27:13 +0100
> From: David Ponce <da_vid@orange.fr>
> 
> On 2024-12-07 16:49, Alan Third wrote:
> > On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
> >>> Date: Sat, 7 Dec 2024 13:13:58 +0100
> >>> From:  David Ponce via "Bug reports for GNU Emacs,
> >>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>>
> >>> While working with images, I found what seems an issue to me with
> >>> `create-image' which unconditionally set the :scale image property to
> >>> 'default' when not specified, ignoring the value of the option
> >>> `image-scaling-factor'.
> >>>
> >>> Here is an illustration:
> >>>
> >>> (let ((image-scaling-factor 1.0))
> >>>      (image-size
> >>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> >>>       t))
> >>> => (63 . 63)
> >>>
> >>> (let ((image-scaling-factor 2.0))
> >>>      (image-size
> >>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
> >>>       t))
> >>> => (63 . 63)
> >>>
> >>> (image-size
> >>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
> >>>     t)
> >>> => (48 . 48)
> >>>
> >>> (image-size
> >>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
> >>>     t)
> >>> => (96 . 96)
> >>>
> >>> You can replace `image-size' with `insert-image' and observe the same.
> >>>
> >>> Here is a simple patch which fix the issue for me:
> >>>
> >>> diff --git a/lisp/image.el b/lisp/image.el
> >>> index ce97eeb3ca1..2c1e865c336 100644
> >>> --- a/lisp/image.el
> >>> +++ b/lisp/image.el
> >>> @@ -536,7 +536,9 @@ create-image
> >>>                             file-or-data)
> >>>                       (and (not (plist-get props :scale))
> >>>                            ;; Add default scaling.
> >>> -                        (list :scale 'default))
> >>> +                        (list :scale (if (numberp image-scaling-factor)
> >>> +                                         image-scaling-factor
> >>> +                                       'default)))
> >>>    	           props)))
> >>>          ;; Add default smoothing.
> >>>          (unless (plist-member props :transform-smoothing)
> >>
> >> AFAIU, this is supposed to be taken care of in image.c.
> >>
> >> Alan, any ideas why this doesn't seem to work?
> > 
> > It's because the image spec doesn't change so the image is pulled from
> > the cache each time.
> > 
> > Flushing the image between calls to image-size fixes it:
> > 
> >      (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))
> > 
> > I'm not sure what the solution is here. My feeling is that
> > image-scaling-factor isn't intended as something you set for each
> > image as you load it, it's a set-and-forget setting, so perhaps we
> > just need to document that in order for it to take effect the image
> > cache needs to be flushed.
> > 
> > Alternatively we make the image cache aware of it.
> > 
> > Perhaps we can flush the cache automatically when it changes? That
> > might give unexpected results too.
> 
> This is exactly what I also observe.
> It seems due to this change:
> 
> author	Po Lu <luangruo@yahoo.com>	2024-06-03 16:34:51 +0800
> committer	Po Lu <luangruo@yahoo.com>	2024-06-03 16:36:29 +0800
> commit	56376585134d627f96c71b7b063ec51548d3ad3f (patch)
> 
> Which replaced
> 
> -                        (list :scale
> -                              (image-compute-scaling-factor
> -                               image-scaling-factor)))
> 
> By this
> 
> +                        (list :scale 'default))
> 
> In create-image.
> 
> With the side effect that the image spec don't change when the scaling
> factor changes, so the same cached image in always used.

Po Lu, what were the reasons for that particular part of the commit?

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 7 Dec 2024 17:27:13 +0100
>> From: David Ponce <da_vid@orange.fr>
>> 
>> On 2024-12-07 16:49, Alan Third wrote:
>> > On Sat, Dec 07, 2024 at 04:49:41PM +0200, Eli Zaretskii wrote:
>> >>> Date: Sat, 7 Dec 2024 13:13:58 +0100
>> >>> From:  David Ponce via "Bug reports for GNU Emacs,
>> >>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> >>>
>> >>> While working with images, I found what seems an issue to me with
>> >>> `create-image' which unconditionally set the :scale image property to
>> >>> 'default' when not specified, ignoring the value of the option
>> >>> `image-scaling-factor'.
>> >>>
>> >>> Here is an illustration:
>> >>>
>> >>> (let ((image-scaling-factor 1.0))
>> >>>      (image-size
>> >>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>> >>>       t))
>> >>> => (63 . 63)
>> >>>
>> >>> (let ((image-scaling-factor 2.0))
>> >>>      (image-size
>> >>>       (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg")))
>> >>>       t))
>> >>> => (63 . 63)
>> >>>
>> >>> (image-size
>> >>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 1)))
>> >>>     t)
>> >>> => (48 . 48)
>> >>>
>> >>> (image-size
>> >>>     (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg" :scale 2)))
>> >>>     t)
>> >>> => (96 . 96)
>> >>>
>> >>> You can replace `image-size' with `insert-image' and observe the same.
>> >>>
>> >>> Here is a simple patch which fix the issue for me:
>> >>>
>> >>> diff --git a/lisp/image.el b/lisp/image.el
>> >>> index ce97eeb3ca1..2c1e865c336 100644
>> >>> --- a/lisp/image.el
>> >>> +++ b/lisp/image.el
>> >>> @@ -536,7 +536,9 @@ create-image
>> >>>                             file-or-data)
>> >>>                       (and (not (plist-get props :scale))
>> >>>                            ;; Add default scaling.
>> >>> -                        (list :scale 'default))
>> >>> +                        (list :scale (if (numberp image-scaling-factor)
>> >>> +                                         image-scaling-factor
>> >>> +                                       'default)))
>> >>>    	           props)))
>> >>>          ;; Add default smoothing.
>> >>>          (unless (plist-member props :transform-smoothing)
>> >>
>> >> AFAIU, this is supposed to be taken care of in image.c.
>> >>
>> >> Alan, any ideas why this doesn't seem to work?
>> > 
>> > It's because the image spec doesn't change so the image is pulled from
>> > the cache each time.
>> > 
>> > Flushing the image between calls to image-size fixes it:
>> > 
>> >      (image-flush (find-image '((:file "icons/hicolor/scalable/apps/emacs.svg"))))
>> > 
>> > I'm not sure what the solution is here. My feeling is that
>> > image-scaling-factor isn't intended as something you set for each
>> > image as you load it, it's a set-and-forget setting, so perhaps we
>> > just need to document that in order for it to take effect the image
>> > cache needs to be flushed.
>> > 
>> > Alternatively we make the image cache aware of it.
>> > 
>> > Perhaps we can flush the cache automatically when it changes? That
>> > might give unexpected results too.
>> 
>> This is exactly what I also observe.
>> It seems due to this change:
>> 
>> author	Po Lu <luangruo@yahoo.com>	2024-06-03 16:34:51 +0800
>> committer	Po Lu <luangruo@yahoo.com>	2024-06-03 16:36:29 +0800
>> commit	56376585134d627f96c71b7b063ec51548d3ad3f (patch)
>> 
>> Which replaced
>> 
>> -                        (list :scale
>> -                              (image-compute-scaling-factor
>> -                               image-scaling-factor)))
>> 
>> By this
>> 
>> +                        (list :scale 'default))
>> 
>> In create-image.
>> 
>> With the side effect that the image spec don't change when the scaling
>> factor changes, so the same cached image in always used.
>
> Po Lu, what were the reasons for that particular part of the commit?

The scale applied by image-scaling-factor is liable to differ by
display, and computing the default scale in Lisp would result in images
being displayed with an incorrect scale in the presence of multiple
displays.

Image caches must be flushed when image-scaling-factor is modified,
unless it is set to `auto' and a display's scale changes, because
image.c has no means of detecting variable modifications and so only the
latter event can be automatically detected.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: David Ponce <da_vid@orange.fr>,  alan@idiocy.org,  74725@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 08:01:39 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> author	Po Lu <luangruo@yahoo.com>	2024-06-03 16:34:51 +0800
> >> committer	Po Lu <luangruo@yahoo.com>	2024-06-03 16:36:29 +0800
> >> commit	56376585134d627f96c71b7b063ec51548d3ad3f (patch)
> >> 
> >> Which replaced
> >> 
> >> -                        (list :scale
> >> -                              (image-compute-scaling-factor
> >> -                               image-scaling-factor)))
> >> 
> >> By this
> >> 
> >> +                        (list :scale 'default))
> >> 
> >> In create-image.
> >> 
> >> With the side effect that the image spec don't change when the scaling
> >> factor changes, so the same cached image in always used.
> >
> > Po Lu, what were the reasons for that particular part of the commit?
> 
> The scale applied by image-scaling-factor is liable to differ by
> display

How so?  Please elaborate.

> and computing the default scale in Lisp would result in images
> being displayed with an incorrect scale in the presence of multiple
> displays.

How does the above changeset solve this problem, then?

> Image caches must be flushed when image-scaling-factor is modified,
> unless it is set to `auto' and a display's scale changes, because
> image.c has no means of detecting variable modifications and so only the
> latter event can be automatically detected.

Please describe the issue in more detail, as I don't think I follow
what you are saying here.  If we need to detect changes in variables,
we can use the add-variable-watcher technique, similar to what we do
in frame.el with variables that need to force redisplay (but maybe I
don't understand the problem you are describing).

In any case, I don't think changes in image-scaling-factor are
supposed to be immediately reflected on display, if that's what you
have in mind.  This is not the documented effect of this variable.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: David Ponce <da_vid@orange.fr>,  alan@idiocy.org,  74725@debbugs.gnu.org
>> Date: Sun, 08 Dec 2024 08:01:39 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> author	Po Lu <luangruo@yahoo.com>	2024-06-03 16:34:51 +0800
>> >> committer	Po Lu <luangruo@yahoo.com>	2024-06-03 16:36:29 +0800
>> >> commit	56376585134d627f96c71b7b063ec51548d3ad3f (patch)
>> >> 
>> >> Which replaced
>> >> 
>> >> -                        (list :scale
>> >> -                              (image-compute-scaling-factor
>> >> -                               image-scaling-factor)))
>> >> 
>> >> By this
>> >> 
>> >> +                        (list :scale 'default))
>> >> 
>> >> In create-image.
>> >> 
>> >> With the side effect that the image spec don't change when the scaling
>> >> factor changes, so the same cached image in always used.
>> >
>> > Po Lu, what were the reasons for that particular part of the commit?
>> 
>> The scale applied by image-scaling-factor is liable to differ by
>> display
>
> How so?  Please elaborate.

When it is set to `auto' (the default value), the scaling factor to be
applied is decided by the configuration of a frame, namely, its
FRAME_COLUMN_WIDTH.

>> and computing the default scale in Lisp would result in images
>> being displayed with an incorrect scale in the presence of multiple
>> displays.
>
> How does the above changeset solve this problem, then?

By moving its application to image.c, which knows where an image is
being displayed and can apply specific scales for each frame.

>> Image caches must be flushed when image-scaling-factor is modified,
>> unless it is set to `auto' and a display's scale changes, because
>> image.c has no means of detecting variable modifications and so only the
>> latter event can be automatically detected.
>
> Please describe the issue in more detail, as I don't think I follow
> what you are saying here.  If we need to detect changes in variables,
> we can use the add-variable-watcher technique, similar to what we do
> in frame.el with variables that need to force redisplay (but maybe I
> don't understand the problem you are describing).
>
> In any case, I don't think changes in image-scaling-factor are
> supposed to be immediately reflected on display, if that's what you
> have in mind.  This is not the documented effect of this variable.

What I am trying to communicate is that changes to
`image-scaling-factor' must be accompanied by flushing the image cache
if it is to take effect on all previously displayed images.  This isn't
a problem, and the OP should simply flush the image cache after
modifying image-scaling-factor, rather than rely on the erroneous
behavior of find-image which was removed.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: da_vid@orange.fr,  alan@idiocy.org,  74725@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 16:03:28 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Po Lu, what were the reasons for that particular part of the commit?
> >> 
> >> The scale applied by image-scaling-factor is liable to differ by
> >> display
> >
> > How so?  Please elaborate.
> 
> When it is set to `auto' (the default value), the scaling factor to be
> applied is decided by the configuration of a frame, namely, its
> FRAME_COLUMN_WIDTH.

So when the default font changes, all the images are supposed to be
resized?  Does that really happen, and if so, is that a good idea in
all cases?

> >> and computing the default scale in Lisp would result in images
> >> being displayed with an incorrect scale in the presence of multiple
> >> displays.
> >
> > How does the above changeset solve this problem, then?
> 
> By moving its application to image.c, which knows where an image is
> being displayed and can apply specific scales for each frame.

But, as this bug seems to indicate, that solution doesn't always work?

> >> Image caches must be flushed when image-scaling-factor is modified,
> >> unless it is set to `auto' and a display's scale changes, because
> >> image.c has no means of detecting variable modifications and so only the
> >> latter event can be automatically detected.
> >
> > Please describe the issue in more detail, as I don't think I follow
> > what you are saying here.  If we need to detect changes in variables,
> > we can use the add-variable-watcher technique, similar to what we do
> > in frame.el with variables that need to force redisplay (but maybe I
> > don't understand the problem you are describing).
> >
> > In any case, I don't think changes in image-scaling-factor are
> > supposed to be immediately reflected on display, if that's what you
> > have in mind.  This is not the documented effect of this variable.
> 
> What I am trying to communicate is that changes to
> `image-scaling-factor' must be accompanied by flushing the image cache
> if it is to take effect on all previously displayed images.  This isn't
> a problem, and the OP should simply flush the image cache after
> modifying image-scaling-factor, rather than rely on the erroneous
> behavior of find-image which was removed.

OK, so do you consider the solution of recording the scale factor in
the cache a reasonable one?

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

Ping!

> Cc: 74725@debbugs.gnu.org, da_vid@orange.fr, alan@idiocy.org
> Date: Sun, 08 Dec 2024 14:15:02 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: da_vid@orange.fr,  alan@idiocy.org,  74725@debbugs.gnu.org
> > Date: Sun, 08 Dec 2024 16:03:28 +0800
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> > Po Lu, what were the reasons for that particular part of the commit?
> > >> 
> > >> The scale applied by image-scaling-factor is liable to differ by
> > >> display
> > >
> > > How so?  Please elaborate.
> > 
> > When it is set to `auto' (the default value), the scaling factor to be
> > applied is decided by the configuration of a frame, namely, its
> > FRAME_COLUMN_WIDTH.
> 
> So when the default font changes, all the images are supposed to be
> resized?  Does that really happen, and if so, is that a good idea in
> all cases?
> 
> > >> and computing the default scale in Lisp would result in images
> > >> being displayed with an incorrect scale in the presence of multiple
> > >> displays.
> > >
> > > How does the above changeset solve this problem, then?
> > 
> > By moving its application to image.c, which knows where an image is
> > being displayed and can apply specific scales for each frame.
> 
> But, as this bug seems to indicate, that solution doesn't always work?
> 
> > >> Image caches must be flushed when image-scaling-factor is modified,
> > >> unless it is set to `auto' and a display's scale changes, because
> > >> image.c has no means of detecting variable modifications and so only the
> > >> latter event can be automatically detected.
> > >
> > > Please describe the issue in more detail, as I don't think I follow
> > > what you are saying here.  If we need to detect changes in variables,
> > > we can use the add-variable-watcher technique, similar to what we do
> > > in frame.el with variables that need to force redisplay (but maybe I
> > > don't understand the problem you are describing).
> > >
> > > In any case, I don't think changes in image-scaling-factor are
> > > supposed to be immediately reflected on display, if that's what you
> > > have in mind.  This is not the documented effect of this variable.
> > 
> > What I am trying to communicate is that changes to
> > `image-scaling-factor' must be accompanied by flushing the image cache
> > if it is to take effect on all previously displayed images.  This isn't
> > a problem, and the OP should simply flush the image cache after
> > modifying image-scaling-factor, rather than rely on the erroneous
> > behavior of find-image which was removed.
> 
> OK, so do you consider the solution of recording the scale factor in
> the cache a reasonable one?
> 
> 
> 
> 

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> So when the default font changes, all the images are supposed to be
>> resized?  Does that really happen, and if so, is that a good idea in
>> all cases?

IMHO, yes, considering that most images are scaled by default.

>> But, as this bug seems to indicate, that solution doesn't always work?

Not when image-scaling-factor is not configured to `auto', yes.

>> OK, so do you consider the solution of recording the scale factor in
>> the cache a reasonable one?

That is alright by me.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 74725@debbugs.gnu.org,  da_vid@orange.fr,  alan@idiocy.org
> Date: Sun, 22 Dec 2024 08:26:25 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So when the default font changes, all the images are supposed to be
> >> resized?  Does that really happen, and if so, is that a good idea in
> >> all cases?
> 
> IMHO, yes, considering that most images are scaled by default.
> 
> >> But, as this bug seems to indicate, that solution doesn't always work?
> 
> Not when image-scaling-factor is not configured to `auto', yes.
> 
> >> OK, so do you consider the solution of recording the scale factor in
> >> the cache a reasonable one?
> 
> That is alright by me.

OK, thanks.

Alan, could you please prepare a patch (for the master branch) that
records the scaling factor in the image cache and rejects a cache hit
with a different scaling factor?

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Alan Third]

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

On Sun, Dec 22, 2024 at 08:48:04AM +0200, Eli Zaretskii wrote:
> Alan, could you please prepare a patch (for the master branch) that
> records the scaling factor in the image cache and rejects a cache hit
> with a different scaling factor?

Hi Eli, patch attached.

I named a new function image_compute_scale because most of the other
functions in that file start image_, however I wasn't sure if I should
name it compute_image_scale to mirror compute_image_size. Let me know
if you think I should change it.
-- 
Alan Third

[-- Attachment #2: 0001-Make-image-cache-aware-of-image-scaling-factor-bug-7.patch --]
[-- Type: text/x-diff, Size: 4649 bytes --]

From 30699b3ab509662dc1ca3bdbccc4269227ef28c8 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Fri, 27 Dec 2024 11:46:45 +0000
Subject: [PATCH] Make image cache aware of image-scaling-factor (bug#74725)

* src/dispextern.h (struct image): Add scale so it can be compared in
search_image_cache.
* src/image.c (search_image_cache): Calculate the scale factor and
compare with the cached value.
(image_compute_scale): Compute the image's scale factor and optionally
store it in the image struct.
(compute_image_size): Move scale calculation code into
image_compute_scale and use it.
---
 src/dispextern.h |  3 +++
 src/image.c      | 45 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index ea7b0399adc..c876856717a 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3257,6 +3257,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
   /* Width and height of the image.  */
   int width, height;
 
+  /* The scale factor applied to the image.  */
+  double scale;
+
   /* These values are used for the rectangles displayed for images
      that can't be loaded.  */
 #define DEFAULT_IMAGE_WIDTH 30
diff --git a/src/image.c b/src/image.c
index 0012abcb451..7b34b24ada9 100644
--- a/src/image.c
+++ b/src/image.c
@@ -210,6 +210,9 @@ #define n_planes n_image_planes
 static void image_edge_detection (struct frame *, struct image *, Lisp_Object,
                                   Lisp_Object);
 
+static double image_compute_scale (struct frame *f, Lisp_Object spec,
+				   struct image *img);
+
 static void init_color_table (void);
 static unsigned long lookup_rgb_color (struct frame *f, int r, int g, int b);
 #ifdef COLOR_TABLE_SUPPORT
@@ -2222,9 +2225,12 @@ search_image_cache (struct frame *f, Lisp_Object spec, EMACS_UINT hash,
      image spec specifies :background.  However, the extra memory
      usage is probably negligible in practice, so we don't bother.  */
 
+  double scale = image_compute_scale (f, spec, NULL);
+
   for (img = c->buckets[i]; img; img = img->next)
     if (img->hash == hash
 	&& !NILP (Fequal (img->spec, spec))
+	&& scale == img->scale
 	&& (ignore_colors || (img->face_foreground == foreground
                               && img->face_background == background
 			      && img->face_font_size == font_size
@@ -2667,18 +2673,15 @@ image_get_dimension (struct image *img, Lisp_Object symbol)
   return -1;
 }
 
-/* Compute the desired size of an image with native size WIDTH x HEIGHT,
-   which is to be displayed on F.  Use IMG 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 (struct frame *f, double width, double height,
-		    struct image *img,
-		    int *d_width, int *d_height)
+/* Calculate the scale of the image.  IMG may be null as it is only
+   required when creating an image, and this function is called from
+   image cache related functions that do not have access to the image
+   structure.  */
+static double
+image_compute_scale (struct frame *f, Lisp_Object spec, struct image *img)
 {
   double scale = 1;
-  Lisp_Object value = image_spec_value (img->spec, QCscale, NULL);
+  Lisp_Object value = image_spec_value (spec, QCscale, NULL);
 
   if (EQ (value, Qdefault))
     {
@@ -2692,7 +2695,9 @@ compute_image_size (struct frame *f, double width, double height,
 	{
 	  /* This is a tag with which callers of `clear_image_cache' can
 	     refer to this image and its likenesses.  */
-	  img->dependencies = Fcons (Qauto, img->dependencies);
+	  if (img)
+	    img->dependencies = Fcons (Qauto, img->dependencies);
+
 	  scale = (FRAME_COLUMN_WIDTH (f) > 10
 		   ? (FRAME_COLUMN_WIDTH (f) / 10.0f) : 1);
 	}
@@ -2716,6 +2721,24 @@ compute_image_size (struct frame *f, double width, double height,
 	scale = dval;
     }
 
+  if (img)
+    img->scale = scale;
+
+  return scale;
+}
+
+/* Compute the desired size of an image with native size WIDTH x HEIGHT,
+   which is to be displayed on F.  Use IMG 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 (struct frame *f, double width, double height,
+		    struct image *img,
+		    int *d_width, int *d_height)
+{
+  double scale = image_compute_scale(f, img->spec, img);
+
   /* 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
-- 
2.45.2

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2024-12-27 13:11, Alan Third wrote:
> On Sun, Dec 22, 2024 at 08:48:04AM +0200, Eli Zaretskii wrote:
>> Alan, could you please prepare a patch (for the master branch) that
>> records the scaling factor in the image cache and rejects a cache hit
>> with a different scaling factor?
> 
> Hi Eli, patch attached.
> 
> I named a new function image_compute_scale because most of the other
> functions in that file start image_, however I wasn't sure if I should
> name it compute_image_scale to mirror compute_image_size. Let me know
> if you think I should change it.

Hi Alan,

FYI, I applied your patch and it fixes the issue for me with the initial
examples I provided.

Many thanks Alan, and to all of you!

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Eli Zaretskii]

> Date: Fri, 27 Dec 2024 12:11:33 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: Po Lu <luangruo@yahoo.com>, 74725@debbugs.gnu.org, da_vid@orange.fr
> 
> > Alan, could you please prepare a patch (for the master branch) that
> > records the scaling factor in the image cache and rejects a cache hit
> > with a different scaling factor?
> 
> Hi Eli, patch attached.

Thanks, LGTM.

> I named a new function image_compute_scale because most of the other
> functions in that file start image_, however I wasn't sure if I should
> name it compute_image_scale to mirror compute_image_size. Let me know
> if you think I should change it.

Static functions can have any names we see fit, so feel free to rename
if you think compute_image_scale would be better.

Feel free to install on master, when you are satisfied with the code.

bug#74725: 31.0.50; image-scaling-factor is ignored by create-image

[Alan Third]

On Sat, Dec 28, 2024 at 02:26:47PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 27 Dec 2024 12:11:33 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: Po Lu <luangruo@yahoo.com>, 74725@debbugs.gnu.org, da_vid@orange.fr
> > 
> > > Alan, could you please prepare a patch (for the master branch) that
> > > records the scaling factor in the image cache and rejects a cache hit
> > > with a different scaling factor?
> > 
> > Hi Eli, patch attached.
> 
> Thanks, LGTM.
> 
> > I named a new function image_compute_scale because most of the other
> > functions in that file start image_, however I wasn't sure if I should
> > name it compute_image_scale to mirror compute_image_size. Let me know
> > if you think I should change it.
> 
> Static functions can have any names we see fit, so feel free to rename
> if you think compute_image_scale would be better.
> 
> Feel free to install on master, when you are satisfied with the code.

Done. Thanks.
-- 
Alan Third