17686 - Preference for animated image display (GIF89a, MNG)

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, enhancement, P3)

Description

[(not reading, please use [e-mail] instead)]

thanks to [e-mail] for
(http://slashdot.org/article.pl?sid=99/10/31/1337238) for this suggestion.

Comment 1

[(not reading, please use [e-mail] instead)]

adding fauxpas to the cc list.

Comment 2

[Sean Richardson]

This would really annoy advertisers - the first frame of an animated ad
banner often doesn't even identify the advertiser, let alone give a reason
to click (even in theory). That's not why I'd like to see this feature, though.

I'd like to see this for another reason. On sites like slashdot,
cnn, infoworld, etc, the ad banner can be the last thing to finish for
a modem-line user. Myself, I hit the STOP button if it looks like the page is
all there (if I haven't already clicked on a link), but I have seen many, many
people wait until the STOP button greys out before even scrolling.

If the TCP/IP connections for gifs were dropped as soon as the first frame was
displayed, the WWW wouldn't appear quite so World-wide-wait-ish to those users.

BTW, the connection *should* be dropped if no animation is to be displayed:
there is no reason to fetch the rest of the frames in the animated gif if they
aren't going to be displayed. I can see how this could complicate
implementation, though.

A refinement might be to just make the webserver(s) sending the animated
gifs wait to send the rest of the packets until everything else on the page
is finished (i.e., until the STOP button greys) - but this would be a feature
on a feature and would need a distinct pref ("Show animated gifs after page
finishes loading" in addition to "Show animated gifs" and "Don't show animated
gifs").

Another related preference may not scare marketers as much: "Display animated
gifs once only" - done right, this would leave the gif on the last frame.
This should be split off if anybody wants to work on it.

BTW, fauxpas does not appear to be on the cc list at present.

Comment 3

[Matthew T (active 1999-2002)]

Maybe fauxpas removed him-/her-self.

Anyway, IE offers the option to disable animations, so advertisers should be
grinning and bearing it already. And in Moz 4.x, the Stop button stays lit not
only while the GIF is loading, but for the whole of the first animation loop of
the graphic. This is overloading the Stop button (stop animations/stop loading).
Perhaps the Stop button needs a drop-down menu, like the Back button does ...

Comment 4

[(not reading, please use [e-mail] instead)]

when we do get around to implementing this, we'll have
to make sure that animated gifs that are chrome:// urls
still animate.

otherwise, the throbber will stop working.

Comment 5

[sairuh (rarely reading bugmail)]

spam: added self to cc list as this might affect my realm --if it's
implemented.

Comment 6

[leger]

Putting in sariuh as QA Contact.

Comment 7

[Mike Palczewski]

16995 may be a duplicate

Comment 8

[leger]

Assigning all open "[e-mail]" bugs to "[e-mail]" to weed 
thru.

Comment 9

[Doron Rosenberg (IBM)]

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

Comment 10

[Doron Rosenberg (IBM)]

moving over to imagelib, as this is not general browser.

Comment 11

[pnunn]

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

Comment 12

[Doron Rosenberg (IBM)]

reassigning to Imagelib owner, as leger has nothing to do with this.

In bug Bug 16995, there is a request to be able to right-mouse click on a
image,and select a "turn animation off" option.

Comment 13

[Eli Goldberg]

Technically, yes, she does. Do you see the keyword "helpwanted"? 

That means that this isn't an issue given to an engineer, but is a general "Help 
Wanted" enhancement request for anyone on the net who wishes to pick up, because 
nobody on Netscape's payroll has elected to implement it.

Reassigning to Pref UI, anyway, since ImageLib doesn't have anything to do with 
this enhancement request, either.

Comment 14

[leger]

Bulk move of all Pref UI component bugs to new Preferences component.  Pref UI 
component will be deleted.

Comment 15

[Ben Bucksch (:BenB)]

Backend work done with bug 33810.

Removing yes/no from SUMMARY, as we also have to option to show one loop.

Sean's suggestion should go into a new bug, I think. Propbaly needs more backend
work. Might not even make sense, see cache.

Bug 11875 is related.

Comment 16

[Ben Bucksch (:BenB)]

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

Comment 17

[don]

Move to M20 target milestone.

Comment 18

[Simon Fraser [no longer active]]

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

Comment 19

[Simon Fraser [no longer active]]

Reassign since 21620 was pnunn's bug.

Comment 20

[Davor Cubranic]

From 21620 (originally from 31978):

"It'd be nice to have a pref that would put a bound on "infinite" so animated
gifs would stop automatically after the specified time. At least on Unix, older
versions of the browser used a lot of CPU cycles for animations even when the
window was covered or minimized."

Comment 21

[Davor Cubranic]

The attached patch implements preference-based animation of images, either full
animation, one loop only, or no animation. There are three parts: preferences
"advanced.animate_images" and "advanced.animate_images_once" are created in
modules/libpref/init/all.js, animation options are added to the preference panel
in xpfe/components/prefwindow/resources/content/pref-advanced.xul (names in
pref-advanced.dtd), and lastly, an image's animate_request field of its
il_container is set accordingly in modules/libimg/src/if.cpp.

Admittedly, the patch is rather crude, and I see it more as a starting point for
this functionality. For example, the animation mode could also be determined by
setting mImageAnimationMode of nsPresContext, but I don't know the code well
enough to be sure that there is always 1-1 mapping between presContext and an
image. Also, I'd like hear opinions about the preference UI -- I'm not too sure
whether using two checkboxes (the loop-once one indented to indicate it's a
sub-option) is better than perhaps three radiobuttons, or something else
entirely.

Comment 22

[Davor Cubranic]

Attachment: animation preference UI

Comment 23

[Matthew T (active 1999-2002)]

Three radio buttons, or a popup menu if pressed for space. Alternative 
presentations:

* (Maximum space)

  When an image is animated,
  ( ) show only the first frame
  ( ) show the animation once
  (*) allow repeated looping

* (Less space)

  Image animation: ( ) none  ( ) one loop only  (*) allow repeated looping

* (Least space)

  Image animation: [allow repeated looping :^]

We probably want a `Stop Animation'/`Start Animation' context menu item for 
animated images, too, since an animated image might be at the bottom of the page 
and have finished its single loop by the time I get to it. The menu item would 
override this preference, just for that image on that occasion.

Comment 24

[Jeremy M. Dolan]

Integrating the bound:

When an image is animated,
  ( ) show only the first frame
  (*) allow repeated looping
  ( ) show the animation [  1] time(s).

Also, remove the multiple pref's the patch added. We only need one integer. -1
for infinate looping, 0 for no looping (first frame), 1+ for specific run count.

Comment 25

[Matthew T (active 1999-2002)]

Tempting ... but with prefs as with variables, overloading their purpose is 
usually a bad idea. I might vacillate between preferring (a) 3 loops and (b) 
infinite loops -- switching between the two depending on my mood. If the 
underlying prefs were combined, the UI wouldn't remember that my favorite loop 
limit was 3, which would be annoying.

Comment 26

[Jeremy M. Dolan]

Right, scratch that idea. I originally thought of combining the two that were
there before I saw the bound idea.

Comment 27

[pnunn]

Hi all.
Before we get too carried away.... the mechanism in
place has 3 options: normal (whatever the animation specifies),
no animation (display first frame only), and loop once.

This bug is about hooking up this mechanism (associated with
the nsPresContext) with a preference.
-p

Comment 28

[Ben Bucksch (:BenB)]

Davor, thanks for fixing this!

Can you or Matthew create a new patch with the improved pref UI, so we can get
this in, please?

Comment 29

[Ben Bucksch (:BenB)]

I removed some glitches in the patch, e.g. the hardcoded default didn't match
the one in all.js and the default pref was never queried.

However, all this has no effect for me. If I go to
<http://www.bucksch.org/1/projects/mozilla>, the banner loops infinitely,
although there is |user_pref("advanced.animate_images", false);| in my prefs.js.
Does it work for anybody?

Comment 30

[Ben Bucksch (:BenB)]

Attachment: Slightly improved patch

Comment 31

[Akkana Peck]

I've appled the patch, and it doesn't work for me, either.  As far as I can
tell, IL_GetImage() is never getting called at all, either for chrome or content
images, so checking the pref in that routine isn't helping.

Comment 32

[Davor Cubranic]

I'm sorry I can't go back to check this on the machine where I developed the 
original patch, because it really is puzzling. I had the check for chrome URL's 
because they were being processed, and I was getting lockups if I tried to make 
them unanimated. This was on a stock RedHat Linux 6.2 to which I won't have 
access until September. I'll try to set up Mozilla on a Windows machine that I 
have during the summer and see whether I can figure out what's happening.

Comment 33

[Gagan]

Not for nsbeta3

Comment 34

[Decklin Foster]

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

Comment 35

[Matthew Tuck [:CodeMachine]]

This looks like a partial dupe of bug #19258.  Not sure which to close out as
both are good, probably keep this one and leave the other for blink. 

Someone needs to reconcile the two.  If we keep this one, be sure to take the
other's keywords into this bug, in particular "access".

Comment 36

[Matthew Tuck [:CodeMachine]]

Would anything need to be done to the plugin architecture to ensure plugins
could read this pref?

I fail to see the difference to the user between aniGIFs and soundless-MPEGs, so
this is likely to be something a plugin will be interested in.

Comment 37

[Akkana Peck]

I've been waiting for this long enough, and decided it was time to go in and
figure out why the patch didn't work.  I still don't know why IL_GetImage() is
never called, but it isn't; but animation can in fact be turned off in the gif
code in gif.cpp, so I've put a somewhat modified version of the patch into that
file.  I changed the name of the prefs to image.animate and image.animate_once,
since advanced.* didn't seem very helpful, but could change them again if anyone
has strong preferences.

One thing about this patch: it checks the prefs every time a new gif is
initialized, which is inefficient; it would probably be better to cache the pref
and add an updater.  I can do that any time if there's any chance of getting a
review; anyone know anyone qualified to review imglib code since Pam is on
sabbatical?

Comment 38

[Akkana Peck]

Attachment: Working patch (but gets pref every time)

Comment 39

[(not reading, please use [e-mail] instead)]

tor might be able to help with the review.

you should keep that pref values around (as a static PRBools?) and set up a pref
callback.  I wouldn't check it in otherwise.

adding alecf to the cc list.

Comment 40

[(not reading, please use [e-mail] instead)]

where are my manners?  I should have started that comment out with:  "Thanks for
reviving the fix."

Comment 41

[tor]

It's likely I'm missing something obvious, but why not make PresContext
a PrefListener and call nsPresContext::SetImageAnimationMode()?

Comment 42

[Alec Flett]

I agree, save it in a member variable or something, then use prefcallbacks (or
preferably, use the nsIObserver system)

Comment 43

[Blake Ross]

reassigning to akk while pam is on sabbatical.

Comment 44

[Blake Ross]

Now I'll really do it...

Comment 45

[Akkana Peck]

Attachment: Better patch (saves pref values, uses nsIObserver)

Comment 46

[Akkana Peck]

Tor: in answer to your question about using nsPresContext, it was because none
of us knew about that. :-)  Thanks for the suggestion!  It does seem to work
there, and makes more sense to put the code there; then we leave the door open
for turning animations back on selectively, e.g. it might be reasonable to turn
animations on when we're pointing directly at an image rather than at an html
page with lots of embedded images, so that the user has a way of seeing selected
animations).

Stand by for another patch -- I'll move my pref updating code into nsPresContext
and do the check in nsPresContext::SetShell (since that's when we actually have
the scheme so we can make sure we don't turn off animations in chrome).

Comment 47

[Akkana Peck]

Here's a patch that puts the logic in the pres context.  It doesn't actually
cache the pref between pages, though; it allows for that (the pref value is
static) but since nsPresContext doesn't cache any other prefs, but gets them
again every time, I wasn't sure it was appropriate to add a mechanism to do that
just for one pref.  But it's easy to do at this point (though I have to store a
"not set yet" value, either by inventing a bogus nsImageAnimation value or by
adding an extra NotInitializedYet boolean ... Opinions?)

Comment 48

[Akkana Peck]

Attachment: Diff: make the decision in nsPresContext instead of imglib

Comment 49

[tor]

Could you attach a unified diff (diff -u)?

Comment 50

[Akkana Peck]

Oops!  I thought I had used -u when I made that diff.  Turns out my finger
slipped and did -i instead.  -u coming.

But thinking about it last night, I decided that the half measure of using a
static to hold the preference's value, but checking it at the same time as the
other prefs which aren't static, didn't make sense.  I'm going to get rid of the
static-ness of it and treat it just like PresContext treats all its other prefs.

Comment 51

[Akkana Peck]

Attachment: Diff -u, pres context, no statics

Comment 52

[tor]

Attachment: akkana's patch with folded logic, memory leak plugged, and charset string change removed

Comment 53

[tor]

Attachment: updated patch - ignore the previous one

Comment 54

[tor]

Attachment: once more...

Comment 55

[Akkana Peck]

Tor and I both like his latest patch (01/05/01 17:45), which fixes several
problems in my patch).  I haven't tested the pref callback because I'm not sure
how to set a pref live.  Blake (or anyone else in earshot), how hard would it be
to add a UI for this pref, maybe somewhere in Advanced or Debug?

