feat(video-streamer): add periodic keyframe forcing for improved seekability#1705
feat(video-streamer): add periodic keyframe forcing for improved seekability#1705irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 4 commits intomasterfrom
Conversation
…ion shadowing The existing keyframe detection only handled VP8 frames. This adds proper VP9 keyframe detection based on the VP9 bitstream specification, supporting all profiles (0-3). The codec type is now threaded through the iterator and block tag layers so keyframe checks use the correct codec-specific logic. Also sets VpxEncoderPreset::BestPerformance for improved re-encoding throughput. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Send Error (not End) to client when shutdown is caused by a stream error - Use AbortOnDrop guard for bridge task to handle early returns/bails - Remove dead UserFriendlyError::UnexpectedEOF variant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull request overview
Adds periodic, interval-based keyframe forcing during re-encoding so the outgoing WebM stream remains seekable over time (and starts new clusters on forced keyframes), and improves EOF-wait robustness by warning when the when_eof oneshot sender is dropped.
Changes:
- Track last forced keyframe time and force a new keyframe every
KEYFRAME_INTERVAL_MS, starting a new cluster for that keyframe. - Log a warning and treat EOF-wait as shutdown if the
when_eofoneshot sender is unexpectedly dropped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/video-streamer/src/streamer/tag_writers.rs | Adds interval-based forced keyframes and cluster splitting to improve seekability/resilience. |
| crates/video-streamer/src/streamer/mod.rs | Adds a warning log when the when_eof oneshot sender is dropped unexpectedly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
irvingouj@Devolutions (irvingoujAtDevolution)
left a comment
There was a problem hiding this comment.
Thanks for catching this! I've updated the PR title and description to accurately reflect what this PR does: adding periodic keyframe forcing (every 10 seconds) for improved seekability. The codec-aware VP9 keyframe detection is already implemented on master in block_tag.rs.
irvingouj@Devolutions (irvingoujAtDevolution)
left a comment
There was a problem hiding this comment.
Regarding test coverage: This is a valid suggestion. Adding an integration test for periodic keyframe forcing would require a test that runs for >10 seconds to verify the behavior. This could be added as a follow-up improvement.
Regarding the state update ordering concern: The current code already follows the correct pattern. Looking at lines 547-551, we call start_new_cluster(cluster_rel)? BEFORE updating cut_block_state, which is exactly the safe ordering suggested. This mirrors the pattern used in compute_met_timestamp (lines 585-589). The code is already correct here.
Summary
when_eofoneshot sender is dropped unexpectedlyContext
Session shadowing re-encodes the video stream. Without periodic keyframes, the WebM stream becomes hard to seek and less resilient to packet loss over time. This change forces a keyframe every 10 seconds and starts a new cluster at that point, making the stream seekable in 10-second increments.
Note: Codec-aware keyframe detection (VP8/VP9) is already implemented on master in
block_tag.rsand is not part of this PR.Test plan
🤖 Generated with Claude Code