Supports multiple active input/output audio nodes in CrasAudioHandler.

chromeos/audio/cras_audio_handler.cc

Index: chromeos/audio/cras_audio_handler.cc
diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc
index 1907190f0ba0e52075ae06e254b6ce3cb27aaead..52d8f9c70a3464f83ddc260c6871b35327d18029 100644
--- a/chromeos/audio/cras_audio_handler.cc
+++ b/chromeos/audio/cras_audio_handler.cc
@@ -170,11 +170,11 @@ int CrasAudioHandler::GetInputGainPercentForDevice(uint64 device_id) {
   }
 }
 
-uint64 CrasAudioHandler::GetActiveOutputNode() const {
+uint64 CrasAudioHandler::GetPrimaryActiveOutputNode() const {
   return active_output_node_id_;
 }
 
-uint64 CrasAudioHandler::GetActiveInputNode() const {
+uint64 CrasAudioHandler::GetPrimaryActiveInputNode() const {
   return active_input_node_id_;
 }
 
@@ -185,7 +185,7 @@ void CrasAudioHandler::GetAudioDevices(AudioDeviceList* device_list) const {
     device_list->push_back(it->second);
 }
 
-bool CrasAudioHandler::GetActiveOutputDevice(AudioDevice* device) const {
+bool CrasAudioHandler::GetPrimaryActiveOutputDevice(AudioDevice* device) const {
   const AudioDevice* active_device = GetDeviceFromId(active_output_node_id_);
   if (!active_device || !device)
     return false;
@@ -197,12 +197,62 @@ void CrasAudioHandler::SetKeyboardMicActive(bool active) {
   const AudioDevice* keyboard_mic = GetKeyboardMic();
   if (!keyboard_mic)
     return;
-  if (active) {
-    chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->
-        AddActiveInputNode(keyboard_mic->id);
-  } else {
-    chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->
-        RemoveActiveInputNode(keyboard_mic->id);
+  // Keyboard mic is invisible to chromeos users. It is always added or removed
+  // as additional active node.
+  DCHECK(active_input_node_id_ && active_input_node_id_ != keyboard_mic->id);
+  if (active)
+    AddActiveNode(keyboard_mic->id);
+  else
+    RemoveActiveNode(keyboard_mic->id);
+}
+
+void CrasAudioHandler::AddActiveNode(uint64 node_id) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device) {
+    VLOG(1) << "AddActiveInputNode: Cannot find device id="
+            << "0x" << std::hex << node_id;
+    return;
+  }
+
+  // If there is no primary active device, set |node_id| to primary active node.
+  if ((device->is_input && !active_input_node_id_) ||
+      (!device->is_input && !active_output_node_id_)) {
+    SwitchToDevice(*device);
+    return;
+  }
+  AddAdditionalActiveNode(node_id);
+}
+
+void CrasAudioHandler::RemoveActiveNode(uint64 node_id) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device) {
+    VLOG(1) << "RemoveActiveInputNode: Cannot find device id="
+            << "0x" << std::hex << node_id;
+    return;
+  }
+
+  // We only allow to remove the primary active node if it is
+  // the only active node.

[rkc, 2014/09/24 01:22:43]

Why?

[jennyz, 2014/09/24 18:01:20]

CrasAudioHandler and Cras need to consistently maintain the primary active node
at both sides, which is set by SetActive* API. we do assume there will be a
primary active node at all times if there are any active nodes. If we remove the
primary active node while still keeping other active nodes, then
CrasAudioHandler has no way to predicate which active node will be selected by
Cras as the new primary active node. It will mess up the state of
CrasAudioHandler. From the use case of cras extension, it  will be much neat
just to remove all active nodes, then add back a new set of active nodes, like
what we did in SetActiveDevices. Until we have a perfect solution to deprecated
the primary active node concept, this might be easier way to keep the state
clean.Based on hychao's suggestion, we just do not allow remove the primary
active node, this is even simpler.

[rkc, 2014/09/24 19:45:29]

I agree - we should not allow this particular method to remove the primary
active node at all.
Though it should be possible to remove all nodes via the audio API - which
looking at the code will still function fine since the audio API doesn't
actually remove nodes individually but calls RemoveAllActiveNodes instead.

On 2014/09/24 18:01:20, jennyz wrote:

+  if (device->is_input && device->id == active_input_node_id_ &&
+      HasAdditionalActiveInputNode()) {

[hychao, 2014/09/24 04:25:47]

would this be simpler if we just don't allow the primary active node be removed?

[jennyz, 2014/09/24 18:01:20]

Yes, that's even better. 

+    VLOG(1) << "Cannot remove the primary active input node: "
+            << "0x" << std::hex << node_id;
+    return;
+  }
+  if (!device->is_input && device->id == active_output_node_id_ &&
+      HasAdditionalActiveOutputNode()) {
+    VLOG(1) << "Cannot remove the primary active output node: "
+            << "0x" << std::hex << node_id;
+    return;
+  }
+  RemoveActiveNodeInternal(node_id);
+}
+
+void CrasAudioHandler::RemoveAllActiveNodes() {
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       ++it) {
+    RemoveActiveNodeInternal(it->second.id);
   }
 }
 
