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

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

[Madhavan Krishnan]

Hello there,

This is on Emacs 28.0.50 (MAIN branch) with "-Q"

Steps to reproduce the bug:

1) M-x eww 2) Insert this url at the prompt.
https://github.com/manateelazycat/emacs-application-framework

as "fix" I was told (setq shr-image-animate nil) would help, but would
defeat the purpose of GIFS

Below you can find the profile report for the cpu.

#+begin_example
- command-execute                                                4514  91%
 - call-interactively                                            4514  91%
  - byte-code                                                    4197  85%
   - read-extended-command                                       4197  85%
    - completing-read                                            4197  85%
     - completing-read-default                                   4197  85%
      - read-from-minibuffer                                     4135  83%
       - timer-event-handler                                     3423  69%
        - apply                                                  3423  69%
         - image-animate-timeout                                 3421  69%
            image-multi-frame-p                                  3420  69%
            time-since                                              1   0%
           #<compiled 0xbc1b6d5890d29>                              1   0%
       - url-http-generic-filter                                   91   1%
        - url-http-content-length-after-change-function                 89   1%
         - url-http-activate-callback                              81   1%
          - apply                                                  81   1%
           - url-queue-callback-function                           81   1%
            - apply                                                81   1%
             + shr-image-fetched                                   81   1%
         - file-size-human-readable-iec                             6   0%
            file-size-human-readable                                3   0%
          apply                                                     2   0%
       - redisplay_internal (C function)                           39   0%
        - funcall                                                  35   0%
         - #<compiled 0x1c38e9a167702bb5>                          35   0%
          + gui-backend-selection-exists-p                         35   0%
        - tool-bar-make-keymap                                      2   0%
         - tool-bar-make-keymap-1                                   2   0%
          - mapcar                                                  2   0%
             #<compiled 0xd4d9e7eb5da225a>                          2   0%
        + #<compiled 0x37cabbbdf65a500>                             2   0%
  - funcall-interactively                                         317   6%
   - execute-extended-command                                     317   6%
    - sit-for                                                     317   6%
     - read-event                                                 174   3%
      - timer-event-handler                                       161   3%
       + apply                                                    161   3%
      - url-http-generic-filter                                     2   0%
       - url-http-content-length-after-change-function                  2   0%
          url-percentage                                            1   0%
      - redisplay_internal (C function)                             1   0%
       + #<compiled 0x37cabbbdf65a500>                              1   0%
     - input-pending-p                                            108   2%
      - timer-event-handler                                       108   2%
       - apply                                                    108   2%
        - image-animate-timeout                                   108   2%
           image-multi-frame-p                                    108   2%
     + redisplay                                                   27   0%
+ timer-event-handler                                             348   7%
+ ...                                                              71   1%
+ redisplay_internal (C function)                                   2   0%
+ url-http-generic-filter                                           1   0%
#+end_example

Regards
-- 
Madhavan Krishnan

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

[Lars Ingebrigtsen]

Madhavan Krishnan <krishnanmadhavan000@gmail.com> writes:

> Steps to reproduce the bug:
>
> 1) M-x eww 2) Insert this url at the prompt.
> https://github.com/manateelazycat/emacs-application-framework

Yes, Emacs's animation code needs to cache the images better.

In any case, Emacs should be stopping animations that take too much CPU.
Do you get the "Stopping animation; animation possibly too big"
messages?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

