Adds logging of configuration information for VideoReceiveStream

webrtc/modules/audio_coding/main/acm2/dump.proto

Index: webrtc/modules/audio_coding/main/acm2/dump.proto
diff --git a/webrtc/modules/audio_coding/main/acm2/dump.proto b/webrtc/modules/audio_coding/main/acm2/dump.proto
index 232faec42871cfc20ef55ffc6f6e6b48c338eec0..ff1eb20f825b96b71a26cf572c5d77d2e1b5ef0c 100644
--- a/webrtc/modules/audio_coding/main/acm2/dump.proto
+++ b/webrtc/modules/audio_coding/main/acm2/dump.proto
@@ -21,25 +21,73 @@ message ACMDumpEvent {
   enum EventType {
     UNKNOWN_EVENT = 0;
     RTP_EVENT = 1;
-    DEBUG_EVENT = 2;
-    CONFIG_EVENT = 3;
+    RTCP_EVENT = 2;
+    DEBUG_EVENT = 3;
+    RECEIVER_CONFIG_EVENT = 4;
+    SENDER_CONFIG_EVENT = 5;
+    AUDIO_RECEIVER_CONFIG_EVENT = 6;
+    AUDIO_SENDER_CONFIG_EVENT = 7;
   }
 
   // required - Indicates the type of this event
   optional EventType type = 2;
 
   // optional - but required if type == RTP_EVENT
-  optional ACMDumpRTPPacket packet = 3;
+  optional ACMDumpRtpPacket rtp_packet = 3;
+
+  // optional - but required if type == RTCP_EVENT
+  optional ACMDumpRtcpPacket rtcp_packet = 4;
 
   // optional - but required if type == DEBUG_EVENT
-  optional ACMDumpDebugEvent debug_event = 4;
+  optional ACMDumpDebugEvent debug_event = 5;
+
+  // optional - but required if type == RECEIVER_CONFIG_EVENT
+  optional ACMDumpVideoReceiveConfig receiver_config = 6;
+
+  // optional - but required if type == SENDER_CONFIG_EVENT
+  optional ACMDumpVideoSendConfig sender_config = 7;
+
+  // optional - but required if type == AUDIO_RECEIVER_CONFIG_EVENT
+  optional ACMDumpAudioReceiveConfig audio_receiver_config = 8;
+
+  // optional - but required if type == AUDIO_SENDER_CONFIG_EVENT
+  optional ACMDumpAudioSendConfig audio_sender_config = 9;
+}
+
+
+message ACMDumpRtpPacket {
+  // Indicates if the packet is incoming or outgoing with respect to the user
+  // that is logging the data.
+  enum Direction {
+    UNKNOWN_DIRECTION = 0;
+    OUTGOING = 1;
+    INCOMING = 2;
+  }
+  enum PayloadType {
+    UNKNOWN_TYPE = 0;
+    AUDIO = 1;
+    VIDEO = 2;
+    RTX = 3;  // TODO(terelius): Where does this get set?

[ivoc, 2015/07/14 12:13:14]

Nowhere yet :-) We should change that.

[terelius, 2015/07/16 12:47:03]

I mean that I don't think the RTX flag makes any sense and that it should be
removed. That is, the way I am using the PayloadType field is to set it based on
MediaType with the valid media types being ANY, AUDIO, VIDEO and DATA.

We should be able to decide whether the packet is a retransmission by parsing
the header, no? 

[ivoc, 2015/07/17 12:14:28]

Okay, makes sense, feel free to remove it in that case.

+  }
+
+  // required
+  optional Direction direction = 1;
+
+  // required
+  optional PayloadType type = 2;
+
+  // required - The size of the packet including both payload and header.
+  optional uint32 packet_length = 3;
 
-  // optional - but required if type == CONFIG_EVENT
-  optional ACMDumpConfigEvent config = 5;
+  // required - The RTP header only.
+  optional bytes header = 4;
+
+  // Logging payloads for user data requires privacy review. Don't uncomment.
+  // optional bytes payload = 5;

[stefan-webrtc, 2015/07/14 13:28:56]

Can we simply not add this until we actually need it?

[ivoc, 2015/07/14 13:58:03]

+1

[terelius, 2015/07/16 12:47:03]

I would *very* strongly advise against this. We should not
create privacy invasive code unless we are actually ready
use it. See e.g.
http://www.wired.com/2012/05/google-wifi-fcc-investigation/
for a previous case where accidental logging of payload data
led to a lot of problems and bad press for Google.

[ivoc, 2015/07/17 12:14:28]

I think we were trying to argue that these two lines should be removed from the
file, not that we should start logging whole packets. 

I agree that we need to do a privacy review before doing any sort of logging of
whole RTP headers.

[terelius, 2015/07/17 17:48:33]

I missed this comment in my last reply.

Based on the word "until", I interpreted Stefan's suggestion as that we should
have a field in the protobuf for full packets, but not use it until later. I
think that sounds very dangerous.

If you want to remove the field itself, I wouldn't mind but I would still prefer
to leave a comment that reminds anyone who might be reading the code that we
should not log user data without some serious consideration of privacy issues.

[minyue-webrtc, 2015/07/21 12:22:46]

I do not know the privacy policy well enough. But it feels to me that, if we put
these two lines, it is like saying "hey, we already considered how to break it."
So I prefer to remove them

 }
 
 
