Skip to content

Fail closed when FFmpeg seek fails - #117

Draft
Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-failed-seek-state-3e4d
Draft

Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-failed-seek-state-3e4d

Conversation

@Blackspirits

@Blackspirits Blackspirits commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Purpose

Third independent follow-up to merged upstream PR SubtitleEdit#14878 ("FFmpeg player: fix eight teardown, seek and clock defects") on the audited base 3e4d052adc78464e71bbd5154880d935fd592960.

The upstream main is now 4ecc74a92a630e62824e92c8f64aa42c7de1b84d, four commits ahead of that audit base. Those commits touch Italian localization, Batch Convert, and Auto Translate only; there is no drift in FfmpegPlayer.cs or its FFmpeg tests, so this tranche stays on the common #115/#116 base to keep the FFmpeg follow-ups isolated and directly comparable.

Finding fixed

Failed av_seek_frame() committed a seek that never happened

Before this PR, PerformSeek() logged a native seek failure and then continued as if it had succeeded:

  • advanced _currentSerial to the failed request;
  • reset audio anchor/clock state to the requested target;
  • flushed video/audio packet queues and decoded video frames;
  • reset the audio sink;
  • cleared EOF state in the demux loop.

The public Seek() path also optimistically assigned _pausedPosition to the requested target and cleared _endReached before libavformat had accepted the seek.

That leaves the player reporting and scheduling from a destination the demuxer never reached, while throwing away the still-valid pre-seek pipeline.

FFmpeg's own ffplay only flushes packet queues and resets the external clock inside the successful-seek branch; a failed seek logs the error and leaves those committed playback structures alone.

Transactional seek behavior

This PR makes the seek state fail closed:

  • Seek() now records only the requested serial/target and wakes demux; committed playhead/end state is left untouched.
  • PerformSeek() returns success/failure.
  • serial advancement, clock rebasing, queue/frame flush, audio reset, and _endReached = false happen only after av_seek_frame() succeeds.
  • a failed latest request rolls _requestedSerial/_requestedTarget back to the still-current pipeline state.
  • if a newer seek arrived while av_seek_frame() was blocked, failure of the older seek does not erase the newer request.
  • demux EOF state is preserved on failure instead of manufacturing another EOF cycle without a successful reposition.
  • native seek failure is promoted from Debug-only output to the normal player error log.

Play / EOF race closed

Keeping the committed end state until native success exposed an important concurrency edge that the old optimistic mutation had been masking:

  1. playback is at EOF;
  2. user seeks away from EOF;
  3. before av_seek_frame() returns, user presses Play;
  4. Play sees the old committed EOF and would otherwise enqueue Seek(0), overwriting the user's requested destination.

Play now refuses the auto-rewind while any seek is outstanding.

The presenter also keeps an old video EOS marker queued while a newer seek is outstanding. This prevents a video-only stream from calling ReachEnd() during the native seek and losing the user's Play intent before the new serial commits.

Audio-only playback has the same guard: its EOF tick cannot call ReachEnd() while a seek is outstanding, so restarting an audio-only file from EOF no longer loses the Play intent before the seek commits.

Regression coverage

Pure tests pin:

  • failure of the latest request rolls back to the current pipeline serial/position;
  • failure of an older request preserves a newer requested serial/target;
  • Play auto-rewind still happens at a committed EOF/end position;
  • Play auto-rewind is suppressed while a seek is outstanding;
  • audio-only EOF completion is suppressed while a seek is outstanding.

Scope / evidence

Final CI

Authoritative run: SubtitleEdit#248 on bf672abc87e812560af43da5bda4dc3912895c6b

  • SeConv: 488 passed / 2 skipped / 0 failed
  • LibUiLogic: 905 passed / 0 skipped / 0 failed
  • LibSE: 2017 passed / 0 skipped / 0 failed
  • UI: 5241 passed / 9 skipped / 0 failed
  • retry step skipped

Compiler warnings are confined to pre-existing accessibility/VoiceManager files outside this PR's changed set.

AI assistance: ChatGPT was used for adversarial seek-state/concurrency review, comparison with FFmpeg's ffplay behavior, and focused regression design.

@Blackspirits
Blackspirits force-pushed the audit/ffmpeg-failed-seek-state-3e4d branch from 5983d4d to bf672ab Compare September 15, 2026 08:21

@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 the frozen HEAD after CI SubtitleEdit#248. Rechecked failed-seek rollback, concurrent newer-seek preservation, EOF-state preservation, Play-from-EOF behavior for video and audio-only media, and the serial/queue commit boundary. No blocking defect found in this tranche. CI SubtitleEdit#248 passed build + full suite on this exact HEAD; retry was not used. This remains an audit draft only: no merge/promotion intended.

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