Fixes a bug where a video stream can get stuck in the suspended state.

Fixes a bug where a video stream can get stuck in the suspended state.

This happens if a lot of FEC is allocated when the stream becomes suspended. The required bitrate to unsuspend can then be too high so that the padding bitrate we are allowed to generate is not enough.

This CL also switches the tests from using ISAC to OPUS as RampUpTest.UpDownUpAudioVideoTransportSequenceNumberRtx relies on audio BWE to work (which is only compatible with OPUS). I don't know why it didn't fail before.

BUG=webrtc:7178
Review-Url: https://codereview.webrtc.org/2705603002
Cr-Commit-Position: refs/heads/master@{#16739}
Committed: https://chromium.googlesource.com/external/webrtc/+/a518a39963d34616d8f0e94991c7f5fbb5affb38

[stefan-webrtc, 2017-02-17 12:50:00]

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

[stefan-webrtc, 2017-02-20 09:49:09]

clean

[stefan-webrtc, 2017-02-20 09:51:15]

.

[stefan-webrtc, 2017-02-20 14:53:57]

all tests passing.

[stefan-webrtc, 2017-02-20 14:54:48]

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

[commit-bot: I haz the power, 2017-02-20 14:54:54]

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

[stefan-webrtc, 2017-02-20 14:55:28]

Please take a look, I hope it should pass all tests now. :)

[stefan-webrtc, 2017-02-20 15:06:01]

Description was changed from

==========
Fixes a bug where a video stream can get stuck in the suspended state.

This happens if a lot of FEC is allocated when the stream becomes suspended. The
required bitrate to unsuspend can then be too high so that the padding bitrate
we are allowed to generate is not enough.

BUG=webrtc:7178
==========

to

==========
Fixes a bug where a video stream can get stuck in the suspended state.

This happens if a lot of FEC is allocated when the stream becomes suspended. The
required bitrate to unsuspend can then be too high so that the padding bitrate
we are allowed to generate is not enough.

This CL also switches the tests from using ISAC to OPUS as
RampUpTest.UpDownUpAudioVideoTransportSequenceNumberRtx relies on audio BWE to
work (which is only compatible with OPUS). I don't know why it didn't fail
before.

BUG=webrtc:7178
==========

[commit-bot: I haz the power, 2017-02-20 17:22:49]

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

[commit-bot: I haz the power, 2017-02-20 17:22:50]

Dry run: This issue passed the CQ dry run.

[terelius, 2017-02-20 17:42:37]

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:497: if (num_flexfec_streams_ == 0 || flex_fec_bytes
> 0) {
How about functions SetLossRates(r1,r2,r3) and SetExpectedFecRates(r) that
allows configuring packet loss and FEC from each test? The tests would be
slightly more verbose (at least if many of them use FEC), but I think it would
increase flexibility and clarity. WDYT?

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:563: RampUpDownUpTester test(1, 0, 0,
kStartBitrateBps,
Why change the number of streams here? Is it because FlexFEC only protects one
stream and you want to compare results with the test below?

[stefan-webrtc, 2017-02-21 09:09:45]

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:497: if (num_flexfec_streams_ == 0 || flex_fec_bytes
> 0) {
On 2017/02/20 17:42:37, terelius wrote:
> How about functions SetLossRates(r1,r2,r3) and SetExpectedFecRates(r) that
> allows configuring packet loss and FEC from each test? The tests would be
> slightly more verbose (at least if many of them use FEC), but I think it would
> increase flexibility and clarity. WDYT?

I improved it quite a bit, but not exactly how you suggested.

https://codereview.webrtc.org/2705603002/diff/80001/webrtc/call/rampup_tests....
webrtc/call/rampup_tests.cc:563: RampUpDownUpTester test(1, 0, 0,
kStartBitrateBps,
On 2017/02/20 17:42:37, terelius wrote:
> Why change the number of streams here? Is it because FlexFEC only protects one
> stream and you want to compare results with the test below?

Mistake. Changing back. :)

[stefan-webrtc, 2017-02-21 09:11:13]

.

[stefan-webrtc, 2017-02-21 09:12:38]

.

[terelius, 2017-02-21 10:29:24]

lgtm with one question:

https://codereview.webrtc.org/2705603002/diff/120001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2705603002/diff/120001/webrtc/call/rampup_tests...
webrtc/call/rampup_tests.cc:567: std::vector<int> loss_rates = {0, 0, 0, 0};
Would it make sense to set loss rate through a method rather than the
constructor? Positive loss seems to be the exceptional case, so it would be nice
to avoid the all-zero vector in every test case.

[stefan-webrtc, 2017-02-21 10:45:18]

https://codereview.webrtc.org/2705603002/diff/120001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2705603002/diff/120001/webrtc/call/rampup_tests...
webrtc/call/rampup_tests.cc:567: std::vector<int> loss_rates = {0, 0, 0, 0};
On 2017/02/21 10:29:24, terelius wrote:
> Would it make sense to set loss rate through a method rather than the
> constructor? Positive loss seems to be the exceptional case, so it would be
nice
> to avoid the all-zero vector in every test case.

Added a constructor instead.

[stefan-webrtc, 2017-02-21 10:51:43]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2017-02-21 10:51:52]

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

[mflodman, 2017-02-21 11:40:32]

LGTM

[commit-bot: I haz the power, 2017-02-21 12:12:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487674303073920,
"parent_rev": "872104ac41d7764f8676c9ea55555210bea4605c", "commit_rev":
"a518a39963d34616d8f0e94991c7f5fbb5affb38"}

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

Description was changed from

==========
Fixes a bug where a video stream can get stuck in the suspended state.

This happens if a lot of FEC is allocated when the stream becomes suspended. The
required bitrate to unsuspend can then be too high so that the padding bitrate
we are allowed to generate is not enough.

This CL also switches the tests from using ISAC to OPUS as
RampUpTest.UpDownUpAudioVideoTransportSequenceNumberRtx relies on audio BWE to
work (which is only compatible with OPUS). I don't know why it didn't fail
before.

BUG=webrtc:7178
==========

to

==========
Fixes a bug where a video stream can get stuck in the suspended state.

This happens if a lot of FEC is allocated when the stream becomes suspended. The
required bitrate to unsuspend can then be too high so that the padding bitrate
we are allowed to generate is not enough.

This CL also switches the tests from using ISAC to OPUS as
RampUpTest.UpDownUpAudioVideoTransportSequenceNumberRtx relies on audio BWE to
work (which is only compatible with OPUS). I don't know why it didn't fail
before.

BUG=webrtc:7178

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

[commit-bot: I haz the power, 2017-02-21 12:12:28]

Committed patchset #8 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/a518a39963d34616d8f0e9499...

[philipel, 2017-02-21 14:52:11]

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

The reason for reverting is: Breaks downstream.