Skip to content

Do not recycle FFmpeg frames removed by seek flush - #132

Draft
Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-frame-flush-no-reuse-3e4d
Draft

Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-frame-flush-no-reuse-3e4d

Conversation

@Blackspirits

@Blackspirits Blackspirits commented Sep 15, 2026

Copy link
Copy Markdown
Owner

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 VideoFrameQueue ownership during seek flush. It does not claim to solve the separate presenter race where _currentSerial can advance after the presenter's serial check but before ShowFrame().

Finding fixed

Seek flush could recycle a frame object while the presenter still held a reference to it

PresentLoop() obtains queue entries through Peek() / 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 then Rent() the same VideoFrame object and overwrite its serial, timestamp and pixel buffer while the presenter still held the pre-flush reference returned by Peek().

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:

  • frames actually consumed by the presenter still go through Return() and are reused;
  • only frames invalidated asynchronously by seek flush are discarded;
  • size-change and close cleanup semantics remain fail-closed.

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:

  1. enqueue a frame;
  2. hold the same object through Peek();
  3. flush the queue;
  4. prove the held frame's native buffer was disposed;
  5. rent another same-sized frame;
  6. prove the flushed object was not reused from the pool.

Known separate boundary

The audit also identified a distinct presenter race: _currentSerial can advance after PresentLoop() checked a frame but before ShowFrame() pops/presents it. That requires a presenter-side serial revalidation and is intentionally not hidden inside this queue-ownership tranche.

Scope / branch state

AI assistance: ChatGPT was used for adversarial presenter/frame-pool concurrency review, ownership modelling, and focused regression design.

Final CI

Authoritative run: SubtitleEdit#265 on cd1ba8af5d5e0e7f7b576d7da96ccce4cfa26c8b

  • SeConv: 488 passed / 2 skipped / 0 failed
  • LibUiLogic: 905 passed / 0 skipped / 0 failed
  • LibSE: 2017 passed / 0 skipped / 0 failed
  • UI: 5231 passed / 9 skipped / 0 failed
  • build: 0 errors / 7 pre-existing warnings
  • retry step skipped
  • failure-artifact upload skipped

Current 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.

@Blackspirits Blackspirits left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant