Replace interface VideoCapturerInput with VideoSinkInterface.

Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling using rtc::VideoSinkWants instead of separately reporting CPU overuse and internally doing downscaling due to QP values.

BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true

Committed: https://crrev.com/95a226f55ae7e32b83a6ba96232fb105a014dc6c
Cr-Commit-Position: refs/heads/master@{#14238}

[perkj_webrtc, 2016-08-19 12:58:13]

Description was changed from

==========
Exchange VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)
BUG=
==========

to

==========
Exchange VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)
BUG=webrtc:5687
==========

[perkj_webrtc, 2016-08-19 13:03:22]

Description was changed from

==========
Exchange VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)
BUG=webrtc:5687
==========

to

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
==========

[perkj_webrtc, 2016-08-19 13:03:22]

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

[perkj_webrtc, 2016-08-19 13:03:53]

please ? Never mind the red bots- I need to update the dependend patchset.

[perkj_webrtc, 2016-08-19 13:04:13]

Patchset #2 (id:20001) has been deleted

[nisse-webrtc, 2016-08-22 07:20:30]

For the classes that implement VideoSourceInterface, but only ever supports a
single sink. I think that's a slight abuse of the interface. 

It might make things simpler to pass the VideoSinkInterface  to the constructor
(I think that's maybe how some of the old code worked?), even if you later have
to invent a new UpdateSinkWants method. Is that a possible alternative?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2056: encoder_sink_ = sink;
Only one sink supported, right? Add a CHECK that encode_sink_ is null?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2062: encoder_sink_ = nullptr;
Similarly, add a CHECK that encoder_sink_ == sink?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/frame_generat...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/frame_generat...
webrtc/test/frame_generator.h:25: class FrameForwarder : public
rtc::VideoSourceInterface<VideoFrame> {
Is this class intended as a temporary hack?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/vcm_capturer....
webrtc/test/vcm_capturer.cc:87: RTC_CHECK(sink_);
Consider stronger check, sink != nullptr, and sink == sink_ ?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/video_capturer.h
File webrtc/test/video_capturer.h (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/video_capture...
webrtc/test/video_capturer.h:24: class VideoCapturer : public
rtc::VideoSourceInterface<VideoFrame> {
The corresponding .cc file is deleted.

So this is now only an interface? It's used for fake capturers, to have tests
not have to depend on cricket::VideoCapturer, or?

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:224: class ViEEncoder::VideoSourceProxy {
I'd like to see a comment briefly explaining the point of this proxy class.

[nisse-webrtc, 2016-08-23 10:13:28]

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1660:
rtc::Optional<int64_t>(rtc::TimeMillis() - frame_delta_ms);
We should soon be able to delete this, since all capturers will use the
TimeStampAligner and have timestamps in the same time scale.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1679:
encoder_sink_->OnFrame(video_frame);
The old IncomingCapturedFrame was responsible for populating the additional
timestamps in webrtc::VideoFrame (set_ntp_time_ms() and set_timestamp()). Can
that be dropped, or where do you intend to move that?

[perkj_webrtc, 2016-09-01 11:46:16]

PTAL- The pre-requisite cl is now landed.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1660:
rtc::Optional<int64_t>(rtc::TimeMillis() - frame_delta_ms);
On 2016/08/23 10:13:28, nisse-webrtc wrote:
> We should soon be able to delete this, since all capturers will use the
> TimeStampAligner and have timestamps in the same time scale.

Acknowledged.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1679:
encoder_sink_->OnFrame(video_frame);
On 2016/08/23 10:13:28, nisse-webrtc wrote:
> The old IncomingCapturedFrame was responsible for populating the additional
> timestamps in webrtc::VideoFrame (set_ntp_time_ms() and set_timestamp()). Can
> that be dropped, or where do you intend to move that?

That is still done in VieEncoder::OnFrame and is waiting for you to fix :->

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2056: encoder_sink_ = sink;
On 2016/08/22 07:20:29, nisse-webrtc wrote:
> Only one sink supported, right? Add a CHECK that encode_sink_ is null?

Done.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2062: encoder_sink_ = nullptr;
On 2016/08/22 07:20:29, nisse-webrtc wrote:
> Similarly, add a CHECK that encoder_sink_ == sink?

Done.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/frame_generat...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/frame_generat...
webrtc/test/frame_generator.h:25: class FrameForwarder : public
rtc::VideoSourceInterface<VideoFrame> {
On 2016/08/22 07:20:29, nisse-webrtc wrote:
> Is this class intended as a temporary hack?

This is in test and intended to be used as an adapter by those tests that
previously just did VideoSendStream->Input()->IncomingFrame().

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/vcm_capturer....
webrtc/test/vcm_capturer.cc:87: RTC_CHECK(sink_);
On 2016/08/22 07:20:29, nisse-webrtc wrote:
> Consider stronger check, sink != nullptr, and sink == sink_ ?

Done.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/video_capturer.h
File webrtc/test/video_capturer.h (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/test/video_capture...
webrtc/test/video_capturer.h:24: class VideoCapturer : public
rtc::VideoSourceInterface<VideoFrame> {
On 2016/08/22 07:20:30, nisse-webrtc wrote:
> The corresponding .cc file is deleted.
> 
> So this is now only an interface? It's used for fake capturers, to have tests
> not have to depend on cricket::VideoCapturer, or?

this is not cricket - it is actually webrtc::test. So yes, this is only to
continue support the existing webrtc:: tests.

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:224: class ViEEncoder::VideoSourceProxy {
On 2016/08/22 07:20:30, nisse-webrtc wrote:
> I'd like to see a comment briefly explaining the point of this proxy class.

Done.

[nisse-webrtc, 2016-09-01 12:03:21]

LGTM, if you fix the SendSend name.

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:171: void SetSendSendStream(VideoSendStream*
stream) {
Do you really want SendSend in the name? I hope there can't ever be a
SetRecvSendStream...

[perkj_webrtc, 2016-09-02 06:50:11]

Patchset #5 (id:100001) has been deleted

[perkj_webrtc, 2016-09-02 06:51:15]

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:171: void SetSendSendStream(VideoSendStream*
stream) {
On 2016/09/01 12:03:21, nisse-webrtc wrote:
> Do you really want SendSend in the name? I hope there can't ever be a
> SetRecvSendStream...

nope- mistake.

[sprang_webrtc, 2016-09-02 07:19:32]

Ugh, we're gonna have a conflict here.
Could you incorporate https://codereview.webrtc.org/1534233002/ into this?

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2082: RTC_DCHECK(encoder_sink_&&
encoder_sink_ = sink);
You don't need to check encoder_sink_ != nullptr if you also check equality with
sink. Unless it's a roundabout way of checking that sink != nullptr?

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/test/frame_generat...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/test/frame_generat...
webrtc/test/frame_generator_capturer.cc:68: RTC_DCHECK(target_fps > 0);
RTC_DCHECK_GT

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/test/frame_generat...
webrtc/test/frame_generator_capturer.cc:138: RTC_CHECK(sink_);
nit: for consistency, check sink_ == sink?

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:719: }
nit: newline

[nisse-webrtc, 2016-09-02 08:10:19]

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2082: RTC_DCHECK(encoder_sink_&&
encoder_sink_ = sink);
On 2016/09/02 07:19:31, språng wrote:
> You don't need to check encoder_sink_ != nullptr if you also check equality
with
> sink. Unless it's a roundabout way of checking that sink != nullptr?

I think it is desirable to fail this DCHECK in the case that encoder_sink_ ==
nullptr && sink == nullptr.

[sprang_webrtc, 2016-09-02 09:07:04]

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2082: RTC_DCHECK(encoder_sink_&&
encoder_sink_ = sink);
On 2016/09/02 08:10:18, nisse-webrtc wrote:
> On 2016/09/02 07:19:31, språng wrote:
> > You don't need to check encoder_sink_ != nullptr if you also check equality
> with
> > sink. Unless it's a roundabout way of checking that sink != nullptr?
> 
> I think it is desirable to fail this DCHECK in the case that encoder_sink_ ==
> nullptr && sink == nullptr.

Sure, but then I'd rather check that sink != nullptr.
Here and in all the other RemoveSink() instances (some only check equality)

[perkj_webrtc, 2016-09-02 12:18:21]

ptal- I would prefer to discuss how to merge that cl separate. 
It does not really have anything to do with this cl -> it is already broken.

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2082: RTC_DCHECK(encoder_sink_&&
encoder_sink_ = sink);
On 2016/09/02 09:07:04, språng wrote:
> On 2016/09/02 08:10:18, nisse-webrtc wrote:
> > On 2016/09/02 07:19:31, språng wrote:
> > > You don't need to check encoder_sink_ != nullptr if you also check
equality
> > with
> > > sink. Unless it's a roundabout way of checking that sink != nullptr?
> > 
> > I think it is desirable to fail this DCHECK in the case that encoder_sink_
==
> > nullptr && sink == nullptr.
> 
> Sure, but then I'd rather check that sink != nullptr.
> Here and in all the other RemoveSink() instances (some only check equality)

Prefer just DCHECK(sink_ == sink);

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/test/frame_generat...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/test/frame_generat...
webrtc/test/frame_generator_capturer.cc:138: RTC_CHECK(sink_);
On 2016/09/02 07:19:31, språng wrote:
> nit: for consistency, check sink_ == sink?

Done.

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:719: }
On 2016/09/02 07:19:31, språng wrote:
> nit: newline

Done.

[perkj_webrtc, 2016-09-05 13:47:04]

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

[perkj_webrtc, 2016-09-05 13:47:05]

Adding flodman as well.

[sprang_webrtc, 2016-09-06 10:04:58]

lgtm with nits

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1663: if (encoder_sink_ == NULL) {
nit: nullptr

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:244: }
nit: remove {} here or add them below, just so we're consistent

[perkj_webrtc, 2016-09-06 11:58:28]

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

[perkj_webrtc, 2016-09-06 11:58:29]

Adding Stefan as well. Maybe you are a more suitable owner for this?

[stefan-webrtc, 2016-09-06 12:24:39]

One general question:

I see that both sinks and sources are set using methods. Shouldn't it be
possible to set one of them through the constructor to reduce the locks
required?

Also, when we are setting a source to a sink, is that relationship sometimes
one-way, or is it always the case that the sink is also set as a source?

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:223: source_->RemoveSink(this);
Do we expect sources to be set and removed dynamically? I'm not sure I like that
we silently remove them as it may hide bugs.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2075: RTC_DCHECK(!encoder_sink_ ||
encoder_sink_ == sink);
Why do we allow setting the same sink twice?

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:266: // Implements
rtc::VideoSourceInterface<webrtc::VideoFrame>
I think we should have a comment saying what the purpose of implementing the
interface is. Is it to connect a VideoSendStream to a renderer? It's not clear
to me when the VideoSendStream produces VideoFrames.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/test/frame_genera...
webrtc/test/frame_generator.h:35: rtc::VideoSinkInterface<VideoFrame>* sink_
GUARDED_BY(crit_) = nullptr;
I prefer using the initializer list.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:206:
forwarder_.IncomingCapturedFrame(CreateFrame(2, nullptr));
The test is changed here. Can you comment on why?

[perkj_webrtc, 2016-09-07 15:10:37]

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:223: source_->RemoveSink(this);
On 2016/09/06 12:24:38, stefan-webrtc (holmer) wrote:
> Do we expect sources to be set and removed dynamically? I'm not sure I like
that
> we silently remove them as it may hide bugs.

this is mimicking the real implementation. SetSource is there so that cameras
can be replaced.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1663: if (encoder_sink_ == NULL) {
On 2016/09/06 10:04:57, språng wrote:
> nit: nullptr

Done.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2075: RTC_DCHECK(!encoder_sink_ ||
encoder_sink_ == sink);
On 2016/09/06 12:24:38, stefan-webrtc (holmer) wrote:
> Why do we allow setting the same sink twice?

This is AddOrUpdateSink. It will be called multiple times in a follow up cl when
I move CPU adaptation logic to VieEncoder.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:266: // Implements
rtc::VideoSourceInterface<webrtc::VideoFrame>
On 2016/09/06 12:24:38, stefan-webrtc (holmer) wrote:
> I think we should have a comment saying what the purpose of implementing the
> interface is. Is it to connect a VideoSendStream to a renderer? It's not clear
> to me when the VideoSendStream produces VideoFrames.

Done.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/test/frame_genera...
webrtc/test/frame_generator.h:35: rtc::VideoSinkInterface<VideoFrame>* sink_
GUARDED_BY(crit_) = nullptr;
On 2016/09/06 12:24:38, stefan-webrtc (holmer) wrote:
> I prefer using the initializer list.

Done.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:244: }
On 2016/09/06 10:04:57, språng wrote:
> nit: remove {} here or add them below, just so we're consistent

Done.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:206:
forwarder_.IncomingCapturedFrame(CreateFrame(2, nullptr));
On 2016/09/06 12:24:38, stefan-webrtc (holmer) wrote:
> The test is changed here. Can you comment on why?

oops. Thanks- its a mistake.

https://codereview.webrtc.org/2257413002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:238:
forwarder_.IncomingCapturedFrame(CreateFrame(2, nullptr));
I also noticed this mistake that passed the unittests since there are no
EXPECT_TRUE( when waiting for an event in this file). Fixed now. forwarder_ also
renamed video_source_.

[stefan-webrtc, 2016-09-13 09:26:43]

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:222: if (source_)
Perhaps DCHECK that source and _source are not the same?

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2053: void
WebRtcVideoChannel2::WebRtcVideoSendStream::AddOrUpdateSink(
Add sounds like several sinks are supported, which doesn't seem to be the case?

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2059: RTC_DCHECK(!encoder_sink_ ||
encoder_sink_ == sink);
It looks error prone to allow adding the same sink twice. Why is that needed?

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2066: RTC_DCHECK(encoder_sink_ ==
sink);
DCHECK_EQ

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:259: RTC_DCHECK(sink == sink_);
DCHECK_EQ. Also the same comment applies as in the previous file.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.h:25: class FrameForwarder : public
rtc::VideoSourceInterface<VideoFrame> {
A short comment on the purpose of this class would be good.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
File webrtc/test/video_capturer.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
webrtc/test/video_capturer.h:29: virtual void Stop() = 0;
Where's the implementation of this now that video_capturer.cc is removed?

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:702: // It forwards all input frames to the
VideoAnalyser for later comparison and
VideoAnalyzer

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:231: // (encoder_queue_) where encoders report its
VideoSinkWants.
"report their"

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:139: rtc::ThreadChecker main_thread_checker_;
It's not clear to me what the main thread is, can we have a better description?

[perkj_webrtc, 2016-09-14 14:20:22]

ptal

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:222: if (source_)
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> Perhaps DCHECK that source and _source are not the same?

Done.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2053: void
WebRtcVideoChannel2::WebRtcVideoSendStream::AddOrUpdateSink(
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> Add sounds like several sinks are supported, which doesn't seem to be the
case?

Nope- there is a 1-1 mapping between sendstream and encoder. End goal is
probably connect the encoder with the capturer. The capturer can have multiple
sinks.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2059: RTC_DCHECK(!encoder_sink_ ||
encoder_sink_ == sink);
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> It looks error prone to allow adding the same sink twice. Why is that needed?

For updating the wants. Will be done in a follow up cl.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2066: RTC_DCHECK(encoder_sink_ ==
sink);
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> DCHECK_EQ

Done.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:259: RTC_DCHECK(sink == sink_);
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> DCHECK_EQ. Also the same comment applies as in the previous file.

Done.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.h:25: class FrameForwarder : public
rtc::VideoSourceInterface<VideoFrame> {
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> A short comment on the purpose of this class would be good.

Done.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
File webrtc/test/video_capturer.cc (left):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
webrtc/test/video_capturer.cc:37: VcmCapturer* vcm_capturer =
VcmCapturer::Create(input, width, height, fps);
this is not really an implementation. It is just a factory method that I think
we can live without.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
File webrtc/test/video_capturer.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/test/video_captur...
webrtc/test/video_capturer.h:29: virtual void Stop() = 0;
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> Where's the implementation of this now that video_capturer.cc is removed?

This is now a pure interface that just extend
rtc::VideoSourceInterface<VideoFrame>  with two methods. Start and Stop.

Its implemented by FrameGeneratorCapturer and VcmCapturer for use in
video_quality_test.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:702: // It forwards all input frames to the
VideoAnalyser for later comparison and
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> VideoAnalyzer

Done.

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:231: // (encoder_queue_) where encoders report its
VideoSinkWants.
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> "report their"

encoder reports its Vide,,

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2257413002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:139: rtc::ThreadChecker main_thread_checker_;
On 2016/09/13 09:26:43, stefan-webrtc (holmer) wrote:
> It's not clear to me what the main thread is, can we have a better
description?

I tried... Not sure if this is any better. In practise it is the libjingle
worker thread.

[stefan-webrtc, 2016-09-14 14:51:09]

lgtm

https://codereview.webrtc.org/2257413002/diff/240001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2257413002/diff/240001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:140: // of ViEEncoder is called on the same thread.
are called

[perkj_webrtc, 2016-09-15 15:20:34]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-09-15 15:20:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
sprang@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2257413002/#ps280001 (title:
"Rebased")

[commit-bot: I haz the power, 2016-09-15 15:20:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[perkj_webrtc, 2016-09-15 15:54:36]

Description was changed from

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
==========

to

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true
==========

[perkj_webrtc, 2016-09-15 15:55:03]

The CQ bit was unchecked by perkj@webrtc.org

[perkj_webrtc, 2016-09-15 15:55:19]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2016-09-15 15:55:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-15 15:57:23]

Description was changed from

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true
==========

to

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true
==========

[commit-bot: I haz the power, 2016-09-15 15:57:25]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-09-15 15:57:32]

Description was changed from

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.


BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true
==========

to

==========
Replace VideoCapturerInput with VideoSinkInterface.
Adds new method VideoSendStream::SetSource(rtc::VideoSourceInterface* and
VieEncoder::SetSource(rtc::VideoSourceInterface*)

This is the first step needed in order for the ViEEncoder to request downscaling
using rtc::VideoSinkWants instead of separately reporting CPU overuse and
internally doing downscaling due to QP values.

BUG=webrtc:5687
// Android CQ seems broken.
NOTRY=true

Committed: https://crrev.com/95a226f55ae7e32b83a6ba96232fb105a014dc6c
Cr-Commit-Position: refs/heads/master@{#14238}
==========

[commit-bot: I haz the power, 2016-09-15 15:57:34]

Patchset 13 (id:??) landed as
https://crrev.com/95a226f55ae7e32b83a6ba96232fb105a014dc6c
Cr-Commit-Position: refs/heads/master@{#14238}

[perkj_webrtc, 2016-09-15 16:18:58]

A revert of this CL (patchset #13 id:280001) has been created in
https://codereview.webrtc.org/2344923002/ by perkj@webrtc.org.

The reason for reverting is: Fails on Mac and Linux webrtc_perf_tests.