Updated structures and functions for setting the max bitrate limit to take rtc::Optional<int>

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index e34056d905257371b6f9c0db015f38a3bdcf3a85..dd46dbb1f9fbfcfa8fa9281cb6070cdb57b35f75 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -78,9 +78,9 @@ webrtc::Call::Config::BitrateConfig GetBitrateConfigForCodec(
   }
   if (codec.GetParam(kCodecParamMaxBitrate, &bitrate_kbps) &&
       bitrate_kbps > 0) {
-    config.max_bitrate_bps = bitrate_kbps * 1000;
+    config.max_bitrate_bps = rtc::Optional<int>(bitrate_kbps * 1000);
   } else {
-    config.max_bitrate_bps = -1;
+    config.max_bitrate_bps = rtc::Optional<int>();
   }
   return config;
 }
@@ -362,11 +362,13 @@ std::vector<webrtc::VideoStream>
 WebRtcVideoChannel2::WebRtcVideoSendStream::CreateSimulcastVideoStreams(
     const VideoCodec& codec,
     const VideoOptions& options,
-    int max_bitrate_bps,
+    rtc::Optional<int> max_bitrate_bps,
     size_t num_streams) {
   int max_qp = kDefaultQpMax;
   codec.GetParam(kCodecParamMaxQuantization, &max_qp);
 
+  // TODO(skvlad): Consider replacing special case constants with
+  // rtc::Optional<int>
   return GetSimulcastConfig(
       num_streams, codec.width, codec.height, max_bitrate_bps, max_qp,
       codec.framerate != 0 ? codec.framerate : kDefaultVideoMaxFramerate);
@@ -376,11 +378,11 @@ std::vector<webrtc::VideoStream>
 WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoStreams(
     const VideoCodec& codec,
     const VideoOptions& options,
-    int max_bitrate_bps,
+    rtc::Optional<int> max_bitrate_bps,
     size_t num_streams) {
   int codec_max_bitrate_kbps;
   if (codec.GetParam(kCodecParamMaxBitrate, &codec_max_bitrate_kbps)) {
-    max_bitrate_bps = codec_max_bitrate_kbps * 1000;
+    max_bitrate_bps = rtc::Optional<int>(codec_max_bitrate_kbps * 1000);
   }
   if (num_streams != 1) {
     return CreateSimulcastVideoStreams(codec, options, max_bitrate_bps,
@@ -388,9 +390,9 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoStreams(
   }
 
   // For unset max bitrates set default bitrate for non-simulcast.
-  if (max_bitrate_bps <= 0) {
-    max_bitrate_bps =
-        GetMaxDefaultVideoBitrateKbps(codec.width, codec.height) * 1000;
+  if (!max_bitrate_bps) {
+    max_bitrate_bps = rtc::Optional<int>(
+        GetMaxDefaultVideoBitrateKbps(codec.width, codec.height) * 1000);
   }
 
   webrtc::VideoStream stream;
@@ -400,7 +402,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoStreams(
       codec.framerate != 0 ? codec.framerate : kDefaultVideoMaxFramerate;
 
   stream.min_bitrate_bps = kMinVideoBitrate * 1000;
-  stream.target_bitrate_bps = stream.max_bitrate_bps = max_bitrate_bps;
+  stream.target_bitrate_bps = stream.max_bitrate_bps = *max_bitrate_bps;
 
   int max_qp = kDefaultQpMax;
   codec.GetParam(kCodecParamMaxQuantization, &max_qp);
@@ -726,11 +728,9 @@ bool WebRtcVideoChannel2::GetChangedSendParameters(
   }
 
   // Handle max bitrate.
-  if (params.max_bandwidth_bps != bitrate_config_.max_bitrate_bps &&
-      params.max_bandwidth_bps >= 0) {
-    // 0 uncaps max bitrate (-1).
-    changed_params->max_bandwidth_bps = rtc::Optional<int>(
-        params.max_bandwidth_bps == 0 ? -1 : params.max_bandwidth_bps);

[Taylor Brandstetter, 2016/03/29 02:26:58]

Are we taking away the behavior where 0 uncaps max bitrate here?

[skvlad, 2016/03/30 19:40:44]

Yes, that is the intention. An unset Optional<int> uncaps the max bitrate, and a
positive value caps it. An Optional with a 0 or negative value is an error.

This may change the treatment of "b=AS:0" in SDP, though: previously it could
uncap the bandwidth for video, or leave it unchanged for audio. Now it will be
an error. Is it important to keep the previous interpretation? I could not find
anything about the meaning of zero bandwidth in any of the relevant RFCs.

+  if (params.max_bitrate_bps != bitrate_config_.max_bitrate_bps) {
+    changed_params->max_bandwidth_bps =
+        rtc::Optional<rtc::Optional<int>>(params.max_bitrate_bps);
   }
 
   // Handle conference mode.
@@ -783,12 +783,11 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
     // WebRtcVideoChannel2 (in which case we're good), or per sender (SSRC), in
     // which case this should not set a Call::BitrateConfig but rather
     // reconfigure all senders.
-    int max_bitrate_bps = *changed_params.max_bandwidth_bps;
     bitrate_config_.start_bitrate_bps = -1;
-    bitrate_config_.max_bitrate_bps = max_bitrate_bps;
-    if (max_bitrate_bps > 0 &&
-        bitrate_config_.min_bitrate_bps > max_bitrate_bps) {
-      bitrate_config_.min_bitrate_bps = max_bitrate_bps;
+    bitrate_config_.max_bitrate_bps = *changed_params.max_bandwidth_bps;
+    if (bitrate_config_.max_bitrate_bps &&
+        (bitrate_config_.min_bitrate_bps > *bitrate_config_.max_bitrate_bps)) {
+      bitrate_config_.min_bitrate_bps = *bitrate_config_.max_bitrate_bps;
     }
     bitrate_config_changed = true;
   }
@@ -1457,7 +1456,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::VideoSendStreamParameters::
     VideoSendStreamParameters(
         const webrtc::VideoSendStream::Config& config,
         const VideoOptions& options,
-        int max_bitrate_bps,
+        rtc::Optional<int> max_bitrate_bps,
         const rtc::Optional<VideoCodecSettings>& codec_settings)
     : config(config),
       options(options),
@@ -1486,7 +1485,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     const VideoOptions& options,
     WebRtcVideoEncoderFactory* external_encoder_factory,
     bool enable_cpu_overuse_detection,
-    int max_bitrate_bps,
+    rtc::Optional<int> max_bitrate_bps,
     const rtc::Optional<VideoCodecSettings>& codec_settings,
     const std::vector<webrtc::RtpExtension>& rtp_extensions,
     // TODO(deadbeef): Don't duplicate information between send_params,
@@ -1845,6 +1844,11 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::ValidateRtpParameters(
         << "Attempted to set RtpParameters without exactly one encoding";
     return false;
   }
+  if (rtp_parameters.encodings[0].max_bitrate_bps &&
+      (*rtp_parameters.encodings[0].max_bitrate_bps <= 0)) {
+    LOG(LS_ERROR) << "Attempted to set a negative bitrate limit";
+    return false;
+  }
   return true;
 }
 
@@ -1887,8 +1891,8 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoderConfig(
     stream_count = 1;
   }
 
-  int stream_max_bitrate =
-      MinPositive(rtp_parameters_.encodings[0].max_bitrate_bps,
+  rtc::Optional<int> stream_max_bitrate =
+      OptionalMin(rtp_parameters_.encodings[0].max_bitrate_bps,
                   parameters_.max_bitrate_bps);
   encoder_config.streams = CreateVideoStreams(
       clamped_codec, parameters_.options, stream_max_bitrate, stream_count);