Let ViEEncoder express resolution requests as Sinkwants

Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation in WebrtcVideoSendStream and replace it with VideoSinkWants through the VideoSourceInterface.

To do that that, some stats for CPU adaptation is moved into VideoSendStream. Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org

Committed: https://crrev.com/803d97f15937f0aba79043b8d92d8a66dfa31cfb
Cr-Commit-Position: refs/heads/master@{#14877}

[perkj_webrtc, 2016-09-08 12:55:01]

Description was changed from

==========
Move Sinkwants.

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

to

==========
Let ViEEncoder express resolution requests as Sinkwants.

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

[perkj_webrtc, 2016-10-04 08:53:14]

Description was changed from

==========
Let ViEEncoder express resolution requests as Sinkwants.

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

to

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

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

[perkj_webrtc, 2016-10-04 08:53:14]

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

[perkj_webrtc, 2016-10-04 08:53:26]

please?

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

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:244: if (source)
{}

[kthelgason, 2016-10-05 12:42:19]

kthelgason@webrtc.org changed reviewers:
+ kthelgason@webrtc.org

[kthelgason, 2016-10-05 12:42:20]

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:411: source_proxy_->SetSource(nullptr, false);
Why hardcode false here?

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:773: if (disable_resolution_scaling_)
Rather than checking this here, I feel it would make sense to do this further up
the stack, and not instantiate the overuse_frame_detector_ unless this is false.
Then we don't waste cycles running that in the background, and it making a bunch
of fruitless callbacks back to this class.

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:795: if (disable_resolution_scaling_)
Ditto

[kthelgason, 2016-10-06 16:46:12]

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:452: encoder_queue_.PostTask([this, sink,
rotation_applied] {
captured variable unused in lambda

[perkj_webrtc, 2016-10-18 07:48:21]

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:411: source_proxy_->SetSource(nullptr, false);
On 2016/10/05 12:42:20, kthelgason wrote:
> Why hardcode false here?

true to disable overuse detection maybe make more sence.

https://codereview.webrtc.org/2304363002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:452: encoder_queue_.PostTask([this, sink,
rotation_applied] {
On 2016/10/06 16:46:12, kthelgason wrote:
> captured variable unused in lambda

Done.

[kthelgason, 2016-10-18 08:31:20]

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:181:
send_stream_->SetSource(frame_generator_capturer_.get(), false);
I personally really dislike boolean arguments like this. There is no way to know
what the "false" means but looking up the definition of the function. IMO this
would be a lot clearer with a boolean enum, something like:

enum CpuAdaptation : bool {
    Off = true,
    On = false,
  };

Then this call would become:
SetSource(capturer, CpuAdaptation::On);

So much clearer. Whether it's worth the hassle on the other hand...

[kthelgason, 2016-10-19 12:06:45]

Couple of small ideas.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:745: return extension.uri ==
webrtc::RtpExtension::kVideoRotationUri;
seems to me that the webrtc:: scope resolution specifier should be unnecessary
here.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:706: call_overuse_frame_sent();
Is this contortion really saving us anything? Seems much simpler to just always
post it to the queue.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:788: LOG(LS_INFO) << "OveruseDetected usage";
This is a weird log message! Perhaps change this to something more descriptive.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:797: max_pixel_count_.value_or(current_pixel_count +
1)) {
I think this would read better as (max_pixel_count_ && current_pixel_count <
*max_pixel_count_)

The current_pixel_count + 1 looks like a workaround to me

Actually, it could just be an int initialized to MAX_INT. That seems simpler,
and easier to code.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:799: max_pixel_count_step_up_ =
rtc::Optional<int>();
Same here, this could just be an int initialized to 0.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:64: static const int kMaxCpuDowngrades = 2;
Why two? Should this be configurable somewhere?

[åsapersson, 2016-10-24 07:54:06]

https://codereview.chromium.org/2304363002/diff/220001/webrtc/call/call_perf_...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/call/call_perf_...
webrtc/call/call_perf_tests.cc:528: bool expect_lower_resolution_wants_;
const

https://codereview.chromium.org/2304363002/diff/220001/webrtc/call/call_perf_...
webrtc/call/call_perf_tests.cc:540: TestCpuOveruse(true);
maybe remove since overuse is tested in test above

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/fa...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/fa...
webrtc/media/engine/fakewebrtccall.h:130: bool resolution_scaling_disabled() {
return resolution_scaling_disabled_; }
const method

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/fa...
webrtc/media/engine/fakewebrtccall.h:133:
rtc::VideoSourceInterface<webrtc::VideoFrame>* source() { return source_; }
ditto

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/fa...
webrtc/media/engine/fakewebrtccall.h:158: bool resolution_scaling_disabled_;
change to resolution_scaling_enabled_?

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/we...
File webrtc/media/engine/webrtcvideoengine2.h (left):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/media/engine/we...
webrtc/media/engine/webrtcvideoengine2.h:352: void UpdateHistograms() const
EXCLUSIVE_LOCKS_REQUIRED(lock_);
remove method

https://codereview.chromium.org/2304363002/diff/220001/webrtc/test/frame_gene...
File webrtc/test/frame_generator.cc (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/test/frame_gene...
webrtc/test/frame_generator.cc:273: size_t FrameForwarder::number_of_sinks()
const {
maybe bool has_sink()

https://codereview.chromium.org/2304363002/diff/220001/webrtc/test/frame_gene...
File webrtc/test/frame_generator.h (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/test/frame_gene...
webrtc/test/frame_generator.h:44: rtc::VideoSinkWants current_sink_wants_
GUARDED_BY(crit_);
maybe sink_wants_

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/send_stat...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/send_stat...
webrtc/video/send_statistics_proxy.h:43: // Number of required UMA samples to be
collected before a metric is added
Number of required samples..

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/send_stat...
webrtc/video/send_statistics_proxy.h:44: // to a UMA histogram.
UMA histogram -> rtc histogram

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/send_stat...
webrtc/video/send_statistics_proxy.h:45: static const int kMinRequiredUMASamples
= 200;
maybe MinRequiredMetricsSamples

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/video_sen...
File webrtc/video/video_send_stream.h (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/video_sen...
webrtc/video/video_send_stream.h:76: bool disable_resolution_scaling) override;
change to enable_resolution_scaling?

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
File webrtc/video/vie_encoder.cc (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.cc:270: bool disable_resolution_scaling) {
change to enable_resolution_scaling?

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.cc:312: // task
move comment up one line

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.cc:348: bool resolution_scaling_disabled_
GUARDED_BY(&crit_);
change to resolution_scaling_enabled_?

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.cc:437: overuse_detector_.reset(nullptr);
Note that the encode time is reported to SendStatisticsProxy via
OveruseFrameDetector::EncodedFrameTimeMeasured. A change might be needed to
still get encode time stats when resolution scaling is disabled (e.g. for
screenshare).

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.cc:445: cpu_restricted_counter_ != 0);
Is SetCpuRestrictedResolution needed? Looks like when cpu_restricted_counter_ is
updated it is reported via OnCpuRestrictedResolutionChanged.

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
File webrtc/video/vie_encoder.h (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.h:80: // |disable_resolution| means that this encoder
will not request |source| to
|disable_resolution_scaling|

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.h:210: // restricted by CPU usage. It is reset if
|source_| is changed.
Could not see where it is reset.

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder.h:211: int cpu_restricted_counter_;  //
ACCESS_ON(&encoder_queue_);
Should access_on be commented out?

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.chromium.org/2304363002/diff/220001/webrtc/video/vie_encod...
webrtc/video/vie_encoder_unittest.cc:435: // lower resolution.
see spacing

[perkj_webrtc, 2016-10-26 16:40:17]

PTAL

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:181:
send_stream_->SetSource(frame_generator_capturer_.get(), false);
On 2016/10/18 08:31:20, kthelgason wrote:
> I personally really dislike boolean arguments like this. There is no way to
know
> what the "false" means but looking up the definition of the function. IMO this
> would be a lot clearer with a boolean enum, something like:
> 
> enum CpuAdaptation : bool {
>     Off = true,
>     On = false,
>   };
> 
> Then this call would become:
> SetSource(capturer, CpuAdaptation::On);
> 
> So much clearer. Whether it's worth the hassle on the other hand...

ok- I agree, I added an enum that match the current spec and hope we can later
implement all aspects of it.  For now it is translated in webrtcvideoengine2.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:528: bool expect_lower_resolution_wants_;
On 2016/10/24 07:54:05, åsapersson wrote:
> const

used your second suggestion. This is used as state in the test now.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:540: TestCpuOveruse(true);
On 2016/10/24 07:54:05, åsapersson wrote:
> maybe remove since overuse is tested in test above

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:130: bool resolution_scaling_disabled() {
return resolution_scaling_disabled_; }
On 2016/10/24 07:54:05, åsapersson wrote:
> const method

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:133:
rtc::VideoSourceInterface<webrtc::VideoFrame>* source() { return source_; }
On 2016/10/24 07:54:05, åsapersson wrote:
> ditto

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:158: bool resolution_scaling_disabled_;
On 2016/10/24 07:54:05, åsapersson wrote:
> change to resolution_scaling_enabled_?

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (left):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:352: void UpdateHistograms() const
EXCLUSIVE_LOCKS_REQUIRED(lock_);
On 2016/10/24 07:54:05, åsapersson wrote:
> remove method

Done.

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

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:273: size_t FrameForwarder::number_of_sinks()
const {
On 2016/10/24 07:54:05, åsapersson wrote:
> maybe bool has_sink()

A source should support multiple sinks.. Currently though, no unit test need
that and thus this implementation only support one. 
But I guess current_sink_wants does not necessarily make sense if there are
multiple sinks so ok, lets rename this for now and we will see in the future.

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

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/test/frame_genera...
webrtc/test/frame_generator.h:44: rtc::VideoSinkWants current_sink_wants_
GUARDED_BY(crit_);
On 2016/10/24 07:54:06, åsapersson wrote:
> maybe sink_wants_

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:43: // Number of required UMA samples to be
collected before a metric is added
On 2016/10/24 07:54:06, åsapersson wrote:
> Number of required samples..

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:44: // to a UMA histogram.
On 2016/10/24 07:54:06, åsapersson wrote:
> UMA histogram -> rtc histogram

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:45: static const int kMinRequiredUMASamples
= 200;
On 2016/10/24 07:54:06, åsapersson wrote:
> maybe MinRequiredMetricsSamples

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:745: return extension.uri ==
webrtc::RtpExtension::kVideoRotationUri;
On 2016/10/19 12:06:44, kthelgason wrote:
> seems to me that the webrtc:: scope resolution specifier should be unnecessary
> here.

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:76: bool disable_resolution_scaling) override;
On 2016/10/24 07:54:06, åsapersson wrote:
> change to enable_resolution_scaling?

I guess I should...
And I thought I should make Kari happy as well and changed this to an enum that
match the enum in the w3c spec for degradation preference.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:270: bool disable_resolution_scaling) {
On 2016/10/24 07:54:06, åsapersson wrote:
> change to enable_resolution_scaling?

now use preferences

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:312: // task
On 2016/10/24 07:54:06, åsapersson wrote:
> move comment up one line

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:348: bool resolution_scaling_disabled_
GUARDED_BY(&crit_);
On 2016/10/24 07:54:06, åsapersson wrote:
> change to resolution_scaling_enabled_?

degradation_preference_

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:437: overuse_detector_.reset(nullptr);
On 2016/10/24 07:54:06, åsapersson wrote:
> Note that the encode time is reported to SendStatisticsProxy via
> OveruseFrameDetector::EncodedFrameTimeMeasured. A change might be needed to
> still get encode time stats when resolution scaling is disabled (e.g. for
> screenshare).  

Thanks for pointing this out. I did this change since Kari pointed out that we
dont need to call the overuse detector at all when when it is off.  But Erik
also mentioned that he want to change the overuseframedetector to also support
signaling that frame rate should be lowered.  So I will revert these changes and
always call the detector for now and only ignore the result.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:445: cpu_restricted_counter_ != 0);
On 2016/10/24 07:54:06, åsapersson wrote:
> Is SetCpuRestrictedResolution needed? Looks like when cpu_restricted_counter_
is
> updated it is reported via OnCpuRestrictedResolutionChanged.

I think it is needed but it is really up to you. What if the source or
degradation preferences are changed? I assume that we then should set the stats
for weather or not we are currently CPU limited accordingly independently of
what the overuseframedetector say.

The reason there is both SetCpuRestricted and OnCpuRestrictedResolutionChanged
is that OnCpuRestrictedResolutionChanged also update the counter of the number
of times the resolution has been requested to change. That counter is not
updated if you for example switch to screen share and back.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:706: call_overuse_frame_sent();
On 2016/10/19 12:06:44, kthelgason wrote:
> Is this contortion really saving us anything? Seems much simpler to just
always
> post it to the queue.

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:788: LOG(LS_INFO) << "OveruseDetected usage";
On 2016/10/19 12:06:44, kthelgason wrote:
> This is a weird log message! Perhaps change this to something more
descriptive.

Done.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:797: max_pixel_count_.value_or(current_pixel_count +
1)) {
On 2016/10/19 12:06:44, kthelgason wrote:
> I think this would read better as (max_pixel_count_ && current_pixel_count <
> *max_pixel_count_)
> 
> The current_pixel_count + 1 looks like a workaround to me
> 
> Actually, it could just be an int initialized to MAX_INT. That seems simpler,
> and easier to code.

Done.

I prefer optional instead of a magic value though to indicate that it is not
set.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:799: max_pixel_count_step_up_ =
rtc::Optional<int>();
On 2016/10/19 12:06:44, kthelgason wrote:
> Same here, this could just be an int initialized to 0.

dito

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:64: static const int kMaxCpuDowngrades = 2;
On 2016/10/19 12:06:45, kthelgason wrote:
> Why two? Should this be configurable somewhere?

I don't want to change behaviour in this cl. That is what it was before.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:80: // |disable_resolution| means that this encoder
will not request |source| to
On 2016/10/24 07:54:06, åsapersson wrote:
> |disable_resolution_scaling|

Change not to preferences.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:210: // restricted by CPU usage. It is reset if
|source_| is changed.
On 2016/10/24 07:54:06, åsapersson wrote:
> Could not see where it is reset.

Actually it is not. Removed comment.

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:211: int cpu_restricted_counter_;  //
ACCESS_ON(&encoder_queue_);
On 2016/10/24 07:54:06, åsapersson wrote:
> Should access_on be commented out?

oops

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

https://codereview.webrtc.org/2304363002/diff/220001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:435: // lower resolution.
On 2016/10/24 07:54:06, åsapersson wrote:
> see spacing

Done.

[perkj_webrtc, 2016-10-26 16:43:22]

perkj@webrtc.org changed reviewers:
+ sprang@google.com

[perkj_webrtc, 2016-10-26 16:43:23]

Added sprang since he expressed interest and mflodman is ooo.

[nisse-webrtc, 2016-10-28 10:53:44]

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1696: // merged.
That time is now. ;-)

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:341: bool IsResutionScalingEnabledLocked() const
EXCLUSIVE_LOCKS_REQUIRED(&crit_) {
Spelling.

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:104: class TestBuffer : public
webrtc::I420Buffer {
Isn't class TestBuffer defined at top-level (in an anonymous namespace)? One
definition ought to be removed.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video_send_stream...
webrtc/video_send_stream.h:199: // if the encoder overshootor or the encoder can
not encode fast enough.
"s " missing in "overshoots or"

[perkj_webrtc, 2016-10-28 14:05:35]

Thanks Nisse. 

Ptal

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1696: // merged.
On 2016/10/28 10:53:44, nisse-webrtc wrote:
> That time is now. ;-)

Great- but it is a little bit more to it so I would like to do it in a separate
cl.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:341: bool IsResutionScalingEnabledLocked() const
EXCLUSIVE_LOCKS_REQUIRED(&crit_) {
On 2016/10/28 10:53:44, nisse-webrtc wrote:
> Spelling.

Done.

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:104: class TestBuffer : public
webrtc::I420Buffer {
On 2016/10/28 10:53:44, nisse-webrtc wrote:
> Isn't class TestBuffer defined at top-level (in an anonymous namespace)? One
> definition ought to be removed.

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video_send_stream...
webrtc/video_send_stream.h:199: // if the encoder overshootor or the encoder can
not encode fast enough.
On 2016/10/28 10:53:44, nisse-webrtc wrote:
> "s " missing in "overshoots or"

Done.

[nisse-webrtc, 2016-10-31 07:52:45]

nisse@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2016-10-31 07:52:46]

LGTM.

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1696: // merged.
On 2016/10/28 14:05:34, perkj_webrtc wrote:
> On 2016/10/28 10:53:44, nisse-webrtc wrote:
> > That time is now. ;-)
> 
> Great- but it is a little bit more to it so I would like to do it in a
separate
> cl.

Sure. I'd also really like to see this change landed sooner than later. Maybe
update TODO comment, though?

[åsapersson, 2016-10-31 08:43:02]

lgtm with nits

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2918: EXPECT_EQ(2,
info.senders[0].adapt_changes);
2 -> stats.number_of_cpu_adapt_changes

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2935: EXPECT_EQ(2,
info.senders[0].adapt_changes);
ditto

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:268: rtc::VideoSinkWants
FrameForwarder::current_sink_wants() const {
sink_wants()

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:746: }) == config_->rtp.extensions.end();
Maybe add a test in video_send_stream_tests.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:461: // ViEEncoder::NormalUsage and
ViEEncoder::Overusedetected.
OveruseDetected and see spacing

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:80: // |degradation_preference| control weather or
not resolution may reduced.
whether

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:433: frame_width * frame_height);
will pass if not set?

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:440: // Trigger CPU overuse a one more
time. This should not trigger a request for
remove "a"

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:494: sink_.WaitForEncodedFrame(3);
maybe 2

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:510:
vie_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
why is OnBitrateUpdated needed?

[perkj_webrtc, 2016-10-31 19:45:19]

Thanks Åsa. Would you mind taking a look at the added test as well? 

sprang, mflodman?

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2918: EXPECT_EQ(2,
info.senders[0].adapt_changes);
On 2016/10/31 08:43:01, åsapersson wrote:
> 2 -> stats.number_of_cpu_adapt_changes

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2935: EXPECT_EQ(2,
info.senders[0].adapt_changes);
On 2016/10/31 08:43:02, åsapersson wrote:
> ditto

Done.

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:268: rtc::VideoSinkWants
FrameForwarder::current_sink_wants() const {
On 2016/10/31 08:43:02, åsapersson wrote:
> sink_wants()

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:746: }) == config_->rtp.extensions.end();
On 2016/10/31 08:43:02, åsapersson wrote:
> Maybe add a test in video_send_stream_tests.

Done.
Added test that verifies that the rotation SinkWants are properly set.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:461: // ViEEncoder::NormalUsage and
ViEEncoder::Overusedetected.
On 2016/10/31 08:43:02, åsapersson wrote:
> OveruseDetected and see spacing

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:80: // |degradation_preference| control weather or
not resolution may reduced.
On 2016/10/31 08:43:02, åsapersson wrote:
> whether

nice spelling ! thanks

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

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:433: frame_width * frame_height);
On 2016/10/31 08:43:02, åsapersson wrote:
> will pass if not set?

that is not so good. Good catch.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:440: // Trigger CPU overuse a one more
time. This should not trigger a request for
On 2016/10/31 08:43:02, åsapersson wrote:
> remove "a"

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:494: sink_.WaitForEncodedFrame(3);
On 2016/10/31 08:43:02, åsapersson wrote:
> maybe 2

Done.

https://codereview.webrtc.org/2304363002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:510:
vie_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
On 2016/10/31 08:43:02, åsapersson wrote:
> why is OnBitrateUpdated needed?

The encoder will not produce an output unless there is a target bitrate. Bitrate
0 means paused.

[åsapersson, 2016-11-01 08:04:55]

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:2917: }
Maybe also add a test that verifies the rtp header extension (something similar
to TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime)).

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

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:467: frame_width * frame_height);
make same change as above

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:489: frame_width * frame_height);
ditto

