Implements MediaStreamVideoSource.

Implements MediaStreamVideoSource.

1) Add MediaStreamDependencyFactory::CreateVideoSource to create a webrtc::VideoSourceInterface that's associated with given capturer.
2) Implement MediaStreamVideoSource:
  * Creates a webrtc::VideoSource instance with a fake cricket::VideoCapturer. Use this webrtc::VideoSource as the adapter to talk to the libjingle world.
  * When AddTrack is called, creates native track for the given blink track. And connect the native track with the webrtc::VideoSource so that the track can get frames from this source.
  * When DeliverVideoFrame is called, feeds the frame to all the registered tracks via the adapter (webrtc::VideoSource)'s FrameInput interface.


BUG=334241
R=joi@chromium.org, perkj@chromium.org, xians@chromium.org

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

[Ronghua Wu (Left Chromium), 2014-01-09 00:11:17]

WIP, not ready for reviewing yet. Will let you know once ready.

[Jói, 2014-01-09 11:18:12]

I didn't review in detail, these are just a couple of nits that I noticed while
skimming the change.

I like the approach of isolating the libjingle usage to an adapter class.

Cheers,
Jói

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

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:49: void
SetMsFactory(MediaStreamDependencyFactory* factory);
If it's only for testing, name it SetMsFactoryForTesting, that way the Chromium
presubmit will help ensure it is only used by testing code.

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:53: private:
missing DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_video_source.cc (right):

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_source.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_video_source.h (right):

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_source.h:24: // The different between
this and webrtc::VideoSource is that this one gets
different between -> difference between

[Peng, 2014-01-09 15:32:55]

