From b5c9d1358749219d12244cbb26b2161eee91ed56 Mon Sep 17 00:00:00 2001 From: Chun-Min Chang Date: Wed, 10 Jul 2019 08:06:02 +0000 Subject: [PATCH] Bug 1530715 - P8: Always use global latency frames to initialize the cubeb stream. r=padenot The global latency frames in the cubeb context is the latency frames of the first cubeb stream created in that cubeb context. The reason is because latency frames cannot be changed when there is an active stream operating in parallel. To make sure the latency won't be changed, the latency frames of the streams after the first stream will be overwritten by the global latency frames. Since we always use the global latency frames, instead of overwritting latency frames of the later streams, we could, we could simply initialize all the streams with the global latency frames. Differential Revision: https://phabricator.services.mozilla.com/D29980 --HG-- extra : moz-landing-system : lando --- .../cubeb-coreaudio-rs/README_MOZILLA | 2 +- .../cubeb-coreaudio-rs/src/backend/mod.rs | 129 ++++++++++-------- 2 files changed, 71 insertions(+), 60 deletions(-) diff --git a/media/libcubeb/cubeb-coreaudio-rs/README_MOZILLA b/media/libcubeb/cubeb-coreaudio-rs/README_MOZILLA index d63f1bfec91d..85b62962a6a2 100644 --- a/media/libcubeb/cubeb-coreaudio-rs/README_MOZILLA +++ b/media/libcubeb/cubeb-coreaudio-rs/README_MOZILLA @@ -3,4 +3,4 @@ git repository using the update.sh script. The cubeb-coreaudio-rs git repository is: https://github.com/ChunMinChang/cubeb-coreaudio-rs -The git commit ID used was 6b900150dc41d37a9cb46ad733e80c7da6f98bad (2019-06-21 14:16:36 -0700) +The git commit ID used was 43e06d4c526cf4774460152f1d39e55400e1d194 (2019-06-21 14:16:54 -0700) diff --git a/media/libcubeb/cubeb-coreaudio-rs/src/backend/mod.rs b/media/libcubeb/cubeb-coreaudio-rs/src/backend/mod.rs index 38e73f999517..b379adc34853 100644 --- a/media/libcubeb/cubeb-coreaudio-rs/src/backend/mod.rs +++ b/media/libcubeb/cubeb-coreaudio-rs/src/backend/mod.rs @@ -220,6 +220,13 @@ fn set_notification_runloop() { } } +fn clamp_latency(latency_frames: u32) -> u32 { + cmp::max( + cmp::min(latency_frames, SAFE_MAX_LATENCY_FRAMES), + SAFE_MIN_LATENCY_FRAMES, + ) +} + fn audiounit_make_silent(io_data: &mut AudioBuffer) { assert!(!io_data.mData.is_null()); let bytes = unsafe { @@ -2196,13 +2203,40 @@ extern "C" fn audiounit_collection_changed_callback( } #[derive(Debug)] -struct ContextStreams { - active_streams: u32, +struct LatencyController { + streams: u32, + latency: Option, } -impl Default for ContextStreams { +impl LatencyController { + fn add_stream(&mut self, latency: u32) -> Option { + self.streams += 1; + // For the 1st stream set anything within safe min-max + if self.streams == 1 { + assert!(self.latency.is_none()); + // Silently clamp the latency down to the platform default, because we + // synthetize the clock from the callbacks, and we want the clock to update often. + self.latency = Some(clamp_latency(latency)); + } + self.latency + } + + fn subtract_stream(&mut self) -> Option { + self.streams -= 1; + if self.streams == 0 { + assert!(self.latency.is_some()); + self.latency = None; + } + self.latency + } +} + +impl Default for LatencyController { fn default() -> Self { - Self { active_streams: 0 } + Self { + streams: 0, + latency: None, + } } } @@ -2217,7 +2251,6 @@ pub const OPS: Ops = capi_new!(AudioUnitContext, AudioUnitStream); pub struct AudioUnitContext { _ops: *const Ops, mutex: OwnedCriticalSection, - global_latency_frames: u32, input_collection_changed_callback: ffi::cubeb_device_collection_changed_callback, input_collection_changed_user_ptr: *mut c_void, output_collection_changed_callback: ffi::cubeb_device_collection_changed_callback, @@ -2231,7 +2264,7 @@ pub struct AudioUnitContext { serial_queue: dispatch_queue_t, layout: atomic::Atomic, channels: u32, - streams: Mutex, + latency_controller: Mutex, } impl AudioUnitContext { @@ -2239,7 +2272,6 @@ impl AudioUnitContext { Self { _ops: &OPS as *const _, mutex: OwnedCriticalSection::new(), - global_latency_frames: 0, input_collection_changed_callback: None, input_collection_changed_user_ptr: ptr::null_mut(), output_collection_changed_callback: None, @@ -2249,7 +2281,7 @@ impl AudioUnitContext { serial_queue: create_dispatch_queue(DISPATCH_QUEUE_LABEL, DISPATCH_QUEUE_SERIAL), layout: atomic::Atomic::new(ChannelLayout::UNDEFINED), channels: 0, - streams: Mutex::new(ContextStreams::default()), + latency_controller: Mutex::new(LatencyController::default()), } } @@ -2257,25 +2289,19 @@ impl AudioUnitContext { self.mutex.init(); } - fn increase_active_streams(&self) { - let mut streams = self.streams.lock().unwrap(); - streams.active_streams += 1; - } - - fn decrease_active_streams(&self) { - let mut streams = self.streams.lock().unwrap(); - streams.active_streams -= 1; - } - fn active_streams(&self) -> u32 { - let streams = self.streams.lock().unwrap(); - streams.active_streams + let controller = self.latency_controller.lock().unwrap(); + controller.streams } - fn set_global_latency(&mut self, latency_frames: u32) { - self.mutex.assert_current_thread_owns(); - assert_eq!(self.active_streams(), 1); - self.global_latency_frames = latency_frames; + fn update_latency_by_adding_stream(&self, latency_frames: u32) -> Option { + let mut controller = self.latency_controller.lock().unwrap(); + controller.add_stream(latency_frames) + } + + fn update_latency_by_removing_stream(&self) -> Option { + let mut controller = self.latency_controller.lock().unwrap(); + controller.subtract_stream() } fn add_devices_changed_listener( @@ -2550,23 +2576,34 @@ impl ContextOps for AudioUnitContext { state_callback: ffi::cubeb_state_callback, user_ptr: *mut c_void, ) -> Result { - assert!(latency_frames > 0); - if (!input_device.is_null() && input_stream_params.is_none()) || (!output_device.is_null() && output_stream_params.is_none()) { return Err(Error::invalid_parameter()); } + // Latency cannot change if another stream is operating in parallel. In this case + // latency is set to the other stream value. + let global_latency_frames = self + .update_latency_by_adding_stream(latency_frames) + .unwrap(); + if global_latency_frames != latency_frames { + cubeb_log!( + "Use global latency {} instead of the requested latency {}.", + global_latency_frames, + latency_frames + ); + } + let mutex_ptr = &mut self.mutex as *mut OwnedCriticalSection; let _context_lock = AutoLock::new(unsafe { &mut (*mutex_ptr) }); - self.increase_active_streams(); + let mut boxed_stream = Box::new(AudioUnitStream::new( self, user_ptr, data_callback, state_callback, - latency_frames, + global_latency_frames, )); boxed_stream.init_mutex(); @@ -2658,14 +2695,14 @@ impl Drop for AudioUnitContext { let _lock = AutoLock::new(unsafe { &mut (*mutex_ptr) }); { - let streams = self.streams.lock().unwrap(); + let controller = self.latency_controller.lock().unwrap(); // Disabling this assert for bug 1083664 -- we seem to leak a stream - // assert(streams.active_streams == 0); - if streams.active_streams > 0 { + // assert(controller.streams == 0); + if controller.streams > 0 { cubeb_log!( "({:p}) API misuse, {} streams active when context destroyed!", self as *const AudioUnitContext, - streams.active_streams + controller.streams ); } } @@ -3529,15 +3566,6 @@ impl<'ctx> AudioUnitStream<'ctx> { Ok(()) } - fn clamp_latency(&mut self, latency_frames: u32) -> u32 { - // For the 1st stream set anything within safe min-max - assert_eq!(self.context.active_streams(), 1); - cmp::max( - cmp::min(latency_frames, SAFE_MAX_LATENCY_FRAMES), - SAFE_MIN_LATENCY_FRAMES, - ) - } - fn set_buffer_size(&mut self, new_size_frames: u32, side: io_side) -> Result<()> { use std::thread; @@ -4023,23 +4051,6 @@ impl<'ctx> AudioUnitStream<'ctx> { } } - // Latency cannot change if another stream is operating in parallel. In this case - // latency is set to the other stream value. - if self.context.active_streams() > 1 { - cubeb_log!( - "({:p}) More than one active stream, use global latency.", - self as *const AudioUnitStream - ); - self.latency_frames = self.context.global_latency_frames; - } else { - // Silently clamp the latency down to the platform default, because we - // synthetize the clock from the callbacks, and we want the clock to update often. - let latency_frames = self.latency_frames; - self.latency_frames = self.clamp_latency(latency_frames); - assert!(self.latency_frames > 0); // Ugly error check - self.context.set_global_latency(self.latency_frames); - } - // Configure I/O stream if self.has_input() { if let Err(r) = self.configure_input() { @@ -4214,7 +4225,7 @@ impl<'ctx> AudioUnitStream<'ctx> { let _lock = AutoLock::new(unsafe { &mut (*mutex_ptr) }); self.close(); assert!(self.context.active_streams() >= 1); - self.context.decrease_active_streams(); + self.context.update_latency_by_removing_stream(); } fn start_internal(&self) -> Result<()> {