bug#47895: 28.0.50; Emacs should only animate images that are visible

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Emacs is generally slow about calculating animated images, and that
should be fixed (there's a separate bug report for that), but there's
one thing Emacs could be doing:

Not animating images that are off screen.

I think most web browsers do this -- to avoid using too much CPU, which
is the same problem we have in Emacs: Opening a web page with many large
animated GIFs usually makes Emacs very slow.


In GNU Emacs 28.0.50 (build 78, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-04-11 built on xo
Repository revision: 751e801f90339480ea43fc2237fc45c8eb39bd6f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Debian GNU/Linux bullseye/sid


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

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 19 Apr 2021 20:19:19 +0200
> 
> Emacs is generally slow about calculating animated images, and that
> should be fixed (there's a separate bug report for that), but there's
> one thing Emacs could be doing:
> 
> Not animating images that are off screen.
> 
> I think most web browsers do this -- to avoid using too much CPU, which
> is the same problem we have in Emacs: Opening a web page with many large
> animated GIFs usually makes Emacs very slow.

I'd be interested to see a backtrace for such animation of an image
that is not on display.  In general, since animation is triggered (I
think) when the image is being prepared for redisplay, what you
describe shouldn't be happening.  If it dies indeed happen, I have a
couple of guesses how that could be triggered, but it would be nice to
see evidence.

Thanks.

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd be interested to see a backtrace for such animation of an image
> that is not on display.  In general, since animation is triggered (I
> think) when the image is being prepared for redisplay, what you
> describe shouldn't be happening.  If it dies indeed happen, I have a
> couple of guesses how that could be triggered, but it would be nice to
> see evidence.

I unfortunately don't have the time to debug right now, but it's easy
enough to reproduce:

(progn
  (eww "https://lars.ingebrigtsen.no/wp-content/uploads/2018/03/candid.gif")
  (bury-buffer))

This will leave you with an Emacs that uses at least 99% CPU, even if
the eww buffer isn't even displayed.  (At least on this Debian/bullseye
system, starting from -Q.)

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

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 47895@debbugs.gnu.org
> Date: Mon, 19 Apr 2021 22:49:55 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'd be interested to see a backtrace for such animation of an image
> > that is not on display.  In general, since animation is triggered (I
> > think) when the image is being prepared for redisplay, what you
> > describe shouldn't be happening.  If it dies indeed happen, I have a
> > couple of guesses how that could be triggered, but it would be nice to
> > see evidence.
> 
> I unfortunately don't have the time to debug right now, but it's easy
> enough to reproduce:
> 
> (progn
>   (eww "https://lars.ingebrigtsen.no/wp-content/uploads/2018/03/candid.gif")
>   (bury-buffer))
> 
> This will leave you with an Emacs that uses at least 99% CPU, even if
> the eww buffer isn't even displayed.  (At least on this Debian/bullseye
> system, starting from -Q.)

Thanks, I will take a look when I have time.

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 47895@debbugs.gnu.org
> Date: Mon, 19 Apr 2021 22:49:55 +0200
> 
> (progn
>   (eww "https://lars.ingebrigtsen.no/wp-content/uploads/2018/03/candid.gif")
>   (bury-buffer))

The timer set up by image.el keeps "displaying" the animated GIF.

In this simple case, we could use

 (get-buffer-window (plist-get (cdr image) :animate-buffer) 'visible)

in image-animate-timeout to see if the buffer is displayed in any
window.  The harder questions are:

  . if the buffer is not displayed, what to do with the timer?
    continue running it? if so, how to interpret the LIMIT arg?

  . what if the window _is_ displayed, but the image is not visible?
    I think we'd need to record the image's buffer position in its
    plist, so that we could use pos-visible-in-window-p to find out
    whether the image is visible

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> The timer set up by image.el keeps "displaying" the animated GIF.

Yup.  But if the image isn't displayed, why does this take any time?
That is, the image.el code increases the :index in the image spec, and
then calls force-window-update, and it's presumably this that takes
time?  Even if the image isn't displayed?  I find that part rather
unexpected.

Hm...  No, even without the force-update, Emacs uses 100% CPU.  I've
done some more testing, and even if image-animate-timeout just does:

  (plist-put (cdr image) :animate-tardiness
             (+ (* (plist-get (cdr image) :animate-tardiness) 0.9)
                (float-time (time-since target-time))))

and then re-runs itself, it'll use 100% CPU.  This seems to indicate
that any alteration of the image plist leads to Emacs re-computing the
image -- even if it isn't displayed?  Both of these things seem
unexpected: 1) Altering a plist item that's not relevant for the display of
the image shouldn't lead to an image recomputation, and 2) if the image
isn't displayed, it shouldn't be recomputed anyway.

I guess 1) is because the redisplay code can't find the image in the
image cache -- because it has no concept of "this is an image-relevant
plist item" -- it just computes a hash of all the properties.

