APM in Audio Service: Don't stop monitoring streams erroneously

When an input stream is being processed in the audio service, its
InputController monitors an output stream (actually, its
OutputController) to get the audio it needs to perform echo
cancellation. When the output stream is replaced, the input
controller should stop monitoring the old output stream and start
monitoring the new one. If the creation and destruction of these
streams are interleaved, this would break, as the input controller
would start monitoring the new stream, then stop monitoring any
stream as the old output stream was destroyed. This CL ensures the
input controller only stops monitoring if the destroyed stream is
the one it's currently monitoring.

Bug: 883651
Change-Id: I5731840d8af11ecd76ecd1a25126034c5ff496ac
Reviewed-on: https://chromium-review.googlesource.com/1245711
Commit-Queue: Oskar Sundbom <ossu@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594704}

services/audio/input_controller.cc

@@ -115,9 +115,10 @@ InputController::ProcessingHelper::~ProcessingHelper() {
   DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
 }
 
-void InputController::ProcessingHelper::ChangeStreamMonitor(Snoopable* stream) {
+void InputController::ProcessingHelper::ChangeMonitoredStream(
+    Snoopable* stream) {
   DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
-  TRACE_EVENT1("audio", "AIC ChangeStreamMonitor", "stream", stream);
+  TRACE_EVENT1("audio", "AIC ChangeMonitoredStream", "stream", stream);
   if (!audio_processor_)
     return;
   if (monitored_output_stream_ == stream)
@@ -179,6 +180,27 @@ media::AudioProcessor* InputController::ProcessingHelper::GetAudioProcessor() {
   return audio_processor_.get();
 }
 
+void InputController::ProcessingHelper::StartMonitoringStream(
+    Snoopable* output_stream) {
+  DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
+  DCHECK(audio_processor_);
+  ChangeMonitoredStream(output_stream);
+}
+
+void InputController::ProcessingHelper::StopMonitoringStream(
+    Snoopable* output_stream) {
+  DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
+  DCHECK(audio_processor_);
+  if (output_stream == monitored_output_stream_)
+    ChangeMonitoredStream(nullptr);
+}
+
+void InputController::ProcessingHelper::StopAllStreamMonitoring() {
+  DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
+  DCHECK(audio_processor_);
+  ChangeMonitoredStream(nullptr);
+}
+
 #endif  // defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
 
 // Private subclass of AIC that covers the state while capturing audio.
@@ -420,7 +442,7 @@ void InputController::Close() {
 #if defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
   // Disconnect from any output stream, so we don't get called when we're gone.
   if (processing_helper_)
-    processing_helper_->ChangeStreamMonitor(nullptr);
+    processing_helper_->StopAllStreamMonitoring();
 #endif
 
   std::string log_string;
@@ -536,7 +558,7 @@ void InputController::OnStreamActive(Snoopable* output_stream) {
     case media::EchoCancellationType::kAec3:
 #if defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
       if (processing_helper_)
-        processing_helper_->ChangeStreamMonitor(output_stream);
+        processing_helper_->StartMonitoringStream(output_stream);
 #endif
       break;
     case media::EchoCancellationType::kDisabled:
@@ -549,7 +571,7 @@ void InputController::OnStreamInactive(Snoopable* output_stream) {
   DCHECK_CALLED_ON_VALID_THREAD(owning_thread_);
 #if defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
   if (processing_helper_)
-    processing_helper_->ChangeStreamMonitor(nullptr);
+    processing_helper_->StopMonitoringStream(output_stream);
 #endif
 }
 

services/audio/input_controller.h

@@ -201,13 +201,24 @@ class InputController final : public StreamMonitor {
     void StartEchoCancellationDump(base::File file) final;
     void StopEchoCancellationDump() final;
 
-    // Starts and/or stops snooping and updates |monitored_output_stream_|
-    // appropriately.
-    void ChangeStreamMonitor(Snoopable* output_stream);
-
     media::AudioProcessor* GetAudioProcessor();
 
+    // Starts monitoring |output_stream| instead of the currently monitored
+    // stream, if any.
+    void StartMonitoringStream(Snoopable* output_stream);
+
+    // Stops monitoring |output_stream|, provided it's the currently monitored
+    // stream.
+    void StopMonitoringStream(Snoopable* output_stream);
+
+    // Stops monitoring |monitored_output_stream_|, if not null.
+    void StopAllStreamMonitoring();
+
    private:
+    // Starts and/or stops snooping and updates |monitored_output_stream_|
+    // appropriately.
+    void ChangeMonitoredStream(Snoopable* output_stream);
+
     THREAD_CHECKER(owning_thread_);
 
     const mojo::Binding<mojom::AudioProcessorControls> binding_;