Adding buster, requesting sr.  We want to make sure we're doing this in the
right place, that SetShell when the URL is set is a reasonable place to put this
code.

Comment 56

[Akkana Peck]

It's in -- woohoo!  Set
user_pref("image.animation_mode", "once");
or
user_pref("image.animation_mode", "none");
to see it.

Comment 57

[Blake Ross]

Filed bug 64831 to cover the UI.

Comment 58

[buster]

akkana:  if you haven't already done so, you should post a note onto the
newsgroups (n.p.m.layout.checkins at least, maybe browser too) so folks know
it's there and can try it out.

Comment 59

[buster]

another thing: is there also a bug to cover the perf enhancement that we should
check to see if the animation is visible in the scroll frame, and avoid decoding
frames if it isn't?

Comment 60

[NilsE]

Now that the feature is in, I tried it with Build 2001011008 (Linux 2.2).
animation_mode "once" works like a charm, but if I set it to "none" I get
frequent crashes on pages that use animated GIFs. Try setting it to "none" and
visit Slashdot. It doesn't always crash - does it depend on the GIF?

Anyone?

Comment 61

[NilsE]

(Sorry, I tried to create an attachment, but no avail. More info on request.)

As written before, force reloading a page when animation mode is "none" crashes
mozilla for me after (on average) 5-7 reloads (100% reproducible). Tried with
CVS snapshot from 9 a.m. GMT+1, Jan 11, 2001 on RedHat 6.2. Here's what gdb says:

