Generalize FlexfecReceiveStream::Config. (CL1)

webrtc/api/call/flexfec_receive_stream.h

Index: webrtc/api/call/flexfec_receive_stream.h
diff --git a/webrtc/api/call/flexfec_receive_stream.h b/webrtc/api/call/flexfec_receive_stream.h
index 5918f7730fa9b081ecbe09ccf9392972177e19e4..f34fd28deee3874edd64b70117e4c4bca9ea4753 100644
--- a/webrtc/api/call/flexfec_receive_stream.h
+++ b/webrtc/api/call/flexfec_receive_stream.h
@@ -12,7 +12,9 @@
 #define WEBRTC_API_CALL_FLEXFEC_RECEIVE_STREAM_H_
 
 #include <string>
+#include <vector>
 
+#include "webrtc/api/call/transport.h"
 #include "webrtc/config.h"

[philipel, 2016/12/02 09:33:40]

Is config.h still used?

[brandtr, 2016/12/02 09:47:57]

Yep, for RtpExtension and RtcpMode.

 
 namespace webrtc {
@@ -32,10 +34,50 @@ class FlexfecReceiveStream {
     int flexfec_bitrate_bps;
   };
 
-  // TODO(brandtr): When we add multistream protection, and thus add a
-  // FlexfecSendStream class, remove FlexfecConfig from config.h and add
-  // the appropriate configs here and in FlexfecSendStream.
-  using Config = FlexfecConfig;
+  struct Config {
+    Config()

[philipel, 2016/12/02 09:33:40]

For small structs like this I think it's cleaner to set the default values of
the members as you declare them, for example:
int payload_type = -1;

Then you can remove the ctor.

[brandtr, 2016/12/02 09:47:57]

Done.

+        : payload_type(-1),
+          remote_ssrc(0),
+          protected_media_ssrcs(),
+          local_ssrc(0),
+          rtcp_mode(RtcpMode::kCompound),
+          rtcp_send_transport(nullptr),
+          transport_cc(false),
+          extensions() {}

[philipel, 2016/12/02 09:33:40]

newline after ctor

[brandtr, 2016/12/02 09:47:57]

Done.

+    ~Config() = default;

[philipel, 2016/12/02 09:33:40]

Remove default dtor declaration.

[brandtr, 2016/12/02 09:47:57]

I think this is a non-POD struct (due to the std::vector's), so isn't it nice to
show that the destructor is not trivial by including it like this?

[philipel, 2016/12/02 10:14:51]

I can't find that rule in the style guide, so I don't think anyone else would
know a declared dtor means this is a non-POD struct.

Also, I think this should be a class since this is a non-POD object, the style
guide is a bit vague so I'm not sure.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[brandtr, 2016/12/02 10:36:50]

I didn't get this from the style guide, but rather from danilchap who sometimes
told me to add destructors like this :)

This config is modeled after the corresponding VideoReceiveStream config:
https://cs.chromium.org/chromium/src/third_party/webrtc/video_receive_stream....,
which is a non-POD struct. So I think it's fine to do the same here. The purpose
of this config is in line with the gist of the style guide ("Use a struct only
for passive objects that carry data"), although the user of the config could do
advanced things with the vectors ("The accessing/setting of fields is done by
directly accessing the fields rather than through method invocations.")

+
+    std::string ToString() const;
+
+    // Payload type for FlexFEC.
+    int payload_type;
+
+    // SSRC for FlexFEC stream to be received.
+    uint32_t remote_ssrc;
+
+    // Vector containing a single element, corresponding to the SSRC of the
+    // media stream being protected by this FlexFEC stream. The vector MUST have
+    // size 1.
+    //
+    // TODO(brandtr): Update comment above when we support multistream
+    // protection.
+    std::vector<uint32_t> protected_media_ssrcs;
+
+    // SSRC for RTCP reports to be sent.
+    uint32_t local_ssrc;
+
+    // What RTCP mode to use in the reports.
+    RtcpMode rtcp_mode;
+
+    // Transport for outgoing RTCP packets.
+    Transport* rtcp_send_transport;
+
+    // |transport_cc| is true whenever the send-side BWE RTCP feedback message
+    // has been negotiated. This is a prerequisite for enabling send-side BWE.
+    bool transport_cc;
+
+    // RTP header extensions that have been negotiated for this track.
+    std::vector<RtpExtension> extensions;
+  };
 
   // Starts stream activity.
   // When a stream is active, it can receive and process packets.