Pass camera facing info to audio client

Pass camera facing info to audio client

Pass camera facing info when camera is started to audio client
so that active audio device can be switched according to camera
facing.

BUG=672695
TEST=Print log in CrasAudioClientImpl::OnVideoCaptureStarted()
and verify that facing info is correctly passed.

Review-Url: https://codereview.chromium.org/2634263002
Cr-Commit-Position: refs/heads/master@{#452180}
Committed: https://chromium.googlesource.com/chromium/src/+/77ec80cbdc1be45775bffd27605bfd9549f8a9a6

[shenghao, 2017-01-17 09:35:11]

Description was changed from

==========
Pass camera facing info to audio client

Pass camera facing info when camera is started to audio client
so that active audio device can be switched according to camera
facing.

BUG=crbug:672695
TEST=Print log in CrasAudioClientImpl::OnVideoCaptureStarted()
and verify that facing info is correctly passed.
==========

to

==========
Pass camera facing info to audio client

Pass camera facing info when camera is started to audio client
so that active audio device can be switched according to camera
facing.

BUG=672695
TEST=Print log in CrasAudioClientImpl::OnVideoCaptureStarted()
and verify that facing info is correctly passed.
==========

[shenghao, 2017-01-17 09:39:54]

shenghao@chromium.org changed reviewers:
+ jennyz@chromium.org, mcasas@chromium.org, posciak@chromium.org,
wuchengli@chromium.org

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

chfremer@chromium.org changed reviewers:
+ chfremer@chromium.org

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

I don't fully understand what the target use case is here.
What would CrasAudioClient do with the information that some video capture
device has started with some facing mode? Note, that there may be more than one
video capture device being operated simultaneously by VideoCaptureManager, each
with a different set of consumers.

It feels to me that the decision to select a particular audio device based on
what video capture device is opened should be made proactively at the location
where the request to open the video capture device comes in instead of reacting
to an event saying that a video capture device has been opened. And it should be
at a location that knows about both audio and video. Maybe MediaStreamManager?

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:663:
chromeos::DBusThreadManager::Get();
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.

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:669: switch
(descriptor.facing) {
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?

[jennyz, 2017-01-18 00:01:37]

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
File chromeos/dbus/cras_audio_client.h (right):

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
chromeos/dbus/cras_audio_client.h:52: enum VideoFacingMode { NONE = 0, USER,
ENVIRONMENT };
First: CrasAudioClient is a thin wrap for cras dbus API logic. It is not
appropriate to add other device logic here. A better place will be
CrasAudioHandler. 

Second: VideoFacingMode is better to be defined in the camera related class
instead of audio related class. '

Third: What is USER and ENVIRONMENT facing mode really mean?

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
chromeos/dbus/cras_audio_client.h:62: virtual void
OnVideoCaptureStarted(VideoFacingMode facing) {}
Move this to CrasAudioHandler.

[wuchengli, 2017-01-18 06:30:17]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[shenghao, 2017-01-18 09:30:07]

> I don't fully understand what the target use case is here.
> What would CrasAudioClient do with the information that some video capture
> device has started with some facing mode? Note, that there may be more than
one
> video capture device being operated simultaneously by VideoCaptureManager,
each
> with a different set of consumers.
> 

This doc explains the feature more thoroughly:
https://docs.google.com/document/d/1x_W76rjUnTfYY6ptnDFwW64Q8_85GteiSY90mpST5...

> It feels to me that the decision to select a particular audio device based on
> what video capture device is opened should be made proactively at the location
> where the request to open the video capture device comes in instead of
reacting
> to an event saying that a video capture device has been opened. 

I think it's better to hook up at the place where the video capture device has
been successfully opened,
instead of where the open request comes in, because we don't need to notify
audio side if the
open request fails.

> And it should be
> at a location that knows about both audio and video. Maybe MediaStreamManager?
> 

I can't find where MediaStreamManager is involved in the call stack.
The START request comes in at VideoCaptureHost::Start(), which implements the
mojom interface.
And then the call sequence is:

VideoCaptureHost::Start() =>
VideoCaptureManager::StartCaptureForClient() =>
VideoCaptureManager::QueueStartDevice() =>
VideoCaptureManager::HandleQueuedStartRequest() =>
VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread()

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

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
File chromeos/dbus/cras_audio_client.h (right):

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
chromeos/dbus/cras_audio_client.h:52: enum VideoFacingMode { NONE = 0, USER,
ENVIRONMENT };
On 2017/01/18 00:01:37, jennyz wrote:
> First: CrasAudioClient is a thin wrap for cras dbus API logic. It is not
> appropriate to add other device logic here. A better place will be
> CrasAudioHandler. 
Moved to CrasAudioHandler

> 
> Second: VideoFacingMode is better to be defined in the camera related class
> instead of audio related class. '

We already have a shared VideoFacingMode defined in //media/base. But since it
can't be included here, I am defining a new one, and I imagine that this
VideoFacingMode will only be used within CrasAudioHandler, so it seems more
natural to define it here. If you think there are other appropriate video
related classes in //chromeos world, please suggest.

> 
> Third: What is USER and ENVIRONMENT facing mode really mean?
Added comments. The naming is consistent with the enum in //media/base and in
Webkit.

https://codereview.chromium.org/2634263002/diff/1/chromeos/dbus/cras_audio_cl...
chromeos/dbus/cras_audio_client.h:62: virtual void
OnVideoCaptureStarted(VideoFacingMode facing) {}
On 2017/01/18 00:01:37, jennyz wrote:
> Move this to CrasAudioHandler.

Done.

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:663:
chromeos::DBusThreadManager::Get();
On 2017/01/17 18:05:55, chfremer wrote:
> 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.

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?

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:669: switch
(descriptor.facing) {
On 2017/01/17 18:05:55, chfremer wrote:
> 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?
Yes, this CL depends on https://codereview.chromium.org/2609863004/#

[shenghao, 2017-01-18 10:43:02]

Patchset #3 (id:40001) has been deleted

[shenghao, 2017-01-18 10:43:09]

Patchset #2 (id:20001) has been deleted

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

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:663:
chromeos::DBusThreadManager::Get();
On 2017/01/18 10:41:12, shenghao wrote:
> On 2017/01/17 18:05:55, chfremer wrote:
> > 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.
> 
> 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?

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.

[chfremer, 2017-01-18 18:39:43]

On 2017/01/18 09:30:07, shenghao wrote:
> > I don't fully understand what the target use case is here.
> > What would CrasAudioClient do with the information that some video capture
> > device has started with some facing mode? Note, that there may be more than
> one
> > video capture device being operated simultaneously by VideoCaptureManager,
> each
> > with a different set of consumers.
> > 
> 
> This doc explains the feature more thoroughly:
>
https://docs.google.com/document/d/1x_W76rjUnTfYY6ptnDFwW64Q8_85GteiSY90mpST5...
> 

After reading the document, my understanding is that we want to know when a
particular client requests a switch from one camera to another, and then switch
to the appropriate microphone accordingly.

The first question I have about this is what API clients would use to switch
cameras. Currently, in Chromium, the cameras available on a device are listed
individually and can be requested by id. I am not aware of any API that
semantically allows switching between cameras on a multi-camera device. Instead,
to switch cameras, clients just close the stream from one camera and open a new
one from the other.

Note that the access that a client in Chromium gets to video capture devices is
not exclusive. As a result, no single client determines when exactly a video
capture device is started or stopped. For example, when client B is requesting
access to a certain camera, the camera may already be running, because a client
A is already using it. Therefore, the event that a camera has started or stopped
cannot reliably tell us if and when a particular client switches from one camera
to another.

It seems what we want is a new API that (similar to the Android API) exposes
multiple devices as a virtual single device and allows switching between front
and back. The implementation of this API would then have the information needed
to decide which camera and microphones to switch to.

Does this make sense or am I completely on the wrong track?

[shenghao, 2017-01-19 07:55:12]

On 2017/01/18 18:39:43, chfremer wrote:
> On 2017/01/18 09:30:07, shenghao wrote:
> > > I don't fully understand what the target use case is here.
> > > What would CrasAudioClient do with the information that some video capture
> > > device has started with some facing mode? Note, that there may be more
than
> > one
> > > video capture device being operated simultaneously by VideoCaptureManager,
> > each
> > > with a different set of consumers.
> > > 
> > 
> > This doc explains the feature more thoroughly:
> >
>
https://docs.google.com/document/d/1x_W76rjUnTfYY6ptnDFwW64Q8_85GteiSY90mpST5...
> > 
> 
> After reading the document, my understanding is that we want to know when a
> particular client requests a switch from one camera to another, and then
switch
> to the appropriate microphone accordingly.
> 
> The first question I have about this is what API clients would use to switch
> cameras. Currently, in Chromium, the cameras available on a device are listed
> individually and can be requested by id. I am not aware of any API that
> semantically allows switching between cameras on a multi-camera device.
Instead,
> to switch cameras, clients just close the stream from one camera and open a
new
> one from the other.
> 

When the second client requests to start a camera that's already started, it's
blocked at
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
Therefore, VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread(), where my
code is added,
will only be executed if the camera is started by the first client.
Similarly, I am adding OnVideoCaptureStopped() at
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...,
which will only be executed when no client is accessing the camera.
This avoids the problem you mentioned.

> Note that the access that a client in Chromium gets to video capture devices
is
> not exclusive. As a result, no single client determines when exactly a video
> capture device is started or stopped. For example, when client B is requesting
> access to a certain camera, the camera may already be running, because a
client
> A is already using it. Therefore, the event that a camera has started or
stopped
> cannot reliably tell us if and when a particular client switches from one
camera
> to another.
> 
> It seems what we want is a new API that (similar to the Android API) exposes
> multiple devices as a virtual single device and allows switching between front
> and back. The implementation of this API would then have the information
needed
> to decide which camera and microphones to switch to.
> 
> Does this make sense or am I completely on the wrong track?

[shenghao, 2017-01-19 13:42:33]

On 2017/01/18 18:36:10, chfremer wrote:
>
https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/media/video_capture_manager.cc (right):
> 
>
https://codereview.chromium.org/2634263002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/media/video_capture_manager.cc:663:
> chromeos::DBusThreadManager::Get();
> On 2017/01/18 10:41:12, shenghao wrote:
> > On 2017/01/17 18:05:55, chfremer wrote:
> > > 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.
> > 
> > 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?
> 
> 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.

1) May you take a look at the latest patch set to see if that is what you
expect?
I moved the Observer to //components/arc/video_facing.h and drafted the CL.

2) Can you suggest a place where I can put the Observer so that both //chromeos
and //content/browser can depend on?
I have tried the current //component/arc location and still encounter cyclic
dependency.
The place I can thought of putting is in //base, but that is super weird.

[chfremer, 2017-01-19 17:57:41]

On 2017/01/19 07:55:12, shenghao wrote:
> On 2017/01/18 18:39:43, chfremer wrote:
> > On 2017/01/18 09:30:07, shenghao wrote:
> > > > I don't fully understand what the target use case is here.
> > > > What would CrasAudioClient do with the information that some video
capture
> > > > device has started with some facing mode? Note, that there may be more
> than
> > > one
> > > > video capture device being operated simultaneously by
VideoCaptureManager,
> > > each
> > > > with a different set of consumers.
> > > > 
> > > 
> > > This doc explains the feature more thoroughly:
> > >
> >
>
https://docs.google.com/document/d/1x_W76rjUnTfYY6ptnDFwW64Q8_85GteiSY90mpST5...
> > > 
> > 
> > After reading the document, my understanding is that we want to know when a
> > particular client requests a switch from one camera to another, and then
> switch
> > to the appropriate microphone accordingly.
> > 
> > The first question I have about this is what API clients would use to switch
> > cameras. Currently, in Chromium, the cameras available on a device are
listed
> > individually and can be requested by id. I am not aware of any API that
> > semantically allows switching between cameras on a multi-camera device.
> Instead,
> > to switch cameras, clients just close the stream from one camera and open a
> new
> > one from the other.
> > 
> 
> When the second client requests to start a camera that's already started, it's
> blocked at
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
> Therefore, VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread(), where my
> code is added,
> will only be executed if the camera is started by the first client.
> Similarly, I am adding OnVideoCaptureStopped() at
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...,
> which will only be executed when no client is accessing the camera.
> This avoids the problem you mentioned.

I am not sure I follow.
Let me make an example:
We have a device with two cameras named Front, Back. 
Then:
1. Client A starts using Front.
2. Observer->OnVideoCaptureStarted(Front) gets invoked.
3. Client B starts using Back.
4. Observer->OnVideoCaptureStarted(Back) gets invoked.
Now, Client B wants to switch from Back to Front:
5. Client B stops using Back.
6. Observer->OnVideoCaptureStopped(Back) gets invoked.
7. Client B starts using Front
No callback to Observer is issued, since Front is already in use.
=> We cannot tell from the Observer events that Client B has switched from Back
to Front.
 
> > Note that the access that a client in Chromium gets to video capture devices
> is
> > not exclusive. As a result, no single client determines when exactly a video
> > capture device is started or stopped. For example, when client B is
requesting
> > access to a certain camera, the camera may already be running, because a
> client
> > A is already using it. Therefore, the event that a camera has started or
> stopped
> > cannot reliably tell us if and when a particular client switches from one
> camera
> > to another.
> > 
> > It seems what we want is a new API that (similar to the Android API) exposes
> > multiple devices as a virtual single device and allows switching between
front
> > and back. The implementation of this API would then have the information
> needed
> > to decide which camera and microphones to switch to.
> > 
> > Does this make sense or am I completely on the wrong track?

[shenghao, 2017-01-20 03:53:39]

> 
> I am not sure I follow.
> Let me make an example:
> We have a device with two cameras named Front, Back. 
> Then:
> 1. Client A starts using Front.
> 2. Observer->OnVideoCaptureStarted(Front) gets invoked.
> 3. Client B starts using Back.
> 4. Observer->OnVideoCaptureStarted(Back) gets invoked.
> Now, Client B wants to switch from Back to Front:
> 5. Client B stops using Back.
> 6. Observer->OnVideoCaptureStopped(Back) gets invoked.
> 7. Client B starts using Front
> No callback to Observer is issued, since Front is already in use.
> => We cannot tell from the Observer events that Client B has switched from
Back
> to Front.

We don't care about which client is using which camera. We only care about which
camera is active.
So in this scenario, at the last stage, audio knows that front camera is the
only active camera since step 6, so front audio should be active.
When there are multiple active cameras, the audio device corresponded to most
recently started camera should be active.
(Even better: if multiple audio devices can be active at the same time, then all
the corresponded ones should be active.)

[chfremer, 2017-01-20 18:23:53]

I sent you an e-mail regarding my questions about the overall use case. I
believe I understand it, assuming that the idea is to expose all microphones as
a single audio input device that internally switches its sources based on which
video capture devices are active.

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
File components/arc/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:10: enum VideoFacingMode {
Can we make this an enum class?

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:11: NONE = 0,
Why is this entry needed and what does it mean?

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:16: };
Thinking about convertible devices, could it happen that the notion of which
camera faces the user changes when the device is "converted", e.g. flipped over?
If so, should we revise the enum value names?

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc
(right):

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:255:
};
I count a total of 9 places where we have unit tests that need this exact same
mock.
It seems awfully redundant to define it 9 times. Would it be possible to move it
to a separate file that can be shared between all the tests that need it?

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:271:
new MockVideoCaptureObserver());
Since neither MediaStreamManager nor VideoCaptureManager take ownership of this,
this looks like a memory leak (here and in the 8 other tests that do the same).

