Periodically update codec bit/frame rate settings.

Fix bug in vie_encoder.cc which caused channel parameters not to be updated at regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other things:
* FakeEncoder should trust the configured FPS value rather than guesstimating itself based on the realtime clock, so as not to completely undershoot targets in offline mode. Also, compensate for key-frame overshoots when outputting delta frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have been seen.
* Fix a bunch of test cases that started failing because they were relying on the fake encoder undershooting.
* Fix test 

BUG=7664
Review-Url: https://codereview.webrtc.org/2883963002
Cr-Commit-Position: refs/heads/master@{#18473}
Committed: https://chromium.googlesource.com/external/webrtc/+/6431e21da672a5f3bbf166d3d4d98b171d015706

[sprang_webrtc, 2017-05-15 15:03:09]

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

[commit-bot: I haz the power, 2017-05-15 15:03: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-05-15 15:08:04]

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

[commit-bot: I haz the power, 2017-05-15 15:08:04]

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/17037)

[sprang_webrtc, 2017-05-16 10:02:41]

Description was changed from

==========
Updates to rate control.

Testing CL for now.

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

to

==========
Fix bug in vie_encoder.cc which caused channel parameters not to be updated at
regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other
things:
* FakeEncoder should trust the configured FPS value rather than guesstimating
itself based on the realtime clock, so as not to completely undershoot targets
in offline mode. Also, compensate for key-frame overshoots when outputting delta
frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have
been seen.
* Fix a bunch of test cases that started failing because they were relying on
the fake encoder undershooting.
* Fix test 

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

[sprang_webrtc, 2017-05-16 10:08:30]

Description was changed from

==========
Fix bug in vie_encoder.cc which caused channel parameters not to be updated at
regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other
things:
* FakeEncoder should trust the configured FPS value rather than guesstimating
itself based on the realtime clock, so as not to completely undershoot targets
in offline mode. Also, compensate for key-frame overshoots when outputting delta
frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have
been seen.
* Fix a bunch of test cases that started failing because they were relying on
the fake encoder undershooting.
* Fix test 

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

to

==========
Fix bug in vie_encoder.cc which caused channel parameters not to be updated at
regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other
things:
* FakeEncoder should trust the configured FPS value rather than guesstimating
itself based on the realtime clock, so as not to completely undershoot targets
in offline mode. Also, compensate for key-frame overshoots when outputting delta
frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have
been seen.
* Fix a bunch of test cases that started failing because they were relying on
the fake encoder undershooting.
* Fix test 

BUG=7664
==========

[sprang_webrtc, 2017-05-16 10:08:30]

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

[sprang_webrtc, 2017-05-22 13:52:04]

ping

[stefan-webrtc, 2017-05-23 17:03:20]

Could you elaborate in the CL description to make it more clear what issues you
are fixing?

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:108: // This is mainly for unit
testing, disabling frame dropping.
I don't see why this disables frame dropping?

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:130: debt_bytes_[i] += total_debt_bytes /
kNumDebtPayments;
An option is to just keep track of a total debt and pay X% of it every frame?
That seems simpler than keeping a vector of debts.

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:136: if (debt_bytes >= stream_bytes) {
This can only be true if == right? What is it that we want to do within this if
clause? Maybe useful with a comment, or I'm just tired. :)

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:993: // Setup screen content disables
frame dropping which makes this easier.
This seems a bit unfortunate. What if we want to reenable frame dropping for
screen content?

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (left):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:198: FALLTHROUGH();
If we remove this I assume we should add a break insetad?

[stefan-webrtc, 2017-05-23 17:03:57]

On 2017/05/23 17:03:20, stefan-webrtc wrote:
> Could you elaborate in the CL description to make it more clear what issues
you
> are fixing?
> 

Or maybe it's elaborate enough, you decide. :)

[sprang_webrtc, 2017-05-24 09:07:08]

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:108: // This is mainly for unit
testing, disabling frame dropping.
On 2017/05/23 17:03:20, stefan-webrtc wrote:
> I don't see why this disables frame dropping?

Allows us to set temporal layer count with generic codec, so that screenshare
mode (the only mode with frame dropping disabled) can be enabled.
I know this is kinda ugly, but there was just soo much plumbing needed (in
production code) otherwise, which I didn't feel was worth it to deflakify some
tests...

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:130: debt_bytes_[i] += total_debt_bytes /
kNumDebtPayments;
On 2017/05/23 17:03:20, stefan-webrtc wrote:
> An option is to just keep track of a total debt and pay X% of it every frame?
> That seems simpler than keeping a vector of debts.

Maybe, but then paying of the whole debt will take some time, and the frame
dropper only has a certain time it allows overshoot, right?

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:136: if (debt_bytes >= stream_bytes) {
On 2017/05/23 17:03:20, stefan-webrtc wrote:
> This can only be true if == right? What is it that we want to do within this
if
> clause? Maybe useful with a comment, or I'm just tired. :)

Oops, the std::min above shouldn't be there.
I'll add a comment.

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:993: // Setup screen content disables
frame dropping which makes this easier.
On 2017/05/23 17:03:20, stefan-webrtc wrote:
> This seems a bit unfortunate. What if we want to reenable frame dropping for
> screen content?

Then we should probably expose some API surface to enable disabling of frame
dropping, but that touches a lot of layers....

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (left):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:198: FALLTHROUGH();
On 2017/05/23 17:03:20, stefan-webrtc wrote:
> If we remove this I assume we should add a break insetad?

FALLTHROUGH() macro is actually only there to prevent compiler warnings if we do
some action in one case clause and then want to fall through and do even more
actions in the next one. For a case like this, where to enums should do the
exact same thing we don't need this macro.

[sprang_webrtc, 2017-05-30 15:23:39]

ping

[sprang_webrtc, 2017-06-02 11:12:04]

stefan, are you there?

[holmer, 2017-06-02 11:38:04]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-06-02 11:38:05]

lgtm

Sorry for the delay. Feel free to ping me if I'm too slow!

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2883963002/diff/20001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:130: debt_bytes_[i] += total_debt_bytes /
kNumDebtPayments;
On 2017/05/24 09:07:07, sprang_webrtc wrote:
> On 2017/05/23 17:03:20, stefan-webrtc wrote:
> > An option is to just keep track of a total debt and pay X% of it every
frame?
> > That seems simpler than keeping a vector of debts.
> 
> Maybe, but then paying of the whole debt will take some time, and the frame
> dropper only has a certain time it allows overshoot, right?

You could have a total_debt and a payed_debt and try to reach total_debt =
payed_debt, or something like that. But maybe it's too complicated.

[sprang_webrtc, 2017-06-05 16:50:26]

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

[commit-bot: I haz the power, 2017-06-05 16:50:33]

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-05 16:51:38]

