Reimplemeted "Test and fix for huge bwe drop after alr state"

Reimplemeted "Test and fix for huge bwe drop after alr state"

BUG=webrtc:7746
Test and fix for huge bwe drop after alr state.

BUG=webrtc:7746
Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba61641962119071646175bfe3bc2bda063

patch from issue 2931873002 at patchset 320001 (http://crrev.com/2931873002#ps320001)

Review-Url: https://codereview.webrtc.org/2970653004
Cr-Commit-Position: refs/heads/master@{#19055}
Committed: https://chromium.googlesource.com/external/webrtc/+/9d117643449bfbac79dc5b3b1c7920f6bf93ee82

[tschumi, 2017-07-04 14:19:40]

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

[commit-bot: I haz the power, 2017-07-04 14:19:44]

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-07-04 14:41:14]

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

[commit-bot: I haz the power, 2017-07-04 14:41:14]

Dry run: This issue passed the CQ dry run.

[tschumi, 2017-07-04 15:58:08]

Description was changed from

==========
Reimplemeted "Test and fix for huge bwe drop after alr state"

BUG=webrtc:7746

Test and fix for huge bwe drop after alr state.

BUG=webrtc:7746

Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed:
https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461...

patch from issue 2931873002 at patchset 320001
(http://crrev.com/2931873002#ps320001)
==========

to

==========
Reimplemeted "Test and fix for huge bwe drop after alr state"

BUG=webrtc:7746

Test and fix for huge bwe drop after alr state.

BUG=webrtc:7746

Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed:
https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461...

patch from issue 2931873002 at patchset 320001
(http://crrev.com/2931873002#ps320001)
==========

[tschumi, 2017-07-04 15:58:08]

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

[tschumi, 2017-07-05 06:32:18]

Here is the re-implementation of the dtx problem fix.

[stefan-webrtc, 2017-07-06 15:28:39]

https://codereview.webrtc.org/2970653004/diff/1/webrtc/audio/test/audio_bwe_i...
File webrtc/audio/test/audio_bwe_integration_test.cc (right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/audio/test/audio_bwe_i...
webrtc/audio/test/audio_bwe_integration_test.cc:60:
SleepMs(GetNetworkPipeConfig().queue_delay_ms);
+1000 to ensure that data in the receiver's queue also has time to get
processed?

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/bitrate_estimator.cc:102: if
(bitrate_estimate_ >= 0.f) {
Why >= 0.f?

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/bitrate_estimator.cc:103:
bitrate_estimate_var_ = 200;
Add a comment why this is done.

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:282:
static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime()));
I prefer:
pacer_->GetApplicationLimitedRegionStartTime() > 0 and no cast.

[tschumi, 2017-07-07 07:51:14]

https://codereview.webrtc.org/2970653004/diff/1/webrtc/audio/test/audio_bwe_i...
File webrtc/audio/test/audio_bwe_integration_test.cc (right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/audio/test/audio_bwe_i...
webrtc/audio/test/audio_bwe_integration_test.cc:60:
SleepMs(GetNetworkPipeConfig().queue_delay_ms);
Ok

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/bitrate_estimator.cc:102: if
(bitrate_estimate_ >= 0.f) {
The goal was to just increase when we have a valid estimation, but when i think
a bit more it seams to be ok to increase it even we don't have a valid value.
Will remove the if.

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/bitrate_estimator.cc:103:
bitrate_estimate_var_ = 200;
On 2017/07/06 15:28:36, stefan-webrtc wrote:
> Add a comment why this is done.

Done.

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:282:
static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime()));
GetApplicationLimitedRegionStartTime returns a rtc::Optional<int64_t>
And the information i like to gain is if the optional is set.
So "GetApplicationLimitedRegionStartTime() > 0" will not work.

[holmer, 2017-07-07 07:53:06]

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

[holmer, 2017-07-07 07:53:07]

lgtm

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:282:
static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime()));
On 2017/07/07 07:51:14, tschumi wrote:
> GetApplicationLimitedRegionStartTime returns a rtc::Optional<int64_t>
> And the information i like to gain is if the optional is set.
> So "GetApplicationLimitedRegionStartTime() > 0" will not work.
> 
> 
> 

Ah, my bad :)

