Set overuse detector max frame interval based on target frame rate.

Set overuse detector max frame interval based on target frame rate.

Currently there is a hard limit for the estimated captured frame
interval of 45ms. As the encoder utilization is calculated as
(input frame interval)/(encode time), overuse signals can be triggered
even though there is plenty of time to go around if the fps is low.

However, in order to avoid falsly estimating low encode usage in case
the capturer has a dynamic frame rate, set the frame interval based on
the actual current max framerate.

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

[sprang_webrtc, 2017-06-05 11:52:23]

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

[commit-bot: I haz the power, 2017-06-05 11:52: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-06-05 12:12:38]

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

[commit-bot: I haz the power, 2017-06-05 12:12:39]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-06-08 11:48:32]

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

[commit-bot: I haz the power, 2017-06-08 11:48:41]

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-06-08 11:50:31]

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

[commit-bot: I haz the power, 2017-06-08 11:50:32]

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

[sprang_webrtc, 2017-06-08 12:25:39]

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

[commit-bot: I haz the power, 2017-06-08 12:25:41]

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-06-08 12:26:25]

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

[sprang_webrtc, 2017-06-09 10:32:33]

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

[sprang_webrtc, 2017-06-09 10:32:34]

+kthelgason@ since asa is OOO

[kthelgason, 2017-06-12 11:56:48]

A few comments. This seems like a good idea to me!

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

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:408:
OnTargetFramerateUpdated(framerate_);
is this supposed to be the last framerate and not the default framerate?
If yes then what's the point of this call?

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:406: void ViEEncoder::UpdateOveruseDetector(
Is this only used in the test? I'd prefer we not make all these changes just to
make that possible. Just make it injectable in the ctor instead.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:899: int requested_framerate =
source_proxy_->RequestFramerateLowerThan(
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:980: int requested_framerate =
source_proxy_->RequestHigherFramerateThan(fps);
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:253: int max_framerate_ ACCESS_ON(&encoder_queue_);
we already store the VideoEncoderConfig. Isn't this info taken from there?

[åsapersson, 2017-06-13 12:15:09]

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

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:414: framerate_ = framerate_fps;
Add a max limit for framerate_ to 30?

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.h:145: int framerate_
GUARDED_BY(task_checker_);
maybe max_framerate_

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:373: int kCapturerMaxFrameRate =
30;
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:374: int kEncodeMaxFrameRate =
20;  // Maximum fps the encoder can sustain.
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:408: int kMinFrameRate = 7;  //
Minimum fps allowed by current detector impl.
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:444: int kMaxFrameRate = 20;
const

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:242: int wanted_framerate = (fps * 2) / 3;
nit: maybe framerate_wanted for consistency with RequestHigherFramerateThan

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:586: last_adaptation_request_->framerate_fps_ > 0) {
last_adaptation_request_->framerate_fps_ will not hold the requested framrate,
right? Holds the input fps before the request.

[sprang_webrtc, 2017-06-14 08:38:09]

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

[commit-bot: I haz the power, 2017-06-14 08:38:17]

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-06-14 08:39:17]

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

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:408:
OnTargetFramerateUpdated(framerate_);
On 2017/06/12 11:56:47, kthelgason wrote:
> is this supposed to be the last framerate and not the default framerate?
> If yes then what's the point of this call?

The state is reset if the resolution is changed, in which case we do not want to
change to the default framerate.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:414: framerate_ = framerate_fps;
On 2017/06/13 12:15:09, åsapersson wrote:
> Add a max limit for framerate_ to 30? 

Do we really want to? I think we at least support up to 60fps, right?
I'll add a dcheck here and maybe cap it in vie_encoder instead.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.h:145: int framerate_
GUARDED_BY(task_checker_);
On 2017/06/13 12:15:09, åsapersson wrote:
> maybe max_framerate_

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:373: int kCapturerMaxFrameRate =
30;
On 2017/06/13 12:15:09, åsapersson wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:374: int kEncodeMaxFrameRate =
20;  // Maximum fps the encoder can sustain.
On 2017/06/13 12:15:09, åsapersson wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:408: int kMinFrameRate = 7;  //
Minimum fps allowed by current detector impl.
On 2017/06/13 12:15:09, åsapersson wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:444: int kMaxFrameRate = 20;
On 2017/06/13 12:15:09, åsapersson wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:242: int wanted_framerate = (fps * 2) / 3;
On 2017/06/13 12:15:09, åsapersson wrote:
> nit: maybe framerate_wanted for consistency with RequestHigherFramerateThan

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:406: void ViEEncoder::UpdateOveruseDetector(
On 2017/06/12 11:56:47, kthelgason wrote:
> Is this only used in the test? I'd prefer we not make all these changes just
to
> make that possible. Just make it injectable in the ctor instead.

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:586: last_adaptation_request_->framerate_fps_ > 0) {
On 2017/06/13 12:15:09, åsapersson wrote:
> last_adaptation_request_->framerate_fps_ will not hold the requested framrate,
> right? Holds the input fps before the request.

Hm, you're right. Guess I'll have to add another field to the adaptation request
struct :/

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:899: int requested_framerate =
source_proxy_->RequestFramerateLowerThan(
On 2017/06/12 11:56:48, kthelgason wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:980: int requested_framerate =
source_proxy_->RequestHigherFramerateThan(fps);
On 2017/06/12 11:56:47, kthelgason wrote:
> const

Done.

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:253: int max_framerate_ ACCESS_ON(&encoder_queue_);
On 2017/06/12 11:56:48, kthelgason wrote:
> we already store the VideoEncoderConfig. Isn't this info taken from there?

You'd think so, but it actually doesn't (explicitly) hold the max framerate.
It's instead assigned by the VideoStreamFactoryInterface instance pointed to by
the VideoEncoderConfig. In the chrome case, that lived is the
EncoderStreamFactory in webrtcvideoengine2, and its maximum is set at
construction time from a constraint.

The alternative is to store the entire std::vector<webrtc::VideoStream> returned
by the factory...

[commit-bot: I haz the power, 2017-06-14 08:40:07]

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

[commit-bot: I haz the power, 2017-06-14 08:40:08]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22305)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/26547)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18151)

