Apply VideoOptions per stream.

talk/media/webrtc/webrtcvideoengine2.cc

Index: talk/media/webrtc/webrtcvideoengine2.cc
diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index f237b8fde66787c2653cac6fac1d3bf4384eea63..df2188f517b3d585ae6c7c74602f65c139fe5bc8 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -55,6 +55,10 @@
 namespace cricket {
 namespace {
 
+// Default values, used when options are unset.
+const bool kDefault_suspend_below_min_bitrate = false;

[pthatcher1, 2016/01/28 19:13:28]

Please make the style like kDefaultSuspendBelowMinBitrate on both of these.

+const int kDefault_screencast_min_bitrate = 0;
+
 // Wrap cricket::WebRtcVideoEncoderFactory as a webrtc::VideoEncoderFactory.
 class EncoderFactoryAdapter : public webrtc::VideoEncoderFactory {
  public:
@@ -619,26 +623,21 @@ WebRtcVideoChannel2::WebRtcVideoChannel2(
     WebRtcVideoDecoderFactory* external_decoder_factory)
     : call_(call),
       unsignalled_ssrc_handler_(&default_unsignalled_ssrc_handler_),
+      signal_cpu_adaptation_(true),
+      disable_prerenderer_smoothing_(false),
       external_encoder_factory_(external_encoder_factory),
       external_decoder_factory_(external_decoder_factory) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  SetDefaultOptions();
-  options_.SetAll(options);
-  if (options_.cpu_overuse_detection)
-    signal_cpu_adaptation_ = *options_.cpu_overuse_detection;
+
+  SetSharedOptions(options);
+  send_params_.options = options;
+
   rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc;
   sending_ = false;
   default_send_ssrc_ = 0;
   SetRecvCodecs(recv_codecs);
 }
 
-void WebRtcVideoChannel2::SetDefaultOptions() {
-  options_.cpu_overuse_detection = rtc::Optional<bool>(true);
-  options_.dscp = rtc::Optional<bool>(false);
-  options_.suspend_below_min_bitrate = rtc::Optional<bool>(false);
-  options_.screencast_min_bitrate = rtc::Optional<int>(0);
-}
-
 WebRtcVideoChannel2::~WebRtcVideoChannel2() {
   for (auto& kv : send_streams_)
     delete kv.second;
@@ -717,17 +716,28 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
   // instead of 4 times.
   if (!SetSendCodecs(params.codecs) ||
       !SetSendRtpHeaderExtensions(params.extensions) ||
-      !SetMaxSendBandwidth(params.max_bandwidth_bps) ||
-      !SetOptions(params.options)) {
+      !SetMaxSendBandwidth(params.max_bandwidth_bps)) {
     return false;
   }
-  if (send_params_.rtcp.reduced_size != params.rtcp.reduced_size) {
+
+  VideoOptions options = send_params_.options;
+  send_params_ = params;
+
+  // Take care to keep old values of options, if the new params
+  // doesn't specify any value.
+  options.SetAll(params.options);
+  send_params_.options = options;
+
+  SetSharedOptions(send_params_.options);
+  // Call each stream's SetSendParameters method. Leaves to the callee
+  // to check if there's any change.
+  {
     rtc::CritScope stream_lock(&stream_crit_);
     for (auto& kv : send_streams_) {
       kv.second->SetSendParameters(params);
     }
   }
-  send_params_ = params;
+
   return true;
 }
 
@@ -923,7 +933,7 @@ bool WebRtcVideoChannel2::SetVideoSend(uint32_t ssrc, bool enable,
     return false;
   }
   if (enable && options) {
-    return SetOptions(*options);
+    return SetOptions(ssrc, *options);
   } else {
     return true;
   }
@@ -969,7 +979,7 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) {
   config.overuse_callback = this;
 
   WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(
-      call_, sp, config, external_encoder_factory_, options_,
+      call_, sp, config, external_encoder_factory_,
       bitrate_config_.max_bitrate_bps, send_codec_, send_rtp_extensions_,
       send_params_);
 
@@ -1099,7 +1109,7 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp,
 
   receive_streams_[ssrc] = new WebRtcVideoReceiveStream(
       call_, sp, config, external_decoder_factory_, default_stream,
-      recv_codecs_, options_.disable_prerenderer_smoothing.value_or(false));
+      recv_codecs_, disable_prerenderer_smoothing_);
 
   return true;
 }
@@ -1450,29 +1460,34 @@ bool WebRtcVideoChannel2::SetMaxSendBandwidth(int max_bitrate_bps) {
   return true;
 }
 
-bool WebRtcVideoChannel2::SetOptions(const VideoOptions& options) {
-  TRACE_EVENT0("webrtc", "WebRtcVideoChannel2::SetOptions");
-  LOG(LS_INFO) << "SetOptions: " << options.ToString();
-  VideoOptions old_options = options_;
-  options_.SetAll(options);
-  if (options_ == old_options) {
-    // No new options to set.
-    return true;
-  }
+void WebRtcVideoChannel2::SetSharedOptions(const VideoOptions& options) {
   {
     rtc::CritScope lock(&capturer_crit_);
-    if (options_.cpu_overuse_detection)
-      signal_cpu_adaptation_ = *options_.cpu_overuse_detection;
+    if (options.cpu_overuse_detection)
+      signal_cpu_adaptation_ = *options.cpu_overuse_detection;
   }
-  rtc::DiffServCodePoint dscp =
-      options_.dscp.value_or(false) ? rtc::DSCP_AF41 : rtc::DSCP_DEFAULT;
-  MediaChannel::SetDscp(dscp);
+  if (options.disable_prerenderer_smoothing)
+    disable_prerenderer_smoothing_ = *options.disable_prerenderer_smoothing;
+
+  if (options.dscp) {
+    rtc::DiffServCodePoint dscp =
+        *options.dscp ? rtc::DSCP_AF41 : rtc::DSCP_DEFAULT;
+    MediaChannel::SetDscp(dscp);
+  }
+}
+
+bool WebRtcVideoChannel2::SetOptions(uint32_t ssrc,
+                                     const VideoOptions& options) {
+  TRACE_EVENT0("webrtc", "WebRtcVideoChannel2::SetOptions");
+  LOG(LS_INFO) << "SetOptions: ssrc " << ssrc << ": " << options.ToString();
+  SetSharedOptions(options);

[pthatcher1, 2016/01/28 19:13:28]

Is this really necessary?  I don't think it is.

+
   rtc::CritScope stream_lock(&stream_crit_);
-  for (std::map<uint32_t, WebRtcVideoSendStream*>::iterator it =
-           send_streams_.begin();
-       it != send_streams_.end(); ++it) {
-    it->second->SetOptions(options_);
+  if (send_streams_.find(ssrc) == send_streams_.end()) {
+    return false;
   }
+  send_streams_[ssrc]->SetOptions(options);

[pthatcher1, 2016/01/28 19:13:28]

You can avoid a double look up by doing this:

auto kv = send_streams_.find(ssrc);
if (kv == send_streams_.end()) {
  return false;
}
kv.second->SetOptions(options);
return true;

[nisse-webrtc, 2016/02/15 08:14:20]

Done. Changed all similar patterns in the file.

+
   return true;
 }
 
@@ -1583,7 +1598,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     const StreamParams& sp,
     const webrtc::VideoSendStream::Config& config,
     WebRtcVideoEncoderFactory* external_encoder_factory,
-    const VideoOptions& options,
     int max_bitrate_bps,
     const rtc::Optional<VideoCodecSettings>& codec_settings,
     const std::vector<webrtc::RtpExtension>& rtp_extensions,
@@ -1595,7 +1609,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
       call_(call),
       external_encoder_factory_(external_encoder_factory),
       stream_(NULL),
-      parameters_(config, options, max_bitrate_bps, codec_settings),
+      parameters_(config, send_params.options, max_bitrate_bps, codec_settings),
       allocated_encoder_(NULL, webrtc::kVideoCodecUnknown, false),
       capturer_(NULL),
       sending_(false),
@@ -1792,6 +1806,7 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetApplyRotation(
 
 void WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions(
     const VideoOptions& options) {
+
   rtc::CritScope cs(&lock_);
   if (parameters_.codec_settings) {
     LOG(LS_INFO) << "SetCodecAndOptions because of SetOptions; options="
@@ -1901,9 +1916,9 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions(
   parameters_.config.rtp.nack.rtp_history_ms =
       HasNack(codec_settings.codec) ? kNackHistoryMs : 0;
 
-  RTC_CHECK(options.suspend_below_min_bitrate);
   parameters_.config.suspend_below_min_bitrate =
-      *options.suspend_below_min_bitrate;
+      options.suspend_below_min_bitrate.value_or(
+          kDefault_suspend_below_min_bitrate);
 
   parameters_.codec_settings =
       rtc::Optional<WebRtcVideoChannel2::VideoCodecSettings>(codec_settings);
@@ -1932,10 +1947,26 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetRtpExtensions(
 void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSendParameters(
     const VideoSendParameters& send_params) {
   rtc::CritScope cs(&lock_);
-  parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size
-                                         ? webrtc::RtcpMode::kReducedSize
-                                         : webrtc::RtcpMode::kCompound;
-  if (stream_ != nullptr) {
+  webrtc::RtcpMode mode = send_params.rtcp.reduced_size
+                          ? webrtc::RtcpMode::kReducedSize
+                          : webrtc::RtcpMode::kCompound;

[pthatcher1, 2016/01/28 19:13:28]

I'd call the variable "rtcp_mode".

+
+  bool need_recreate = (mode != parameters_.config.rtp.rtcp_mode);
+
+  parameters_.config.rtp.rtcp_mode = mode;

[pthatcher1, 2016/01/28 19:13:28]

This might be more clear as:

bool need_recreate = false;
webrtc::RtcpMode rtcp_mode = ...;
if (parameters_.config.rtp.rtcp_mode != rtcp_mode) {
  parameters_.config.rtp.rtcp_mode = rtcp_mode;
  need_recreate = true;
}

[pbos-webrtc, 2016/01/28 22:47:36]

I think this code looks different at HEAD, I touched SetSendParameters.

+
+  VideoOptions old_options = parameters_.options;
+  parameters_.options.SetAll(send_params.options);
+
+  if (!(parameters_.options == old_options) &&
+      parameters_.codec_settings) {
+    LOG(LS_INFO) << "SetCodecAndOptions because of SetSendParameters; options="
+                 << send_params.options.ToString();
+    SetCodecAndOptions(*parameters_.codec_settings, send_params.options);
+    // RecreateWebRtcStream already called
+    need_recreate = false;
+  }
+  if (need_recreate && stream_ != nullptr) {
     LOG(LS_INFO) << "RecreateWebRtcStream (send) because of SetSendParameters";
     RecreateWebRtcStream();
   }
@@ -1947,9 +1978,9 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoderConfig(
     const VideoCodec& codec) const {
   webrtc::VideoEncoderConfig encoder_config;
   if (dimensions.is_screencast) {
-    RTC_CHECK(parameters_.options.screencast_min_bitrate);
     encoder_config.min_transmit_bitrate_bps =
-        *parameters_.options.screencast_min_bitrate * 1000;
+        1000 * parameters_.options.screencast_min_bitrate.value_or(
+            kDefault_screencast_min_bitrate);
     encoder_config.content_type =
         webrtc::VideoEncoderConfig::ContentType::kScreen;
   } else {