Fix native memory corruption when frame stepping runs concurrently with seeks (ShowFrameNext/Prev vs Seek task) - #706
Open
dderoia wants to merge 2 commits into
Open
Fix native memory corruption when frame stepping runs concurrently with seeks (ShowFrameNext/Prev vs Seek task)#706dderoia wants to merge 2 commits into
dderoia wants to merge 2 commits into
Conversation
Two races, both admitted by the in-source TBR notes ('Missing locks (e.g.
GetFrameNext)' / 'Currently ensure you pause VideoDecoder before seek'):
1. Player.Seek's background task runs decoder.Seek + GetVideoFrame OUTSIDE
Player.lockActions, so ShowFrame/ShowFrameNext/ShowFramePrev (holding
lockActions) could Flush/decode on the same AVCodecContext concurrently
with the seek task's avcodec_send_packet. New DecoderContext.StepSeekLock
serializes both paths; always outermost, so the pre-existing inverted
internal lock orders (Seek: codecCtx->fmtCtx, GetVideoFrame:
fmtCtx->codecCtx) are never composed across threads. SeekCompleted now
fires outside the lock (subscriber re-entrancy).
2. VideoDecoder.GetFrameNext documented 'Decoder/Demuxer must not be
running' but never enforced it: after a seek, VideoDemuxer.Start() leaves
the demuxer thread filling queues, sharing fmtCtx AND the demuxer's
single packet field with GetNextPacket. GetFrameNext now pauses both,
exactly as GetFrame always did.
Repro (before): 3-4 Player instances rapid-stepping right after accurate
seeks died within seconds - AV in avcodec_send_packet or av_read_frame,
heap corruption 0xC0000374, fail-fast 0xC0000409. After: clean runs at 3
and 4 instances, frame-exact round trips, pixel-identical cross-instance.
GetVideoFrame (the accurate-seek path) returned as soon as av_read_frame hit EOF, leaving the codec's delay pipeline undrained - so the stream's FINAL frame (and, depending on the target's half-frame acceptance window, the last few frames) could never be presented by an accurate seek. A seek targeting the end of the stream presented NOTHING while CurTime echoed the request and SeekCompleted fired with the requested ms. The frame-step path already drains (DecodeFrameNext); this gives the seek path the same ability: at demuxer EOF, send the drain packet (avcodec_send_packet with null - a new SendDrainAVPacket, bypassing SendAVPacket whose key-packet validation dereferences the packet) and receive the remaining frames under GetVideoFrame's own acceptance test. The caller's lockFmtCtx + lockCodecCtx are already held; the next Flush/Seek's avcodec_flush_buffers resets draining state as it always has. Verified against real footage (25fps split outputs with non-zero video start times and a 60fps recording): end-of-stream accurate seeks now present the true final frame (native-resolution snapshots match ffmpeg-extracted last frames at 44-46dB PSNR; previously the picture kept the pre-seek frame).
Author
|
▎ Added: a second fix in the same seek path (commit 836add3). GetVideoFrame (the accurate-seek path) returns as soon as av_read_frame hits EOF, leaving the codec's delay pipeline undrained — so an accurate seek targeting a stream's final frame presents nothing, while CurTime and SeekCompleted both report success. The frame-step path already drains at EOF (DecodeFrameNext); this gives the seek path the same ability via a null avcodec_send_packet, receiving the tail under GetVideoFrame's existing half-frame acceptance test. Reproduction: SeekAccurate to the last frame of any file — the picture never changes. Verified on 25 fps and 60 fps content by comparing rendered output against ffmpeg-extracted final frames. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Running frame stepping (
ShowFrameNext/ShowFramePrev/ShowFrame) while a seek's decoder work is still in flight corrupts native FFmpeg state. With 3+Playerinstances stepping right after accurate seeks (a multi-angle review app), stock v3.10.4 crashes within seconds, 100% reproducibly —AccessViolationExceptioninavcodec_send_packetorav_read_frame, heap corruption0xC0000374, or fail-fast0xC0000409. This PR fixes the two races behind it. Likely the root cause of #431 (rapidShowFrameNext/SeekBackwardjog-wheel crashes) and related to #457 (rapid seek crashes).Both races are already acknowledged in the library's own comments — this isn't a speculative change:
The "locks at higher level" assumption doesn't hold: the seek task runs outside
Player.lockActions.Race 1 — seek task vs step path on the same
AVCodecContextPlayer.Seek(private) does its decoder work (decoder.PauseDecoders→decoder.Seek→decoder.GetVideoFrame→avcodec_send_packet) on aTask.Runbackground task that holds no player-level lock.ShowFrame/ShowFrameNext/ShowFramePrevholdlockActions— which the seek task never takes — and calldecoder.Flush()/VideoDecoder.GetFrame/GetFrameNext(which take no internal locks). Result: one thread flushes/decodes on the codec context while another is insideavcodec_send_packeton the same context → AV / heap corruption.Fix: a new
DecoderContext.StepSeekLockserializes the seek task's decoder section against the three step methods.Why a new outermost lock instead of reusing the existing ones: the internal lock orders are already inverted between the two sides of the seek path —
DecoderContext.SeektakeslockCodecCtx→lockFmtCtx, whileGetVideoFrametakeslockFmtCtx→lockCodecCtx(safe today only because both run sequentially on the seek task). Composing either order into the step path from a different thread would introduce a lock-order deadlock.StepSeekLockis therefore always the outermost lock in both paths and never nests the existing locks in a new order.SeekCompletedis deliberately raised outside the lock, so a subscriber that calls back into alockActions-taking player API can't deadlock against a step holdinglockActionswhile waiting forStepSeekLock.Race 2 — demuxer thread vs
GetFrameNexton the samefmtCtx(and the samepacketfield)GetNextPacketdocuments "Demuxer must not be running", andGetFrameenforces it (demuxer.Pause(); Pause();) — butGetFrameNextnever did. After any seek,VideoDemuxer.Start()leaves the demuxer thread filling queues; a subsequentShowFrameNextthat misses thevFramesqueue callsGetFrameNext→GetNextPacket→av_read_frame(fmtCtx, packet)concurrently with the demuxer thread's ownav_read_frame— on the samefmtCtxand writing into the demuxer's singlepacketfield. This was the second crash signature (AV inav_read_frameinsideDemuxer.RunInternal) that surfaced once Race 1 was fixed.Fix:
GetFrameNextnow pauses demuxer + decoder first, exactly asGetFramealways did.Reproduction
3–4
Playerinstances on the same local 1080p60 file, paused, thenShowFrameNextissued to all instances repeatedly right afterSeekAccuratecalls (the pattern of a multi-angle app stepping a synced group, or #431's 10 Hz jog wheel). Stock v3.10.4: crash within seconds, every run, with the signatures above. Two instances never reproduce it — at that load the seek task's decoder work completes in ~0.6 s, before the first step arrives; at four instances seeks take ~1.2 s and the windows stay open. The tile count only widens the race windows; the races themselves are per-instance.Verification
With this patch, the previously 100%-crashing matrix passes: 3 and 4 instances, steps issued back-to-back and 100 ms-paced, plus stress bursts (60 rapid steps, alternating +1/-1, step/seek interleaving) — 23 probe runs (~1,400 native steps) and 3 stress runs, all clean, with frame-exact step landings verified by pixel comparison against a burned-in frame counter (round trips return to the exact tick and a 0-diff frame; all instances land the identical frame).
One honest caveat: a single unreproduced
0xC0000005occurred in an early patched run (1 in ~26 runs; never again in 23 subsequent targeted attempts). If something remains, the render-frame lifetime path (vFramesdispose vs in-flight present) is the un-ruled-out suspect — this patch doesn't touch it. The two races fixed here account for every reproducible crash and both captured stacks.