Added fields for configuration information to the protobuf format

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;
 }
 
+

[stefan-webrtc, 2015/06/24 11:46:16]

nit: Do we need multiple (2) empty lines between each message?

[terelius, 2015/06/26 11:20:26]

Personally I think it improves readability since it is easier to see where each
message ends, especially when the different fields of a message are separated by
empty lines too.

The amount of white space is not specified by the protobuf style guide.

[stefan-webrtc, 2015/06/27 11:01:06]

Acknowledged.

 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;
   }
 
   // required - Indicates the type of this event
   optional EventType type = 2;
 
   // optional - but required if type == RTP_EVENT
   optional ACMDumpRTPPacket packet = 3;
 
   // optional - but required if type == DEBUG_EVENT
   optional ACMDumpDebugEvent debug_event = 4;
+
+  // optional - but required if type == CONFIG_EVENT
+  optional ACMDumpConfigEvent config = 5;
 }
 
+
 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;
   }
 
   // required
   optional Direction direction = 1;
 
   // required
   optional PayloadType type = 2;
 
   // required - Contains the whole RTP packet (header+payload).
   optional bytes RTP_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 audiostreams could in principle share ssrc,

[hlundin-webrtc, 2015/06/24 11:07:55]

Nit: Please, write SSRC capitalized in comments.

[stefan-webrtc, 2015/06/24 11:46:16]

audio streams

[terelius, 2015/06/24 11:51:19]

Done.

+// 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.
+  optional uint32 remote_ssrc = 1;
+
+  // RTX settings for incoming video payloads that may be received. RTX is
+  // disabled if there's no config present.

[ivoc, 2015/06/24 11:27:12]

One of the experienced protobuf guys told me "Don’t make the presence/absence of
a field semantically meaningful for proto messages". It might be a good idea to
add a boolean to indicate if RTX is enabled or not for clarity.

[terelius, 2015/06/26 11:20:26]

The presence/absence of RTX settings in the C++ code is used to turn RTX on or
off. I think it makes sense to use the same principle in the protobuf, i.e. the
RTX fields in the *ReceiveStream::Config should be set if and only if an RTX
setting is stored in the protobuf.

+  optional RtcpConfig rtcp_config = 3;

[hlundin-webrtc, 2015/06/24 11:07:55]

I'm not a protobuf expert, but why is there no field = 2?

[terelius, 2015/06/24 11:51:18]

I (mostly) modeled the config fields on the VideoReceiveStream::Config struct,
which has a field local_ssrc (in addition to the remote_ssrc). As far as I can
determine, the local_ssrc is only used for sending RTCP so it made sense to
locate it together with the other RTCP settings. However I still left "space"
for it in the ACMDumpConfigEvent in case this assumption turns out to be
incorrect. Not sure whether this is a good or bad idea.

The numbers themselves are not important except that each field needs a unique
tag and that tags <16 use a more compact coding than larger tags.

[hlundin-webrtc, 2015/06/29 08:56:17]

Acknowledged.

+
+  // Map from video RTP payload type -> RTX config.
+  repeated RtxMap rtx_map = 4;

[ivoc, 2015/06/24 11:27:12]

Maybe we can use a map here instead of a seperate message? See
https://developers.google.com/protocol-buffers/docs/proto#maps .

[terelius, 2015/06/26 11:20:26]

I've changed to use the map feature instead of an explicit list of pairs.

+
+  // RTP header extensions used for the received stream.
+  repeated RtpHeaderExtension header_extensions = 5;

[ivoc, 2015/06/24 11:27:12]

This could be another candidate to use the Map feature

[terelius, 2015/06/26 11:20:26]

I've changed to use map instead of explicit list of pairs. This makes the
protobuf specification shorter, but I'm unsure of whether this will have any
advantage in the C++ interface since e.g. VideoReceiveStream::Config uses a
std::vector of structs/pairs.

+
+  // List of decoders associated with the stream;

[hlundin-webrtc, 2015/06/24 11:07:55]

End sentence with '.', not ';'

[terelius, 2015/06/24 11:51:19]

Done.

+  repeated DecoderConfig decoders = 6;

[ivoc, 2015/06/24 11:27:12]

Maybe another map?

[terelius, 2015/06/26 11:20:26]

Changed, but see previous comment on maps.

+}
+
+
+// Maps decoder names to payload types.
+message DecoderConfig {
+  optional string name = 1; // required

[ivoc, 2015/06/24 11:27:12]

I think it would be good to keep comments on a seperate line, like in the rest
of the file. Also a blank line between the different variables would be nice.

[stefan-webrtc, 2015/06/24 11:46:16]

And start with a capital letter and end with a period.

[terelius, 2015/06/26 11:20:26]

For now I made my comments consistent with previous comments in this file, i.e.
on a separate line starting with capital letter and ending with a period. The
exception is keywords like required and optional, which still use lower case.

[stefan-webrtc, 2015/06/27 11:01:06]

sgtm

+  optional sint32 payload_type = 2; // required
+}
+
+// Maps RTP header extension names to numerical ids.
+message RtpHeaderExtension {
+  optional string name = 1; // required
+  optional sint32 id = 2; // required
+}
+
+
+// RTX settings for incoming video payloads that may be received. RTX is
+// disabled if there's no config present.
+message RtxConfig {
+  // SSRCs to use for the RTX streams.

[hlundin-webrtc, 2015/06/24 11:07:55]

The comment says SSRCs (in plural). But the field is scalar, right?

[terelius, 2015/06/24 11:51:19]

You're right. I'll consider a complete rewrite of the documentation for
RtxConfig.

+  optional uint32 ssrc = 1; // required
+
+  // Payload type to use for the RTX stream.
+  optional sint32 payload_type = 2; // required
+}
+
+
+message RtxMap {
+  optional sint32 payload_type = 1; // required
+  optional RtxConfig config = 2; // required
+}
+
+
+// The interesting Rtcp packets are the ones that the sender receives

[hlundin-webrtc, 2015/06/24 11:07:55]

Nit: Rtcp -> RTCP, when written in comments.

[terelius, 2015/06/24 11:51:19]

Done.

+// Ergo, incoming Rtcp should be logged at the 'send'-side
+message RtcpConfig {
+  // Sender SSRC used for sending RTCP (such as receiver reports).
+  optional uint32 local_ssrc = 1;
+
+  // RTCP mode to use. Compound mode is described by RFC 4585 and reduced-size
+  // RTCP mode is described by RFC 5506.
+  enum RtcpMode {KRTCPCOMPOUND = 1; KRTCPREDUCEDSIZE = 2;}

[stefan-webrtc, 2015/06/24 11:46:16]

Not knowing the style guide for protobufs, is this the suggested way of writing
constants? Seems like a mix of kRtcpCompound and RTCP_COMPOUND.

[terelius, 2015/06/26 11:20:26]

You're right. I'll change to RTCP_COMPOUND, etc.

+  optional RtcpMode rtcp_mode = 2;
+
+  // Extended RTCP settings.
+  optional bool receiver_reference_time_report = 3;
+
+  // Receiver estimated maximum bandwidth.
+  optional bool remb = 4;
+}