Add framerate to VideoSinkWants and ability to signal on overuse

Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=webrtc:4172
Review-Url: https://codereview.webrtc.org/2716643002
Cr-Commit-Position: refs/heads/master@{#17327}
Committed: https://chromium.googlesource.com/external/webrtc/+/72acf2526177bb4dbb5103cd6e165eb4361a5ae6

[sprang_webrtc, 2017-02-23 17:02:09]

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

[commit-bot: I haz the power, 2017-02-23 17:02:14]

Dry run: 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, 2017-02-23 17:07:07]

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

[commit-bot: I haz the power, 2017-02-23 17:07:10]

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

[sprang_webrtc, 2017-02-26 12:07:36]

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

[commit-bot: I haz the power, 2017-02-26 12:07:43]

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

[sprang_webrtc, 2017-02-26 12:08:16]

Description was changed from

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

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

to

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=4172
==========

[sprang_webrtc, 2017-02-26 12:08:29]

Description was changed from

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=4172
==========

to

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=webrtc:4172
==========

[sprang_webrtc, 2017-02-26 12:10:53]

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

[sprang_webrtc, 2017-02-26 12:10:54]

Please have a look.
I'll be adding additional tests shortly.

[commit-bot: I haz the power, 2017-02-26 12:27:13]

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

[commit-bot: I haz the power, 2017-02-26 12:27:15]

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

[magjed_webrtc, 2017-02-27 09:30:35]

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: if (std::numeric_limits<int>::max() != 0)
{
I think this should be 'max_framerate_request_ != 0' instead. Also, if
'max_framerate_request_ == 0', shouldn't we always return false?

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster_unittest.cc:133:
EXPECT_TRUE(!broadcaster.wants().max_framerate_fps_);
Use EXPECT_FALSE instead and remove the negation?

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:235: if (framerate_fps < kMinFramerateFps)
Shouldn't it be 'framerate_wanted < kMinFramerateFps'?

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:804: // Don't request lower resolution if the
current resolution is not
Reflow comment.

[magjed_webrtc, 2017-02-27 09:58:30]

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

[magjed_webrtc, 2017-02-27 09:58:30]

I'm adding Kári as reviewer as well.

[sprang_webrtc, 2017-02-27 12:51:49]

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: if (std::numeric_limits<int>::max() != 0)
{
On 2017/02/27 09:30:35, magjed_webrtc wrote:
> I think this should be 'max_framerate_request_ != 0' instead. Also, if
> 'max_framerate_request_ == 0', shouldn't we always return false?

Whoops. That should be 
max_framerate_request_ != std::numeric_limits<int>::max()
bad copy/paste screwup... These are the tests I mentioned are coming :)
And yes, should return false if max_framerate_request_ <= 0

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster_unittest.cc:133:
EXPECT_TRUE(!broadcaster.wants().max_framerate_fps_);
On 2017/02/27 09:30:35, magjed_webrtc wrote:
> Use EXPECT_FALSE instead and remove the negation?

Done.

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:235: if (framerate_fps < kMinFramerateFps)
On 2017/02/27 09:30:35, magjed_webrtc wrote:
> Shouldn't it be 'framerate_wanted < kMinFramerateFps'?

I think I intended it to be framerate_fps, but placed it badly. Though maybe it
makes more sense to actually restrict base on framerate_wanted. I'll change.

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:804: // Don't request lower resolution if the
current resolution is not
On 2017/02/27 09:30:35, magjed_webrtc wrote:
> Reflow comment.

Done.

[kthelgason, 2017-02-27 14:02:39]

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster_unittest.cc:13: //#include
"webrtc/base/gunit.h"
I guess this should not have been included?

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:37: 
nit: unnecessary empty line

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.h:223: std::map<size_t, std::vector<int>>
scale_counter_ ACCESS_ON(&encoder_queue_);
Why not have this a map from DegradationPreference -> std::vector? 
It's clearer and saves on a cast.

Can we make the container const since the number of entries is known at
compile-time?

[sprang_webrtc, 2017-02-27 19:28:05]

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

[commit-bot: I haz the power, 2017-02-27 19:28:15]

Dry run: 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, 2017-02-27 20:37:31]

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

[commit-bot: I haz the power, 2017-02-27 20:37:31]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-02-28 08:46:28]

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
File webrtc/media/base/videobroadcaster_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
webrtc/media/base/videobroadcaster_unittest.cc:13: //#include
"webrtc/base/gunit.h"
On 2017/02/27 14:02:39, kthelgason wrote:
> I guess this should not have been included?

Oops. Removed

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:37: 
On 2017/02/27 14:02:39, kthelgason wrote:
> nit: unnecessary empty line