....
Document http://www.slashdot.org/ loaded successfully
###!!! ASSERTION: frame already has posted event: '!*FindPostedEventFor(aFrame)',
 file nsFrameManager.cpp, line 963

###!!! ASSERTION: frame already has posted event: '!*FindPostedEventFor(aFrame)',
 file nsFrameManager.cpp, line 963
###!!! Break: at file nsFrameManager.cpp, line 963
WARNING: not calling OnDataAvailable, file nsAsyncStreamListener.cpp, line 410

Program received signal SIGSEGV, Segmentation fault.
0x4152b213 in nsCSSFrameConstructor::CantRenderReplacedElement (this=0x85f27d0, a
PresShell=0x87db068, aPresContext=0x87f4b68, aFrame=0x87522cc) at nsCSSFrameConst
ructor.cpp:10248

Comment 62

[NilsE]

Now that a have a recent gdb 5 snapshot, I could provide a backtrace of a crash.
But is anyone reading this at all as the bug is "resolved"? Let alone
interested? Should I open another bug? Hm ....

Comment 63

[Mark Slater]

I'm interested. I'm reading. :-)

Comment 64

[Akkana Peck]

I'm reading, but I haven't been able to reproduce the crash.  I've been running
my dogfood optimized build with animation mode None, and haven't crashed once.

