Added protobuf message for loss-based BWE events, and wired it up to the send side bandwidth estima…

Added protobuf message for loss-based BWE events, and wired it up to the send side bandwidth estimator.

BUG=

Committed: https://crrev.com/006d93d3c679ffd15223c327d649066a72400639
Cr-Commit-Position: refs/heads/master@{#10531}

[terelius, 2015-10-19 08:10:15]

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

[terelius, 2015-10-19 08:10:16]

Please have a look at this CL which adds logging capabilities to the loss-based
send-side bandwidth estimator.

[stefan-webrtc, 2015-10-19 08:43:22]

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp);
Pass in clock_->TimeInMicroseconds() directly.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t
bitrate,
I think we can make this a regular int. Same for total_packets, and maybe even
for fraction_loss?

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:108: // required - Bandwidth estimate after the
update.
unit

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:472: size_t rtcp_index = 1, playout_index
= 1, bwe_loss_index = 1;
Personally, I prefer to have one variable per line.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211:
event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_,
Would there be a point in differentiating between bitrate increases and
decreases in the log?

[ivoc, 2015-10-19 16:03:38]

Nice, looks good. Just a few minor nits.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1;
I don't really understand why there are 2 signed int's and 1 unsigned int in
this message, when as far as I can tell they all represent non-negative numbers.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i <
bwe_loss_count; i++) {
why not just make i an int? I don't see any compelling reason to make it a
size_t (nor in the previous loops for that matter).

[terelius, 2015-10-26 17:40:18]

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp);
On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> Pass in clock_->TimeInMicroseconds() directly.

I agree with this idea for Rtcp and Rtp packets since they are logged far from
the socket, but wouldn't it just be an inconvenience for the caller in the case
of BWE events? I mean, the function which updates the birate estimate calls this
function directly and this function immediately reads the clock.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t
bitrate,
On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> I think we can make this a regular int. Same for total_packets, and maybe even
> for fraction_loss?

The type here should correspond to the protobuf I think. Since the bitrate is an
int32 in the protobuf, there will be an implicit typecast to int32_t when we
access the field.

The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What we
are actually logging is an aggregate of several packets, but the variable still
seems to be an uint8_t in all places it is used.)

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:108: // required - Bandwidth estimate after the
update.
On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> unit

Done. (bps)

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1;
On 2015/10/19 16:03:38, ivoc wrote:
> I don't really understand why there are 2 signed int's and 1 unsigned int in
> this message, when as far as I can tell they all represent non-negative
numbers.

The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The total
expected packets is an int in the C++ code. The bitrate in the send side
estimator is currently unsigned, but the getters and setters are signed. As
negative numbers could potentially be used as some sort of sentinel or special
signal, I think it might be good to allow them in the protobuf.

Personally I agree with you though; I prefer to use unsigned for all values that
should be positive.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i <
bwe_loss_count; i++) {
On 2015/10/19 16:03:38, ivoc wrote:
> why not just make i an int? I don't see any compelling reason to make it a
> size_t (nor in the previous loops for that matter).

No, there is no strong reason for doing it either way in this case. In general,
I prefer to always use size_t for the length of an array and by extension also
for indices into an array.

The google standard is ambiguous on this issue:
"We use int very often, for integers we know are not going to be too big, e.g.,
loop counters."
But also:
"When appropriate, you are welcome to use standard types like size_t"

Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs 927
occurrences of "for (size_t i".

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:472: size_t rtcp_index = 1, playout_index
= 1, bwe_loss_index = 1;
On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote:
> Personally, I prefer to have one variable per line.

Done.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211:
event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_,
On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote:
> Would there be a point in differentiating between bitrate increases and
> decreases in the log?

Since we have log the fraction lost, we are able to figure out whether the
change should be an increase of decrease. (At least assuming that we know
roughly which version of WebRTC is used and the packet loss limits don't change
dramatically between WebRTC versions. Current limits are 2% and 10%.)

Once we start logging all BWE changes we should also be able to see whether the
change was an increase or decrease by comparing whith the previous estimate.

[stefan-webrtc, 2015-10-28 16:03:18]

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp);
On 2015/10/26 17:40:18, terelius wrote:
> On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> > Pass in clock_->TimeInMicroseconds() directly.
> 
> I agree with this idea for Rtcp and Rtp packets since they are logged far from
> the socket, but wouldn't it just be an inconvenience for the caller in the
case
> of BWE events? I mean, the function which updates the birate estimate calls
this
> function directly and this function immediately reads the clock.

Oh, I meant that instead of doing event.set_timestamp_us(timestamp) you could
do:

event.set_timestamp_us(clock_->TimeInMicroseconds());