Done.

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.h:223: std::map<size_t, std::vector<int>>
scale_counter_ ACCESS_ON(&encoder_queue_);
On 2017/02/27 14:02:39, kthelgason wrote:
> Why not have this a map from DegradationPreference -> std::vector? 
> It's clearer and saves on a cast.
> 
> Can we make the container const since the number of entries is known at
> compile-time?

Hm, when I tried this I kept getting giant obscure error messages so I switched
to ints, but now it seems to work fine for some reason. Wonder what I did...

The container itself can't be const as then we can't mutate the the vectors
within. Or am I missing something?

[kthelgason, 2017-02-28 09:09:10]

On 2017/02/28 08:46:28, språng wrote:
>
https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
> File webrtc/media/base/videobroadcaster_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videobr...
> webrtc/media/base/videobroadcaster_unittest.cc:13: //#include
> "webrtc/base/gunit.h"
> On 2017/02/27 14:02:39, kthelgason wrote:
> > I guess this should not have been included?
> 
> Oops. Removed
> 
>
https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
> File webrtc/media/base/videosourceinterface.h (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/20001/webrtc/media/base/videoso...
> webrtc/media/base/videosourceinterface.h:37: 
> On 2017/02/27 14:02:39, kthelgason wrote:
> > nit: unnecessary empty line
> 
> Done.
> 
> https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.h
> File webrtc/video/vie_encoder.h (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
> webrtc/video/vie_encoder.h:223: std::map<size_t, std::vector<int>>
> scale_counter_ ACCESS_ON(&encoder_queue_);
> On 2017/02/27 14:02:39, kthelgason wrote:
> > Why not have this a map from DegradationPreference -> std::vector? 
> > It's clearer and saves on a cast.
> > 
> > Can we make the container const since the number of entries is known at
> > compile-time?
> 
> Hm, when I tried this I kept getting giant obscure error messages so I
switched
> to ints, but now it seems to work fine for some reason. Wonder what I did...
> 
> The container itself can't be const as then we can't mutate the the vectors
> within. Or am I missing something?

Thanks Erik. LGTM.

[magjed_webrtc, 2017-02-28 14:21:55]

lgtm

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:235: if (framerate_fps < kMinFramerateFps)
On 2017/02/27 12:51:49, språng wrote:
> On 2017/02/27 09:30:35, magjed_webrtc wrote:
> > Shouldn't it be 'framerate_wanted < kMinFramerateFps'?
> 
> I think I intended it to be framerate_fps, but placed it badly. Though maybe
it
> makes more sense to actually restrict base on framerate_wanted. I'll change.

Hmm, maybe just do:
framerate_wanted = max(kMinFramerateFps, (framerate_fps * 2) / 3);
instead?

https://codereview.webrtc.org/2716643002/diff/40001/webrtc/video/vie_encoder_...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/40001/webrtc/video/vie_encoder_...
webrtc/video/vie_encoder_unittest.cc:1299: // TODO(sprang): Find where there's
roundign errors or stuff causing the
spelling nit: rounding
Also, this comment is a little bit worrying :/

[sprang_webrtc, 2017-02-28 15:14:18]

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

[commit-bot: I haz the power, 2017-02-28 15:14:28]

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

[sprang_webrtc, 2017-02-28 15:15:30]

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:235: if (framerate_fps < kMinFramerateFps)
On 2017/02/28 14:21:55, magjed_webrtc wrote:
> On 2017/02/27 12:51:49, språng wrote:
> > On 2017/02/27 09:30:35, magjed_webrtc wrote:
> > > Shouldn't it be 'framerate_wanted < kMinFramerateFps'?
> > 
> > I think I intended it to be framerate_fps, but placed it badly. Though maybe
> it
> > makes more sense to actually restrict base on framerate_wanted. I'll change.
> 
> Hmm, maybe just do:
> framerate_wanted = max(kMinFramerateFps, (framerate_fps * 2) / 3);
> instead?

Done.

https://codereview.webrtc.org/2716643002/diff/40001/webrtc/video/vie_encoder_...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2716643002/diff/40001/webrtc/video/vie_encoder_...
webrtc/video/vie_encoder_unittest.cc:1299: // TODO(sprang): Find where there's
roundign errors or stuff causing the
On 2017/02/28 14:21:55, magjed_webrtc wrote:
> spelling nit: rounding
> Also, this comment is a little bit worrying :/

Done. And yes :(

[commit-bot: I haz the power, 2017-02-28 15:40:44]

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

[commit-bot: I haz the power, 2017-02-28 15:40:46]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-08 08:08:54]

Please consider dropping optional where not really needed. I trust your
judgement on that.

LGTM.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: if (requested_format_ &&
requested_format_->interval)
The second part of the conditioal seems unnecessary. I think i'd write it as

int64_t frame_interval_ns = request_format_ ? request_format_->interval : 0;

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:126: frame_interval_ns = std::max<int64_t>(
Can we make this part unconditional? We ought to have INTMAX >
kNumNanosecsPerSec, so result should be the same, second argument of std::max
being zero.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.h:28: explicit VideoAdapter(int
required_resolution_alignment);
Not your change, but this argument deserves a comment.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:38: rtc::Optional<int>
max_framerate_fps_;
Consider changing it to non-optional with default value,

  int max_framerate_fps_ = std::numeric_limits<int>::max();

Could do the same to max_pixel_count, of course.

That would simplify the logic in several places.

[kthelgason, 2017-03-08 08:15:59]

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:38: rtc::Optional<int>
max_framerate_fps_;
On 2017/03/08 08:08:54, nisse-webrtc (ooo March 6) wrote:
> Consider changing it to non-optional with default value,
> 
>   int max_framerate_fps_ = std::numeric_limits<int>::max();
> 
> Could do the same to max_pixel_count, of course.
> 
> That would simplify the logic in several places.

I endorse this comment!

[sprang_webrtc, 2017-03-09 13:19:34]

A sneaked in a little bit of test code after the lgtm and also changed the
behavior of AdaptUp. Might wanna take another pass

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: if (requested_format_ &&
requested_format_->interval)
On 2017/03/08 08:08:54, nisse-webrtc (ooo March 6) wrote:
> The second part of the conditioal seems unnecessary. I think i'd write it as
> 
> int64_t frame_interval_ns = request_format_ ? request_format_->interval : 0;

Done.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:126: frame_interval_ns = std::max<int64_t>(
On 2017/03/08 08:08:54, nisse-webrtc (ooo March 6) wrote:
> Can we make this part unconditional? We ought to have INTMAX >
> kNumNanosecsPerSec, so result should be the same, second argument of std::max
> being zero.

Sure, but I'll add a comment about that as it's a little less readable then imo.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.h:28: explicit VideoAdapter(int
required_resolution_alignment);
On 2017/03/08 08:08:54, nisse-webrtc (ooo March 6) wrote:
> Not your change, but this argument deserves a comment.

Done.

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2716643002/diff/60001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:38: rtc::Optional<int>
max_framerate_fps_;
On 2017/03/08 08:08:54, nisse-webrtc (ooo March 6) wrote:
> Consider changing it to non-optional with default value,
> 
>   int max_framerate_fps_ = std::numeric_limits<int>::max();
> 
> Could do the same to max_pixel_count, of course.
> 
> That would simplify the logic in several places.

It might simplify it in some places, like videobroadcaster.
It will make it uglier in others, where we'll have to change
  if (!foo.max_framerate_fps_)
to
  if (foo_max_framerate_fps_ == std::numeric_limits<int>::max())

I suggest we save this for an impending overhaul of the entire struct.

[sprang_webrtc, 2017-03-09 13:21:39]

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

[commit-bot: I haz the power, 2017-03-09 13:21:46]

Dry run: 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, 2017-03-09 13:25:36]

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

[commit-bot: I haz the power, 2017-03-09 13:25:37]

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

[nisse-webrtc, 2017-03-09 13:32:59]

On 2017/03/09 13:19:34, språng wrote:
> It will make it uglier in others, where we'll have to change
>   if (!foo.max_framerate_fps_)
> to
>   if (foo_max_framerate_fps_ == std::numeric_limits<int>::max())

Where do you need the latter comparison? I would expect 

  !max_framerate_fps_ 

to have exactly the same effect as 

  *max_framerate_fps == std::numeric_limits<int>::max().

And then any special casing would need to check both conditions.

> I suggest we save this for an impending overhaul of the entire struct.

Ok with me.

[nisse-webrtc, 2017-03-09 13:33:18]

LGTM.

https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer....
webrtc/test/vcm_capturer.cc:115: int cropped_width = 0;
Adding adaptation here makes some sense. Just curious why is it needed for this
cl?

[sprang_webrtc, 2017-03-09 13:36:22]

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

[commit-bot: I haz the power, 2017-03-09 13:36:29]

Dry run: 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, 2017-03-09 13:39:23]

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

[commit-bot: I haz the power, 2017-03-09 13:39:25]

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

[sprang_webrtc, 2017-03-09 13:42:26]

https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer....
webrtc/test/vcm_capturer.cc:115: int cropped_width = 0;
On 2017/03/09 13:33:18, nisse-webrtc (ooo March 6) wrote:
> Adding adaptation here makes some sense. Just curious why is it needed for
this
> cl?

It's not technically. I just that now we have usage of the code in this CL for
both the end-to-end perf tests and manual tests, in addition to unit tests. The
more coverage the better?

[sprang_webrtc, 2017-03-09 13:42:38]

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

[commit-bot: I haz the power, 2017-03-09 13:42:52]

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

[nisse-webrtc, 2017-03-09 13:51:20]

On 2017/03/09 13:42:26, språng wrote:
>
https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer.cc
> File webrtc/test/vcm_capturer.cc (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/80001/webrtc/test/vcm_capturer....
> webrtc/test/vcm_capturer.cc:115: int cropped_width = 0;
> On 2017/03/09 13:33:18, nisse-webrtc (ooo March 6) wrote:
> > Adding adaptation here makes some sense. Just curious why is it needed for
> this
> > cl?
> 
> It's not technically. I just that now we have usage of the code in this CL for
> both the end-to-end perf tests and manual tests, in addition to unit tests.
The
> more coverage the better?

It's not really clear to me where the various *Capturer classes are used.

But for test cases, beware that automatic adaptation can cause flakyness. See,
e.g., https://codereview.webrtc.org/2636903004

[commit-bot: I haz the power, 2017-03-09 13:57:39]

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

[commit-bot: I haz the power, 2017-03-09 13:57:40]

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

[sprang_webrtc, 2017-03-13 18:04:57]

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

[commit-bot: I haz the power, 2017-03-13 18:05:07]

Dry run: 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, 2017-03-13 18:11:56]

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

[commit-bot: I haz the power, 2017-03-13 18:11:57]

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

[sprang_webrtc, 2017-03-14 08:24:31]

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

[commit-bot: I haz the power, 2017-03-14 08:24:38]

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

[sprang_webrtc, 2017-03-14 08:27:16]

I've done some further work here as I found a few issues. One of them is that
maintain-resolution was treated as "cpu adaptation off". That will no longer be
the case, so I've added explicit disabled option. Also, changed balanced into
maintain-framerate and made balanced as alias for that.
Plus a bunch of new tests and a few bugfixes.

You'll want to take another pass.
I realize this cl has now grown precariously, but it's gonna be tricky to split
cleanly into smaller patches :/

[commit-bot: I haz the power, 2017-03-14 08:48:25]

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

[commit-bot: I haz the power, 2017-03-14 08:48:27]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-03-14 09:00:29]

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.cc:269: const rtc::Optional<int>& framerate_fps)
{
Even if you want to keep optional in the SinkWants struct for now,
please drop it from the two max values here, and move the value_or(INT_MAX)
logic to the caller.

Use of optional for const function arguments is discouraged, so at some point
you may want to change target_pixel_count to const int* too, but that's a
borderline case.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:111: framerate_scaling_enalbed_(false),
Spelling.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:256: switch (degradation_preference) {
This is only ever called once in the object's lifetime?

Otherwise, you'd need to assign both flags in all cases.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:172: bool framerate_scaling_enalbed_;
Spelling.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:41: using DegradationPreference =
webrtc::VideoSendStream::DegradationPreference;
Perhaps not for this cl, but if DegradationPreference is intended as a public
high-level API, it should perhaps be in the top-level webrtc namespace.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1643: degradation_preference =
DegradationPreference::kDegradationDisabled;
I wonder if this disable value should be made available to applications, all the
way up to js? I don't think the name is quite right in that context.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2101: ?
webrtc::VideoSendStream::DegradationPreference::kMaintainFramerate
Why is this logic different from logic in SetVideoSend, above?

It would be clearer if the logic were the same, and common code extracted to a
helper method.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:238: int framerate_fps_;
How is that measured? Or is it requested/configured fps?

[kthelgason, 2017-03-14 10:01:21]

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1632:
webrtc::VideoSendStream::DegradationPreference::kDegradationDisabled);
just DegradationPreference::kDegradationDisabled given the using directive
above.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:109: int64_t sleep_time_ms;
can this be initialized here, rather than further down in the method?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:122: int cropped_height = 0;
these look unused

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:152: tick_->Wait(sleep_time_ms);
the use of `tick` is behind a lock in `Init` but not here. Is the lock
necessary?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/vcm_capturer...
webrtc/test/vcm_capturer.cc:132: }
Is it possible to share this logic somewhere so we can avoid having it in
multiple places? Maybe in webrtc/test/video_capturer.h or some other common
place.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:236: rtc::Optional<std::pair<int, int>>
overuse_periods;
Why does this variable exist? Seems to me that the std::pair is enough and could
be used directly.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:253: instance.reset(new
OverdoseInjector(options, overuse_periods->first,
This condidional is unnecessary. `instance.reset` can be moved in to the block
above and the else can follow that.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:190: // If clear any constraints from the current
sink wants that don't apply to
I guess this `if` should not be here?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:259: // Remove any restrains.
nit: constraints.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:896: int* scale_counter =
&((*GetScaleCounters())[reason]);
I'd prefer this to just be an int and then the vector updated afterwards with
the new value. It's easier to reason about when it's clear when the counters are
being updated.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:984: std::vector<int>*
ViEEncoder::GetScaleCounters() {
Can we have this return a `std::vector<int>&` instead? It seems like we are
having to do a lot of dereferencing in the code.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:988: return GetScaleCounters();
Is there a reason for not just returning the newly inserted vector?

[sprang_webrtc, 2017-03-14 14:14:32]

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

[commit-bot: I haz the power, 2017-03-14 14:14:39]

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

[sprang_webrtc, 2017-03-14 14:15:03]

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.cc:269: const rtc::Optional<int>& framerate_fps)
{
On 2017/03/14 09:00:28, nisse-webrtc wrote:
> Even if you want to keep optional in the SinkWants struct for now,
> please drop it from the two max values here, and move the value_or(INT_MAX)
> logic to the caller.
> 
> Use of optional for const function arguments is discouraged, so at some point
> you may want to change target_pixel_count to const int* too, but that's a
> borderline case.

Removed optional everywhere for max_pixel and fps. Kept target as optional for
now.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:111: framerate_scaling_enalbed_(false),
On 2017/03/14 09:00:28, nisse-webrtc wrote:
> Spelling.

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:256: switch (degradation_preference) {
On 2017/03/14 09:00:28, nisse-webrtc wrote:
> This is only ever called once in the object's lifetime?
> 
> Otherwise, you'd need to assign both flags in all cases.

Right, thanks. I misread this code.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:172: bool framerate_scaling_enalbed_;
On 2017/03/14 09:00:29, nisse-webrtc wrote:
> Spelling.

Done.

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

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:41: using DegradationPreference =
webrtc::VideoSendStream::DegradationPreference;
On 2017/03/14 09:00:29, nisse-webrtc wrote:
> Perhaps not for this cl, but if DegradationPreference is intended as a public
> high-level API, it should perhaps be in the top-level webrtc namespace.

Agreed. I think we need to overhaul a few pieces of our api's. But that usually
get's messy, so let's do it in something smaller and more easily revertable :)

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1632:
webrtc::VideoSendStream::DegradationPreference::kDegradationDisabled);
On 2017/03/14 10:01:20, kthelgason wrote:
> just DegradationPreference::kDegradationDisabled given the using directive
> above.

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1643: degradation_preference =
DegradationPreference::kDegradationDisabled;
On 2017/03/14 09:00:29, nisse-webrtc wrote:
> I wonder if this disable value should be made available to applications, all
the
> way up to js? I don't think the name is quite right in that context. 

|enable_cpu_overuse_detection_| is currently populated from js as a media
constraint I believe. I'm just making a simple translation here. For ortc I
guess we wan't to just use degradation preference, right?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:2101: ?
webrtc::VideoSendStream::DegradationPreference::kMaintainFramerate
On 2017/03/14 09:00:29, nisse-webrtc wrote:
> Why is this logic different from logic in SetVideoSend, above?
> 
> It would be clearer if the logic were the same, and common code extracted to a
> helper method.
> 

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:109: int64_t sleep_time_ms;
On 2017/03/14 10:01:20, kthelgason wrote:
> can this be initialized here, rather than further down in the method?

Need to hold the lock then, so made sense to me to do it closer to where it's
used below, no?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:122: int cropped_height = 0;
On 2017/03/14 10:01:20, kthelgason wrote:
> these look unused

They're out params from video_adpter_->AdaptFrameResolution().
I guess I can get away with just the one, but not sure it helps readability?

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:152: tick_->Wait(sleep_time_ms);
On 2017/03/14 10:01:20, kthelgason wrote:
> the use of `tick` is behind a lock in `Init` but not here. Is the lock
> necessary?

The lock is for |target_fps_|, not |tick_|.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/vcm_capturer.cc
File webrtc/test/vcm_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/test/vcm_capturer...
webrtc/test/vcm_capturer.cc:132: }
On 2017/03/14 10:01:20, kthelgason wrote:
> Is it possible to share this logic somewhere so we can avoid having it in
> multiple places? Maybe in webrtc/test/video_capturer.h or some other common
> place.

Done. Also did some general cleanup in this class since I didn't feel the cl was
quite big enough yet.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:236: rtc::Optional<std::pair<int, int>>
overuse_periods;
On 2017/03/14 10:01:20, kthelgason wrote:
> Why does this variable exist? Seems to me that the std::pair is enough and
could
> be used directly.

This was a bit of a rush job. The complexity here is that there are so many
points in which the parameter parsing can fail. I just switched to two plain
variables and simple set instance below if it hadn't been set to a over version.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:253: instance.reset(new
OverdoseInjector(options, overuse_periods->first,
On 2017/03/14 10:01:20, kthelgason wrote:
> This condidional is unnecessary. `instance.reset` can be moved in to the block
> above and the else can follow that.

Acknowledged.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:190: // If clear any constraints from the current
sink wants that don't apply to
On 2017/03/14 10:01:20, kthelgason wrote:
> I guess this `if` should not be here?

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:259: // Remove any restrains.
On 2017/03/14 10:01:20, kthelgason wrote:
> nit: constraints.

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:896: int* scale_counter =
&((*GetScaleCounters())[reason]);
On 2017/03/14 10:01:20, kthelgason wrote:
> I'd prefer this to just be an int and then the vector updated afterwards with
> the new value. It's easier to reason about when it's clear when the counters
are
> being updated.

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:984: std::vector<int>*
ViEEncoder::GetScaleCounters() {
On 2017/03/14 10:01:20, kthelgason wrote:
> Can we have this return a `std::vector<int>&` instead? It seems like we are
> having to do a lot of dereferencing in the code.

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:988: return GetScaleCounters();
On 2017/03/14 10:01:20, kthelgason wrote:
> Is there a reason for not just returning the newly inserted vector?

Done.

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2716643002/diff/150001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:238: int framerate_fps_;
On 2017/03/14 09:00:29, nisse-webrtc wrote:
> How is that measured? Or is it requested/configured fps?

This is the measured input fps to the encoder, the same value that is used to
update the encoder instance.

[commit-bot: I haz the power, 2017-03-14 14:43:05]

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

[commit-bot: I haz the power, 2017-03-14 14:43:06]

Dry run: This issue passed the CQ dry run.

[kthelgason, 2017-03-15 09:24:59]

This CL is getting quite unwieldy! I like the newest patchset though, it LQGTM.

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:948: scale_sum += i;
small nit: I prefer const int sum = std::accumulate(v.begin(), v.end(), 0),
fewer lines and one less mutable variable.

[kthelgason, 2017-03-15 09:25:15]

On 2017/03/15 09:24:59, kthelgason wrote:
> This CL is getting quite unwieldy! I like the newest patchset though, it
LQGTM.
> 
>
https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder.cc
> File webrtc/video/vie_encoder.cc (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder...
> webrtc/video/vie_encoder.cc:948: scale_sum += i;
> small nit: I prefer const int sum = std::accumulate(v.begin(), v.end(), 0),
> fewer lines and one less mutable variable.

lgtm

[nisse-webrtc, 2017-03-15 09:45:23]

lgtm % nits

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:56: // |max_pixel_count|. If |max_pixel_count|
is not set, treat is as being the
Update comment on max_pixel_cpunt.

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:63: int framerate_fps);
Should it be max_framerate_fps? Or is the semantics closer to
target_framerate_fps?

[sprang_webrtc, 2017-03-20 09:40:46]

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

[commit-bot: I haz the power, 2017-03-20 09:40:50]

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

[sprang_webrtc, 2017-03-20 09:41:29]

sprang@webrtc.org changed reviewers:
+ ilnik@webrtc.org

[sprang_webrtc, 2017-03-20 09:41:31]

+ilnik for webrtc/test/frame_generator_capturer.* as I rebased on top of recent
refactorings there.

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:56: // |max_pixel_count|. If |max_pixel_count|
is not set, treat is as being the
On 2017/03/15 09:45:22, nisse-webrtc wrote:
> Update comment on max_pixel_cpunt.

Done.

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:63: int framerate_fps);
On 2017/03/15 09:45:22, nisse-webrtc wrote:
> Should it be max_framerate_fps? Or is the semantics closer to
> target_framerate_fps?

It should say max.

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2716643002/diff/170001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:948: scale_sum += i;
On 2017/03/15 09:24:59, kthelgason wrote:
> small nit: I prefer const int sum = std::accumulate(v.begin(), v.end(), 0),
> fewer lines and one less mutable variable.

Done.

[commit-bot: I haz the power, 2017-03-20 09:44:19]

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

[commit-bot: I haz the power, 2017-03-20 09:44:20]

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

[nisse-webrtc, 2017-03-20 09:49:41]

One more comment comment. Still lgtm.

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:56: // |framerate_fps| is essentially analogous
to |max_pixel_count|, but for
|max_framerate_fps| also in comment...

[ilnik, 2017-03-20 10:02:52]

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:77: return repeat_interval_ms_ == 0;
There is now a memory leak possibility. If a repeating task did not repost
itself because requested frame rate changed, it will not be removed. Either do
not create a new task above, or adjust a return value here, please.

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:210:
wanted_fps_.emplace(wants.max_framerate_fps);
May it happen that |wants.max_framerate_fps| may be less than |target_fps_|, but
later be larger than it? Issue here is that |wanted_fps_| will not be updated
and frame generator will continue to always generate less fps than it could.

[sprang_webrtc, 2017-03-20 10:18:38]

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

[sprang_webrtc, 2017-03-20 10:18:45]

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
File webrtc/media/base/videoadapter.h (right):

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
webrtc/media/base/videoadapter.h:56: // |framerate_fps| is essentially analogous
to |max_pixel_count|, but for
On 2017/03/20 09:49:41, nisse-webrtc wrote:
> |max_framerate_fps| also in comment...

Done.

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:77: return repeat_interval_ms_ == 0;
On 2017/03/20 10:02:52, ilnik wrote:
> There is now a memory leak possibility. If a repeating task did not repost
> itself because requested frame rate changed, it will not be removed. Either do
> not create a new task above, or adjust a return value here, please.

Good catch! Done.

https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:210:
wanted_fps_.emplace(wants.max_framerate_fps);
On 2017/03/20 10:02:52, ilnik wrote:
> May it happen that |wants.max_framerate_fps| may be less than |target_fps_|,
but
> later be larger than it? Issue here is that |wanted_fps_| will not be updated
> and frame generator will continue to always generate less fps than it could.

You're right. I botched the rebase here. Fixed.

[commit-bot: I haz the power, 2017-03-20 10:18:50]

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

[ilnik, 2017-03-20 10:21:18]

On 2017/03/20 10:18:45, språng wrote:
>
https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
> File webrtc/media/base/videoadapter.h (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/210001/webrtc/media/base/videoa...
> webrtc/media/base/videoadapter.h:56: // |framerate_fps| is essentially
analogous
> to |max_pixel_count|, but for
> On 2017/03/20 09:49:41, nisse-webrtc wrote:
> > |max_framerate_fps| also in comment...
> 
> Done.
> 
>
https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
> File webrtc/test/frame_generator_capturer.cc (right):
> 
>
https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
> webrtc/test/frame_generator_capturer.cc:77: return repeat_interval_ms_ == 0;
> On 2017/03/20 10:02:52, ilnik wrote:
> > There is now a memory leak possibility. If a repeating task did not repost
> > itself because requested frame rate changed, it will not be removed. Either
do
> > not create a new task above, or adjust a return value here, please.
> 
> Good catch! Done.
> 
>
https://codereview.webrtc.org/2716643002/diff/210001/webrtc/test/frame_genera...
> webrtc/test/frame_generator_capturer.cc:210:
> wanted_fps_.emplace(wants.max_framerate_fps);
> On 2017/03/20 10:02:52, ilnik wrote:
> > May it happen that |wants.max_framerate_fps| may be less than |target_fps_|,
> but
> > later be larger than it? Issue here is that |wanted_fps_| will not be
updated
> > and frame generator will continue to always generate less fps than it could.
> 
> You're right. I botched the rebase here. Fixed.

LGTM

[commit-bot: I haz the power, 2017-03-20 10:40:13]

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

[commit-bot: I haz the power, 2017-03-20 10:40:14]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-03-20 10:44:02]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-03-20 10:44:02]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
kthelgason@webrtc.org, nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2716643002/#ps230001 (title:
"Addressed comments, fixed missing include")

