Adds MediaStreamSource, MediaStreamAudioSource and MediaStreamVideoCaptureDeviceSource

This cl rename MediaStreamSourceExtraData to MediaStreamSource. This class is used as a common base class for MediaStreamAudioSource and MediaStreamVideoSource.

Further more it adds a specialization MediaStreamVideoSource called MediaStreamVideoCaptureDeviceSource. This is used for 
local video source such that can be created used GetUserMedia.

The cl hooks up the above classes to MediaStreamImpl to make sure they are used when gUM i used from JS.

BUG=334241
TBR jam for content_renderer.gypi
TBR=jam

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247937

[perkj_chrome, 2014-01-09 14:03:24]

Hej
This is what I have been hacking at the last couple of days. 
No need to review yet , just FYI since I saw that there is some overlap with
Ronghuas cl https://codereview.chromium.org/129923002/.

[Ronghua Wu (Left Chromium), 2014-01-10 20:01:08]

Few thoughts.

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capture_device_source.cc (right):

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_video_capture_device_source.cc:28:
dependency_factory->CreateVideoCapturer(device_info());
Instead of reusing the RtcVideoCapturer, I think we should use this one to
replace RtcVideoCapturer. Basically taking the frames from
RtcVideoCaptureDelegate, deliver it to the base class (MediaStreamVideoSource)
via the DeliverVideoFrame interface. The point of MediaStreamVideoSource is to
hide the libjingle detail, i.e. we don't need to implement
cricket::VideoCapturer.

Something like:

class MediaStreamVideoCaptureDeviceSource : public MediaStreamVideoSource {
   ...
   delegate_(new RtcVideoCaptureDelegate(id, vc_manager)),
   ...
  delegate_->StartCapture(
      request,
      base::Bind(&MediaStreamVideoCaptureDeviceSource::OnFrameCaptured,
base::Unretained(this)),
      base::Bind(&MediaStreamVideoCaptureDeviceSource::OnStateChange,
base::Unretained(this)));
  ...


