74169 - user pref "image.animation_mode" ignored

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Florin Iucha]

I have used user_pref("image.animation_mode", "once"); since Mozilla 0.8 .

Today I have downloaded the latest Mozilla build on Windows and the animated
gifs keep cycling.

It was better yesterday when all the animated gifs were black :)

Comment 1

[Florin Iucha]

user_pref("image.animation_mode", "none"); is ignored too...

Comment 2

[NilsE]

Confirming on Linux RH 6.2
Mozilla CVS pull 20010403.

As you said, it worked last week. "regression" keyword is appropriate.

Even worse, you can't manually stop the animation which ESC.

(I'll attach a test case.)

Comment 3

[NilsE]

Attachment: test case: animated GIF

Comment 4

[NilsE]

Sorry ... above should read "... with ESC".

Florin, could you change the summary to sth. more generic, like
"User pref  image.animation_mode  ignored"?

BTW, my setup: 
"Mozilla/5.0 (X11; U; Linux 2.2.18pre11-va1.8 i686; en-US; 0.8.1)"

Comment 5

[Sean Richardson]

It seems that the new Imglib2 code is not respecting the image.animation_mode
pref. See bug 17686, "Preference for animated image display (GIF89a, MNG)":
this regression is of the functionality added in the fix for that RFE.

Confirming, and adding comment in 17686.

Comment 6

[Akkana Peck]

Comments from bug 74650:
---
libpr0n doesn't check the
pres shell's GetImageAnimationMode().  

Here's how it was called in the old imglib:
http://lxr.mozilla.org/seamonkey/source/modules/libimg/src/if.cpp#1833

which was called from IL_GetImage on line 1918 of the same file.

I can add the call to libpr0n if you point me in the right direction (perhaps in
imgContainer::Init or imgContainer::StartAnimation?)
---
I've also added the test case from that bug here.

Comment 7

[Akkana Peck]

*** Bug 74650 has been marked as a duplicate of this bug. ***

Comment 8

[Matthias Versen [:Matti]]

*** Bug 74802 has been marked as a duplicate of this bug. ***

Comment 9

[Akkana Peck]

Attachment: Patch in libimg

Comment 10

[Akkana Peck]

Attachment: Patch to layout/base/src/nsImageFrame.cpp

Comment 11

[Akkana Peck]

Taking this bug -- I have a fix.

imgContainer doesn't have access to the pres context, so I made a new API on it
so that nsImageFrame could set the animation mode.  Then it turned out that the
GIF decoder was calling StartAnimation too early, so I moved that call into
imgContainer (it'll only be called in case of images which are actually
animated).  Finally, imgContainer had some apparently redundant code to start
the animation timers, which I replaced with a call to StartAnimation.  Animation
now works in all three modes.

Comment 12

[Akkana Peck]

Pavlov, could you please review this and make sure what I've done is kosher? 
Are you the sr'er for imglib?

Comment 13

[Simon Fraser [no longer active]]

Comments: 
1. I think the animation modes should be enums in the IDL
2. why this in nsGIFDecoder2:
  -  mImageContainer->StartAnimation();  ??

Not your changes, but:

   PRBool   mCurrentFrameIsFinishedDecoding;
   PRBool   mDoneDecoding;
   PRBool   mAnimating;
+  PRUint16 mAnimationMode;

should use PRPackedBools for space reasons.

Comment 14

[Akkana Peck]

I'm in the process of moving the enums into the idl as Simon suggested (though
unfortunately that touches a lot more files).  Stay tuned for new patch.

My patch removes StartAnimation from the nsGIFDecoder and instead handles that
within imgContainer, which seems more general and also allows us to control when
it happens (in the gif decoder it was happening too early, before any outside
class had a handle to the image and a chance to stop the animation).

The packed bool idea sounds like a good one -- I'll go ahead and make that
change in my tree, and the libpr0n reviewer (Pav?  Saari?) can veto if there's
some reason not to do that.

Comment 15

[Akkana Peck]

Attachment: Prospective patch for the future when libimg1 is gone

Comment 16

[Akkana Peck]

Attached a patch of what had to change when I moved the enums into libpr0n. 
This doesn't currently work -- it breaks files in the old libimg, which Pav says
are going away soon.  I suggest we hold off on that part of the patch, and go
with the original simple patches for now, then do the more sweeping fix after
0.9 when Pav removes the old libimg.

Comment 17

[Stuart Parmenter]

r=pavlov on patches 30633 and 30634.  Please get saari to r= the imgContainer 
patch since he has done more with it than I have.

Patch 30818 looks ok to me as well, except I don't think that:
+%{C++
+typedef short nsImageAnimation;
+%}
is needed, and since it isn't used in any of the imgContainer code, i'd prefer 
it not be there.

Get saari's r= and i'll find someone to sr= it.

Comment 18

[Simon Fraser [no longer active]]

sr=sfraser on the 4/12 patches.

Comment 19

[Akkana Peck]

Also got a verbal r=saari.  Checked in the 4/12 patches.  Leaving bug open for
the later patch, once the old imglib is excised from the build.  Resetting
target milestone and status whiteboard appropriately.

Comment 20

[NilsE]

Although this is fixed (and works on all pages I've tried so far), I wonder why
the blinking eyes on resource:///res/samples/test2.html still animate. Are local
resources excluded from this pref?

Comment 21

[Akkana Peck]

Yes, the code explicitly excludes resource and chrome urls, because we didn't
want this pref to turn off animation in the chrome (e.g. the throbber).

Comment 22

[Mirek]

It seems that in Moz 0.9.1. (build 2001060703 WinNT) this user_pref is still
ignored. I cannot find a way to stop the animated gifs.
Moreover the ESC and context stop are not working as well.

Comment 23

[Akkana Peck]

The pref is working for me.  Did someone reopen the bug, or did it not get
closed when I checked in the fix?  Mirek, what is your pref set to?

ESC / Stop not working is a different bug, assigned to Pavlov.