Split out dscp option from VideoOptions to new struct MediaChannelOptions.

talk/media/base/mediachannel.h

Index: talk/media/base/mediachannel.h
diff --git a/talk/media/base/mediachannel.h b/talk/media/base/mediachannel.h
index 3b27b92dd96d613764f44386e4b9d3dcd07e6297..9a12fbd8867b12c71cafd9ddc1300c0ab33f5f4a 100644
--- a/talk/media/base/mediachannel.h
+++ b/talk/media/base/mediachannel.h
@@ -94,6 +94,13 @@ static std::string VectorToString(const std::vector<T>& vals) {
     return ost.str();
 }
 
+struct MediaChannelOptions {
+  MediaChannelOptions() {}

[pbos-webrtc, 2016/01/29 16:22:56]

Drop the ctor unless you want to move it to a .cc file.

[nisse-webrtc, 2016/02/01 08:29:49]

I think I'm showing C++ ignorance here. The thing is, I want a default
constructor setting the options to false. If I define a plain struct in C++, do
I get a default constructor initializing all members properly? Or will new
instances get uninitialized values?

+  // Set DSCP value for packet sent from media channel. This flag
+  // comes from the PeerConnection constraint 'googDscp'.
+  bool dscp;  // TODO(nisse): More descriptive name?

[pbos-webrtc, 2016/01/29 16:22:56]

enable_dscp? or enable_dscp_41 or whatever it was called? 'bool enable_dscp =
false;'

[nisse-webrtc, 2016/02/01 08:29:49]

enable_dcsp sounds good. And the actual codepoint used when enabled is different
for audio and video channels. 

That's why I had to add the DscpValue method. I don't quite like the way it's
used, I would prefer to have a member of type rtc::DiffServCodePoint, and set it
on construction depending on the value of the enable_dscp option and the
subclass being constructed. But I haven't found any nice and easy way to do that
(the "obvious" way, to let the constructor call the virtual method DscpValue,
doesn't work).

+};
+
 // Options that can be applied to a VoiceMediaChannel or a VoiceMediaEngine.
 // Used to be flags, but that makes it hard to selectively apply options.
 // We are moving all of the setting of options to structs like this,
@@ -124,7 +131,6 @@ struct AudioOptions {
     SetFrom(&tx_agc_limiter, change.tx_agc_limiter);
     SetFrom(&recording_sample_rate, change.recording_sample_rate);
     SetFrom(&playout_sample_rate, change.playout_sample_rate);
-    SetFrom(&dscp, change.dscp);
     SetFrom(&combined_audio_video_bwe, change.combined_audio_video_bwe);
   }
 
@@ -151,7 +157,6 @@ struct AudioOptions {
         tx_agc_limiter == o.tx_agc_limiter &&
         recording_sample_rate == o.recording_sample_rate &&
         playout_sample_rate == o.playout_sample_rate &&
-        dscp == o.dscp &&
         combined_audio_video_bwe == o.combined_audio_video_bwe;
   }
 
@@ -182,7 +187,6 @@ struct AudioOptions {
     ost << ToStringIfSet("tx_agc_limiter", tx_agc_limiter);
     ost << ToStringIfSet("recording_sample_rate", recording_sample_rate);
     ost << ToStringIfSet("playout_sample_rate", playout_sample_rate);
-    ost << ToStringIfSet("dscp", dscp);
     ost << ToStringIfSet("combined_audio_video_bwe", combined_audio_video_bwe);
     ost << "}";
     return ost.str();
@@ -219,8 +223,6 @@ struct AudioOptions {
   rtc::Optional<bool> tx_agc_limiter;
   rtc::Optional<uint32_t> recording_sample_rate;
   rtc::Optional<uint32_t> playout_sample_rate;
-  // Set DSCP value for packet sent from audio channel.
-  rtc::Optional<bool> dscp;
   // Enable combined audio+bandwidth BWE.
   rtc::Optional<bool> combined_audio_video_bwe;
 
@@ -242,7 +244,6 @@ struct VideoOptions {
     SetFrom(&video_noise_reduction, change.video_noise_reduction);
     SetFrom(&cpu_overuse_detection, change.cpu_overuse_detection);
     SetFrom(&conference_mode, change.conference_mode);
-    SetFrom(&dscp, change.dscp);
     SetFrom(&suspend_below_min_bitrate, change.suspend_below_min_bitrate);
     SetFrom(&screencast_min_bitrate_kbps, change.screencast_min_bitrate_kbps);
     SetFrom(&disable_prerenderer_smoothing,
@@ -253,7 +254,6 @@ struct VideoOptions {
     return video_noise_reduction == o.video_noise_reduction &&
            cpu_overuse_detection == o.cpu_overuse_detection &&
            conference_mode == o.conference_mode &&
-           dscp == o.dscp &&
            suspend_below_min_bitrate == o.suspend_below_min_bitrate &&
            screencast_min_bitrate_kbps == o.screencast_min_bitrate_kbps &&
            disable_prerenderer_smoothing == o.disable_prerenderer_smoothing;
@@ -265,7 +265,6 @@ struct VideoOptions {
     ost << ToStringIfSet("noise reduction", video_noise_reduction);
     ost << ToStringIfSet("cpu overuse detection", cpu_overuse_detection);
     ost << ToStringIfSet("conference mode", conference_mode);
-    ost << ToStringIfSet("dscp", dscp);
     ost << ToStringIfSet("suspend below min bitrate",
                          suspend_below_min_bitrate);
     ost << ToStringIfSet("screencast min bitrate kbps",
@@ -290,12 +289,6 @@ struct VideoOptions {
   // WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoderConfig.
   // The special screencast behaviour is disabled by default.
   rtc::Optional<bool> conference_mode;
-  // Set DSCP value for packet sent from video channel. This flag
-  // comes from the PeerConnection constraint 'googDscp' and,
-  // WebRtcVideoChannel2::SetOptions checks it before calling
-  // MediaChannel::SetDscp. If enabled, rtc::DSCP_AF41 is used. If
-  // disabled, which is the default, rtc::DSCP_DEFAULT is used.
-  rtc::Optional<bool> dscp;
   // Enable WebRTC suspension of video. No video frames will be sent
   // when the bitrate is below the configured minimum bitrate. This
   // flag comes from the PeerConnection constraint
@@ -384,15 +377,18 @@ class MediaChannel : public sigslot::has_slots<> {
     virtual ~NetworkInterface() {}
   };
 
-  MediaChannel() : network_interface_(NULL) {}
+  MediaChannel(const MediaChannelOptions& options)
+      : options_(options), network_interface_(NULL) {}
+  MediaChannel() : options_(), network_interface_(NULL) {}
   virtual ~MediaChannel() {}
 
   // Sets the abstract interface class for sending RTP/RTCP data.
   virtual void SetInterface(NetworkInterface *iface) {
     rtc::CritScope cs(&network_interface_crit_);
     network_interface_ = iface;
+    SetDscp(options_.dscp ? DscpValue() : rtc::DSCP_DEFAULT);
   }
-
+  virtual rtc::DiffServCodePoint DscpValue() const { return rtc::DSCP_DEFAULT; }
   // Called when a RTP packet is received.
   virtual void OnPacketReceived(rtc::Buffer* packet,
                                 const rtc::PacketTime& packet_time) = 0;
@@ -440,7 +436,7 @@ class MediaChannel : public sigslot::has_slots<> {
     return network_interface_->SetOption(type, opt, option);
   }
 
- protected:
+ private:
   // This method sets DSCP |value| on both RTP and RTCP channels.
   int SetDscp(rtc::DiffServCodePoint value) {
     int ret;
@@ -455,7 +451,6 @@ class MediaChannel : public sigslot::has_slots<> {
     return ret;
   }
 
- private:
   bool DoSendPacket(rtc::Buffer* packet,
                     bool rtcp,
                     const rtc::PacketOptions& options) {
@@ -467,6 +462,7 @@ class MediaChannel : public sigslot::has_slots<> {
                    : network_interface_->SendRtcp(packet, options);
   }
 
+  const MediaChannelOptions options_;
   // |network_interface_| can be accessed from the worker_thread and
   // from any MediaEngine threads. This critical section is to protect accessing
   // of network_interface_ object.
@@ -919,7 +915,10 @@ class VoiceMediaChannel : public MediaChannel {
     ERROR_PLAY_SRTP_REPLAY,               // Packet replay detected.
   };
 
+  VoiceMediaChannel(const MediaChannelOptions& options)
+      : MediaChannel(options) {}
   VoiceMediaChannel() {}
+
   virtual ~VoiceMediaChannel() {}
   virtual bool SetSendParameters(const AudioSendParameters& params) = 0;
   virtual bool SetRecvParameters(const AudioRecvParameters& params) = 0;
@@ -982,7 +981,9 @@ class VideoMediaChannel : public MediaChannel {
     ERROR_PLAY_SRTP_REPLAY,               // Packet replay detected.
   };
 
-  VideoMediaChannel() : renderer_(NULL) {}
+  VideoMediaChannel(const MediaChannelOptions& options)
+      : MediaChannel(options), renderer_(NULL) {}
+  VideoMediaChannel() : MediaChannel(), renderer_(NULL) {}
   virtual ~VideoMediaChannel() {}
 
   virtual bool SetSendParameters(const VideoSendParameters& params) = 0;
@@ -1115,6 +1116,8 @@ class DataMediaChannel : public MediaChannel {
     ERROR_RECV_SRTP_REPLAY,               // Packet replay detected.
   };
 
+  DataMediaChannel() {}

[pbos-webrtc, 2016/01/29 16:22:56]

Do you need/want this ctor?

[nisse-webrtc, 2016/02/01 08:29:49]

Probably not, it's a leftover from an earlier version where MediaChannel lacked
a default constructor. I'll delete it.

+
   virtual ~DataMediaChannel() {}
 
   virtual bool SetSendParameters(const DataSendParameters& params) = 0;