Pass time constant to bwe smoothing filter.

Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303
Committed: https://crrev.com/9abbf5ae4ec7d688a9b4aa03a405f3faadb74b90
Cr-Commit-Position: refs/heads/master@{#15266}

[michaelt, 2016-11-21 16:20:06]

michaelt@webrtc.org changed reviewers:
+ minyue@webrtc.org, stefan@webrtc.org

[minyue-webrtc, 2016-11-22 10:14:25]

generally look good to me. But I need Stefan to judge if the propagation through
the layers as proposed are good.

https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream_unittest.cc:422: }
what does 418-421 try to test?

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller_unittest.cc:191: }
would be nice to test that TransportFeedbackAdapter.GetProbingInterval() is
invoked. But I understand that we do not have mock TransportFeedbackAdapter, and
if it is too much work to add, we can live as it is.

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:276: if
(!probing_interval_ms)
... {
...
}

https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:1318:
bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4);
add some reason of choosing 4

[hlundin-webrtc, 2016-11-22 15:09:53]

On 2016/11/22 10:14:25, minyue-webrtc wrote:
> generally look good to me. But I need Stefan to judge if the propagation
through
> the layers as proposed are good.
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
> File webrtc/audio/audio_send_stream_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
> webrtc/audio/audio_send_stream_unittest.cc:422: }
> what does 418-421 try to test?
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
> File webrtc/modules/congestion_controller/congestion_controller_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
> webrtc/modules/congestion_controller/congestion_controller_unittest.cc:191: }
> would be nice to test that TransportFeedbackAdapter.GetProbingInterval() is
> invoked. But I understand that we do not have mock TransportFeedbackAdapter,
and
> if it is too much work to add, we can live as it is.
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
> File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
> webrtc/modules/congestion_controller/delay_based_bwe.cc:276: if
> (!probing_interval_ms)
> ... {
> ...
> }
> 
> https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.cc
> File webrtc/voice_engine/channel.cc (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.c...
> webrtc/voice_engine/channel.cc:1318:
> bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4);
> add some reason of choosing 4

Drive-by: In the CL title and description, change 'constance' to 'constant'.

[michaelt, 2016-11-22 16:29:00]

https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream_unittest.cc:422: }
The the general idea is to test if the correct probing interval is passed. In
the first case 5000ms and in the second 6000ms.
I changed the second test since this impl. is quite unclear.

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller_unittest.cc:191: }
I think mocking TransportFeedbackAdapter should not be terrible hard.
But I'm nut sure if Stefan would like it. Since we would have to change
CongestionController more than without the mock.

Stefan WDYT ?

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:276: if
(!probing_interval_ms)
I removed the else, since its not necessary.

https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:1318:
bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4);
On 2016/11/22 10:14:25, minyue-webrtc wrote:
> add some reason of choosing 4

Done.

[stefan-webrtc, 2016-11-22 16:39:05]

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller_unittest.cc:191: }
On 2016/11/22 16:29:00, michaelt wrote:
> I think mocking TransportFeedbackAdapter should not be terrible hard.
> But I'm nut sure if Stefan would like it. Since we would have to change
> CongestionController more than without the mock.
> 
> Stefan WDYT ? 
> 
> 

I don't think that's what we want to test. We want to make sure that
OnNetworkChanged provides us with a non-zero value, right? Just write a test
which checks that instead.

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

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:275: auto
probing_interval_ms = probing_interval_estimator_.GetIntervalMs();
int

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1319: // 0.22 = 1 - e^(-t/probing_interval * 4) |
t = probing_interval.
This comment needs to be expanded, as I don't understand it... :)

Is it describing the implementation of the smoother?

[stefan-webrtc, 2016-11-22 17:21:40]

Description was changed from

==========
Pass time constance to bwe smoothingi filter.

BUG=webrtc:6443, webrtc:6303
==========

to

==========
Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303
==========

[the sun, 2016-11-23 09:12:34]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2016-11-23 09:12:34]

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:94: stream_config_.rtp.ssrc = kSsrc;
This is dupe from 3 lines above.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:52: int
probing_interval_ms) = 0;
I suppose there is good reason for it, but it is a bit weird that we use two
different types in the same function prototype, to represent time.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:53: int
GetProbingIntervalMs() const;
Other methods here appear to use int64_t for milliseconds.

[michaelt, 2016-11-23 12:28:29]

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

