Status

()

defect
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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. =\
Attachment #8542941 - Flags: review?(tnikkel)
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
Attachment #8542941 - Attachment is obsolete: true
Attachment #8542941 - Flags: review?(tnikkel)
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.
Attachment #8545571 - Flags: review?(tnikkel)
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.
Attachment #8545663 - Flags: review?(tnikkel)
Attachment #8545577 - Attachment is obsolete: true
Attachment #8545577 - Flags: review?(tnikkel)
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?
Attachment #8545571 - Flags: review?(tnikkel) → review+
(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".
Attachment #8545572 - Flags: review?(tnikkel) → review+
Attachment #8545574 - Flags: review?(tnikkel) → review+
Attachment #8545663 - Flags: review?(tnikkel) → review+
Attachment #8545580 - Flags: review?(tnikkel) → review+
You need to log in before you can comment on or make changes to this bug.