In bug 1116733 we made imgFrame accessors do more locking. The patches in that bug tried to improve efficiency somewhat, but FrameAnimator still takes the lock a lot. To fix that in a really clean way, a major refactoring of FrameAnimator is needed, but I don't have time to do it right now. This bug is intended to reduce the amount of locking FrameAnimator does as much as possible without major changes to its design.

This patch reduces locking by three strategies: - GetSingleLoopLength caches its result. - Pure functions that don't do anything that requires locking replace existing functions that required locking where possible. This is used for GetCurrentImgFrameEndTime, which turns into GetFrameEndTime, and GetTimeoutForFrame, which turns into NormalizedTimeout, although the original API also remains since code outside FrameAnimator calls it. - For code in the main set of functions (RequestRefresh, AdvanceFrame, and DoBlend), I've used the approach of doing the things that require locking a single time, in RequestRefresh, and then passing the results into AdvanceFrame and DoBlend. This does have the unfortunate side effect of making RequestRefresh in particular much uglier, I'm afraid. As I mentioned above, doing this cleanly will require a more substantial refactoring that I don't have time to do. =\

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=f51802df097e

A lot of those failures seem to have come from patches earlier in my patch stack. Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=b2d2cbd1d242

I decided to split up this patch to make it easier to debug and review. These patches contain the same stuff that the original single patch did.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=67b12a9798b2

Fixed a bug: we should make sure that we're done decoding before assuming that being at the end of the frame list means we should loop.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=b8002e1cbd48

Comment on attachment 8545571 [details] [diff] [review] (Part 1) - Return more information from imgFrame::AnimationData Is there a reason we need the AnimationData() (no arguments) constructor?

(In reply to Timothy Nikkel (:tn) from comment #12) > Comment on attachment 8545571 [details] [diff] [review] > (Part 1) - Return more information from imgFrame::AnimationData > > Is there a reason we need the AnimationData() (no arguments) constructor? Yeah, but it doesn't show up until part 4. =) It's so we can declare an AnimationData object outside of the loop in FrameAnimator::RequestRefresh, which allows us to share the same object between loop iterations. That makes sense, because in one iteration a frame is the "next frame", and in the next iteration it's the "previous frame".