We seem to have a situation where sometimes we want VideoCaptureManager to take
ownership (e.g. in all the tests), and sometimes we don't, i.e. when passing in
a pointer to a global object.

My recommendation here is to make |capture_observer_| in VideoCaptureManager a
std::unique_ptr<>. Then, to pass in CrasAudioHandler::Get(), we should wrap it
using a class that forwards to a non-owned raw pointer, i.e.:

class NonOwnedVideoCaptureObserver : public VideoCaptureObserver {
 public:
  NonOwnedVideoCaptureObserver(VideoCaptureObserver* observer)
      : observer_(observer) {}

  void OnVideoCaptureStarted(component::VideoFacingMode facing_mode) {
    observer_->OnVideoCaptureStarted(facing_mode);
  }

  void OnVideoCaptureStopped(component::VideoFacingMode facing_mode) {
    observer_->OnVideoCaptureStopped(facing_mode);
  }

 private:
  VideoCaptureObserver* const observer_;
};

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:674:
capture_observer_->OnVideoCaptureStarted(facing);
Where will we call OnVideoCaptureStopped()?

[shenghao, 2017-01-24 12:45:58]

shenghao@chromium.org changed reviewers:
+ jochen@chromium.org, xhwang@chromium.org

[shenghao, 2017-01-24 12:47:06]

