Do not recycle FFmpeg frames removed by seek flush - #132
Draft
Blackspirits wants to merge 1 commit into
Draft
Blackspirits wants to merge 1 commit into
Blackspirits wants to merge 1 commit into
Conversation
Blackspirits
commented
Sep 15, 2026
Blackspirits
left a comment
Owner
Author
There was a problem hiding this comment.
Final adversarial review on cd1ba8af5d5e0e7f7b576d7da96ccce4cfa26c8b after authoritative CI SubtitleEdit#265. Rechecked flush ownership, normal pool reuse, size-change/close behavior, seek-bounded allocation cost, and the known separate presenter serial race. No blocker found in this tranche; the presenter race remains intentionally out of scope.
This was referenced Sep 15, 2026
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.
Purpose
Independent FFmpeg-player follow-up to merged upstream PR SubtitleEdit#14878, kept on the common audited base
3e4d052adc78464e71bbd5154880d935fd592960.This tranche is deliberately limited to
VideoFrameQueueownership during seek flush. It does not claim to solve the separate presenter race where_currentSerialcan advance after the presenter's serial check but beforeShowFrame().Finding fixed
Seek flush could recycle a frame object while the presenter still held a reference to it
PresentLoop()obtains queue entries throughPeek()/PeekSecond()and then reads their managed metadata (Serial,Pts,IsEndOfStream) outside the queue lock.At the same time, a successful seek calls
VideoFrameQueue.Flush()from the demux thread.Before this PR,
Flush()dequeued valid queued frames and immediately pushed them into the reusable frame pool. The video decoder could thenRent()the sameVideoFrameobject and overwrite its serial, timestamp and pixel buffer while the presenter still held the pre-flush reference returned byPeek().That turns an ordinary stale-reference situation into a concurrent object-reuse/data race. Identity checks later in presentation can prevent some stale frames from being shown, but they cannot make an object safe to mutate while another thread is still reading that same instance.
Fix
Frames removed specifically by
Flush()are now disposed instead of returned to the pool.Normal pooling remains unchanged:
Return()and are reused;A seek is rare relative to frame presentation, and the queue is capped at three converted frames, so the bounded reallocation cost is preferable to reusing an object whose last presenter reference cannot be proven released.
Regression coverage
A focused queue test pins the ownership rule:
Peek();Known separate boundary
The audit also identified a distinct presenter race:
_currentSerialcan advance afterPresentLoop()checked a frame but beforeShowFrame()pops/presents it. That requires a presenter-side serial revalidation and is intentionally not hidden inside this queue-ownership tranche.Scope / branch state
0252d55fd2f95752425c2e30b70363e6af7a93e83e4d052adc78464e71bbd5154880d935fd592960cd1ba8af5d5e0e7f7b576d7da96ccce4cfa26c8bAI assistance: ChatGPT was used for adversarial presenter/frame-pool concurrency review, ownership modelling, and focused regression design.
Final CI
Authoritative run: SubtitleEdit#265 on
cd1ba8af5d5e0e7f7b576d7da96ccce4cfa26c8bCurrent upstream main was rechecked at
0e8b50b27f4084a12cd4a3b0ed01245366f91379; the delta is Japanese localization only and does not touch the FFmpeg player or frame queue.Final adversarial review was performed on this exact HEAD after CI; no blocker found.