bug#45224: 28.0.50; eww and GIFS (cpu usage shoots through the roof)

[Stefan Kangas <stefan@marxist.se>]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Madhavan Krishnan <krishnanmadhavan000@gmail.com> writes:
>
>> Thank you for investigating this issue, I am not entirely sure of what
>> the next step would be. Is there a sample code that I can refer to for
>> the ImageMagic version you are referring to? (to get some idea about how
>> it can be patched)
>
> imagemagick_get_animation_cache and friends in src/image.c.

I've been looking into this a bit, and here are my preliminary
conclusions.  I hope it is clear, and appreciate any feedback!

The first part of the problem is making sure that we can cache animated
images, which currently never happens.  Once that is in place, the
second part is to cache all animated images up-front.  I only discuss
the first part below.

Besides the special cache for ImageMagick we also have tho one used for
*all* images.  So my thinking is: why not use the "real" cache also for
animated images?  Why implement a specialized cache just for that?

I believe it is possible.[1]  And actually, the first question is: why
aren't they already cached?  The reason is this: in search_image_cache
(image.c:1599), we use sxhash and Fequal to compare the Lisp image
descriptor (or "image specification" or "spec" as its called in image.c)
to the one we have in cache.  But in an animated gif the image spec is
changed in image.el:

    (image :type gif
           :file "/some/file.gif"
           :scale 1
           :max-width 1248
           :max-height 1321
           :format nil
           :transform-smoothing t
           :animate-buffer "file.gif"
           :animate-tardiness 0.9810895737588246
           :animate-multi-frame-data (62 . 0.04)
           :index 61)

The value of :animate-tardiness is updated in image.el on every
iteration.  When image.el updates :animate-tardiness, it will never be
equal to something in the cache and we always get a cache miss!

I have attached a patch that will demonstrate how we get back caching by
simply disabling the updating of the image spec in image.el.  This is
completely the wrong thing to do, but demonstrates what is going on.
With that patch, caching will be enabled, and you can see it the second
time you try to run an animated gif.  (Use RET to start the animation.)

The correct fix here is to somehow *only* compare the relevant
attributes above, and ignore or leave out e.g. :animate-tardiness.

I have experimented with different approaches.  My first idea was to
create a new comparison function "image_cache_equal" that basically
only compares the attributes we are interested.  But then the problem
was that we use the "sxhash" function to find which hash bucket to
find the list in, so that didn't help much.  We would also need to
replace the sxhash with something else.  Perhaps sxhash_equal?  Hmm.

So I thought of two other ways to go about this:

A) We create a new stripped down Lisp plist containing only the
   attributes we are interested in and use that to compare against the
   cache.  We obviously need to make sure the cache also contains only
   stripped down list.  This means we can keep using Fequal and sxhash,
   but we also need to create a new list for *every cache lookup*!

B) We parse the relevant parts of the image spec into a C struct and
   then use that struct for comparisons.  It turns out we already have
   most of the code in place to do this, see parse_image_spec
   (image.c:898), so most of it should be some restructuring, and then
   writing up a comparison function.  With that in place, we would just
   need to .

Option A) seems ugly to me: why would we be consing up Lisp lists on
such a low level, which also makes me worry about creating a lot of
unnecessary garbage.  So I prefer Option B).  The struct also seems
more clean to me.  Perhaps there are some performance implications I'm
not thinking of?  Or perhaps there is some even better way to do the
cache check than a new struct, such as using the "image struct"
directly?

A third alternative is to somehow change image.el to put this
information outside the image specifier, but that leaves unfixed a
rather subtle issue with caching.  That issue may or may not bite
someone later.

Comments about all this is very welcome!  I'm basically at the point
where any approach I choose means investing a bunch of work, and it
would be useful with some feedback before I rush ahead and attempt any
of them.  Perhaps someone here has an idea or hunch about which
approach might prove more fruitful.

Footnotes:
[1] There is a comment in the ImageMagick cache saying: "We have to
     maintain a cache separate from the image cache, because the images
     may be scaled before display."  However, this was written before we
     had native image scaling, and actually it seems to me either
     incorrect, based on the above, or correct but only applicable to
     ImageMagick, or, perhaps more likely, I am missing something
     important.