shenghao@chromium.org changed reviewers:
+ hubbe@chromium.org
- xhwang@chromium.org

[shenghao, 2017-01-24 12:52:01]

I did a major refactor. Please take a look.

1. Since I can't find any reasonble place to put the Observer class that both
//chromeos/audio and //content/browser can depend on, I make a general Observer
in //media/base that takes in the //chromeos/audio observer as constructor
parameter.
This way, we can eliminate the #ifdef(OS_CHROMEOS) in media_stream_manager and
video_capture_manager.

2. In browser_main_loop, CrasAudioHandler is initialized after
media_stream_manager is initialized, so putting the observer as ctor parameter
of media_stream_manager is impossible. I changed it to call
SetVideoCaptureObserver() after CrasAudioHandler is initialized.

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
File components/arc/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:10: enum VideoFacingMode {
On 2017/01/20 18:23:52, chfremer wrote:
> Can we make this an enum class?

Done.

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:11: NONE = 0,
On 2017/01/20 18:23:52, chfremer wrote:
> Why is this entry needed and what does it mean?

This enum should have 1-1 mapping from VideoFacingMode in
//media/base/video_facing.h.
The "NONE" here is useful for some devices that do not have facing, such as
desktop capturing.

https://codereview.chromium.org/2634263002/diff/80001/components/arc/video_fa...
components/arc/video_facing.h:16: };
On 2017/01/20 18:23:52, chfremer wrote:
> Thinking about convertible devices, could it happen that the notion of which
> camera faces the user changes when the device is "converted", e.g. flipped
over?
> If so, should we revise the enum value names?

Yeah I am aware of the convertibles. However I still want to name it
"environment-facing" cameras even though it can flip to the same side as
"user-facing" cameras. The audio devices are mounted at similar positions as
cameras, so when environment-facing camera is active, the environment-facing
audio device should be active, whether the camera is flipped is unrelated.

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc
(right):

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:255:
};
On 2017/01/20 18:23:52, chfremer wrote:
> I count a total of 9 places where we have unit tests that need this exact same
> mock.
> It seems awfully redundant to define it 9 times. Would it be possible to move
it
> to a separate file that can be shared between all the tests that need it?

Done.

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc:271:
new MockVideoCaptureObserver());
On 2017/01/20 18:23:53, chfremer wrote:
> Since neither MediaStreamManager nor VideoCaptureManager take ownership of
this,
> this looks like a memory leak (here and in the 8 other tests that do the
same).
> 
> We seem to have a situation where sometimes we want VideoCaptureManager to
take
> ownership (e.g. in all the tests), and sometimes we don't, i.e. when passing
in
> a pointer to a global object.
> 
> My recommendation here is to make |capture_observer_| in VideoCaptureManager a
> std::unique_ptr<>. Then, to pass in CrasAudioHandler::Get(), we should wrap it
> using a class that forwards to a non-owned raw pointer, i.e.:
> 
> class NonOwnedVideoCaptureObserver : public VideoCaptureObserver {
>  public:
>   NonOwnedVideoCaptureObserver(VideoCaptureObserver* observer)
>       : observer_(observer) {}
> 
>   void OnVideoCaptureStarted(component::VideoFacingMode facing_mode) {
>     observer_->OnVideoCaptureStarted(facing_mode);
>   }
> 
>   void OnVideoCaptureStopped(component::VideoFacingMode facing_mode) {
>     observer_->OnVideoCaptureStopped(facing_mode);
>   }
> 
>  private:
>   VideoCaptureObserver* const observer_;
> };

Removed all the changes in tests.

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:674:
capture_observer_->OnVideoCaptureStarted(facing);
On 2017/01/20 18:23:53, chfremer wrote:
> Where will we call OnVideoCaptureStopped()?

Oh I forgot to add it.

[shenghao, 2017-01-24 12:53:24]

Patchset #4 (id:100001) has been deleted

[chfremer1, 2017-01-25 18:13:43]

chfremer@google.com changed reviewers:
+ chfremer@google.com

[chfremer1, 2017-01-25 18:13:44]

Thanks so much for putting in the effort to make things clean.
I think we are on the right way, just not quite there yet.
Please see my comments and questions.

Also, I think we should add at least one test case for class VideoCaptureManager
that verifies the newly added functionality, i.e. that the callbacks to
VideoCaptureObserver arrive as expected.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1167: new media::VideoCaptureObserver(
Similar to the issue we had in the previous PatchSet with the tests, this
media::VideoCaptureObserver leaks, because noone takes ownership. I still
recommend the solution I proposed in the comments for that PatchSet to fix this.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1169: chromeos::CrasAudioHandler::Get())));
Can we guarantee that the CrasAudioHandler instance outlives
|media_stream_manager|?

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: void
SetVideoCaptureObserver(media::VideoCaptureObserver* capture_observer);
I recommend that we indicate to the reader that this method is part of the
construction+configuration interface of MediaStreamManager, as opposed to the
interface intended for "usage by its regular clients". 

In my opinion, the cleanest way to express this in C++ would be to explicitly
define and expose an interface containing only the methods that are intended for
"usage by regular clients", and SetVideoCaptureObserver() would probably not be
a part of that. This approach has the extra advantage of allowing us to easily
mock out MediaStreamManager when testing its clients.

If that is too much work, the "cheap" way would be to express this information
in a comment.

Either way, we should express the following information to the reader:
* At what time of the lifecycle of MediaStreamManager is it legal to call this
method.
* On what thread would it need to be called (probably the same as the
constructor?)
* Is it legal to call it more than once? Potentially setting |capture_observer|
back to nullptr? (This would be useful for clients who cannot guarantee that
|capture_observer| outlives the MediaStreamManager instance.)
* Is it legal to not call it at all?
* On what thread will the callbacks to |capture_observer| arrive?

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:57: void
SetVideoCaptureObserver(media::VideoCaptureObserver* observer);
Same considerations here as with the SetVideoCaptureObserver() method in
MediaStreamManager.

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facin...
media/base/video_facing.h:9: #include "chromeos/audio/cras_audio_handler.h"
I am not familiar with the dependency structures around chromeos.
But I am surprised that (according to this CL) chromeos is "lower-level" than
media/base, i.e. it is okay for media/base to depend on chromeos and not the
other way around. Can you confirm that this is really the case?