and save one line. I do see that a temporary variable is used in other places
here though, so if you prefer that way, go for it.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t
bitrate,
On 2015/10/26 17:40:18, terelius wrote:
> On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> > I think we can make this a regular int. Same for total_packets, and maybe
even
> > for fraction_loss?
> 
> The type here should correspond to the protobuf I think. Since the bitrate is
an
> int32 in the protobuf, there will be an implicit typecast to int32_t when we
> access the field.
> 
> The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What we
> are actually logging is an aggregate of several packets, but the variable
still
> seems to be an uint8_t in all places it is used.)

That is true, although I think it's mostly legacy and there isn't any real
reason for using uint8_t or int32_t here. The styleguide suggests using int in
most cases except for sizes and in case you actually need a certain size.

I don't feel very strongly about this though, so if you think it makes sense to
have same types as in the protobuf, then go ahead.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1;
On 2015/10/26 17:40:18, terelius wrote:
> On 2015/10/19 16:03:38, ivoc wrote:
> > I don't really understand why there are 2 signed int's and 1 unsigned int in
> > this message, when as far as I can tell they all represent non-negative
> numbers.
> 
> The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The
total
> expected packets is an int in the C++ code. The bitrate in the send side
> estimator is currently unsigned, but the getters and setters are signed. As
> negative numbers could potentially be used as some sort of sentinel or special
> signal, I think it might be good to allow them in the protobuf.
> 
> Personally I agree with you though; I prefer to use unsigned for all values
that
> should be positive.

I'm trying to move most of the BWE code to use ints instead of unsigned types
where possible to better follow the styleguide:
https://www.chromium.org/developers/coding-style
"Do not use unsigned types to mean "this value should never be < 0". For that,
use assertions or run-time checks (as appropriate)."

That may not apply to protobufs though, actually probably not.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i <
bwe_loss_count; i++) {
On 2015/10/26 17:40:18, terelius wrote:
> On 2015/10/19 16:03:38, ivoc wrote:
> > why not just make i an int? I don't see any compelling reason to make it a
> > size_t (nor in the previous loops for that matter).
> 
> No, there is no strong reason for doing it either way in this case. In
general,
> I prefer to always use size_t for the length of an array and by extension also
> for indices into an array.
> 
> The google standard is ambiguous on this issue:
> "We use int very often, for integers we know are not going to be too big,
e.g.,
> loop counters."
> But also:
> "When appropriate, you are welcome to use standard types like size_t"
> 
> Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs 927
> occurrences of "for (size_t i".

Chromium style is more explicit when it comes to size_t:
https://www.chromium.org/developers/coding-style

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211:
event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_,
On 2015/10/26 17:40:18, terelius wrote:
> On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote:
> > Would there be a point in differentiating between bitrate increases and
> > decreases in the log?
> 
> Since we have log the fraction lost, we are able to figure out whether the
> change should be an increase of decrease. (At least assuming that we know
> roughly which version of WebRTC is used and the packet loss limits don't
change
> dramatically between WebRTC versions. Current limits are 2% and 10%.)
> 
> Once we start logging all BWE changes we should also be able to see whether
the
> change was an increase or decrease by comparing whith the previous estimate.

True, sgtm

[terelius, 2015-10-30 10:51:40]

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp);
On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> On 2015/10/26 17:40:18, terelius wrote:
> > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> > > Pass in clock_->TimeInMicroseconds() directly.
> > 
> > I agree with this idea for Rtcp and Rtp packets since they are logged far
from
> > the socket, but wouldn't it just be an inconvenience for the caller in the
> case
> > of BWE events? I mean, the function which updates the birate estimate calls
> this
> > function directly and this function immediately reads the clock.
> 
> Oh, I meant that instead of doing event.set_timestamp_us(timestamp) you could
> do:
> 
> event.set_timestamp_us(clock_->TimeInMicroseconds());
> 
> and save one line. I do see that a temporary variable is used in other places
> here though, so if you prefer that way, go for it.

Done. Sorry, I misunderstood what you wanted.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t
bitrate,
On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> On 2015/10/26 17:40:18, terelius wrote:
> > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> > > I think we can make this a regular int. Same for total_packets, and maybe
> even
> > > for fraction_loss?
> > 
> > The type here should correspond to the protobuf I think. Since the bitrate
is
> an
> > int32 in the protobuf, there will be an implicit typecast to int32_t when we
> > access the field.
> > 
> > The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What
we
> > are actually logging is an aggregate of several packets, but the variable
> still
> > seems to be an uint8_t in all places it is used.)
> 
> That is true, although I think it's mostly legacy and there isn't any real
> reason for using uint8_t or int32_t here. The styleguide suggests using int in
> most cases except for sizes and in case you actually need a certain size.
> 
> I don't feel very strongly about this though, so if you think it makes sense
to
> have same types as in the protobuf, then go ahead.

