Generalize FlexfecReceiveStream::Config. (CL1)

webrtc/api/call/flexfec_receive_stream.h

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_API_CALL_FLEXFEC_RECEIVE_STREAM_H_
 #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 {
 
 // WORK IN PROGRESS!
 // This class is under development and it is not yet intended for use outside
 // of WebRTC.
 //
 // TODO(brandtr): Remove this comment when FlexFEC is ready for public use.
 
 class FlexfecReceiveStream {
  public:
   struct Stats {
     std::string ToString(int64_t time_ms) const;
 
     // TODO(brandtr): Add appropriate stats here.
     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.
   virtual void Start() = 0;
   // Stops stream activity.
   // When a stream is stopped, it can't receive nor process packets.
   virtual void Stop() = 0;
 
   virtual Stats GetStats() const = 0;
 
  protected:
   virtual ~FlexfecReceiveStream() = default;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_CALL_FLEXFEC_RECEIVE_STREAM_H_