Delete FrameInput method and FrameInputWrapper class.

Delete FrameInput method and FrameInputWrapper class.

Added VideoTrackInterface::GetSink method, for use by
VideoRtpReceiver. This lets us delete FrameInput.

I realized this change doesn't depend on VideoSinkInterface changes,
so this is a more standalone version of cl
https://codereview.webrtc.org/1664773002/

BUG=webrtc:5426
Committed: https://crrev.com/8e8908aadd23173c654078f69f6a9c8ec4786e2a
Cr-Commit-Position: refs/heads/master@{#11498}

[nisse-webrtc, 2016-02-03 14:17:32]

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

[nisse-webrtc, 2016-02-03 14:17:33]

I guess this standalone version is easier to land. If you agree, you can ignore
the earlier cl https://codereview.webrtc.org/1664773002/

[perkj_webrtc, 2016-02-03 15:09:58]

looks good with these comments addressed.

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource.h
File talk/app/webrtc/videosource.h (left):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource.h...
talk/app/webrtc/videosource.h:77: virtual cricket::VideoRenderer* FrameInput();
check in chrome if this is mocked ?

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource_u...
File talk/app/webrtc/videosource_unittest.cc (right):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource_u...
talk/app/webrtc/videosource_unittest.cc:212: TEST_F(VideoSourceTest,
StartStopRemote) {
Keep the test for now but remove use of FrameInput.

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videotrack.cc
File talk/app/webrtc/videotrack.cc (right):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videotrack.cc...
talk/app/webrtc/videotrack.cc:62: RTC_DCHECK(video_source_->remote());
Remove DCHECK here.

[nisse-webrtc, 2016-02-03 15:27:22]

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource.h
File talk/app/webrtc/videosource.h (left):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource.h...
talk/app/webrtc/videosource.h:77: virtual cricket::VideoRenderer* FrameInput();
On 2016/02/03 15:09:58, perkj_webrtc wrote:
> check in chrome if this is mocked ? 

It is, overridden in MockVideoSource. Is it sufficient to add a default
implementation in webrtc, returning nullptr? I'm doing that now.

Maybe the change to VideoTrackInterface is more of a problem? It's inherited by
MockWebRtcVideoTrack. Could add a dummy implementation in the interface, but I
guess chrome test could still fail without a real implementation of
MockWebRtcVideoTrack::GetSink? Needs some investigation.

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource_u...
File talk/app/webrtc/videosource_unittest.cc (right):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videosource_u...
talk/app/webrtc/videosource_unittest.cc:212: TEST_F(VideoSourceTest,
StartStopRemote) {
On 2016/02/03 15:09:58, perkj_webrtc wrote:
> Keep the test for now but remove use of FrameInput.

Done.

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videotrack.cc
File talk/app/webrtc/videotrack.cc (right):

https://codereview.webrtc.org/1660103003/diff/1/talk/app/webrtc/videotrack.cc...
talk/app/webrtc/videotrack.cc:62: RTC_DCHECK(video_source_->remote());
On 2016/02/03 15:09:58, perkj_webrtc wrote:
> Remove DCHECK here.

Done.

[nisse-webrtc, 2016-02-03 15:27:40]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-03 15:27:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660103003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660103003/20001

[commit-bot: I haz the power, 2016-02-03 15:30:24]

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

[commit-bot: I haz the power, 2016-02-03 15:30:24]

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

[pthatcher1, 2016-02-03 16:17:40]

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
talk/app/webrtc/mediastreaminterface.h:153: virtual cricket::VideoRenderer*
GetSink() = 0;
We should describe what this returns.  I believe it's a thing, that when given
frames, will allow the frames to go into the track.  

But that just makes me wonder: why doesn't VideoTrackInterface just implement
VideoRendererInterface?

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:89: provider_->SetVideoPlayout(ssrc_, true,
track_->GetSink());
Then you could just pass in track_ here.

In fact, SetVideoPlayout could be changed to take a
rtc::VideoSinkInterface<VideoFrame>, because that's all the MediaChannel and
WebRtcVideoMediaChannel2 need.  Then you could have VideoTrack implement
rtc::VideoSinkInterface<VideoFrame> and not have a VideoRenderer at all.

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpsender...
File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:184: video_track_->GetSink()));
And here

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/videotrac...
File talk/app/webrtc/videotrack.cc (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/videotrac...
talk/app/webrtc/videotrack.cc:63: }
Instead of VideoTrack::GetSink, you could just have VideoTrack::RenderFrame
which calls renderers_->RenderFrame() and you wouldn't need the GetSink
indirection.

[perkj_webrtc, 2016-02-04 07:32:44]

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
talk/app/webrtc/mediastreaminterface.h:153: virtual cricket::VideoRenderer*
GetSink() = 0;
On 2016/02/03 16:17:39, pthatcher1 wrote:
> We should describe what this returns.  I believe it's a thing, that when given
> frames, will allow the frames to go into the track.  
> 
> But that just makes me wonder: why doesn't VideoTrackInterface just implement
> VideoRendererInterface?

Doing that would force the track proxies to also have to implement
VideoRendererInterface and all frames would be marshalled onto the signaling
thread. Not so good. 
I like the other idea better that all tracks just pass AddSink on to the source
and let source do the frame forwarding. But before we can do that- this feels
like a step in the right direction.

However - if will break the possibility to use a remote track as input to
another PC. It will not affect Chrome though so I think that is acceptable while
we are doing this refactoring.

[perkj_webrtc, 2016-02-04 07:35:13]

lgtm

[nisse-webrtc, 2016-02-04 08:22:37]

With patchset 3, this appears to not break building of chrome.

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
talk/app/webrtc/mediastreaminterface.h:153: virtual cricket::VideoRenderer*
GetSink() = 0;
On 2016/02/03 16:17:39, pthatcher1 wrote:
> We should describe what this returns.  I believe it's a thing, that when given
> frames, will allow the frames to go into the track.  

Done.

> But that just makes me wonder: why doesn't VideoTrackInterface just implement
> VideoRendererInterface?

That's what I tried first ;-). It made sense to have a VideoTrack be a mux,
implementing both Source and Sink interfaces. 

I first got into a mundane problem: I did that on the branch after changing
these things from VideoRenderer to VideoSinkInterface. But I got compilation
errors when adding the OnFrame method to VideoTrackProxy which I couldn't sort
out.

And then after talking to Per, I realized that it's probably not a good idea
anyway to have the VideoTrackProxy implement OnFrame. It seemed better to have
the proxy implement GetSink(), and return a pointer to the underlying object.

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:89: provider_->SetVideoPlayout(ssrc_, true,
track_->GetSink());
On 2016/02/03 16:17:39, pthatcher1 wrote:
> Then you could just pass in track_ here.
> 
> In fact, SetVideoPlayout could be changed to take a
> rtc::VideoSinkInterface<VideoFrame>, because that's all the MediaChannel and
> WebRtcVideoMediaChannel2 need.  Then you could have VideoTrack implement
> rtc::VideoSinkInterface<VideoFrame> and not have a VideoRenderer at all.

That's *exactly* what I did, before preparing the current cl.

[nisse-webrtc, 2016-02-04 10:05:19]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 10:05:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660103003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660103003/80001

[commit-bot: I haz the power, 2016-02-04 11:36:42]

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

[commit-bot: I haz the power, 2016-02-04 11:36:42]

Dry run: This issue passed the CQ dry run.

[pthatcher1, 2016-02-04 17:30:45]

lgtm, assuming you add the comments I asked for

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1660103003/diff/20001/talk/app/webrtc/mediastre...
talk/app/webrtc/mediastreaminterface.h:153: virtual cricket::VideoRenderer*
GetSink() = 0;
On 2016/02/04 08:22:37, nisse-webrtc wrote:
> On 2016/02/03 16:17:39, pthatcher1 wrote:
> > We should describe what this returns.  I believe it's a thing, that when
given
> > frames, will allow the frames to go into the track.  
> 
> Done.
> 
> > But that just makes me wonder: why doesn't VideoTrackInterface just
implement
> > VideoRendererInterface?
> 
> That's what I tried first ;-). It made sense to have a VideoTrack be a mux,
> implementing both Source and Sink interfaces. 
> 
> I first got into a mundane problem: I did that on the branch after changing
> these things from VideoRenderer to VideoSinkInterface. But I got compilation
> errors when adding the OnFrame method to VideoTrackProxy which I couldn't sort
> out.
> 
> And then after talking to Per, I realized that it's probably not a good idea
> anyway to have the VideoTrackProxy implement OnFrame. It seemed better to have
> the proxy implement GetSink(), and return a pointer to the underlying object.

So a Track is neither a Source nor a Sink, but just has a Source and Sink.  I
guess that sort of makes sense.  Except... are they guaranteed not to change?  A
sure hope so.  In fact, can you put a comment saying something like "the value
returned by GetSink() must not change between calls", or something like that?

Also, can you write a comment explaining why VideoTrackInterface can't implement
VideoSinkInterface, so that next time someone asks this the answer will just be
right there?  I'm *sure* I won't be the last person to ask.

[nisse-webrtc, 2016-02-05 08:45:04]

On 2016/02/04 17:30:45, pthatcher1 wrote:
> lgtm, assuming you add the comments I asked for

I'll add them in a moment.

> So a Track is neither a Source nor a Sink, but just has a Source and Sink.  I
> guess that sort of makes sense. 

I think it is a Source, it should be changed as soon as the cl creating
rtc::VideoSourceInterface is landed.

And if we decide to leave muxing/broadcast to VideoSource, as we have discussed
elsewhere, VideoTrack should still be a Source. On AddSink, it should pass on
the sink to the VideoSource's AddSink, *and* add it to its own lists of sinks. 

This list is needed for enable/disable logic, since if I understand the higher
level apis correctly, an application may have a reference to a VideoTrack and
switch it between the enabled and disabled, and we have to make sure that
affects the right sinks, and the VideoSource can't do that by itself because it
won't know which if its sinks belong to which track. So one way to implement
VideoTrack::set_enabled(false) might be to call VideoSource::RemoveSink on all
sinks associated with the track, and instead add them to a BlackFrameSource
owned by the track.

[nisse-webrtc, 2016-02-05 08:52:01]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-02-05 08:52:01]

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

[commit-bot: I haz the power, 2016-02-05 08:52:10]

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

[commit-bot: I haz the power, 2016-02-05 09:52:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-05 09:52:26]

Description was changed from

==========
Delete FrameInput method and FrameInputWrapper class.

Added VideoTrackInterface::GetSink method, for use by
VideoRtpReceiver. This lets us delete FrameInput.

I realized this change doesn't depend on VideoSinkInterface changes,
so this is a more standalone version of cl
https://codereview.webrtc.org/1664773002/

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

to

==========
Delete FrameInput method and FrameInputWrapper class.

Added VideoTrackInterface::GetSink method, for use by
VideoRtpReceiver. This lets us delete FrameInput.

I realized this change doesn't depend on VideoSinkInterface changes,
so this is a more standalone version of cl
https://codereview.webrtc.org/1664773002/

BUG=webrtc:5426

Committed: https://crrev.com/8e8908aadd23173c654078f69f6a9c8ec4786e2a
Cr-Commit-Position: refs/heads/master@{#11498}
==========

[commit-bot: I haz the power, 2016-02-05 09:52:27]

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