Preference for animated image display (GIF89a, MNG)

VERIFIED FIXED in Future

Status

SeaMonkey
Preferences
P3
enhancement
VERIFIED FIXED
19 years ago
10 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: Akkana Peck)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

adding fauxpas to the cc list.

Updated

19 years ago
Severity: normal → enhancement

Updated

19 years ago
Summary: [HELP WANTED] "display animated GIFs (yes/no)" functionality (pref UI and backend work required) → [HELP WANTED] Preference for animated GIF display (yes/no) - (pref UI and backend work required)

Comment 2

19 years ago
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

19 years ago
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 ...
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.
spam: added self to cc list as this might affect my realm --if it's
implemented.

Updated

19 years ago
Keywords: helpwanted

Comment 6

19 years ago
Putting in sariuh as QA Contact.
QA Contact: leger → sairuh

Comment 7

19 years ago
16995 may be a duplicate

Comment 8

19 years ago
Assigning all open "nobody@mozilla.org" bugs to "leger@netscape.com" to weed 
thru.
Assignee: nobody → leger

Comment 9

19 years ago
*** Bug 16995 has been marked as a duplicate of this bug. ***
moving over to imagelib, as this is not general browser.
Component: Browser-General → ImageLib

Updated

19 years ago
Blocks: 16995

Updated

19 years ago
Hardware: PC → All

Updated

19 years ago
Summary: [HELP WANTED] Preference for animated GIF display (yes/no) - (pref UI and backend work required) → Preference for animated GIF display (yes/no)
Whiteboard: HELP WANTED → pref UI and backend work required

Comment 11

19 years ago
*** Bug 16995 has been marked as a duplicate of this bug. ***
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.
Assignee: leger → pnunn
QA Contact: sairuh → elig

Comment 13

19 years ago
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.
Assignee: pnunn → matt
Component: ImageLib → Pref UI
QA Contact: elig → sairuh

Comment 14

19 years ago
Bulk move of all Pref UI component bugs to new Preferences component.  Pref UI 
component will be deleted.
Component: Pref UI → Preferences

Updated

19 years ago
Target Milestone: --- → M19

Comment 15

18 years ago
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.
Whiteboard: pref UI and backend work required → Backend done, just need to create preference

Comment 16

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

Updated

18 years ago
Summary: Preference for animated GIF display (yes/no) → Preference for animated GIF display

Comment 17

18 years ago
Move to M20 target milestone.
Target Milestone: M19 → M20

Comment 18

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

Comment 19

18 years ago
Reassign since 21620 was pnunn's bug.
Assignee: matt → pnunn

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 20

18 years ago
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

18 years ago
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

18 years ago
Created attachment 9495 [details] [diff] [review]
animation preference UI

Comment 23

18 years ago
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

18 years ago
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.
Summary: Preference for animated GIF display → Preference for animated image display (GIF89a, MNG)

Comment 25

18 years ago
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

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

Comment 27

18 years ago
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

18 years ago
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?
Whiteboard: Backend done, just need to create preference → Nearly fixed.

Updated

18 years ago
Target Milestone: M20 → M17

Comment 29

18 years ago
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

18 years ago
Created attachment 10516 [details] [diff] [review]
Slightly improved patch
(Assignee)

Comment 31

18 years ago
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

18 years ago
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.

Updated

18 years ago
Keywords: helpwanted → nsbeta3
Whiteboard: Nearly fixed.
Target Milestone: M17 → M18

Comment 33

18 years ago
Not for nsbeta3
Whiteboard: [nsbeta3-]

Comment 34

18 years ago
*** Bug 52227 has been marked as a duplicate of this bug. ***
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".
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.
QA Contact: sairuh → tpreston

Updated

18 years ago
Keywords: mozilla0.9

Updated

18 years ago
Target Milestone: M18 → Future

Updated

18 years ago
Blocks: 61527
(Assignee)

Comment 37

18 years ago
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?
(Assignee)

Comment 38

18 years ago
Created attachment 21712 [details] [diff] [review]
Working patch (but gets pref every time)
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.
where are my manners?  I should have started that comment out with:  "Thanks for
reviving the fix."

Comment 41

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

Comment 42

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

Comment 43

18 years ago
reassigning to akk while pam is on sabbatical.
Keywords: nsbeta3 → nsbeta1
Whiteboard: [nsbeta3-]

Comment 44

18 years ago
Now I'll really do it...
Assignee: pnunn → akkana
Status: ASSIGNED → NEW
(Assignee)

Comment 45

18 years ago
Created attachment 21791 [details] [diff] [review]
Better patch (saves pref values, uses nsIObserver)
(Assignee)

Comment 46

18 years ago
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).
Status: NEW → ASSIGNED
(Assignee)

Comment 47

18 years ago
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?)
(Assignee)

Comment 48

18 years ago
Created attachment 21804 [details] [diff] [review]
Diff: make the decision in nsPresContext instead of imglib

Comment 49

18 years ago
Could you attach a unified diff (diff -u)?
(Assignee)

Comment 50

18 years ago
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.
(Assignee)

Comment 51

18 years ago
Created attachment 21848 [details] [diff] [review]
Diff -u, pres context, no statics

Comment 52

18 years ago
Created attachment 21886 [details] [diff] [review]
akkana's patch with folded logic, memory leak plugged, and charset string change removed

Comment 53

18 years ago
Created attachment 21892 [details] [diff] [review]
updated patch - ignore the previous one

Comment 54

18 years ago
Created attachment 21895 [details] [diff] [review]
once more...
(Assignee)

Comment 55

18 years ago
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.
(Assignee)

Comment 56

18 years ago
It's in -- woohoo!  Set
user_pref("image.animation_mode", "once");
or
user_pref("image.animation_mode", "none");
to see it.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 57

18 years ago
Filed bug 64831 to cover the UI.

Comment 58

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
(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

18 years ago
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

18 years ago
I'm interested. I'm reading. :-)
(Assignee)

Comment 64

18 years ago
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.
(Assignee)

Comment 65

18 years ago
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

18 years ago
I filed bug 65016 for crash.

Comment 67

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

Updated

18 years ago
Blocks: 66966
(Assignee)

Comment 68

18 years ago
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?
(Assignee)

Comment 69

18 years ago
Looks like this was a temporary thing.  Working again on the 2/2 build.  Whew!
*** Bug 68125 has been marked as a duplicate of this bug. ***

Comment 71

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

Comment 72

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

Comment 73

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

Comment 74

18 years ago
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

18 years ago
user_pref("image.animation_mode", "none") doesn't work on 2001040311/Linux.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 76

18 years ago
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.
(Assignee)

Comment 77

18 years ago
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.
Depends on: 74650

Comment 78

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

Comment 79

18 years ago
Thanks, closing this bug again since the new one tracks the regression.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 80

18 years ago
Verified
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey

Comment 81

10 years ago
I suggest reopening this, because the option to turn off animated GIFs has been removed from the Options menu in the Firefox 3 release.
You need to log in before you can comment on or make changes to this bug.