Fix the inconsitent active audio node issue caused by multiple NodesChanged signal. When multiple NodesChanged signal are fired for a single node change, GetNodes returns stale nodes data from the later NodesChanged signal, which could overwrite the latest active audio node.

BUG=292741

Review URL: https://chromiumcodereview.appspot.com/23736008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223931 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 224 insertions, 10 deletions

diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc
index 60663eb..de6313f 100644
--- a/chromeos/audio/cras_audio_handler.cc
+++ b/chromeos/audio/cras_audio_handler.cc
@@ -529,16 +529,8 @@ bool CrasAudioHandler::NonActiveDeviceUnplugged(
     size_t old_devices_size,
     size_t new_devices_size,
     uint64 current_active_node) {
-  // There could be cases that more than one NodesChanged signals are
-  // triggered by cras for unplugging or plugging one audio devices, both coming
-  // with the same node data. After handling the first NodesChanged signal, the
-  // audio_devices_ can be overwritten by staled node data from handling 2nd
-  // NodesChanged signal. Therefore, we need to check if the device with
-  // current_active_node is consistently active or not.
-  // crbug.com/274641.
   return (new_devices_size <= old_devices_size &&
-          GetDeviceFromId(current_active_node) &&
-          audio_devices_[current_active_node].active);
+          GetDeviceFromId(current_active_node));
 }
 
 void CrasAudioHandler::SwitchToDevice(const AudioDevice& device) {
@@ -592,6 +584,49 @@ bool CrasAudioHandler::FoundNewDevice(const AudioDevice& device) {
   return false;
 }
 
+// Sanitize the audio node data. When a device is plugged in or unplugged, there
+// should be only one NodesChanged signal from cras. However, we've observed
+// the case that multiple NodesChanged signals being sent from cras. After the
+// first NodesChanged being processed, chrome sets the active node properly.
+// However, the NodesChanged received after the first one, can return stale
+// nodes data in GetNodes call, the staled nodes data does not reflect the
+// latest active node state. Since active audio node should only be set by
+// chrome, the inconsistent data from cras could be the result of stale data
+// described above and sanitized.
+AudioDevice CrasAudioHandler::GetSanitizedAudioDevice(const AudioNode& node) {
+  AudioDevice device(node);
+  if (device.is_input) {
+    if (device.active && device.id != active_input_node_id_) {
+      LOG(WARNING) << "Stale audio device data, should not be active: "
+          << " device = " << device.ToString()
+          << " current active input node id = 0x" << std::hex
+          << active_input_node_id_;
+      device.active = false;
+    } else if (device.id == active_input_node_id_ && !device.active) {
+      LOG(WARNING) << "Stale audio device data, should be active:"
+          << " device = " << device.ToString()
+          << " current active input node id = 0x" << std::hex
+          << active_input_node_id_;
+      device.active = true;
+    }
+  } else {
+    if (device.active && device.id != active_output_node_id_) {
+      LOG(WARNING) << "Stale audio device data, should not be active: "
+          << " device = " << device.ToString()
+          << " current active output node id = 0x" << std::hex
+          << active_output_node_id_;
+      device.active = false;
+    } else if (device.id == active_output_node_id_ && !device.active) {
+      LOG(WARNING) << "Stale audio device data, should be active:"
+          << " device = " << device.ToString()
+          << " current active output node id = 0x" << std::hex
+          << active_output_node_id_;
+      device.active = true;
+    }
+  }
+  return device;
+}
+
 void CrasAudioHandler::UpdateDevicesAndSwitchActive(
     const AudioNodeList& nodes) {
   size_t old_audio_devices_size = audio_devices_.size();
@@ -607,7 +642,7 @@ void CrasAudioHandler::UpdateDevicesAndSwitchActive(
     output_devices_pq_.pop();
 
   for (size_t i = 0; i < nodes.size(); ++i) {
-    AudioDevice device(nodes[i]);
+    AudioDevice device = GetSanitizedAudioDevice(nodes[i]);
     audio_devices_[device.id] = device;
 
     if (!has_alternative_input_ &&
diff --git a/chromeos/audio/cras_audio_handler.h b/chromeos/audio/cras_audio_handler.h
index 6777ced..f2a35a5 100644
--- a/chromeos/audio/cras_audio_handler.h
+++ b/chromeos/audio/cras_audio_handler.h
@@ -241,6 +241,9 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
   // Returns true if |device| is not found in audio_devices_.
   bool FoundNewDevice(const AudioDevice& device);
 
+  // Returns a sanitized AudioDevice from |node|.
+  AudioDevice GetSanitizedAudioDevice(const AudioNode& node);
+
   scoped_refptr<AudioDevicesPrefHandler> audio_pref_handler_;
   base::WeakPtrFactory<CrasAudioHandler> weak_ptr_factory_;
   ObserverList<AudioObserver> observers_;
diff --git a/chromeos/audio/cras_audio_handler_unittest.cc b/chromeos/audio/cras_audio_handler_unittest.cc
index f584f21..573daea 100644
--- a/chromeos/audio/cras_audio_handler_unittest.cc
+++ b/chromeos/audio/cras_audio_handler_unittest.cc
@@ -381,6 +381,10 @@ TEST_F(CrasAudioHandlerTest, PlugHeadphone) {
   EXPECT_FALSE(cras_audio_handler_->has_alternative_output());
 
   // Plug the headphone.
+  audio_nodes.clear();
+  AudioNode internal_speaker(kInternalSpeaker);
+  internal_speaker.active = true;
+  audio_nodes.push_back(internal_speaker);
   audio_nodes.push_back(kHeadphone);
   ChangeAudioNodes(audio_nodes);
 
@@ -512,6 +516,7 @@ TEST_F(CrasAudioHandlerTest, ConnectAndDisconnectBluetoothHeadset) {
   // Disconnect bluetooth headset.
   audio_nodes.clear();
   audio_nodes.push_back(kInternalSpeaker);
+  headphone.active = false;
   audio_nodes.push_back(headphone);
   ChangeAudioNodes(audio_nodes);
 
@@ -1003,6 +1008,10 @@ TEST_F(CrasAudioHandlerTest, PlugUSBMic) {
   EXPECT_FALSE(cras_audio_handler_->has_alternative_input());
 
   // Plug the USB Mic.
+  audio_nodes.clear();
+  AudioNode internal_mic(kInternalMic);
+  internal_mic.active = true;
+  audio_nodes.push_back(internal_mic);
   audio_nodes.push_back(kUSBMic);
   ChangeAudioNodes(audio_nodes);
 
@@ -1093,6 +1102,14 @@ TEST_F(CrasAudioHandlerTest, PlugUSBMicNotAffectActiveOutput) {
   EXPECT_EQ(kInternalSpeaker.id, cras_audio_handler_->GetActiveOutputNode());
 
   // Plug the USB Mic.
+  audio_nodes.clear();
+  AudioNode internal_speaker_node(kInternalSpeaker);
+  internal_speaker_node.active = true;
+  audio_nodes.push_back(internal_speaker_node);
+  audio_nodes.push_back(kHeadphone);
+  AudioNode internal_mic(kInternalMic);
+  internal_mic.active = true;
+  audio_nodes.push_back(internal_mic);
   audio_nodes.push_back(kUSBMic);
   ChangeAudioNodes(audio_nodes);
 
@@ -1114,6 +1131,165 @@ TEST_F(CrasAudioHandlerTest, PlugUSBMicNotAffectActiveOutput) {
   EXPECT_EQ(kInternalSpeaker.id, cras_audio_handler_->GetActiveOutputNode());
 }
 
+TEST_F(CrasAudioHandlerTest, MultipleNodesChangedSignalsOnPlugInHeadphone) {
+  // Set up initial audio devices.
+  AudioNodeList audio_nodes;
+  audio_nodes.push_back(kInternalSpeaker);
+  audio_nodes.push_back(kBluetoothHeadset);
+  SetUpCrasAudioHandler(audio_nodes);
+  const size_t init_nodes_size = audio_nodes.size();
+
+  // Verify the audio devices size.
+  EXPECT_EQ(0, test_observer_->audio_nodes_changed_count());
+  AudioDeviceList audio_devices;
+  cras_audio_handler_->GetAudioDevices(&audio_devices);
+  EXPECT_EQ(init_nodes_size, audio_devices.size());
+
+  // Verify the bluetooth headset is selected as the active output.
+  EXPECT_EQ(0, test_observer_->active_output_node_changed_count());
+  EXPECT_EQ(kBluetoothHeadsetId, cras_audio_handler_->GetActiveOutputNode());
+  AudioDevice active_output;
+  EXPECT_TRUE(cras_audio_handler_->GetActiveOutputDevice(&active_output));
+  EXPECT_TRUE(cras_audio_handler_->has_alternative_output());
+
+  // Plug in headphone, but fire NodesChanged signal twice.
+  audio_nodes.clear();
+  audio_nodes.push_back(kInternalSpeaker);
+  AudioNode bluetooth_headset(kBluetoothHeadset);
+  bluetooth_headset.plugged_time = 1000;
+  bluetooth_headset.active = true;
+  audio_nodes.push_back(bluetooth_headset);
+  AudioNode headphone(kHeadphone);
+  headphone.active = false;
+  headphone.plugged_time = 2000;
+  audio_nodes.push_back(headphone);
+  ChangeAudioNodes(audio_nodes);
+  ChangeAudioNodes(audio_nodes);
+
+  // Verify the active output device is set to headphone.
+  EXPECT_EQ(2, test_observer_->audio_nodes_changed_count());
+  EXPECT_EQ(1, test_observer_->active_output_node_changed_count());
+  EXPECT_EQ(headphone.id, cras_audio_handler_->GetActiveOutputNode());
+  EXPECT_TRUE(cras_audio_handler_->GetActiveOutputDevice(&active_output));
+  EXPECT_EQ(headphone.id, active_output.id);
+
+  // Verfiy the audio devices data is consistent, i.e., the active output device
+  // should be headphone.
+  cras_audio_handler_->GetAudioDevices(&audio_devices);
+  EXPECT_EQ(init_nodes_size + 1, audio_devices.size());
+  for (size_t i = 0; i < audio_devices.size(); ++i) {
+    if (audio_devices[i].id == kInternalSpeaker.id)
+      EXPECT_FALSE(audio_devices[i].active);
+    else if (audio_devices[i].id == bluetooth_headset.id)
+      EXPECT_FALSE(audio_devices[i].active);
+    else if (audio_devices[i].id == headphone.id)
+      EXPECT_TRUE(audio_devices[i].active);
+    else
+      NOTREACHED();
+  }
+}
+
+TEST_F(CrasAudioHandlerTest, MultipleNodesChangedSignalsOnPlugInUSBMic) {
+  // Set up initial audio devices.
+  AudioNodeList audio_nodes;
+  audio_nodes.push_back(kInternalMic);
+  SetUpCrasAudioHandler(audio_nodes);
+  const size_t init_nodes_size = audio_nodes.size();
+
+  // Verify the audio devices size.
+  EXPECT_EQ(0, test_observer_->audio_nodes_changed_count());
+  AudioDeviceList audio_devices;
+  cras_audio_handler_->GetAudioDevices(&audio_devices);
+  EXPECT_EQ(init_nodes_size, audio_devices.size());
+
+  // Verify the internal mic is selected as the active output.
+  EXPECT_EQ(0, test_observer_->active_output_node_changed_count());
+  EXPECT_EQ(kInternalMic.id, cras_audio_handler_->GetActiveInputNode());
+  EXPECT_FALSE(cras_audio_handler_->has_alternative_output());
+  EXPECT_TRUE(audio_devices[0].active);
+
+  // Plug in usb mic, but fire NodesChanged signal twice.
+  audio_nodes.clear();
+  AudioNode internal_mic(kInternalMic);
+  internal_mic.active = true;
+  internal_mic.plugged_time = 1000;
+  audio_nodes.push_back(internal_mic);
+  AudioNode usb_mic(kUSBMic);
+  usb_mic.active = false;
+  usb_mic.plugged_time = 2000;
+  audio_nodes.push_back(usb_mic);
+  ChangeAudioNodes(audio_nodes);
+  ChangeAudioNodes(audio_nodes);
+
+  // Verify the active output device is set to headphone.
+  EXPECT_EQ(2, test_observer_->audio_nodes_changed_count());
+  EXPECT_EQ(1, test_observer_->active_input_node_changed_count());
+  EXPECT_EQ(usb_mic.id, cras_audio_handler_->GetActiveInputNode());
+  EXPECT_TRUE(cras_audio_handler_->has_alternative_input());
+
+  // Verfiy the audio devices data is consistent, i.e., the active input device
+  // should be usb mic.
+  cras_audio_handler_->GetAudioDevices(&audio_devices);
+  EXPECT_EQ(init_nodes_size + 1, audio_devices.size());
+  for (size_t i = 0; i < audio_devices.size(); ++i) {
+    if (audio_devices[i].id == kInternalMic.id)
+      EXPECT_FALSE(audio_devices[i].active);
+    else if (audio_devices[i].id == usb_mic.id)
+      EXPECT_TRUE(audio_devices[i].active);
+    else
+      NOTREACHED();
+  }
+}
+
+// This is the case of crbug.com/291303.
+TEST_F(CrasAudioHandlerTest, MultipleNodesChangedSignalsOnSystemBoot) {
+  // Set up audio handler with empty audio_nodes.
+  AudioNodeList audio_nodes;
+  SetUpCrasAudioHandler(audio_nodes);
+
+  AudioNode internal_speaker(kInternalSpeaker);
+  internal_speaker.active = false;
+  AudioNode headphone(kHeadphone);
+  headphone.active = false;
+  AudioNode internal_mic(kInternalMic);
+  internal_mic.active = false;
+  audio_nodes.push_back(internal_speaker);
+  audio_nodes.push_back(headphone);
+  audio_nodes.push_back(internal_mic);
+  const size_t init_nodes_size = audio_nodes.size();
+
+  // Simulate AudioNodesChanged signal being fired twice during system boot.
+  ChangeAudioNodes(audio_nodes);
+  ChangeAudioNodes(audio_nodes);
+
+  // Verify the active output device is set to headphone.
+  EXPECT_EQ(2, test_observer_->audio_nodes_changed_count());
+  EXPECT_EQ(1, test_observer_->active_output_node_changed_count());
+  EXPECT_EQ(headphone.id, cras_audio_handler_->GetActiveOutputNode());
+  AudioDevice active_output;
+  EXPECT_TRUE(cras_audio_handler_->GetActiveOutputDevice(&active_output));
+  EXPECT_EQ(headphone.id, active_output.id);
+
+  // Verify the active input device id is set to internal mic.
+  EXPECT_EQ(internal_mic.id, cras_audio_handler_->GetActiveInputNode());
+
+  // Verfiy the audio devices data is consistent, i.e., the active output device
+  // should be headphone, and the active input device should internal mic.
+  AudioDeviceList audio_devices;
+  cras_audio_handler_->GetAudioDevices(&audio_devices);
+  EXPECT_EQ(init_nodes_size, audio_devices.size());
+  for (size_t i = 0; i < audio_devices.size(); ++i) {
+    if (audio_devices[i].id == internal_speaker.id)
+      EXPECT_FALSE(audio_devices[i].active);
+    else if (audio_devices[i].id == headphone.id)
+      EXPECT_TRUE(audio_devices[i].active);
+    else if (audio_devices[i].id == internal_mic.id)
+      EXPECT_TRUE(audio_devices[i].active);
+    else
+      NOTREACHED();
+  }
+}
+
 TEST_F(CrasAudioHandlerTest, SetOutputMute) {
   AudioNodeList audio_nodes;
   audio_nodes.push_back(kInternalSpeaker);