Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activa…

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 444eab643cbb4ab81cc8aef0975e9c0b7199c49c..2fd99768ccf5a96de4c9afc29253f4f180319850 100644
--- a/chromeos/audio/cras_audio_handler.cc
+++ b/chromeos/audio/cras_audio_handler.cc
@@ -41,7 +41,7 @@ const std::vector<double> kStereoToMono = {0.5, 0.5, 0.5, 0.5};
 // Mixer matrix, [1, 0; 0, 1]
 const std::vector<double> kStereoToStereo = {1, 0, 0, 1};
 
-static CrasAudioHandler* g_cras_audio_handler = NULL;
+static CrasAudioHandler* g_cras_audio_handler = nullptr;
 
 bool IsSameAudioDevice(const AudioDevice& a, const AudioDevice& b) {
   return a.stable_device_id == b.stable_device_id && a.is_input == b.is_input &&
@@ -111,12 +111,12 @@ void CrasAudioHandler::InitializeForTesting() {
 void CrasAudioHandler::Shutdown() {
   CHECK(g_cras_audio_handler);
   delete g_cras_audio_handler;
-  g_cras_audio_handler = NULL;
+  g_cras_audio_handler = nullptr;
 }
 
 // static
 bool CrasAudioHandler::IsInitialized() {
-  return g_cras_audio_handler != NULL;
+  return g_cras_audio_handler != nullptr;
 }
 
 // static
@@ -127,11 +127,54 @@ CrasAudioHandler* CrasAudioHandler::Get() {
 }
 
 void CrasAudioHandler::OnVideoCaptureStarted(media::VideoFacingMode facing) {
-  // TODO(jennyz): Switch active audio device according to video facing.
+  // Do nothing if the device doesn't have both front and rear microphones.
+  if (!HasDualInternalMic())
+    return;
+  switch (facing) {
+    case media::MEDIA_VIDEO_FACING_USER:
+      front_camera_on_ = true;
+      break;
+    case media::MEDIA_VIDEO_FACING_ENVIRONMENT:
+      rear_camera_on_ = true;
+      break;
+    default:
+      NOTREACHED();

[shenghao, 2017/03/01 11:25:55]

I am not 100% sure but I think it's possible to reach here. External camera
should have facing=MEDIA_VIDEO_FACING_NONE.

[jennyz, 2017/03/02 17:31:40]

I need to know in which scenario it will pass MEDIA_VIDEO_FACING_NONE in under
dual internal mic mode, and how we are going to handle it? For now, I would just
log a warning and do nothing just in case it happens.

[shenghao, 2017/03/03 03:45:21]

Since external camera has MEDIA_VIDEO_FACING_NONE, I don't think we need to log
warning for it. Only log warning for NUM_MEDIA_VIDEO_FACING_MODES case.

[jennyz, 2017/03/03 19:35:15]

Done.

+  }
+
+  // If the current active input is an external device, keep it.
+  const AudioDevice* active_input = GetDeviceFromId(active_input_node_id_);
+  if (active_input && active_input->IsExternalDevice())
+    return;
+
+  // Activate the correct mic for the current active camera.
+  ActivateMicForCamera(facing);
 }
 
 void CrasAudioHandler::OnVideoCaptureStopped(media::VideoFacingMode facing) {
-  // TODO(jennyz): Switch active audio device according to video facing.
+  // Do nothing if the device doesn't have both front and rear microphones.
+  if (!HasDualInternalMic())
+    return;
+
+  switch (facing) {
+    case media::MEDIA_VIDEO_FACING_USER:
+      front_camera_on_ = false;
+      break;
+    case media::MEDIA_VIDEO_FACING_ENVIRONMENT:
+      rear_camera_on_ = false;
+      break;
+    default:
+      NOTREACHED();
+  }
+
+  // If the current active input is an external device, keep it.
+  const AudioDevice* active_input = GetDeviceFromId(active_input_node_id_);
+  if (active_input && active_input->IsExternalDevice())
+    return;
+
+  // Switch to front mic properly.

[shenghao, 2017/03/01 11:25:55]

Why is it "front mic"? Do you mean "correct mic"?

[jennyz, 2017/03/02 17:31:41]

In R58, chrome code will only handle the dual camera/dual mic case with only one
camera active at a time, which is the typical chrome book and camera app case.
For the developers, they can use audio extension api to set both mic to be
active if they need to do so for their app.

For our use case I mentioned above, the camera stop, typically, user close the
camera app, we would switch back to internal mic and the front mic is the
default mic to set active. 

+  DeviceActivateType activate_by =

[shenghao, 2017/03/01 11:25:55]

s/activate_by/activated_by/

[jennyz, 2017/03/02 17:31:41]

Done.

+      HasExternalDevice(true) ? ACTIVATE_BY_USER : ACTIVATE_BY_PRIORITY;
+  SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by);

[shenghao, 2017/03/01 11:25:55]

Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to
AUDIO_TYPE_BACK_MIC?

[shenghao, 2017/03/01 11:27:54]

typo: also => always

[jennyz, 2017/03/02 17:31:40]

Acknowledged.

[jennyz, 2017/03/02 17:31:41]

See comments above.

 }
 
 void CrasAudioHandler::AddAudioObserver(AudioObserver* observer) {
@@ -143,7 +186,7 @@ void CrasAudioHandler::RemoveAudioObserver(AudioObserver* observer) {
 }
 
 bool CrasAudioHandler::HasKeyboardMic() {
-  return GetKeyboardMic() != NULL;
+  return GetKeyboardMic() != nullptr;
 }
 
 bool CrasAudioHandler::IsOutputMuted() {
@@ -604,7 +647,7 @@ CrasAudioHandler::~CrasAudioHandler() {
       RemoveObserver(this);
   if (audio_pref_handler_.get())
     audio_pref_handler_->RemoveAudioPrefObserver(this);
-  audio_pref_handler_ = NULL;
+  audio_pref_handler_ = nullptr;
 }
 
 void CrasAudioHandler::AudioClientRestarted() {
@@ -712,7 +755,7 @@ void CrasAudioHandler::EmitLoginPromptVisibleCalled() {
 const AudioDevice* CrasAudioHandler::GetDeviceFromId(uint64_t device_id) const {
   AudioDeviceMap::const_iterator it = audio_devices_.find(device_id);
   if (it == audio_devices_.end())
-    return NULL;
+    return nullptr;
 
   return &(it->second);
 }
@@ -724,7 +767,7 @@ const AudioDevice* CrasAudioHandler::GetDeviceFromStableDeviceId(
     if (it->second.stable_device_id == stable_device_id)
       return &(it->second);
   }
-  return NULL;
+  return nullptr;
 }
 
 const AudioDevice* CrasAudioHandler::GetKeyboardMic() const {
@@ -733,7 +776,7 @@ const AudioDevice* CrasAudioHandler::GetKeyboardMic() const {
     if (it->second.is_input && it->second.type == AUDIO_TYPE_KEYBOARD_MIC)
       return &(it->second);
   }
-  return NULL;
+  return nullptr;
 }
 
 void CrasAudioHandler::SetupAudioInputState() {
@@ -1183,8 +1226,20 @@ void CrasAudioHandler::HandleHotPlugDevice(
 void CrasAudioHandler::SwitchToTopPriorityDevice(bool is_input) {
   AudioDevice top_device =
       is_input ? input_devices_pq_.top() : output_devices_pq_.top();
-  if (top_device.is_for_simple_usage())
-    SwitchToDevice(top_device, true, ACTIVATE_BY_PRIORITY);
+  if (!top_device.is_for_simple_usage())
+    return;
+
+  // For the dual camera and dual microphone case, choose microphone
+  // that is consistent to the active camera.
+  if (IsFrontOrRearMic(top_device) && HasDualInternalMic() && IsCameraOn()) {

[shenghao, 2017/03/01 11:25:55]

I think you can write:

if (IsFrontOrRearMic(top_device)) {
  ActivateInternalMicForActiveCamera();
  return;
}

[jennyz, 2017/03/02 17:31:40]

The above code is not equivalent to the original lines. But I did simplify it by
calling ActivateInternalMicForActiveCamera() inside the if block.

+    media::VideoFacingMode facing = front_camera_on_
+                                        ? media::MEDIA_VIDEO_FACING_USER
+                                        : media::MEDIA_VIDEO_FACING_ENVIRONMENT;
+    ActivateMicForCamera(facing);
+    return;
+  }
+
+  SwitchToDevice(top_device, true, ACTIVATE_BY_PRIORITY);
 }
 
 void CrasAudioHandler::SwitchToPreviousActiveDeviceIfAvailable(bool is_input) {
@@ -1423,4 +1478,94 @@ void CrasAudioHandler::SetHDMIRediscoverGracePeriodForTesting(
   hdmi_rediscover_grace_period_duration_in_ms_ = duration_in_ms;
 }
 
+void CrasAudioHandler::ActivateMicForCamera(
+    media::VideoFacingMode camera_facing) {
+  const AudioDevice* mic = GetMicForCamera(camera_facing);
+  if (!mic || mic->active)
+    return;
+
+  SwitchToDevice(*mic, true, ACTIVATE_BY_CAMERA);
+}
+
+void CrasAudioHandler::ActivateInternalMicForActiveCamera() {
+  if (HasDualInternalMic() && IsCameraOn()) {
+    media::VideoFacingMode facing = front_camera_on_
+                                        ? media::MEDIA_VIDEO_FACING_USER
+                                        : media::MEDIA_VIDEO_FACING_ENVIRONMENT;
+    ActivateMicForCamera(facing);
+  }
+}
+
+// For the dual microphone case, from user point of view, they only see internal
+// microphone in UI. Chrome will make the best decision on which one to pick.
+// If the camera is off, the front microphone should be picked as the default
+// active microphone. Otherwise, it will switch to the microphone that
+// matches the active camera, i.e. front microphone for front camera and
+// reaa microphone for rear camera.

[shenghao, 2017/03/01 11:25:55]

s/reaa/rear/

[jennyz, 2017/03/02 17:31:41]

Done.

+void CrasAudioHandler::SwitchToFrontOrRearMic() {
+  DCHECK(HasDualInternalMic());
+  if (IsCameraOn()) {

[shenghao, 2017/03/01 11:25:55]

Why do we need to check IsCameraOn() twice here and line 1491?

[jennyz, 2017/03/02 17:31:40]

Removed IsCameraOn() from 1491.

+    ActivateInternalMicForActiveCamera();
+  } else {
+    SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true,
+                   ACTIVATE_BY_USER);
+  }
+}
+
+const AudioDevice* CrasAudioHandler::GetMicForCamera(
+    media::VideoFacingMode camera_facing) {
+  switch (camera_facing) {
+    case media::MEDIA_VIDEO_FACING_USER:
+      return GetDeviceByType(AUDIO_TYPE_FRONT_MIC);
+    case media::MEDIA_VIDEO_FACING_ENVIRONMENT:
+      return GetDeviceByType(AUDIO_TYPE_REAR_MIC);
+      break;

[shenghao, 2017/03/01 11:25:55]

You don't need "break" here because of the return statement.

[jennyz, 2017/03/02 17:31:40]

Done.

+    default:
+      NOTREACHED();

[shenghao, 2017/03/01 11:25:55]

Same as line 141

[jennyz, 2017/03/02 17:31:41]

This code path is not reachable, the code path to call this function would check
and filter out the case that facing is not the first two cases.

+  }
+  return nullptr;
+}
+
+const AudioDevice* CrasAudioHandler::GetDeviceByType(AudioDeviceType type) {
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();

[shenghao, 2017/03/01 11:25:55]

For all the "AudioDeviceMap::const_iterator", can we use "const auto" to replace
them?

[jennyz, 2017/03/02 17:31:40]

Done.

+       it != audio_devices_.end(); ++it) {
+    if (it->second.type == type)
+      return &(it->second);
+  }
+  return nullptr;
+}
+
+bool CrasAudioHandler::HasDualInternalMic() const {
+  bool has_front_mic = false;
+  bool has_rear_mic = false;
+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (it->second.type == AUDIO_TYPE_FRONT_MIC)
+      has_front_mic = true;
+    else if (it->second.type == AUDIO_TYPE_REAR_MIC)
+      has_rear_mic = true;
+    if (has_front_mic && has_rear_mic)
+      break;
+  }
+  return has_front_mic && has_rear_mic;
+}
+
+bool CrasAudioHandler::IsFrontOrRearMic(const AudioDevice& device) const {
+  return device.is_input && (device.type == AUDIO_TYPE_FRONT_MIC ||
+                             device.type == AUDIO_TYPE_REAR_MIC);
+}
+
+bool CrasAudioHandler::IsCameraOn() const {
+  return front_camera_on_ || rear_camera_on_;
+}
+
+bool CrasAudioHandler::HasExternalDevice(bool is_input) const {

[shenghao, 2017/03/01 11:25:55]

It seems that this method is only called in one place, and |is_input| is true.
Can we remove the parameter?

[jennyz, 2017/03/02 17:31:40]

I would rather keep it just in case it could be used by output device in the
future.

+  for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (is_input == it->second.is_input && it->second.IsExternalDevice())
+      return true;
+  }
+  return false;
+}
+
 }  // namespace chromeos