Pass camera facing info to audio client

content/browser/renderer_host/media/video_capture_manager.cc

 // Copyright (c) 2012 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 "content/browser/renderer_host/media/video_capture_manager.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 38 matching lines @@
 #if defined(USE_AURA)
 #include "content/browser/media/capture/desktop_capture_device_aura.h"
 #endif
 
 #if defined(OS_ANDROID)
 #include "content/browser/media/capture/screen_capture_device_android.h"
 #endif
 
 #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
     : public media::VideoFrameConsumerFeedbackObserver {
  public:
   VideoFrameConsumerFeedbackObserverOnTaskRunner(
       media::VideoFrameConsumerFeedbackObserver* observer,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner)
       : observer_(observer), task_runner_(std::move(task_runner)) {}
 
@@ skipping 577 matching lines @@
 
   std::unique_ptr<VideoCaptureDevice> video_capture_device;
   video_capture_device =
       video_capture_device_factory_->CreateDevice(descriptor);
 
   if (!video_capture_device) {
     device_client->OnError(FROM_HERE, "Could not create capture device");
     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;
 }
 
 std::unique_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DoStartTabCaptureOnDeviceThread(
     const std::string& id,
     const media::VideoCaptureParams& params,
     std::unique_ptr<VideoCaptureDevice::Client> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
@@ skipping 662 matching lines @@
     if (!device_in_queue) {
       // Session ID is only valid for Screen capture. So we can fake it to
       // resume video capture devices here.
       QueueStartDevice(kFakeSessionId, entry.get(), entry->parameters);
     }
   }
 }
 #endif  // defined(OS_ANDROID)
 
 }  // namespace content