If chromeos could depend on media/base, we could just place the interface
VideoCaptureObserver in here. This would eliminate the need for the adapter
defined below.

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facin...
media/base/video_facing.h:23: class VideoCaptureObserver {
It would be cleaner (and make our life easier in testing) if the thing that
VideoCaptureManager takes a dependency on is a pure interface. If we really need
two versions of this interface (one in chromeos:: and one in media::), then the
adapter functionality currently expressed in this class should be separate from
that, i.e. a class

VideoCaptureObserverAdapter : public media::VideoCaptureObserver {
 public:
   ...
 private:
  chromeos::VideoCaptureObserver* observer_;
}

[jennyz, 2017-01-26 00:26:12]

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.cc:134: // TODO(jennyz): Switch active audio
device according to video facing.
Upon receiving video capture stopped notification, how should audio switch
device according to video facing? Will there be another active camera, or both
camera become inactive. How can we find out?

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:35: };
Why do we need this enum here? Isn't it defined in video_facing.h?

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:46: public VideoCaptureObserver {
Need to "#include media/base/video_facing.h".

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:50: AudioDevicePriorityQueue;
Why change this line?

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:121: void
OnVideoCaptureStarted(VideoFacingMode facing) override;
Add a line above like:
// media::VideoCaptureObserver overrides.

[shenghao, 2017-01-26 15:33:35]

On 2017/01/26 00:26:12, jennyz wrote:
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> File chromeos/audio/cras_audio_handler.cc (right):
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> chromeos/audio/cras_audio_handler.cc:134: // TODO(jennyz): Switch active audio
> device according to video facing.
> Upon receiving video capture stopped notification, how should audio switch
> device according to video facing? Will there be another active camera, or both
> camera become inactive. How can we find out?
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> File chromeos/audio/cras_audio_handler.h (right):
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> chromeos/audio/cras_audio_handler.h:35: };
> Why do we need this enum here? Isn't it defined in video_facing.h?
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> chromeos/audio/cras_audio_handler.h:46: public VideoCaptureObserver {
> Need to "#include media/base/video_facing.h".
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> chromeos/audio/cras_audio_handler.h:50: AudioDevicePriorityQueue;
> Why change this line?
> 
>
https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
> chromeos/audio/cras_audio_handler.h:121: void
> OnVideoCaptureStarted(VideoFacingMode facing) override;
> Add a line above like:
> // media::VideoCaptureObserver overrides.

Sorry I don't have time to address the comments before vacation (1/27-2/4). I
will come back to this CL after the vacation.

[shenghao, 2017-01-29 10:21:02]

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.cc:134: // TODO(jennyz): Switch active audio
device according to video facing.
On 2017/01/26 00:26:11, jennyz wrote:
> Upon receiving video capture stopped notification, how should audio switch
> device according to video facing? Will there be another active camera, or both
> camera become inactive. How can we find out?

Talked offline with Jenny.

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:35: };
On 2017/01/26 00:26:11, jennyz wrote:
> Why do we need this enum here? Isn't it defined in video_facing.h?

We can't include //media/base/video_facing.h due to cyclic dependency.

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:46: public VideoCaptureObserver {
On 2017/01/26 00:26:12, jennyz wrote:
> Need to "#include media/base/video_facing.h".

We can't include //media/base/video_facing.h due to cyclic dependency.

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:50: AudioDevicePriorityQueue;
On 2017/01/26 00:26:12, jennyz wrote:
> Why change this line?

This is changed by "git cl format"

https://codereview.chromium.org/2634263002/diff/120001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:121: void
OnVideoCaptureStarted(VideoFacingMode facing) override;
On 2017/01/26 00:26:12, jennyz wrote:
> Add a line above like:
> // media::VideoCaptureObserver overrides.

It doesn't override media::VideoCaptureObserver. It overrides
VideoCaptureObserver defined at the top of this file.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1167: new media::VideoCaptureObserver(
On 2017/01/25 18:13:44, chfremer1 wrote:
> Similar to the issue we had in the previous PatchSet with the tests, this
> media::VideoCaptureObserver leaks, because noone takes ownership. I still
> recommend the solution I proposed in the comments for that PatchSet to fix
this.

Done.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1169: chromeos::CrasAudioHandler::Get())));
On 2017/01/25 18:13:44, chfremer1 wrote:
> Can we guarantee that the CrasAudioHandler instance outlives
> |media_stream_manager|?

Nope, CrasAudioHandler is deleted in PostMainMessageLoopRun() on line 1226.
Added a statement to clear the observer before that happens.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: void
SetVideoCaptureObserver(media::VideoCaptureObserver* capture_observer);
On 2017/01/25 18:13:44, chfremer1 wrote:
> I recommend that we indicate to the reader that this method is part of the
> construction+configuration interface of MediaStreamManager, as opposed to the
> interface intended for "usage by its regular clients". 
> 
> In my opinion, the cleanest way to express this in C++ would be to explicitly
> define and expose an interface containing only the methods that are intended
for
> "usage by regular clients", and SetVideoCaptureObserver() would probably not
be
> a part of that. This approach has the extra advantage of allowing us to easily
> mock out MediaStreamManager when testing its clients.
> 
> If that is too much work, the "cheap" way would be to express this information
> in a comment.
> 
> Either way, we should express the following information to the reader:
> * At what time of the lifecycle of MediaStreamManager is it legal to call this
> method.
> * On what thread would it need to be called (probably the same as the
> constructor?)
> * Is it legal to call it more than once? Potentially setting
|capture_observer|
> back to nullptr? (This would be useful for clients who cannot guarantee that
> |capture_observer| outlives the MediaStreamManager instance.)
> * Is it legal to not call it at all?
> * On what thread will the callbacks to |capture_observer| arrive?

Done.

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:57: void
SetVideoCaptureObserver(media::VideoCaptureObserver* observer);
On 2017/01/25 18:13:44, chfremer1 wrote:
> Same considerations here as with the SetVideoCaptureObserver() method in
> MediaStreamManager.

Done.

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facin...
media/base/video_facing.h:9: #include "chromeos/audio/cras_audio_handler.h"
On 2017/01/25 18:13:44, chfremer1 wrote:
> I am not familiar with the dependency structures around chromeos.
> But I am surprised that (according to this CL) chromeos is "lower-level" than
> media/base, i.e. it is okay for media/base to depend on chromeos and not the
> other way around. Can you confirm that this is really the case?

Yes, as you can see in https://cs.chromium.org/chromium/src/media/DEPS //media/
depends on //chromeos/audio (//media/base does not have its own DEPS file).

And even if I push the dependency in //media/ to //media/audio, //chromeos/audio
still can't depend on //media/base due to a cyclic dependency.

> 
> If chromeos could depend on media/base, we could just place the interface
> VideoCaptureObserver in here. This would eliminate the need for the adapter
> defined below.

