We invalidate if FrameAnimator::UpdateState marks the composited frame as valid, but we fail to do so if FrameAnimator::RequestRefresh does so.
The other place that sets the composited frame as valid is RasterImage::Decode. This call is actually redundant because the UpdateState call will do the same.
Even though we will invalidate when the decode produces results we could still draw incorrectly if something else invalidates. In which case we would only draw the part of the image that was invalidated. But that should actually be impossible as explained in the comment.
We invalidate if FrameAnimator::UpdateState marks the composited frame as valid, but we fail to do so if FrameAnimator::RequestRefresh does so.
The other place that sets the composited frame as valid is RasterImage::Decode. This call is actually redundant because the UpdateState call will do the same.
Even though we will invalidate when the decode produces results we could still draw incorrectly if something else invalidates. In which case we would only draw the part of the image that was invalidated. But that should actually be impossible as explained in the comment.
We draw nothing when the composited frame is invalid, so when we mark it valid we should invalidate. Usually the action that causes the composited frame to be valid will invalidate (ie RequestRefresh).
If the SurfaceCache discards our frames on another thread, the runnable that notifies us of that discard could race with a decode complete notification. So we can't rely on any ordering of SetDiscarded and NotifyDecodeComplete. Thus we must derive our state purely from the SurfaceCache (and mAnimationFinished from RasterImage).
We also update the image state in RequestRefresh (the main place where we use the state that is updated).
The other main place we use the state is GetCompositedFrame, but we don't update the state there. It should be fine because the only time this might lag behind reality is if the frames are discarded, and it should be fine to continue drawing the composited frame until the discard notification arrives.
The way that we tell that an animated image has all of its frames complete in the surface cache is less than ideal.
The SurfaceCache can discard on any thread at any time. So we could be in the middle of advancing frames of a fully decoded animated image and then the frames could disappear out from under us.
Making the code deal with that kind of a situation would make the logic very complicated. So instead just look up the frames once and pass them around, that way they never change during while we are advancing the frame.
Do this to allow GetTimeoutForFrame to be called for frames that haven't been decoded yet. Propagate a Maybe result where it makes sense. The remaining callers just bail if they get no return value. Many of them can just assert that they get a return value because they already got the same frame, so the timeout has to be available.
The logic is a little tricky because we have "Forever" timeouts that were sort of treated as error cases.
For animated images with finite animations we can finish running their animation. At which point we won't call RequestRefresh, and so we will never mark the composited frame as valid (since that is the only place we do that).
To fix this we mark the composited frame as valid when we finish decoding.
But we can do better than that, we can mark the composited frame as valid immediately when we create a new decoded since we are just drawing the final frame from now on.
When we allow animated images to be discarded we still want to track if the image has been fully decoded before, but it would be confusing to say that it is "done decoding" because that sounds like the image is currently decoded, even though it could be discarded at the time.
FrameAnimator::GetCompositedFrame is only ever called with the current animation frame index. This is good because it can return invalid results if it is called for some other frame number.
This means that in RasterImage::LookupFrame when we are asked to do a sync decode (if needed) we use WaitUntilComplete to wait until the frame is finished decoding. But we would actually return after the next progressive pass notified the monitor to wake up. Thus, we would draw a not-fully-decoded image even though the sync decode flag was passed.
The change in FrameAnimator means that we won't draw the next frame in an animated image until all progressive passes of that image are complete. This seems like what we want anyways.
There is one real use of IsImageComplete left, in imgFrame::Draw, where we need to know if the decoded image data covers the whole image frame. (There are a couple of uses of IsImageComplete in asserts.)
When storing ms, 32 bit ints can hold 2^32/1000/60/60/24 ~= 49 days. It's quite conceivable that someone would leave a tab in the background for 50 days.
GetSingleLoopTime returns -1 on exceptional cases but we used an unsigned int to hold the return value in AdvanceFrame. So the |loopTime > 0| check would succeed. Fortunately the |delay.ToMilliseconds() > loopTime| check would fail because loopTime was MAX_UNIT32, so we didn't do anything incorrect.
http://hg.mozilla.org/mozilla-central/rev/263980931d1b (bug 890743) changed GetSingleLoopTime from returning 0 (and uint32_t) to -1 (and int32_t) on exceptional cases. But the caller of GetSingleLoopTime wasn't updated.
Before the previous patch we would (wrongly) loop through the decoded frames even though we didn't have all of the frames of the animation. This had the beneficial side effect of advancing mCurrentAnimationFrameTime to aTime (the current time). With the previous patch we stop at the last decoded frame and don't advance mCurrentAnimationFrameTime, so it can lag behind. The problem with this is that when we have finished decoding we will then try to catch mCurrentAnimationFrameTime up, and this will jump us to a random point in the animation. So we need to advance mCurrentAnimationFrameTime ourselves.
If we were blocked on network/decoding then displaying the last available decoded frame is the correct frame to be displaying. So we are up to date. So we advance mCurrentAnimationFrameTime to the current time.
mImage->GetNumFrames() is the current number of decoded frames (that the RasterImage knows about), so it only represents the last frame of the animation if we are done decoding.
If we are not fully decoded, and we are on the last decoded frame, just stay on the last decoded frame. When more frames get decoded (or we determine that we are the last frame of the animation) we will advance.
One might expect that if |nextFrameIndex == mImage->GetNumFrames()| then |GetRawFrame(nextFrameIndex)| would return a null surface. But that is not the case because the decoding thread can insert frames into the surface cache that the RasterImage hasn't acknowledged yet (because it has to do so on the main thread, which we are currently running on).
This is why moving animated images to the surface cache is likely the cause of this bug.
This introduces an issue that is explained in, and fixed by the next patch.
Timothy Nikkel
Iris Hsiao
Andrew Osmond
Seth Fowler
Nicholas Nethercote
Chris Peterson