Store audio device's active state in preference

chromeos/audio/cras_audio_handler.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/audio/cras_audio_handler.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/bind.h"
@@ skipping 25 matching lines @@
 bool IsSameAudioDevice(const AudioDevice& a, const AudioDevice& b) {
   return a.id == b.id && a.is_input == b.is_input && a.type == b.type
       && a.device_name == b.device_name;
 }
 
 bool IsInNodeList(uint64_t node_id,
                   const CrasAudioHandler::NodeIdList& id_list) {
   return std::find(id_list.begin(), id_list.end(), node_id) != id_list.end();
 }
 
-bool IsNodeInTheList(uint64_t node_id, const AudioNodeList& node_list) {
-  for (size_t i = 0; i < node_list.size(); ++i) {
-    if (node_id == node_list[i].id)
-      return true;
-  }
-  return false;
-}
-
 }  // namespace
 
 CrasAudioHandler::AudioObserver::AudioObserver() {
 }
 
 CrasAudioHandler::AudioObserver::~AudioObserver() {
 }
 
 void CrasAudioHandler::AudioObserver::OnOutputNodeVolumeChanged(
     uint64_t /* node_id */,
@@ skipping 174 matching lines @@
   // 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, notify);
     return;
   }
 
   AddAdditionalActiveNode(node_id, notify);
 }
 
 void CrasAudioHandler::ChangeActiveNodes(const NodeIdList& new_active_ids) {

[jennyz, 2015/12/17 22:50:21]

What about multiple active audio nodes case?

[hychao, 2015/12/29 10:57:25]

actually I forgot about the multiple active node path when I worked on this
commit. By current implementation, only the primary (1st added) active node will
have its state stored into preference. And potentially affect which node to
switch to, at the next time nodes changed.

Other options for multiple active nodes are:
(1) store device state for every node
(2) exclude multiple active node path from storing device state

For me it's not trivial to tell which option makes the most sense. It's a
complicated scenario when using both extension API and UI to change active node
at the same time, so it's hard to say how user would expect the behavior to be.
Plus that after stable_device_id is exposed through extension API, Hotrod app is
going to maintain its own device selection preference. So I'd prefer not adding
more code to implement (1) or (2).

[jennyz, 2016/01/05 23:46:55]

Yes, it makes sense to exclude the new active node preference logic from this
code path, since this is only used by hotrod, who maintains its own selection
preference. Please add a comment for the exclusion reason.

[hychao, 2016/01/06 06:34:12]

Done.

   // Flags for whether there are input or output nodes passed in from
   // |new_active_ids|. If there are no input nodes passed in, we will not
   // make any change for input nodes; same for the output nodes.
   bool request_input_change = false;
   bool request_output_change = false;
 
   // Flags for whether we will actually change active status of input
   // or output nodes.
   bool make_input_change = false;
   bool make_output_change = false;
@@ skipping 129 matching lines @@
   SetInputMuteInternal(mute_on);
   FOR_EACH_OBSERVER(AudioObserver, observers_,
                     OnInputMuteChanged(input_mute_on_));
 }
 
 void CrasAudioHandler::SetActiveOutputNode(uint64_t node_id, bool notify) {
   chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->
       SetActiveOutputNode(node_id);
   if (notify)
     NotifyActiveNodeChanged(false);
+
+  // Save state for all output nodes.
+  for (AudioDeviceMap::iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (it->second.is_input)
+      continue;
+    audio_pref_handler_->SetDeviceState(it->second,
+        it->second.active ? AUDIO_STATE_ACTIVE : AUDIO_STATE_INACTIVE);
+  }
 }
 
 void CrasAudioHandler::SetActiveInputNode(uint64_t node_id, bool notify) {
   chromeos::DBusThreadManager::Get()->GetCrasAudioClient()->
       SetActiveInputNode(node_id);
   if (notify)
     NotifyActiveNodeChanged(true);
+
+  // Save state for all input nodes.
+  for (AudioDeviceMap::iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (!it->second.is_input)
+      continue;
+    audio_pref_handler_->SetDeviceState(it->second,
+        it->second.active ? AUDIO_STATE_ACTIVE : AUDIO_STATE_INACTIVE);
+  }
 }
 
 void CrasAudioHandler::SetVolumeGainPercentForDevice(uint64_t device_id,
                                                      int value) {
   const AudioDevice* device = GetDeviceFromId(device_id);
   if (!device)
     return;
 
   if (device->is_input)
     SetInputNodeGainPercent(device_id, value);
@@ skipping 358 matching lines @@
   } else {
     if (!ChangeActiveDevice(device, &active_output_node_id_))
       return;
     SetupAudioOutputState();
     SetActiveOutputNode(active_output_node_id_, notify);
   }
 }
 
 bool CrasAudioHandler::HasDeviceChange(const AudioNodeList& new_nodes,
                                        bool is_input,
-                                       AudioNodeList* new_discovered) {
+                                       AudioDevicePriorityQueue* new_discovered) {
   size_t num_old_devices = 0;
   size_t num_new_devices = 0;
   for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
        it != audio_devices_.end(); ++it) {
     if (is_input == it->second.is_input)
       ++num_old_devices;
   }
 
   bool new_or_changed_device = false;