https://codereview.chromium.org/2634263002/diff/120001/media/base/video_facin...
media/base/video_facing.h:23: class VideoCaptureObserver {
On 2017/01/25 18:13:44, chfremer1 wrote:
> It would be cleaner (and make our life easier in testing) if the thing that
> VideoCaptureManager takes a dependency on is a pure interface. If we really
need
> two versions of this interface (one in chromeos:: and one in media::), then
the
> adapter functionality currently expressed in this class should be separate
from
> that, i.e. a class
> 
> VideoCaptureObserverAdapter : public media::VideoCaptureObserver {
>  public:
>    ...
>  private:
>   chromeos::VideoCaptureObserver* observer_;
> }

Done.

[DaleCurtis, 2017-01-30 19:22:18]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tommi@chromium.org

[DaleCurtis, 2017-01-30 19:22:19]

I think this might be happening at the wrong level. It's very hard to switch
input streams from the device level since the sample rate and channel count of
the device can change. You would need to be prepared to inject a converter
between the consumer and the AudioInputStream.

Instead it seems like this should trigger some sort of input device change for
the application using the AudioInputStream (getUserMedia/WebRTC/Pepper/etc) at
that time. It can then decide either to change to the new microphone or reject
the change because it can't support the parameter change.

+tommi for thoughts/routing to appropriate capture folk.

[chfremer, 2017-01-30 19:49:28]

A few more technical nits, see code comments.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1168: new
media::VideoCaptureObserverAdapter(
We should use base::MakeUnique<> here.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1169:
static_cast<chromeos::VideoCaptureObserver*>(
I don't think the compiler needs this cast, and I feel that even though it is
nice to verbosely indicate to readers that the instance is passed in as a
pointer to the interface, it may not be worth the extra line of code.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1228:
std::unique_ptr<media::VideoCaptureObserver>());
media_stream_manager_->SetVideoCaptureObserver(nullptr) should work and feels
more clear to me.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:339:
capture_observer_(nullptr) {}
No need to initialize a std::unique_ptr<> member to nullptr. It is automatic.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:482: if
(device_info != NULL && capture_observer_.get() != NULL) {
NULL -> nullptr

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:542: if
(capture_observer_.get() != NULL) {
NULL -> nullptr

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:61: //
media::VideoCaptureObserver arrive on browser threads.
This seems too permissive to be true.
If we could really call this at any time from any thread, we would get race
conditions with the usage of |capture_observer_|.

Also, clients probably need to know on exactly which browser thread the
callbacks will arrive (not enough to know that it is _some_ browser thread).

https://codereview.chromium.org/2634263002/diff/140001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/140001/media/base/video_facin...
media/base/video_facing.h:29: class VideoCaptureObserverAdapter : public
VideoCaptureObserver {
The way this looks to me now is that VideoCaptureObserverAdapter is a
chromeos-specific implementation of media::VideoCaptureObserver. 

As such, it would make sense to move the chromeos specific parts to a separate
file that is only built for chromeos builds. This way we could avoid the #if
defined(OS_CHROMEOS) in the platform-independent layer of the code. And then we
could call this class ChromeOSVideoCaptureObserver, which would make it more
clear that the dependency of media on chromeos serves the purpose of providing a
chromeos-specific implementation of a platform-independent media interface.

[jennyz, 2017-02-03 22:44:13]

The BUILD deps needs to be fixed, I patched the cl, it fails. But I manually
changed BUILD.gn to fix the dependency between content/browser and media/base,
so that I can build the image. I have tested the image on reef 9202.7.0 build.
It seems not working, CrasAudioHandler does not receive any notification when I
rotate the device. I haven't dig further to see the upper stack to find out why
the event is generated.

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:41: };
It is kind of a little weird to define a VideoCaptureObserver in
CrasAudioHandler. Can you put VideorCaptureObserver in the video related file
and use Observer pattern, and use AddObserver/RemoveObserver to either add or
remove CrasAudioHandler object to the Video object that broadcast the video
events?

[jennyz, 2017-02-07 00:45:07]

On 2017/01/30 19:22:19, DaleCurtis wrote:
> I think this might be happening at the wrong level. It's very hard to switch
> input streams from the device level since the sample rate and channel count of
> the device can change. You would need to be prepared to inject a converter
> between the consumer and the AudioInputStream.
> 
> Instead it seems like this should trigger some sort of input device change for
> the application using the AudioInputStream (getUserMedia/WebRTC/Pepper/etc) at
> that time. It can then decide either to change to the new microphone or reject
> the change because it can't support the parameter change.
> 
> +tommi for thoughts/routing to appropriate capture folk.

From CrasAudioHandler side, we can call audio dbus API(SetActiveInputNode) to
change the active input device. But I am not sure at the media stream level,
what will happen. If we just switch the active input device, will it break
anything? The typical use cases we have now is to change active audio devices
when a new device is hot plugged, or an existing device is removed, or user pick
different device from UI. For output devices, they work well. But we haven't
tried the real cases with input devices, although the logic are identical for
input devices.

[DaleCurtis, 2017-02-07 01:18:48]

On 2017/02/07 at 00:45:07, jennyz wrote:
> On 2017/01/30 19:22:19, DaleCurtis wrote:
> > I think this might be happening at the wrong level. It's very hard to switch
> > input streams from the device level since the sample rate and channel count
of
> > the device can change. You would need to be prepared to inject a converter
> > between the consumer and the AudioInputStream.
> > 
> > Instead it seems like this should trigger some sort of input device change
for
> > the application using the AudioInputStream (getUserMedia/WebRTC/Pepper/etc)
at
> > that time. It can then decide either to change to the new microphone or
reject
> > the change because it can't support the parameter change.
> > 
> > +tommi for thoughts/routing to appropriate capture folk.
> 
> From CrasAudioHandler side, we can call audio dbus API(SetActiveInputNode) to
change the active input device. But I am not sure at the media stream level,
what will happen. If we just switch the active input device, will it break
anything? The typical use cases we have now is to change active audio devices
when a new device is hot plugged, or an existing device is removed, or user pick
different device from UI. For output devices, they work well. But we haven't
tried the real cases with input devices, although the logic are identical for
input devices.

I suspect it will break things. Especially if the parameters change between the
input devices.

[dgreid, 2017-02-07 01:55:10]

On 2017/02/07 01:18:48, DaleCurtis wrote:
> On 2017/02/07 at 00:45:07, jennyz wrote:
> > On 2017/01/30 19:22:19, DaleCurtis wrote:
> > > I think this might be happening at the wrong level. It's very hard to
switch
> > > input streams from the device level since the sample rate and channel
count
> of
> > > the device can change. You would need to be prepared to inject a converter
> > > between the consumer and the AudioInputStream.
> > > 
> > > Instead it seems like this should trigger some sort of input device change
> for
> > > the application using the AudioInputStream
(getUserMedia/WebRTC/Pepper/etc)
> at
> > > that time. It can then decide either to change to the new microphone or
> reject
> > > the change because it can't support the parameter change.
> > > 
> > > +tommi for thoughts/routing to appropriate capture folk.
> > 
> > From CrasAudioHandler side, we can call audio dbus API(SetActiveInputNode)
to
> change the active input device. But I am not sure at the media stream level,
> what will happen. If we just switch the active input device, will it break
> anything? The typical use cases we have now is to change active audio devices
> when a new device is hot plugged, or an existing device is removed, or user
pick
> different device from UI. For output devices, they work well. But we haven't
> tried the real cases with input devices, although the logic are identical for
> input devices.
> 
> I suspect it will break things. Especially if the parameters change between
the
> input devices.

For chormeOS, cras will convert to whatever stream parameters you ask for.  But
I don't think that will be needed in this case since the mics will be the same
type of device and run at the same sample rate and format.

[DaleCurtis, 2017-02-07 02:09:19]

Per chat discussion I defer to the media stream owners for approval here. This
seems fine if it's a CrOS specific feature; it will definitely need more
systemic work if it were to be enabled on other platforms for the reasons I
mentioned previously.

[shenghao, 2017-02-08 02:07:10]

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:41: };
On 2017/02/03 22:44:13, jennyz wrote:
> It is kind of a little weird to define a VideoCaptureObserver in
> CrasAudioHandler. Can you put VideorCaptureObserver in the video related file
> and use Observer pattern, and use AddObserver/RemoveObserver to either add or
> remove CrasAudioHandler object to the Video object that broadcast the video
> events?

Talked offline with Jenny. She is trying to find a better place. Otherwise we
will keep it as is.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1168: new
media::VideoCaptureObserverAdapter(
On 2017/01/30 19:49:28, chfremer wrote:
> We should use base::MakeUnique<> here.

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1169:
static_cast<chromeos::VideoCaptureObserver*>(
On 2017/01/30 19:49:28, chfremer wrote:
> I don't think the compiler needs this cast, and I feel that even though it is
> nice to verbosely indicate to readers that the instance is passed in as a
> pointer to the interface, it may not be worth the extra line of code.

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/browse...
content/browser/browser_main_loop.cc:1228:
std::unique_ptr<media::VideoCaptureObserver>());
On 2017/01/30 19:49:28, chfremer wrote:
> media_stream_manager_->SetVideoCaptureObserver(nullptr) should work and feels
> more clear to me.

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:339:
capture_observer_(nullptr) {}
On 2017/01/30 19:49:28, chfremer wrote:
> No need to initialize a std::unique_ptr<> member to nullptr. It is automatic.

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:482: if
(device_info != NULL && capture_observer_.get() != NULL) {
On 2017/01/30 19:49:28, chfremer wrote:
> NULL -> nullptr

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:542: if
(capture_observer_.get() != NULL) {
On 2017/01/30 19:49:28, chfremer wrote:
> NULL -> nullptr

Done.

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:61: //
media::VideoCaptureObserver arrive on browser threads.
On 2017/01/30 19:49:28, chfremer wrote:
> This seems too permissive to be true.
> If we could really call this at any time from any thread, we would get race
> conditions with the usage of |capture_observer_|.
> 
> Also, clients probably need to know on exactly which browser thread the
> callbacks will arrive (not enough to know that it is _some_ browser thread).

Done.

https://codereview.chromium.org/2634263002/diff/140001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/140001/media/base/video_facin...
media/base/video_facing.h:29: class VideoCaptureObserverAdapter : public
VideoCaptureObserver {
On 2017/01/30 19:49:28, chfremer wrote:
> The way this looks to me now is that VideoCaptureObserverAdapter is a
> chromeos-specific implementation of media::VideoCaptureObserver. 
> 
> As such, it would make sense to move the chromeos specific parts to a separate
> file that is only built for chromeos builds. This way we could avoid the #if
> defined(OS_CHROMEOS) in the platform-independent layer of the code. And then
we
> could call this class ChromeOSVideoCaptureObserver, which would make it more
> clear that the dependency of media on chromeos serves the purpose of providing
a
> chromeos-specific implementation of a platform-independent media interface.

I name it to be VideoCaptureObserverChromeOS. The convention is to put ChromeOS
as the suffix.

[shenghao, 2017-02-08 02:08:03]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 02:09:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 04:20:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:20:01]

Dry run: This issue passed the CQ dry run.

[jennyz, 2017-02-08 22:21:33]

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:41: };
On 2017/02/08 02:07:09, shenghao wrote:
> On 2017/02/03 22:44:13, jennyz wrote:
> > It is kind of a little weird to define a VideoCaptureObserver in
> > CrasAudioHandler. Can you put VideorCaptureObserver in the video related
file
> > and use Observer pattern, and use AddObserver/RemoveObserver to either add
or
> > remove CrasAudioHandler object to the Video object that broadcast the video
> > events?
> 
> Talked offline with Jenny. She is trying to find a better place. Otherwise we
> will keep it as is.

Have you tried to make a separate target for the media::VideoCaptureObserver
class, instead of building it with //media/base? By making it a separate class,
you can have chromeos depend on this new target, instead of //media/base, then
it breaks the cyclical dependency issue you had before. It will simplify the
code a lot.

https://codereview.chromium.org/2634263002/diff/160001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/160001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:50: AudioDevicePriorityQueue;
Why changing the format of this line?

[shenghao, 2017-02-09 02:08:51]

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/140001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:41: };
On 2017/02/08 22:21:33, jennyz wrote:
> On 2017/02/08 02:07:09, shenghao wrote:
> > On 2017/02/03 22:44:13, jennyz wrote:
> > > It is kind of a little weird to define a VideoCaptureObserver in
> > > CrasAudioHandler. Can you put VideorCaptureObserver in the video related
> file
> > > and use Observer pattern, and use AddObserver/RemoveObserver to either add
> or
> > > remove CrasAudioHandler object to the Video object that broadcast the
video
> > > events?
> > 
> > Talked offline with Jenny. She is trying to find a better place. Otherwise
we
> > will keep it as is.
> 
> Have you tried to make a separate target for the media::VideoCaptureObserver
> class, instead of building it with //media/base? By making it a separate
class,
> you can have chromeos depend on this new target, instead of //media/base, then
> it breaks the cyclical dependency issue you had before. It will simplify the
> code a lot.

Removed this part.

https://codereview.chromium.org/2634263002/diff/160001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2634263002/diff/160001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:50: AudioDevicePriorityQueue;
On 2017/02/08 22:21:33, jennyz wrote:
> Why changing the format of this line?

This is changed by "git cl format". You have raised this question on 1/26 with
the same comment and I have replied it on 1/29.

[shenghao, 2017-02-09 02:09:56]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:10:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 02:14:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 02:14:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shenghao, 2017-02-09 02:22:39]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[shenghao, 2017-02-09 02:22:46]

Patchset #7 (id:180001) has been deleted

[shenghao, 2017-02-09 02:22:56]

Patchset #7 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-02-09 02:23:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 02:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 02:27:38]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shenghao, 2017-02-09 18:20:19]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 18:20:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 18:25:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 18:25:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jennyz, 2017-02-09 19:42:56]

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: //
SetVideoCaptureObserver() and RemoveAllVideoCaptureObservers() should be
Where is SetVideoCaptureObserver, should be AddVideoCaptureObserver, right?

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:343: if (observer
!= nullptr)
client code is not supposed to call this function passing a nullptr, what about
using a DCHECK to guard?

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:57: //
AddVideoCaptureObserver() can be called only before any devices.
before any devices are opened or what?

[shenghao, 2017-02-09 22:32:38]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[shenghao, 2017-02-09 22:32:51]

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: //
SetVideoCaptureObserver() and RemoveAllVideoCaptureObservers() should be
On 2017/02/09 19:42:55, jennyz wrote:
> Where is SetVideoCaptureObserver, should be AddVideoCaptureObserver, right?

Done.

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:343: if (observer
!= nullptr)
On 2017/02/09 19:42:55, jennyz wrote:
> client code is not supposed to call this function passing a nullptr, what
about
> using a DCHECK to guard?

Done.

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/220001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:57: //
AddVideoCaptureObserver() can be called only before any devices.
On 2017/02/09 19:42:55, jennyz wrote:
> before any devices are opened or what?

Done.

[commit-bot: I haz the power, 2017-02-09 22:33:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 23:04:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 23:04:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shenghao, 2017-02-10 00:17:54]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 00:18:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 00:34:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 00:34:14]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shenghao, 2017-02-10 01:00:51]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[shenghao, 2017-02-10 01:01:06]

Patchset #8 (id:240001) has been deleted

[shenghao, 2017-02-10 01:01:18]

Patchset #8 (id:260001) has been deleted

[commit-bot: I haz the power, 2017-02-10 01:01:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-10 01:01:29]

Patchset #8 (id:280001) has been deleted

[shenghao, 2017-02-10 01:39:24]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 01:39:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 07:40:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 07:40:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shenghao, 2017-02-10 18:19:48]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 18:20:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jennyz, 2017-02-10 18:28:28]