  void RtcVideoCapturer::OnFrameCaptured(...) {
    ...
    DeliverVideoFrame(frame);
  }
}

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:34:
webrtc::VideoSourceInterface* GetAdapter() {
I think the derived class of this class shouldn't need to know anything about
libjingle. Which is why I didn't expose VideoSourceInterface in my cl.

This way the chrome capturer implementation doesn't need to use
cricket::VideoCapturer. The only interface will be DeliverVideoFrame.

[perkj_chrome, 2014-01-12 19:34:38]

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capture_device_source.cc (right):

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_video_capture_device_source.cc:28:
dependency_factory->CreateVideoCapturer(device_info());
On 2014/01/10 20:01:08, Ronghua Wu wrote:
> Instead of reusing the RtcVideoCapturer, I think we should use this one to
> replace RtcVideoCapturer. Basically taking the frames from
> RtcVideoCaptureDelegate, deliver it to the base class (MediaStreamVideoSource)
> via the DeliverVideoFrame interface. The point of MediaStreamVideoSource is to
> hide the libjingle detail, i.e. we don't need to implement
> cricket::VideoCapturer.
> 
> Something like:
> 
> class MediaStreamVideoCaptureDeviceSource : public MediaStreamVideoSource {
>    ...
>    delegate_(new RtcVideoCaptureDelegate(id, vc_manager)),
>    ...
>   delegate_->StartCapture(
>       request,
>       base::Bind(&MediaStreamVideoCaptureDeviceSource::OnFrameCaptured,
> base::Unretained(this)),
>       base::Bind(&MediaStreamVideoCaptureDeviceSource::OnStateChange,
> base::Unretained(this)));
>   ...
> 
> 
>   void RtcVideoCapturer::OnFrameCaptured(...) {
>     ...
>     DeliverVideoFrame(frame);
>   }
> }

Yes - that is what I plan to do in the future. But since all contstraints
parsing is done videosource.cc it will be fairly large cl. So I did this as an
intermediate step.

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:34:
webrtc::VideoSourceInterface* GetAdapter() {
On 2014/01/10 20:01:08, Ronghua Wu wrote:
> I think the derived class of this class shouldn't need to know anything about
> libjingle. Which is why I didn't expose VideoSourceInterface in my cl.
> 
> This way the chrome capturer implementation doesn't need to use
> cricket::VideoCapturer. The only interface will be DeliverVideoFrame.

I don't think that will work until we have properly implemented the
MediaStreamVideo track and moved all adapter layers to where we do
PeerConnection::AddStream. 

We currently support creating new tracks of existing sources so in order to do
that we need to expose the old source interface.

[perkj_chrome, 2014-01-13 16:56:38]

Adding hclam since he also seem to work on a video source implementation. 

This seem to work but the cl is not completely done yet.

[Alpha Left Google, 2014-01-15 01:57:47]

It looks like this is moving to the right direction. I have a suggestion about
logistics.

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.h (right):

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:22: class
CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
It looks like we need an implementation of MediaStreamVideoSource that is based
on libjingle anyway. It's hard that we can remove libjingle dependency in one
step. For example Pepper code still depends on libjingle and
cricket::VideoCapturer to provide video frames.

If so why not just make call this WebRtcMediaStreamVideoSource or something
similar? We can implement a separate class for a local video camera.

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:27: : public
MediaStreamSource {
Just trying to understand a few more items.

Do you plan to have a spearate implementation of MediaStreamVideoSource for
video capture device and webrtc sources?

If so maybe it is better to make an abstract class?

[perkj_chrome, 2014-01-16 15:48:08]

Hej
I think this is ready for review before I grow it even larger. 

The next step after this is to move the constraints parsing from libjingle to
MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use
RtcVideoCaptureDelegate directly.

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.h (right):

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:22: class
CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
On 2014/01/15 01:57:47, Alpha wrote:
> It looks like we need an implementation of MediaStreamVideoSource that is
based
> on libjingle anyway. It's hard that we can remove libjingle dependency in one
> step. For example Pepper code still depends on libjingle and
> cricket::VideoCapturer to provide video frames.
> 
> If so why not just make call this WebRtcMediaStreamVideoSource or something
> similar? We can implement a separate class for a local video camera.

What do you mean by pepper in this case? The extension for creating a video
track from pepper? 
My plan is to move the constraints parsing from libjingle into
MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use the
rtc_video_capture_delegate instead of the cricket::VideoCapturer. 

Ronghua landed a patch for MediaStreamVideoSource where a general libjingle
video source is created (that don't care about the constraints or resolution)
that we can use until we have properly implemented the VideoTracks and the
adapters for libjingle.

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:27: : public
MediaStreamSource {
On 2014/01/15 01:57:47, Alpha wrote:
> Just trying to understand a few more items.
> 
> Do you plan to have a spearate implementation of MediaStreamVideoSource for
> video capture device and webrtc sources?
> 
> If so maybe it is better to make an abstract class? 

My plan was to use MediaStreamSource as a base.
Currently we have identified that we need one implementation for sources created
by getusermedia, one for the new NaCl plugin that can create a source by it self
and 1 for remote video tracks that acts as the bridge between incoming libjingle
video tracks and chromes video tracks.

[Alpha Left Google, 2014-01-16 22:38:36]

On 2014/01/16 15:48:08, perkj wrote:
> Hej
> I think this is ready for review before I grow it even larger. 
> 
> The next step after this is to move the constraints parsing from libjingle to
> MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use
> RtcVideoCaptureDelegate directly.
> 
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> File content/renderer/media/media_stream_video_capture_device_source.h
(right):
> 
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> content/renderer/media/media_stream_video_capture_device_source.h:22: class
> CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
> On 2014/01/15 01:57:47, Alpha wrote:
> > It looks like we need an implementation of MediaStreamVideoSource that is
> based
> > on libjingle anyway. It's hard that we can remove libjingle dependency in
one
> > step. For example Pepper code still depends on libjingle and
> > cricket::VideoCapturer to provide video frames.
> > 
> > If so why not just make call this WebRtcMediaStreamVideoSource or something
> > similar? We can implement a separate class for a local video camera.
> 
> What do you mean by pepper in this case? The extension for creating a video
> track from pepper? 
> My plan is to move the constraints parsing from libjingle into
> MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use the
> rtc_video_capture_delegate instead of the cricket::VideoCapturer. 
> 
> Ronghua landed a patch for MediaStreamVideoSource where a general libjingle
> video source is created (that don't care about the constraints or resolution)
> that we can use until we have properly implemented the VideoTracks and the
> adapters for libjingle.
> 
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> File content/renderer/media/media_stream_video_source.h (right):
> 
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> content/renderer/media/media_stream_video_source.h:27: : public
> MediaStreamSource {
> On 2014/01/15 01:57:47, Alpha wrote:
> > Just trying to understand a few more items.
> > 
> > Do you plan to have a spearate implementation of MediaStreamVideoSource for
> > video capture device and webrtc sources?
> > 
> > If so maybe it is better to make an abstract class? 
> 
> My plan was to use MediaStreamSource as a base.
> Currently we have identified that we need one implementation for sources
created
> by getusermedia, one for the new NaCl plugin that can create a source by it
self
> and 1 for remote video tracks that acts as the bridge between incoming
libjingle
> video tracks and chromes video tracks.

Okay. I see.

[Alpha Left Google, 2014-01-16 22:45:18]

On 2014/01/16 22:38:36, Alpha wrote:
> On 2014/01/16 15:48:08, perkj wrote:
> > Hej
> > I think this is ready for review before I grow it even larger. 
> > 
> > The next step after this is to move the constraints parsing from libjingle
to
> > MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use
> > RtcVideoCaptureDelegate directly.
> > 
> >
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> > File content/renderer/media/media_stream_video_capture_device_source.h
> (right):
> > 
> >
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> > content/renderer/media/media_stream_video_capture_device_source.h:22: class
> > CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
> > On 2014/01/15 01:57:47, Alpha wrote:
> > > It looks like we need an implementation of MediaStreamVideoSource that is
> > based
> > > on libjingle anyway. It's hard that we can remove libjingle dependency in
> one
> > > step. For example Pepper code still depends on libjingle and
> > > cricket::VideoCapturer to provide video frames.
> > > 
> > > If so why not just make call this WebRtcMediaStreamVideoSource or
something
> > > similar? We can implement a separate class for a local video camera.
> > 
> > What do you mean by pepper in this case? The extension for creating a video
> > track from pepper? 
> > My plan is to move the constraints parsing from libjingle into
> > MediaStreamVideoSource and let MediaStreamVideoCaptureDeviceSource use the
> > rtc_video_capture_delegate instead of the cricket::VideoCapturer. 
> > 
> > Ronghua landed a patch for MediaStreamVideoSource where a general libjingle
> > video source is created (that don't care about the constraints or
resolution)
> > that we can use until we have properly implemented the VideoTracks and the
> > adapters for libjingle.
> > 
> >
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> > File content/renderer/media/media_stream_video_source.h (right):
> > 
> >
>
https://codereview.chromium.org/131763002/diff/310001/content/renderer/media/...
> > content/renderer/media/media_stream_video_source.h:27: : public
> > MediaStreamSource {
> > On 2014/01/15 01:57:47, Alpha wrote:
> > > Just trying to understand a few more items.
> > > 
> > > Do you plan to have a spearate implementation of MediaStreamVideoSource
for
> > > video capture device and webrtc sources?
> > > 
> > > If so maybe it is better to make an abstract class? 
> > 
> > My plan was to use MediaStreamSource as a base.
> > Currently we have identified that we need one implementation for sources
> created
> > by getusermedia, one for the new NaCl plugin that can create a source by it
> self
> > and 1 for remote video tracks that acts as the bridge between incoming
> libjingle
> > video tracks and chromes video tracks.
> 
> Okay. I see.

It's a still a fuzzy to me the relationship between source and track.
Furthermore how it related to VideoCaptureImpl in the ideal design. But I'll
just observe now.

[Ronghua Wu (Left Chromium), 2014-01-16 23:02:36]

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:48: // TODO(hclam): This
should be merged with |audio_source_| such that it
I don't see audio_source_ in this file.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:343:
DCHECK(track.extraData() == NULL);
add comment saying this is to check if native track as already been created for
|track|.  otherwise it's hard to understand without knowing what is in the
extradata.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:77: //
CreateNativeMediaStreamAudioSources creates a MediaStream source object
Looking at the implementation of this method, it's more of initialize the source
instead of creating a new one.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:86: virtual
cricket::VideoCapturer* CreateVideoCapturer(
add comment

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:140: void
CallbackonTracksStarted(const ResourcesReady& callback);
CallbackOnTracksStarted

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return  sources_.empty();}
remove extra space

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:37: // Return device information
about the camera or microphone.
camera or microphone should just be 2 examples. suggest to update the comment
accordingly.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:53: // Called when OnLocalsSource
is called.
comment is confusing to me

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:56: // Set device information about
the camera or microphone.
camera or microphone should just be 2 examples. suggest to update the comment
accordingly.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:61: // Set a callback that is
triggered when OnLocalSourceStop is called.
"will be triggered"?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:69: 
remove empty line

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.cc:33:
&webrtc_constraints));
I think this should call the base class Init at the end. It's a no-op today, but
we may add other stuff to base class init in the future.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:8: #include
<vector>
not needed

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:22: class
CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
Maybe CapturerMediaStreamVideoSource instead of
MediaStreamVideoCaptureDeviceSource?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:32: virtual
void Init(const blink::WebMediaConstraints& constraints) OVERRIDE;
why protected? who's going to call?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:18: bool
MandatoryConstraintsMatch(const blink::WebMediaConstraints& c1,
c1->original
c2->new

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:21: // WebMediaConstraints
are NULL in unit tests unfortunately.
being null makes sense in real case also, right? i meant can we remove this
comment?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:42: return m1.size() >=
matched_constraints + m2.size();
sorry I don't get this one. Why matched_constraints + m2.size()? Isn't
matched_constraints part of m2?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:71: DCHECK(GetAdapter());
any particular reason why use getter instead of using adapter_ directly?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:75:
GetAdapter()->RegisterObserver(this);
this doesn't seem be right. i.e. when will you RegisterObserver if adapter is
created before addtrack.

In fact, i think we should return error if adapter is null when addtrack is
called.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:53: // default adapter if the
adapter is not set already.
add back the line above this line

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:19:
blink::WebMediaConstraints constraints;
remove

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:139:
StartAdapter();
we don't need to start adapter in the real case, right? If that's the case, we
should setlive the mock source by default

[Jói, 2014-01-17 08:57:07]

The overall approach makes sense to me. See comments below.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:869: void
MediaStreamImpl::UserMediaRequestInfo::CallbackonTracksStarted(
on -> On

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:900: sources_.begin();
this line should indent +5, and the next +1

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return  sources_.empty();}
missing space after ; (and extra space after return as Ronghua pointed out)

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:44: // if set. All pointers to this
object is invalid after calling this.
nit: is invalid -> are invalid

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:22: class
CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> Maybe CapturerMediaStreamVideoSource instead of
> MediaStreamVideoCaptureDeviceSource?

+1

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:42: return m1.size() >=
matched_constraints + m2.size();
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> sorry I don't get this one. Why matched_constraints + m2.size()? Isn't
> matched_constraints part of m2?

Perhaps the check you want is |matched_constraints == m2.size()|? If that is the
case then please update the documentation of the method to say that c2 does not
need to contain all the constraints in c1, but all of the constraints that c2
does contain need to appear in c1 and match the values from there.

[perkj_chrome, 2014-01-17 13:19:45]

Thanks
PTAL

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:48: // TODO(hclam): This
should be merged with |audio_source_| such that it
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> I don't see audio_source_ in this file.

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:77: //
CreateNativeMediaStreamAudioSources creates a MediaStream source object
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> Looking at the implementation of this method, it's more of initialize the
source
> instead of creating a new one.

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:86: virtual
cricket::VideoCapturer* CreateVideoCapturer(
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> add comment

done. Note that this is kind of temporary. I can not create the RTCVideoCapturer
in the source object itself because it can not be mocked for MediaStreamImpl
unit tests easely. Since it will go away in my next cl I don't want to spend
time on it now.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:869: void
MediaStreamImpl::UserMediaRequestInfo::CallbackonTracksStarted(
On 2014/01/17 08:57:08, Jói wrote:
> on -> On

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:900: sources_.begin();
On 2014/01/17 08:57:08, Jói wrote:
> this line should indent +5, and the next +1

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:140: void
CallbackonTracksStarted(const ResourcesReady& callback);
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> CallbackOnTracksStarted

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return  sources_.empty();}
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> remove extra space

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return  sources_.empty();}
On 2014/01/17 08:57:08, Jói wrote:
> missing space after ; (and extra space after return as Ronghua pointed out)

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:37: // Return device information
about the camera or microphone.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> camera or microphone should just be 2 examples. suggest to update the comment
> accordingly.
ok.
This method isn't something I am very proud of. It only applies for those types
of sources that comes from GetUserMedia.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:44: // if set. All pointers to this
object is invalid after calling this.
On 2014/01/17 08:57:08, Jói wrote:
> nit: is invalid -> are invalid

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:53: // Called when OnLocalsSource
is called.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> comment is confusing to me

Since its wrong after I renamed the method maybe?

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:56: // Set device information about
the camera or microphone.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> camera or microphone should just be 2 examples. suggest to update the comment
> accordingly.

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:61: // Set a callback that is
triggered when OnLocalSourceStop is called.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> "will be triggered"?

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source.h:69: 
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> remove empty line

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.cc:33:
&webrtc_constraints));
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> I think this should call the base class Init at the end. It's a no-op today,
but
> we may add other stuff to base class init in the future. 

ptal - its the other way around. The base class call init and this overrides the
base class implementation.
Once I don't need the RTCVideoCapturer (cricket::VideoCapturer) This goes away.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_device_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> 2014

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:8: #include
<vector>
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> not needed

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:22: class
CONTENT_EXPORT MediaStreamVideoCaptureDeviceSource
On 2014/01/17 08:57:08, Jói wrote:
> On 2014/01/16 23:02:37, Ronghua Wu wrote:
> > Maybe CapturerMediaStreamVideoSource instead of
> > MediaStreamVideoCaptureDeviceSource?
> 
> +1

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_device_source.h:32: virtual
void Init(const blink::WebMediaConstraints& constraints) OVERRIDE;
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> why protected? who's going to call?

See new implementation of MediaStreamSource::AddTrack.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:18: bool
MandatoryConstraintsMatch(const blink::WebMediaConstraints& c1,
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> c1->original
> c2->new

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:21: // WebMediaConstraints
are NULL in unit tests unfortunately.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> being null makes sense in real case also, right? i meant can we remove this
> comment?

Not 100% sure.
Better safe than sorry.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:42: return m1.size() >=
matched_constraints + m2.size();
On 2014/01/17 08:57:08, Jói wrote:
> On 2014/01/16 23:02:37, Ronghua Wu wrote:
> > sorry I don't get this one. Why matched_constraints + m2.size()? Isn't
> > matched_constraints part of m2?
> 
> Perhaps the check you want is |matched_constraints == m2.size()|? If that is
the
> case then please update the documentation of the method to say that c2 does
not
> need to contain all the constraints in c1, but all of the constraints that c2
> does contain need to appear in c1 and match the values from there.

Thinking about it more I think we need to skip this for now. The purpose was to
check that a second call to GetUserMedia failed if constraints were set that
forced a resolution change. 

But the current Chrome implementation don't do that and I can't do it properly
in this cl since the constraints parsing is done in libjingle which mean that I
don't really know that resolution was decided in the first place and therefore
can't properly match the constraints int the second call either.
So I would like to skip this for now and keep the current (not 100% correct)
behaviour until we actually parse the constraints here.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:71: DCHECK(GetAdapter());
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> any particular reason why use getter instead of using adapter_ directly?

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:75:
GetAdapter()->RegisterObserver(this);
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> this doesn't seem be right. i.e. when will you RegisterObserver if adapter is
> created before addtrack.
> 
> In fact, i think we should return error if adapter is null when addtrack is
> called.

The adapter is the webrtc::source and I register to know if it has been started
sucessfully. Init creates the adapter.
In case of local video capture - we can not create adapter until the constraints
has been set. (video resolution) and we don't know if the camera was started
until the adapter change state.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:53: // default adapter if the
adapter is not set already.
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> add back the line above this line

Ptal. Note that SetAdapter can be removed once local video capturer don't need
cricket::VideoCapturer.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:19:
blink::WebMediaConstraints constraints;
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> remove

Done.

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:139:
StartAdapter();
On 2014/01/16 23:02:37, Ronghua Wu wrote:
> we don't need to start adapter in the real case, right? If that's the case, we
> should setlive the mock source by default

I guess that depend on the webrtc::MediaStreamVideoSource implementation. The
current video capture implementation is set to live when the camera has been
started. I think it do that when the first video frame is received.
I added it here to test what happens if the source fail to start. Once the
"real" don't use cricket::VideoCapturer we must have another way of testing what
happen if the video capture device dont start. 
I changed the method name to hopefully better reflect the purpose.

[no longer working on chromium, 2014-01-17 14:51:50]

This kind of big CL is not ideal for Friday afternoon, I got tired after
reviewing this :)

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
File content/renderer/media/media_stream_source_observer.h (left):

https://codereview.chromium.org/131763002/diff/380001/content/renderer/media/...
content/renderer/media/media_stream_source_observer.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
Can you just remove this file?

https://codereview.chromium.org/131763002/diff/530001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/131763002/diff/530001/content/content_rendere...
content/content_renderer.gypi:605:
'renderer/media/capturer_media_stream_video_source.h',
shouldn't the name be media_stream_video_capturer_source?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:160: bool
MediaStreamDependencyFactory::InitializeMediaStreamAudioSource(
Curiously, is this method going to initialize a vector of audio sources? 

And what is the follow-up plan for this method?
Can we somehow remove this method or change it to CreateMediaStreamAudioSource()
after the audio path has the correct implementation for the source ?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:164: DVLOG(1) <<
"CreateNativeMediaStreamAudioSources()";
is it CreateNativeMediaStreamAudioSources or InitializeMediaStreamAudioSource ?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:513:
request->StartTrack((*webkit_tracks)[i], constraints);
The code here is a bit hard to read, is it possible to refactor the
InitializeMediaStreamAudioSource to initialize the individual source? so that we
can do
for (size_t i = 0; i < overridden_audio_array.size(); ++i) {
  InitializeSourceObject(...)
  dependency_factory_->InitializeMediaStreamAudioSource(..)
  (*webkit_tracks)[i].initialize(audio_source...);
  request->StartTrack((*webkit_tracks)[i], constraints);
}

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:578:
static_cast<MediaStreamSource*>(
nit, you can move the static_cast one line above.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:723: static_cast<MediaStreamSource*>
(source.extraData());
fix the indentation.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:731: static_cast<MediaStreamSource*>
(source.extraData());
nit, indentation.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:732: CHECK(source_impl);
nit, you don't need the check here, since you defer it later anyway.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:856: const
blink::WebMediaConstraints& constraints) {
can we add thread check for this class?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:888: request_failed_ = true;
You can remove this member if you pass the success to CheckAllTracksStarted as
input:
CheckAllTracksStarted(success);

And in line 873, you use CheckAllTracksStarted(true)

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:902: if (source_it->id() ==
source.id()) {
nit, remove {}

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return sources_.empty(); }
nit bool IsAllSourcesRemoved() { return sources_.empty(); } const

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:32: virtual void AddTrack(const
blink::WebMediaStreamTrack& track,
so the constraints is back to AddTrack?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:37: // Return device information
about a source that has been created by a
nit, Returns

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:60: void SetDeviceInfo(const
StreamDeviceInfo& device_info) {
The source should be created with a specific device_info, can we pass the
device_info via the constructor and remove this method?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:65: void SetStopCallback(const
SourceStoppedCallback& stop_callback) {
same here, pass the stop_callback via the constructor, if possible.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:72: SourceStoppedCallback
stop_callback_;
and make those members const if they can be passed in the constructor.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:73: adapter_ =
factory_->CreateVideoSource(new webrtc::RemoteVideoCapturer(),
can we change the CreateVideoSource to use blink::constraints?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:122: void
MediaStreamVideoSource::CheckIfAdapterIsLive() {
in which thread this method is called?

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:133: initializing_ = false;
don't you need to call adapter_->UnregisterObserver(this) here?

Please add comment to explain.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:141: if (!it->is_null())
can the callback be null?

[perkj_chrome, 2014-01-19 15:52:38]

PTAL

https://codereview.chromium.org/131763002/diff/530001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/131763002/diff/530001/content/content_rendere...
content/content_renderer.gypi:605:
'renderer/media/capturer_media_stream_video_source.h',
On 2014/01/17 14:51:51, xians1 wrote:
> shouldn't the name be media_stream_video_capturer_source?

ok.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:160: bool
MediaStreamDependencyFactory::InitializeMediaStreamAudioSource(
On 2014/01/17 14:51:51, xians1 wrote:
> Curiously, is this method going to initialize a vector of audio sources? 
> 
> And what is the follow-up plan for this method?
> Can we somehow remove this method or change it to
CreateMediaStreamAudioSource()
> after the audio path has the correct implementation for the source ?
I have left this pretty much untouched and hope you will move all of it to
MediaStreamAudioSource in a follow up.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:164: DVLOG(1) <<
"CreateNativeMediaStreamAudioSources()";
On 2014/01/17 14:51:51, xians1 wrote:
> is it CreateNativeMediaStreamAudioSources or InitializeMediaStreamAudioSource
?

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:513:
request->StartTrack((*webkit_tracks)[i], constraints);
On 2014/01/17 14:51:51, xians1 wrote:
> The code here is a bit hard to read, is it possible to refactor the
> InitializeMediaStreamAudioSource to initialize the individual source? so that
we
> can do
> for (size_t i = 0; i < overridden_audio_array.size(); ++i) {
>   InitializeSourceObject(...)
>   dependency_factory_->InitializeMediaStreamAudioSource(..)
>   (*webkit_tracks)[i].initialize(audio_source...);
>   request->StartTrack((*webkit_tracks)[i], constraints);
> }

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:578:
static_cast<MediaStreamSource*>(
On 2014/01/17 14:51:51, xians1 wrote:
> nit, you can move the static_cast one line above.

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:723: static_cast<MediaStreamSource*>
(source.extraData());
On 2014/01/17 14:51:51, xians1 wrote:
> fix the indentation.

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:731: static_cast<MediaStreamSource*>
(source.extraData());
On 2014/01/17 14:51:51, xians1 wrote:
> nit, indentation.

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:732: CHECK(source_impl);
On 2014/01/17 14:51:51, xians1 wrote:
> nit, you don't need the check here, since you defer it later anyway.

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:856: const
blink::WebMediaConstraints& constraints) {
On 2014/01/17 14:51:51, xians1 wrote:
> can we add thread check for this class?

I don't think that is needed since this is an internal class and its parent is a
single threaded class.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:888: request_failed_ = true;
On 2014/01/17 14:51:51, xians1 wrote:
> You can remove this member if you pass the success to CheckAllTracksStarted as
> input:
> CheckAllTracksStarted(success);
> 
> And in line 873, you use CheckAllTracksStarted(true)

? There can be multiple tracks that we try to start. The guM request fails if
one of  the tracks fails.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:902: if (source_it->id() ==
source.id()) {
On 2014/01/17 14:51:51, xians1 wrote:
> nit, remove {}

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() {
return sources_.empty(); }
On 2014/01/17 14:51:51, xians1 wrote:
> nit bool IsAllSourcesRemoved() { return sources_.empty(); } const

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:32: virtual void AddTrack(const
blink::WebMediaStreamTrack& track,
On 2014/01/17 14:51:51, xians1 wrote:
> so the constraints is back to AddTrack?

Yupp :->

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:37: // Return device information
about a source that has been created by a
On 2014/01/17 14:51:51, xians1 wrote:
> nit, Returns

Done.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:60: void SetDeviceInfo(const
StreamDeviceInfo& device_info) {
On 2014/01/17 14:51:51, xians1 wrote:
> The source should be created with a specific device_info, can we pass the
> device_info via the constructor and remove this method?

There are other types of sources that don't need the device info. Such WebAudio
or the VideoSource that Ronghua is trying to implement for NaCl. So I prefer to
not have it in the constructor.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:65: void SetStopCallback(const
SourceStoppedCallback& stop_callback) {
On 2014/01/17 14:51:51, xians1 wrote:
> same here, pass the stop_callback via the constructor, if possible.

dito. Not all sources need the stop callback I think.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_source.h:72: SourceStoppedCallback
stop_callback_;
On 2014/01/17 14:51:51, xians1 wrote:
> and make those members const if they can be passed in the constructor.
See response above.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:73: adapter_ =
factory_->CreateVideoSource(new webrtc::RemoteVideoCapturer(),
On 2014/01/17 14:51:51, xians1 wrote:
> can we change the CreateVideoSource to use blink::constraints?

I guess we can but Ronghua seemed to prefer to keep the webrtc interface and
since this create a webrt MediaStreamSourceInterface I think that make sence. We
will hopefully not have live with th e webrtc interface for too much longer.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:122: void
MediaStreamVideoSource::CheckIfAdapterIsLive() {
On 2014/01/17 14:51:51, xians1 wrote:
> in which thread this method is called?
This is an internal method so see where it is called from. All public methods
should be thread checked.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:133: initializing_ = false;
On 2014/01/17 14:51:51, xians1 wrote:
> don't you need to call adapter_->UnregisterObserver(this) here?
> 
> Please add comment to explain.

ptal. I had forgot to change GetAdapter to adapter_. I hope this is clear.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:141: if (!it->is_null())
On 2014/01/17 14:51:51, xians1 wrote:
> can the callback be null?

I have never really understood when / if is_null check is required. It can be
null if someone has called Reset on the Callback object. But does the Run method
check this itself or not?

[no longer working on chromium, 2014-01-20 13:36:19]

Almost there.

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:888: request_failed_ = true;
On 2014/01/19 15:52:39, perkj wrote:
> On 2014/01/17 14:51:51, xians1 wrote:
> > You can remove this member if you pass the success to CheckAllTracksStarted
as
> > input:
> > CheckAllTracksStarted(success);
> > 
> > And in line 873, you use CheckAllTracksStarted(true)
> 
> ? There can be multiple tracks that we try to start. The guM request fails if
> one of  the tracks fails.

Are you saying that you will only trigger the callback after getting results
from all the tracks? I thought you would trigger an error callback if any of the
track failed.

To clarify, I was thinking about something like this:
void MediaStreamImpl::UserMediaRequestInfo::CheckAllTracksStarted(bool success)
{
  // TODO, consider a DCHECK instead?
  if (ready_callback_.is_null())
    return;

  // Success but waiting for result from other tracks:
  if (success && !sources_waiting_for_callback_.empty())
    return;

  // Failure or all the tracks have success.
  ready_callback_.Run(this, success);
}

https://codereview.chromium.org/131763002/diff/590001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/590001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:145: bool IsAllSourcesRemoved() const
{ return sources_.empty(); };
nit, you don't need this ;

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:415: << webkit_source->id().utf8();
nit, indentation.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:450: return;
nit, remove the return

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:513: void
MediaStreamImpl::OnCreateNativeTracksComplete(
nit, s/OnCreateNativeTracksCompleted/OnCreateNativeTracksComplete/g

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:585: bool
MediaStreamImpl::FindSourceInRequests(
nit, this method returns true or false depending on if the source is in
requests, how about renaming it to SourceExists() or IsSourceInRequests() ?

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:881: if (!ready_callback_.is_null()
&& sources_waiting_for_callback_.empty())
nit, is it possible to have ready_callback_ to be null?

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:98: void OnLocalMediaStreamStop(const
std::string& label);
nit, would you mind changing this method name to OnLocalMediaStreamStopped() as
well?

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:58: // Set device information about
a source that has been created by a
sets

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:64: // Set a callback that will be
triggered when StopSource is called.
nit, sets

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:65: void SetStopCallback(const
SourceStoppedCallback& stop_callback) {
We discussed this a bit in our lunch time, and I will let you decide if you
should move these two setters into a initialize() method or the constructor.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:127: ? true : false;
you don't need ? true : false
it works with bool success = (adapter_->state() ==
webrtc::MediaSourceInterface::kLive);

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:130:
adapter_->UnregisterObserver(this);
curiously, can this be possible? what can adapter_->state() possibly be in such
case?

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:135:
constraints_callbacks_.clear();
use swap here:
std::vector<ConstraintsCallback> callbacks;
callbacks.swap(constraints_callbacks_);

And curiously, why you need to swap it to a local variable?
Won't it work with triggering the callback with constraints_callbacks_ and clear
it later on?

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:560: // |extra_data| is
NULL if the track is a remote audio track.
nit, %s/audio_source/extra_data/g

[perkj_chrome, 2014-01-20 16:12:05]

PTAL

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/530001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:888: request_failed_ = true;
On 2014/01/20 13:36:20, xians1 wrote:
> On 2014/01/19 15:52:39, perkj wrote:
> > On 2014/01/17 14:51:51, xians1 wrote:
> > > You can remove this member if you pass the success to
CheckAllTracksStarted
> as
> > > input:
> > > CheckAllTracksStarted(success);
> > > 
> > > And in line 873, you use CheckAllTracksStarted(true)
> > 
> > ? There can be multiple tracks that we try to start. The guM request fails
if
> > one of  the tracks fails.
> 
> Are you saying that you will only trigger the callback after getting results
> from all the tracks? I thought you would trigger an error callback if any of
the
> track failed.
> 
> To clarify, I was thinking about something like this:
> void MediaStreamImpl::UserMediaRequestInfo::CheckAllTracksStarted(bool
success)
> {
>   // TODO, consider a DCHECK instead?
>   if (ready_callback_.is_null())
>     return;
> 
>   // Success but waiting for result from other tracks:
>   if (success && !sources_waiting_for_callback_.empty())
>     return;
> 
>   // Failure or all the tracks have success.
>   ready_callback_.Run(this, success);
> }

Do you feel strongly about this? If I do as you suggest I would have to handle
the case where I can get another callback after one of the tracks has failed.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:415: << webkit_source->id().utf8();
On 2014/01/20 13:36:20, xians1 wrote:
> nit, indentation.

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:450: return;
On 2014/01/20 13:36:20, xians1 wrote:
> nit, remove the return

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:513: void
MediaStreamImpl::OnCreateNativeTracksComplete(
On 2014/01/20 13:36:20, xians1 wrote:
> nit, s/OnCreateNativeTracksCompleted/OnCreateNativeTracksComplete/g

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:585: bool
MediaStreamImpl::FindSourceInRequests(
On 2014/01/20 13:36:20, xians1 wrote:
> nit, this method returns true or false depending on if the source is in
> requests, how about renaming it to SourceExists() or IsSourceInRequests() ?

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:881: if (!ready_callback_.is_null()
&& sources_waiting_for_callback_.empty())
On 2014/01/20 13:36:20, xians1 wrote:
> nit, is it possible to have ready_callback_ to be null?
Yes- if the source is started before CheckAllTracksStarted().

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:98: void OnLocalMediaStreamStop(const
std::string& label);
On 2014/01/20 13:36:20, xians1 wrote:
> nit, would you mind changing this method name to OnLocalMediaStreamStopped()
as
> well?

The meaning is unfortunatly slightly different. In this case- MediaStreamImpl
must stop all  sources in the MediaStream. I added a description.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:58: // Set device information about
a source that has been created by a
On 2014/01/20 13:36:20, xians1 wrote:
> sets

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:64: // Set a callback that will be
triggered when StopSource is called.
On 2014/01/20 13:36:20, xians1 wrote:
> nit, sets

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_source.h:65: void SetStopCallback(const
SourceStoppedCallback& stop_callback) {
On 2014/01/20 13:36:20, xians1 wrote:
> We discussed this a bit in our lunch time, and I will let you decide if you
> should move these two setters into a initialize() method or the constructor.

Thanks. Since not all Sources can set them in the ctor I prefer the setter
methods.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:127: ? true : false;
On 2014/01/20 13:36:20, xians1 wrote:
> you don't need ? true : false
> it works with bool success = (adapter_->state() ==
> webrtc::MediaSourceInterface::kLive);

Done.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:130:
adapter_->UnregisterObserver(this);
On 2014/01/20 13:36:20, xians1 wrote:
> curiously, can this be possible? what can adapter_->state() possibly be in
such
> case?

Ex - if the video capture device fail to start, the state will be Ended.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:135:
constraints_callbacks_.clear();
On 2014/01/20 13:36:20, xians1 wrote:
> use swap here:
> std::vector<ConstraintsCallback> callbacks;
> callbacks.swap(constraints_callbacks_);
> 
> And curiously, why you need to swap it to a local variable?
> Won't it work with triggering the callback with constraints_callbacks_ and
clear
> it later on?

No- the source object can be deleted as a consequence of the callback.

ie- GetUserMedia -> source fail to start -> media stream impl delete the source
objects.

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/131763002/diff/660001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:560: // |extra_data| is
NULL if the track is a remote audio track.
On 2014/01/20 13:36:20, xians1 wrote:
> nit, %s/audio_source/extra_data/g

Done.

[no longer working on chromium, 2014-01-21 09:25:29]

Great work.

lgtm with one nit and one question.

https://chromiumcodereview.appspot.com/131763002/diff/530001/content/renderer...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/131763002/diff/530001/content/renderer...
content/renderer/media/media_stream_impl.cc:888: request_failed_ = true;
On 2014/01/20 16:12:06, perkj wrote:
> On 2014/01/20 13:36:20, xians1 wrote:
> > On 2014/01/19 15:52:39, perkj wrote:
> > > On 2014/01/17 14:51:51, xians1 wrote:
> > > > You can remove this member if you pass the success to
> CheckAllTracksStarted
> > as
> > > > input:
> > > > CheckAllTracksStarted(success);
> > > > 
> > > > And in line 873, you use CheckAllTracksStarted(true)
> > > 
> > > ? There can be multiple tracks that we try to start. The guM request fails
> if
> > > one of  the tracks fails.
> > 
> > Are you saying that you will only trigger the callback after getting results
> > from all the tracks? I thought you would trigger an error callback if any of
> the
> > track failed.
> > 
> > To clarify, I was thinking about something like this:
> > void MediaStreamImpl::UserMediaRequestInfo::CheckAllTracksStarted(bool
> success)
> > {
> >   // TODO, consider a DCHECK instead?
> >   if (ready_callback_.is_null())
> >     return;
> > 
> >   // Success but waiting for result from other tracks:
> >   if (success && !sources_waiting_for_callback_.empty())
> >     return;
> > 
> >   // Failure or all the tracks have success.
> >   ready_callback_.Run(this, success);
> > }
> 
> Do you feel strongly about this? If I do as you suggest I would have to handle
> the case where I can get another callback after one of the tracks has failed.

No, I just tried to explain what I thought. Please use the best approach that
you think about.

https://chromiumcodereview.appspot.com/131763002/diff/660001/content/renderer...
File content/renderer/media/media_stream_video_source.cc (right):

https://chromiumcodereview.appspot.com/131763002/diff/660001/content/renderer...
content/renderer/media/media_stream_video_source.cc:135:
constraints_callbacks_.clear();
On 2014/01/20 16:12:06, perkj wrote:
> On 2014/01/20 13:36:20, xians1 wrote:
> > use swap here:
> > std::vector<ConstraintsCallback> callbacks;
> > callbacks.swap(constraints_callbacks_);
> > 
> > And curiously, why you need to swap it to a local variable?
> > Won't it work with triggering the callback with constraints_callbacks_ and
> clear
> > it later on?
> 
> No- the source object can be deleted as a consequence of the callback.
> 
> ie- GetUserMedia -> source fail to start -> media stream impl delete the
source
> objects.

You mean that OnChanged event can be triggered by the callback?

https://chromiumcodereview.appspot.com/131763002/diff/880001/content/content_...
File content/content_renderer.gypi (right):

https://chromiumcodereview.appspot.com/131763002/diff/880001/content/content_...
content/content_renderer.gypi:616:
'renderer/media/media_stream_audio_source.cc',
could you please make sure chrome works with enable_webrtc=0 ?

https://chromiumcodereview.appspot.com/131763002/diff/880001/content/renderer...
File content/renderer/media/media_stream_video_source.cc (right):

https://chromiumcodereview.appspot.com/131763002/diff/880001/content/renderer...
content/renderer/media/media_stream_video_source.cc:126: bool success =
(adapter_->state() == webrtc::MediaSourceInterface::kLive);
nit, move this line closer to where it is used, line 135

[Ronghua Wu (Left Chromium), 2014-01-22 01:44:03]

Good job. I have few more comments. But feel free to commit if you think you
addressed them. LGTM

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:340:
AddNativeTrackToBlinkTrack(webrtc_track, track, true);
nit: bool local_track = true;

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:455: if
(!EnsurePeerConnectionFactory()) {
It's unclear to me what is the rule to call or not call
EnsurePeerConnectionFactory in the beginning of those functions.

We can create a factory getter function and put EnsurePeerConnectionFactory
there once for everyone.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:69: void
MediaStreamVideoSource::Init(
Call this InitAdapter maybe?

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:131: }
It's confusing to put these 3 lines here. Suggest to move them to OnChanged.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:62: void
StartDevice() {
StartSource

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:81: int
NumberOfSuccessConstraintsCallbacks() {
nit const

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:85: int
NumberOfFailedConstraintsCallbacks() {
dito

[perkj_chrome, 2014-01-22 16:56:37]

Can you please PTAL since the requested change of introducing a getter for
pc_factory() changed the code a bit.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:340:
AddNativeTrackToBlinkTrack(webrtc_track, track, true);
On 2014/01/22 01:44:04, Ronghua Wu wrote:
> nit: bool local_track = true;

Done.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:455: if
(!EnsurePeerConnectionFactory()) {
On 2014/01/22 01:44:04, Ronghua Wu wrote:
> It's unclear to me what is the rule to call or not call
> EnsurePeerConnectionFactory in the beginning of those functions.
> 
> We can create a factory getter function and put EnsurePeerConnectionFactory
> there once for everyone.

good idea

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:69: void
MediaStreamVideoSource::Init(
On 2014/01/22 01:44:04, Ronghua Wu wrote:
> Call this InitAdapter maybe?

Done.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:126: bool success =
(adapter_->state() == webrtc::MediaSourceInterface::kLive);
On 2014/01/21 09:25:30, xians1 wrote:
> nit, move this line closer to where it is used, line 135

Done.

https://codereview.chromium.org/131763002/diff/880001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:131: }
On 2014/01/22 01:44:04, Ronghua Wu wrote:
> It's confusing to put these 3 lines here. Suggest to move them to OnChanged.

I can't really. OnChanged is only triggered if the state is not already 
!kInitializing when I register. So instead of checking and register I would
prefer to always do the same.

[Ronghua Wu (Left Chromium), 2014-01-23 00:42:42]

still LG, but please consider the comment.

https://codereview.chromium.org/131763002/diff/1070001/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/1070001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:457:
CreatePeerConnectionFactory();
lazy initialization lg. but how many case do we use MediaStreamDependencyFactory
without needing the pc_factory_? Should we just call CreatePeerConnectionFactory
in the MediaStreamDependencyFactory ctor?

[perkj_chrome, 2014-01-27 18:47:16]

I found the reason why TabCapture.ApiTestsAudio failed. 
It expected a failure if constraints parsing of google specific audio parameters
failed.  These parameters are parsed in libjingle so  I moved the initialization
of the audio source to MediaStreamAudioSource and made sure to check the result.


xians and Joi, PTAL.

https://codereview.chromium.org/131763002/diff/1070001/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/1070001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:457:
CreatePeerConnectionFactory();
On 2014/01/23 00:42:43, Ronghua Wu wrote:
> lazy initialization lg. but how many case do we use
MediaStreamDependencyFactory
> without needing the pc_factory_? Should we just call
CreatePeerConnectionFactory

The MediaStreamDependencyFactory is created when the render thread is created
even if no gum or pc is used. So no- I think it should be lazy.
> in the MediaStreamDependencyFactory ctor?

[Jói, 2014-01-27 22:14:56]

I just have a few nits. LGTM with those resolved. Big change!

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:41: 
nit: unnecessary blank line

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_audio_source.h:53: int render_view_id_; //
Render view Id that created this source.
nit: Two spaces before //
Also, Id -> ID

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:161: // TODO(xians):
Create a new capturer for difference microphones when we
"difference microphones"? Do you mean "different microphones"?

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:459:
MediaStreamDependencyFactory::pc_factory() {
This is not just a simple getter since it creates the factory on demand. Should
be renamed to GetPeerConnectionFactory or GetPCFactory.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.cc:490: it !=
overridden_audio_array.end(); ++it) {
indent +1

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:98: // Called when a MediaStream with
label |label| has been order to stop from
has been order -> has been ordered

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:99: // JavaScript. The implementation
must stop all sources that is not used by
sources that is -> sources that are

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:144: // Triggers |callback| when all
sources used in this request has either
has either -> have either

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:151: bool IsAllSourcesRemoved() const
{ return sources_.empty(); };
Suggest renaming to AreAllSourcesRemoved

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_source.h:38: // JavaScript call to
GetUserMedia. F.E a camera or microphone.
GetUserMedia. F.E. a camera -> GetUserMedia, e.g., a camera

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.cc:48: // we ignore it the
constraints do not match the constraints that was used
"we ignore it the constraints" -> was this supposed to be "we ignore it if the
constraints" ?

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.cc:122: void
MediaStreamVideoSource::CheckIfAdapterIsLive() {
I'm a bit confused by the responsibilities of this method, particularly since it
changes the adapter state and fires callbacks. Improve name and/or
documentation?

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.h:59: // It must be called by a
derived class that overrides the Init method.
Init method -> InitAdapter method

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source_unittest.cc:33: :
number_of_successfull_constraints_applied_(0),
successfull -> successful

[perkj_chrome, 2014-01-28 08:31:42]

Thanks

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:41: 
On 2014/01/27 22:14:57, Jói wrote:
> nit: unnecessary blank line

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_audio_source.h:53: int render_view_id_; //
Render view Id that created this source.
On 2014/01/27 22:14:57, Jói wrote:
> nit: Two spaces before //
> Also, Id -> ID

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:161: // TODO(xians):
Create a new capturer for difference microphones when we
On 2014/01/27 22:14:57, Jói wrote:
> "difference microphones"? Do you mean "different microphones"?

Copy paste - but I guess so...
or wait... it looks like this comment is now plain wrong. A new capturer is
created even before this cl.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:459:
MediaStreamDependencyFactory::pc_factory() {
On 2014/01/27 22:14:57, Jói wrote:
> This is not just a simple getter since it creates the factory on demand.
Should
> be renamed to GetPeerConnectionFactory or GetPCFactory.

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.cc:490: it !=
overridden_audio_array.end(); ++it) {
On 2014/01/27 22:14:57, Jói wrote:
> indent +1

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:98: // Called when a MediaStream with
label |label| has been order to stop from
On 2014/01/27 22:14:57, Jói wrote:
> has been order -> has been ordered

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:99: // JavaScript. The implementation
must stop all sources that is not used by
On 2014/01/27 22:14:57, Jói wrote:
> sources that is -> sources that are

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:144: // Triggers |callback| when all
sources used in this request has either
On 2014/01/27 22:14:57, Jói wrote:
> has either -> have either

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_impl.h:151: bool IsAllSourcesRemoved() const
{ return sources_.empty(); };
On 2014/01/27 22:14:57, Jói wrote:
> Suggest renaming to AreAllSourcesRemoved

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_source.h:38: // JavaScript call to
GetUserMedia. F.E a camera or microphone.
On 2014/01/27 22:14:57, Jói wrote:
> GetUserMedia. F.E. a camera -> GetUserMedia, e.g., a camera

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.cc:48: // we ignore it the
constraints do not match the constraints that was used
On 2014/01/27 22:14:57, Jói wrote:
> "we ignore it the constraints" -> was this supposed to be "we ignore it if the
> constraints" ?

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.cc:122: void
MediaStreamVideoSource::CheckIfAdapterIsLive() {
On 2014/01/27 22:14:57, Jói wrote:
> I'm a bit confused by the responsibilities of this method, particularly since
it
> changes the adapter state and fires callbacks. Improve name and/or
> documentation?

TriggerConstraintsCallbackOnStateChange ?

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source.h:59: // It must be called by a
derived class that overrides the Init method.
On 2014/01/27 22:14:57, Jói wrote:
> Init method -> InitAdapter method

Done.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_video_source_unittest.cc:33: :
number_of_successfull_constraints_applied_(0),
On 2014/01/27 22:14:57, Jói wrote:
> successfull -> successful

Done.

[no longer working on chromium, 2014-01-28 12:09:21]

still lgtm with a nit.

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/131763002/diff/1150001/content/renderer/media...
content/renderer/media/media_stream_audio_source.h:60:
MediaStreamDependencyFactory* factory_;
Could you please add a TODO here to remind me removing this
MediaStreamDependencyFactory member here? The initialization of the source
should not be in MediaStreamDependencyFactory, but I can do this after you land
this CL.

Tack.

SX

[commit-bot: I haz the power, 2014-01-30 08:56:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/131763002/1260001

[commit-bot: I haz the power, 2014-01-30 09:39:37]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-30 09:46:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/131763002/1260001

[commit-bot: I haz the power, 2014-01-30 13:28:32]

Change committed as 247937