Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal thread, VideoBroadCaster must be made thread safe.

BUG=webrtc:5426
R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/f0dcfe2c8143f44f07526a919d15bcf4858c6dd5
Cr-Commit-Position: refs/heads/master@{#11944}

[perkj_webrtc, 2016-03-09 02:22:48]

Description was changed from

==========
Implement a remote video source.

BUG=
==========

to

==========
Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and
RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal
thread, VideoBroadCaster must be made thread safe. 

BUG=webrtc:5426
==========

[perkj_webrtc, 2016-03-09 02:24:32]

perkj@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[perkj_webrtc, 2016-03-09 02:24:33]

Can you please review?

[pthatcher1, 2016-03-09 18:16:48]

pthatcher@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[pthatcher1, 2016-03-09 18:16:49]

Taylor, can you review this as well?

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (left):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1707: DestroyVideoReceiver(stream, video_track);
Can you put a comment here saying "Stopping or destroying a VideoRtpReceiver
will end VideoRtpReceiver::track()"?

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1323: return video_receiver;
Why do we keep ownership of the proxy but then return the non-proxy?  That seems
weird.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:172: uint32_t ssrc);
Can you add a comment explaining that this is the value that should be in
VideoRtpReceiver.track().id(), and that we need to pass it into the RtpReceiver
because PeerConnection allows this to be set via SetRemoteDescription.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:89: rtc::VideoBroadcaster broadcaster_;
Can you add a comment explaining that this is needed because decoder's can't
handle more than one sink, and that someday later, when decoder's can, we won't
need this?

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/videotracksour...
File webrtc/api/videotracksource.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/videotracksour...
webrtc/api/videotracksource.h:46: cricket::VideoCapturer* GetVideoCapturer()
override { return nullptr; }
Why do we still need this?

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:25: // VideoBroadcaster broadcast video
frames to sinks and combine
combines

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:26: // VideoSinkWants from its sinks. It do
that by implementing the
do that => does that

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:26: // VideoSinkWants from its sinks. It do
that by implementing the
implementing the => implementing

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:29: // jVideo frames can be broadcasted on
any thread. I.e VideoBroadcaster::OnFrame
jVideo => video

[perkj_webrtc, 2016-03-09 21:22:59]

PTAL

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (left):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1707: DestroyVideoReceiver(stream, video_track);
On 2016/03/09 18:16:49, pthatcher1 wrote:
> Can you put a comment here saying "Stopping or destroying a VideoRtpReceiver
> will end VideoRtpReceiver::track()"?

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1323: return video_receiver;
On 2016/03/09 18:16:49, pthatcher1 wrote:
> Why do we keep ownership of the proxy but then return the non-proxy?  That
seems
> weird.

That is my attempt to avoid a static cast and to be able to get a
videotrackinterface and not a mediastreamtrack from the VideoRtpReceiver. I can
have this method return a VideoRtpReceiverInteface and return the proxy if I add
a new method to the interface. Let me try that and see if  you like it better.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:89: rtc::VideoBroadcaster broadcaster_;
On 2016/03/09 18:16:49, pthatcher1 wrote:
> Can you add a comment explaining that this is needed because decoder's can't
> handle more than one sink, and that someday later, when decoder's can, we
won't
> need this?

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/videotracksour...
File webrtc/api/videotracksource.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/videotracksour...
webrtc/api/videotracksource.h:46: cricket::VideoCapturer* GetVideoCapturer()
override { return nullptr; }
On 2016/03/09 18:16:49, pthatcher1 wrote:
> Why do we still need this?