https://codereview.chromium.org/2634263002/diff/290016/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/290016/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:350:
std::vector<media::VideoCaptureObserver*> capture_observers_;
Why don't use base::ObserverList<media::VideoCaptureObserver>, then you can use
.AddObserver, RemoveObserver, etc, to manage the observers.
see https://cs.chromium.org/chromium/src/base/observer_list.h

[commit-bot: I haz the power, 2017-02-10 19:56:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 19:56:35]

Dry run: This issue passed the CQ dry run.

[shenghao, 2017-02-11 00:54:31]

Patchset #8 (id:300001) has been deleted

[shenghao, 2017-02-11 00:54:51]

https://codereview.chromium.org/2634263002/diff/290016/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/290016/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:350:
std::vector<media::VideoCaptureObserver*> capture_observers_;
On 2017/02/10 18:28:28, jennyz wrote:
> Why don't use base::ObserverList<media::VideoCaptureObserver>, then you can
use
> .AddObserver, RemoveObserver, etc, to manage the observers.
> see https://cs.chromium.org/chromium/src/base/observer_list.h

Done.

[shenghao, 2017-02-11 00:55:24]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:56:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 06:57:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 06:57:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shenghao, 2017-02-13 17:47:02]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 17:47:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jennyz, 2017-02-13 18:33:03]

https://codereview.chromium.org/2634263002/diff/330001/chromeos/audio/DEPS
File chromeos/audio/DEPS (right):

https://codereview.chromium.org/2634263002/diff/330001/chromeos/audio/DEPS#ne...
chromeos/audio/DEPS:3: ]
Since deps of BUILD.gn is at chromeos level, why not just put this into DEPS of
chromeos level, it seems make more sense to keep them at the same level.

[chfremer, 2017-02-13 19:00:15]