(Please keep the debbugs address in the Cc headers -- otherwise your
mails won't reach the Emacs bug tracker.)

Madhavan Krishnan <krishnanmadhavan000@gmail.com> writes:

>>> In any case, Emacs should be stopping animations that take too much CPU.
>>> Do you get the "Stopping animation; animation possibly too big"
>>> messages?
>>
>> Nope, I was hardly able to scroll to the end of the page, at any rate my
>> cpu fan kicked in and I killed the entire session.
>
> My bad, I miss spoke yes the message does show "Stopping animation;
> animation possibly too big" (when you scroll the buffer message is gone)
> but the cpu usage is still not down (untill the buffer is killed
> manually)

I see Emacs gradually killing more and more of the animations until
Emacs has reached a usable state again, but it takes a while.  Perhaps
it should be more aggressive in stopping the animations...

Anyway, that's a side issue -- this should really be fixed by making the
GIF animations faster.  I'm not sure whether there's been any work done
on that -- it's been mentioned a few times, but possibly nobody has done
the work?

If I remember correctly, the current code will decode the entire GIF
file for each frame, which is pretty pessimal.  The ImageMagick version
of the animation code keeps a special cache to avoid doing all those
decodings, so perhaps that code can be reused...

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

[Madhavan Krishnan]

On 2020-12-15, 06:41 +0100, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> I see Emacs gradually killing more and more of the animations until
> Emacs has reached a usable state again, but it takes a while.  Perhaps
> it should be more aggressive in stopping the animations...
>
> Anyway, that's a side issue -- this should really be fixed by making the
> GIF animations faster.  I'm not sure whether there's been any work done
> on that -- it's been mentioned a few times, but possibly nobody has done
> the work?
>
> If I remember correctly, the current code will decode the entire GIF
> file for each frame, which is pretty pessimal.  The ImageMagick version
> of the animation code keeps a special cache to avoid doing all those
> decodings, so perhaps that code can be reused...

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)

Regards
-- 
Madhavan Krishnan

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

[Lars Ingebrigtsen]

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.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 4539 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 ones.  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.

I can think of two main ways to do 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 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!

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

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

[Stefan Kangas]

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

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

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

Ahh!  Yes, this has bit us before, I think (with image.el changing the
plist triggering recalculation unnecessarily.  Uhm...  4aa4a8f9 for
instance.

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

Yes, me too.

> 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 struct, such as
> using the "image struct" directly?

The problem is that it's not well-defined which elements in the plist
really affect display and which ones don't.  If you change :max-width of
an image plist, then it should definitely affect display, but if you
change :gazonk, then it shouldn't.

So perhaps we should just document which elements that affect display,
and put those fields into the struct?  (And then use the struct instead
of the plist for caching.)  That way, if somebody adds a new field that
affects display, then they know to also update the struct.

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

That would be a simpler solution -- image.el could easily just use a
hash table to keep track of the data.  But as you say, people will trip
over this elsewhere, too, because stashing data in the plist seems like
such an obvious thing to do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yes, Emacs's animation code needs to cache the images better.

I've now fixed this in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've now fixed this in Emacs 29.

That is, using this test image:

https://c.tenor.com/V5YkL4e_LOIAAAAC/couple-cuddle.gif

Emacs 28.1 uses about 50% CPU when displaying that image on this laptop.
After the caching changes, Emacs uses about 5% CPU.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> 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 struct, such as
>> using the "image struct" directly?
>
> The problem is that it's not well-defined which elements in the plist
> really affect display and which ones don't.  If you change :max-width of
> an image plist, then it should definitely affect display, but if you
> change :gazonk, then it shouldn't.

And as I was looking at the image stuff anyway, I've now gone ahead and
added filtering for the animation elements from the image cache.

In my test GIF image, Emacs used to use 30% CPU when animating it.
Creating the GIF cache took that down to 3%.  Fixing the first-level
image cache takes that down to 1%.

So finally Emacs should be usable when displaying a buffer with a bunch
of animated images.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 11 Apr 2022 18:49:46 +0200
> Cc: Madhavan Krishnan <krishnanmadhavan000@gmail.com>, 45224@debbugs.gnu.org
> 
> And as I was looking at the image stuff anyway, I've now gone ahead and
> added filtering for the animation elements from the image cache.
> 
> In my test GIF image, Emacs used to use 30% CPU when animating it.
> Creating the GIF cache took that down to 3%.  Fixing the first-level
> image cache takes that down to 1%.
> 
> So finally Emacs should be usable when displaying a buffer with a bunch
> of animated images.  :-)

Great, thanks!