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..fea6f2dec9c4a160da5e4eb55ebe2dd6363fdac0 100644
--- a/talk/media/base/mediachannel.h
+++ b/talk/media/base/mediachannel.h
@@ -94,6 +94,12 @@ static std::string VectorToString(const std::vector<T>& vals) {
     return ost.str();
 }
 
+struct MediaChannelOptions {
+  // Set DSCP value for packet sent from media channel. This flag
+  // comes from the PeerConnection constraint 'googDscp'.
+  bool enable_dscp = false;

[pbos-webrtc, 2016/02/01 10:57:54]

To me it sounds like DSCP_DEFAULT is also some form of DSCP? If so this flag
name doesn't make sense (but it didn't before either). pthatcher@ can you help
name this?

If DSCP_DEFAULT means that DSCP is off, please comment that in the DscpValue()
method below, or rename to DSCP_OFF if possible.

[nisse-webrtc, 2016/02/01 11:26:34]

We're talking about is the diffserv bits in the IPv4 header, right? If so, it's
not optional, they're present in every packet, with some default if you don't
tell the IP stack otherwise.

In this piece of the code, my intention is to just mirror the constraint on the
wire. I hope we don't need three states, i.e., that a missing "googDscp"
constraint should be in all respects equivalent to a "googDscp" constraint with
the value false.  

+};
+
 // 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 +130,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 +156,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 +186,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 +222,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 +243,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 +253,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 +264,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 +288,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 +376,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_.enable_dscp ? DscpValue() : rtc::DSCP_DEFAULT);
   }
-
+  virtual rtc::DiffServCodePoint DscpValue() const { return rtc::DSCP_DEFAULT; }

[pbos-webrtc, 2016/02/01 10:57:54]

This method name confuses me, should this be DscpValueForMediaTypeWhenEnabled()
or something? pthatcher@ how do we make this more readable?

[nisse-webrtc, 2016/02/01 11:26:34]

The intention is to return the diffserve codepoint appropriate for this media
type. It's very static. The videoengine and voiceengine report different values,


For video AF41, "assured forwarding", see
https://en.wikipedia.org/wiki/Differentiated_services#Assured_Forwarding_.28A...

For audio, it's EF, high priority expedited forwarding.

For data, I just return DSCP_DEFAULT, which means that enable_dscp has no
effect.

Personally, I think diffserv is pretty useless.

[pbos-webrtc, 2016/02/01 11:27:21]

Name this MediaTypeDscpValue() then perhaps?

[nisse-webrtc, 2016/02/01 12:09:45]

Done.

   // Called when a RTP packet is received.
   virtual void OnPacketReceived(rtc::Buffer* packet,
                                 const rtc::PacketTime& packet_time) = 0;
@@ -440,7 +435,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 +450,6 @@ class MediaChannel : public sigslot::has_slots<> {
     return ret;
   }
 
- private:
   bool DoSendPacket(rtc::Buffer* packet,
                     bool rtcp,
                     const rtc::PacketOptions& options) {
@@ -467,6 +461,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 +914,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 +980,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;