If someone can get a stack trace of the crash, you bet I'll look into it, but
with a crash that I can't reproduce, and without a stack trace, it's hard to do
anything useful.

Might be a good idea to open a new bug anyway rather than continuing the crash
debugging in this bug ... assign it to me.

Comment 65

[Akkana Peck]

nsCSSFrameConstructor:10248 is:
  aFrame->GetParent(&parentFrame);
and a simple null check wouldn't help here, because aFrame isn't null.
The crash may be something subtle like imglib sets a flag to stop loading on the
image, and then somewhere along the line some part of the image is deleted
prematurely but there are still netlib/layout callbacks operating on it.  Or
something like that.

Anyway, please do file a new bug (with repro instructions if you can find any,
and as much in the way of stack traces as you can get), assign it to me, cc
pnunn (it might have to wait until she comes back from sabbatical, since she
understands imglib a lot better than I do).

Comment 66

[Koike Kazuhiko]

I filed bug 65016 for crash.

Comment 67

[Ben Bucksch (:BenB)]

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

Comment 68

[Akkana Peck]

Piffle!  This seems to have regressed.  The code hasn't been backed out, the
pres context is still tracking the preference and setting mImageAnimationMode to
the right thing, but it looks like the pres context's mImageAnimationMode is no
longer being respected by imagelib (or someone -- I'm not sure where the
regression is).