It is mostly that I don't think it makes sense to have an unsigned variable,
cast it to signed only to immediately cast it back to unsigned to write it in
the protobuf.

In general, I prefer to make the type casts visible to the user by using the
same type in the logging API as in the protobuf. We could conceivably change the
protobuf to allow signed ints for the fraction loss though, but since the type
is specified in the RFC it  seemed fairly safe to assume that it would not
change.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1;
On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> On 2015/10/26 17:40:18, terelius wrote:
> > On 2015/10/19 16:03:38, ivoc wrote:
> > > I don't really understand why there are 2 signed int's and 1 unsigned int
in
> > > this message, when as far as I can tell they all represent non-negative
> > numbers.
> > 
> > The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The
> total
> > expected packets is an int in the C++ code. The bitrate in the send side
> > estimator is currently unsigned, but the getters and setters are signed. As
> > negative numbers could potentially be used as some sort of sentinel or
special
> > signal, I think it might be good to allow them in the protobuf.
> > 
> > Personally I agree with you though; I prefer to use unsigned for all values
> that
> > should be positive.
> 
> I'm trying to move most of the BWE code to use ints instead of unsigned types
> where possible to better follow the styleguide:
> https://www.chromium.org/developers/coding-style
> "Do not use unsigned types to mean "this value should never be < 0". For that,
> use assertions or run-time checks (as appropriate)."
> 
> That may not apply to protobufs though, actually probably not.

Acknowledged, though I'd like to register my dissatisfaction with the google
style guide on this issue. First of all, the style guide example suggests using
int for loop counters which contradicts chromiums recommendation of always using
size_t. Secondly, the style guide's argument about type conversions between
signed and unsigned being dangerous only implies that signed and unsigned should
not be mixed. Using only unsigned would also solve that problem. Thirdly, the
style guide ignores the fact that the arithmetic for unsigned types is 
mathematically meaningful whereas arithmetic for signed types is highly error
prone and in some cases even semantically undefined.

Consider:
void f(int high, int low) {
  assert(high > low);
  int diff = high - low;
  assert(diff > 0); // This can fail!
}