https://codereview.webrtc.org/2518923003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller_unittest.cc:191: }
Since we relay here on concert classes, we would have to take internal
implementation in to account for the test. 
We would just have a non zero probing interval after the first bitrate drop. I'm
not sure if we should do such a Test.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:94: stream_config_.rtp.ssrc = kSsrc;
Removed the duplicated line.

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

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:275: auto
probing_interval_ms = probing_interval_estimator_.GetIntervalMs();
Its not a int.
GetIntervalMs() returns a optional. 

I changed auto to Optional<int64_t>.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:52: int
probing_interval_ms) = 0;
The only reason I had to choose int was that I expect that probing_interval_ms
never exceed the range of a int.
 
Changed the type from probing_interval_ms form int to int64_t.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:53: int
GetProbingIntervalMs() const;
Changed the type from GetProbingIntervalMs form int to int64_t.

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1319: // 0.22 = 1 - e^(-t/probing_interval * 4) |
t = probing_interval.
Yes. I tried to to show how the filter would react to a step response. 
Since its a exponential filter, the formula shows which value we would reach on
a sept response of the length of the probing interval. 

Will try to make this more clear in the comment.

[the sun, 2016-11-24 09:58:45]

lgtm for the comments I had

[stefan-webrtc, 2016-11-24 10:40:46]

I'm missing a unittest for GetProbingInterval(). Either on CongestionController
level, TransportFeedbackAdapter level or at DelayBasedBwe level...

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

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:275: auto
probing_interval_ms = probing_interval_estimator_.GetIntervalMs();
On 2016/11/23 12:28:29, michaelt wrote:
> Its not a int.
> GetIntervalMs() returns a optional. 
> 
> I changed auto to Optional<int64_t>.

Ah. Why do we return an optional from the estimator if we're not making use of
it?

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1320: // The calculation show a step response for
a
for an

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1321: // expensional filter (bitrate_smoother use
an exponential filter) with the
exponential

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1323: // probing_interval and a amplitude of 1.
Move to the line above.

an amplitude

[michaelt, 2016-11-24 13:47:19]

I added unittest for DelayBasedBwe and CongestionController.

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

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:275: auto
probing_interval_ms = probing_interval_estimator_.GetIntervalMs();
Right this don't make sense here. 
I removed the optional and report a default interval instead.

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1320: // The calculation show a step response for
a
On 2016/11/24 10:40:45, stefan-webrtc (holmer) wrote:
> for an

Done.

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1321: // expensional filter (bitrate_smoother use
an exponential filter) with the
On 2016/11/24 10:40:46, stefan-webrtc (holmer) wrote:
> exponential

Done.

https://codereview.webrtc.org/2518923003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1323: // probing_interval and a amplitude of 1.
On 2016/11/24 10:40:45, stefan-webrtc (holmer) wrote:
> Move to the line above.
> 
> an amplitude

Done.

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
I pass here rtc::Optional<int64_t>, since we should Ignore the value, passed as
initial value to the encoder, in the smoothing filter. Since we have no
information about the probing interval at this point. Passing zero, here would
lead to the skip of the init phase of the filter.

[stefan-webrtc, 2016-11-24 13:52:40]

Thanks for the tests!

One last question

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
On 2016/11/24 13:47:18, michaelt wrote:
> I pass here rtc::Optional<int64_t>, since we should Ignore the value, passed
as
> initial value to the encoder, in the smoothing filter. Since we have no
> information about the probing interval at this point. Passing zero, here would
> lead to the skip of the init phase of the filter. 

Hm, why don't we pass optional all the way from the probing estimator? Do we
ever use the default value in the beginning before we have a real estimate?

[michaelt, 2016-11-24 13:59:37]

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
When the start BWE is lower than the actual BW, we would use the default
interval till we had the first drop. And since it mad more sense to me to define
the default in the estimator I choose this impl.

[stefan-webrtc, 2016-11-24 14:07:11]

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
On 2016/11/24 13:59:37, michaelt wrote:
> When the start BWE is lower than the actual BW, we would use the default
> interval till we had the first drop. And since it mad more sense to me to
define
> the default in the estimator I choose this impl.   
> 

Ok, I guess my question is, why does it even have to be optional here? Does it
ever get called with it not being set? I see that it's called from
VoECodec::SetBitRate(), but that function is in turn never being called...

https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/voe_code...

