41268 - imglib should cache chrome animations

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Warren Harris]

Right now the situation is completely insane when the throbber kicks in. 
Animated GIFs aren't cached by imagelib, which causes us to end up fetching them 
again with necko. This ends up reading the file from disk because the file: 
protocol doesn't cache in the memory cache either. This also causes contention 
for the file transport, consumes inordinate amounts of memory, takes a lot of 
time, etc.

Clearly, the throbber images should get cached somewhere, ideally by imagelib in 
their decoded state. We should extend the imagelib APIs to allow us to specify 
when an animated gif should be cached

Comment 1

[pnunn]

W:
I agree that the way the throbber works now is _less_
than ideal.

but I think it would make more sense to cache throbber data
in necko before it is decompressed. Decoding is cheap. We are
already running into memory requirement problems on the mac.

If we wanted to cache the decoded image data, what about using a
different format? We handle streaming image data fine. Most apps
use a 'doctored' format for internal images. We don't. but we
could in this case.

-P

Comment 2

[Warren Harris]

But that would require hooking up the file: and jar: protocols to the memory 
cache which doesn't seem like the right thing to do. It's very rare (except for 
the throbber case) that you'd try to retrieve the same file more than once. I 
think the image cache is a better place for this.

Comment 3

[David Hyatt]

I agree with warren.

Comment 4

[Chris McAfee]

nominating for nsbeta2, this could be a big performance win.

Comment 5

[Jim Roskind]

If this throbber is really sucking down IO and CPU cycles... is there any chance 
that we could slow the refresh rate on the images until we have the real 
architectuural fix in place?  For most users, a throbber that was a little 
slow/jerky would b ea nice tradeoff for a faster browser.

Comment 6

[Warren Harris]

I think this is clearly a beta3 thing, but not a beta2 thing.

Jar: You might be thinking of the time spent cycling through the animated gif, 
but I'm concerned about hitting the disk each time we start and end a load. 
Both should be looked at.

Comment 7

[leger]

Agreed...giving [nsbeta2-] and putting on nsbeta3 nominee radar.

Comment 8

[pnunn]

I have some changes for bug#30852 (an nsbeta2+) that will
give us a way to tell the throbber to _atleast_ get internal
images from the necko cache. 

I noticed today that all chrome and internal
images are requested with a 4402 and 4400 necko load attributes,
which is FORCE_VALIDATION. This means even non animated images are
not cached.

The only way imglib can validate the freshness of an image is to
tell it to not use the imgcache and go get it from necko. At this
point necko should know whether it can get the data from its cache,
or it should reload the data from the source (server, file, whatever).

And I agree that this is a beta3 issue.
-p

Comment 9

[pnunn]

Also be aware that all animations are more costly on X than other
platforms... and this may be due to the way we render on X.
Pav had some good insights into ways we might speed things up.
(window invalidation, reusing structures rather than using new
ones...etc.) 

Comment 10

[pnunn]

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

Comment 11

[Gagan]

neeti has a potential fix for this! Check with her. 
making this nsbeta3+

Comment 12

[Warren Harris]

What is it? I really want the fix here to be "make file: protocol use memory 
cache".

Comment 13

[pnunn]

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

Comment 14

[pnunn]

Changing summary to the generic problem.
-p

Comment 15

[Gagan]

Isn't this the same bug as bug 49097?

Comment 16

[pnunn]

Not really. I have to handle animations for the first time in the
imgcache.
-p

Comment 17

[pnunn]

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

Comment 18

[pnunn]

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

Comment 19

[pnunn]

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

Comment 20

[Phil Peterson]

Not holding PR3 for this, marking nsbeta3-. Please nominate for rtm if we really
have to fix this before shipping Seamonkey.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Nominating for RTM

Comment 22

[Eric Burley]

this bug blocks a theme authors ability to animate buttons on rollover.

Comment 23

[Gagan]

approving for rtm.

Comment 24

[pnunn]

I'm still working on this. Its a non trivial fix.
If we back out the PIN_CHROME changes by #undef'ing
PIN_CHROME, it will also unblock 
"theme authors ability to animate buttons on rollover".

The PIN_CHROME changes prevent the unloading of chrome
images in the img cache and was put in to  help performance.
Unfortunately, there are several unpleasant side effects.

Comment 25

[Simon Fraser [no longer active]]

Can we get enough of the necko memory cache working for chrome animations?

Comment 26

[Blake Ross]

Updating QA Contact

Comment 27

[Michael La Guardia]

Marking "needinfo". This seems like a bad thing, but we need a patch, review and
super review to clear for checkin.

Comment 28

[pnunn]

I am still working on this one. Its not ready for testing or
initial reviews.
-p

Comment 29

[Jason Eager]

After 2 weeks, is this dead for RTM?

Comment 30

[pnunn]

I'm working on it. I'm working on it.

Crashers have priority over performance and 
mozilla is demanding alot of attention these
days.

Thanks for asking,
-p

Comment 31

[Jason Eager]

Changing this from [rtm+ need info] to [rtm need info], since the [rtm+ need
info] state doesn't exist.

Comment 32

[Gagan]

No fix in sight for rtm.

Comment 33

[pnunn]

All pnunn bugs reassigned to Pav, who is taking over
the imglib.

Comment 34

[Stuart Parmenter]

new imagelib caches all chrome animations

Comment 35

[Stuart Parmenter]

er, not just chrome animations.  all animations including chrome ones.