Skip to content

Retry FFmpeg decoder packets rejected with EAGAIN - #119

Draft
Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-decode-eagain-3e4d
Draft

Blackspirits wants to merge 1 commit into
upl/review-base-3e4dfrom
audit/ffmpeg-decode-eagain-3e4d

Conversation

@Blackspirits

@Blackspirits Blackspirits commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Purpose

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

Current upstream main was rechecked immediately before this tranche and remains 4ecc74a92a630e62824e92c8f64aa42c7de1b84d; its four commits after the audit base do not touch FfmpegPlayer.cs or the FFmpeg player tests.

This PR is deliberately separate from #118 (playback-speed clock re-anchoring).

Finding fixed

avcodec_send_packet(EAGAIN) dropped compressed input

Both the video and audio decode loops previously did this:

  1. call avcodec_send_packet(codec, packet);
  2. free the packet immediately;
  3. tolerate AVERROR(EAGAIN);
  4. drain available decoded frames with avcodec_receive_frame();
  5. continue with the next demuxed packet.

That violates libavcodec's send/receive contract.

When avcodec_send_packet() returns AVERROR(EAGAIN), the input packet was not accepted. FFmpeg requires the caller to receive pending output and then resend that same packet once output has been drained. Freeing it and advancing to the next demux packet can therefore lose compressed video/audio input.

The issue also applies to the flush/drain packet state machine: receive-side progress must happen before a rejected send is retried.

Fix

The decode loops now:

  • retain ownership of the packet while avcodec_send_packet() returns EAGAIN;
  • drain avcodec_receive_frame() as before;
  • resend the same retained packet only after receive-side progress;
  • abandon the retry if close or a newer seek makes the packet stale;
  • free the packet exactly once after it is accepted or deliberately abandoned;
  • preserve the existing hardware-decoder fallback behavior;
  • fail safe instead of spinning if a decoder ever violates the FFmpeg state-machine guarantee by returning send-side EAGAIN without permitting receive-side progress.

The implementation is intentionally compact; an earlier structurally equivalent patch was reworked because it caused hundreds of lines of indentation-only diff noise.

Regression coverage

A focused pure helper test pins that retry is allowed only when all three conditions hold:

  • send returned AVERROR(EAGAIN);
  • receive-side output made progress;
  • the packet has not been invalidated by close/seek.

Non-EAGAIN and EOF cases are explicitly rejected.

FFmpeg API contract

Current FFmpeg documentation states that for avcodec_send_packet():

  • AVERROR(EAGAIN) means the input is not accepted in the current state;
  • the caller must read output with avcodec_receive_frame();
  • once all output is read, the same packet should be resent.

FFmpeg also guarantees that send and receive cannot both remain in EAGAIN at the same point in the state machine.

Scope / branch state

AI assistance: ChatGPT was used for adversarial libavcodec state-machine/ownership review, FFmpeg API verification, and focused regression design.

Final CI

Authoritative run: SubtitleEdit#250 on 7d4322e310d4c8eef6ebb8de61daaafda7834b84

  • 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
  • retry step skipped
  • failure-artifact upload skipped

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

@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 frozen HEAD 7d4322e310d4c8eef6ebb8de61daaafda7834b84 after CI SubtitleEdit#250.

Rechecked libavcodec packet ownership and send/receive ordering for video, audio, drain packets, hardware fallback, close, and a newer seek arriving while a rejected packet is being drained. A packet rejected with AVERROR(EAGAIN) is retained until receive-side progress is drained and is then resent; it is not retried after close/seek invalidates its serial, and it is freed exactly once on every exit path.

Also rechecked the impossible-state guard: FFmpeg documents that send and receive may not both remain in EAGAIN, so the no-progress path logs and drops instead of spinning forever.

CI SubtitleEdit#250 passed restore, build, and the full suite on this exact HEAD: SeConv 488/2/0, LibUiLogic 905/0/0, LibSE 2017/0/0, UI 5231/9/0. Retry was not used.

No blocking defect found in this tranche. This remains an audit draft only; no merge/promotion intended.

Copy link
Copy Markdown
Owner Author

Adversarial final review of HEAD 7d4322e310d4c8eef6ebb8de61daaafda7834b84.

Scope: libavcodec send/receive state machine in the FFmpeg player, isolated on common audit base 3e4d052adc78464e71bbd5154880d935fd592960.

Confirmed the original defect: both decode loops freed a compressed AVPacket after avcodec_send_packet() returned AVERROR(EAGAIN), although FFmpeg defines that result as input-not-accepted and requires the same packet to be resent after receive-side progress.

Final patch review confirms:

  • rejected packets retain ownership until accepted or deliberately abandoned;
  • receive-side progress is required before retry;
  • close/newer-seek invalidation prevents stale packets being resent;
  • packet lifetime still ends exactly once;
  • hardware-decoder fallback remains intact;
  • the impossible send-EAGAIN/receive-no-progress state fails safe instead of spinning;
  • the compact implementation avoids the earlier indentation-only diff noise.

CI SubtitleEdit#250 passed on this exact HEAD:

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

No unresolved correctness blocker remains in this EAGAIN/packet-ownership tranche. PR intentionally remains draft; no merge/promotion performed.

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