bug#71913: 29.1; shr: shr-resize-image does not behave as expected

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[George Huebner]

I noticed an issue with =shr-max-image-proportion= wherein images 
would be correctly resized on initial load, but would then appear 
much larger.
I suspected this was an issue with caching, and sure enough, 
either disabling the cache with ~(setq shr-ignore-cache t)~ or 
manually deleting images from the cache results in images 
displaying correctly.
This unanswered 
[[https://www.reddit.com/r/emacs/comments/esa3x5/image_scaling_in_elfeed_entry_window/][Reddit 
post]] seems to corroborate this behaviour.
It looks like it might be because of the codepath taken in 
~shr-tag-img~ wherein a cache miss results in the creation of a 
placeholder image; I had some trouble with edebug though, so 
that's just an educated guess.

Here's a minimal reproducible example (credit to Sacha Chua):
#+begin_src elisp :eval no
;; run this (image is very large), run again many times without 
   killing *test* (image is correct size), kill *test* and run 
   again (image is large again)
;; won't observe this behaviour if you disable cache
;; (setq shr-ignore-cache t)
(setq shr-max-image-proportion 0.5)
(with-current-buffer (get-buffer-create "*test*")
  (erase-buffer)
  (insert "<img 
  src=\"https://upload.wikimedia.org/wikipedia/commons/8/83/The_GNU_logo.png\">")
  (shr-insert-document (libxml-parse-html-region (point-min) 
  (point-max)))
  (display-buffer (current-buffer)))
#+end_src elisp

I also observed this behaviour on Emacs master (667ca66), but I 
daily drive 29.1.
In GNU Emacs 29.1 (build 1, aarch64-apple-darwin23.4.0, Carbon 
Version
170 AppKit 2487.5)
Windowing system distributor 'Apple Inc.', version 14.4.0
System Description:  macOS 14.4

Configured using:
 'configure
 --prefix=/nix/store/ismv7jzf3hcqziq5bpjfs54zd4qfjjn7-emacs-mac-macport-29.1
 --disable-build-details --with-modules --without-gif 
 --without-jpeg
 --without-png --without-tiff --without-x --without-xpm
 '--enable-mac-app=$$out/Applications' --with-gnutls --with-mac
 --with-xml2 --without-ns --with-compress-install
 --with-toolkit-scroll-bars --with-native-compilation
 --without-imagemagick --with-mailutils --without-small-ja-dic
 --with-tree-sitter --without-xinput2 --without-xwidgets 
 --without-dbus
 --without-selinux --with-xwidgets'

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

On 7/2/2024 11:37 PM, George Huebner wrote:
> I noticed an issue with =shr-max-image-proportion= wherein images would 
> be correctly resized on initial load, but would then appear much larger.
[snip]
> Here's a minimal reproducible example (credit to Sacha Chua):
> #+begin_src elisp :eval no
> ;; run this (image is very large), run again many times without   
> killing *test* (image is correct size), kill *test* and run   again 
> (image is large again)
> ;; won't observe this behaviour if you disable cache
> ;; (setq shr-ignore-cache t)
> (setq shr-max-image-proportion 0.5)
> (with-current-buffer (get-buffer-create "*test*")
>   (erase-buffer)
>   (insert "<img 
>   src=\"https://upload.wikimedia.org/wikipedia/commons/8/83/The_GNU_logo.png\">")
>   (shr-insert-document (libxml-parse-html-region (point-min)  (point-max)))
>   (display-buffer (current-buffer)))
> #+end_src elisp

The bug here is really in your steps to reproduce, which I suppose is 
roughly what Elfeed is doing too (though I haven't looked at that code 
to be totally sure).

SHR scales images with respect to the size of the buffer's window. 
However, by rendering the HTML document with SHR *before* displaying the 
buffer in a window, it's impossible for Emacs to do that: there's no 
such window yet! If you swap the 'display-buffer' and 
'shr-insert-document' lines though, all should work properly.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

(Cc'ing the bug list to follow up there too.)

On Wed, Jul 3, 2024 at 1:26 PM George Huebner <george@feyor.sh> wrote:
>
> You're quite right... I understood ~(not (get-buffer-window
> (current-buffer) t))~ (in shr-rescale-image) was preventing
> rescaling in undisplayed buffers, but in retrospect this is the
> responsibility of the caller, as you say.
>
> Apologies for the fruitless bug report, this thread can be closed.

