diff --git a/Cargo.lock b/Cargo.lock index 90d3158..b9056ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -356,6 +356,7 @@ dependencies = [ "ndk-context", "rand 0.8.6", "reqwest", + "rtrb", "sdl2", "thiserror 2.0.18", "tokio", @@ -2662,6 +2663,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rtrb" +version = "0.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ade083ccbb4bf536df69d1f6432cc23deb7acccff86b183f3923a6fd56a1153" + [[package]] name = "rusqlite" version = "0.32.1" diff --git a/apps/chanora_flutter/pubspec.yaml b/apps/chanora_flutter/pubspec.yaml index b519afc..ab5d739 100644 --- a/apps/chanora_flutter/pubspec.yaml +++ b/apps/chanora_flutter/pubspec.yaml @@ -16,7 +16,7 @@ publish_to: 'none' # Remove this line if you wish to publish to pub.dev # https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html # In Windows, build-name is used as the major, minor, and patch parts # of the product and file versions while build-number is used as the build suffix. -version: 1.0.0-rc.8+72 +version: 1.0.0-rc.8+73 environment: sdk: ^3.11.5 diff --git a/crates/chanora_audio/Cargo.toml b/crates/chanora_audio/Cargo.toml index 64eff23..9c44dd5 100644 --- a/crates/chanora_audio/Cargo.toml +++ b/crates/chanora_audio/Cargo.toml @@ -58,6 +58,24 @@ tokio = { version = "1", features = ["sync", "rt", "macros", "time"] } # for AudioUnit construction + property access. coreaudio-rs = "0.14" +# Lock-free SPSC ring buffer to decouple the VPIO render callback +# (consumer, runs on iOS's audio thread with strict realtime +# constraints) from the AudioHandler decoder (producer, runs on a +# tokio worker). Without this, the render callback calls +# AudioHandler::fill_buffer directly under a Mutex, and any time +# the decoder is mid-burst the callback either blocks or gets a +# partial-fill that produces clicks at the partial-fill boundary +# plus choppy fragments from the missing tail. iOS's VPIO calls +# the render callback at irregular intervals (we logged +# `frames_changes=60+ per 100 callbacks` = the buffer size flips +# on ~60% of callbacks); decoupling the two via a stable-rate +# ring buffer is the standard fix used by every production VoIP +# audio engine. `rtrb` 0.3.4 (mgeier, 6.8M downloads) is the +# realtime-safe SPSC ring buffer the Rust audio community +# converged on \u2014 lock-free push / pop with no allocation on +# the audio thread. +rtrb = "0.3" + [target.'cfg(target_os = "android")'.dependencies] # JNI bindings to flip Android's AudioManager into MODE_IN_COMMUNICATION # when the voice-comm preset is requested. ndk_context is initialised diff --git a/crates/chanora_audio/src/ios_voice_unit.rs b/crates/chanora_audio/src/ios_voice_unit.rs index 3705641..afb78bd 100644 --- a/crates/chanora_audio/src/ios_voice_unit.rs +++ b/crates/chanora_audio/src/ios_voice_unit.rs @@ -77,6 +77,7 @@ use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; +use std::time::Duration; use audiopus::coder::Encoder as OpusEncoder; use audiopus::{ @@ -87,6 +88,7 @@ use coreaudio::audio_unit::audio_format::LinearPcmFlags; use coreaudio::audio_unit::render_callback::{self, data}; use coreaudio::audio_unit::{AudioUnit, Element, SampleFormat, Scope, StreamFormat}; use coreaudio::audio_unit::IOType; +use rtrb::{Consumer, Producer, RingBuffer}; use tokio::sync::mpsc; use tracing::{debug, error, info, warn}; use tsclientlib::audio::AudioHandler; @@ -110,6 +112,30 @@ const FRAME_SAMPLES_MONO: usize = 960; /// shared `crate::framing` module. const MAX_OPUS_FRAME: usize = 1275; +/// Ring buffer capacity between the AudioHandler decoder +/// (producer, tokio task) and the VPIO render callback +/// (consumer, iOS audio thread). Sized for 200 ms of mono i16 +/// content at 48 kHz = 9600 samples. Rationale: +/// * Producer pushes one 20 ms chunk (FRAME_SAMPLES_MONO = 960 +/// mono i16 samples) per tick at 50 Hz. +/// * Consumer pulls whatever VPIO asks for (typically 960 or +/// 1104 mono samples per callback at irregular intervals). +/// * 200 ms = 10 producer ticks = enough headroom to absorb +/// iOS callback-size jitter (we logged frames_changes=60 +/// per 100 callbacks at +72) without underrunning when the +/// producer briefly stalls. +/// * Larger than 200 ms adds perceptible latency to the user +/// voice path (200 ms is already noticeable; doubling it +/// would hurt conversational responsiveness). +const RING_BUFFER_SAMPLES: usize = 9600; + +/// Producer task tick interval. Matches the Opus packet rate +/// (50 Hz = 20 ms) so each tick drains exactly one Opus-frame's +/// worth of decoded samples from AudioHandler. Smaller intervals +/// would burn CPU on lock acquisition; larger would risk +/// underrun bursts. +const PRODUCER_TICK_MS: u64 = 20; + /// Sample rate every layer above us assumes. Matches the Opus /// encoder rate, the `tsclientlib::AudioHandler` mix rate, and the /// sample rate we ask iOS to give us via VPIO's StreamFormat. @@ -304,8 +330,16 @@ impl IosCaptureState { /// flowing; drop = audio stopped. pub struct IosVoiceUnit { // Drop order: stop the unit first (severs callbacks), then - // drop the wrapper so `AudioComponentInstanceDispose` runs. + // signal the producer task to exit (its consumer-side + // counterpart in the render callback is gone, so any further + // push would just fill the ring), then drop the wrapper so + // `AudioComponentInstanceDispose` runs. unit: AudioUnit, + /// One-shot channel to signal the producer task to stop. + /// `Option` so `Drop` can `take()` it without owning `&mut self` + /// through Send semantics. Sending fails if the receiver is + /// already dropped (task exited) \u2014 harmless, we ignore. + producer_shutdown_tx: Option>, } impl IosVoiceUnit { @@ -480,132 +514,189 @@ impl IosVoiceUnit { // contract every other backend follows (matches // SdlOutput::callback and the cpal output stream). // - // The closure owns the f32 scratch Vec so we re-use the - // backing allocation across callbacks. The first few - // callbacks may grow it; after that the audio thread - // never touches the allocator. - let mut scratch_stereo: Vec = Vec::with_capacity(FRAME_SAMPLES_MONO * 2); - let handler_for_render = handler.clone(); + // Build the playback pipeline. + // + // Architecture (decoupled producer / consumer; per the + // external review at +72 that identified iOS render-callback + // jitter as the root cause of voice + clicks + chopiness): + // + // Producer task (tokio, 50 Hz timer): + // loop { + // lock AudioHandler + // fill_buffer(scratch_stereo_f32, 1920 samples = 20 ms stereo) + // release lock + // downmix L+R -> mono i16 (960 samples) + // ring_buffer.push_slice(mono_i16) + // sleep 20 ms + // } + // + // Consumer (VPIO render callback, audio thread): + // loop { + // num_frames = args.data.buffer.len() + // pop num_frames i16 samples from ring_buffer into args.data.buffer + // zero-fill tail if ring buffer was short (underrun) + // apply muted/gain post-process + // } + // + // Why this fixes the +72 "voice + clicks + choppy" symptom: + // + // * iOS VPIO calls the render callback at irregular + // intervals (logged frames_changes >= 60 per 100 + // callbacks at +72) and with varying num_frames. + // Calling fill_buffer directly under those conditions + // causes AudioHandler to repeatedly enter its + // `buffering_samples` state (returns &[] = silence) + // when the request size is misaligned with its 20 ms + // internal frame size. Silent gaps in the middle of + // the output buffer create discontinuities = audible + // clicks. + // + // * The ring buffer absorbs the rate mismatch. Producer + // always asks for a stable 20 ms chunk (aligned with + // AudioHandler's internal Opus packet size). Consumer + // pulls whatever iOS asks for whenever iOS schedules + // it; the ring buffer's 200 ms depth covers jitter. + // + // Locking: rtrb is lock-free SPSC. The audio thread + // (consumer) never blocks. The producer task can block on + // the AudioHandler mutex but that's contention with the + // inbound forwarder (handle_packet), not with the audio + // thread \u2014 acceptable. + // + // Allocation: ring buffer allocated once at engine start. + // Audio thread never touches the allocator. Producer task + // allocates one scratch_stereo Vec and re-uses across + // ticks. + let (mut ring_producer, mut ring_consumer): (Producer, Consumer) = + RingBuffer::new(RING_BUFFER_SAMPLES); + + // Spawn the producer task. Owns: + // - Arc> clone (shared with the + // inbound forwarder). + // - Producer end of the ring buffer. + // - Shutdown receiver. + let handler_for_producer = handler.clone(); + let (producer_shutdown_tx, mut producer_shutdown_rx) = + tokio::sync::oneshot::channel::<()>(); + tokio::spawn(async move { + let mut scratch_stereo = vec![0.0_f32; FRAME_SAMPLES_MONO * 2]; + let mut interval = tokio::time::interval(Duration::from_millis(PRODUCER_TICK_MS)); + interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + let mut produced_chunks: u64 = 0; + let mut ring_full_drops: u64 = 0; + loop { + tokio::select! { + _ = &mut producer_shutdown_rx => { + debug!( + target: "chanora_audio", + produced_chunks, + ring_full_drops, + "ios VPIO producer task shutting down" + ); + break; + } + _ = interval.tick() => { + for s in scratch_stereo.iter_mut() { + *s = 0.0; + } + { + let mut h = handler_for_producer.lock().unwrap(); + let _removed = h.fill_buffer(&mut scratch_stereo); + } + let free = ring_producer.slots(); + if free < FRAME_SAMPLES_MONO { + ring_full_drops = ring_full_drops.wrapping_add(1); + if ring_full_drops.is_multiple_of(50) { + warn!( + target: "chanora_audio", + ring_full_drops, + free_slots = free, + "ios VPIO ring buffer overflow (consumer slow)" + ); + } + continue; + } + for i in 0..FRAME_SAMPLES_MONO { + let l = scratch_stereo[i * 2]; + let r = scratch_stereo[i * 2 + 1]; + let mono_f32 = (l + r) * 0.5; + let clamped = mono_f32.clamp(-1.0, 1.0); + let sample = (clamped * i16::MAX as f32) as i16; + let _ = ring_producer.push(sample); + } + produced_chunks = produced_chunks.wrapping_add(1); + } + } + } + }); + + // Render callback (consumer). Pulls mono i16 samples from + // the ring buffer into the VPIO output buffer. let output_gain_for_render = output_gain.clone(); let output_muted_for_render = output_muted.clone(); - // Diagnostic counters (per external review pointing out - // that "peak alone is insufficient" \u2014 we also need - // RMS, clip count, underrun count, callback frame size - // variability). Sampled every 100 callbacks (~2 s). let mut cb_count: u64 = 0; let mut last_num_frames: usize = 0; let mut num_frames_changes: u32 = 0; + let mut underruns: u64 = 0; + let mut underrun_samples: u64 = 0; unit.set_render_callback(move |args: render_callback::Args>| { let out: &mut [i16] = args.data.buffer; - // VPIO with our pinned stream format gives us a mono - // buffer; `num_frames` == `out.len()`. Stereo f32 - // scratch is twice that. let num_frames = out.len(); - let needed = num_frames * 2; - if scratch_stereo.len() < needed { - scratch_stereo.resize(needed, 0.0); - } - // Zero only the live slice. AudioHandler::fill_buffer - // writes silence into untouched samples but residual - // values from earlier callbacks (when the buffer was - // bigger) would leak through otherwise. `fill` - // optimises to memset. - scratch_stereo[..needed].fill(0.0); - { - let mut h = handler_for_render.lock().unwrap(); - let _removed_ids = h.fill_buffer(&mut scratch_stereo[..needed]); - // `_removed_ids` lists clients whose stream just - // finished draining; the bridge layer surfaces - // "stopped speaking" via BridgeEvent::VoiceState - // already, so we ignore it here (matches SDL + - // cpal output behaviour). + // Pop up to num_frames samples from the ring buffer + // into the output. Anything not filled stays at + // whatever was there (we zero-fill the tail + // explicitly below to avoid stale-buffer clicks). + let mut filled: usize = 0; + while filled < num_frames { + match ring_consumer.pop() { + Ok(sample) => { + out[filled] = sample; + filled += 1; + } + Err(_) => { + // Ring empty (producer behind or no audio). + // Zero-fill the rest \u2014 same contract as + // the cpal/SDL paths (silence on underrun + // is acceptable; click on stale memory is + // not). + for s in out[filled..].iter_mut() { + *s = 0; + } + underruns = underruns.wrapping_add(1); + underrun_samples = + underrun_samples.wrapping_add((num_frames - filled) as u64); + break; + } + } } + // Post-process: mute then gain. Gain is applied + // CONSUMER-SIDE (not producer-side) so user-driven + // volume changes take effect on the next callback + // rather than after the current ring contents drain + // (\u2264 200 ms latency). if output_muted_for_render.load(Ordering::Relaxed) { for s in out.iter_mut() { *s = 0; } return Ok(()); } - let gain = f32::from_bits(output_gain_for_render.load(Ordering::Relaxed)); - // Downmix stereo -> mono with master gain. `(l + r) * 0.5` - // averaging preserves total signal energy with a 3 dB - // headroom against sum-of-correlated-peaks clipping. - // Multiplying by gain after the downmix saves one - // multiplication per sample. - // - // Cast to i16 with saturate-on-overflow. Hard-clip is - // acceptable here because the upstream f32 stereo signal - // is already in [-1.0, 1.0] from the AudioHandler mix; - // gain values >1.0 are the only path to clipping and - // hard-clip at the engine boundary matches what every - // other backend's i16 path does (see the cpal-side - // FromF32 for i16 impl in engine.rs). - // - // Note: no iOS-specific output boost. The historical - // "low playback volume" reports on VPIO output stemmed - // from the AVAudioSession mode .voiceChat aggressively - // ducking output to the earpiece. Once the session mode - // is set to .default with .defaultToSpeaker (see - // AppDelegate.swift), the speaker drives at normal - // media-channel loudness and no software boost is - // needed. Pattern documented by Twilio's WebRTC iOS - // SDK + Daily.co; reverts the 8x boost from c16318c + - // e85a6d3 which were treating the symptom not the - // cause. - for (i, dst) in out.iter_mut().enumerate() { - let l = scratch_stereo[i * 2]; - let r = scratch_stereo[i * 2 + 1]; - let mono_f32 = (l + r) * 0.5 * gain; - let clamped = mono_f32.clamp(-1.0, 1.0); - *dst = (clamped * i16::MAX as f32) as i16; + if (gain - 1.0).abs() > f32::EPSILON { + for s in out.iter_mut() { + let scaled = (*s as f32) * gain; + *s = scaled.clamp(i16::MIN as f32, i16::MAX as f32) as i16; + } } - // Comprehensive callback diagnostic (per external - // review). Reports: - // * num_frames : VPIO buffer size this call. - // Should be stable; transitions - // indicate iOS re-negotiating. - // * frames_change_cnt : count of times num_frames - // changed across callbacks. - // Non-trivial = iOS jitter. - // * peak_stereo_f32 : peak |sample| of AudioHandler - // output before downmix. - // * rms_stereo_f32 : RMS over the scratch buffer. - // Gives loudness perception not - // just transient peaks. - // * peak_out_i16 : peak |sample| handed to VPIO. - // * clip_count_i16 : samples at \u00b132767 (digital - // clipping). Non-zero with our - // gain=1.0 indicates upstream - // is already at full scale. - // * input_was_zero : true if AudioHandler returned - // silence (jitter underrun or - // no talker). Distinguishes - // "no audio to play" from - // "audio reached us but - // broken". - // * gain : current master output gain. + // Diagnostic sampling (~2 Hz). if last_num_frames != 0 && last_num_frames != num_frames { num_frames_changes = num_frames_changes.wrapping_add(1); } last_num_frames = num_frames; cb_count = cb_count.wrapping_add(1); if cb_count.is_multiple_of(100) { - // Scratch peak + RMS. - let mut peak_stereo: f32 = 0.0; - let mut sumsq: f64 = 0.0; - for &s in scratch_stereo[..needed].iter() { - let a = s.abs(); - if a > peak_stereo { - peak_stereo = a; - } - sumsq += (s as f64) * (s as f64); - } - let rms_stereo = (sumsq / needed as f64).sqrt() as f32; - // Output peak + clip count. let mut peak_out: i16 = 0; let mut clipped: u32 = 0; for &s in out.iter() { @@ -622,13 +713,12 @@ impl IosVoiceUnit { cb = cb_count, num_frames, frames_changes = num_frames_changes, - peak_stereo = peak_stereo, - rms_stereo = rms_stereo, + underruns, + underrun_samples, peak_out_i16 = peak_out, clip_count_i16 = clipped, - input_was_zero = peak_stereo == 0.0, gain, - "ios VPIO render callback diagnostic sample" + "ios VPIO render callback diagnostic sample (ring-buf consumer)" ); } Ok(()) @@ -697,20 +787,32 @@ impl IosVoiceUnit { ), } - Ok(Self { unit }) + Ok(Self { + unit, + producer_shutdown_tx: Some(producer_shutdown_tx), + }) } } impl Drop for IosVoiceUnit { fn drop(&mut self) { - // `AudioUnit::stop` returns Result but we can't surface it - // from Drop. Log on failure so the chanora.log timeline - // matches engine shutdown. The wrapper's own Drop calls - // AudioComponentInstanceDispose after stop returns. + // Stop the AudioUnit first so the render callback no longer + // fires (consumer side of the ring buffer is now idle). if let Err(e) = self.unit.stop() { warn!(target: "chanora_audio", error = %e, "vpio stop on drop failed"); } else { info!(target: "chanora_audio", "ios VPIO audio unit stopped"); } + // Signal the producer task to exit. Sending fails (Err) + // if the receiver is already dropped \u2014 harmless, + // ignored. The task observes the shutdown signal at its + // next select! iteration (every PRODUCER_TICK_MS = 20 ms + // at most) and exits its loop, dropping the Producer + // end of the ring buffer which makes the Consumer's + // pop() return Err on subsequent reads (now irrelevant + // because the audio unit is stopped). + if let Some(tx) = self.producer_shutdown_tx.take() { + let _ = tx.send(()); + } } }