Anyone hear of any recent changes that might have affected this?

Comment 69

[Akkana Peck]

Looks like this was a temporary thing.  Working again on the 2/2 build.  Whew!

Comment 70

[Doron Rosenberg (IBM)]

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

Comment 71

[RC]

Exactly how does one get this to work?  Tried setting
user_pref("image.animation_mode", "once"); in prefs.js and nothing happened.

Comment 72

[Lasse Kärkkäinen]

I think there should be option (in rightclick popup) to load all frames of
specific picture, even when in "none" animation mode.

Comment 73

[Akkana Peck]

Filed bug 69405 on the request for a more immediate UI to control animation.

Comment 74

[robbe]

This has regressed in 2001040213 (linux), perhaps due to libpr0n which is now
the default. I set "image.animation_mode" to either "once" or "none", but anim
gifs are not influenced in any way.

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=22329
can be used as a testcase.

(Why am I not allowed to reopen this bug? Filing a new dup would certainly be
worse, and everyone is granted that right. The mind boggles...)

Comment 75

[Koike Kazuhiko]

user_pref("image.animation_mode", "none") doesn't work on 2001040311/Linux.

Comment 76

[Akkana Peck]

Actually, filing a new bug is more appropriate in this case.  What happened is
that the new imglib landed, and I suspect that turning off image animation isn't
implemented yet.  I'll talk to Pavlov and find out if there's already a bug
tracking that issue; if not, I'll file one and help with implementing it.  That
makes much more sense than reopening every old bug that depends on image
animation control when it's only the control mechanism that regressed.

I'll leave this as REOPENED until I'm sure there's another open bug tracking the
issue, though, and I'll note the new bug # here before closing again.

Comment 77

[Akkana Peck]

Filed bug 74650 for checking the pres shell's animation status in libpr0n.
But I'm not entirely sure the pref is being set at the right place any more;
just to be sure, I'll keep this bug open until that bug is fixed and we can
verify that the pref is working again.

Comment 78

[Sean Richardson]

Bug 74169, "user pref "image.animation_mode" ignored", regression, is tracking 
the issue this bug was reopened for. Move discussion there?

Comment 79

[Akkana Peck]

Thanks, closing this bug again since the new one tracks the regression.

Comment 80

[Terri Preston]

Verified

Comment 81

[John David Galt]

I suggest reopening this, because the option to turn off animated GIFs has been removed from the Options menu in the Firefox 3 release.

Comment 82

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/49bcf5553f20
[fenix] For https://github.com/mozilla-mobile/fenix/issues/17686 - Use a custom behavior to scroll InfoBanner with the top toolbar