Maybe you don't even need the cast then?

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/bitrate_estimator.cc:102: // By setting the
bitrate-estimate variance to a hight value we allow the
higher

[terelius, 2017-07-07 08:01:19]

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:282:
static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime()));
On 2017/07/07 07:53:06, holmer wrote:
> On 2017/07/07 07:51:14, tschumi wrote:
> > GetApplicationLimitedRegionStartTime returns a rtc::Optional<int64_t>
> > And the information i like to gain is if the optional is set.
> > So "GetApplicationLimitedRegionStartTime() > 0" will not work.
> > 
> > 
> > 
> 
> Ah, my bad :)
> 
> Maybe you don't even need the cast then?

I think the cast is needed, but there is a .has_value() that you can use
instead.

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/bitrate_estimator.cc:104:
bitrate_estimate_var_ = 200;
Why 200 compared to say, 50 or 20? Assuming that 200 is the correct value,
shouldn't we use that initially too? What if the previously estimated variance
is larger than 200?

[holmer, 2017-07-07 08:08:00]

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/bitrate_estimator.cc:104:
bitrate_estimate_var_ = 200;
On 2017/07/07 08:01:19, terelius wrote:
> Why 200 compared to say, 50 or 20? Assuming that 200 is the correct value,
> shouldn't we use that initially too? What if the previously estimated variance
> is larger than 200?

Good point.

Maybe += 200 or similar?

[tschumi, 2017-07-07 09:07:59]

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:282:
static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime()));
Ok use has_value() now.

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/bitrate_estimator.cc (right):

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/bitrate_estimator.cc:102: // By setting the
bitrate-estimate variance to a hight value we allow the
On 2017/07/07 07:53:07, holmer wrote:
> higher

Done.

https://codereview.webrtc.org/2970653004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/bitrate_estimator.cc:104:
bitrate_estimate_var_ = 200;
I have no strong verification for 200. I used the Integration test to verify it.
It seams hard to find a the right value :).

Its ok to me that its bigger than the initial value since the initialization of
the filter as a different behavior than the after its initialized. 

Will go for += 200. 

But i'm open to other ideas :)