@@ -215,29 +265,26 @@ bool CrasAudioHandler::has_alternative_output() const {
 }
 
 void CrasAudioHandler::SetOutputVolumePercent(int volume_percent) {
-  volume_percent = min(max(volume_percent, 0), 100);
-  if (volume_percent <= kMuteThresholdPercent)
-    volume_percent = 0;
-  output_volume_ = volume_percent;
-
-  if (const AudioDevice* device = GetDeviceFromId(active_output_node_id_))
-    audio_pref_handler_->SetVolumeGainValue(*device, output_volume_);
-
-  SetOutputNodeVolume(active_output_node_id_, output_volume_);
-  FOR_EACH_OBSERVER(AudioObserver, observers_, OnOutputVolumeChanged());
+  // Set all active devices to the same volume.
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       it++) {
+    const AudioDevice& device = it->second;
+    if (!device.is_input && device.active)
+      SetOutputNodeVolumePercent(device.id, volume_percent);
+  }
 }
 
 // TODO: Rename the 'Percent' to something more meaningful.
 void CrasAudioHandler::SetInputGainPercent(int gain_percent) {
-  // NOTE: We do not sanitize input gain values since the range is completely
-  // dependent on the device.
-  input_gain_ = gain_percent;
-
-  if (const AudioDevice* device = GetDeviceFromId(active_input_node_id_))
-    audio_pref_handler_->SetVolumeGainValue(*device, input_gain_);
-
-  SetInputNodeGain(active_input_node_id_, input_gain_);
-  FOR_EACH_OBSERVER(AudioObserver, observers_, OnInputGainChanged());
+  // TODO(jennyz): Should we set all input devices' gain to the same level?
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       it++) {
+    const AudioDevice& device = it->second;
+    if (device.is_input && device.active)
+      SetInputNodeGainPercent(active_input_node_id_, gain_percent);
+  }
 }
 
 void CrasAudioHandler::AdjustOutputVolumeByPercent(int adjust_by_percent) {
@@ -248,9 +295,14 @@ void CrasAudioHandler::SetOutputMute(bool mute_on) {
   if (!SetOutputMuteInternal(mute_on))
     return;
 
-  if (const AudioDevice* device = GetDeviceFromId(active_output_node_id_)) {
-    DCHECK(!device->is_input);
-    audio_pref_handler_->SetMuteValue(*device, output_mute_on_);
+  // Save the mute state for all active output audio devices.
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       it++) {
+    const AudioDevice& device = it->second;
+    if (!device.is_input && device.active) {
+      audio_pref_handler_->SetMuteValue(device, output_mute_on_);
+    }
   }
 
   FOR_EACH_OBSERVER(AudioObserver, observers_, OnOutputMuteChanged());
@@ -285,22 +337,14 @@ void CrasAudioHandler::SetActiveInputNode(uint64 node_id) {
 
 void CrasAudioHandler::SetVolumeGainPercentForDevice(uint64 device_id,
                                                      int value) {
-  if (device_id == active_output_node_id_) {
-    SetOutputVolumePercent(value);
-    return;
-  } else if (device_id == active_input_node_id_) {
-    SetInputGainPercent(value);
+  const AudioDevice* device = GetDeviceFromId(device_id);
+  if (!device)
     return;
-  }
 
-  if (const AudioDevice* device = GetDeviceFromId(device_id)) {
-    if (!device->is_input) {
-      value = min(max(value, 0), 100);
-      if (value <= kMuteThresholdPercent)
-        value = 0;
-    }
-    audio_pref_handler_->SetVolumeGainValue(*device, value);
-  }
+  if (device->is_input)
+    SetInputNodeGainPercent(device_id, value);
+  else
+    SetOutputNodeVolumePercent(device_id, value);
 }
 
 void CrasAudioHandler::SetMuteForDevice(uint64 device_id, bool mute_on) {
@@ -379,7 +423,7 @@ void CrasAudioHandler::AudioClientRestarted() {
 
 void CrasAudioHandler::NodesChanged() {
   // Refresh audio nodes data.
-  GetNodes();
+  GetNodes(true);  // NodesChanged signal received.
 }
 
 void CrasAudioHandler::ActiveOutputNodeChanged(uint64 node_id) {
@@ -470,9 +514,30 @@ void CrasAudioHandler::SetupAudioOutputState() {
   SetOutputNodeVolume(active_output_node_id_, output_volume_);
 }
 
+// This sets up the state of an additional active node.
+void CrasAudioHandler::SetupAdditionalActiveAudioNodeState(uint64 node_id) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device) {
+    VLOG(1) << "Can't set up audio state for unknown device id ="
+            << "0x" << std::hex << node_id;
+    return;
+  }
+
+  DCHECK(node_id != active_output_node_id_ && node_id != active_input_node_id_);
+
+  // Note: The mute state is a system wide state, we don't set mute per device,
+  // but just keep the mute state consistent for the active node in prefs.
+  // The output volume should be set to the same value for all active output
+  // devices. For input devices, we don't restore their gain value so far.

[rkc, 2014/09/24 01:22:43]

I don't recall the reason exactly, but I believe the reason we weren't
storing/restoring input gain was as a hack to fix something, correct? Did we
ever decide if this was the correct behavior we wanted long term?

[jennyz, 2014/09/24 18:01:20]

I also remembered this might be a hack to fix some input device problem, but I
don't remember exactly what the problem is. One thing I could think about is we
don't have UI for user to adjust the input capture gain, if the input capture
gain was set to a very low number, user could run into problem with input
device. For the same reason, we don't persist the mute setting for input device
as well. I would rather keep this as IS for this cl, majorly not introduce
another issue for this cl. We can fix this problem in a different cl if we want
to.

[rkc, 2014/09/24 19:45:30]

That sounds fine. Please do create a bug to track the issue and link it here
though.

On 2014/09/24 18:01:20, jennyz wrote:

[jennyz, 2014/09/24 20:47:18]

Done.

+  if (!device->is_input) {
+    audio_pref_handler_->SetMuteValue(*device, IsOutputMuted());
+    SetOutputNodeVolumePercent(node_id, GetOutputVolumePercent());
+  }
+}
+
 void CrasAudioHandler::InitializeAudioState() {
   ApplyAudioPolicy();
-  GetNodes();
+  GetNodes(true);
 }
 
 void CrasAudioHandler::ApplyAudioPolicy() {
@@ -506,6 +571,26 @@ void CrasAudioHandler::SetOutputNodeVolume(uint64 node_id, int volume) {
       SetOutputNodeVolume(node_id, volume);
 }
 
+void CrasAudioHandler::SetOutputNodeVolumePercent(uint64 node_id,
+                                                  int volume_percent) {
+  const AudioDevice* device = this->GetDeviceFromId(node_id);
+  if (!device || device->is_input)
+    return;
+
+  volume_percent = min(max(volume_percent, 0), 100);
+  if (volume_percent <= kMuteThresholdPercent)
+    volume_percent = 0;
+  if (node_id == active_output_node_id_)
+    output_volume_ = volume_percent;
+
+  audio_pref_handler_->SetVolumeGainValue(*device, volume_percent);
+
+  if (device->active) {
+    SetOutputNodeVolume(node_id, volume_percent);
+    FOR_EACH_OBSERVER(AudioObserver, observers_, OnOutputVolumeChanged());
+  }
+}
+
 bool  CrasAudioHandler::SetOutputMuteInternal(bool mute_on) {
   if (output_mute_locked_)
     return false;
@@ -521,24 +606,53 @@ void CrasAudioHandler::SetInputNodeGain(uint64 node_id, int gain) {
       SetInputNodeGain(node_id, gain);
 }
 
+void CrasAudioHandler::SetInputNodeGainPercent(uint64 node_id,
+                                               int gain_percent) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device || !device->is_input)
+    return;
+
+  // NOTE: We do not sanitize input gain values since the range is completely
+  // dependent on the device.
+  if (active_input_node_id_ == node_id)
+    input_gain_ = gain_percent;
+
+  audio_pref_handler_->SetVolumeGainValue(*device, gain_percent);
+
+  if (device->active) {
+    SetInputNodeGain(node_id, gain_percent);
+    FOR_EACH_OBSERVER(AudioObserver, observers_, OnInputGainChanged());
+  }
+}
+
 bool CrasAudioHandler::SetInputMuteInternal(bool mute_on) {
   if (input_mute_locked_)
     return false;
 
-  VLOG(1) << "SetInputMuteInternal sets active input device id="
-          << "0x" << std::hex << active_input_node_id_ << " mute=" << mute_on;
   input_mute_on_ = mute_on;
   chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->
       SetInputMute(mute_on);
   return true;
 }
 
-void CrasAudioHandler::GetNodes() {
-  chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->GetNodes(
-      base::Bind(&CrasAudioHandler::HandleGetNodes,
-                 weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&CrasAudioHandler::HandleGetNodesError,
-                 weak_ptr_factory_.GetWeakPtr()));
+void CrasAudioHandler::GetNodes(bool nodes_changed) {
+  if (nodes_changed) {
+    chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->GetNodes(
+        base::Bind(&CrasAudioHandler::HandleGetNodes,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&CrasAudioHandler::HandleGetNodesError,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    // If GetNodes is not triggered by NodesChanged signal,

[rkc, 2014/09/24 01:22:43]

This code seems unused. Everywhere that GetNodes is being called from is with
parameter set to true. If it isn't being used, we should check it in once it is
being used.

[jennyz, 2014/09/24 18:01:20]

Yes, this is not used in the current code, it could be useful for diagnostic
purpose if we just want to check the nodes data from Cras. I could remove it if
you really don't like it.

[rkc, 2014/09/24 19:45:29]

I am really uncomfortable checking in unused code. I'm a big fan of YAGNI :)

On 2014/09/24 18:01:20, jennyz wrote:

[jennyz, 2014/09/24 20:47:18]

Done.

+    // HandleGetNodesWithoutNodesChange will just retrieve all the
+    // nodes data from cras without switching active device.
+    // This could be used for diagnostic purpose.
+    chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->GetNodes(
+        base::Bind(&CrasAudioHandler::HandleGetNodesWithoutNodesChange,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&CrasAudioHandler::HandleGetNodesError,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 bool CrasAudioHandler::ChangeActiveDevice(const AudioDevice& new_active_device,
@@ -604,14 +718,14 @@ bool CrasAudioHandler::HasDeviceChange(const AudioNodeList& new_nodes,
       ++num_new_devices;
       // Look to see if the new device not in the old device list.
       AudioDevice device(*it);
-      if (FoundNewDevice(device))
+      if (FoundNewOrChangedDevice(device))
         return true;
     }
   }
   return num_old_devices != num_new_devices;
 }
 
-bool CrasAudioHandler::FoundNewDevice(const AudioDevice& device) {
+bool CrasAudioHandler::FoundNewOrChangedDevice(const AudioDevice& device) {
   const AudioDevice* device_found = GetDeviceFromId(device.id);
   if (!device_found)
     return true;
@@ -621,51 +735,35 @@ bool CrasAudioHandler::FoundNewDevice(const AudioDevice& device) {
         << " new device: " << device.ToString()
         << " old device: " << device_found->ToString();
     return true;
+  } else if (device.active != device_found->active) {
+    return true;
   }
+
   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;
+bool CrasAudioHandler::HasAdditionalActiveOutputNode() const {
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       ++it) {
+    if (active_output_node_id_ && !it->second.is_input && it->second.active &&
+        it->second.id != active_output_node_id_) {
+      return 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 false;
+}
+
+bool CrasAudioHandler::HasAdditionalActiveInputNode() const {
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end();
+       ++it) {
+    if (active_input_node_id_ && it->second.is_input && it->second.active &&
+        it->second.id != active_input_node_id_) {
+      return true;
     }
   }
-  return device;
+  return false;
 }
 
 void CrasAudioHandler::UpdateDevicesAndSwitchActive(
@@ -683,7 +781,7 @@ void CrasAudioHandler::UpdateDevicesAndSwitchActive(
     output_devices_pq_.pop();
 
   for (size_t i = 0; i < nodes.size(); ++i) {
-    AudioDevice device = GetSanitizedAudioDevice(nodes[i]);
+    AudioDevice device(nodes[i]);
     audio_devices_[device.id] = device;
 
     if (!has_alternative_input_ &&
@@ -732,9 +830,77 @@ void CrasAudioHandler::HandleGetNodes(const chromeos::AudioNodeList& node_list,
   FOR_EACH_OBSERVER(AudioObserver, observers_, OnAudioNodesChanged());
 }
 
+void CrasAudioHandler::HandleGetNodesWithoutNodesChange(
+    const chromeos::AudioNodeList& node_list,
+    bool success) {
+  if (!success) {
+    LOG_IF(ERROR, log_errors_) << "Failed to retrieve audio nodes data";
+    return;
+  }
+
+  // Just refresh the audio_devices_ with the latest audio_list from cras.
+  for (size_t i = 0; i < node_list.size(); ++i) {
+    AudioDevice device(node_list[i]);
+    audio_devices_[device.id] = device;
+  }
+}
+
 void CrasAudioHandler::HandleGetNodesError(const std::string& error_name,
                                            const std::string& error_msg) {
   LOG_IF(ERROR, log_errors_) << "Failed to call GetNodes: "
       << error_name  << ": " << error_msg;
 }
+
+void CrasAudioHandler::AddAdditionalActiveNode(uint64 node_id) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device) {
+    VLOG(1) << "AddActiveInputNode: Cannot find device id="
+            << "0x" << std::hex << node_id;
+    return;
+  }
+
+  audio_devices_[node_id].active = true;
+  SetupAdditionalActiveAudioNodeState(node_id);
+
+  if (device->is_input) {
+    DCHECK(node_id != active_input_node_id_);
+    chromeos::DBusThreadManager::Get()
+        ->GetCrasAudioClient()
+        ->AddActiveInputNode(node_id);
+    FOR_EACH_OBSERVER(AudioObserver, observers_, OnActiveInputNodeChanged());
+  } else {
+    DCHECK(node_id != active_output_node_id_);
+    chromeos::DBusThreadManager::Get()
+        ->GetCrasAudioClient()
+        ->AddActiveOutputNode(node_id);
+    FOR_EACH_OBSERVER(AudioObserver, observers_, OnActiveOutputNodeChanged());
+  }
+}
+
+void CrasAudioHandler::RemoveActiveNodeInternal(uint64 node_id) {
+  const AudioDevice* device = GetDeviceFromId(node_id);
+  if (!device) {
+    VLOG(1) << "RemoveActiveInputNode: Cannot find device id="
+            << "0x" << std::hex << node_id;
+    return;
+  }
+
+  audio_devices_[node_id].active = false;
+  if (device->is_input) {
+    if (node_id == active_input_node_id_)
+      active_input_node_id_ = 0;
+    chromeos::DBusThreadManager::Get()
+        ->GetCrasAudioClient()
+        ->RemoveActiveInputNode(node_id);
+    FOR_EACH_OBSERVER(AudioObserver, observers_, OnActiveInputNodeChanged());
+  } else {
+    if (node_id == active_output_node_id_)
+      active_output_node_id_ = 0;
+    chromeos::DBusThreadManager::Get()
+        ->GetCrasAudioClient()
+        ->RemoveActiveOutputNode(node_id);
+  }
+  FOR_EACH_OBSERVER(AudioObserver, observers_, OnActiveOutputNodeChanged());
+}
+
 }  // namespace chromeos