Fix audio and receiver issues, improve performance and safety - #2
Merged
Conversation
pts * 1000000000LL * tb_num / tb_den overflows int64 well within plausible session lengths. with a 90 kHz video timebase the pts * 1e9 multiplication overflows around the 28h mark, and FFmpeg genpts/PTS remappings can land us there sooner. av_rescale_q does the same conversion safely with proper rounding.
- av_packet_alloc / av_frame_alloc in irl_receiver_thread: bail out cleanly instead of crashing on the first deref. - av_strdup in apply_demuxer_options: was passed straight into strtok_r on NULL. - swr_alloc_set_opts2 takes a SwrContext** that it allocates if NULL, so the prior swr_alloc() call was a no-op leak path. Drop it and check the v2 API's return code plus the resulting pointer.
audio_buffer_init published buf->data before pthread_mutex_init. audio_buffer_fill_ms_locked (called from main thread without audio_state_lock by the stats proc_handler) gates locking on buf->data != NULL, so it could lock an uninitialised mutex. Init the mutex first. audio_buffer_free now keys off sample_rate, so a calloc failure (capacity=0, data=NULL) doesn't leak the mutex.
if the audio thread created OK but the receiver thread create failed, thread_active stayed true with an uninitialised receiver_thread handle, and irl_receiver_stop would deadlock joining it. log, unwind, and join the audio thread on failure.
…cting these flags are read from FFmpeg's interrupt callback (potentially on a foreign network thread) and written from main and receiver threads. volatile bool gives no atomicity or memory ordering guarantees on its own. switch to OBS's os_atomic_load_bool / os_atomic_store_bool (seq_cst on both POSIX and Windows backends). declarations stay volatile bool, which matches the os_atomic_* API.
receiver-thread writes to fields read by the audio thread under audio_state_lock now go through the same lock, eliminating the torn-read / lost-update windows the previous code relied on x86 atomicity for. covers: - latest_audio_stream_pts_ns and decoded_frame_samples after each decoded audio frame (receiver-audio.c). - latest_video_stream_pts_ns when video frame lands (receiver-video.c). - fade_in_pending / fade_in_frames_remaining / startup_audio_warmup_remaining_ms in irl_prepare_new_connection and irl_handle_stream_read_error (receiver-stream.c). - audio_recovery_until_us via irl_mark_audio_recovery, which is now always called inside the lock from receiver-side paths (receiver-audio.c, receiver-decode.c, receiver-stream.c). frame_timestamp (called from receiver thread) snapshots the audio playout fields under the lock so the video PTS mapping sees a consistent state instead of a torn pair.
irl_source_get_stats runs on the main OBS thread and previously read audio_buffer state plus a pile of receiver-thread-owned counters without synchronisation. the worst case was racing the receiver thread's audio_buffer_reconfigure (free + realloc of buf->data) while the stats reader was inside audio_buffer_fill_ms_locked. snapshot everything under the lock, then format calldata after release. the snapshot also gives the consumer a single consistent view of the counters instead of a torn mix.
each pump (~50 Hz) was malloc'ing the OBS output buffer and each decoded audio frame was malloc'ing the resample target. on lossy IRL streams those bursts coincide with the worst allocator pressure; the per-frame allocations were a real source of microsecond-scale jitter. add two ctx-owned scratch buffers, grown on demand: - audio_pump_scratch (audio thread) covers normal pump output and the underrun silence path. - audio_resample_scratch (receiver thread) covers swresample target and PTS-repair silence insertion. both are receiver-thread-owned and the silence is consumed (memcpy'd into the jitter buffer) before the next swresample, so the buffer is safe to reuse. both are freed in irl_source_destroy. no lock needed: each scratch is owned by exactly one thread.
the swscale fallback path was malloc'ing a frame-sized buffer per video frame (30-60 Hz, hundreds of KB at 1080p). cache it on the context, grown on demand, and free on destroy. only matters for streams using pixel formats OBS does not accept directly, but it removes a real allocator-pressure source on those streams.
audio_buffer_trim_to_keep_ms drops chunks under one lock until fill falls below the target. the maybe_trim_hidden_audio_backlog path used to relock for every dropped chunk plus an extra fill_ms read, so trimming N chunks was 2N+1 lock acquires. now it's one. maybe_log_audio_timing_diag now only takes the buffer lock to read fill_ms after the throttle gate passes, instead of every pump.
audio_buffer_write previously dropped data into the ring without recording a chunk. downstream audio_buffer_read_pts then returned pts=0 for those bytes while pts_consume kept advancing the chunk queue, gradually desyncing the pts metadata against the pcm. derive the continuation pts from the previous chunk's end (last_pts plus samples_in_chunk / sample_rate) and record a chunk so the queue stays consistent. callers that don't track pts (legacy helpers, future silence/pad paths) get sensible pts continuation without having to compute it themselves. current tree has no caller of this variant; the irl audio path uses audio_buffer_write_pts. this commit defangs the API for any future caller and removes the silent footgun that was waiting in the queue invariants.
ts * 1000 * tb_num overflows int64 around ts ~= 9.2e15 / tb_num, which is reachable on long-running streams with 90 kHz video timebase or AAC at 48 kHz running for many days. av_rescale_q does the same conversion safely with proper rounding. ms_to_ts_ceil follows av_rescale_q_rnd with AV_ROUND_UP so the relock step still rounds up to at least one tick.
the previous behaviour treated every backward jump as a reorder and overwrote last_pts with the smaller value. on the next forward frame, "expected" was lower than reality, the gap looked huge, and we'd insert silence or trigger a reset against an artefact of our own bookkeeping. split the backward case: - gap < small_gap_ms: real reorder / decoder ts wobble. pass the pts through but leave last_pts alone so the baseline keeps tracking the leading edge of the stream. - gap >= small_gap_ms: timeline reset (sender pts wrap, segment remap, decoder reset). reanchor and return PTS_ACTION_RESET so the audio path flushes the buffer and reinits timing, same as for a large forward gap. forward < 1 ms case is unchanged.
avframe linesizes can be negative when the frame is laid out bottom-up. taking abs() before handing the buffer to OBS would present the data as if it were top-down, silently flipping the image vertically. detect any negative linesize and force the swscale fallback path which produces a clean top-down nv12. real ffmpeg decoders almost never emit this (it's mostly a sws_scale flip artifact), but it's cheap to be safe.
bzalloc/bfree match the rest of the plugin's allocations and play nicely with whatever debug allocator OBS may swap in. functionally equivalent to calloc/free on current builds.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR refactors the receiver pipeline to improve thread safety, memory efficiency, and timestamp handling. It introduces atomic operations for thread state management, replaces heap allocations with growable per-thread scratch buffers, upgrades PTS repair logic to use FFmpeg rescaling, and tightens lock coverage around shared audio/video state to prevent races. ChangesAudio Receiver Pipeline Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. 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
Release Notes
Bug Fixes
Performance