Fix video pacing by re-deriving due times and updating audio index handling - #14
Open
datagutt wants to merge 2 commits into
Open
Fix video pacing by re-deriving due times and updating audio index handling#14datagutt wants to merge 2 commits into
datagutt wants to merge 2 commits into
Conversation
A frame's due time was computed once, when it was decoded, from the audio playout offset current at that moment, and kept that schedule for its whole residence in the pacing queue. The offset is not a constant. The speed controller moves it continuously — a chunk emitted at +5% advances the OBS end by frames_out/rate against a stream end that advanced by in_frames/rate, so the offset shrinks by ~4.8% of that chunk — and a re-anchor steps it outright. Every reclaim the audio side performed therefore left queued video behind by the reclaimed amount until the queue drained: ~5% of residence sustained through a drain, and the full step for a queue's depth after a re-anchor. Video was the one part of the pipeline not connected to the latency-reclaim mechanism it was supposed to stay level with. That bound was invisible while the queue was shallow. It is not shallow when audio conceals and video keeps flowing, which packet loss produces routinely: concealment advances the OBS side while the stream side is frozen, so intake due times stretch at twice real time for the length of the outage, and the stale tail then plays out for as long as the queue is deep. A field log shows the signature plainly — video lead peaking at 7016ms against a buffer fill peak of 1280ms, which the residence identity (due - now = fill + obs_lead) cannot produce any other way. Reschedule the whole queue against one offset per pacing cycle instead. The spacing between frames is preserved (their due times differ only by their PTS deltas) and the queue moves with the audio it is mapped to, so video rides the same drain rather than trailing it. No new ceiling and no new policy: the frame and byte ceilings go back to being the pure memory guards they were described as, and the speed controller stays the only thing reclaiming latency. irl_video_playout_offset() is deliberately free of irl_video_due_time()'s side effects — lead stats, the warning log, the video-only fallback anchor all belong to a frame arriving, not to rescheduling one already held. It reads audio presence from the mapping fields rather than from audio_stream_idx, which the receiver thread rewrites on reconnect and which the video thread cannot read without a race; the two are equivalent here because the pump only publishes those fields for a chunk it actually handed to OBS. A re-anchor zeroes the published mapping for the one pump iteration it takes to rebuild, so the last good offset is held across that window: dropping to the video-only fallback there would move the entire queue onto a different clock. The hold has to be bounded on both sides. It expires well short of the reconnect delay, a clear drops it outright, and reset_runtime_state() clears it with the worker threads stopped — a stop/start pair never delivers a clear to the video thread, so a restart completing inside the hold would otherwise map a new PTS epoch through the previous stream's offset.
irl_video_due_time() runs on the video thread, via pacing_intake(), and tested ctx->audio_stream_idx in two places. That field is receiver-thread state: it indexes fmt_ctx->streams and the receiver thread rewrites it on every reconnect, so both reads raced a concurrent write. Every other cross-thread read in the plugin goes through os_atomic_*; these predate the rule and were missed. The two tests want different things, so they get different fixes. The first guards the audio playout mapping, and there the index was already redundant: the pump publishes latest_audio_obs_end_ts_ns and latest_audio_buffered_end_pts_ns only for a chunk it actually handed to OBS, so a published mapping implies an audio stream. Dropping the term removes the read outright. The second cannot use that argument — it decides whether to hold video back for audio that has not primed yet, so it runs precisely when no mapping exists. It needs the fact rather than the index, so the receiver thread now mirrors the fact into audio_stream_present alongside every assignment of audio_stream_idx, and the video thread loads it atomically.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughVideo pacing now stores frame PTS values and recalculates due times from live audio playout offsets. Audio presence is published atomically. A public offset helper manages recent mappings and resets state across runtime restarts. ChangesVideo pacing synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 by CodeRabbit