[åsapersson, 2017-06-14 10:50:26]

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

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:414: framerate_ = framerate_fps;
On 2017/06/14 08:39:16, sprang_webrtc wrote:
> On 2017/06/13 12:15:09, åsapersson wrote:
> > Add a max limit for framerate_ to 30? 
> 
> Do we really want to? I think we at least support up to 60fps, right?
> I'll add a dcheck here and maybe cap it in vie_encoder instead.

What if max is 60fps and input 30fps. Wouldn't max_sample_diff_ms_ be 22.5 ms
when running at 30fps?

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

https://codereview.webrtc.org/2918143003/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:579:
last_adaptation_request_->selected_framerate_fps_) {
would it be better to get sink_wants_.max_framerate_fps from source_proxy_?

[sprang_webrtc, 2017-06-14 13:41:28]

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

https://codereview.webrtc.org/2918143003/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:414: framerate_ = framerate_fps;
On 2017/06/14 10:50:26, åsapersson wrote:
> On 2017/06/14 08:39:16, sprang_webrtc wrote:
> > On 2017/06/13 12:15:09, åsapersson wrote:
> > > Add a max limit for framerate_ to 30? 
> > 
> > Do we really want to? I think we at least support up to 60fps, right?
> > I'll add a dcheck here and maybe cap it in vie_encoder instead.
> 
> What if max is 60fps and input 30fps. Wouldn't max_sample_diff_ms_ be 22.5 ms
> when running at 30fps?

Yes, if the user configures a max framerate of 60fps, then the
max_sample_diff_ms_ will be 22.5ms regardless of input fps.
Running this code in chrome however it seems we are never fed the actual fps
capability of the capturer, but rather a default value of 60 is provided. This
is problematic.

I think maybe we may want to change the behavior to update the max framerate
based (reliable) measurements, if available, but that will complicate things
further.

Just capping to 30fps will not give a worse experience than the previous and
will still improve the low fps case. Let's just do that for now and revisit in
the future.

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

https://codereview.webrtc.org/2918143003/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:579:
last_adaptation_request_->selected_framerate_fps_) {
On 2017/06/14 10:50:26, åsapersson wrote:
> would it be better to get sink_wants_.max_framerate_fps from source_proxy_?

Sure, I'll expose a getter that can be used.

[åsapersson, 2017-06-14 14:46:22]

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:586: if (current_fps_want !=
std::numeric_limits<int>::max()) {
remove if statement above?

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:903: std::min(max_framerate_, requested_framerate));
can max_framerate_ be -1 here?

[sprang_webrtc, 2017-06-14 15:29:56]

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:586: if (current_fps_want !=
std::numeric_limits<int>::max()) {
On 2017/06/14 14:46:22, åsapersson wrote:
> remove if statement above?

Done.

https://codereview.webrtc.org/2918143003/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:903: std::min(max_framerate_, requested_framerate));
On 2017/06/14 14:46:22, åsapersson wrote:
> can max_framerate_ be -1 here?

Only if AdaptDown() can be called before ReconfigureEncoder()?
I'll add a DCHECK

[åsapersson, 2017-06-15 08:50:45]

lgtm

[sprang_webrtc, 2017-06-15 09:06:24]

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

[commit-bot: I haz the power, 2017-06-15 09:06:35]

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-06-15 09:31:22]

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

[commit-bot: I haz the power, 2017-06-15 09:31:23]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-06-15 11:19:01]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-15 11:19:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2918143003/#ps220001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-06-15 11:19:03]

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-06-15 11:21:08]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1497525541013610,
"parent_rev": "19e087fc91453b761b6b56ba84e7c9c5fafd9e8e", "commit_rev":
"fda496a31e5c4d0aac98d3309eb288495748ba8f"}

[commit-bot: I haz the power, 2017-06-15 11:21:15]

Description was changed from

==========
Set overuse detector max frame interval based on target frame rate.

Currently there is a hard limit for the estimated captured frame
interval of 45ms. As the encoder utilization is calculated as
(input frame interval)/(encode time), overuse signals can be triggered
even though there is plenty of time to go around if the fps is low.

However, in order to avoid falsly estimating low encode usage in case
the capturer has a dynamic frame rate, set the frame interval based on
the actual current max framerate.

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

to

==========
Set overuse detector max frame interval based on target frame rate.

Currently there is a hard limit for the estimated captured frame
interval of 45ms. As the encoder utilization is calculated as
(input frame interval)/(encode time), overuse signals can be triggered
even though there is plenty of time to go around if the fps is low.

However, in order to avoid falsly estimating low encode usage in case
the capturer has a dynamic frame rate, set the frame interval based on
the actual current max framerate.

BUG=webrtc:4172

Review-Url: https://codereview.webrtc.org/2918143003
Cr-Commit-Position: refs/heads/master@{#18610}
Committed:
https://chromium.googlesource.com/external/webrtc/+/fda496a31e5c4d0aac98d3309...
==========

[commit-bot: I haz the power, 2017-06-15 11:21:16]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/fda496a31e5c4d0aac98d3309...