Updated with less accurate but much simpler debt handling. Feel free to have a
look and approve with your @webrtc account instead ;)

[commit-bot: I haz the power, 2017-06-05 17:09:59]

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

[commit-bot: I haz the power, 2017-06-05 17:10:00]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-06-05 18:33:21]

lgtm assuming only fake_encoder.* was changed. But consider my comment.

https://codereview.webrtc.org/2883963002/diff/80001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2883963002/diff/80001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:136: stream_bytes -= payment_size;
Why don't we do this inside the debt_bytes_ > 0 block? We shouldn't pay back if
there's no debt, right? Or are you simply assuming that payment_size is zero
here if there's no debt? In that case perhaps add a DCHECK?

[sprang_webrtc, 2017-06-07 08:11:20]

https://codereview.webrtc.org/2883963002/diff/80001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2883963002/diff/80001/webrtc/test/fake_encoder....
webrtc/test/fake_encoder.cc:136: stream_bytes -= payment_size;
On 2017/06/05 18:33:21, stefan-webrtc wrote:
> Why don't we do this inside the debt_bytes_ > 0 block? We shouldn't pay back
if
> there's no debt, right? Or are you simply assuming that payment_size is zero
> here if there's no debt? In that case perhaps add a DCHECK?

No, this was just sloppy cleanup of some temp code. Fixed.

[sprang_webrtc, 2017-06-07 08:11:26]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-07 08:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2883963002/#ps100001 (title:
"cleanup")

[commit-bot: I haz the power, 2017-06-07 08:11:30]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-07 08:13:19]

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

[commit-bot: I haz the power, 2017-06-07 08:13:20]

Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25230)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22076)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17854)

[sprang_webrtc, 2017-06-07 08:58:42]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-07 08:58:42]

The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2883963002/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-06-07 08:59:01]

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-07 09:21:43]

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

[commit-bot: I haz the power, 2017-06-07 09:21:44]

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

[sprang_webrtc, 2017-06-07 11:41:22]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-07 11:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2883963002/#ps140001 (title:
"Dumb typo")

[commit-bot: I haz the power, 2017-06-07 11:41:26]

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-07 11:59:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496835682861100,
"parent_rev": "2038df452cf0d73947268c13aaba0c68ed2491cc", "commit_rev":
"6431e21da672a5f3bbf166d3d4d98b171d015706"}

[commit-bot: I haz the power, 2017-06-07 11:59:42]

Description was changed from

==========
Fix bug in vie_encoder.cc which caused channel parameters not to be updated at
regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other
things:
* FakeEncoder should trust the configured FPS value rather than guesstimating
itself based on the realtime clock, so as not to completely undershoot targets
in offline mode. Also, compensate for key-frame overshoots when outputting delta
frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have
been seen.
* Fix a bunch of test cases that started failing because they were relying on
the fake encoder undershooting.
* Fix test 

BUG=7664
==========

to

==========
Fix bug in vie_encoder.cc which caused channel parameters not to be updated at
regular intervals, as it was intended.

That however exposes a bunch of failed test, so this CL also fixed a few other
things:
* FakeEncoder should trust the configured FPS value rather than guesstimating
itself based on the realtime clock, so as not to completely undershoot targets
in offline mode. Also, compensate for key-frame overshoots when outputting delta
frames.
* FrameDropper should not assuming incoming frame rate is 0 if no frames have
been seen.
* Fix a bunch of test cases that started failing because they were relying on
the fake encoder undershooting.
* Fix test 

BUG=7664

Review-Url: https://codereview.webrtc.org/2883963002
Cr-Commit-Position: refs/heads/master@{#18473}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6431e21da672a5f3bbf166d3d...
==========

[commit-bot: I haz the power, 2017-06-07 11:59:43]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/6431e21da672a5f3bbf166d3d...

[sprang_webrtc, 2017-06-07 13:17:39]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/2923993002/ by sprang@webrtc.org.

The reason for reverting is: Breaks some Call perf tests that are not run by the
try bots.....