Pace outgoing frames in frame scheduler to match target bitrate.

Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are
rescheduled whenever target bitrate changes.

BUG=645656
Committed: https://crrev.com/af3ebce990837650253c9ef7b2b058e6fa7bc575
Cr-Commit-Position: refs/heads/master@{#423374}

[Sergey Ulanov, 2016-09-29 22:00:22]

Description was changed from

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than network
can deliver, which increases buffer delay.

BUG=645656
==========

to

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are 
rescheduled whenever target bitrate changes.

BUG=645656
==========

[Sergey Ulanov, 2016-09-29 22:00:23]

sergeyu@chromium.org changed reviewers:
+ isheriff@chromium.org

[Irfan, 2016-09-29 22:22:41]

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket.h
File remoting/base/leaky_bucket.h (right):

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket....
remoting/base/leaky_bucket.h:15: // |depth| is in bytes. |rate| is specified in
bytes/second. Zero depth
bytes/second is a bit odd to track (for debug etc.,) since BW is measured in bps
or kbps.

Suggest doing kbps or bps and adding a suffix to the name to clarify (say
xyz_kbps_)

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket....
remoting/base/leaky_bucket.h:40: int level_;
document what level means ?  may be call it current_level

https://codereview.chromium.org/2381153002/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2381153002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:34: : pacing_bucket_(-1,
1000), frame_processing_delay_us_(kStatsWindow) {}
-1 should perhaps be a constant to indicate UNLIMITED_BUFFER or something

why 1000 ?

[Sergey Ulanov, 2016-09-29 23:20:13]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2016-09-29 23:24:46]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2016-09-29 23:25:21]

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket.h
File remoting/base/leaky_bucket.h (right):

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket....
remoting/base/leaky_bucket.h:15: // |depth| is in bytes. |rate| is specified in
bytes/second. Zero depth
On 2016/09/29 22:22:41, Irfan wrote:
> bytes/second is a bit odd to track (for debug etc.,) since BW is measured in
bps
> or kbps.
> 
> Suggest doing kbps or bps and adding a suffix to the name to clarify (say
> xyz_kbps_)

I think it's best to use the same units for depth and rate (I also find it more
natural to measure speed in B/s instead of b/s as it makes all calculation
simpler). And in general depth and rate don't have to be bytes, as this class
can be used to rate-limit anything, not only byte stream. Updated this comment
to say that depth is measured in "drops" and rate is "drops/second", where drops
can be bytes, bits or anything else, depending on the consumer.

https://codereview.chromium.org/2381153002/diff/1/remoting/base/leaky_bucket....
remoting/base/leaky_bucket.h:40: int level_;
On 2016/09/29 22:22:41, Irfan wrote:
> document what level means ?  may be call it current_level

Done.

https://codereview.chromium.org/2381153002/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2381153002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:34: : pacing_bucket_(-1,
1000), frame_processing_delay_us_(kStatsWindow) {}
On 2016/09/29 22:22:41, Irfan wrote:
> -1 should perhaps be a constant to indicate UNLIMITED_BUFFER or something

Done.

> 
> why 1000 ?

Changed it to 0 and updated ScheduleNextFrame() to schedule a frame only when
bitrate > 0

[Irfan, 2016-10-03 19:35:09]

lgtm

[Sergey Ulanov, 2016-10-03 23:36:30]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-10-03 23:37:05]

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

[commit-bot: I haz the power, 2016-10-04 00:16:45]

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

[commit-bot: I haz the power, 2016-10-04 00:16:48]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Sergey Ulanov, 2016-10-05 22:36:53]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-05 22:36:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isheriff@chromium.org
Link to the patchset: https://codereview.chromium.org/2381153002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-10-05 22:37:41]

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

[commit-bot: I haz the power, 2016-10-06 01:15:02]

Description was changed from

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are 
rescheduled whenever target bitrate changes.

BUG=645656
==========

to

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are 
rescheduled whenever target bitrate changes.

BUG=645656
==========

[commit-bot: I haz the power, 2016-10-06 01:15:03]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-10-06 01:18:29]

Description was changed from

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are 
rescheduled whenever target bitrate changes.

BUG=645656
==========

to

==========
Pace outgoing frames in frame scheduler to match target bitrate.

Previously WebrtcFrameSchedulerSimple was scheduling outgoing frames
according to the target bitrate, but it wasn't limiting that bandwidth
precisely, so it was possible for it to send more data than the
network can deliver, which increases bufferbloat. Also now frames are
rescheduled whenever target bitrate changes.

BUG=645656

Committed: https://crrev.com/af3ebce990837650253c9ef7b2b058e6fa7bc575
Cr-Commit-Position: refs/heads/master@{#423374}
==========

[commit-bot: I haz the power, 2016-10-06 01:18:30]

Patchset 3 (id:??) landed as
https://crrev.com/af3ebce990837650253c9ef7b2b058e6fa7bc575
Cr-Commit-Position: refs/heads/master@{#423374}