Add AlrDetector

Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable probing.

BUG=webrtc:6332
Committed: https://crrev.com/3168781d2fb06af02cfaad453e74fac7832c3cf9
Cr-Commit-Position: refs/heads/master@{#14505}

[Irfan, 2016-09-14 22:05:05]

Description was changed from

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

BUG=webrtc:6332
==========

to

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

BUG=webrtc:6332
==========

[Irfan, 2016-09-14 22:05:05]

isheriff@chromium.org changed reviewers:
+ philipel@webrtc.org, stefan@webrtc.org

[Irfan, 2016-09-14 22:05:13]

PTAL

[philipel, 2016-09-15 15:00:20]

I would prefer if the PacedSender did not know about the AlrDetector.

I would suggest that we change PacedSender::SendPacket and
PacedSender::SendPadding to return the number of byts sent, and then at the end
of PacedSender::Process we use a callback to report how many bytes that were
sent (and maybe at what bitrate as well). CongestionController can implement
that callback and then update the AlrDetector.

Also, I don't think we have to worry about the PacedSender not sending anything
periodically since we always send padding at a min bitrate.

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:38: : total_bytes_sent_(0),
I think |total_bytes_sent_| is not really correct, something like 
|measurement_interval_bytes_sent_| maybe?

Same for |total_elapsed_time_ms_|

WDYT?

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:46: RTC_DCHECK(delta_time_ms);
RTC_DCHECK_GT(delta_time_ms, 0);

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:70: total_bytes_sent_ += bytes;
I think we should combine ReportBytesSent and ReportElapsedTime so that every
time we report that more bits have been sent we also calculate the accumulated
possible bits sent and then use that value to calculate the utilization.

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.h:33: void ReportBytesSent(size_t bytes);
Internally we try to keep everything in bits. I know that PacedSender use bytes
but we want to move away from that.

[Irfan, 2016-09-19 06:50:26]

Patchset #2 (id:20001) has been deleted

[Irfan, 2016-09-19 06:51:14]

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:38: : total_bytes_sent_(0),
On 2016/09/15 15:00:20, philipel wrote:
> I think |total_bytes_sent_| is not really correct, something like 
> |measurement_interval_bytes_sent_| maybe?
> 
> Same for |total_elapsed_time_ms_|
> 
> WDYT?

Done.

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:46: RTC_DCHECK(delta_time_ms);
On 2016/09/15 15:00:20, philipel wrote:
> RTC_DCHECK_GT(delta_time_ms, 0);

Done.

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.cc:70: total_bytes_sent_ += bytes;
On 2016/09/15 15:00:20, philipel wrote:
> I think we should combine ReportBytesSent and ReportElapsedTime so that every
> time we report that more bits have been sent we also calculate the accumulated
> possible bits sent and then use that value to calculate the utilization.

Done.

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2340763004/diff/1/webrtc/modules/pacing/alr_det...
webrtc/modules/pacing/alr_detector.h:33: void ReportBytesSent(size_t bytes);
On 2016/09/15 15:00:20, philipel wrote:
> Internally we try to keep everything in bits. I know that PacedSender use
bytes
> but we want to move away from that.

I think we should keep that change outside this since that requires changing a
bunch of places.

(Orthogonal to this change, am not sure changing everything to bits makes it
more readable. IMHO, packet size is best measured in bytes to be able to compare
to MTU and such)

[Irfan, 2016-09-19 07:06:30]

Patchset #2 (id:40001) has been deleted

[Irfan, 2016-09-19 07:08:29]

PTAL. Rebased on top of Issue 2347023002 to leverage the changes made there.

[philipel, 2016-09-19 11:25:23]

lg

https://codereview.webrtc.org/2340763004/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/2340763004/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.h:56: class PacingObserver {
Add a unittest that use the PacingObserver.

https://codereview.webrtc.org/2340763004/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2340763004/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:112: send_bucket_.reset(new
PacedSender(&clock_, &callback_, nullptr));
PacingObserver can't be null right?

I think we should check in PacedSender::Process if the PacingObserver is not
null and only then do the callback. Then we can add a ctor to PacedSender with
the old signature.

[Irfan, 2016-09-19 22:15:03]

https://codereview.chromium.org/2340763004/diff/60001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.chromium.org/2340763004/diff/60001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.h:56: class PacingObserver {
On 2016/09/19 11:25:22, philipel wrote:
> Add a unittest that use the PacingObserver.

Done.

https://codereview.chromium.org/2340763004/diff/60001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.chromium.org/2340763004/diff/60001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender_unittest.cc:112: send_bucket_.reset(new
PacedSender(&clock_, &callback_, nullptr));
On 2016/09/19 11:25:23, philipel wrote:
> PacingObserver can't be null right?
> 
> I think we should check in PacedSender::Process if the PacingObserver is not
> null and only then do the callback. Then we can add a ctor to PacedSender with
> the old signature.

Done.

[Irfan, 2016-09-19 23:14:23]

PTAL

[philipel, 2016-09-20 08:53:07]

lgtm

[Irfan, 2016-09-22 18:05:59]

Patchset #4 (id:100001) has been deleted

[Irfan, 2016-09-22 18:07:21]

Stefan, PTAL

[stefan-webrtc, 2016-09-23 13:38:31]

Add a comment in the description that this is a noop at the moment since it's
not used.