-  new_discovered->clear();
+  while (!new_discovered->empty())
+    new_discovered->pop();
   for (AudioNodeList::const_iterator it = new_nodes.begin();
        it != new_nodes.end(); ++it) {
     if (is_input == it->is_input) {
       ++num_new_devices;
       // Look to see if the new device not in the old device list.
       AudioDevice device(*it);
       DeviceStatus status = CheckDeviceStatus(device);
       if (status == NEW_DEVICE)
-        new_discovered->push_back(*it);
+        new_discovered->push(device);
       if (status == NEW_DEVICE || status == CHANGED_DEVICE) {
         new_or_changed_device = true;
       }
     }
   }
   return new_or_changed_device || (num_old_devices != num_new_devices);
 }
 
 CrasAudioHandler::DeviceStatus CrasAudioHandler::CheckDeviceStatus(
     const AudioDevice& device) {
@@ skipping 25 matching lines @@
   size_t old_output_device_size = 0;
   size_t old_input_device_size = 0;
   for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
        it != audio_devices_.end(); ++it) {
     if (it->second.is_input)
       ++old_input_device_size;
     else
       ++old_output_device_size;
   }
 
-  AudioNodeList hotplug_output_nodes;
-  AudioNodeList hotplug_input_nodes;
+  AudioDevicePriorityQueue hotplug_output_nodes;
+  AudioDevicePriorityQueue hotplug_input_nodes;
   bool output_devices_changed =
       HasDeviceChange(nodes, false, &hotplug_output_nodes);
   bool input_devices_changed =
       HasDeviceChange(nodes, true, &hotplug_input_nodes);
   audio_devices_.clear();
   has_alternative_input_ = false;
   has_alternative_output_ = false;
 
   while (!input_devices_pq_.empty())
     input_devices_pq_.pop();
@@ skipping 31 matching lines @@
   // See crbug.com/478968.
   const AudioDevice* active_output = GetDeviceFromId(active_output_node_id_);
   if (!active_output || !active_output->active)
     active_output_node_id_ = 0;
   const AudioDevice* active_input = GetDeviceFromId(active_input_node_id_);
   if (!active_input || !active_input->active)
     active_input_node_id_ = 0;
 
   // If audio nodes change is caused by unplugging some non-active audio
   // devices, the previously set active audio device will stay active.
   // Otherwise, switch to a new active audio device according to their priority.

[jennyz, 2015/12/17 22:50:21]

The comments for how the active audio device is selected is kind of messy after
so many changes, mostly my fault. We should clean it up now. For this one, maybe
better be:
If audio nodes change is caused by unplugging some non-active audio devices, we 
wont't change the current active audio devices.

[hychao, 2015/12/29 10:57:25]