[michaelt, 2016-11-24 15:13:46]

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
I made it Optional because of VoECodec::SetBitRate() :).
You are right this function is never called. So I think a can safely remove the
Optional.

[stefan-webrtc, 2016-11-24 15:19:02]

lgtm

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2518923003/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:75: rtc::Optional<int64_t>
probing_interval_ms);
On 2016/11/24 15:13:46, michaelt wrote:
> I made it Optional because of VoECodec::SetBitRate() :).
> You are right this function is never called. So I think a can safely remove
the
> Optional. 
> 

I would remove that method too. Maybe that should be done separately. I leave it
to solenberg and minyue to decide

[minyue-webrtc, 2016-11-28 12:00:00]

I just found some nits

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:417:
EXPECT_CALL(*helper.channel_proxy(), SetBitrate(_, 6000));
what is additional value of testing the second call?

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator_unittest.cc:64:
allocator_->OnNetworkChanged(300000u, 0, 0, 0);
should we use something more meaningful, e.g., kDefaultProbingIntervalMs

Although this is a test that any value should work, it would be good to use
meaningful values (unless the test is sanity check), so that people don't get
bad examples of using OnNetworkChanged  when reading this.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator_unittest.cc:513:
allocator_->OnNetworkChanged(300000, 0, 50, 8000);
what is the additional value of testing the second call?

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1321: // exponential filter (bitrate_smoother use
an exponential filter) with the
merge this line with up

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1322: // length of probing_interval and an
amplitude of 1.
and put |var| around variables, no need in equation though (so as to avoid
confusion with abs)

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_impl.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_impl.cc:141: bitrate_bps, 0);
Should we give something more meaningful? "0" seems unexpected.

[minyue-webrtc, 2016-11-28 12:38:20]

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1319: // 0.22 = 1 - e^(-t/probing_interval * 4) |
t = probing_interval.
On 2016/11/23 12:28:29, michaelt wrote:
> Yes. I tried to to show how the filter would react to a step response. 
> Since its a exponential filter, the formula shows which value we would reach
on
> a sept response of the length of the probing interval. 
> 
> Will try to make this more clear in the comment.
> 

It may be clearer to say something like the following.

The step response of an exponential filter is
u(t) = 1 - e^(-t / time_constant).
In order to limit the affect of BWE spikes within 25% before the next probing,
we would choose a time constant that fufills
1 - e^(-probing_interval / time_constant) < 0.25
Then 4 * probing_interval is a good choice.

[michaelt, 2016-11-28 13:03:38]

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1319: // 0.22 = 1 - e^(-t/probing_interval * 4) |
t = probing_interval.
Done according to offline discussion.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:417:
EXPECT_CALL(*helper.channel_proxy(), SetBitrate(_, 6000));
It shows that the value which is set in SetBitrate is not fixed.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator_unittest.cc:64:
allocator_->OnNetworkChanged(300000u, 0, 0, 0);
On 2016/11/28 12:00:00, minyue-webrtc wrote:
> should we use something more meaningful, e.g., kDefaultProbingIntervalMs
> 
> Although this is a test that any value should work, it would be good to use
> meaningful values (unless the test is sanity check), so that people don't get
> bad examples of using OnNetworkChanged  when reading this.

Done.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator_unittest.cc:513:
allocator_->OnNetworkChanged(300000, 0, 50, 8000);
It shows that in the observe not just a fixed value is set.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1321: // exponential filter (bitrate_smoother use
an exponential filter) with the
Changed description according to the offline discussion.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1322: // length of probing_interval and an
amplitude of 1.
Changed description according to the offline discussion.

[minyue-webrtc, 2016-11-28 13:14:43]

final nits

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:417:
EXPECT_CALL(*helper.channel_proxy(), SetBitrate(_, 6000));
On 2016/11/28 13:03:37, michaelt wrote:
> It shows that the value which is set in SetBitrate is not fixed.
> 

Yes. but I don't think it is that valuable. There can be all sorts of things to
test at such a low severity.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_impl.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_impl.cc:141: bitrate_bps, 0);
On 2016/11/28 12:00:00, minyue-webrtc wrote:
> Should we give something more meaningful? "0" seems unexpected.

?

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.h:45: void Stop() override;
this is due to rebasing. try to do rebasing on separate patch set (which
requires lightest review.)

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1321: // 1 - e^(-probing_interval /
time_constant) < 0.25
probing_interval -> probing_interval_ms to be consistent with context. (sorry
for being not careful enough in my suggested text)

