Closed Bug 1510601 Opened 2 years ago Closed 2 years ago

Assertion failure: mSizeKnown, at /builds/worker/workspace/build/src/image/AnimationFrameBuffer.cpp:200

Categories

(Core :: ImageLib, defect, P3)

65 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed

People

(Reporter: bc, Assigned: aosmond)

References

()

Details

(Keywords: assertion, regression)

Attachments

(2 files)

1. https://www.bloomberg.com/news/features/2018-11-15/the-u-s-is-playing-catch-up-with-rivals-as-globalization-marches-on?srnd=businessweek-v2

2. Windows/Linux Nightly 65
mozversion INFO | application_buildid: 20181127050227
mozversion INFO | application_changeset: 510f4bccd603a6a64b514e174c5d9e52d7afd185


Assertion failure: mSizeKnown, at /builds/worker/workspace/build/src/image/AnimationFrameBuffer.cpp:200
#01: mozilla::image::AnimationFrameRecyclingQueue::MarkComplete(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) [image/AnimationFrameBuffer.cpp:453]
#02: mozilla::image::AnimationSurfaceProvider::CheckForNewFrameAtTerminalState() [image/AnimationSurfaceProvider.cpp:393]
#03: mozilla::image::AnimationSurfaceProvider::Run() [image/AnimationSurfaceProvider.cpp:245]
#04: mozilla::image::DecodePoolWorker::Run() [mfbt/RefPtr.h:69]

Assertion added in bug 1465619
Flags: needinfo?(aosmond)
I can reproduce locally, thank you.
Assignee: nobody → aosmond
Keywords: regression
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Version: unspecified → 65 Branch
So this is less scary than I first thought. The animation was reset, and there are two asserts we may end up tripping as a result.

1) https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/image/AnimationFrameBuffer.cpp#200

AnimationFrameRecyclingQueue::ResetInternal sets mInsertIndex back to 0. We didn't haven't finished a full decoding pass yet, so mSizeKnown is false, and mSize is some non-zero value.

AnimationFrameRecyclingQueue::MarkComplete checks if mInsertIndex is the same as mSize. It isn't, because the new decoder wasn't able to produce as many frames as before due to an OOM. Thus it wants to mark us as having a redecode error, and we assert mSizeKnown to ensure this is indeed a redecode.

I think the assert can be removed, since it isn't valid.

2) https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/image/FrameAnimator.cpp#457

Similarly, AnimationFrameRecyclingQueue::ResetInternal clears mDisplay, so we have no frames anymore. Even if it didn't we may not have had the first frame in that queue.

The new decoder hasn't produced the first frame again yet.

FrameAnimator::RequestRefresh gets an exact match from the surface cache, and tries to get the first frame (important point: not for display!). It isn't available so it fails. The assumption is that this can only happen if we discarded the animation. Obviously this is not what happened, we just reset the animation.

So I think that assert is also faulty.
Flags: needinfo?(aosmond)
Keywords: regression
Keywords: regression
Specifically the regression came from bug 1508393, as that was were recycling was turned on (it had generate full frames as a dependency).
Blocks: 1508393
The size was originally incremented in AnimationFrameBuffer::Insert
however if an animation was reset before we finished decoding, it would
count some frames twice in the counter. Now we increment it inside
InsertInternal, where AnimationFrameDiscardingQueue can make a more
informed decision on whether the frame is a duplicate or not.

Additionally we now fail explicitly when we insert more frames on
subsequent decodes than the original decoders. This will help avoid
getting out of sync with FrameAnimator.
When an animation is reset, we can actually avoid the reallocations by
keeping the frames in the mRecycle and mDiscard queues. There is no real
need to do this. We just need to make sure the recycle rect calculation
is done appropriately, particularly when we haven't reached the end yet
and thus don't know the first frame refresh area yet.

Depends on D13464
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0861a85d13bb
Part 1. Move size increments into AnimationFrameBuffer::InsertInternal. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a3f89d4bb6
Part 2. Improve recycling of frames when an animation is reset. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/0861a85d13bb
https://hg.mozilla.org/mozilla-central/rev/d8a3f89d4bb6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.