> In this simple case, we could use
>
>  (get-buffer-window (plist-get (cdr image) :animate-buffer) 'visible)
>
> in image-animate-timeout to see if the buffer is displayed in any
> window.  The harder questions are:
>
>   . if the buffer is not displayed, what to do with the timer?
>     continue running it? if so, how to interpret the LIMIT arg?

I'd keep interpreting that the same -- that is, count down, even if the
image isn't displayed.

>   . what if the window _is_ displayed, but the image is not visible?
>     I think we'd need to record the image's buffer position in its
>     plist, so that we could use pos-visible-in-window-p to find out
>     whether the image is visible

Or just compute the position on each iteration -- the image may change
its position if more text is inserted, for instance.

But I'm still wondering about why this doesn't just work
"automatically" -- if we could handle this in the redisplay code, that
would be more natural.

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

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 47895@debbugs.gnu.org
> Date: Sun, 25 Apr 2021 20:48:47 +0200
> 
> Hm...  No, even without the force-update, Emacs uses 100% CPU.

Beware: changing that might make the timer run more frequently, so you
might see CPU usage soar even though Emacs does almost nothing.

>   (plist-put (cdr image) :animate-tardiness
>              (+ (* (plist-get (cdr image) :animate-tardiness) 0.9)
>                 (float-time (time-since target-time))))
> 
> and then re-runs itself, it'll use 100% CPU.  This seems to indicate
> that any alteration of the image plist leads to Emacs re-computing the
> image -- even if it isn't displayed?  Both of these things seem
> unexpected: 1) Altering a plist item that's not relevant for the display of
> the image shouldn't lead to an image recomputation, and 2) if the image
> isn't displayed, it shouldn't be recomputed anyway.

We access a different frame of the GIF image, so that would mean
regenerating the pixmap for the image, no?

> > In this simple case, we could use
> >
> >  (get-buffer-window (plist-get (cdr image) :animate-buffer) 'visible)
> >
> > in image-animate-timeout to see if the buffer is displayed in any
> > window.  The harder questions are:
> >
> >   . if the buffer is not displayed, what to do with the timer?
> >     continue running it? if so, how to interpret the LIMIT arg?
> 
> I'd keep interpreting that the same -- that is, count down, even if the
> image isn't displayed.

But then if and when the image becomes visible, it won't show the
animation, because it already reached the LIMIT.  Right?

> >   . what if the window _is_ displayed, but the image is not visible?
> >     I think we'd need to record the image's buffer position in its
> >     plist, so that we could use pos-visible-in-window-p to find out
> >     whether the image is visible
> 
> Or just compute the position on each iteration -- the image may change
> its position if more text is inserted, for instance.

Sure, but even if the position doesn't change we currently cannot tell
if the image is visible.  We have pos-visible-in-window-p, but that
needs a buffer position -- which is why I suggest to record that
position in the image.

> But I'm still wondering about why this doesn't just work
> "automatically" -- if we could handle this in the redisplay code, that
> would be more natural.

Animation doesn't work in redisplay, it works in this code I pointed
to.

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hm...  No, even without the force-update, Emacs uses 100% CPU.
>
> Beware: changing that might make the timer run more frequently, so you
> might see CPU usage soar even though Emacs does almost nothing.

It's still updating at the same rate (25 times a second for the image in
question).

> We access a different frame of the GIF image, so that would mean
> regenerating the pixmap for the image, no?