-message ACMDumpRTPPacket {
+message ACMDumpRtcpPacket {
   // Indicates if the packet is incoming or outgoing with respect to the user
   // that is logging the data.
   enum Direction {
@@ -47,11 +95,11 @@ message ACMDumpRTPPacket {
     OUTGOING = 1;
     INCOMING = 2;
   }
+  // RTX is not used for RTCP packets.

[stefan-webrtc, 2015/07/14 13:28:56]

I don't think this is necessary to comment on.

[terelius, 2015/07/16 12:47:03]

Done.

   enum PayloadType {
     UNKNOWN_TYPE = 0;
     AUDIO = 1;
     VIDEO = 2;
-    RTX = 3;
   }
 
   // required
@@ -60,8 +108,8 @@ message ACMDumpRTPPacket {
   // required
   optional PayloadType type = 2;
 
-  // required - Contains the whole RTP packet (header+payload).
-  optional bytes RTP_data = 3;
+  // required - The whole packet including both payload and header.
+  optional bytes data = 3;
 }
 
 
@@ -89,22 +137,35 @@ message ACMDumpDebugEvent {
 // 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 ACMDumpConfigEvent {
-  // Synchronization source (stream identifier) to be received.
+message ACMDumpVideoReceiveConfig {
+  // 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;
+
+  // Compound mode is described by RFC 4585 and reduced-size
+  // RTCP mode is described by RFC 5506.
+  enum RtcpMode {
+    RTCP_COMPOUND = 1;
+    RTCP_REDUCEDSIZE = 2;
+  }
+  // required - RTCP mode to use.
+  optional RtcpMode rtcp_mode = 3;
+
+  // required - Extended RTCP settings.
+  optional bool receiver_reference_time_report = 4;
 
-  // RTX settings for incoming video payloads that may be received. RTX is
-  // disabled if there's no config present.
-  optional RtcpConfig rtcp_config = 3;
+  // required - Receiver estimated maximum bandwidth.
+  optional bool remb = 5;
 
   // Map from video RTP payload type -> RTX config.
-  repeated RtxMap rtx_map = 4;
+  repeated RtxMap rtx_map = 6;
 
   // RTP header extensions used for the received stream.
-  repeated RtpHeaderExtension header_extensions = 5;
+  repeated RtpHeaderExtension header_extensions = 7;
 
   // List of decoders associated with the stream.
-  repeated DecoderConfig decoders = 6;
+  repeated DecoderConfig decoders = 8;
 }
 
 
@@ -118,7 +179,7 @@ message DecoderConfig {
 }
 
 
-// Maps RTP header extension names to numerical ids.
+// Maps RTP header extension names to numerical IDs.
 message RtpHeaderExtension {
   // required
   optional string name = 1;
@@ -131,11 +192,11 @@ message RtpHeaderExtension {
 // RTX settings for incoming video payloads that may be received.
 // RTX is disabled if there's no config present.
 message RtxConfig {
-  // required - SSRCs to use for the RTX streams.
-  optional uint32 ssrc = 1;
+  // required - SSRC to use for the RTX stream.
+  optional uint32 rtx_ssrc = 1;
 
   // required - Payload type to use for the RTX stream.
-  optional sint32 payload_type = 2;
+  optional sint32 rtx_payload_type = 2;
 }
 
 
@@ -148,22 +209,44 @@ message RtxMap {
 }
 
 
-// Configuration information for RTCP.
-// For bandwidth estimation purposes it is more interesting to log the
-// RTCP messages that the sender receives, but we will support logging
-// at the receiver side too.
-message RtcpConfig {
-  // Sender SSRC used for sending RTCP (such as receiver reports).
-  optional uint32 local_ssrc = 1;
+message ACMDumpVideoSendConfig {
+  // Synchronization source (stream identifier) for outgoing stream.
+  // One stream can have several ssrcs for e.g. simulcast.
+  // At least one ssrc is required.
+  repeated uint32 ssrcs = 1;
 
-  // RTCP mode to use. Compound mode is described by RFC 4585 and reduced-size
-  // RTCP mode is described by RFC 5506.
-  enum RtcpMode {RTCP_COMPOUND = 1; RTCP_REDUCEDSIZE = 2;}
-  optional RtcpMode rtcp_mode = 2;
+  // RTP header extensions used for the outgoing stream.
+  repeated RtpHeaderExtension header_extensions = 2;
+
+  // List of SSRCs for retransmitted packets.
+  repeated uint32 rtx_ssrcs = 3;
+
+  // required if rtx_ssrcs is used - Payload type for retransmitted packets.
+  optional sint32 rtx_payload_type = 4;
+
+  // required - Canonical end-point identifier.
+  optional string c_name = 5;
+
+  // required - Encoder associated with the stream.
+  optional EncoderConfig encoder = 6;
+}
+
+
+// Maps encoder names to payload types.
+message EncoderConfig {
+  // required
+  optional string name = 1;
+
+  // required
+  optional sint32 payload_type = 2;
+}
+
+
+message ACMDumpAudioReceiveConfig {
+  // TODO(terelius): Figure out what the requirements are from the audio team.

[ivoc, 2015/07/14 12:13:14]

I will try to look into that.

[terelius, 2015/07/16 12:47:03]

Acknowledged.

+}
 
-  // Extended RTCP settings.
-  optional bool receiver_reference_time_report = 3;
 
-  // Receiver estimated maximum bandwidth.
-  optional bool remb = 4;
+message ACMDumpAudioSendConfig {
+  // TODO(terelius): Figure out what the requirements are from the audio team.
 }