[commit-bot: I haz the power, 2017-03-20 10:44:13]

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, 2017-03-20 10:48:42]

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

[commit-bot: I haz the power, 2017-03-20 10:48:44]

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

[sprang_webrtc, 2017-03-20 11:40:16]

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

[sprang_webrtc, 2017-03-20 11:40:17]

+stefan as owner for
    webrtc/call/*
    webrtc/test/*
    webrtc/video_send_stream.h

[stefan-webrtc, 2017-03-21 15:37:36]

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

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:175:
VideoSendStream::DegradationPreference::kMaintainFramerate);
Isn't balanced a better default, or why do we change it?

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

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:503: } else if (wants.max_pixel_count <
std::numeric_limits<int>::max()) {
It's not clear to me what this change means. Can you explain?

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:43: // task queue is the same same as
the current configures frame rate.
configured

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/video_captur...
File webrtc/test/video_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/video_captur...
webrtc/test/video_capturer.cc:32: }
Looks like we are adapting the resolution here, and then scaling below, is that
because we crop here? Shouldn't the method say so in that case, or at least
maybe we should have a comment?

[sprang_webrtc, 2017-03-21 15:59:18]

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

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:175:
VideoSendStream::DegradationPreference::kMaintainFramerate);
On 2017/03/21 15:37:36, stefan-webrtc wrote:
> Isn't balanced a better default, or why do we change it?

Balanced was never balanced. It was maintain-framerate (ie scale down on overuse
detection). I've thus renamed it everywhere. When we introduce a balanced mode
we will probably want to evaluate it before making it the default.

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

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:503: } else if (wants.max_pixel_count <
std::numeric_limits<int>::max()) {
On 2017/03/21 15:37:36, stefan-webrtc wrote:
> It's not clear to me what this change means. Can you explain?

max_pixel_count is now a plain int instead of an rtc::Optional<int>
If no max is desired, it is set to maxint.

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:43: // task queue is the same same as
the current configures frame rate.
On 2017/03/21 15:37:36, stefan-webrtc wrote:
> configured

Done.

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/video_captur...
File webrtc/test/video_capturer.cc (right):

https://codereview.webrtc.org/2716643002/diff/230001/webrtc/test/video_captur...
webrtc/test/video_capturer.cc:32: }
On 2017/03/21 15:37:36, stefan-webrtc wrote:
> Looks like we are adapting the resolution here, and then scaling below, is
that
> because we crop here? Shouldn't the method say so in that case, or at least
> maybe we should have a comment?

No, if we return here out_frame is not set - ie drop frame. I'll add a comment.

[stefan-webrtc, 2017-03-21 16:13:52]

lgtm

[sprang_webrtc, 2017-03-21 16:39:26]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-03-21 16:39:27]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
nisse@webrtc.org, kthelgason@webrtc.org, ilnik@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2716643002/#ps250001 (title:
"Comments")

[commit-bot: I haz the power, 2017-03-21 16:39:33]

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, 2017-03-21 16:57:22]

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

[commit-bot: I haz the power, 2017-03-21 16:57:24]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22638)
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/18972)

[sprang_webrtc, 2017-03-21 18:38:23]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2017-03-21 18:38:34]

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, 2017-03-21 18:54:13]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1490121503404680,
"parent_rev": "38989e593c994b6865f2b2cc496abf59d5925eb8", "commit_rev":
"72acf2526177bb4dbb5103cd6e165eb4361a5ae6"}

[commit-bot: I haz the power, 2017-03-21 18:54:17]

Description was changed from

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=webrtc:4172
==========

to

==========
Add framerate to VideoSinkWants and ability to signal on overuse

In ViEEncoder, try to reduce framerate instead of resolution if the
current degradation preference is maintain-resolution rather than
balanced.

BUG=webrtc:4172

Review-Url: https://codereview.webrtc.org/2716643002
Cr-Commit-Position: refs/heads/master@{#17327}
Committed:
https://chromium.googlesource.com/external/webrtc/+/72acf2526177bb4dbb5103cd6...
==========

[commit-bot: I haz the power, 2017-03-21 18:54:19]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/external/webrtc/+/72acf2526177bb4dbb5103cd6...

[skvlad, 2017-03-21 20:22:33]

On 2017/03/21 18:54:19, commit-bot: I haz the power wrote:
> Committed patchset #14 (id:250001) as
>
https://chromium.googlesource.com/external/webrtc/+/72acf2526177bb4dbb5103cd6...

This change seems to be breaking multiple perf tests:
https://build.chromium.org/p/client.webrtc.perf/builders/Win7/builds/1679/ste...
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20...

[ RUN      ] CallPerfTest.ReceivesCpuOveruseAndUnderuse
../../webrtc/call/call_perf_tests.cc:520: Failure
Value of: Wait()
  Actual: false
Expected: true
Timed out before receiving an overuse callback.

[skvlad, 2017-03-21 20:25:45]

A revert of this CL (patchset #14 id:250001) has been created in
https://codereview.webrtc.org/2764133002/ by skvlad@webrtc.org.

The reason for reverting is: Breaks perf tests:
https://build.chromium.org/p/client.webrtc.perf/builders/Win7/builds/1679
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20....

[ilnik, 2017-03-22 08:42:04]

On 2017/03/21 20:25:45, skvlad wrote:
> A revert of this CL (patchset #14 id:250001) has been created in
> https://codereview.webrtc.org/2764133002/ by mailto:skvlad@webrtc.org.
> 
> The reason for reverting is: Breaks perf tests:
> https://build.chromium.org/p/client.webrtc.perf/builders/Win7/builds/1679
>
https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20....

Erik, I had problem with this test on windows and android also. Issue was caused
by not so precise FrameGenaratorCapturer. One explanation could be that your
changes broke timer-adjusting behavior inside the InsertFrameTask.