We were marking them used even if only a decode was requested.
This can cause us to hold extra decoded copies of the image around because we have a tendency to request decode at the intrinsic size.
In FrameAnimator::RequestRefresh and AdvanceFrame, we currently create
several RawAccessFrameRef objects to the same frames, either to get
timeouts or perform the blending. With some tweaking, we can avoid
requesting the same frame more than once. This will avoid mutex locks on
the surface provider and the frame itself.
DrawableSurface only exposes DrawableFrameRef to its users. This is
sufficient for the drawing related code in general, but FrameAnimator
really needs RawAccessFrameRef to the underlying pixel data (which may
be paletted). While one can get a RawAccessFrameRef from a
DrawableFrameRef, it requires yet another lock of the imgFrame's mutex.
We can avoid this extra lock if we just allow the callers to get the
right data type in the first place.
All animated images on a page are currently registered with the refresh
driver and advance with the tick refresh. These animations may not even
be in view, and if they are large and thus cause redecoding, cause a
marked increase in CPU usage for no benefit to the user.
This patch adds an additional flag, mCompositedFrameRequested, to the
AnimationState used by FrameAnimator. It is set to true each time the
current animated image frame is requested via
FrameAnimator::GetCompositedFrame. It is set to false each time the
frame is advanced in FrameAnimator::AdvanceFrame (via
FrameAnimator::RequestRefresh). If it is true when
FrameAnimator::RequestRefresh is called, then it will advance the
animation according to the normal rules. If it is false, then it will
set the current animation time to the current time instead of advancing.
This should not cause the animation to fall behind anymore or skip
frames more than it does today. This is because if
FrameAnimator::GetCompositedFrame is not called, then the internal state
of the animation is advancing ahead of what the user sees. Once it is
called, the new frame is far ahead of the previously displayed frame.
The only difference now is that we will display the previous frame for
slightly longer until the next refresh tick.
Note that if an animated image is layerized (should not happen today) or
otherwise uses an ImageContainer, this optimization fails. While we know
whether or not we have an image container, we do not know if anything is
actively using it.
Note that AnimationSurfaceProvider will override these methods to give a
proper implementation in a later patch in this series. For now, they are
mostly stubbed, using the default implementation from ISurfaceProvider.
They focus on the main operations we perform on an animation:
1) Progressing through the animation, e.g. advancing a frame. If we
don't decode the whole animation up front, we need to know at the
decoder level where we are in the display of the animation.
2) Restarting an animation from the beginning. This is a specialized
case of the above, where we want to skip explicitly advancing through
the remaining frames and instead restart at the beginning. The decoder
may have already discarded the earliest frames and must start redecoding
them.
3) Knowing whether or not the decoder is still active, e.g. can we be
missing frames.
When an animated image has been discarded, we avoided marking the
composited frame invalid unless it had been previously decoded. Most of
the time this was fine, but if the animated image was still decoding for
the first time, then we still had a composited frame lingering that we
did not mark as invalid. As a result, when we called
RasterImage::LookupFrame (and indirectly
FrameAnimator::GetCompositedFrame), it would always return the
composited frame. This meant that RasterImage::Decode would never be
called to trigger a redecode. At the same time,
FrameAnimator::RequestRefresh would not cause us to advance the frame
because the state was still discarded.
With this patch we separate out the concepts of "has ever requested to
be decoded" and "has ever completed decoding." The former is now used to
control whether or not a composited frame is marked as invalid after we
discover we currently have no surface for the animation -- this solves
the animation remaining frozen as we now request the redecode as
expected. The latter remains used to determine if we actually know the
total number of frames.
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.
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.
Also add crashtest with a corrupt animated PNG image that causes a leak to be reported (thanks to these MOZ_COUNT calls). This leak is fixed by the patch on separate bug 1237709.
Timothy Nikkel
Andrew Osmond
Seth Fowler
Daniel Holbert
Carsten "Tomcat" Book
Birunthan Mohanathas