On the contrary, I think it makes sense to do *something* here. At
minimum, Elfeed should probably get a fix, since I've noticed this
issue there too (but never bothered to figure out why). Now that you
found minimal steps to reproduce, hopefully we can fix Elfeed.

It might also be good to at least document this limitation, or even
enhance 'shr-max-image-proportion' to support absolute pixel sizes,
which would be immune to this issue.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[George Huebner]

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

Aha: after patching Elfeed to render after buffer display (which 
seems to fix the display of raster images), I actually did find 
behaviour that I believe should be addressed in shr, not in 
Elfeed; let me know what you think.

If an image is a SVG, `shr-put-image` will not respect 
`shr-max-image-proportion`, because it will pass it directly to 
`create-image`, instead of resizing it:
#+begin_src elisp
;; in `shr-put-image`
((eq content-type 'image/svg+xml)
         (when (image-type-available-p 'svg)
   (create-image data 'svg t :ascent shr-image-ascent)))
(t
 (ignore-errors (shr-rescale-image data content-type
                                           (plist-get flags 
                                           :width)
                                           (plist-get flags 
                                           :height)))))))
#+end_src

As I understand it, this decision was likely made because the 
dimensions of SVGs is weird; they aren't inherently required to 
have a size, but the SVG itself can define a viewbox with 
accompanying width and height (full disclosure: I have no idea 
what I'm talking about).

Using `shr-rescale-image` instead seems to do the trick (and 
respects SVGs that should display smaller than 
`shr-max-image-proportion`), but please do let me know if I'm 
ignoring an edge case or there's a better way to address this. 

#+begin_src elisp
(setq shr-put-image-function (lambda (spec alt &optional flags)
  (if (display-graphic-p)
      (let* ((size (cdr (assq 'size flags)))
	     (data (if (consp spec)
		       (car spec)
		     spec))
	     (content-type (and (consp spec)
				(cadr spec)))
	     (start (point))
	     (image (cond
		     ((eq size 'original)
		      (create-image data nil t :ascent 100
				    :format content-type))
             ;; BEGIN fix
		     ((eq content-type 'image/svg+xml)
                      (when (image-type-available-p 'svg)
		                ; (create-image data 'svg t :ascent 100)))
		                (shr-rescale-image data 'svg (plist-get 
		                flags :width) (plist-get flags :height))))
             ;; END fix
		     ((eq size 'full)
		      (ignore-errors
			(shr-rescale-image data content-type
                                           (plist-get flags 
                                           :width)
                                           (plist-get flags 
                                           :height))))
		     (t
		      (ignore-errors
			(shr-rescale-image data content-type
                                           (plist-get flags 
                                           :width)
                                           (plist-get flags 
                                           :height)))))))
        (when image
	  ;; When inserting big-ish pictures, put them at the
	  ;; beginning of the line.
	  (when (and (> (current-column) 0)
		     (> (car (image-size image t)) 400))
	    (insert "\n"))
          (let ((image-pos (point)))
	    (if (eq size 'original)
	        (insert-sliced-image image (or alt "*") nil 20 1)
	      (insert-image image (or alt "*")))
	    (put-text-property start (point) 'image-size size)
	    (when (and shr-image-animate
                       (cdr (image-multi-frame-p image)))
              (image-animate image nil 60 image-pos))))
	image)
    (insert (or alt "")))))
#+end_src elisp


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Elfeed: render after display --]
[-- Type: text/x-patch, Size: 1144 bytes --]

From 8f911a56beaf52f0df18537d0f9abae8373d7133 Mon Sep 17 00:00:00 2001
From: George Huebner <george@feyor.sh>
Date: Wed, 10 Jul 2024 01:57:15 -0500
Subject: [PATCH] elfeed-show: render after switching to entry buffer

shr can't respect `shr-max-image-proportion` because it isn't rendered
in a window; call `elfeed-search-show-entry` first to fix this.

See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=71913
---
 elfeed-show.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/elfeed-show.el b/elfeed-show.el
index 4915cae..e16b4bf 100644
--- a/elfeed-show.el
+++ b/elfeed-show.el
@@ -214,9 +214,9 @@ The result depends on the value of `elfeed-show-unique-buffers'."
   (let ((buff (get-buffer-create (elfeed-show--buffer-name entry))))
     (with-current-buffer buff
       (elfeed-show-mode)
-      (setq elfeed-show-entry entry)
-      (elfeed-show-refresh))
-    (funcall elfeed-show-entry-switch buff)))
+      (setq elfeed-show-entry entry))
+    (funcall elfeed-show-entry-switch buff)
+    (elfeed-show-refresh)))
 
 (defun elfeed-show-next ()
   "Show the next item in the elfeed-search buffer."
-- 
2.44.1


[-- Attachment #3: Type: text/plain, Size: 1004 bytes --]


On Fri, Jul  5, 2024 at 13:11:29 EST Jim Porter wrote:

> (Cc'ing the bug list to follow up there too.)
>
> On Wed, Jul 3, 2024 at 1:26 PM George Huebner <george@feyor.sh> 
> wrote:
>>
>> You're quite right... I understood ~(not (get-buffer-window
>> (current-buffer) t))~ (in shr-rescale-image) was preventing
>> rescaling in undisplayed buffers, but in retrospect this is the
>> responsibility of the caller, as you say.
>>
>> Apologies for the fruitless bug report, this thread can be 
>> closed.
>
> On the contrary, I think it makes sense to do *something* 
> here. At
> minimum, Elfeed should probably get a fix, since I've noticed 
> this
> issue there too (but never bothered to figure out why). Now that 
> you
> found minimal steps to reproduce, hopefully we can fix Elfeed.
>
> It might also be good to at least document this limitation, or 
> even
> enhance 'shr-max-image-proportion' to support absolute pixel 
> sizes,
> which would be immune to this issue.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

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

On 7/10/2024 12:02 PM, George Huebner wrote:
> As I understand it, this decision was likely made because the dimensions 
> of SVGs is weird; they aren't inherently required to have a size, but 
> the SVG itself can define a viewbox with accompanying width and height 
> (full disclosure: I have no idea what I'm talking about).

I think this was just a mistake. I tried getting rid of the special case 
for SVG and everything works just fine in builds with or without SVG 
support. (In builds without SVG support, you just get the empty string 
where an SVG would be, before and after this patch.)

[-- Attachment #2: 0001-Treat-SVG-images-like-other-image-types-in-shr-put-i.patch --]
[-- Type: text/plain, Size: 2195 bytes --]

From 70ecc19f735e0cea0fda8721d7f2709c39f1bbff Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 11 Jul 2024 16:29:37 -0700
Subject: [PATCH] Treat SVG images like other image types in 'shr-put-image'

For both SVG and no-SVG builds, this works as expected (in the no-SVG
case, it would raise an error which subsequently gets ignored).
However, compared to the previous implementation, this lets users resize
SVG images just like every other image type.

* lisp/net/shr.el (shr-put-image): Don't special-case SVGs.
---
 lisp/net/shr.el | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 39271cc5296..d3c48b34428 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1193,23 +1193,18 @@ shr-put-image
   (if (display-graphic-p)
       (let* ((zoom (or (plist-get flags :zoom)
                        (car shr-image-zoom-levels)))
-             (zoom-function (nth 2 (assq zoom shr-image-zoom-level-alist)))
+             (zoom-function (or (nth 2 (assq zoom shr-image-zoom-level-alist))
+                                (error "Unrecognized zoom level %s" zoom)))
 	     (data (if (consp spec)
 		       (car spec)
 		     spec))
 	     (content-type (and (consp spec)
 				(cadr spec)))
 	     (start (point))
-	     (image (cond
-		     ((eq content-type 'image/svg+xml)
-                      (when (image-type-available-p 'svg)
-		        (create-image data 'svg t :ascent shr-image-ascent)))
-                     (zoom-function
-                      (ignore-errors
-                        (funcall zoom-function data content-type
-                                 (plist-get flags :width)
-                                 (plist-get flags :height))))
-                     (t (error "Unrecognized zoom level %s" zoom)))))
+	     (image (ignore-errors
+                      (funcall zoom-function data content-type
+                               (plist-get flags :width)
+                               (plist-get flags :height)))))
         (when image
           ;; The trailing space can confuse shr-insert into not
           ;; putting any space after inline images.
-- 
2.25.1

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Eli Zaretskii]

> Cc: 71913@debbugs.gnu.org
> Date: Thu, 11 Jul 2024 16:51:53 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 7/10/2024 12:02 PM, George Huebner wrote:
> > As I understand it, this decision was likely made because the dimensions 
> > of SVGs is weird; they aren't inherently required to have a size, but 
> > the SVG itself can define a viewbox with accompanying width and height 
> > (full disclosure: I have no idea what I'm talking about).
> 
> I think this was just a mistake. I tried getting rid of the special case 
> for SVG and everything works just fine in builds with or without SVG 
> support. (In builds without SVG support, you just get the empty string 
> where an SVG would be, before and after this patch.)

What version of librsvg did you test this with?  Please test also with
librsvg 2.40.x, as that was the last version which didn't need a Rust
compiler to build, and so some people (including yours truly) stay
with those old versions to this day.

Thanks.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

On 7/11/2024 10:58 PM, Eli Zaretskii wrote:
> What version of librsvg did you test this with?  Please test also with
> librsvg 2.40.x, as that was the last version which didn't need a Rust
> compiler to build, and so some people (including yours truly) stay
> with those old versions to this day.

I'm usually using 2.48.x since it's what my distro provides, but I've 
now tested against 2.40.21 (after a slightly bumpy road getting 
everything to build correctly) and the results are the same for both 
versions of librsvg. (Out of an abundance of caution, I've also verified 
with 'ldd' that the correct version of librsvg is in use during these 
tests.)

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Eli Zaretskii]

> Date: Fri, 12 Jul 2024 21:11:35 -0700
> Cc: george@feyor.sh, 71913@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 7/11/2024 10:58 PM, Eli Zaretskii wrote:
> > What version of librsvg did you test this with?  Please test also with
> > librsvg 2.40.x, as that was the last version which didn't need a Rust
> > compiler to build, and so some people (including yours truly) stay
> > with those old versions to this day.
> 
> I'm usually using 2.48.x since it's what my distro provides, but I've 
> now tested against 2.40.21 (after a slightly bumpy road getting 
> everything to build correctly) and the results are the same for both 
> versions of librsvg. (Out of an abundance of caution, I've also verified 
> with 'ldd' that the correct version of librsvg is in use during these 
> tests.)

Thanks.  Then I have no objections for fixing this on the emacs-30
release branch.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

On 7/12/2024 11:20 PM, Eli Zaretskii wrote:
> Thanks.  Then I have no objections for fixing this on the emacs-30
> release branch.

Sounds good. I've merged my patch to the master branch though (as 
f38c42d1c7a), since the code change depends on the second round of 
patches from bug#71666, which are also only on the master branch.

I could probably make an Emacs 30 version of this patch if desired, but 
I think this particular issue is fairly small (and has been around for a 
long time). Maybe it's better to play it safe and leave it on the master 
branch only? I don't have a strong opinion here.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Eli Zaretskii]

> Date: Sat, 13 Jul 2024 10:29:43 -0700
> Cc: george@feyor.sh, 71913@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 7/12/2024 11:20 PM, Eli Zaretskii wrote:
> > Thanks.  Then I have no objections for fixing this on the emacs-30
> > release branch.
> 
> Sounds good. I've merged my patch to the master branch though (as 
> f38c42d1c7a), since the code change depends on the second round of 
> patches from bug#71666, which are also only on the master branch.
> 
> I could probably make an Emacs 30 version of this patch if desired, but 
> I think this particular issue is fairly small (and has been around for a 
> long time). Maybe it's better to play it safe and leave it on the master 
> branch only? I don't have a strong opinion here.

Leaving this on master is fine by me, thanks.

bug#71913: 29.1; shr: shr-resize-image does not behave as expected

[Jim Porter]

On 7/13/2024 10:39 AM, Eli Zaretskii wrote:
> Leaving this on master is fine by me, thanks.

Thanks. Closing this bug now, then.

George: we can track this elsewhere since it's not an Emacs bug per se, 
but did you want to submit your Elfeed patch to Elfeed? If you do, let 
me know and I'd be happy to chime in over there to support it getting 
merged.