It does, but why does that happen before redisplay has decided to
display the image?

>> I'd keep interpreting that the same -- that is, count down, even if the
>> image isn't displayed.
>
> But then if and when the image becomes visible, it won't show the
> animation, because it already reached the LIMIT.  Right?

Yes.  I think that's fine -- you've asked for X repetitions, and you get
X repetitions, whether it's shown or not.

>> Or just compute the position on each iteration -- the image may change
>> its position if more text is inserted, for instance.
>
> Sure, but even if the position doesn't change we currently cannot tell
> if the image is visible.  We have pos-visible-in-window-p, but that
> needs a buffer position -- which is why I suggest to record that
> position in the image.

Sure.  Could make it a marker, I guess.

>> But I'm still wondering about why this doesn't just work
>> "automatically" -- if we could handle this in the redisplay code, that
>> would be more natural.
>
> Animation doesn't work in redisplay, it works in this code I pointed
> to.

The code just alters some elements in the image plist.  It's unexpected
that this should lead to Emacs doing a lot of work -- unless it's
actually displaying the image.

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

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Alan Third]

(I don't think this sent originally, apologies if it shows up twice.)

On Sun, Apr 25, 2021 at 09:07:48PM +0200, Lars Ingebrigtsen wrote:
> 
> The code just alters some elements in the image plist.  It's unexpected
> that this should lead to Emacs doing a lot of work -- unless it's
> actually displaying the image.

Since the image is being loaded whether it's displayed or not it may
be worth checking if it's also being flushed from the cache every time
through the animation. If so the high CPU usage is presumably due to
Emacs having to reload the frame every time.

-- 
Alan Third

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> Since the image is being loaded whether it's displayed or not it may
> be worth checking if it's also being flushed from the cache every time
> through the animation. If so the high CPU usage is presumably due to
> Emacs having to reload the frame every time.

Yup; should also be checked.  (I introduced a separate cache for this in
the ImageMagick case many years ago, but the non-ImageMagick one doesn't
have an animation cache, if I remember correctly.)

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

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 47895@debbugs.gnu.org
> Date: Sun, 25 Apr 2021 21:07:48 +0200
> 
> > We access a different frame of the GIF image, so that would mean
> > regenerating the pixmap for the image, no?
> 
> It does, but why does that happen before redisplay has decided to
> display the image?

Because image-animate-timeout calls image-metadata (via
image-multi-frame-p), which calls lookup_image, which regenerates the
pixmap.

> >> I'd keep interpreting that the same -- that is, count down, even if the
> >> image isn't displayed.
> >
> > But then if and when the image becomes visible, it won't show the
> > animation, because it already reached the LIMIT.  Right?
> 
> Yes.  I think that's fine -- you've asked for X repetitions, and you get
> X repetitions, whether it's shown or not.

But no repetitions were actually displayed yet, so won't this be
confusing?  Shouldn't we start counting only when the image is
actually visible?

> > Animation doesn't work in redisplay, it works in this code I pointed
> > to.
> 
> The code just alters some elements in the image plist.  It's unexpected
> that this should lead to Emacs doing a lot of work -- unless it's
> actually displaying the image.

It's computing the image, see above.

bug#47895: 28.0.50; Emacs should only animate images that are visible

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Because image-animate-timeout calls image-metadata (via
> image-multi-frame-p), which calls lookup_image, which regenerates the
> pixmap.

Aah!  Thanks; image.el doesn't have to keep calling that function -- it
can just call it once and then stash the data in the image plist.

I've now done that change on the trunk, and my test case went from using
100% CPU to using 7% CPU, which is an improvement.  :-)

Virtually all of the remaining CPU usage comes from the call to
`force-window-update' -- and I guess that shouldn't be called if the
buffer isn't displayed in a window.  Let's see...

Yup, with that change, the CPU usage went down to 2%.

So I think that this problem is now fixed, and I'm closing this bug report.

> But no repetitions were actually displayed yet, so won't this be
> confusing?  Shouldn't we start counting only when the image is
> actually visible?

I don't really have an opinion here -- but the image animation code
hasn't taken this into consideration before, so that would be a change
in behaviour.  

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