It is part of the public interface until nisse can land
https://codereview.webrtc.org/1766653002/

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster.h (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:25: // VideoBroadcaster broadcast video
frames to sinks and combine
On 2016/03/09 18:16:49, pthatcher1 wrote:
> combines

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:26: // VideoSinkWants from its sinks. It do
that by implementing the
On 2016/03/09 18:16:49, pthatcher1 wrote:
> do that => does that

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:26: // VideoSinkWants from its sinks. It do
that by implementing the
On 2016/03/09 18:16:49, pthatcher1 wrote:
> do that => does that

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:26: // VideoSinkWants from its sinks. It do
that by implementing the
On 2016/03/09 18:16:49, pthatcher1 wrote:
> implementing the => implementing

Done.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster.h:29: // jVideo frames can be broadcasted on
any thread. I.e VideoBroadcaster::OnFrame
On 2016/03/09 18:16:49, pthatcher1 wrote:
> jVideo => video

Done.

[pthatcher1, 2016-03-09 21:29:04]

lgtm

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:172: // RtpReceierInterface. Returns the
VideoRtpReceiver, not the proxy.
RtpReceiverInterface

[Taylor Brandstetter, 2016-03-09 22:10:41]

lgtm; I'm very happy about this change. :)

Just a couple minor comments.

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1323: return video_receiver;
On 2016/03/09 21:22:59, perkj_webrtc wrote:
> On 2016/03/09 18:16:49, pthatcher1 wrote:
> > Why do we keep ownership of the proxy but then return the non-proxy?  That
> seems
> > weird.
> 
> That is my attempt to avoid a static cast and to be able to get a
> videotrackinterface and not a mediastreamtrack from the VideoRtpReceiver. I
can
> have this method return a VideoRtpReceiverInteface and return the proxy if I
add
> a new method to the interface. Let me try that and see if  you like it better.

I assume you mean "return an RtpReceiverInterface"? I'm actually planning on
cleaning up that interface so it doesn't have any methods besides those in the
spec. So I'd prefer the current implementation. Or better yet, pass a stream
into CreateVideoReceiver and add the track to the stream within the method, to
avoid the need for returning anything.

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
File webrtc/api/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
webrtc/api/rtpsenderreceiver_unittest.cc:156: video_track_ =
video_rtp_receiver_->video_track();
Just for consistency, this should probably add the video track to the stream_.

[perkj_webrtc, 2016-03-10 00:25:28]

ptal?

[perkj_webrtc, 2016-03-10 00:26:38]

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1323: return video_receiver;
On 2016/03/09 22:10:41, Taylor Brandstetter wrote:
> On 2016/03/09 21:22:59, perkj_webrtc wrote:
> > On 2016/03/09 18:16:49, pthatcher1 wrote:
> > > Why do we keep ownership of the proxy but then return the non-proxy?  That
> > seems
> > > weird.
> > 
> > That is my attempt to avoid a static cast and to be able to get a
> > videotrackinterface and not a mediastreamtrack from the VideoRtpReceiver. I
> can
> > have this method return a VideoRtpReceiverInteface and return the proxy if I
> add
> > a new method to the interface. Let me try that and see if  you like it
better.
> 
> I assume you mean "return an RtpReceiverInterface"? I'm actually planning on
> cleaning up that interface so it doesn't have any methods besides those in the
> spec. So I'd prefer the current implementation. Or better yet, pass a stream
> into CreateVideoReceiver and add the track to the stream within the method, to
> avoid the need for returning anything.

that is a very good idea.

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
File webrtc/api/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
webrtc/api/rtpsenderreceiver_unittest.cc:156: video_track_ =
video_rtp_receiver_->video_track();
On 2016/03/09 22:10:41, Taylor Brandstetter wrote:
> Just for consistency, this should probably add the video track to the stream_.

Done.

[Taylor Brandstetter, 2016-03-10 00:43:17]

On 2016/03/10 00:26:38, perkj_webrtc wrote:
>
https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection.cc
> File webrtc/api/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1765423005/diff/40001/webrtc/api/peerconnection...
> webrtc/api/peerconnection.cc:1323: return video_receiver;
> On 2016/03/09 22:10:41, Taylor Brandstetter wrote:
> > On 2016/03/09 21:22:59, perkj_webrtc wrote:
> > > On 2016/03/09 18:16:49, pthatcher1 wrote:
> > > > Why do we keep ownership of the proxy but then return the non-proxy? 
That
> > > seems
> > > > weird.
> > > 
> > > That is my attempt to avoid a static cast and to be able to get a
> > > videotrackinterface and not a mediastreamtrack from the VideoRtpReceiver.
I
> > can
> > > have this method return a VideoRtpReceiverInteface and return the proxy if
I
> > add
> > > a new method to the interface. Let me try that and see if  you like it
> better.
> > 
> > I assume you mean "return an RtpReceiverInterface"? I'm actually planning on
> > cleaning up that interface so it doesn't have any methods besides those in
the
> > spec. So I'd prefer the current implementation. Or better yet, pass a stream
> > into CreateVideoReceiver and add the track to the stream within the method,
to
> > avoid the need for returning anything.
> 
> that is a very good idea.
> 
>
https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
> File webrtc/api/rtpsenderreceiver_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1765423005/diff/60001/webrtc/api/rtpsenderrecei...
> webrtc/api/rtpsenderreceiver_unittest.cc:156: video_track_ =
> video_rtp_receiver_->video_track();
> On 2016/03/09 22:10:41, Taylor Brandstetter wrote:
> > Just for consistency, this should probably add the video track to the
stream_.
> 
> Done.

lgtm.

[perkj_webrtc, 2016-03-10 00:52:13]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-03-10 00:52:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1765423005/#ps100001 (title:
"Rebased")

