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

webrtc/call/rtc_event_log.proto

 syntax = "proto2";
 option optimize_for = LITE_RUNTIME;
 package webrtc.rtclog;
 
 
 enum MediaType {
   ANY = 0;
   AUDIO = 1;
   VIDEO = 2;
   DATA = 3;
@@ skipping 16 matching lines @@
   // The different types of events that can occur, the UNKNOWN_EVENT entry
   // is added in case future EventTypes are added, in that case old code will
   // receive the new events as UNKNOWN_EVENT.
   enum EventType {
     UNKNOWN_EVENT = 0;
     LOG_START = 1;
     LOG_END = 2;
     RTP_EVENT = 3;
     RTCP_EVENT = 4;
     AUDIO_PLAYOUT_EVENT = 5;
-    VIDEO_RECEIVER_CONFIG_EVENT = 6;
-    VIDEO_SENDER_CONFIG_EVENT = 7;
-    AUDIO_RECEIVER_CONFIG_EVENT = 8;
-    AUDIO_SENDER_CONFIG_EVENT = 9;
+    BWE_PACKET_LOSS_EVENT = 6;
+    BWE_PACKET_DELAY_EVENT = 7;
+    VIDEO_RECEIVER_CONFIG_EVENT = 8;
+    VIDEO_SENDER_CONFIG_EVENT = 9;
+    AUDIO_RECEIVER_CONFIG_EVENT = 10;
+    AUDIO_SENDER_CONFIG_EVENT = 11;
   }
 
   // required - Indicates the type of this event
   optional EventType type = 2;
 
   // optional - but required if type == RTP_EVENT
   optional RtpPacket rtp_packet = 3;
 
   // optional - but required if type == RTCP_EVENT
   optional RtcpPacket rtcp_packet = 4;
 
   // optional - but required if type == AUDIO_PLAYOUT_EVENT
   optional AudioPlayoutEvent audio_playout_event = 5;
 
+  // optional - but required if type == BWE_PACKET_LOSS_EVENT
+  optional BwePacketLossEvent bwe_packet_loss_event = 6;
+
   // optional - but required if type == VIDEO_RECEIVER_CONFIG_EVENT
-  optional VideoReceiveConfig video_receiver_config = 6;
+  optional VideoReceiveConfig video_receiver_config = 8;
 
   // optional - but required if type == VIDEO_SENDER_CONFIG_EVENT
-  optional VideoSendConfig video_sender_config = 7;
+  optional VideoSendConfig video_sender_config = 9;
 
   // optional - but required if type == AUDIO_RECEIVER_CONFIG_EVENT
-  optional AudioReceiveConfig audio_receiver_config = 8;
+  optional AudioReceiveConfig audio_receiver_config = 10;
 
   // optional - but required if type == AUDIO_SENDER_CONFIG_EVENT
-  optional AudioSendConfig audio_sender_config = 9;
+  optional AudioSendConfig audio_sender_config = 11;
 }
 
 
 message RtpPacket {
   // required - True if the packet is incoming w.r.t. the user logging the data
   optional bool incoming = 1;
 
   // required
   optional MediaType type = 2;
 
@@ skipping 16 matching lines @@
 
   // required - The whole packet including both payload and header.
   optional bytes packet_data = 3;
 }
 
 message AudioPlayoutEvent {
   // required - The SSRC of the audio stream associated with the playout event.
   optional uint32 local_ssrc = 2;
 }
 
+message BwePacketLossEvent {
+  // required - Bandwidth estimate after the update.

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

unit

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

Done. (bps)

+  optional sint32 bitrate = 1;

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

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.

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

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.

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

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.

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

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)
}

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

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

How can the first example fail?

+
+  // required - Fraction of lost packets since last receiver report
+  // computed as floor( 256 * (#lost_packets / #total_packets) ).
+  // The possible values range from 0 to 255.
+  optional uint32 fraction_loss = 2;
+
+  // TODO(terelius): Is this really needed? Remove or make optional?
+  // required - Total number of packets that the BWE update is based on.
+  optional sint32 total_packets = 3;
+}
 
 // TODO(terelius): Video and audio streams could in principle share SSRC,
 // so identifying a stream based only on SSRC might not work.
 // It might be better to use a combination of SSRC and media type
 // or SSRC and port number, but for now we will rely on SSRC only.
 message VideoReceiveConfig {
   // required - Synchronization source (stream identifier) to be received.
   optional uint32 remote_ssrc = 1;
   // required - Sender SSRC used for sending RTCP (such as receiver reports).
   optional uint32 local_ssrc = 2;
@@ skipping 109 matching lines @@
 }
 
 
 message AudioSendConfig {
   // required - Synchronization source (stream identifier) for outgoing stream.
   optional uint32 ssrc = 1;
 
   // RTP header extensions used for the outgoing audio stream.
   repeated RtpHeaderExtension header_extensions = 2;
 }