Fix video lead cap, hardware frame pool sizing, and decoder packet handling - #10
Merged
Conversation
Video PTS is mapped through the audio playout offset for lip sync, then
handed to libobs with a future timestamp for libobs to schedule. That makes
the audio jitter buffer's excursions the video path's problem, against two
libobs limits that are invisible from a plugin:
- ready_async_frame() advances its play head at wall-clock rate, so a
frame timestamped past it holds the previous frame on screen, and
- cache_video() throws the entire queue away at MAX_ASYNC_FRAMES (30),
silently, dropping the incoming frame with it.
A backlog excursion (960ms bleed ceiling, drained at only +5%) or an
accumulation of concealment offset therefore parks 30+ frames in libobs at
60fps and the queue starts wiping every time it refills: freeze frames and
forward jumps while audio, which paces itself, plays clean. The Media Source
never sees this because media-playback sleeps until each frame is due and
hands libobs a near-now timestamp, keeping its queue one frame deep.
Cap the lead at the configured target plus an excursion allowance. Only the
excursion is capped, never the steady lead, since libobs anchors its play
head to the first frame and holds only the growth since then — so a large
Target Buffer still buys real buffered lip sync. The allowance is bounded
above by a frame count (libobs queues frames, not milliseconds: the same
400ms is 12 frames at 30fps and 24 at 60fps, and a fixed allowance would
either clamp 30fps streams that were never at risk or fail to protect 60fps
ones) and below by the audio re-anchor margin (cap under it and concealment
offset the audio side never reclaims becomes permanent lip sync error rather
than transient). Modelling ready_async_frame/cache_video over a repeated
excursion to the bleed ceiling: 60fps goes from a wiped queue to a peak depth
of 26 and none, 30fps keeps its lip sync intact, steady state at any target
is untouched.
Past the cap the trade is explicit and transient: video runs ahead of audio
by the excess and re-syncs as the speed controller drains the buffer, meeting
the mapped line tangentially so the recovery costs about one duplicated frame
in twenty. Above ~60fps the two bounds conflict, the floor wins, and it is
logged — that is where libobs scheduling runs out and pacing has to move
in-plugin.
Adds video_lead_ms and video_lead_clamps to the stats (proc_handler,
websocket vendor, periodic log), since none of this was observable before.
extra_hw_frames budgeted IRL_VIDEO_QUEUE_SIZE surfaces, but the queue is not the only place a decoded hardware frame is held. At peak the plugin pins the queue's frames, the one the video thread has popped and is transferring in irl_video_output_frame(), and the one avcodec_receive_frame() just returned and irl_handle_video_frame() has not yet unref'd — two more than the pool was told about. (The clone irl_video_queue_push() takes references the same surface as the returned frame, so it does not add a third.) On fixed-pool decoders, which is what extra_hw_frames exists for at all, the shortfall makes the decoder stall waiting for a surface the queue is sitting on. The symptom is the same one the video lead cap addresses from the other end: frozen video while audio keeps playing.
None of the state behind the freeze-frame reports was visible from a user's
OBS log, so every diagnosis of it so far has rested on reading the code.
Four additions, each aimed at a specific claim that should be falsifiable
from a field log rather than argued from source:
pinned_peak=N/M Peak decoder surfaces this plugin holds at once (queued
frames plus the one the video thread has popped), against
what extra_hw_frames budgets. The pool needs peak + 1 for
the decoder's own frame. A peak of 5 confirms the previous
budget of 4 was short; a peak that never exceeds 3 says the
sizing fix was treating a problem that does not occur.
eagain=V/A avcodec_send_packet() returning EAGAIN, which means the
decoder refused the packet. Should be zero. Non-zero is the
direct signature of exhausted decoder surfaces, and it is
also the case the send path currently mishandles (the
packet is dropped rather than resent) — counted here first
so the frequency is known before changing that behaviour.
buf peak= Jitter-buffer high-water mark alongside instantaneous fill.
vlead peak= Video lead high-water mark alongside the instantaneous one.
The two peaks exist because stats are sampled every 30s while the backlog
excursion that drives this whole failure mode builds in ~1s and drains in
~17s. Sampling instantaneous values will usually miss it and report a healthy
buffer on exactly the streams worth investigating. Peaks are cumulative for
the source, matching vq_drops, so a two-hour stream's log answers the question
without needing the sample to land inside the event.
avcodec_send_packet() returning EAGAIN means the decoder did not accept the
packet: output has to be read and the same packet resent. Both decode paths
folded EAGAIN into the success branch, drained, and then let the caller unref
the packet — losing it. For video a lost reference frame is artifacts until
the next keyframe, not one missing frame.
This is reachable through the surface exhaustion the previous commit fixes:
a fixed-pool hardware decoder with no free surface refuses packets, so an
undersized extra_hw_frames was silently costing picture quality rather than
merely stalling.
Drain and resend once. Deliberately one retry rather than a loop: a single
drain frees every surface the decoder was waiting on, and this runs on the
receiver thread, which also feeds audio intake — a decoder returning EAGAIN
without producing frames would spin there and starve the jitter buffer,
trading a dropped packet for the underrun/concealment cascade. A packet still
refused after the retry is counted and handled exactly as before.
The receive loops move into drain_{audio,video}_frames() so the retry can
reuse them. They are lifted verbatim, error handling and all, so the decoder
error state machine (corruption counters, flush cooldowns, the audio state
reset) is untouched — verified by diffing the extracted bodies against their
originals. Counters are logged as eagain=V/A and pktdrop=V/A; both should be
zero, and pktdrop is the one that costs quality.
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThe change measures video lead, adds decoder EAGAIN retry handling, tracks pinned decoder surfaces, estimates frame intervals, expands statistics, updates media dependencies, and improves dependency-build diagnostics. ChangesVideo pipeline
Bundled media dependencies
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PacketHandler
participant FFmpegDecoder
participant drain_video_frames
participant ReceiverVideo
participant Statistics
PacketHandler->>FFmpegDecoder: send packet
FFmpegDecoder-->>PacketHandler: return EAGAIN
PacketHandler->>drain_video_frames: drain decoder output
drain_video_frames->>FFmpegDecoder: receive decoded frames
PacketHandler->>FFmpegDecoder: retry packet
ReceiverVideo->>Statistics: publish timing and pinned-surface state
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/receiver-audio.c`:
- Around line 700-701: Synchronize the telemetry updates and reads: in
src/receiver-audio.c:700-701, guard the audio_fill_peak_ms update with
audio_state_lock, acquiring shared timing state before the audio-buffer mutex;
in src/receiver-stream.c:525-578, snapshot audio_fill_peak_ms, video_lead_ns,
video_lead_peak_ns, video_lead_clamps, and video_frame_interval_ns under
audio_state_lock, then snapshot video_pinned_peak under video_queue_lock before
irl_log_receiver_stats() calls blog. Preserve the lock ordering and log only the
synchronized snapshots.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 758feb7c-70ff-45f5-a633-e4f09a15c5c9
📒 Files selected for processing (10)
README.mddeps/versions.envinclude/irl-source.hsrc/irl-source.csrc/receiver-audio.csrc/receiver-decode.csrc/receiver-stream.csrc/receiver-video.csrc/video-handler.csrc/websocket-vendor.c
The stats line formatted several fields straight out of struct irl_source while other threads were writing them: audio_fill_peak_ms is written by the audio thread, and video_lead_ns, video_lead_peak_ns, video_lead_clamps and video_pinned_peak by the video thread. The av_drift computation had the same shape, and worse — its three inputs are only meaningful relative to each other, so reading them separately could report a drift that never existed. Publish the audio-side peak under audio_state_lock, and snapshot everything the log needs before formatting any of it. Two separate acquisitions rather than one nested pair: nothing in the plugin holds video_queue_lock and audio_state_lock at the same time — the video thread drops the queue lock before irl_video_output_frame, and irl_handle_video_frame drops the state lock before pushing — and a stats line is the last place to introduce that ordering edge. The buffer fill is hoisted out of the argument list for the same reason, so no lock is held across blog(). audio_buffer_peek_state releases the buffer mutex before returning, so the new acquisition in the pump nests nothing and the documented order (audio_state_lock before the buffer mutex) is preserved. video_frame_interval_ns is snapshotted with the rest for uniformity, though it was not actually racy: the receiver thread is its only writer while the stream runs, and the log runs on that same thread. Not addressed: the same blog() still reads about a dozen pre-existing audio-thread counters unlocked (audio_underruns, current_speed, the audio_last_chunk_* figures and friends). Those are monotonic diagnostics rather than mutually-dependent state, and synchronizing them is a wider change than this fix.
…library
zlib 1.3.2 reworked its CMake and broke the Windows dep build with "ERROR:
zlib requested but not found".
Two changes upstream, both silent:
- ZLIB_BUILD_SHARED and ZLIB_BUILD_STATIC are zlib's own switches, default
ON, and are not driven by BUILD_SHARED_LIBS. So the build installed
z.dll plus an *import* z.lib next to the static library.
- The MSVC static output is now zs (1.3.1 called it zlibstatic).
ensure_msvc_lib_name returns early when the target name already exists, and
z.lib did exist — as the DLL's import library. So the static lib was never
wired up, and FFmpeg's zlib check failed. Had it passed, it would have been
worse: a bundled-static plugin linking a zlib DLL, which is the exact thing
verify-plugin.sh exists to prevent.
Turn the shared build off, add zs to the candidate names, and hard-fail if a
zlib DLL ever lands in the prefix again rather than let a future rename
reintroduce this quietly. Verified against the 1.3.2 tarball: WIN32 sets
zlib_static_suffix=s, ZLIB_BUILD_SHARED gates both the build and the install
of the DLL, and configuring with these flags installs the static library
alone where the old flags install both.
ZLIB_BUILD_EXAMPLES was also renamed to ZLIB_BUILD_TESTING in 1.3.2, so the
existing flag had quietly become a no-op; both are passed now.
The cache key hashes build-deps.sh, so this also evicts the prefix the failed
run cached with the zlib marker already written — otherwise the rebuild would
skip zlib and fail the same way.
…e dies
The failure dump was tail -60 of config.log, which cannot explain an
autodetected library. configure probes those early and then dies about them
in a sweep at the very end ("$lib requested but not found", configure:8361),
so the probe that holds the compiler and linker invocation sits roughly a
thousand lines above the tail. The Windows zlib failure printed a stdbit
check and nothing else of use, twice.
Grep the probe block for each library we require out of config.log before
printing the tail.
curl's --retry treats only timeouts and 5xx as transient, so "Failed to connect" and "Connection reset by peer" fail on the first attempt. ffmpeg.org returned both in one CI run and took the Windows job down with exit 35 before FFmpeg was even unpacked. --retry-all-errors and --retry-connrefused cover the connection-level cases, with a longer connect timeout and one more attempt.
FFmpeg's msvc flag translator special-cases zlib and asks for zlib.lib, not
z.lib like every other -l name:
-lz) echo zlib.lib ;;
-l*) echo ${flag#-l}.lib ;;
Nothing in the prefix provided that, so configure failed both zlib probes
with "LNK1181: cannot open input file 'zlib.lib'" and died much later in its
autodetect sweep as "zlib requested but not found".
Under zlib 1.3.1 the name existed by accident: 1.3.1 called its shared target
zlib, so zlib.lib was the DLL's import library and FFmpeg linked that. The
Windows build has therefore been linking zlib dynamically all along. 1.3.2
renamed the shared output to z, the accident stopped happening, and turning
the DLL off in the previous commit removed the last thing occupying the name.
Map the static archive to both spellings: zlib.lib for FFmpeg, z.lib for the
generic -l resolution in the generated CMake description.
The clamp was meant to keep libobs's async queue under its 30-frame wipe threshold. It compared the lead against the configured target, but what libobs queues is the lead's *growth* since its play head last anchored — as the code's own comment said. A large steady lead queues nothing, and the clamp had no way to tell the two apart. A 720p120 stream showed the cost. Its jitter buffer sat against the 960ms bleed ceiling for the whole run (buf=952ms peak=975ms, speed pinned at 1.050, av_drift=-1485ms: the +5% drain working and being refilled by a sender about 5% fast), so a ~1032ms lead was legitimate and steady. The clamp cut it to 520ms and held video half a second ahead of audio for the entire stream, against a queue that was very likely one frame deep. It also mis-reported the depth as ~47 frames, since that estimate assumes the play head sits at the target. Clamping only ever engaged when the buffer was parked high, which is exactly the state where the lead is steady rather than growing — so it was mostly buying desync for nothing. Keep the measurement: video_lead_ms, its peak, and the count of frames past what the queue could absorb (video_lead_clamps becomes video_lead_excess, since nothing is clamped now). Those are how we tell whether video pacing fixes this properly. The warning drops to LOG_INFO and says what it means: a steady lead is harmless, a rising one is not.
…eeds
Video pacing has to sleep until a frame is due while staying responsive to a
queue push, a clear, or shutdown — a wait with a deadline. Nothing in
irl-threading.h offered one, and plugin code cannot reach for
pthread_cond_timedwait directly (librist's MSVC shim wins the link over
w32-pthreads; verify-plugin.sh fails the build if a direct pthread_* call
reappears).
Three backends, because the portable spelling does not exist:
- Windows: SleepConditionVariableCS already takes a millisecond timeout.
- macOS: pthread_cond_timedwait_relative_np, which takes an interval.
- elsewhere: pthread_cond_timedwait against an absolute deadline, with the
condvar bound to CLOCK_MONOTONIC via pthread_condattr_setclock. The
default clock is CLOCK_REALTIME, so without that an NTP step or a manual
clock change would stretch or collapse a pacing wait. macOS has no
condattr_setclock, which is why it uses the relative call instead — that
one is immune by construction.
Verified on the POSIX path against the real header: a 300ms wait with no
signal returns at 300ms, and a wait with a 5s timeout signalled at 50ms wakes
at 51ms.
…er early
Video PTS is mapped through the audio playout offset for lip sync, so a
frame's correct display moment is roughly one audio buffer ahead of now.
Handing that to libobs immediately makes libobs hold it, and past
MAX_ASYNC_FRAMES (30) held frames cache_video() drops the incoming frame,
throws the whole queue away and resets last_frame_ts, with nothing logged.
That is the freeze-frame report this branch started from, and the reason the
lead cap existed: it traded lip sync for queue depth, and on a 720p120 stream
it cost half a second of desync for a queue that was one frame deep.
Pace in the plugin instead, the way OBS's own media source does
(mp_media_sleep in shared/media-playback): keep the frame and hand it over
when it is due. The timestamp maths is untouched — the same mapped value goes
to libobs, sampled at the same point — only the moment of handover changes.
libobs then holds about one frame, its 30-frame limit stops being reachable,
and no lead cap is needed at any frame rate.
Modelled against ready_async_frame/cache_video across 30/60/120fps at 120ms
and 1030ms leads: libobs's queue peaks at 1-3 frames with zero wipes in every
combination, where 120fps at a 1030ms lead previously overflowed continuously.
Two consequences worth knowing:
- Decoded frames are copied out of the hardware pool on arrival rather than
mapped. av_hwframe_map's CPU view still pins the surface it maps, and
pacing holds frames for the whole lead, so mapping would drain the
decoder pool within a few frames. VAAPI and VideoToolbox pay one extra
copy for this; D3D11VA and CUDA already fell back to a copy.
- An N-millisecond lead now means N milliseconds of decoded video held in
this process. That is not new — libobs was storing the same thing, capped
at 30 frames and discarded without warning — but it is now ours and
bounded explicitly, by frame count and by bytes, whichever binds first.
Past either the head goes out early, which is precisely the old
behaviour, and it is counted rather than silent.
The pacing queue needs no lock: the receiver thread never touches it and a
clear is routed through video_clear_pending, which the video thread consumes.
Its counters are mirrored under video_queue_lock once per cycle so the stats
line has something synchronised to read (paced=now/peak(MB) early=N).
A 720p120 test stream sat at buf=952ms against a 120ms target for its whole
run, with playback pinned at +5% and av_drift falling 1485ms over 30s — the
drain working at full authority and being refilled just as fast. The sender
was delivering roughly 5% faster than real time.
Nothing here is malfunctioning. Regulation is speed-only by design and the
drain is bounded at +5%, so a sender faster than that cannot be caught: the
buffer rises to the read loop's bleed ceiling, backpressure holds it there,
and latency sits at the ceiling until the sender slows down. Draining harder
would mean skipping audio, which this design does not do once primed.
What was wrong is that it looked exactly like healthy operation. Every
quality counter stays zero — no underruns, no resyncs, no trims — because
nothing is being lost. The only visible symptom is the number of milliseconds
in buf=, which reads as a tuning artefact rather than a stuck controller.
Report it instead: once playback has been at full authority for 20s without
the fill coming down, log what is happening and why the plugin cannot fix it.
Twenty seconds sits clear of a legitimate burst, which drains from the
ceiling back under buffer_max in about 13s at the default target, and the
window resets whenever fill drops by 100ms so a slow but real recovery stays
quiet. Modelled against senders at 0%, +2%, +5% and +8% over real time: the
first two stay silent, the last two report at 20s.
Two things this does not change, both worth knowing:
- The bleed ceiling is min(buffer_max*3, 1000ms), so the worst-case
latency lands between 660ms and 1000ms across the whole 20..500ms
Target Buffer range — largely independent of the setting the README
calls the main latency knob. That is deliberate burst headroom, and it
only becomes the operating point when a sender outruns the drain.
- irl_audio_maybe_reanchor_offset() bails while fill is above target, so a
pinned buffer also disables the concealment-latency reclaim. A stream
that hit both would recover from neither.
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
New Features
Bug Fixes
Documentation