[commit-bot: I haz the power, 2016-03-10 00:52:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765423005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765423005/100001

[commit-bot: I haz the power, 2016-03-10 01:37:11]

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

[commit-bot: I haz the power, 2016-03-10 01:37:12]

Try jobs failed on following builders:
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/10498)

[perkj_webrtc, 2016-03-10 13:58:04]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2016-03-10 13:58:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765423005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765423005/100001

[commit-bot: I haz the power, 2016-03-10 14:39:30]

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

[commit-bot: I haz the power, 2016-03-10 14:39:31]

Try jobs failed on following builders:
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/10515)

[perkj_webrtc, 2016-03-10 17:00:39]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-03-10 17:00:39]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1765423005/#ps140001 (title:
"Fixed up the comments.")

[commit-bot: I haz the power, 2016-03-10 17:00:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765423005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765423005/140001

[perkj_webrtc, 2016-03-10 17:32:12]

Description was changed from

==========
Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and
RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal
thread, VideoBroadCaster must be made thread safe. 

BUG=webrtc:5426
==========

to

==========
Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and
RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal
thread, VideoBroadCaster must be made thread safe.

BUG=webrtc:5426
R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f0dcfe2c8143f44f07526a919...
==========

[commit-bot: I haz the power, 2016-03-10 17:32:14]

Description was changed from

==========
Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and
RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal
thread, VideoBroadCaster must be made thread safe.

BUG=webrtc:5426
R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f0dcfe2c8143f44f07526a919...
==========

to

==========
Change VideoRtpReceiver to create remote VideoTrack and VideoTrackSource.

This enabled us to be able to remove VideoTrack::GetSink and
RemoteVideoCapturer.

Since video frames from the decoder is delivered on a media engine internal
thread, VideoBroadCaster must be made thread safe.

BUG=webrtc:5426
R=deadbeef@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/f0dcfe2c8143f44f07526a919d15bcf4858c6dd5
Cr-Commit-Position: refs/heads/master@{#11944}
==========

[perkj_webrtc, 2016-03-10 17:32:14]

Committed patchset #8 (id:140001) manually as
f0dcfe2c8143f44f07526a919d15bcf4858c6dd5 (presubmit successful).

[commit-bot: I haz the power, 2016-03-10 17:32:15]

Patchset 8 (id:??) landed as
https://crrev.com/f0dcfe2c8143f44f07526a919d15bcf4858c6dd5
Cr-Commit-Position: refs/heads/master@{#11944}