[stefan-webrtc, 2016-09-23 13:39:02]

Should we add a UMA metric so we can understand how well this detector works?

[stefan-webrtc, 2016-09-23 13:41:15]

One final question:
Do you think there's any potential problem that this detector will detect ALR
when we're exponentially ramping up (multipluicative increase mode)? The encoder
will likely not be able to keep up with our MI, so we may end up ALR for a
moment. Maybe a good thing as we may want to trigger probing anyway, but I
wonder if it could cause problems?

[Irfan, 2016-09-26 23:17:40]

Description was changed from

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

BUG=webrtc:6332
==========

to

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable
probing.

BUG=webrtc:6332
==========

[Irfan, 2016-09-26 23:20:52]

I think the UMA makes sense for a follow up CL that leverages the AlrDetector to
do probing. Right now, it does nothing.

On your question of detecting ALR in MI mode: yes, if the sender is unable to
keep up with the target bitrate. But, currently our detection logic for ALR is
quite conservative (sending should be < 30% of the actual bitrate). 

The plan with ALR itself is again to be conservative until it is verified to
work well. Basically, initiate probing when there is a BW drop and probing has
not happened for a long time.

[Irfan, 2016-09-26 23:20:52]

I think the UMA makes sense for a follow up CL that leverages the AlrDetector to
do probing. Right now, it does nothing.

On your question of detecting ALR in MI mode: yes, if the sender is unable to
keep up with the target bitrate. But, currently our detection logic for ALR is
quite conservative (sending should be < 30% of the actual bitrate). 

The plan with ALR itself is again to be conservative until it is verified to
work well. Basically, initiate probing when there is a BW drop and probing has
not happened for a long time.

[stefan-webrtc, 2016-09-27 14:20:29]

On 2016/09/26 23:20:52, Irfan wrote:
> I think the UMA makes sense for a follow up CL that leverages the AlrDetector
to
> do probing. Right now, it does nothing.
> 
> On your question of detecting ALR in MI mode: yes, if the sender is unable to
> keep up with the target bitrate. But, currently our detection logic for ALR is
> quite conservative (sending should be < 30% of the actual bitrate). 
> 
> The plan with ALR itself is again to be conservative until it is verified to
> work well. Basically, initiate probing when there is a BW drop and probing has
> not happened for a long time.

Yes. We should keep the MI case in mind though and run a test later to ensure it
doesn't trigger unexpectedly.

[stefan-webrtc, 2016-09-27 14:26:09]

LG, just a question about where we're placing this component.

https://codereview.chromium.org/2340763004/diff/120001/webrtc/modules/pacing/...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.chromium.org/2340763004/diff/120001/webrtc/modules/pacing/...
webrtc/modules/pacing/paced_sender.h:56: class PacingObserver {
Wouldn't it be simpler to let the PacedSender own the AlrDetector to avoid this
Observer interface? Anyone who wants to know if we are app limited could then
ask the PacedSender. Seems a bit involved right now.

[Irfan, 2016-09-29 08:33:22]

PTAL

[stefan-webrtc, 2016-09-29 09:19:08]

lg, but a question regarding threading.

https://codereview.chromium.org/2340763004/diff/140001/webrtc/modules/pacing/...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.chromium.org/2340763004/diff/140001/webrtc/modules/pacing/...
webrtc/modules/pacing/alr_detector.cc:77: bool
AlrDetector::InApplicationLimitedRegion() {
What thread are you expecting this to be called on? I'm trying to understand if
we need locks for all of this. If we'd make sure SetEstimatedBitrate() is called
by the pacer thread (the one calling PacedSender::Process()), we may be able to
get rid of the lock.

[Irfan, 2016-09-29 21:26:41]

Patchset #6 (id:160001) has been deleted

[Irfan, 2016-09-29 21:42:23]

Patchset #6 (id:180001) has been deleted

[Irfan, 2016-09-29 21:46:38]

PTAL. The locking in alr_detector is a remnant from integrating in congestion
controller. Paced sender can ensure a lock before accessing alr.

[stefan-webrtc, 2016-09-30 08:23:55]

lgtm

[Irfan, 2016-10-03 19:26:56]

Patchset #7 (id:220001) has been deleted

[Irfan, 2016-10-04 15:32:46]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-10-04 15:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2340763004/#ps240001 (title:
"Rebased")

[commit-bot: I haz the power, 2016-10-04 15:32:54]

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-10-04 15:43:11]

Description was changed from

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable
probing.

BUG=webrtc:6332
==========

to

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable
probing.

BUG=webrtc:6332
==========

[commit-bot: I haz the power, 2016-10-04 15:43:12]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2016-10-04 15:43:21]

Description was changed from

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable
probing.

BUG=webrtc:6332
==========

to

==========
Add AlrDetector

This is a simple application limited region detector that is quite conservative
at the moment. We detect as being application-limited if we see sending rate
as less than 30% of the estimated bandwidth over 500 ms. The moment we detect
a single burst above 30% over a 100 ms period, we consider ourselves network
limited.

This class is currently not used. A follow up CL will leverage this to enable
probing.

BUG=webrtc:6332

Committed: https://crrev.com/3168781d2fb06af02cfaad453e74fac7832c3cf9
Cr-Commit-Position: refs/heads/master@{#14505}
==========

[commit-bot: I haz the power, 2016-10-04 15:43:22]

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