[terelius, 2017-07-11 20:54:19]

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc
(right):

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: void
AcknowledgedBitrateEstimator::CheckAlrEndTime(bool currently_in_alr) {
How about
bool currently_in_alr = pacer->GetAlrStartTime().has_value()
if (was_in_alr && !currently_in_alr) {
  ack_bitrate_estimator.SetAlrExitTime(now_ms)
}
was_in_alr = currently_in_alr
in the congestion controller?

That way we wouldn't have to pass currently_in_alr to every function in this
class. In the future, we could also reuse the same logic to set the alr exit
time of the probe_controller.

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:66: void
AcknowledgedBitrateEstimator::MaybeExpectFastRateChange(
Maybe inline this function? (It is important that we reset alr_ended_time_ms_,
but that is not obvious from the function name. Hence, I have to jump into the
function to look at what it does. Personally, I find this more  distracting than
adding 3 lines to the function above.)

[tschumi, 2017-07-13 09:05:07]

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc
(right):

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: void
AcknowledgedBitrateEstimator::CheckAlrEndTime(bool currently_in_alr) {
Impl. according to offline discussion.

https://codereview.webrtc.org/2970653004/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:66: void
AcknowledgedBitrateEstimator::MaybeExpectFastRateChange(
:)
I prefer it this way.

[terelius, 2017-07-13 10:46:01]

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:284: if
(alr_state_->ChangeWhenAlrStartTimeIs(
I don't like this name. Is is unclear whether you are telling it to change now,
telling it to change when the ALR start time has reached a certain value or
asking whether it has changed. I don't think you need to specify what the
parameters are in the function name though.

Maybe just call it HasChanged, or maybe split into two functions, e.g.
void Update(rtc::Optional<...>) and
bool HasChangedTo(AlrState::Change)?

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.h:59: static constexpr int
kDefaultAlrStartBudgetLevelPercent = 80;
These numbers have changed a lot. How certain are we that that the new values
are correct? Do the numbers matter or only the difference?

[tschumi, 2017-07-13 11:54:35]

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:284: if
(alr_state_->ChangeWhenAlrStartTimeIs(
On 2017/07/13 10:46:01, terelius wrote:
> I don't like this name. Is is unclear whether you are telling it to change
now,
> telling it to change when the ALR start time has reached a certain value or
> asking whether it has changed. I don't think you need to specify what the
> parameters are in the function name though.
> 
> Maybe just call it HasChanged, or maybe split into two functions, e.g.
> void Update(rtc::Optional<...>) and
> bool HasChangedTo(AlrState::Change)?
> 

Done.

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2970653004/diff/80001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.h:59: static constexpr int
kDefaultAlrStartBudgetLevelPercent = 80;
The detector was to slow in reporting the leafing alr. So we already overused
before alr left was reported. 

So i changed the thresholds in way that it takes more time till we report "in
alr" and less time til we report "left alr".  

Is hard to tell if the numbers are correct.

[terelius, 2017-07-13 12:27:53]

lgtm

Thanks!

[tschumi, 2017-07-13 12:40:06]

The CQ bit was checked by tschumim@webrtc.org

[tschumi, 2017-07-13 12:40:07]

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

[tschumi, 2017-07-13 12:40:08]

The CQ bit was unchecked by tschumim@webrtc.org

[tschumi, 2017-07-13 12:40:15]

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

[commit-bot: I haz the power, 2017-07-13 12:40:19]

Dry run: CQ is trying da patch.

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

[terelius, 2017-07-13 12:40:34]

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

[commit-bot: I haz the power, 2017-07-13 12:40:41]

Dry run: CQ is trying da patch.

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

[tschumi, 2017-07-13 12:43:05]

tschumim@webrtc.org changed reviewers:
+ philipel@webrtc.org

[tschumi, 2017-07-13 12:43:50]

@philipel:
Can you take a look at the changes regarding the pacer ?

[tschumi, 2017-07-13 12:50:11]

tschumim@webrtc.org changed reviewers:
+ ossu@webrtc.org

[tschumi, 2017-07-13 12:51:00]

@ossu:
Can you take a look at the audio_bwe_integration_test ?

[commit-bot: I haz the power, 2017-07-13 13:03:10]

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

[commit-bot: I haz the power, 2017-07-13 13:03:11]

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

[tschumi, 2017-07-13 14:09:39]

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

[commit-bot: I haz the power, 2017-07-13 14:09:53]

Dry run: CQ is trying da patch.

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

[ossu, 2017-07-13 14:31:44]

Can't really say much about the applicability of this test, so I defer to the
other reviewers on that.

Structurally, audio_bwe_integration_test lgtm!

[commit-bot: I haz the power, 2017-07-13 14:41:21]

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

[commit-bot: I haz the power, 2017-07-13 14:41:22]

Dry run: This issue passed the CQ dry run.

[tschumi, 2017-07-14 13:20:00]

The CQ bit was checked by tschumim@webrtc.org

[tschumi, 2017-07-14 13:20:00]

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

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

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-07-14 13:26:31]

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

[commit-bot: I haz the power, 2017-07-14 13:26:33]

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

[philipel, 2017-07-14 13:56:23]

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/congestio...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:158:
const std::unique_ptr<AlrState> alr_state_;
No need for this to be a unique_ptr.

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:114: alr_state_changed_to_.reset();
Isn't |alr_state_changed_to_| just |current_state_|.

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:116: was_in_alr_ =
alr_start_time_ms.has_value();
|was_in_alr_| same as |in_alr_|?

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.h:76: class AlrState {
Do we really need a class to keep track of whether we are in ALR or not? I'm not
sure I understand the reason for this class.

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector_unittest.cc (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector_unittest.cc:13: #include
"webrtc/rtc_base/fakeclock.h"
Used?

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector_unittest.cc:19: // constexpr int64_t
kAlrEndedTimeMs = 1234;
remove

[tschumi, 2017-07-14 15:36:51]

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/congestio...
File
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h
(right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/include/send_side_congestion_controller.h:158:
const std::unique_ptr<AlrState> alr_state_;
Removed AlrState

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:114: alr_state_changed_to_.reset();
Removed AlrState

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:116: was_in_alr_ =
alr_start_time_ms.has_value();
Removed AlrState

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.h:76: class AlrState {
Removed AlrState

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector_unittest.cc (right):

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector_unittest.cc:13: #include
"webrtc/rtc_base/fakeclock.h"
Removed

https://codereview.webrtc.org/2970653004/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector_unittest.cc:19: // constexpr int64_t
kAlrEndedTimeMs = 1234;
On 2017/07/14 13:56:23, philipel wrote:
> remove

Done.

[terelius, 2017-07-14 15:44:52]

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:284:
rtc::Optional<int64_t> alr_start_time_ms =
If we are never going to use the value, could we change it to booleans
currently_in_alr and was_in_alr?

[philipel, 2017-07-14 15:56:28]

lgtm for webrtc/modules/pacing* with one comment

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (left):

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:135: // traffic to meet the current channel
capacity.
Is this comment not correct any more? If so, add a new comment or if not, just
restore it.

[tschumi, 2017-07-17 06:37:16]

Patchset #10 (id:180001) has been deleted

[tschumi, 2017-07-17 06:38:18]

Patchset #10 (id:200001) has been deleted

[tschumi, 2017-07-17 06:41:05]

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/send_side_congestion_controller.cc
(right):

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/send_side_congestion_controller.cc:284:
rtc::Optional<int64_t> alr_start_time_ms =
On 2017/07/14 15:44:52, terelius wrote:
> If we are never going to use the value, could we change it to booleans
> currently_in_alr and was_in_alr?

Done.

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (left):

https://codereview.webrtc.org/2970653004/diff/160001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:135: // traffic to meet the current channel
capacity.
This was not intentional removed.

[tschumi, 2017-07-17 06:44:05]

tschumim@webrtc.org changed reviewers:
+ henrika@webrtc.org

[tschumi, 2017-07-17 06:46:45]

@Henrik: 
Hi, 
Can you take a look at the change in webrtc/BUILD.gn ?

[henrika_webrtc, 2017-07-17 07:51:51]

lgtm

[tschumi, 2017-07-17 07:52:20]

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

[commit-bot: I haz the power, 2017-07-17 07:52:26]

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-07-17 08:23:57]

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

[commit-bot: I haz the power, 2017-07-17 08:23:58]

Dry run: This issue passed the CQ dry run.

[tschumi, 2017-07-17 08:38:44]

The CQ bit was checked by tschumim@webrtc.org

[tschumi, 2017-07-17 08:38:45]

The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com,
terelius@webrtc.org, ossu@webrtc.org, philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2970653004/#ps220001 (title:
"Respond to comments")

[commit-bot: I haz the power, 2017-07-17 08:38:57]

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-07-17 08:41:41]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500280724663490,
"parent_rev": "d4cb3a778242f118ab0c6679abf3c66dc5664ea7", "commit_rev":
"9d117643449bfbac79dc5b3b1c7920f6bf93ee82"}

[commit-bot: I haz the power, 2017-07-17 08:41:48]

Description was changed from

==========
Reimplemeted "Test and fix for huge bwe drop after alr state"

BUG=webrtc:7746

Test and fix for huge bwe drop after alr state.

BUG=webrtc:7746

Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed:
https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461...

patch from issue 2931873002 at patchset 320001
(http://crrev.com/2931873002#ps320001)
==========

to

==========
Reimplemeted "Test and fix for huge bwe drop after alr state"

BUG=webrtc:7746

Test and fix for huge bwe drop after alr state.

BUG=webrtc:7746

Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed:
https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461...

patch from issue 2931873002 at patchset 320001
(http://crrev.com/2931873002#ps320001)

Review-Url: https://codereview.webrtc.org/2970653004
Cr-Commit-Position: refs/heads/master@{#19055}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9d117643449bfbac79dc5b3b1...
==========

[commit-bot: I haz the power, 2017-07-17 08:41:50]

Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/9d117643449bfbac79dc5b3b1...