Done.

   if (input_devices_changed &&
       !NonActiveDeviceUnplugged(old_input_device_size,
                                 new_input_device_size,
                                 active_input_node_id_)) {
     // Some devices like chromeboxes don't have the internal audio input. In
     // that case the active input node id should be reset.
     if (input_devices_pq_.empty()) {
       active_input_node_id_ = 0;
       NotifyActiveNodeChanged(true);
     } else {
       // If user has hot plugged a new node, we should change to the active
-      // device to the new node if it has the highest priority; otherwise,
-      // we should keep the existing active node chosen by user.
+      // device to the new node if (1) it has the highest priority; or (2)
+      // the last state before it hot plugged was active; otherwise, we should
+      // keep the existing active node chosen by user.
       // For all other cases, we will choose the node with highest priority.

[jennyz, 2015/12/17 22:50:21]

How about change this as:
If there is no current active node, or, no new hot plugged node, select the
active node by their priorities.

[hychao, 2015/12/29 10:57:25]

Done.

-      if (!active_input_node_id_ || hotplug_input_nodes.empty() ||
-          IsNodeInTheList(input_devices_pq_.top().id, hotplug_input_nodes)) {
+      if (!active_input_node_id_ || hotplug_input_nodes.empty()) {
         SwitchToDevice(input_devices_pq_.top(), true);
+      } else {

[jennyz, 2015/12/17 22:50:21]

How about comment like this:
If user has hot plugged any input nodes, look at the one with highest
priorty(normally, there is only one hot plugged input node), 1) If it was a new
device with higher priority, switch the active to this device. 2) If it was
device plugged in before, and was NOT selected as the active one last time
before it was unplugged, then don't change the current active device. 3) If it
was plugged in before and was selected as the active one before it was unplugged
last time, switch the active to this device.

[hychao, 2015/12/29 10:57:25]

Thanks for the suggestion. I broke down and moved the comments of these device
states to each the switch case below, to make it more readable.

+        AudioDeviceState last_state =
+            audio_pref_handler_->GetDeviceState(hotplug_input_nodes.top());
+        switch (last_state) {
+        case AUDIO_STATE_ACTIVE:
+          SwitchToDevice(hotplug_input_nodes.top(), true);
+          break;
+        case AUDIO_STATE_NOT_AVAILABLE:

[jennyz, 2015/12/17 22:50:21]

This is the case if the device is new, not plugged in before, right? If true,
please add a comment.

[hychao, 2015/12/29 10:57:25]

Done.

+          if (input_devices_pq_.top().id == hotplug_input_nodes.top().id)
+            SwitchToDevice(hotplug_input_nodes.top(), true);
+          break;
+        case AUDIO_STATE_INACTIVE:

[jennyz, 2015/12/17 22:50:21]

No need to add this line, it should fall into default case. If you would like to
add this line for drawing attention for this case, please add a comment.

[hychao, 2015/12/29 10:57:25]

Thanks for reminding. I think its useful to keep this line, so I can put the
comment right next to AUDIO_STATE_INACTIVE case.

+        default:
+          break;
+        }
       }
     }
   }
   if (output_devices_changed &&
       !NonActiveDeviceUnplugged(old_output_device_size,
                                 new_output_device_size,
                                 active_output_node_id_)) {
     // This is really unlikely to happen because all ChromeOS devices have the
     // internal audio output.

[jennyz, 2015/12/17 22:50:21]

gitto input nodes logic.

[hychao, 2015/12/29 10:57:25]

Done.

     if (output_devices_pq_.empty()) {
       active_output_node_id_ = 0;
       NotifyActiveNodeChanged(false);
     } else {
       // ditto input node case.
-      if (!active_output_node_id_ || hotplug_output_nodes.empty() ||
-          IsNodeInTheList(output_devices_pq_.top().id, hotplug_output_nodes)) {
+      if (!active_output_node_id_ || hotplug_output_nodes.empty()) {
         SwitchToDevice(output_devices_pq_.top(), true);
+      } else {
+        AudioDeviceState last_state =
+            audio_pref_handler_->GetDeviceState(hotplug_output_nodes.top());
+        switch (last_state) {
+        case AUDIO_STATE_ACTIVE:
+          SwitchToDevice(hotplug_output_nodes.top(), true);
+          break;
+        case AUDIO_STATE_NOT_AVAILABLE:
+          if (output_devices_pq_.top().id == hotplug_output_nodes.top().id)
+            SwitchToDevice(hotplug_output_nodes.top(), true);
+          break;
+        case AUDIO_STATE_INACTIVE:

[jennyz, 2015/12/17 22:50:21]

ditto

[hychao, 2015/12/29 10:57:25]

Although the comments are not copied over, I keep this line for symmetry to the
input logic.

+        default:
+          break;
+        }
       }
     }
   }
 }
 
 void CrasAudioHandler::HandleGetNodes(const chromeos::AudioNodeList& node_list,
                                       bool success) {
   if (!success) {
     LOG_IF(ERROR, log_errors_) << "Failed to retrieve audio nodes data";
     return;
@@ skipping 94 matching lines @@
                      hdmi_rediscover_grace_period_duration_in_ms_),
       this, &CrasAudioHandler::UpdateAudioAfterHDMIRediscoverGracePeriod);
 }
 
 void CrasAudioHandler::SetHDMIRediscoverGracePeriodForTesting(
     int duration_in_ms) {
   hdmi_rediscover_grace_period_duration_in_ms_ = duration_in_ms;
 }
 
 }  // namespace chromeos