Issue 155853002: Chrome MediaStream VideoTrack implementation.

Issue 155853002: Chrome MediaStream VideoTrack implementation. (Closed)

Created:
6 years, 10 months ago by perkj_chrome

Modified:
6 years, 9 months ago

Reviewers:
Ronghua Wu (Left Chromium), no longer working on chromium, miu, Jói, Alpha Left Google

CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

MediaStream VideoTrack implementation. This implementation only use Chrome video classes for local rendering. A libjingle representation is created once its connected to a PeerConnection. BUG=334241 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255846

Patch Set 1 #

Patch Set 2 : Rebased #

Patch Set 3 : Added unittests for video track and some cleanup. #

Patch Set 4 : More cleanup. #

Total comments: 56

Patch Set 5 : Addressed code review comments. #

Patch Set 6 : Fixed a comment. #

Total comments: 18

Patch Set 7 : Fixed compile and addressed code review. #

Patch Set 8 : Use YV12 in track sink instead. #

Patch Set 9 : rebased #

Patch Set 10 : Removed whitespaces... #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+474 lines, -101 lines)			Patch
M	content/content_tests.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	content/renderer/media/media_stream_audio_source.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -4 lines	0 comments	Download
M	content/renderer/media/media_stream_audio_source.cc	View	1	1 chunk	+0 lines, -5 lines	0 comments	Download
M	content/renderer/media/media_stream_center.cc	View	1 2 3 4 5 6	2 chunks	+9 lines, -5 lines	0 comments	Download
M	content/renderer/media/media_stream_dependency_factory.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	content/renderer/media/media_stream_dependency_factory_unittest.cc	View	1 2 3 4	1 chunk	+8 lines, -4 lines	0 comments	Download
M	content/renderer/media/media_stream_impl.h	View	1 2 3 4	1 chunk	+7 lines, -2 lines	0 comments	Download
M	content/renderer/media/media_stream_impl.cc	View	1 2 3 4 5 6	4 chunks	+26 lines, -8 lines	0 comments	Download
M	content/renderer/media/media_stream_source.h	View	1 2 3 4	1 chunk	+0 lines, -5 lines	0 comments	Download
M	content/renderer/media/media_stream_track.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	content/renderer/media/media_stream_video_source.h	View	1 2 3 4 5 6 7 8	3 chunks	+12 lines, -4 lines	0 comments	Download
M	content/renderer/media/media_stream_video_source.cc	View	1 2 3 4 5 6 7 8 9	9 chunks	+33 lines, -9 lines	0 comments	Download
M	content/renderer/media/media_stream_video_source_unittest.cc	View	1 2 3 4 5 6 7 8	7 chunks	+16 lines, -20 lines	0 comments	Download
M	content/renderer/media/media_stream_video_track.h	View	1 2 3 4 5 6	3 chunks	+66 lines, -8 lines	0 comments	Download
M	content/renderer/media/media_stream_video_track.cc	View	1 2 3 4 5 6 7	2 chunks	+137 lines, -21 lines	0 comments	Download
A	content/renderer/media/media_stream_video_track_unittest.cc	View	1 2 3 4 5 6	1 chunk	+144 lines, -0 lines	0 comments	Download
M	content/renderer/media/rtc_peer_connection_handler_unittest.cc	View	1 2 3 4	2 chunks	+10 lines, -5 lines	0 comments	Download

Messages

Total messages: 25 (0 generated)

Expand Messages | Collapse Messages

perkj_chrome

Fyi - this runs with Chrome only local rendering and tab capture. I just want ...

6 years, 10 months ago (2014-02-05 15:33:26 UTC) #1

Alpha Left Google

First pass of comments. This is looking good. https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_center.cc File content/renderer/media/media_stream_center.cc (right): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_center.cc#newcode51 content/renderer/media/media_stream_center.cc:51: MediaStreamVideoSource* ...

6 years, 9 months ago (2014-02-28 02:49:22 UTC) #3

Jói

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_dependency_factory_unittest.cc File content/renderer/media/media_stream_dependency_factory_unittest.cc (right): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_dependency_factory_unittest.cc#newcode112 content/renderer/media/media_stream_dependency_factory_unittest.cc:112: native_source, nit: this and next parameter would fit on ...

6 years, 9 months ago (2014-02-28 14:52:50 UTC) #4

Jói

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/rtc_video_renderer.cc File content/renderer/media/rtc_video_renderer.cc (right): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/rtc_video_renderer.cc#newcode113 content/renderer/media/rtc_video_renderer.cc:113: base::Bind(&ReleaseOriginalFrame, frame)); On 2014/02/28 14:52:50, Jói wrote: > You've ...

