From 9ea3d3455d599178af64f6ef1d2832b82ba95638 Mon Sep 17 00:00:00 2001 From: Andreas Pehrson Date: Fri, 14 Jun 2024 22:33:02 +0200 Subject: [PATCH] Only apply VPIO properties while running An Apple bug breaks, at least, the bypass property if it is set prior to starting the VPIO unit. Broken here means that the property can be set to any value later while running, without having an effect. The VPIO defaults is bypass OFF and AGC ON. Starting like that and then turning on bypass as requested is not going to cause an unexpected glitch. We reset the VPIO to defaults before stopping so that on reuse it doesn't risk causing a glitch either. --- src/backend/mod.rs | 98 ++++++++++++++++++++++++--------- src/backend/tests/interfaces.rs | 1 + 2 files changed, 73 insertions(+), 26 deletions(-) diff --git a/src/backend/mod.rs b/src/backend/mod.rs index b66398f5..c8171e9b 100644 --- a/src/backend/mod.rs +++ b/src/backend/mod.rs @@ -377,13 +377,7 @@ fn set_input_processing_params(unit: AudioUnit, params: InputProcessingParams) - let aec = params.contains(InputProcessingParams::ECHO_CANCELLATION); let ns = params.contains(InputProcessingParams::NOISE_SUPPRESSION); let agc = params.contains(InputProcessingParams::AUTOMATIC_GAIN_CONTROL); - - // AEC and NS are active as soon as VPIO is not bypassed, therefore the only combinations - // of those we can explicitly support are {} and {aec, ns}. - if aec != ns { - // No control to turn on AEC without NS or vice versa. - return Err(Error::error()); - } + assert_eq!(aec, ns); let mut old_agc: u32 = 0; let r = audio_unit_get_property( @@ -419,6 +413,11 @@ fn set_input_processing_params(unit: AudioUnit, params: InputProcessingParams) - ); return Err(Error::error()); } + cubeb_log!( + "set_input_processing_params on unit {:p} - set agc: {}", + unit, + agc + ); } let mut old_bypass: u32 = 0; @@ -455,6 +454,11 @@ fn set_input_processing_params(unit: AudioUnit, params: InputProcessingParams) - ); return Err(Error::error()); } + cubeb_log!( + "set_input_processing_params on unit {:p} - set bypass: {}", + unit, + bypass + ); } Ok(()) @@ -3015,6 +3019,7 @@ struct CoreStreamData<'ctx> { input_processing_params: InputProcessingParams, input_mute: bool, input_buffer_manager: Option, + units_running: bool, // Listeners indicating what system events are monitored. default_input_listener: Option, default_output_listener: Option, @@ -3064,6 +3069,7 @@ impl<'ctx> Default for CoreStreamData<'ctx> { input_processing_params: InputProcessingParams::NONE, input_mute: false, input_buffer_manager: None, + units_running: false, default_input_listener: None, default_output_listener: None, input_alive_listener: None, @@ -3119,6 +3125,7 @@ impl<'ctx> CoreStreamData<'ctx> { input_processing_params: InputProcessingParams::NONE, input_mute: false, input_buffer_manager: None, + units_running: false, default_input_listener: None, default_output_listener: None, input_alive_listener: None, @@ -3145,7 +3152,7 @@ impl<'ctx> CoreStreamData<'ctx> { stm.queue.debug_assert_is_current(); } - fn start_audiounits(&self) -> Result<()> { + fn start_audiounits(&mut self) -> Result<()> { self.debug_assert_is_on_stream_queue(); // Only allowed to be called after the stream is initialized // and before the stream is destroyed. @@ -3156,22 +3163,50 @@ impl<'ctx> CoreStreamData<'ctx> { } if self.using_voice_processing_unit() { // Handle the VoiceProcessIO case where there is a single unit. + + // Always try to remember the applied input processing params. If they cannot + // be applied in the new device pair, we notify the client of an error and it + // will have to open a new stream. + if let Err(r) = + set_input_processing_params(self.input_unit, self.input_processing_params) + { + cubeb_log!( + "({:p}) Failed to set params of voiceprocessing. Error: {}", + self.stm_ptr, + r + ); + return Err(r); + } return Ok(()); } if !self.output_unit.is_null() { start_audiounit(self.output_unit)?; } + self.units_running = true; Ok(()) } - fn stop_audiounits(&self) { + fn stop_audiounits(&mut self) { self.debug_assert_is_on_stream_queue(); + self.units_running = false; if !self.input_unit.is_null() { let r = stop_audiounit(self.input_unit); assert!(r.is_ok()); } if self.using_voice_processing_unit() { // Handle the VoiceProcessIO case where there is a single unit. + + // Always reset input processing params to VPIO defaults in case VPIO is reused later. + let vpio_defaults = InputProcessingParams::ECHO_CANCELLATION + | InputProcessingParams::AUTOMATIC_GAIN_CONTROL + | InputProcessingParams::NOISE_SUPPRESSION; + if let Err(r) = set_input_processing_params(self.input_unit, vpio_defaults) { + cubeb_log!( + "({:p}) Failed to reset params of voiceprocessing. Error: {}", + self.stm_ptr, + r + ); + } return; } if !self.output_unit.is_null() { @@ -4095,20 +4130,6 @@ impl<'ctx> CoreStreamData<'ctx> { ); return Err(r); } - - // Always try to remember the applied input processing params. If they cannot - // be applied in the new device pair, we notify the client of an error and it - // will have to open a new stream. - if let Err(r) = - set_input_processing_params(self.input_unit, self.input_processing_params) - { - cubeb_log!( - "({:p}) Failed to set params of voiceprocessing. Error: {}", - self.stm_ptr, - r - ); - return Err(r); - } } if let Err(r) = self.install_system_changed_callback() { @@ -4838,6 +4859,7 @@ impl<'ctx> StreamOps for AudioUnitStream<'ctx> { // Execute start in serial queue to avoid racing with destroy or reinit. let result = self .queue + .clone() .run_sync(|| self.core_stream_data.start_audiounits()) .unwrap(); @@ -4855,6 +4877,7 @@ impl<'ctx> StreamOps for AudioUnitStream<'ctx> { if !self.stopped.swap(true, Ordering::SeqCst) { // Execute stop in serial queue to avoid racing with destroy or reinit. self.queue + .clone() .run_sync(|| self.core_stream_data.stop_audiounits()); self.notify_state_changed(State::Stopped); @@ -4997,6 +5020,20 @@ impl<'ctx> StreamOps for AudioUnitStream<'ctx> { return Err(Error::invalid_parameter()); } + // AEC and NS are active as soon as VPIO is not bypassed, therefore the only combinations + // of those we can explicitly support are {} and {aec, ns}. + let aec = params.contains(InputProcessingParams::ECHO_CANCELLATION); + let ns = params.contains(InputProcessingParams::NOISE_SUPPRESSION); + if aec != ns { + // No control to turn on AEC without NS or vice versa. + cubeb_log!( + "Cubeb stream ({:p}) couldn't set input processing params {:?}. AEC != NS.", + self as *const AudioUnitStream, + params + ); + return Err(Error::error()); + } + // CUBEB_ERROR if params could not be applied // note: only works with VoiceProcessingIO if !self.core_stream_data.using_voice_processing_unit() { @@ -5005,17 +5042,26 @@ impl<'ctx> StreamOps for AudioUnitStream<'ctx> { // Execute set_input_processing_params in serial queue to avoid racing with destroy or reinit. let mut result = Err(Error::error()); - let set = &mut result; + let result_ = &mut result; + let mut deferred = false; + let deferred_ = &mut deferred; let stream = &self; self.queue.run_sync(move || { - *set = set_input_processing_params(stream.core_stream_data.input_unit, params); + if stream.core_stream_data.units_running { + *deferred_ = true; + *result_ = Ok(()); + } else { + *deferred_ = false; + *result_ = set_input_processing_params(stream.core_stream_data.input_unit, params); + } }); result?; cubeb_log!( - "Cubeb stream ({:p}) set input processing params to {:?}.", + "Cubeb stream ({:p}) {} input processing params {:?}.", self as *const AudioUnitStream, + if deferred { "deferred" } else { "set" }, params ); self.core_stream_data.input_processing_params = params; diff --git a/src/backend/tests/interfaces.rs b/src/backend/tests/interfaces.rs index c3f4a388..3da50cc2 100644 --- a/src/backend/tests/interfaces.rs +++ b/src/backend/tests/interfaces.rs @@ -1623,6 +1623,7 @@ fn test_ops_timing_sensitive_multiple_duplex_voice_stream_params() { |stream| { let stm = unsafe { &mut *(stream as *mut AudioUnitStream) }; assert!(stm.core_stream_data.using_voice_processing_unit()); + assert_eq!(unsafe { OPS.stream_start.unwrap()(stream) }, ffi::CUBEB_OK); let queue = stm.queue.clone(); // Test that input processing params does not carry over when reusing vpio. let mut bypass: u32 = 0;