I am not familiar with webrtc. Forgive me if my questions are silly.

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:16: const
blink::WebMediaStreamTrack& track,
If the video track has been connected to a video source (maybe a webcam), what
will happen?

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:17: const
blink::WebMediaConstraints& constraints) {
Does it allow adding a track more than once? maybe with different constraints?

[Ronghua Wu (Left Chromium), 2014-01-09 16:50:05]

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:16: const
blink::WebMediaStreamTrack& track,
On 2014/01/09 15:32:55, Peng wrote:
> If the video track has been connected to a video source (maybe a webcam), what
> will happen?

I've not decided yet. Open to suggestion. Either the new source overrides the
original source or just return an error. A track can only have one source at a
time.

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:17: const
blink::WebMediaConstraints& constraints) {
On 2014/01/09 15:32:55, Peng wrote:
> Does it allow adding a track more than once? maybe with different constraints?

A track can update its constraints. So I think what we need here is an
ApplyConstraints interface. Thanks for pointing out.

http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-Constrainable-app...

[Peng, 2014-01-09 19:29:46]

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:16: const
blink::WebMediaStreamTrack& track,
On 2014/01/09 16:50:06, Ronghua Wu wrote:
> On 2014/01/09 15:32:55, Peng wrote:
> > If the video track has been connected to a video source (maybe a webcam),
what
> > will happen?
> 
> I've not decided yet. Open to suggestion. Either the new source overrides the
> original source or just return an error. A track can only have one source at a
> time.

IMHO, supporting changing video source in a track is not a good idea. Not sure
if blink::WebMediaStreamTrack supports it. It may have some unexpected problems.
I remember MediaStream supports adding and removing tracks. So if we want to
change a video source, we can always remove old track which contains the video
source from MediaStream and add a new track with the new video source.


BTW, I still don't know how to create an empty WebMediaStreamTrack which can be
used here and be passed to JS. If this class could have a helper function which
can create a WebMediaStreamTrack from a MediaStreamVideoSource, it will be
great. Thanks.

[Ronghua Wu (Left Chromium), 2014-01-09 21:24:53]

On 2014/01/09 19:29:46, Peng wrote:
>
https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
> File content/renderer/media/media_stream_video_source.cc (right):
> 
>
https://codereview.chromium.org/129923002/diff/40001/content/renderer/media/m...
> content/renderer/media/media_stream_video_source.cc:16: const
> blink::WebMediaStreamTrack& track,
> On 2014/01/09 16:50:06, Ronghua Wu wrote:
> > On 2014/01/09 15:32:55, Peng wrote:
> > > If the video track has been connected to a video source (maybe a webcam),
> what
> > > will happen?
> > 
> > I've not decided yet. Open to suggestion. Either the new source overrides
the
> > original source or just return an error. A track can only have one source at
a
> > time.
> 
> IMHO, supporting changing video source in a track is not a good idea. Not sure
> if blink::WebMediaStreamTrack supports it. It may have some unexpected
problems.
> I remember MediaStream supports adding and removing tracks. So if we want to
> change a video source, we can always remove old track which contains the video
> source from MediaStream and add a new track with the new video source.
> 
> 
> BTW, I still don't know how to create an empty WebMediaStreamTrack which can
be
> used here and be passed to JS. If this class could have a helper function
which
> can create a WebMediaStreamTrack from a MediaStreamVideoSource, it will be
> great. Thanks.

Hmm, maybe creating a WebMediaStreamTrack fits better in
MediaStreamDependencyFactory. But regardless I will add one.

[Ronghua Wu (Left Chromium), 2014-01-10 06:25:33]

Updated the cl after discussing with Per. See the updated cl description for
detail.

Also added a helper function to create an empty video track per Peng's
suggestion. With current cl, the implementation of VideoTrackProducerHost can be
something like:

class VideoTrackProducerHost : public MediaStreamVideoSource {
  ...
  blink::WebMediaStreamTrack processed_track =
factory->CreateMediaStreamVideoTrack("123");
  AddTrack(processed_track, constrains);
  ...
  // return |processed_track| to Javascript
  ...
  // Deliver processed frame.
  DeliverVideoFrame(frame);
  …
}

[perkj_chrome, 2014-01-10 13:06:59]

Hej- I took some of your ideas from this an encapsulated them in
https://codereview.chromium.org/131763002/.

My I suggest you base this cl ontop of that instead and create a new
implementation of a MediaStreamVideoSource what suites your needs. (Implement
DeliverVideoFrame in MediaStreamVideoSource).

My cl works on apprtc and peerconnection manual but I have not updated all unit
tests yet and I have not cleaned it up and implemented it complete.

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

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:618:
MediaStreamDependencyFactory::CreateMediaStreamVideoTrack(
I am not sure this belong here. The dependency factory was created originally to
wrap instansiating the webrtc classes.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:629:
MediaStreamDependencyFactory::CreateVideoSource(
Make sure this method is used everywhere in this class to remove code
duplication.

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

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:25: if (!factory_) {
You might want to use dependency injection of the factory in order to be able to
write a unit test that use the mock factory.

Please also see my CL MediaStreamVideoCaptureDeviceSource. I suggest you create
a new source that inherits from MediaStreamVideoSource and have Init on that
method. 

The only difference between the two implementations at the moment would be that
you create a webrtc::RemoteVideoCapturer instead of a RTCVideoCapturer.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:43:
blink::WebMediaStreamSource source = track.source();
This seem wrong. The source already has extra data and just because you add a
new track you can not create an extra data field.

Actually - if you look at my cl https://codereview.chromium.org/131763002/- you
will see that you don't need to implement this at all at the moment. 
Just continue to let the webrtc track add itself to the webrtc video source
interface.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: // TODO(ronghuawu): What
can be done here? Do we need RemoveTrack or how?
This means that a track is being destroyed by webkit and that the source should
no longer provide data to it.

Since we still use the webrtc way and you don't store a reference from the
source to the track- you can just do nothing at the time.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:81: // TODO(ronghuawu):
convert from media::VideoFrame to cricket::VideoFrame.
Take a look at RTCVideoCapture to see how we convert at the moment.

[Ronghua Wu (Left Chromium), 2014-01-11 01:22:58]

Regarding to your comment "create a new implementation of a
MediaStreamVideoSource what suites your needs". It's actually not much to do for
MediaStreamVideoSourceNaCl as the frame provider part is not ready yet. If I do
it it will simply be an empty class just like the one I have in the unit test -
MediaStreamVideoSourceTest.

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

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:618:
MediaStreamDependencyFactory::CreateMediaStreamVideoTrack(
On 2014/01/10 13:07:00, perkj wrote:
> I am not sure this belong here. The dependency factory was created originally
to
> wrap instansiating the webrtc classes. 

Not sure where else should I put this though. The comment for the dependency
factory does say "Object factory for RTC MediaStreams and RTC PeerConnections.".

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:629:
MediaStreamDependencyFactory::CreateVideoSource(
On 2014/01/10 13:07:00, perkj wrote:
> Make sure this method is used everywhere in this class to remove code
> duplication.

Done.

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

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:25: if (!factory_) {
On 2014/01/10 13:07:00, perkj wrote:
> You might want to use dependency injection of the factory in order to be able
to
> write a unit test that use the mock factory.
> 
> Please also see my CL MediaStreamVideoCaptureDeviceSource. I suggest you
create
> a new source that inherits from MediaStreamVideoSource and have Init on that
> method. 
> 
> The only difference between the two implementations at the moment would be
that
> you create a webrtc::RemoteVideoCapturer instead of a RTCVideoCapturer.

I do have the injection, see the ctor.

Regarding to the RTCVideoCapturer, I had some comments in your cl. Basically I'm
trying to hide the libjingle detail inside the MediaStreamVideoSource. The
derived class of MediaStreamVideoSource should only use DeliverVideoFrame
interface instead of injecting a VideoSourceInterface.

The base class on the other hand, always uses the fake capturer just to fulfill
the need of the webrtcvideoengine, so that chromium code doesn't need to use
cricket::VideoCapturer anywhere else.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:43:
blink::WebMediaStreamSource source = track.source();
On 2014/01/10 13:07:00, perkj wrote:
> This seem wrong. The source already has extra data and just because you add a
> new track you can not create an extra data field.
> 
> Actually - if you look at my cl https://codereview.chromium.org/131763002/-
you
> will see that you don't need to implement this at all at the moment. 
> Just continue to let the webrtc track add itself to the webrtc video source
> interface.

The idea is before a track is add/associated with a source, there should be no
source for the track. For example the track created using the new
MediaStreamDependencyFactory::CreateMediaStreamVideoTrack.

https://codereview.chromium.org/129923002/diff/230001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: // TODO(ronghuawu): What
can be done here? Do we need RemoveTrack or how?
On 2014/01/10 13:07:00, perkj wrote:
> This means that a track is being destroyed by webkit and that the source
should
> no longer provide data to it.
> 
> Since we still use the webrtc way and you don't store a reference from the
> source to the track- you can just do nothing at the time. 

Ack

[Ronghua Wu (Left Chromium), 2014-01-14 01:42:50]

Per, this is now updated based on our discussion and your latest cl.

1) I got your point that we can't get rid of the webrtc::VideoSourceInterface
from MediaStreamVideoSource's public interface for the chrome capturer at this
point, so it's now added so that your MediaStreamVideoCaptureDeviceSource can
use it.

2) I think we agreed that for long term we should remove
webrtc::VideoSourceInterface from MediaStreamVideoSource. So I think the NaCl
video source or any other new sources implementation shouldn't need to know
about webrtc::VideoSourceInterface. So if given a null adapter, the
MediaStreamVideoSource creates a webrtc::VideoSource internally using the
webrtc::RemoteVideoCapturer. Please take a look at
media_stream_video_source_unittest.cc see how I expect NaCl to use
MediaStreamVideoSource.

I think this will meet the need for both NaCl and existing capturer case. And
it's now ready for reviewing. PTAL.

[perkj_chrome, 2014-01-14 08:29:09]

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:9: #include
"content/renderer/media/media_stream_source_extra_data.h"
media_stream_source_extra_data.h is not needed here. Its needed in the header
file.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:11: #include
"third_party/WebKit/public/platform/WebMediaStreamSource.h"
unused?

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:25: if (!factory_) {
I would prefer to always incject the factory for now.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:35: bool
MediaStreamVideoSource::AddTrack(
why bool ?

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:80: 1, 1, elapsed_time,
time_stamp, 0);
nit, fits on line above.

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:31: 
remove empty line

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:34:
webrtc::VideoSourceInterface* adapter);
If you look at my cl you will see that I can not create the adapter at once. I
will have to set it once the constraints are applied.
But you can have a protected Init function that create the adapter unless it has
been set already. Please see suggestion in your test code.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:38: virtual bool
AddTrack(const blink::WebMediaStreamTrack& track,
Please keep void for now.

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:18: :
MediaStreamVideoSource(factory, NULL) {
Instead of NULL here and since I can not pass in the webrtc::sourceinterface at
creation. How about having a protected Init function on MediaStreamVideoSource
that create the adapter unless it has already been set. 
So MediaStreamVideoDummySource::MediaStreamVideoDummySource
: MediaStreamSource(factory) {
  Init()
}

[Ronghua Wu (Left Chromium), 2014-01-14 19:42:10]

Thanks Per! All done.

Peng, please have a look also especially the
media_stream_video_source_unittest.cc where you can get a sense how
VideoTrackProducer will use the MediaStreamVideoSource.

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:9: #include
"content/renderer/media/media_stream_source_extra_data.h"
On 2014/01/14 08:29:10, perkj wrote:
> media_stream_source_extra_data.h is not needed here. Its needed in the header
> file.

Done.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:11: #include
"third_party/WebKit/public/platform/WebMediaStreamSource.h"
On 2014/01/14 08:29:10, perkj wrote:
> unused?

Done.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:25: if (!factory_) {
On 2014/01/14 08:29:10, perkj wrote:
> I would prefer to always incject the factory for now.

Done.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:35: bool
MediaStreamVideoSource::AddTrack(
On 2014/01/14 08:29:10, perkj wrote:
> why bool ?

Done.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:80: 1, 1, elapsed_time,
time_stamp, 0);
On 2014/01/14 08:29:10, perkj wrote:
> nit, fits on line above.

It doesn't actually.

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:31: 
On 2014/01/14 08:29:10, perkj wrote:
> remove empty line

Done.

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:38: virtual bool
AddTrack(const blink::WebMediaStreamTrack& track,
On 2014/01/14 08:29:10, perkj wrote:
> Please keep void for now. 

Done.

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

https://codereview.chromium.org/129923002/diff/350001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:18: :
MediaStreamVideoSource(factory, NULL) {
On 2014/01/14 08:29:10, perkj wrote:
> Instead of NULL here and since I can not pass in the webrtc::sourceinterface
at
> creation. How about having a protected Init function on MediaStreamVideoSource
> that create the adapter unless it has already been set. 
> So MediaStreamVideoDummySource::MediaStreamVideoDummySource
> : MediaStreamSource(factory) {
>   Init()
> }

Done.

[perkj_chrome, 2014-01-14 20:12:24]

lgtm with the following nits.

also - the destructor of the test source need to be virtual to compile...

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

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:47: void
SetAdapter(webrtc::VideoSourceInterface* adapter) {
nit: Can be  protected.

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:431:
cricket::VideoCapturer* capturer,
indentation

[Ronghua Wu (Left Chromium), 2014-01-14 22:52:43]

Thanks Per.

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

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:47: void
SetAdapter(webrtc::VideoSourceInterface* adapter) {
On 2014/01/14 20:12:25, perkj wrote:
> nit: Can be  protected.

Done, I was just trying to put those 2 together.

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/129923002/diff/550001/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:431:
cricket::VideoCapturer* capturer,
On 2014/01/14 20:12:25, perkj wrote:
> indentation

Done.

[Ronghua Wu (Left Chromium), 2014-01-15 00:03:02]

+Shijing for reviewing and owner approval. We will need something similar for
audio as well.

[perkj_chrome, 2014-01-15 15:49:36]

On 2014/01/15 00:03:02, Ronghua Wu wrote:
> +Shijing for reviewing and owner approval. We will need something similar for
> audio as well.

Joi - do you want to take a look? I will try to rebase my cl ontop of this once
its landed.

Ronghua - can you refer to bug 334241.

[no longer working on chromium, 2014-01-15 15:57:22]

I am not familiar with the video code, so I only did some sanity check on the
style.

True, I am going to do similar things for audio soon.

SX

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:618: // The video
source takes ownership of |capturer|.
nit, can you move this comment to the interface instead?

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:34: void
MediaStreamVideoSource::Init() {
how this Init() should be used? can it be called more than once?

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:64: // TODO(ronghuawu): we
assume contiguous layout of image planes and
Is this a comment or TODO?

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66: const int64 elapsed_time
=
nit, s/elapsed_time/elapsed_time_ns/g
the same for time_stamp

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:31: MediaStreamVideoSource(
explicit MediaStreamVideoSource(MediaStreamDependencyFactory* factory);

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:57: virtual void Init();
why this is virtual?

[perkj_chrome, 2014-01-15 16:24:53]

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:619:
scoped_refptr<webrtc::VideoSourceInterface> source =
I just realized that you probably have to call EnsurePeerConnectionFactory here
since this might be the first thing done using pc_factory_.

[Jói, 2014-01-15 16:37:04]

I have very little to add, Per is the expert here. LGTM once Per is happy and
with my nit below addressed.

Cheers,
Jói

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:15: class
MediaStreamVideoDummySource : public MediaStreamVideoSource {
Suggest naming MockMediaStreamVideoSource or DummyMediaStreamVideoSource.

[Ronghua Wu (Left Chromium), 2014-01-15 18:38:58]

Thanks for comments. PTAL.

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:618: // The video
source takes ownership of |capturer|.
On 2014/01/15 15:57:22, xians1 wrote:
> nit, can you move this comment to the interface instead?

Done.

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:619:
scoped_refptr<webrtc::VideoSourceInterface> source =
On 2014/01/15 16:24:54, perkj wrote:
> I just realized that you probably have to call EnsurePeerConnectionFactory
here
> since this might be the first thing done using pc_factory_.
>  

I think it's the callers's responsibility to do that. Otherwise we will need to
do EnsurePeerConnectionFactory on all the calls.

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:34: void
MediaStreamVideoSource::Init() {
On 2014/01/15 15:57:22, xians1 wrote:
> how this Init() should be used? can it be called more than once?

Added comments in header file.

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:64: // TODO(ronghuawu): we
assume contiguous layout of image planes and
On 2014/01/15 15:57:22, xians1 wrote:
> Is this a comment or TODO?

Moved to header as a note.

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66: const int64 elapsed_time
=
On 2014/01/15 15:57:22, xians1 wrote:
> nit, s/elapsed_time/elapsed_time_ns/g
> the same for time_stamp

Done.

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:31: MediaStreamVideoSource(
On 2014/01/15 15:57:22, xians1 wrote:
> explicit MediaStreamVideoSource(MediaStreamDependencyFactory* factory);

Done.

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:57: virtual void Init();
On 2014/01/15 15:57:22, xians1 wrote:
> why this is virtual?

Removed.

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

https://codereview.chromium.org/129923002/diff/740001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:15: class
MediaStreamVideoDummySource : public MediaStreamVideoSource {
On 2014/01/15 16:37:05, Jói wrote:
> Suggest naming MockMediaStreamVideoSource or DummyMediaStreamVideoSource.

Was trying to follow Per's naming convention in his cl. :) Changed to
DummyMediaStreamVideoSource.

[no longer working on chromium, 2014-01-15 18:41:50]

lgtm

[commit-bot: I haz the power, 2014-01-15 18:53:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/129923002/1020001

[commit-bot: I haz the power, 2014-01-15 22:23:30]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Ronghua Wu (Left Chromium), 2014-01-15 22:35:47]

Implements MediaStreamVideoSource. 

1) Add MediaStreamDependencyFactory::CreateVideoSource to create a
webrtc::VideoSourceInterface that's associated with given capturer. 
2) Implement MediaStreamVideoSource: 
* Creates a webrtc::VideoSource instance with a fake cricket::VideoCapturer. Use
this webrtc::VideoSource as the adapter to talk to the libjingle world. 
* When AddTrack is called, creates native track for the given blink track. And
connect the native track with the webrtc::VideoSource so that the track can get
frames from this source. 
* When DeliverVideoFrame is called, feeds the frame to all the registered tracks
via the adapter (webrtc::VideoSource)'s FrameInput interface.

[Ronghua Wu (Left Chromium), 2014-01-15 22:37:51]

Rebased and will commit.

[Ronghua Wu (Left Chromium), 2014-01-15 22:47:27]

Committed patchset #17 manually as r245001 (presubmit successful).