Patch Set 9 lgtm % missing virtual destructor in interface VideoCaptureObserver

Thanks for putting in the effort to make this as clean as possible!

https://codereview.chromium.org/2634263002/diff/330001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2634263002/diff/330001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:351:
base::ObserverList<media::VideoCaptureObserver> capture_observers_;
Nice! I didn't even know we had something like that in base.

https://codereview.chromium.org/2634263002/diff/330001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/330001/media/base/video_facin...
media/base/video_facing.h:20: public:
As an interface, VideoCaptureObserver must get a virtual destructor. Please add.

[shenghao, 2017-02-13 19:52:48]

https://codereview.chromium.org/2634263002/diff/330001/chromeos/audio/DEPS
File chromeos/audio/DEPS (right):

https://codereview.chromium.org/2634263002/diff/330001/chromeos/audio/DEPS#ne...
chromeos/audio/DEPS:3: ]
On 2017/02/13 18:33:02, jennyz wrote:
> Since deps of BUILD.gn is at chromeos level, why not just put this into DEPS
of
> chromeos level, it seems make more sense to keep them at the same level.

Done.

https://codereview.chromium.org/2634263002/diff/330001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/330001/media/base/video_facin...
media/base/video_facing.h:20: public:
On 2017/02/13 19:00:15, chfremer wrote:
> As an interface, VideoCaptureObserver must get a virtual destructor. Please
add.

Done.

[jennyz, 2017-02-13 21:05:11]

lgtm

[shenghao, 2017-02-13 21:10:38]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 21:11:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-02-13 22:28:20]

media/ rslgtm

[DaleCurtis, 2017-02-13 22:28:28]

lgtm

[shenghao, 2017-02-13 22:49:35]

shenghao@chromium.org changed reviewers:
+ satorux@chromium.org

[commit-bot: I haz the power, 2017-02-13 23:01:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 23:01:04]

Dry run: This issue passed the CQ dry run.

[satorux1, 2017-02-14 00:58:46]

