fix(capture): make Linux preview seek robust at/past the last frame - #551
Open
Beetix wants to merge 4 commits into
Open
fix(capture): make Linux preview seek robust at/past the last frame#551Beetix wants to merge 4 commits into
Beetix wants to merge 4 commits into
Conversation
`decode_at` seeks BACKWARD to the nearest keyframe then decodes forward to the target. On `av_read_frame` EOF it broke out immediately without sending a NULL flush packet, so frames still buffered inside the decoder (decode latency / B-frame reorder) were never received. When the target lands in the short tail after the last keyframe there are too few packets to beat that latency, `found` stayed null, and the preview failed with "decode_at(frame_idx=…) : aucune frame reçue" — intermittently, when scrubbing to the end. Drain the decoder at EOF like the sequential `receive_into` path already does: send a NULL packet, pull the remaining frames into `found`, keeping the target-pts check so the last frame is still returned when scrubbing past the end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
decode_at (the seek/scrub path behind Decoder::seek_to) skipped a misaligned post-seek packet by comparing send_packet's return to -0x2A2A2A2A — the tag `****`, which is not an ffmpeg error code. AVERROR_INVALIDDATA is -1094995529, so the guard never matched and a genuinely fragmented first NAL after a BACKWARD seek fell through to the fatal bail!, aborting the whole seek. Symptom: the preview failed when time-travelling/scrubbing (as opposed to reaching the end), even after the EOF-drain fix. This is the same defect receive_into was already fixed for on the sequential pump; apply the identical correction here — compare against the AVERROR_INVALIDDATA constant, and use the named AVERROR_EAGAIN/AVERROR_EOF constants in the receive loop instead of bare -11 / -541478725 literals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ropped captures) decode_at bailed with "aucune frame reçue" whenever the requested index was at or past the file's last frame. On a frame-dropped capture whose real avg_frame_rate (e.g. 56.34) is below the nominal 60, a frame index computed on the 60fps grid lands past the true frame count, so the preview failed the moment it switched to such a clip. Mechanism: for a target at/after the last frame, av_seek_frame BACKWARD succeeds but lands on a lone undecodable packet just before EOF; after avcodec_flush_buffers that reference-less frame yields nothing even when drained, so found stayed null. Confirmed by sweeping decode_at over a real 56.34fps/631-frame recording: indices 0..=630 decoded, 631+ failed. Fix: extract the packet pump into pump_to_target(); when the seek-based pump returns no frame, rewind to 0 and forward-scan (the same linear scan already used as the unindexed-WebM fallback). The forward pump keeps the last frame <= target, so a past-end index yields the final frame instead of erroring. Adds an env-gated regression test (OPENSCREEN_SWEEP_FILE) that asserts decode_at never fails for 0..nb_frames+30. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Empirically (decode_at index sweep on a real 56.34fps recording) the EOF drain returns zero frames on the app's own captures: they are H.264 baseline with has_b_frames=0, so the sequential receive loop already has every frame and there is nothing buffered to drain. The drain stays necessary for any stream WITH B-frames (imported video, other codecs), where the last frame is held in the reorder buffer. Document that so the drain is not mistaken for dead code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Linux decoder now retries failed indexed seeks with sequential pumping, drains delayed frames at EOF, uses shared FFmpeg error constants, centralizes final frame validation, and adds an ignored regression test for indices at and beyond the stream’s final frame. ChangesLinux decoder handling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The Linux preview now recovers frames at or beyond the recording end, including draining delayed decoder frames. The change is mergeable with owner awareness because malformed or interrupted media could still be mistaken for end-of-file, and repeated decoder failures may increase memory use through unreleased temporary frames. Sequence Diagram(s)sequenceDiagram
participant SwDecoder
participant demuxer
participant FFmpegDecoder
SwDecoder->>demuxer: Seek to target timestamp
demuxer-->>SwDecoder: Seek result
SwDecoder->>FFmpegDecoder: Decode packets
FFmpegDecoder-->>SwDecoder: Frame or no frame
SwDecoder->>demuxer: Rewind and scan sequentially when needed
demuxer-->>SwDecoder: Packets through target
SwDecoder->>FFmpegDecoder: Flush delayed frames at EOF
FFmpegDecoder-->>SwDecoder: Final decoded frames
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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
Makes the Linux software decoder's seek path (
crates/compositor/src/linux_decode.rs,SwDecoder::decode_at) robust when the preview asks for a frame at or past the last frame of a recording. Previously the preview went blank withdecode_at(frame_idx=…) : aucune frame reçue.The headline fix is the past-end fallback. For a target at/after the last frame,
av_seek_frame(BACKWARD)succeeds but lands on a single undecodable packet just before EOF; afteravcodec_flush_buffersthat reference-less packet yields no frame, sodecode_atbailed. The pump loop is now factored intopump_to_target(), and when the seek-based decode returns nothing,decode_atrewinds to 0 and forward-scans — the same linear fallback already used for unindexed WebM — returning the last frame ≤ target instead of erroring.This surfaces on frame-dropped recordings whose real
avg_frame_rateis below the nominal 60 fps (produced by #511): a frame index computed on the 60 fps grid points past the file's true frame count. Measured on a real 56.34 fps / 631-frame capture,decode_atdecoded0..=630and failed for every index ≥ 631; after this change the full sweep (0..=nb_frames+30) passes.Two smaller, related seek-path hardening changes ride along (both inert on these captures but correct for other inputs, and empirically verified so — see Testing):
has_b_frames=0) so nothing is buffered and it is inert here. Mirrors the fixreceive_intoalready carries.AVERROR_INVALIDDATAconstant — the post-seek fragmented-packet guard compared against-0x2A2A2A2A(the tag****, not an ffmpeg error), so it never matched; corrected to the realAVERROR_INVALIDDATA(-1094995529). Same fixreceive_intoalready received. Not triggered by these indexed MP4s.Related issue
Fixes #550
Refs #511 (the upstream capture-side frame drops that produce the sub-60fps files which trigger this)
Type of change
Release impact
Desktop impact
Screenshots / video
N/A — removes an error state in the preview.
Testing
cargo test -p openscreen-compositor --lib— 181 pass.decode_at_never_fails_past_end(same pattern as theOPENSCREEN_GOLDEN_*tests, since no fixture mp4 is committed): assertsdecode_atreturns a frame for every index in0..nb_frames+30.OPENSCREEN_SWEEP_FILE=/path/to.mp4 cargo test -p openscreen-compositor --lib decode_at_never_fails_past_end -- --ignoredAVERROR_INVALIDDATAconstant and removing the EOF drain both produced byte-identical results on these captures (confirming they are inert here — the captures have no B-frames — while the past-end fallback is the load-bearing fix).🤖 Generated with Claude Code
Summary by CodeRabbit