6 years, 9 months ago (2014-02-28 15:46:10 UTC) #5

Ronghua Wu (Left Chromium)

First pass, will do more review next week. https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_audio_source.h File content/renderer/media/media_stream_audio_source.h (left): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_audio_source.h#oldcode30 content/renderer/media/media_stream_audio_source.h:30: virtual ...

6 years, 9 months ago (2014-03-01 01:26:03 UTC) #6

perkj_chrome

PTAL https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_audio_source.h File content/renderer/media/media_stream_audio_source.h (left): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_audio_source.h#oldcode30 content/renderer/media/media_stream_audio_source.h:30: virtual void RemoveTrack(const blink::WebMediaStreamTrack& track) OVERRIDE; On 2014/03/01 ...

6 years, 9 months ago (2014-03-02 09:20:44 UTC) #7

PTAL

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_source.h (left):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_audio_source.h:30: virtual void
RemoveTrack(const blink::WebMediaStreamTrack& track) OVERRIDE;
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> prefer to do this in separated cl.
Since I remove this from the base class I need to do it for this subclass as
well. Also- this is not implemented and used yet so its easiest to do now and
let audio use whatever track interface it wants.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:51: MediaStreamVideoSource*
video_source =
On 2014/02/28 02:49:22, Alpha wrote:
> The set of APIs used in the method does make me confused.
> 
> I think first this should be called |native_source|.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_center.cc:51: MediaStreamVideoSource*
video_source =
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> On 2014/02/28 02:49:22, Alpha wrote:
> > The set of APIs used in the method does make me confused.
> > 
> > I think first this should be called |native_source|.
> 
> +1

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:731:
track.setExtraData(new WebRtcMediaStreamVideoTrack(
On 2014/02/28 02:49:22, Alpha wrote:
> Suggestion: this method should be renamed to setNativeTrack. Probably worth
> doing as a cleanup.

Well - setExtraData is a blink method. And the base class is a
blink::WebMediaStreamTrack::ExtraData. 
In blink we also have blink::WebMediaStreamSource::ExtraData and
blink::WebMediaStream::ExtraData that all work the same way. 

All in all, I can't change it and I don't think its worth getting help to rename
them and all locations in Chrome that call them.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory_unittest.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory_unittest.cc:112:
native_source,
On 2014/02/28 14:52:50, Jói wrote:
> nit: this and next parameter would fit on previous line

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory_unittest.cc:120: 
On 2014/02/28 02:49:22, Alpha wrote:
> nit: remove these empty lines.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:35: // If a subclass override this
method it has to call the base class.
On 2014/02/28 14:52:50, Jói wrote:
> override this -> overrides this

Done.

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

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:325: for
(std::vector<MediaStreamVideoTrack*>::iterator it = tracks_.begin();
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> should we do this if capture_adapter_ is not null? Will a track gets 2 copies
of
> frames?

No- capture_adapter_ provides frames to the webrtc::MediaStreamVideoSource. It
has nothing to do with chromes MediaStreamVideoTracks.

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

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:53: static
MediaStreamVideoSource* GetVideoSource(
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> Put comments.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:57: virtual void
AddTrack(const blink::WebMediaStreamTrack& track,
I can not call AddTrack in the ctor. gUM needs to know the result of AddTrack.
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> On 2014/02/28 02:49:22, Alpha wrote:
> > This is odd. I think it should just take the native track instead of a web
> > track. The implementation of this method doesn't actually need a
> > WebMediaStreamTrack. Also it is more consistent to match RemoveTrack()
below.
> 
> Make sense.
> 
> And I saw you called RemoveTrack from the ~MediaStreamVideoTrack, I think it
> should be possible to call AddTrack in MediaStreamVideoTrack ctor.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:57: virtual void
AddTrack(const blink::WebMediaStreamTrack& track,
On 2014/02/28 02:49:22, Alpha wrote:
> This is odd. I think it should just take the native track instead of a web
> track. The implementation of this method doesn't actually need a
> WebMediaStreamTrack. Also it is more consistent to match RemoveTrack() below.
ok - but then I need to change the base class and have separate AddTrack for
audio and video. But it probably make sence.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:26: }
On 2014/02/28 02:49:22, Alpha wrote:
> I'd actually call source_->AddTrack(this) here.
> 
> It looks odd that the destructor of this class calls RemoveTrack() but rely on
> an external body to add the track.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:30: if (source_)
On 2014/02/28 02:49:22, Alpha wrote:
> There must be a source isn't it?

Well- Admitingly WebRtcMediaStreamVideoTrack is a temporary solution for remote
video tracks. They have source = NULL.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:48:
webrtc::VideoTrackInterface* MediaStreamVideoTrack::GetVideoAdapter() {
On 2014/02/28 02:49:22, Alpha wrote:
> Should this method be moved to WebRtcMediaStreamVideoTrack?
> 
> It looks like this is the last dependency on libjingle and should be moved.

No, we still need to be able to create a libjingle track when a track is
connected to a PeerConnection. We can consider, when everything else is done, to
create and own |tracks_| outside the actual chrome track implementation.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:104:
WebRtcMediaStreamVideoTrack::WebRtcMediaStreamVideoTrack(
On 2014/02/28 02:49:22, Alpha wrote:
> This should be moved to a different file.

This should all be replaced with a remote MediaStreamVideoSource implementation.
Either me or Joi will do that soon. Are you ok with keeping this here until that
is done?

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.h:70: // TODO(perkj): Replace
RemoteMediaStreamVideoTrack with a remote video source
On 2014/02/28 14:52:50, Jói wrote:
> RemoteVideoStreamTrack -> WebRtcMediaStreamVideoTrack (?)

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.h:72: class CONTENT_EXPORT
WebRtcMediaStreamVideoTrack
On 2014/03/01 01:26:04, Ronghua Wu wrote:
> comments for this class.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track_unittest.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track_unittest.cc:35: int
number_of_frames() const { return number_of_frames_;}
On 2014/02/28 14:52:50, Jói wrote:
> space after ;

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track_unittest.cc:36: bool enabled()
const { return enabled_;}
On 2014/02/28 14:52:50, Jói wrote:
> space after ;

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track_unittest.cc:37:
blink::WebMediaStreamSource::ReadyState state() const {return state_;}
On 2014/02/28 14:52:50, Jói wrote:
> space after { and before }

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_renderer.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_renderer.cc:14: // Reviewers ? Do you know a
better way than to add this noop method?
On 2014/02/28 14:52:50, Jói wrote:
> You can use base::DoNothing (which is void DoNothing(void))

After discussion- this is needed to addref and release the original video frame.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_renderer.cc:113:
base::Bind(&ReleaseOriginalFrame, frame));
On 2014/02/28 15:46:10, Jói wrote:
> On 2014/02/28 14:52:50, Jói wrote:
> > You've bound both parameters so this wouldn't work with the do-nothing
> > ReleaseOriginalFrame function above. However, if you just use
base::DoNothing
> > instead, this would work fine.
> 
> My mistake, I was assuming the WrapExternalYuvData function expected a
callback
> with a single parameter, but it actually wants a closure (i.e. no parameters).
> So binding it the way you're doing is the right thing to do.

Done.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_renderer.cc:116: repaint_cb_.Run(frame);
On 2014/02/28 02:49:22, Alpha wrote:
> nit: What about:
> 
> if (frame->format() != media::VideoFrame::I420) {
>   repaint_cb.Run(framee);
>   return;
> }
> 
> // And then rest of the media::VideoFrame::WrapExternalYuvData code here.

Done.

no longer working on chromium

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/media_stream_audio_source.h File content/renderer/media/media_stream_audio_source.h (right): https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/media_stream_audio_source.h#newcode27 content/renderer/media/media_stream_audio_source.h:27: virtual void AddTrack(const blink::WebMediaStreamTrack& track, can we remove virtual ...

6 years, 9 months ago (2014-03-02 10:57:35 UTC) #8

Alpha Left Google

LGTM after some TODOs and comments. https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_dependency_factory.cc File content/renderer/media/media_stream_dependency_factory.cc (right): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_dependency_factory.cc#newcode731 content/renderer/media/media_stream_dependency_factory.cc:731: track.setExtraData(new WebRtcMediaStreamVideoTrack( On ...

6 years, 9 months ago (2014-03-03 06:45:03 UTC) #9

LGTM after some TODOs and comments.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:731:
track.setExtraData(new WebRtcMediaStreamVideoTrack(
On 2014/03/02 09:20:44, perkj wrote:
> On 2014/02/28 02:49:22, Alpha wrote:
> > Suggestion: this method should be renamed to setNativeTrack. Probably worth
> > doing as a cleanup.
> 
> Well - setExtraData is a blink method. And the base class is a
> blink::WebMediaStreamTrack::ExtraData. 
> In blink we also have blink::WebMediaStreamSource::ExtraData and
> blink::WebMediaStream::ExtraData that all work the same way. 
> 
> All in all, I can't change it and I don't think its worth getting help to
rename
> them and all locations in Chrome that call them. 

Okay. What I mean is a cleanup for future. :)

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

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:57: virtual void
AddTrack(const blink::WebMediaStreamTrack& track,
On 2014/03/02 09:20:44, perkj wrote:
> I can not call AddTrack in the ctor. gUM needs to know the result of AddTrack.
> On 2014/03/01 01:26:04, Ronghua Wu wrote:
> > On 2014/02/28 02:49:22, Alpha wrote:
> > > This is odd. I think it should just take the native track instead of a web
> > > track. The implementation of this method doesn't actually need a
> > > WebMediaStreamTrack. Also it is more consistent to match RemoveTrack()
> below.
> > 
> > Make sense.
> > 
> > And I saw you called RemoveTrack from the ~MediaStreamVideoTrack, I think it
> > should be possible to call AddTrack in MediaStreamVideoTrack ctor.
> 

Okay.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:48:
webrtc::VideoTrackInterface* MediaStreamVideoTrack::GetVideoAdapter() {
On 2014/03/02 09:20:44, perkj wrote:
> On 2014/02/28 02:49:22, Alpha wrote:
> > Should this method be moved to WebRtcMediaStreamVideoTrack?
> > 
> > It looks like this is the last dependency on libjingle and should be moved.
> 
> No, we still need to be able to create a libjingle track when a track is
> connected to a PeerConnection. We can consider, when everything else is done,
to
> create and own |tracks_| outside the actual chrome track implementation. 

Then I think there should be a TODO here and a pointer to bug (the master bug of
piranha plant is fine). The objective is to isolate the dependency of libjingle
to a single directory.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:104:
WebRtcMediaStreamVideoTrack::WebRtcMediaStreamVideoTrack(
On 2014/03/02 09:20:44, perkj wrote:
> On 2014/02/28 02:49:22, Alpha wrote:
> > This should be moved to a different file.
> 
> This should all be replaced with a remote MediaStreamVideoSource
implementation.
> Either me or Joi will do that soon. Are you ok with keeping this here until
that
> is done? 

Okay. Just want to call this out. A TODO would be fine.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
File content/renderer/media/rtc_video_renderer.cc (right):

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/rtc_video_renderer.cc:105:
scoped_refptr<media::VideoFrame> yuv_frame =
A comment would help to explain what this is doing and why it is necessary.

Ronghua Wu (Left Chromium)

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_video_source.cc File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/media_stream_video_source.cc#newcode325 content/renderer/media/media_stream_video_source.cc:325: for (std::vector<MediaStreamVideoTrack*>::iterator it = tracks_.begin(); On 2014/03/02 09:20:44, perkj ...

6 years, 9 months ago (2014-03-04 01:04:10 UTC) #11

perkj_chrome

6 years, 9 months ago (2014-03-04 10:44:50 UTC) #12

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

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:325: for
(std::vector<MediaStreamVideoTrack*>::iterator it = tracks_.begin();
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> On 2014/03/02 09:20:44, perkj wrote:
> > On 2014/03/01 01:26:04, Ronghua Wu wrote:
> > > should we do this if capture_adapter_ is not null? Will a track gets 2
> copies
> > of
> > > frames?
> > 
> > No- capture_adapter_ provides frames to the webrtc::MediaStreamVideoSource.
It
> > has nothing to do with chromes MediaStreamVideoTracks. 
> 
> Assume you meant webrtc::VideoSource. So you are saying the tracks that are
got
> feeded by webrtc::VideoSource won't be connected to MediaStreamVideoSource? 
> 
> I thought the point is to move stuff from webrtc::VideoSource to
> MediaStreamVideoSource.

Currently they work in parallell. ie video frames are fed:
MediaStreamVideoSource->capture_adapter_->libjingle
MediaStreamVideoSource->MediaSteamVideoTrack->sinks.

In the desig document we have written that we should consider doing
MediaStreamVideoSource->MediaSteamVideoTrack->capture_adapter_->libjingle

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:48:
webrtc::VideoTrackInterface* MediaStreamVideoTrack::GetVideoAdapter() {
On 2014/03/03 06:45:04, Alpha wrote:
> On 2014/03/02 09:20:44, perkj wrote:
> > On 2014/02/28 02:49:22, Alpha wrote:
> > > Should this method be moved to WebRtcMediaStreamVideoTrack?
> > > 
> > > It looks like this is the last dependency on libjingle and should be
moved.
> > 
> > No, we still need to be able to create a libjingle track when a track is
> > connected to a PeerConnection. We can consider, when everything else is
done,
> to
> > create and own |tracks_| outside the actual chrome track implementation. 
> 
> Then I think there should be a TODO here and a pointer to bug (the master bug
of
> piranha plant is fine). The objective is to isolate the dependency of
libjingle
> to a single directory. 

ok- added in the header file.

https://codereview.chromium.org/155853002/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:104:
WebRtcMediaStreamVideoTrack::WebRtcMediaStreamVideoTrack(
On 2014/03/03 06:45:04, Alpha wrote:
> On 2014/03/02 09:20:44, perkj wrote:
> > On 2014/02/28 02:49:22, Alpha wrote:
> > > This should be moved to a different file.
> > 
> > This should all be replaced with a remote MediaStreamVideoSource
> implementation.
> > Either me or Joi will do that soon. Are you ok with keeping this here until
> that
> > is done? 
> 
> Okay. Just want to call this out. A TODO would be fine.
Its in the header file.

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

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:27: virtual void
AddTrack(const blink::WebMediaStreamTrack& track,
On 2014/03/02 10:57:36, xians1 wrote:
> can we remove virtual since it it not overriding an interface any more.

Done.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:29: const
ConstraintsCallback& callback);
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> why remove OVERRIDE but not virtual?

Done.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:59:
MediaStreamVideoTrack::CreateVideoTrack(
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> I'm confused, who gets the newly created blink::WebMediaStreamTrack?
> 
> Or the return value will be the same as |track|? In which case |track| should
> not be const&, i.e |track| is changed.

Good catch - right - this is wrong.... |track| has been created by blink so we
should not create a new one. Simply initialize the chrome version.

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

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:869: return
MediaStreamVideoTrack::CreateVideoTrack(
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> indention

Done.

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

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source_unittest.cc:90: true, NULL);
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> prefer to have meaningful variables for these

Done.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:88: //
WebRtcMediaStreamVideoTrack is a Chrome representation of a video track.
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> MediaStreamVideoTrack is 'a representation in content'. And this one is 'a
> Chrome representation'. I can't tell the different from the comment.

ok- sticking to content.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
File content/renderer/media/rtc_video_renderer.cc (right):

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/rtc_video_renderer.cc:101: repaint_cb_.Run(frame);
On 2014/03/04 01:04:11, Ronghua Wu wrote:
> what formats are expected here?
> 
> It seems YV12 will come here too.

That is correct. We need to special handle I420. See the added comment.

https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/...
content/renderer/media/rtc_video_renderer.cc:105:
scoped_refptr<media::VideoFrame> yuv_frame =
On 2014/03/03 06:45:04, Alpha wrote:
> A comment would help to explain what this is doing and why it is necessary.

Done.

Ronghua Wu (Left Chromium)

LGTM if you fix the pepper test. https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/media_stream_center.cc File content/renderer/media/media_stream_center.cc (right): https://codereview.chromium.org/155853002/diff/120001/content/renderer/media/media_stream_center.cc#newcode59 content/renderer/media/media_stream_center.cc:59: MediaStreamVideoTrack::CreateVideoTrack( On ...

6 years, 9 months ago (2014-03-04 23:50:54 UTC) #13

perkj_chrome

Guys - I changed back to let the sinks receive YV12 video frames if the ...

6 years, 9 months ago (2014-03-05 10:12:25 UTC) #14

Jói

SLGTM On Wed, Mar 5, 2014 at 10:12 AM, <perkj@chromium.org> wrote: > Guys - I ...

6 years, 9 months ago (2014-03-05 10:46:37 UTC) #15

no longer working on chromium

lgtm to the amedia_stream_audio_source.cc :)

6 years, 9 months ago (2014-03-05 20:55:34 UTC) #16

Alpha Left Google

On 2014/03/06 01:14:02, Ronghua Wu wrote: > SLGTM Okay. LGTM.

6 years, 9 months ago (2014-03-06 01:18:41 UTC) #18

commit-bot: I haz the power

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

6 years, 9 months ago (2014-03-09 09:18:52 UTC) #20

commit-bot: I haz the power

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

6 years, 9 months ago (2014-03-09 09:44:55 UTC) #21

commit-bot: I haz the power

Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=54301

6 years, 9 months ago (2014-03-09 09:44:56 UTC) #22

commit-bot: I haz the power

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

6 years, 9 months ago (2014-03-09 10:14:54 UTC) #24

Message was sent while issue was closed.

Change committed as 255846

Expand Messages | Collapse Messages