or
void f(int x, int y) {
  int s = x>>10;  // This is undefined. (If x is negative)
  int t = x<<10;  // This is actually also undefined. (If x*1024>2^31)
}

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni...
webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i <
bwe_loss_count; i++) {
On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> On 2015/10/26 17:40:18, terelius wrote:
> > On 2015/10/19 16:03:38, ivoc wrote:
> > > why not just make i an int? I don't see any compelling reason to make it a
> > > size_t (nor in the previous loops for that matter).
> > 
> > No, there is no strong reason for doing it either way in this case. In
> general,
> > I prefer to always use size_t for the length of an array and by extension
also
> > for indices into an array.
> > 
> > The google standard is ambiguous on this issue:
> > "We use int very often, for integers we know are not going to be too big,
> e.g.,
> > loop counters."
> > But also:
> > "When appropriate, you are welcome to use standard types like size_t"
> > 
> > Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs
927
> > occurrences of "for (size_t i".
> 
> Chromium style is more explicit when it comes to size_t:
> https://www.chromium.org/developers/coding-style

Acknowledged.

[stefan-webrtc, 2015-10-30 10:58:04]

lgtm

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h
File webrtc/call/rtc_event_log.h (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n...
webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t
bitrate,
On 2015/10/30 10:51:40, terelius wrote:
> On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> > On 2015/10/26 17:40:18, terelius wrote:
> > > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote:
> > > > I think we can make this a regular int. Same for total_packets, and
maybe
> > even
> > > > for fraction_loss?
> > > 
> > > The type here should correspond to the protobuf I think. Since the bitrate
> is
> > an
> > > int32 in the protobuf, there will be an implicit typecast to int32_t when
we
> > > access the field.
> > > 
> > > The fraction lost field is specified in RFC 3550 as an 8 bit integer.
(What
> we
> > > are actually logging is an aggregate of several packets, but the variable
> > still
> > > seems to be an uint8_t in all places it is used.)
> > 
> > That is true, although I think it's mostly legacy and there isn't any real
> > reason for using uint8_t or int32_t here. The styleguide suggests using int
in
> > most cases except for sizes and in case you actually need a certain size.
> > 
> > I don't feel very strongly about this though, so if you think it makes sense
> to
> > have same types as in the protobuf, then go ahead.
> 
> It is mostly that I don't think it makes sense to have an unsigned variable,
> cast it to signed only to immediately cast it back to unsigned to write it in
> the protobuf.
> 
> In general, I prefer to make the type casts visible to the user by using the
> same type in the logging API as in the protobuf. We could conceivably change
the
> protobuf to allow signed ints for the fraction loss though, but since the type
> is specified in the RFC it  seemed fairly safe to assume that it would not
> change.

Agree it doesn't make sense to cast just to cast back later, so let's keep as is
then.

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto
File webrtc/call/rtc_event_log.proto (right):

https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro...
webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1;
On 2015/10/30 10:51:40, terelius wrote:
> On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote:
> > On 2015/10/26 17:40:18, terelius wrote:
> > > On 2015/10/19 16:03:38, ivoc wrote:
> > > > I don't really understand why there are 2 signed int's and 1 unsigned
int
> in
> > > > this message, when as far as I can tell they all represent non-negative
> > > numbers.
> > > 
> > > The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The
> > total
> > > expected packets is an int in the C++ code. The bitrate in the send side
> > > estimator is currently unsigned, but the getters and setters are signed.
As
> > > negative numbers could potentially be used as some sort of sentinel or
> special
> > > signal, I think it might be good to allow them in the protobuf.
> > > 
> > > Personally I agree with you though; I prefer to use unsigned for all
values
> > that
> > > should be positive.
> > 
> > I'm trying to move most of the BWE code to use ints instead of unsigned
types
> > where possible to better follow the styleguide:
> > https://www.chromium.org/developers/coding-style
> > "Do not use unsigned types to mean "this value should never be < 0". For
that,
> > use assertions or run-time checks (as appropriate)."
> > 
> > That may not apply to protobufs though, actually probably not.
> 
> Acknowledged, though I'd like to register my dissatisfaction with the google
> style guide on this issue. First of all, the style guide example suggests
using
> int for loop counters which contradicts chromiums recommendation of always
using
> size_t. Secondly, the style guide's argument about type conversions between
> signed and unsigned being dangerous only implies that signed and unsigned
should
> not be mixed. Using only unsigned would also solve that problem. Thirdly, the
> style guide ignores the fact that the arithmetic for unsigned types is 
> mathematically meaningful whereas arithmetic for signed types is highly error
> prone and in some cases even semantically undefined.
> 
> Consider:
> void f(int high, int low) {
>   assert(high > low);
>   int diff = high - low;
>   assert(diff > 0); // This can fail!
> }
> 
> or
> void f(int x, int y) {
>   int s = x>>10;  // This is undefined. (If x is negative)
>   int t = x<<10;  // This is actually also undefined. (If x*1024>2^31)
> }

Totally agree that there are situations where unsigned makes sense. Especially
fixed sizes, such as uint16_t.

How can the first example fail?

[terelius, 2015-10-30 11:08:33]

> > Acknowledged, though I'd like to register my dissatisfaction with the google
> > style guide on this issue. First of all, the style guide example suggests
> using
> > int for loop counters which contradicts chromiums recommendation of always
> using
> > size_t. Secondly, the style guide's argument about type conversions between
> > signed and unsigned being dangerous only implies that signed and unsigned
> should
> > not be mixed. Using only unsigned would also solve that problem. Thirdly,
the
> > style guide ignores the fact that the arithmetic for unsigned types is 
> > mathematically meaningful whereas arithmetic for signed types is highly
error
> > prone and in some cases even semantically undefined.
> > 
> > Consider:
> > void f(int high, int low) {
> >   assert(high > low);
> >   int diff = high - low;
> >   assert(diff > 0); // This can fail!
> > }
> > 
> > or
> > void f(int x, int y) {
> >   int s = x>>10;  // This is undefined. (If x is negative)
> >   int t = x<<10;  // This is actually also undefined. (If x*1024>2^31)
> > }
> 
> Totally agree that there are situations where unsigned makes sense. Especially
> fixed sizes, such as uint16_t.
> 
> How can the first example fail?

Because the (positive) difference between two ints might be larger than what is
representable as an int. We actually had an instance of this bug in the random
number generator recently. Basically a scaling factor could become negative
which caused our uniform random numbers to be neither uniform, nor in the
requested range.

[ivoc, 2015-11-03 15:14:30]

LGTM.

[terelius, 2015-11-05 17:56:46]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2015-11-05 17:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1411673003/#ps80001 (title:
"Rebase")

[commit-bot: I haz the power, 2015-11-05 17:56:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411673003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411673003/80001

[commit-bot: I haz the power, 2015-11-05 20:02:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-05 20:02:25]

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