Adds logging of configuration information for VideoReceiveStream

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

 syntax = "proto2";
 option optimize_for = LITE_RUNTIME;
 package webrtc;
 
 // This is the main message to dump to a file, it can contain multiple event
 // messages, but it is possible to append multiple EventStreams (each with a
 // single event) to a file.
 // This has the benefit that there's no need to keep all data in memory.
 message ACMDumpEventStream {
   repeated ACMDumpEvent stream = 1;
 }
 
 
 message ACMDumpEvent {
   // required - Elapsed wallclock time in us since the start of the log.
   optional int64 timestamp_us = 1;
 
   // 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;
     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 == CONFIG_EVENT
-  optional ACMDumpConfigEvent config = 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 {
+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;
+
+  // 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 ACMDumpRtcpPacket {
+  // 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;
   }
+  // 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
   optional Direction direction = 1;
 
   // 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;
 }
 
 
 message ACMDumpDebugEvent {
   // Indicates the type of the debug event.
   // LOG_START and LOG_END indicate the start and end of the log respectively.
   // AUDIO_PLAYOUT indicates a call to the PlayoutData10Ms() function in ACM.
   enum EventType {
     UNKNOWN_EVENT = 0;
     LOG_START = 1;
     LOG_END = 2;
     AUDIO_PLAYOUT = 3;
   }
 
   // required
   optional EventType type = 1;
 
   // An optional message that can be used to store additional information about
   // the debug event.
   optional string message = 2;
 }
 
 
 // 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 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;
 
-  // RTX settings for incoming video payloads that may be received. RTX is
-  // disabled if there's no config present.
-  optional RtcpConfig rtcp_config = 3;
+  // 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;
+
+  // 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;
 }
 
 
 // Maps decoder names to payload types.
 message DecoderConfig {
   // required
   optional string name = 1;
 
   // required
   optional sint32 payload_type = 2;
 }
 
 
-// Maps RTP header extension names to numerical ids.
+// Maps RTP header extension names to numerical IDs.
 message RtpHeaderExtension {
   // required
   optional string name = 1;
 
   // required
   optional sint32 id = 2;
 }
 
 
 // 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;
 }
 
 
 message RtxMap {
   // required
   optional sint32 payload_type = 1;
 
   // required
   optional RtxConfig config = 2;
 }
 
 
-// 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;
 
-  // Extended RTCP settings.
-  optional bool receiver_reference_time_report = 3;
+  // List of SSRCs for retransmitted packets.
+  repeated uint32 rtx_ssrcs = 3;
 
-  // Receiver estimated maximum bandwidth.
-  optional bool remb = 4;
+  // 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.

+}
+
+
+message ACMDumpAudioSendConfig {
+  // TODO(terelius): Figure out what the requirements are from the audio team.
+}