[perkj_webrtc, 2016-11-01 18:00:42]

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:2917: }
On 2016/11/01 08:04:55, åsapersson wrote:
> Maybe also add a test that verifies the rtp header extension (something
similar
> to TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime)).

ok, I did not add VideoRotation but I guess it does not hurt.

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

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:467: frame_width * frame_height);
On 2016/11/01 08:04:55, åsapersson wrote:
> make same change as above

Sorry I missed this.
Done.

https://codereview.webrtc.org/2304363002/diff/360001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:489: frame_width * frame_height);
On 2016/11/01 08:04:55, åsapersson wrote:
> ditto

Done.

[perkj_webrtc, 2016-11-01 18:01:29]

I am going to make an attempt at landing this now. 
mflodman, sprang, please take a look when you can.

[perkj_webrtc, 2016-11-01 18:01:49]

Description was changed from

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

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

to

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org
==========

[perkj_webrtc, 2016-11-01 18:02:00]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-11-01 18:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2304363002/#ps400001 (title:
"Add VideoSendStream test for video rotation extension.")

[commit-bot: I haz the power, 2016-11-01 18:02:08]

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-11-01 18:03:17]

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

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

Try jobs failed on following builders:
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12073)
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1969)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14426)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14313)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19658)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18347)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

[perkj_webrtc, 2016-11-01 18:19:38]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-11-01 18:19:39]

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

[commit-bot: I haz the power, 2016-11-01 18:19:49]

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-11-01 18:45:50]

Description was changed from

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org
==========

to

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org
==========

[commit-bot: I haz the power, 2016-11-01 18:45:51]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-11-01 19:01:55]

Description was changed from

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface. 

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org
==========

to

==========
Let ViEEncoder express resolution requests as Sinkwants.
This removes the VideoSendStream::LoadObserver interface and the implementation
in WebrtcVideoSendStream and replace it with VideoSinkWants through the
VideoSourceInterface.

To do that that, some stats for CPU adaptation is moved into VideoSendStream.
Also handling of the CVO rtp header extension is moved to VideoSendStreamImpl.

BUG=webrtc:5687
TBR=mflodman@webrtc.org

Committed: https://crrev.com/803d97f15937f0aba79043b8d92d8a66dfa31cfb
Cr-Commit-Position: refs/heads/master@{#14877}
==========

[commit-bot: I haz the power, 2016-11-01 19:01:57]

Patchset 22 (id:??) landed as
https://crrev.com/803d97f15937f0aba79043b8d92d8a66dfa31cfb
Cr-Commit-Position: refs/heads/master@{#14877}