chromeos/... lgtm

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:14: MEDIA_VIDEO_FACING_ENVIRONMENT,
random idea: I'm not familiar with the terminology here but front-facing and
rear-facing are terms that I often hear. Maybe MEDIA_VIDEO_FRONT_FACING,
MEDIA_VIDEO_REAR_FACING?

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:16: NUM_MEDIA_VIDEO_FACING_MODE
nit: Add 'S' at the end?

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:19: class VideoCaptureObserver {
class comment is missing. I think it'd be good to add.

[tommi (sloooow) - chröme, 2017-02-14 09:11:19]

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:464: void
MediaStreamManager::AddVideoCaptureObserver(
please make sure that these to methods are always called on the IO thread, like
all the other methods that touch the video capture manager.
(and add  DCHECK_CURRENTLY_ON(BrowserThread::IO) at the top)

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: //
AddVideoCaptureObserver() and RemoveAllVideoCaptureObservers() should be
nit: s/should/must

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:105: //
media::VideoCaptureObserver callacks.
callacks -> callbacks

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:106: // The methods
can be called on whatever thread. The callbacks of
I would much prefer to access the video_capture_manager_ always on the same
thread.  As is, this design (these two new methods) actually affect the design
of the video_capture_manager_ implementation and could break threading
assumptions set by that design.

[jochen (gone - plz use gerrit), 2017-02-14 12:20:40]

jochen@chromium.org changed reviewers:
+ hta@chromium.org

[jochen (gone - plz use gerrit), 2017-02-14 12:20:41]

+hta

[shenghao, 2017-02-15 03:00:38]

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:464: void
MediaStreamManager::AddVideoCaptureObserver(
On 2017/02/14 09:11:19, tommi (krómíum) wrote:
> please make sure that these to methods are always called on the IO thread,
like
> all the other methods that touch the video capture manager.
> (and add  DCHECK_CURRENTLY_ON(BrowserThread::IO) at the top)

Done.

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: //
AddVideoCaptureObserver() and RemoveAllVideoCaptureObservers() should be
On 2017/02/14 09:11:19, tommi (krómíum) wrote:
> nit: s/should/must

Done.

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:105: //
media::VideoCaptureObserver callacks.
On 2017/02/14 09:11:19, tommi (krómíum) wrote:
> callacks -> callbacks

Done.

https://codereview.chromium.org/2634263002/diff/370001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:106: // The methods
can be called on whatever thread. The callbacks of
On 2017/02/14 09:11:19, tommi (krómíum) wrote:
> I would much prefer to access the video_capture_manager_ always on the same
> thread.  As is, this design (these two new methods) actually affect the design
> of the video_capture_manager_ implementation and could break threading
> assumptions set by that design.

Done.

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facing.h
File media/base/video_facing.h (right):

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:14: MEDIA_VIDEO_FACING_ENVIRONMENT,
On 2017/02/14 00:58:45, satorux1 wrote:
> random idea: I'm not familiar with the terminology here but front-facing and
> rear-facing are terms that I often hear. Maybe MEDIA_VIDEO_FRONT_FACING,
> MEDIA_VIDEO_REAR_FACING? 

We want to use the same terms as in
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebMe...

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:16: NUM_MEDIA_VIDEO_FACING_MODE
On 2017/02/14 00:58:45, satorux1 wrote:
> nit: Add 'S' at the end?

Done.

https://codereview.chromium.org/2634263002/diff/370001/media/base/video_facin...
media/base/video_facing.h:19: class VideoCaptureObserver {
On 2017/02/14 00:58:45, satorux1 wrote:
> class comment is missing. I think it'd be good to add.

Done.

[shenghao, 2017-02-15 03:01:05]

shenghao@chromium.org changed reviewers:
+ nasko@chromium.org

[shenghao, 2017-02-15 03:05:55]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:07:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 03:12:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 03:12:28]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shenghao, 2017-02-15 03:49:32]

Patchset #13 (id:410001) has been deleted

[shenghao, 2017-02-15 03:49:45]

Patchset #12 (id:390001) has been deleted

[shenghao, 2017-02-15 03:49:54]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:51:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 05:56:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 05:56:54]

Dry run: This issue passed the CQ dry run.

[tommi (sloooow) - chröme, 2017-02-15 18:56:04]

lgtm % one request

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:343: void
VideoCaptureManager::AddVideoCaptureObserver(
please add DCHECK_CURRENTLY_ON(BrowserThread::IO); to both
AddVideoCaptureObserver and RemoveAllVideoCaptureObservers

[nasko, 2017-02-15 19:13:47]

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
content/browser/browser_main_loop.cc:1169: if (media_stream_manager_.get() !=
NULL) {
Use nullptr, no more NULL. Also, just dropping the ".get() != NULL" part should
still work and is the preferred pattern to use.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
content/browser/browser_main_loop.cc:1176: DLOG(ERROR) << "media_stream_manager_
is null.";
Are those DLOGs just for debugging and will be remove before commiting the code?

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:456: if
(video_capture_manager_.get() != nullptr) {
Drop the ".get()" and ideally also the nullptr comparison.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:463: if
(video_capture_manager_.get() != nullptr) {
Same here.

https://codereview.chromium.org/2634263002/diff/430001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2634263002/diff/430001/content/public/common/...
content/public/common/BUILD.gn:265: "//media/base:video_facing",
Why does content/public get another dependency when there are no content/public
changes?

[shenghao, 2017-02-15 22:46:49]

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
content/browser/browser_main_loop.cc:1169: if (media_stream_manager_.get() !=
NULL) {
On 2017/02/15 19:13:47, nasko wrote:
> Use nullptr, no more NULL. Also, just dropping the ".get() != NULL" part
should
> still work and is the preferred pattern to use.

Done.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/browse...
content/browser/browser_main_loop.cc:1176: DLOG(ERROR) << "media_stream_manager_
is null.";
On 2017/02/15 19:13:47, nasko wrote:
> Are those DLOGs just for debugging and will be remove before commiting the
code?

Nope. I want to keep it here.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:456: if
(video_capture_manager_.get() != nullptr) {
On 2017/02/15 19:13:47, nasko wrote:
> Drop the ".get()" and ideally also the nullptr comparison.

Done.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:463: if
(video_capture_manager_.get() != nullptr) {
On 2017/02/15 19:13:47, nasko wrote:
> Same here.

Done.

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2634263002/diff/430001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:343: void
VideoCaptureManager::AddVideoCaptureObserver(
On 2017/02/15 18:56:04, tommi (krómíum) wrote:
> please add DCHECK_CURRENTLY_ON(BrowserThread::IO); to both
> AddVideoCaptureObserver and RemoveAllVideoCaptureObservers

Done.

https://codereview.chromium.org/2634263002/diff/430001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2634263002/diff/430001/content/public/common/...
content/public/common/BUILD.gn:265: "//media/base:video_facing",
On 2017/02/15 19:13:47, nasko wrote:
> Why does content/public get another dependency when there are no
content/public
> changes?

Because //content/public/common/media_stream_request.h depends on
//media/base/video_facing.h, and since we make video_facing.h into its own
target, all targets depend on it need to include //media/base:video_facing.

[shenghao, 2017-02-16 01:21:57]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 01:23:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 03:26:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:26:39]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[jochen (gone - plz use gerrit), 2017-02-16 15:07:15]

instead of adding the new video_facing as a dependency everywhere, can't you
make it a public dependency of the existing target?

also, please add a test.

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
content/browser/browser_main_loop.cc:1176: DLOG(ERROR) << "media_stream_manager_
is null.";
please use DVLOG()

how can this happen?

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
content/browser/browser_main_loop.cc:1179: DLOG(ERROR) << "CrasAudioHandler is
not initialized.";
how can this happen?

[shenghao, 2017-02-16 21:12:28]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:13:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 21:20:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 21:20:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shenghao, 2017-02-16 22:59:41]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 23:01:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-17 03:35:23]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 03:36:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-17 03:37:53]

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
content/browser/browser_main_loop.cc:1176: DLOG(ERROR) << "media_stream_manager_
is null.";
On 2017/02/16 15:07:15, jochen wrote:
> please use DVLOG()
> 
> how can this happen?

Done. If the call sequence is not correct, it could happen.

https://codereview.chromium.org/2634263002/diff/450001/content/browser/browse...
content/browser/browser_main_loop.cc:1179: DLOG(ERROR) << "CrasAudioHandler is
not initialized.";
On 2017/02/16 15:07:15, jochen wrote:
> how can this happen?

Done. If the call sequence is not correct, it could happen.

[shenghao, 2017-02-17 03:38:17]

Patchset #14 (id:470001) has been deleted

[shenghao, 2017-02-17 03:38:32]

Patchset #14 (id:490001) has been deleted

[shenghao, 2017-02-17 03:38:46]

Patchset #10 (id:350001) has been deleted

[commit-bot: I haz the power, 2017-02-17 04:00:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 04:00:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hta - Chromium, 2017-02-17 10:14:22]

High level comment:

If audio devices are tied to cameras, they need to populate the group ID field
in media device info.
I don't think that's part of this CL, but should definitely be done, so that Web
applications that do explicit device management can get the info they need to
make the right decisions, and not depend on automatic behavior to get the right
microphone.

[shenghao, 2017-02-17 19:21:31]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 19:22:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-17 20:33:03]

Patchset #13 (id:510001) has been deleted

[shenghao, 2017-02-17 20:33:20]

Patchset #13 (id:530001) has been deleted

[shenghao, 2017-02-17 20:33:35]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 20:35:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-17 20:37:59]

shenghao@chromium.org changed reviewers:
+ palmer@chromium.org

[palmer, 2017-02-17 23:11:28]

RS LGTM

https://codereview.chromium.org/2634263002/diff/550001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/2634263002/diff/550001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:101: //
AddVideoCaptureObserver() and RemoveAllVideoCaptureObservers() must be
Nit: Notate code identifiers with |...|:

  |AddVideoCaptureObserver|
  |media::VideoCaptureObserver|

[commit-bot: I haz the power, 2017-02-17 23:26:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:26:29]

Dry run: This issue passed the CQ dry run.

[shenghao, 2017-02-20 19:53:52]

On 2017/02/17 10:14:22, hta - Chromium wrote:
> High level comment:
> 
> If audio devices are tied to cameras, they need to populate the group ID field
> in media device info.
> I don't think that's part of this CL, but should definitely be done, so that
Web
> applications that do explicit device management can get the info they need to
> make the right decisions, and not depend on automatic behavior to get the
right
> microphone.

I think Jenny can do it in separate CL. But let's focus on getting this CL in
because it blocks all the subsequent development.

[jochen (gone - plz use gerrit), 2017-02-21 11:37:24]

https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
content/browser/browser_main_loop.cc:1168: if
(chromeos::CrasAudioHandler::IsInitialized()) {
please move this entire logic to chrome_browser_main_chromeos.cc

I'd add the observer methods to media_capture_devices

[jochen (gone - plz use gerrit), 2017-02-21 11:38:19]

On 2017/02/21 at 11:37:24, jochen wrote:
>
https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
> File content/browser/browser_main_loop.cc (right):
> 
>
https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
> content/browser/browser_main_loop.cc:1168: if
(chromeos::CrasAudioHandler::IsInitialized()) {
> please move this entire logic to chrome_browser_main_chromeos.cc


The reason is that this code depends on the audio handler being initialized
there - content shouldn't (even implicitly) rely on the behavior of the embedder
here.

[shenghao, 2017-02-21 21:40:25]

The CQ bit was checked by shenghao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 21:41:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shenghao, 2017-02-21 21:41:15]

https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2634263002/diff/550001/content/browser/browse...
content/browser/browser_main_loop.cc:1168: if
(chromeos::CrasAudioHandler::IsInitialized()) {
On 2017/02/21 11:37:24, jochen wrote:
> please move this entire logic to chrome_browser_main_chromeos.cc
> 
> I'd add the observer methods to media_capture_devices

Done.

[commit-bot: I haz the power, 2017-02-21 23:44:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:44:48]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[jochen (gone - plz use gerrit), 2017-02-22 08:58:56]

lgtm

[shenghao, 2017-02-22 17:19:32]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2017-02-22 17:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chfremer@chromium.org, dalecurtis@chromium.org, satorux@chromium.org,
jennyz@chromium.org, tommi@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2634263002/#ps570001
(title: "Moved to chrome_browser_main_chromeos.cc")

[commit-bot: I haz the power, 2017-02-22 17:20:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:14:25]

CQ is committing da patch.

Bot data: {"patchset_id": 570001, "attempt_start_ts": 1487783972879970,
"parent_rev": "4d2ef6f0772e9f78fa3088d8dccccc0692aec3f1", "commit_rev":
"77ec80cbdc1be45775bffd27605bfd9549f8a9a6"}

[commit-bot: I haz the power, 2017-02-22 20:15:07]

Description was changed from

==========
Pass camera facing info to audio client

Pass camera facing info when camera is started to audio client
so that active audio device can be switched according to camera
facing.

BUG=672695
TEST=Print log in CrasAudioClientImpl::OnVideoCaptureStarted()
and verify that facing info is correctly passed.
==========

to

==========
Pass camera facing info to audio client

Pass camera facing info when camera is started to audio client
so that active audio device can be switched according to camera
facing.

BUG=672695
TEST=Print log in CrasAudioClientImpl::OnVideoCaptureStarted()
and verify that facing info is correctly passed.

Review-Url: https://codereview.chromium.org/2634263002
Cr-Commit-Position: refs/heads/master@{#452180}
Committed:
https://chromium.googlesource.com/chromium/src/+/77ec80cbdc1be45775bffd27605b...
==========

[commit-bot: I haz the power, 2017-02-22 20:15:10]

Committed patchset #14 (id:570001) as
https://chromium.googlesource.com/chromium/src/+/77ec80cbdc1be45775bffd27605b...