[-- Attachment #2: 0001-gif-load-debug.patch --]
[-- Type: text/x-diff, Size: 4042 bytes --]

From 2041083b611c002882ea5ad2e3341bf63b8c94b4 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sun, 31 Oct 2021 01:37:31 +0200
Subject: [PATCH] gif load debug

---
 lisp/image.el | 37 ++++++++++++++++++++-----------------
 src/image.c   |  3 +++
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/lisp/image.el b/lisp/image.el
index bfcbb66c25..3ab414a4d3 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -841,10 +841,10 @@ image-animate
       (if (setq timer (image-animate-timer image))
 	  (cancel-timer timer))
       (plist-put (cdr image) :animate-buffer (current-buffer))
-      (plist-put (cdr image) :animate-tardiness 0)
+      ;; (plist-put (cdr image) :animate-tardiness 0)
       ;; Stash the data about the animation here so that we don't
       ;; trigger image recomputation unnecessarily later.
-      (plist-put (cdr image) :animate-multi-frame-data animation)
+      ;; (plist-put (cdr image) :animate-multi-frame-data animation)
       (run-with-timer 0.2 nil #'image-animate-timeout
 		      image (or index 0) (car animation)
 		      0 limit (+ (float-time) 0.2)))))
@@ -873,10 +873,10 @@ image-show-frame
   "Show frame N of IMAGE.
 Frames are indexed from 0.  Optional argument NOCHECK non-nil means
 do not check N is within the range of frames present in the image."
-  (unless nocheck
-    (if (< n 0) (setq n 0)
-      (setq n (min n (1- (car (plist-get (cdr image)
-                                         :animate-multi-frame-data)))))))
+  ;; (unless nocheck
+  ;;   (if (< n 0) (setq n 0)
+  ;;     (setq n (min n (1- (car (plist-get (cdr image)
+  ;;                                        :animate-multi-frame-data)))))))
   (plist-put (cdr image) :index n)
   (force-window-update (plist-get (cdr image) :animate-buffer)))
 
@@ -912,23 +912,26 @@ image-animate-timeout
 for the animation speed.  A negative value means to animate in reverse."
   ;; We keep track of "how late" image frames arrive.  We decay the
   ;; previous cumulative value by 10% and then add the current delay.
-  (plist-put (cdr image) :animate-tardiness
-             (+ (* (plist-get (cdr image) :animate-tardiness) 0.9)
-                (float-time (time-since target-time))))
+  ;; (plist-put (cdr image) :animate-tardiness
+  ;;            (+ (* (plist-get (cdr image) :animate-tardiness) 0.9)
+  ;;               (float-time (time-since target-time))))
   (when (and (buffer-live-p (plist-get (cdr image) :animate-buffer))
              ;; Cumulatively delayed two seconds more than expected.
-             (or (< (plist-get (cdr image) :animate-tardiness) 2)
-		 (progn
-		   (message "Stopping animation; animation possibly too big")
-		   nil)))
+             t
+             ;; (or (< (plist-get (cdr image) :animate-tardiness) 2)
+             ;;     (progn
+             ;;       (message "Stopping animation; animation possibly too big")
+             ;;       nil))
+             )
     (image-show-frame image n t)
     (let* ((speed (image-animate-get-speed image))
 	   (time (current-time))
 	   (time-to-load-image (time-since time))
-	   (stated-delay-time
-            (/ (or (cdr (plist-get (cdr image) :animate-multi-frame-data))
-		   image-default-frame-delay)
-	       (float (abs speed))))
+           (stated-delay-time 0.07
+            ;; (/ (or (cdr (plist-get (cdr image) :animate-multi-frame-data))
+            ;;        image-default-frame-delay)
+            ;;    (float (abs speed)))
+            )
 	   ;; Subtract off the time we took to load the image from the
 	   ;; stated delay time.
 	   (delay (max (float-time (time-subtract stated-delay-time
diff --git a/src/image.c b/src/image.c
index 2ec44584b6..b87d50d17f 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8487,6 +8487,9 @@ gif_load (struct frame *f, struct image *img)
       }
   }
 
+  AUTO_STRING (msg, "#### gif_load=%s (%s)");
+  CALLN (Fmessage, msg, make_fixnum (idx), Fimage_cache_size ());
+
   width = img->width = gif->SWidth;
   height = img->height = gif->SHeight;
 
-- 
2.30.2