Stop increasing loss-based BWE if no feedback is received.

Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
R=terelius@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/52200d0b7f76d0cc371949645a2d693eba4fbbc0

[stefan-webrtc, 2016-08-23 09:22:10]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%.

BUG=6238
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=6238
==========

[stefan-webrtc, 2016-08-23 09:24:29]

.

[stefan-webrtc, 2016-08-23 09:38:08]

stefan@webrtc.org changed reviewers:
+ mflodman@webrtc.org, terelius@webrtc.org

[stefan-webrtc, 2016-08-23 09:38:08]

PTAL

It would be great to land this in time for M54.

https://codereview.webrtc.org/2262213002/diff/60001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc
(left):

https://codereview.webrtc.org/2262213002/diff/60001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc:974:
TEST(BweTestFramework_AdaptiveVideoSenderTest, FeedbackChangesBitrate) {
I decided to remove these tests as it's a bit difficult to rewrite them now that
feedback must be provided periodically. They don't provide that much value
anyway since they test a small part of the simulation framework, and regressions
will be caught by the perf tests if things fail.

[stefan-webrtc, 2016-08-23 09:38:49]

Update.

[stefan-webrtc, 2016-08-23 13:12:35]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=6238
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
==========

[stefan-webrtc, 2016-08-23 14:28:29]

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

[commit-bot: I haz the power, 2016-08-23 14:28:34]

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, 2016-08-23 16:29:19]

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

[commit-bot: I haz the power, 2016-08-23 16:29:21]

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

[terelius, 2016-08-23 17:33:29]

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc (right):

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc:223:
report_blocks.push_back(CreateReportBlock(kSsrc2, 2, 0, 31));
It seems we are not testing the same thing as before. What's the reason for
switching to a different SSRC and what did the old code test?

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:165:
UpdateEstimate(now_ms);
Nice. I've been meaning to try moving the UpdateEstimate into the "if
(number_of_packets > 0)" block.

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:280:
last_timeout_ms_ = now_ms;
Should the accumulators (expected_packets_since_last_loss_update_,
lost_packets_since_last_loss_update_Q8_) be reset too, or is it better to keep
whatever data we have even if it is old?

[mflodman, 2016-09-01 06:40:31]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=6238
==========

[mflodman, 2016-09-01 06:43:23]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=6238
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
==========

[stefan-webrtc, 2016-09-04 11:16:11]

.

[stefan-webrtc, 2016-09-04 11:17:35]

Magnus, would you like to take a look while terelius is OOO?

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc (right):

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc:223:
report_blocks.push_back(CreateReportBlock(kSsrc2, 2, 0, 31));
On 2016/08/23 17:33:29, terelius wrote:
> It seems we are not testing the same thing as before. What's the reason for
> switching to a different SSRC and what did the old code test?

The test is supposed to test receiving report blocks from two different streams,
but it used the same ssrc for both of them but with different observer objects.
The correct way to test it is to use different ssrcs and different observer
objects. Changing that makes it behave a bit differently, specifically, ramp-up
becomes a bit slower than before because we need two reports on one of the
streams until we can determine that enough packets have been received (as
indicated by the extended sequence number in the report block) before we can
update the estimate.

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/2262213002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:280:
last_timeout_ms_ = now_ms;
On 2016/08/23 17:33:29, terelius wrote:
> Should the accumulators (expected_packets_since_last_loss_update_,
> lost_packets_since_last_loss_update_Q8_) be reset too, or is it better to keep
> whatever data we have even if it is old?

Perhaps we should reset those too.

[stefan-webrtc, 2016-09-15 11:27:40]

ping!

[terelius, 2016-09-19 09:37:04]

lgtm

[stefan-webrtc, 2016-09-20 10:38:36]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-09-20 10:38:36]

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

[commit-bot: I haz the power, 2016-09-20 10:38:44]

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

[commit-bot: I haz the power, 2016-09-20 12:14:56]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
R=terelius@webrtc.org

Committed: https://crrev.com/52200d0b7f76d0cc371949645a2d693eba4fbbc0
Cr-Commit-Position: refs/heads/master@{#14306}
==========

[stefan-webrtc, 2016-09-20 12:14:56]

Description was changed from

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
R=terelius@webrtc.org

Committed: https://crrev.com/52200d0b7f76d0cc371949645a2d693eba4fbbc0
Cr-Commit-Position: refs/heads/master@{#14306}
==========

to

==========
Stop increasing loss-based BWE if no feedback is received.

This includes if RTCP is received, but the number of packets received by the
other end hasn't increased.

Further, if no RTCP is received for more than 3 feedback intervals (3 seconds)
we start reducing the estimate by 20%. This is put under an experiment.

BUG=webrtc:6238
R=terelius@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/52200d0b7f76d0cc371949645...
==========

[commit-bot: I haz the power, 2016-09-20 12:14:57]

Patchset 7 (id:??) landed as
https://crrev.com/52200d0b7f76d0cc371949645a2d693eba4fbbc0
Cr-Commit-Position: refs/heads/master@{#14306}

[stefan-webrtc, 2016-09-20 12:14:57]

Committed patchset #7 (id:120001) manually as
52200d0b7f76d0cc371949645a2d693eba4fbbc0 (presubmit successful).