[michaelt, 2016-11-28 13:30:09]

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream_unittest.cc:417:
EXPECT_CALL(*helper.channel_proxy(), SetBitrate(_, 6000));
Ok will remove the second test.

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
File webrtc/voice_engine/voe_codec_impl.cc (right):

https://codereview.webrtc.org/2518923003/diff/80001/webrtc/voice_engine/voe_c...
webrtc/voice_engine/voe_codec_impl.cc:141: bitrate_bps, 0);
Oh sorry missed that one. This function is not used at all. I think we can
remove the function. But we should do it in a different CL.

I could use 
constexpr int64_t kDefaultProbingIntervalMs = 3000;
To make this a bit more clear here.

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2518923003/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:1321: // 1 - e^(-probing_interval /
time_constant) < 0.25
On 2016/11/28 13:14:43, minyue-webrtc wrote:
> probing_interval -> probing_interval_ms to be consistent with context. (sorry
> for being not careful enough in my suggested text)

Done.

[minyue-webrtc, 2016-11-28 13:54:32]

lgtm

[minyue-webrtc, 2016-11-28 13:54:33]

lgtm

lgtm

[michaelt, 2016-11-28 13:57:26]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-28 13:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2518923003/#ps120001 (title:
"Respond to comments.")

[commit-bot: I haz the power, 2016-11-28 13:57:33]

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-11-28 14:05:56]

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

[commit-bot: I haz the power, 2016-11-28 14:05:57]

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

[michaelt, 2016-11-28 14:11:03]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-28 14:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2518923003/#ps140001 (title:
"Add probing_interval_ms to NullBitrateObserver.")

[commit-bot: I haz the power, 2016-11-28 14:11:10]

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-11-28 15:00:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480342263093880,
"parent_rev": "dd419841a6563070c7df080449b9521b931be02b", "commit_rev":
"4bbc99abc42a1277cb04f48403b8da0809f5ebb0"}

[commit-bot: I haz the power, 2016-11-28 15:00:22]

Description was changed from

==========
Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303
==========

to

==========
Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303
==========

[commit-bot: I haz the power, 2016-11-28 15:00:23]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-28 15:00:31]

Description was changed from

==========
Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303
==========

to

==========
Pass time constanct to bwe smoothing filter.

BUG=webrtc:6443, webrtc:6303

Committed: https://crrev.com/9abbf5ae4ec7d688a9b4aa03a405f3faadb74b90
Cr-Commit-Position: refs/heads/master@{#15266}
==========

[commit-bot: I haz the power, 2016-11-28 15:00:32]

Patchset 8 (id:??) landed as
https://crrev.com/9abbf5ae4ec7d688a9b4aa03a405f3faadb74b90
Cr-Commit-Position: refs/heads/master@{#15266}

[ossu, 2016-11-28 16:04:56]

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

The reason for reverting is: Unfortunately, this change breaks internal
projects. Specifically the change to the CongestionController interface means
anything implementing it will be forced to change in lock-step..

[stefan-webrtc, 2016-11-28 16:06:53]

https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/include/congestion_controller.h:52: int64_t
probing_interval_ms) = 0;
To avoid the build issues with internal projects I'd keep the old method around
but deprecate it, and create a new overloaded method with this parameter added
to it.

[minyue-webrtc, 2016-11-28 16:07:34]

On 2016/11/28 16:06:53, stefan-webrtc (holmer) wrote:
>
https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
> File webrtc/modules/congestion_controller/include/congestion_controller.h
> (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
> webrtc/modules/congestion_controller/include/congestion_controller.h:52:
int64_t
> probing_interval_ms) = 0;
> To avoid the build issues with internal projects I'd keep the old method
around
> but deprecate it, and create a new overloaded method with this parameter added
> to it.

Sure. I will do it.

[minyue-webrtc, 2016-11-28 16:07:39]

On 2016/11/28 16:06:53, stefan-webrtc (holmer) wrote:
>
https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
> File webrtc/modules/congestion_controller/include/congestion_controller.h
> (right):
> 
>
https://codereview.webrtc.org/2518923003/diff/140001/webrtc/modules/congestio...
> webrtc/modules/congestion_controller/include/congestion_controller.h:52:
int64_t
> probing_interval_ms) = 0;
> To avoid the build issues with internal projects I'd keep the old method
around
> but deprecate it, and create a new overloaded method with this parameter added
> to it.

Sure. I will do it.