Make overuse estimator one dimensional.

Make overuse estimator one dimensional.

This drops the payload size difference dimension of the Kalman filter,
which doesn't improve the quality of the estimation when pacing packets
on the send-side.

R=gaetano.carlucci@gmail.com, mflodman@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/06e05a85b9e4def75ed5e6b582c4df842616f25f
Cr-Commit-Position: refs/heads/master@{#10809}

[stefan-webrtc, 2015-10-01 14:06:15]

stefan@webrtc.org changed reviewers:
+ gaetano.carlucci@gmail.com, mflodman@webrtc.org, terelius@webrtc.org

[stefan-webrtc, 2015-10-01 14:06:16]

gaetano, please take a close look at the changes in overuse_estimator.cc.

terelius, could you review the changes to all files under webrtc/modules? This
removes the payload size dimension of the Kalman filter, which has been proven
not to have any significant effect.

mflodman, webrtc/common_types.h and whatever else you want. :)

[stefan-webrtc, 2015-10-01 14:06:34]

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

[commit-bot: I haz the power, 2015-10-01 14:07:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376423002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376423002/40001

[commit-bot: I haz the power, 2015-10-01 14:14:58]

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

[commit-bot: I haz the power, 2015-10-01 14:14:59]

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

[Gaetano Carlucci, 2015-10-06 13:50:25]

lgtm

LGTM after having run some tests. My only concern is about  the setting of the
process noise variance which probably should be made smaller. We can run more
test later.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc:33:
process_noise_(0.01),
We can run some tests to verity this setting of the process noise variance.

[terelius, 2015-10-12 12:04:41]

I want to look a bit more closely on the estimator, but I think everything looks
good. Some minor questions below.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.cc (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:73:
current_timestamp_group_.complete_time_ms = arrival_time_ms;
= max(complete_time_ms, arrival_time_ms); maybe?

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.h (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.h:27: // most
timestamp_group_length_ticks older than the first timestamp in that
s/older/newer or maybe s/first/last ?

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc:43: double
ts_delta,
s/t_delta/recv_t_delta or something similar?
s/ts_delta/send_t_delta ?

Should they have different types?

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc
(right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc:55:
CapacityDropTestHelper(3, true, 567);
Use of expected_drop_delta? Reason for change?

[stefan-webrtc, 2015-11-24 16:08:03]

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.cc (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:73:
current_timestamp_group_.complete_time_ms = arrival_time_ms;
On 2015/10/12 12:04:40, terelius wrote:
> = max(complete_time_ms, arrival_time_ms); maybe?

Done.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.h (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.h:27: // most
timestamp_group_length_ticks older than the first timestamp in that
On 2015/10/12 12:04:40, terelius wrote:
> s/older/newer or maybe s/first/last ?

Done.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc (right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc:33:
process_noise_(0.01),
On 2015/10/06 13:50:25, Gaetano Carlucci wrote:
> We can run some tests to verity this setting of the process noise variance.

We'll have to tune this, but for now I'll keep it as is. In a follow-up I think
we will have to make it somewhat depending on sample rate.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/overuse_estimator.cc:43: double
ts_delta,
On 2015/10/12 12:04:41, terelius wrote:
> s/t_delta/recv_t_delta or something similar?
> s/ts_delta/send_t_delta ?
> 
> Should they have different types?

Done.

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc
(right):

https://codereview.webrtc.org/1376423002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc:55:
CapacityDropTestHelper(3, true, 567);
On 2015/10/12 12:04:41, terelius wrote:
> Use of expected_drop_delta? Reason for change?

I analyzed this and this is just a matter of timing. In all these test cases
network overuse is detected roughly at the same time, it differs roughly 10-20
ms because all streams don't start at the same time.

This together with the different start time of the streams means that the time
it takes from detection of overuse until the bandwidth estimate finally falls
below 500 kbps (which the test looks for) is a bit different. It turns out that
this case gets a sample almost exactly when it goes below 500 kbps (around 499
kbps), while the others slightly before (around 502-508 kbps).

An important difference is also that all streams are 30 fps, so a single stream
using all bandwidth means that it will only have 30 updates of the BWE per
second, while three streams may get up to 90 updates per second.

I also noted that I accidentally corrected the wrong test here. It should have
been TwoStreams.

[stefan-webrtc, 2015-11-26 07:57:24]

Any further comments on this CL?

[terelius, 2015-11-26 09:15:39]

lgtm

[stefan-webrtc, 2015-11-26 10:11:00]

Magnus, I need your eyes on webrtc/common_types.h. I believe you will happily
accept that change. :)

[mflodman, 2015-11-26 12:15:07]

Very much so, LGTM :)

[stefan-webrtc, 2015-11-26 12:30:11]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-11-26 12:30:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
gaetano.carlucci@gmail.com
Link to the patchset: https://codereview.webrtc.org/1376423002/#ps80001 (title:
".")

[commit-bot: I haz the power, 2015-11-26 12:30:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376423002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376423002/80001

[commit-bot: I haz the power, 2015-11-26 14:30:59]

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

[commit-bot: I haz the power, 2015-11-26 14:31:00]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[stefan-webrtc, 2015-11-26 14:35:14]

Description was changed from

==========
Make overuse estimator one dimensional.

This drops the payload size difference dimension of the Kalman filter,
which doesn't improve the quality of the estimation when pacing packets
on the send-side.
==========

to

==========
Make overuse estimator one dimensional.

This drops the payload size difference dimension of the Kalman filter,
which doesn't improve the quality of the estimation when pacing packets
on the send-side.

R=gaetano.carlucci@gmail.com, mflodman@webrtc.org, terelius@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/06e05a85b9e4def75ed5e6b58...
==========

[stefan-webrtc, 2015-11-26 14:35:16]

Committed patchset #5 (id:80001) manually as
06e05a85b9e4def75ed5e6b582c4df842616f25f (presubmit successful).

[commit-bot: I haz the power, 2015-11-26 14:35:18]

Description was changed from

==========
Make overuse estimator one dimensional.

This drops the payload size difference dimension of the Kalman filter,
which doesn't improve the quality of the estimation when pacing packets
on the send-side.

R=gaetano.carlucci@gmail.com, mflodman@webrtc.org, terelius@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/06e05a85b9e4def75ed5e6b58...
==========

to

==========
Make overuse estimator one dimensional.

This drops the payload size difference dimension of the Kalman filter,
which doesn't improve the quality of the estimation when pacing packets
on the send-side.

R=gaetano.carlucci@gmail.com, mflodman@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/06e05a85b9e4def75ed5e6b582c4df842616f25f
Cr-Commit-Position: refs/heads/master@{#10809}
==========

[commit-bot: I haz the power, 2015-11-26 14:35:19]

Patchset 5 (id:??) landed as
https://crrev.com/06e05a85b9e4def75ed5e6b582c4df842616f25f
Cr-Commit-Position: refs/heads/master@{#10809}

[stefan-webrtc, 2015-11-27 09:02:14]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/1481003002/ by stefan@webrtc.org.

The reason for reverting is: Broke webrtc_perf_tests on bots..