Refactor NACK bitrate allocation

Refactor NACK bitrate allocation

Nack bitrate allocation should not be done on a per-rtp-module basis,
but rather shared bitrate pool per call. This CL moves allocation to the
pacer and cleans up a bunch if bitrate stats handling.

BUG=
R=danilchap@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/5fc59e810b8ea848702f2dbc740fe190608e1413

[sprang_webrtc, 2016-06-20 15:35:48]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org

[sprang_webrtc, 2016-06-20 15:39:41]

sprang@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[sprang_webrtc, 2016-06-20 15:39:42]

+mflodman for FYI and cleanup of dead code in webrtc/common_types.h

[danilchap, 2016-06-20 16:53:46]

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:289: uint32_t* nackRate) const = 0;
this would break downstream code.

[sprang_webrtc, 2016-06-23 07:54:28]

sprang@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[sprang_webrtc, 2016-06-23 07:54:29]

+danil as reviewer, perhaps you want to take a first pass?

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:289: uint32_t* nackRate) const = 0;
On 2016/06/20 16:53:46, danilchap wrote:
> this would break downstream code.

Acknowledged.

[danilchap, 2016-06-23 12:46:12]

First pass

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

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:35: const int64_t kMaxRtt = 1000;
kMaxRttMs

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:265: retransmission_rate_(kMaxRtt, 8) { 
// Scale 8 for bits per second.
shouldn't this be 8000 instead of 8? (scale is for 8bps not 8bpms)

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:342: int64_t max_retransmission_rate_bps =
rate (bits/s) * rtt (ms) should result in bits, not bits/s
Can you add a comment how you get bits/s or why is it valid to compare bits vs
bits/s next line

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

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.h:114: bool
AllocateRetransmissionBitrate(size_t bytes) override;
this function doesn't seem to allocate(reserve) bitrate(speed), but checks if
retransmission is allowed because of bitrate.
It also assume the packet would be then retransmitted and updates rate because
of it.
May be remove this function from interface, and call it from InsertPacket
instead (that would be allowed to fail now).
i.e. in PacedPacket::InsertPacket 
have code like
if (retransmission) {
  if (!HasBandwidthForRetransmission()) {
    return false;
  }
  retransmission_rate_.Update(bytes, now_ms);
}

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:395: virtual int
CurrentRetransmissionBitrate() = 0;
maybe call it CurrentRetransmisisonBitrateBps to put units directly into
function name instead of comment in a derived class.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:18: #include
"webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h"
alphabetical order

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:734: int rtx = kRtxOff;
what about the use case where RTPSender is used without pacer?

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:409: rtc::CriticalSection
stats_critsect_;
may be use already existent statistics_crit_  instead (and avoid locking two
critsect in UpdateRtpStats)

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:45: fec_bitrate_(1000, 8000),
 // 1000ms window, bits/s scale.
8000 is so popular constant for RateStatistic class. May be put it as constant
into it?
Or at least name this constants in this file, something like
namespace {
constexpr int64_t kMaxWindowSizeMs = 1000;
constexpr float kBpsScale = 8000.0;
}  // namespace

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:352: return
fec_bitrate_.Rate(clock_->TimeInMilliseconds()).value_or(0);
may be better make RateStatistic::Rate function const? (e.g. using mutables)
that function may change some internal caching variables, but doesn't change
logical state of the RateStatistic, i.e. calling RateStatistic::Rate doesn't
affect results of any further calls... as long as time is monotonic.
(It is also possible to change Rate function to calculate same value without
changing internal variables, but it would make code more complicated and less
efficient)

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:577: // printf("@Nack bitrate for %u is
%u\n", ssrc, retransmit_bitrate_bps);
do not forget to remove this line

[sprang_webrtc, 2016-06-28 09:12:33]

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

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:265: retransmission_rate_(kMaxRtt, 8) { 
// Scale 8 for bits per second.
On 2016/06/23 12:46:12, danilchap wrote:
> shouldn't this be 8000 instead of 8? (scale is for 8bps not 8bpms)

Yes.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:342: int64_t max_retransmission_rate_bps =
On 2016/06/23 12:46:12, danilchap wrote:
> rate (bits/s) * rtt (ms) should result in bits, not bits/s
> Can you add a comment how you get bits/s or why is it valid to compare bits vs
> bits/s next line

Not sure what happened here, thought I had removed this code. I changed it to
update the window size of the rate limiter instead and just use rate limit. Not
sure how this (incorrect) code ended up back here...

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

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.h:114: bool
AllocateRetransmissionBitrate(size_t bytes) override;
On 2016/06/23 12:46:12, danilchap wrote:
> this function doesn't seem to allocate(reserve) bitrate(speed), but checks if
> retransmission is allowed because of bitrate.
> It also assume the packet would be then retransmitted and updates rate because
> of it.
> May be remove this function from interface, and call it from InsertPacket
> instead (that would be allowed to fail now).
> i.e. in PacedPacket::InsertPacket 
> have code like
> if (retransmission) {
>   if (!HasBandwidthForRetransmission()) {
>     return false;
>   }
>   retransmission_rate_.Update(bytes, now_ms);
> }

Actually, having this in the pacer didn't work well. I've moved it out to a
separate interface and renamed it RateLimiter.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:395: virtual int
CurrentRetransmissionBitrate() = 0;
On 2016/06/23 12:46:12, danilchap wrote:
> maybe call it CurrentRetransmisisonBitrateBps to put units directly into
> function name instead of comment in a derived class.

Done. And removed. :)

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:18: #include
"webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h"
On 2016/06/23 12:46:12, danilchap wrote:
> alphabetical order

Done.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:734: int rtx = kRtxOff;
On 2016/06/23 12:46:12, danilchap wrote:
> what about the use case where RTPSender is used without pacer?

You're right, this wouldn't worked. Moved this func out of pacer.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:409: rtc::CriticalSection
stats_critsect_;
On 2016/06/23 12:46:12, danilchap wrote:
> may be use already existent statistics_crit_  instead (and avoid locking two
> critsect in UpdateRtpStats)

Done.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:45: fec_bitrate_(1000, 8000),
 // 1000ms window, bits/s scale.
On 2016/06/23 12:46:12, danilchap wrote:
> 8000 is so popular constant for RateStatistic class. May be put it as constant
> into it?
> Or at least name this constants in this file, something like
> namespace {
> constexpr int64_t kMaxWindowSizeMs = 1000;
> constexpr float kBpsScale = 8000.0;
> }  // namespace

Done.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:352: return
fec_bitrate_.Rate(clock_->TimeInMilliseconds()).value_or(0);
On 2016/06/23 12:46:12, danilchap wrote:
> may be better make RateStatistic::Rate function const? (e.g. using mutables)
> that function may change some internal caching variables, but doesn't change
> logical state of the RateStatistic, i.e. calling RateStatistic::Rate doesn't
> affect results of any further calls... as long as time is monotonic.
> (It is also possible to change Rate function to calculate same value without
> changing internal variables, but it would make code more complicated and less
> efficient)

Yeah. It's a bit ugly but probably worth it.

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2061423003/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:577: // printf("@Nack bitrate for %u is
%u\n", ssrc, retransmit_bitrate_bps);
On 2016/06/23 12:46:12, danilchap wrote:
> do not forget to remove this line

Oops!

[danilchap, 2016-06-28 14:26:18]

Mostly nits:

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/base/rate_statisti...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/base/rate_statisti...
webrtc/base/rate_statistics.cc:80: rtc::Optional<uint32_t>
RateStatistics::Rate(int64_t now_ms) const {
May be it would be slightly cleaner if only one Rate function would remain
(const version) and const_cast would be used to call EraseOld.

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

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:176: max_rate_bps_
= max_bitrate_bps;
maybe rename max_rate_bps_ to max_bitrate_bps_
or max_bitrate_bps to max_rate_bps
to make names look more consistant.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:194:
RTC_DISALLOW_COPY_AND_ASSIGN(RateLimiter);
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RateLimiter) since you are not using default
constructor too.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:236:
kMaxRetransmitWindowSizeMs,
RateLimiter is local class, and window size/bitrate are initialized with same
constants. May be use this constants inside RateLimiter constructor class
instead of passing them as parameters. Would make CongestionController
constructors cleaner.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:367: int64_t
nack_window_size = max_rtt_ms;
nack_window_size_ms

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

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:35: class
RateLimiter;
this class is defined inside congestion_controller.cc i.e. local to
ConestionController.
probably better to declare it inside CongestionController class:
class CongestionControler : ...
 private:
  class RateLimiter;
  ...
  const std::unique<RateLimiter> nack_rate_limiter_;

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (left):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:34: 
may be revert this change to make CL slightly smaller.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:400: class NackRateLimiter {
Probably Retransmission instead of Nack.
Since it limits retransmitting packets, not rtcp nack packets.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:406: // current rate.
bytes -> |bytes|.
May be rename it to |packet_size_bytes|

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:33:
incoming_bitrate_(kStatisticsProcessIntervalMs, 8000),
RateStatistic::kBpsScale instead 8000?
Comment might not be needed then.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:800: if (bytes_sent == 0) {
this block not needed now.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:484: uint32_t target_bitrate_
GUARDED_BY(target_bitrate_critsect_);
Is target_bitrate_ still in use?

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1: /*
May be add a test with non-null (Nack/Retransmission)RateLimiter

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2687: for (auto& it : stats_map) {
const auto&
may be name variable kv [key_value] or stat (it is no longer iterator. Thought
'it' might mean 'it'...)

[sprang_webrtc, 2016-07-04 09:33:04]

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/base/rate_statisti...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/base/rate_statisti...
webrtc/base/rate_statistics.cc:80: rtc::Optional<uint32_t>
RateStatistics::Rate(int64_t now_ms) const {
On 2016/06/28 14:26:17, danilchap wrote:
> May be it would be slightly cleaner if only one Rate function would remain
> (const version) and const_cast would be used to call EraseOld.

Done.

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

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:176: max_rate_bps_
= max_bitrate_bps;
On 2016/06/28 14:26:17, danilchap wrote:
> maybe rename max_rate_bps_ to max_bitrate_bps_
> or max_bitrate_bps to max_rate_bps
> to make names look more consistant.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:194:
RTC_DISALLOW_COPY_AND_ASSIGN(RateLimiter);
On 2016/06/28 14:26:17, danilchap wrote:
> RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RateLimiter) since you are not using
default
> constructor too.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:236:
kMaxRetransmitWindowSizeMs,
On 2016/06/28 14:26:17, danilchap wrote:
> RateLimiter is local class, and window size/bitrate are initialized with same
> constants. May be use this constants inside RateLimiter constructor class
> instead of passing them as parameters. Would make CongestionController
> constructors cleaner.

True. This was a leftover from a partial refactoring.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:367: int64_t
nack_window_size = max_rtt_ms;
On 2016/06/28 14:26:17, danilchap wrote:
> nack_window_size_ms

Done.

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

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/congestion_controller.h:35: class
RateLimiter;
On 2016/06/28 14:26:18, danilchap wrote:
> this class is defined inside congestion_controller.cc i.e. local to
> ConestionController.
> probably better to declare it inside CongestionController class:
> class CongestionControler : ...
>  private:
>   class RateLimiter;
>   ...
>   const std::unique<RateLimiter> nack_rate_limiter_;
> 
>     

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (left):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:34: 
On 2016/06/28 14:26:18, danilchap wrote:
> may be revert this change to make CL slightly smaller.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:400: class NackRateLimiter {
On 2016/06/28 14:26:18, danilchap wrote:
> Probably Retransmission instead of Nack.
> Since it limits retransmitting packets, not rtcp nack packets.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:406: // current rate.
On 2016/06/28 14:26:18, danilchap wrote:
> bytes -> |bytes|.
> May be rename it to |packet_size_bytes|

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:33:
incoming_bitrate_(kStatisticsProcessIntervalMs, 8000),
On 2016/06/28 14:26:18, danilchap wrote:
> RateStatistic::kBpsScale instead 8000?
> Comment might not be needed then.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:800: if (bytes_sent == 0) {
On 2016/06/28 14:26:18, danilchap wrote:
> this block not needed now.

Done.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:484: uint32_t target_bitrate_
GUARDED_BY(target_bitrate_critsect_);
On 2016/06/28 14:26:18, danilchap wrote:
> Is target_bitrate_ still in use?

Right, this was a big part of why I did this refactoring. I'd intended to remove
this separately, but not sure that makes sense. Ripped a bunch of things out
with it.

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1: /*
On 2016/06/28 14:26:18, danilchap wrote:
> May be add a test with non-null (Nack/Retransmission)RateLimiter

I fixed the nack rate test already here, so it actually tests what is says it
does...

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2061423003/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2687: for (auto& it : stats_map) {
On 2016/06/28 14:26:18, danilchap wrote:
> const auto&
> may be name variable kv [key_value] or stat (it is no longer iterator. Thought
> 'it' might mean 'it'...)

Done.

[danilchap, 2016-07-04 12:22:57]

lgtm % nit

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:29: static const int64_t kWindowSize =
1000;
constexpr kWindowSizeMs

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:30: static const uint32_t kMaxRateBps =
100000;
constexpr

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:37:
EXPECT_TRUE(rate_limiter->TryUseRate(kMaxRateBps / 16));
Can you expand this line to avoid thinking that TryRate tries to use kmaxRATEbps
devided by magical constant 16:

TryRate(bitrate/16) actually means
size_t kSizeFillingBandwidthBytes = (kMaxRateBps / kBitsInByte) * (kWindowSize /
kMsInS);  // last multiplier = 1, but explains how bytes/s got converted into
bytes
rate_limiter->TryUseRate(kSizeFillingBandwidthBytes / 2);

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:54:
EXPECT_TRUE(rate_limiter->TryUseRate(kMaxRateBps / 16));
same here (which hints constant with packet size that fills kMaxRateBps alone
may be in RateLimitTest.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:28: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
with RateLimiter moved to base this is no longer needed (together with
congestion_constroller -> rtp_rtcp dependency)

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:30: class RtcEventLog;
almost :) one more line above.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1280: const uint32_t
KexpectedRateBps =
kE instead of Ke

[sprang_webrtc, 2016-07-04 12:46:59]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org
- pbos@webrtc.org

[sprang_webrtc, 2016-07-04 12:47:01]

+stefan -pbos

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:29: static const int64_t kWindowSize =
1000;
On 2016/07/04 12:22:57, danilchap wrote:
> constexpr kWindowSizeMs

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:30: static const uint32_t kMaxRateBps =
100000;
On 2016/07/04 12:22:57, danilchap wrote:
> constexpr

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:37:
EXPECT_TRUE(rate_limiter->TryUseRate(kMaxRateBps / 16));
On 2016/07/04 12:22:57, danilchap wrote:
> Can you expand this line to avoid thinking that TryRate tries to use
kmaxRATEbps
> devided by magical constant 16:
> 
> TryRate(bitrate/16) actually means
> size_t kSizeFillingBandwidthBytes = (kMaxRateBps / kBitsInByte) * (kWindowSize
/
> kMsInS);  // last multiplier = 1, but explains how bytes/s got converted into
> bytes
> rate_limiter->TryUseRate(kSizeFillingBandwidthBytes / 2);
> 

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:54:
EXPECT_TRUE(rate_limiter->TryUseRate(kMaxRateBps / 16));
On 2016/07/04 12:22:57, danilchap wrote:
> same here (which hints constant with packet size that fills kMaxRateBps alone
> may be in RateLimitTest.

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:28: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
On 2016/07/04 12:22:57, danilchap wrote:
> with RateLimiter moved to base this is no longer needed (together with
> congestion_constroller -> rtp_rtcp dependency)

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:30: class RtcEventLog;
On 2016/07/04 12:22:57, danilchap wrote:
> almost :) one more line above.

Done.

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1280: const uint32_t
KexpectedRateBps =
On 2016/07/04 12:22:57, danilchap wrote:
> kE instead of Ke

Done.

[sprang_webrtc, 2016-07-06 12:07:17]

ping stefan?

[stefan-webrtc, 2016-07-06 12:43:21]

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:36: // allowed due to too high bitrate caused by a
single packet.
When is there not a current rate? Does this always happen on low packet rates?
Sounds dangerous to always allow retransmits in that case.

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:65:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
Isn't mutable an option instead?

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:338:
retransmission_rate_limiter_->SetWindowSize(nack_window_size_ms);
CongestionController doesn't know anything about retransmits/nack today, and I'm
not sure it makes sense that it should?

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:710: if
(retransmission_rate_limiter_ &&
Should probably just CHECK on having the rate limiter?

[sprang_webrtc, 2016-07-06 14:13:07]

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:36: // allowed due to too high bitrate caused by a
single packet.
On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> When is there not a current rate? Does this always happen on low packet rates?
> Sounds dangerous to always allow retransmits in that case.

Less than one packet per window (1pps). How would we otherwise deal with the
situation that (for instance) estimated rate is 40kbps, RTT is 250ms and a nack
arrives for packet of 1500 bytes. That would indicate a retransmission rate of
1500 * 8 / 0.250 = 48kbps and so would always be denied...

I think one packet per RTT is OK, even if it technically is too much.

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:65:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> Isn't mutable an option instead?

We'd have to declare more or less all members as mutable then. I think this is
cleaner. Will add comment to that effect.

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/congestio...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/congestio...
webrtc/modules/congestion_controller/congestion_controller.cc:338:
retransmission_rate_limiter_->SetWindowSize(nack_window_size_ms);
On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> CongestionController doesn't know anything about retransmits/nack today, and
I'm
> not sure it makes sense that it should?

Maybe not. It had all the needed data to do the job though.
Moved it to Call instead. OK?

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:710: if
(retransmission_rate_limiter_ &&
On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> Should probably just CHECK on having the rate limiter?

Done.

[sprang_webrtc, 2016-07-07 12:09:14]

sprang@webrtc.org changed reviewers:
+ tommi@webrtc.org
- mflodman@webrtc.org

[sprang_webrtc, 2016-07-07 12:09:15]

As per offline discussion, moved rate limiter back to congestion controller.
ptal

-mflodman +tommi as owner for base files.

[tommi, 2016-07-07 12:48:47]

took a quick look

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:25: // current rate.
keep the function level documentation in the header

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:27: rtc::CritScope cs(&lock_);
can you add documentation about threading? Why not single threaded for instance?
On what threads do we expect the method to be called when not the constructing
thread?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:44: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
calling out to a different implementation while holding a lock. Would be good to
avoid that.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:51: max_rate_bps_ = max_rate_bps;
If this variable is changed on the same thread as TryUseRate is called on, we
don't need a lock.  If we do need a lock, please document what will be going on.
(great if we could dcheck somehow what we expect to be happening)

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:56: void RateLimiter::SetWindowSize(int64_t
window_size_ms) {
same for this method. keep the method level documentation in the header,
document the threading model well.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:59: current_rate_.SetWindowSize(window_size_ms,
clock_->TimeInMilliseconds());
calling into a different implementation while holding a lock... does this
SetWindowSize grab a lock by any chance?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.h
File webrtc/base/rate_limiter.h (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.h:24: class RateLimiter {
documentation?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.h:27: virtual ~RateLimiter();
why virtual?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:20: class RateLimitTest : public
::testing::Test {
since the class that these tests are testing, is currently implemented to be
multithreaded, it would be good to have tests for that.  As is, all the tests
appear to be single threaded.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:67:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
is it correct to call EraseOld in this method?
If so, then the data that EraseOld erases, should be mutable.  Does it belong
together in a nested, class perhaps (and accessed via a mutable member)?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:712:
!retransmission_rate_limiter_->TryUseRate(length))
{}

(can you run git cl format?)

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:113: const
rtc::CriticalSection stats_crit_;
const critical section?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2527: const int kPacketLossFrac = 25;  // Drop
every 25th packet => 4% loss.
should be packet_loss_frac or static const int kPacketLossFrac

[sprang_webrtc, 2016-07-07 13:46:51]

Tommi, you seem to really really dislike that I made RateLimiter thread safe. As
noted in comments below, I did this in order to avoid having everything be
virtual and then using a synchronizing wrapper, but it sounds like you may
prefer me resurrecting that?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:25: // current rate.
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> keep the function level documentation in the header

Done.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:27: rtc::CritScope cs(&lock_);
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> can you add documentation about threading? Why not single threaded for
instance?
> On what threads do we expect the method to be called when not the constructing
> thread?

In the initial use case, TryUseRate() will be called from the network thread,
SetWindowSize() from module processing thread (rtcp module) and SetMaxRate from
the bandwidth estimator (unsure which thread).

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:44: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> calling out to a different implementation while holding a lock. Would be good
to
> avoid that.

I looked into bolting rate limiting logic onto RateStatistics, but didn't like
it. Also really don't want to duplicate the RateStatistics logic here, so
settled for this wrapper around the rate tracker instead.
I guess we could just move this class into the same file as RateStatistics,
but...

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:51: max_rate_bps_ = max_rate_bps;
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> If this variable is changed on the same thread as TryUseRate is called on, we
> don't need a lock.  If we do need a lock, please document what will be going
on.
> (great if we could dcheck somehow what we expect to be happening)

For the initial use case at least, SetMaxRate() and TryUseRate() will not be
called from the same thread. Same for SetWindowSize().

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:56: void RateLimiter::SetWindowSize(int64_t
window_size_ms) {
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> same for this method. keep the method level documentation in the header,
> document the threading model well.

Done.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:59: current_rate_.SetWindowSize(window_size_ms,
clock_->TimeInMilliseconds());
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> calling into a different implementation while holding a lock... does this
> SetWindowSize grab a lock by any chance?

Nope. RateStatistics does not use any locks.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.h
File webrtc/base/rate_limiter.h (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.h:24: class RateLimiter {
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> documentation?

Done.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.h:27: virtual ~RateLimiter();
On 2016/07/07 12:48:46, tommi-webrtc wrote:
> why virtual?

I had the methods be virtual before, in order to be able to add a synchronized
wrapper, but it got kinda ugly so I just made this whole class synchronized
instead.
Removed virtual.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:20: class RateLimitTest : public
::testing::Test {
On 2016/07/07 12:48:47, tommi-webrtc wrote:
> since the class that these tests are testing, is currently implemented to be
> multithreaded, it would be good to have tests for that.  As is, all the tests
> appear to be single threaded.

Since all methods are synchronized, and with the same lock, all access into the
class is serialized so I'm not quite sure what you're hoping to accomplish with
a multi threaded test?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:67:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
On 2016/07/07 12:48:47, tommi-webrtc wrote:
> is it correct to call EraseOld in this method?
> If so, then the data that EraseOld erases, should be mutable.  Does it belong
> together in a nested, class perhaps (and accessed via a mutable member)?

EraseOld() moves the averaging window. It may mutate every non-const member
apart form current_window_size_ms_. Note sure it's worth creating a wrapper
class for this.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:712:
!retransmission_rate_limiter_->TryUseRate(length))
On 2016/07/07 12:48:47, tommi-webrtc wrote:
> {}
> 
> (can you run git cl format?)

Done.
git cl format was run. It doesn't add braces on its own though.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:113: const
rtc::CriticalSection stats_crit_;
On 2016/07/07 12:48:47, tommi-webrtc wrote:
> const critical section?

Copied from |crit_| above. A bit odd, I'll remove const from both.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2527: const int kPacketLossFrac = 25;  // Drop
every 25th packet => 4% loss.
On 2016/07/07 12:48:47, tommi-webrtc wrote:
> should be packet_loss_frac or static const int kPacketLossFrac

Done.

[tommi, 2016-07-07 15:27:37]

On 2016/07/07 13:46:51, språng wrote:
> Tommi, you seem to really really dislike that I made RateLimiter thread safe.
As
> noted in comments below, I did this in order to avoid having everything be
> virtual and then using a synchronizing wrapper, but it sounds like you may
> prefer me resurrecting that?

Those are good things, so definitely don't dislike that.  Sorry my comments came
off so negative.
I took a quick look and the new code in base looked like new code to me.  I like
it when the usage expectations of a class are known, understood and can be
tested.  If the implementation has to protect itself against any eventuality, it
makes it harder to test and harder to understand what the program flow should
be. It also has negative performance implications.  So, if we can document and
[d]check how the class is expected to be used, that helps future proof the code.

[tommi, 2016-07-08 08:52:20]

looks good - I still have a few minor questions so please bear with me.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:27: rtc::CritScope cs(&lock_);
On 2016/07/07 13:46:50, språng wrote:
> On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > can you add documentation about threading? Why not single threaded for
> instance?
> > On what threads do we expect the method to be called when not the
constructing
> > thread?
> 
> In the initial use case, TryUseRate() will be called from the network thread,
> SetWindowSize() from module processing thread (rtcp module) and SetMaxRate
from
> the bandwidth estimator (unsure which thread).

Thanks, that's useful information.  What about putting that here as a comment? 
Knowing usage patterns like that can allow us to make improvements such as
having unlocked reads on the only thread/execution context that modifies a
variable.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:44: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
On 2016/07/07 13:46:50, språng wrote:
> On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > calling out to a different implementation while holding a lock. Would be
good
> to
> > avoid that.
> 
> I looked into bolting rate limiting logic onto RateStatistics, but didn't like
> it. Also really don't want to duplicate the RateStatistics logic here, so
> settled for this wrapper around the rate tracker instead.
> I guess we could just move this class into the same file as RateStatistics,
> but...

Might make sense at some point in the future.  This approach is a good step in
the right direction.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:59: current_rate_.SetWindowSize(window_size_ms,
clock_->TimeInMilliseconds());
On 2016/07/07 13:46:50, språng wrote:
> On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > calling into a different implementation while holding a lock... does this
> > SetWindowSize grab a lock by any chance?
> 
> Nope. RateStatistics does not use any locks.

Thanks.  I took a closer look at how these classes are related (I didn't have
time to do that yesterday unfortuantely), so this makes a lot more sense to me
now.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.h
File webrtc/base/rate_limiter.h (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.h:27: virtual ~RateLimiter();
On 2016/07/07 13:46:51, språng wrote:
> On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > why virtual?
> 
> I had the methods be virtual before, in order to be able to add a synchronized
> wrapper, but it got kinda ugly so I just made this whole class synchronized
> instead.
> Removed virtual.

great.  I think that's an improvement and the fact that it's not virtual at all
makes it clearer to the reader that wysiwyg. (haven't used that acronym in a
while)

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:20: class RateLimitTest : public
::testing::Test {
On 2016/07/07 13:46:51, språng wrote:
> On 2016/07/07 12:48:47, tommi-webrtc wrote:
> > since the class that these tests are testing, is currently implemented to be
> > multithreaded, it would be good to have tests for that.  As is, all the
tests
> > appear to be single threaded.
> 
> Since all methods are synchronized, and with the same lock, all access into
the
> class is serialized so I'm not quite sure what you're hoping to accomplish
with
> a multi threaded test?

I'm thinking about it from a regression point of view (as well as correct
behavior I guess). If the implementation is modified, e.g. someone forgets to
grab a lock while accessing a member variable then a multithreaded test running
on tsan could catch that regression.  It sounds like you're reviewing the code
and from inspection assuming that everything is correct.  It might be, but code
review doesn't catch everything.

Generally speaking, if a class is supposed to be multithreaded, I think some
sort of a test that represents multithreaded usage of that class, would be a
good thing.
---
As a more practical question for this cl, would this result in an expected
state?

RateLimiter limiter(...);

void Thread1() {
  int64_t max_rtt_ms = GetMaxRttMs();  // Runs first.
  Sleep(1000);  // Simulate context switch.
  limiter.SetWindowSize(max_rtt_ms);  // Runs second - sets the oldest max.
}

void Thread2() {
  Sleep(1000);  // Simulate context switch.
  int64_t max_rtt_ms = GetMaxRttMs(); // Runs second.
  limiter.SetWindowSize(max_rtt_ms);// Runs first, sets the newest max.
}

StartBothThreads();
WaitForThreadsToExit();
EXPECT_TRUE(WindowSizeIsCorrectAccordingToTheLatestBitrate());


If not and if SetWindowSize() must always be called from the same thread, then
that can be checked.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:66: // of the members as mutable...
there is one more alternative :)

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:67:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
On 2016/07/07 13:46:51, språng wrote:
> On 2016/07/07 12:48:47, tommi-webrtc wrote:
> > is it correct to call EraseOld in this method?
> > If so, then the data that EraseOld erases, should be mutable.  Does it
belong
> > together in a nested, class perhaps (and accessed via a mutable member)?
> 
> EraseOld() moves the averaging window. It may mutate every non-const member
> apart form current_window_size_ms_. Note sure it's worth creating a wrapper
> class for this.

ok. given this and that there's a comment that explains that despite being
const, the method modifies the state, I wonder if it should be const?  What
about making it non-const and explain why it's not const?  (I might be missing a
const context that this method needs to be callable from)

nit: can we change the name of the function to GetRate?

https://codereview.webrtc.org/2061423003/diff/200001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/200001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:42: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
nit: just call TimeInMilliseconds once

[sprang_webrtc, 2016-07-08 11:46:59]

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:27: rtc::CritScope cs(&lock_);
On 2016/07/08 08:52:20, tommi-webrtc wrote:
> On 2016/07/07 13:46:50, språng wrote:
> > On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > > can you add documentation about threading? Why not single threaded for
> > instance?
> > > On what threads do we expect the method to be called when not the
> constructing
> > > thread?
> > 
> > In the initial use case, TryUseRate() will be called from the network
thread,
> > SetWindowSize() from module processing thread (rtcp module) and SetMaxRate
> from
> > the bandwidth estimator (unsure which thread).
> 
> Thanks, that's useful information.  What about putting that here as a comment?

> Knowing usage patterns like that can allow us to make improvements such as
> having unlocked reads on the only thread/execution context that modifies a
> variable.

Done.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:44: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
On 2016/07/08 08:52:20, tommi-webrtc wrote:
> On 2016/07/07 13:46:50, språng wrote:
> > On 2016/07/07 12:48:46, tommi-webrtc wrote:
> > > calling out to a different implementation while holding a lock. Would be
> good
> > to
> > > avoid that.
> > 
> > I looked into bolting rate limiting logic onto RateStatistics, but didn't
like
> > it. Also really don't want to duplicate the RateStatistics logic here, so
> > settled for this wrapper around the rate tracker instead.
> > I guess we could just move this class into the same file as RateStatistics,
> > but...
> 
> Might make sense at some point in the future.  This approach is a good step in
> the right direction.

Acknowledged.

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:20: class RateLimitTest : public
::testing::Test {
On 2016/07/08 08:52:20, tommi-webrtc wrote:
> On 2016/07/07 13:46:51, språng wrote:
> > On 2016/07/07 12:48:47, tommi-webrtc wrote:
> > > since the class that these tests are testing, is currently implemented to
be
> > > multithreaded, it would be good to have tests for that.  As is, all the
> tests
> > > appear to be single threaded.
> > 
> > Since all methods are synchronized, and with the same lock, all access into
> the
> > class is serialized so I'm not quite sure what you're hoping to accomplish
> with
> > a multi threaded test?
> 
> I'm thinking about it from a regression point of view (as well as correct
> behavior I guess). If the implementation is modified, e.g. someone forgets to
> grab a lock while accessing a member variable then a multithreaded test
running
> on tsan could catch that regression.  It sounds like you're reviewing the code
> and from inspection assuming that everything is correct.  It might be, but
code
> review doesn't catch everything.
> 
> Generally speaking, if a class is supposed to be multithreaded, I think some
> sort of a test that represents multithreaded usage of that class, would be a
> good thing.
> ---
> As a more practical question for this cl, would this result in an expected
> state?
> 
> RateLimiter limiter(...);
> 
> void Thread1() {
>   int64_t max_rtt_ms = GetMaxRttMs();  // Runs first.
>   Sleep(1000);  // Simulate context switch.
>   limiter.SetWindowSize(max_rtt_ms);  // Runs second - sets the oldest max.
> }
> 
> void Thread2() {
>   Sleep(1000);  // Simulate context switch.
>   int64_t max_rtt_ms = GetMaxRttMs(); // Runs second.
>   limiter.SetWindowSize(max_rtt_ms);// Runs first, sets the newest max.
> }
> 
> StartBothThreads();
> WaitForThreadsToExit();
> EXPECT_TRUE(WindowSizeIsCorrectAccordingToTheLatestBitrate());
> 
> 
> If not and if SetWindowSize() must always be called from the same thread, then
> that can be checked.

I see. I implemented a test case where each of the public methods are called on
separate threads, in order to catch data race regressions with tsan. wdyt?

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/180001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:67:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
On 2016/07/08 08:52:20, tommi-webrtc wrote:
> On 2016/07/07 13:46:51, språng wrote:
> > On 2016/07/07 12:48:47, tommi-webrtc wrote:
> > > is it correct to call EraseOld in this method?
> > > If so, then the data that EraseOld erases, should be mutable.  Does it
> belong
> > > together in a nested, class perhaps (and accessed via a mutable member)?
> > 
> > EraseOld() moves the averaging window. It may mutate every non-const member
> > apart form current_window_size_ms_. Note sure it's worth creating a wrapper
> > class for this.
> 
> ok. given this and that there's a comment that explains that despite being
> const, the method modifies the state, I wonder if it should be const?  What
> about making it non-const and explain why it's not const?  (I might be missing
a
> const context that this method needs to be callable from)
> 
> nit: can we change the name of the function to GetRate? 

The big reason for this is that I'm deleting the old
webrtc/modules/rtp_rtcp/source/bitrate.cc 
That implementation had a few problems, one of them being that you needed to
register a module processing thread to actually update the state.

If you look at the first few patch sets, I indeed kept this as non-const method,
but that resulted in needing to change a number of the base rtp/rtcp interfaces,
breaking upstream code and other ugliness. Felt this was preferable...

https://codereview.webrtc.org/2061423003/diff/200001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/200001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:42: current_rate_.Update(packet_size_bytes,
clock_->TimeInMilliseconds());
On 2016/07/08 08:52:20, tommi-webrtc wrote:
> nit: just call TimeInMilliseconds once

Done.

[stefan-webrtc, 2016-07-08 11:55:58]

lgtm

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:36: // allowed due to too high bitrate caused by a
single packet.
On 2016/07/06 14:13:07, språng wrote:
> On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> > When is there not a current rate? Does this always happen on low packet
rates?
> > Sounds dangerous to always allow retransmits in that case.
> 
> Less than one packet per window (1pps). How would we otherwise deal with the
> situation that (for instance) estimated rate is 40kbps, RTT is 250ms and a
nack
> arrives for packet of 1500 bytes. That would indicate a retransmission rate of
> 1500 * 8 / 0.250 = 48kbps and so would always be denied...
> 
> I think one packet per RTT is OK, even if it technically is too much.

Agree... :)

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
File webrtc/base/rate_statistics.cc (right):

https://codereview.webrtc.org/2061423003/diff/140001/webrtc/base/rate_statist...
webrtc/base/rate_statistics.cc:65:
const_cast<RateStatistics*>(this)->EraseOld(now_ms);
On 2016/07/06 14:13:07, språng wrote:
> On 2016/07/06 12:43:21, stefan-webrtc (holmer) wrote:
> > Isn't mutable an option instead?
> 
> We'd have to declare more or less all members as mutable then. I think this is
> cleaner. Will add comment to that effect.

Acknowledged.

[tommi, 2016-07-08 14:07:04]

just one sanity check and then I'll shut up.

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:30: int64_t now_ms = clock_->TimeInMilliseconds();
just double checking, since I thought we were previously fetching the time
inside the lock, on purpose.

What if this happens:

thread1: Gets timestamp = 1, runs out of quanta before grabbing the lock.
thread2: Gets timestamp = 2 and grabs the lock.

thread2 now calls Rate(2), current_rate_.Update(..., 2), etc.  and releases the
lock.

thread1 now grabs the lock, calls Rate(1), , current_rate_.Update(..., 1), etc. 
and releases the lock.

The timestamps will essentially be out of order and not monotonically increasing
as seen by current_rate_.  Is that OK?

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
File webrtc/base/rate_limiter_unittest.cc (right):

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:104: TEST_F(RateLimitTest,
WindowSizeLimits) {
nice

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter_unittest.cc:139: TEST_F(RateLimitTest,
MultiThreadedUsage) {
thanks!

[sprang_webrtc, 2016-07-08 14:16:51]

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter.cc
File webrtc/base/rate_limiter.cc (right):

https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
webrtc/base/rate_limiter.cc:30: int64_t now_ms = clock_->TimeInMilliseconds();
On 2016/07/08 14:07:04, tommi-webrtc wrote:
> just double checking, since I thought we were previously fetching the time
> inside the lock, on purpose.
> 
> What if this happens:
> 
> thread1: Gets timestamp = 1, runs out of quanta before grabbing the lock.
> thread2: Gets timestamp = 2 and grabs the lock.
> 
> thread2 now calls Rate(2), current_rate_.Update(..., 2), etc.  and releases
the
> lock.
> 
> thread1 now grabs the lock, calls Rate(1), , current_rate_.Update(..., 1),
etc. 
> and releases the lock.
> 
> The timestamps will essentially be out of order and not monotonically
increasing
> as seen by current_rate_.  Is that OK?

Actually, yes. Discussed with Stefan yesterday another case where this could
happen for RateStatistics. But I think I'd actually sleep better if I moved
reading the time back to inside the holding the lock.

[tommi, 2016-07-08 14:17:36]

On 2016/07/08 14:16:51, språng wrote:
>
https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter.cc
> File webrtc/base/rate_limiter.cc (right):
> 
>
https://codereview.webrtc.org/2061423003/diff/220001/webrtc/base/rate_limiter...
> webrtc/base/rate_limiter.cc:30: int64_t now_ms = clock_->TimeInMilliseconds();
> On 2016/07/08 14:07:04, tommi-webrtc wrote:
> > just double checking, since I thought we were previously fetching the time
> > inside the lock, on purpose.
> > 
> > What if this happens:
> > 
> > thread1: Gets timestamp = 1, runs out of quanta before grabbing the lock.
> > thread2: Gets timestamp = 2 and grabs the lock.
> > 
> > thread2 now calls Rate(2), current_rate_.Update(..., 2), etc.  and releases
> the
> > lock.
> > 
> > thread1 now grabs the lock, calls Rate(1), , current_rate_.Update(..., 1),
> etc. 
> > and releases the lock.
> > 
> > The timestamps will essentially be out of order and not monotonically
> increasing
> > as seen by current_rate_.  Is that OK?
> 
> Actually, yes. Discussed with Stefan yesterday another case where this could
> happen for RateStatistics. But I think I'd actually sleep better if I moved
> reading the time back to inside the holding the lock.

me too.  lgtm with that

[sprang_webrtc, 2016-07-08 14:45:22]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-07-08 14:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2061423003/#ps300001 (title:
"Fixed nit")

[commit-bot: I haz the power, 2016-07-08 14:45:29]

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-07-08 16:15:35]

Description was changed from

==========
Refactor NACK bitrate allocation

Nack bitrate allocation should not be done on a per-rtp-module basis,
but rather shared bitrate pool per call. This CL moves allocation to the
pacer and cleans up a bunch if bitrate stats handling.

BUG=
==========

to

==========
Refactor NACK bitrate allocation

Nack bitrate allocation should not be done on a per-rtp-module basis,
but rather shared bitrate pool per call. This CL moves allocation to the
pacer and cleans up a bunch if bitrate stats handling.

BUG=
R=danilchap@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/5fc59e810b8ea848702f2dbc740fe190608e1413
Cr-Commit-Position: refs/heads/master@{#13416}
==========

[commit-bot: I haz the power, 2016-07-08 16:15:37]

Patchset 16 (id:??) landed as
https://crrev.com/5fc59e810b8ea848702f2dbc740fe190608e1413
Cr-Commit-Position: refs/heads/master@{#13416}

[sprang_webrtc, 2016-07-08 16:15:37]

Description was changed from

==========
Refactor NACK bitrate allocation

Nack bitrate allocation should not be done on a per-rtp-module basis,
but rather shared bitrate pool per call. This CL moves allocation to the
pacer and cleans up a bunch if bitrate stats handling.

BUG=
R=danilchap@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/5fc59e810b8ea848702f2dbc740fe190608e1413
Cr-Commit-Position: refs/heads/master@{#13416}
==========

to

==========
Refactor NACK bitrate allocation

Nack bitrate allocation should not be done on a per-rtp-module basis,
but rather shared bitrate pool per call. This CL moves allocation to the
pacer and cleans up a bunch if bitrate stats handling.

BUG=
R=danilchap@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/5fc59e810b8ea848702f2dbc7...
==========

[sprang_webrtc, 2016-07-08 16:15:40]

Committed patchset #16 (id:300001) manually as
5fc59e810b8ea848702f2dbc740fe190608e1413 (presubmit successful).

[sprang_webrtc, 2016-07-08 16:38:37]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.webrtc.org/2131913003/ by sprang@webrtc.org.

The reason for reverting is: Breaks upstream code..