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

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: bc, Assigned: aosmond)

Tracking

(Blocks 1 bug, {assertion, regression})

65 Branch
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 unaffected, firefox65 fixed)

Details

()

Attachments

(2 attachments)

Reporter

Description

7 months ago
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)
Assignee

Comment 1

7 months ago
I can reproduce locally, thank you.
Assignee: nobody → aosmond
Keywords: regression
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Version: unspecified → 65 Branch
Assignee

Comment 2

7 months ago
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
Assignee

Updated

7 months ago
Keywords: regression
Assignee

Comment 3

7 months ago
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
Assignee

Comment 4

7 months ago
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.
Assignee

Comment 5

7 months ago
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

Comment 6

7 months ago
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

Comment 7

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0861a85d13bb
https://hg.mozilla.org/mozilla-central/rev/d8a3f89d4bb6
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.