Pass camera facing info to audio client

content/browser/renderer_host/media/video_capture_manager.cc

Index: content/browser/renderer_host/media/video_capture_manager.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager.cc b/content/browser/renderer_host/media/video_capture_manager.cc
index 24880a5759a4fb77c265057242bd5fb93c51fed1..904d0c1d2ca4c1b5ce97d193106f0c78e9793937 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -56,6 +56,11 @@
 
 #endif  // defined(ENABLE_SCREEN_CAPTURE)
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/cras_audio_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#endif
+
 namespace {
 
 class VideoFrameConsumerFeedbackObserverOnTaskRunner
@@ -653,6 +658,31 @@ VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread(
     return nullptr;
   }
 
+#if defined(OS_CHROMEOS)
+  chromeos::DBusThreadManager* thread_manager =
+      chromeos::DBusThreadManager::Get();

[chfremer, 2017/01/17 18:05:55]

Independent of the question whether or not this is the right place to hook into
for deciding to switch the audio device, I am concerned about the
testability/maintainability of this.

We are adding a hard dependency on a global object here, which makes it very
difficult to test this functionality in isolation from a context that has such a
global object set up correctly. Additionally, we take a dependency on a large
audio-specific interface CrasAudioClient, when all we really use from it is a
single method.

I recommend using dependency injection instead, i.e., pass an interface
VideoCaptureStartObserver into the constructor of VideoCaptureManager. This
interface can expose a single method: 

class VideoCaptureStartObserver {
 public:
  virtual ~VideoCaptureStartObserver() {}
  void OnVideoCaptureStarted(media::VideoFacingMode facing_mode) = 0;
};

I would also prefer that if we add such an observer mechanism, we do it for all
platforms. I think the reduction in complexity for testining/maintaining we get
from eliminating the #ifdefs is worth the minimal extra runtime cost on
platforms that do not currently use it.

[shenghao, 2017/01/18 10:41:12]

Yeah. I totally agree with you on the idea to eliminate dependency on global
objects. But the observer mechanism is not feasible here because we will need to
make CrasAudioClient implements the observer interface; however, CrasAudioClient
can't depend on //content/browser due to cyclic dependency.

Do you have other ideas to pass facing information to CrasAudioClient?

[chfremer, 2017/01/18 18:36:10]

The observer interface does not need to live in //content/browser. It should
live in a place that is okay to depend on for both CrasAudioClient and
VideoCaptureManager.

+  if (thread_manager != NULL) {
+    chromeos::CrasAudioClient* client = thread_manager->GetCrasAudioClient();
+    if (client != NULL) {
+      chromeos::CrasAudioClient::VideoFacingMode facing =
+          chromeos::CrasAudioClient::VideoFacingMode::NONE;
+      switch (descriptor.facing) {

[chfremer, 2017/01/17 18:05:55]

In the current version of the code indexed in code search, I do not see
VideoCaptureDeviceDescriptor expose a field "facing". Is this to be added?

[shenghao, 2017/01/18 10:41:12]

Yes, this CL depends on https://codereview.chromium.org/2609863004/#

+        case MEDIA_VIDEO_FACING_NONE:
+        case NUM_MEDIA_VIDEO_FACING_MODE:
+          facing = chromeos::CrasAudioClient::VideoFacingMode::NONE;
+          break;
+        case MEDIA_VIDEO_FACING_USER:
+          facing = chromeos::CrasAudioClient::VideoFacingMode::USER;
+          break;
+        case MEDIA_VIDEO_FACING_ENVIRONMENT:
+          facing = chromeos::CrasAudioClient::VideoFacingMode::ENVIRONMENT;
+          break;
+      }
+      client->OnVideoCaptureStarted(facing);
+    }
+  }
+#endif
+
   video_capture_device->AllocateAndStart(params, std::move(device_client));
   return video_capture_device;
 }