Enable setting the maximum bitrate limit in RtpSender.

webrtc/media/engine/webrtcvideoengine2.cc

 /*
  *  Copyright (c) 2014 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.
  */
 
@@ skipping 772 matching lines @@
     // TODO(holmer): Changing the codec parameters shouldn't necessarily mean
     // that we change the min/max of bandwidth estimation. Reevaluate this.
     bitrate_config_ = GetBitrateConfigForCodec(codec_settings.codec);
     bitrate_config_changed = true;
   }
 
   if (changed_params.rtp_header_extensions) {
     send_rtp_extensions_ = *changed_params.rtp_header_extensions;
   }
 
+  int max_global_bitrate = -1;

[Taylor Brandstetter, 2016/03/12 01:57:06]

This variable can remain inside the scoped block.

You can just pass "*changed_params.max_bandwidth_bps" into MinPositive.

[skvlad, 2016/03/15 21:18:18]

Good catch, thanks!

   if (changed_params.max_bandwidth_bps) {
     // TODO(pbos): Figure out whether b=AS means max bitrate for this
     // 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;
+    max_global_bitrate = *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 = max_global_bitrate;
+    if (max_global_bitrate > 0 &&
+        bitrate_config_.min_bitrate_bps > max_global_bitrate) {
+      bitrate_config_.min_bitrate_bps = max_global_bitrate;
+      // TODO(skvlad): limit min_bitrate_bps to not exceed the lowest of all

[pthatcher1, 2016/03/12 01:21:04]

limit => Limit

[skvlad, 2016/03/15 21:18:18]

Done.

+      // per-SSRC bitrate limits

[pthatcher1, 2016/03/12 01:21:04]

Actually, pbos said that once we move to UnifiedPlan, we'll just get rid of the
SetBitrateConfig here, so maybe just make the TODO reflect that.

[skvlad, 2016/03/15 21:18:18]

Done.

     }
     bitrate_config_changed = true;
   }
 
   if (bitrate_config_changed) {
     call_->SetBitrateConfig(bitrate_config_);
   }
 
   if (changed_params.options)
     send_params_.options.SetAll(*changed_params.options);
 
   {
     rtc::CritScope stream_lock(&stream_crit_);
     for (auto& kv : send_streams_) {
+      if (changed_params.max_bandwidth_bps) {
+        changed_params.max_bandwidth_bps = rtc::Optional<int>(MinPositive(
+            max_global_bitrate,

[pthatcher1, 2016/03/12 01:21:04]

You could make max_global_bitrate rtc::Optional<int> also.

[skvlad, 2016/03/15 21:18:18]

rtc::Optional changes will come in a separate code review.

+            kv.second->rtp_parameters().encodings[0].max_bitrate_bps));

[Taylor Brandstetter, 2016/03/12 01:57:06]

A problem with this is that even if the minimum of the two maximums did not
change, "max_bandwidth_bps" will be set in the changed_params, and the encoder
will get needlessly reconfigured on the next frame. Same with SetRtpParameters.

I think fixing this (the right way) requires some refactoring. Basically,
passing a plain "VideoSendParameters" into the VideoSendStream, then merging
that with the stream's current RtpParameters, and THEN computing the
ChangedSendParameters from that. But we can probably do that in a separate CL.
For now I think it would be fine to just add a TODO comment so we don't forget
about this.

[pthatcher1, 2016/03/14 16:26:50]

Good point to avoid reconfiguration.

[skvlad, 2016/03/15 21:18:18]

Added a TODO. I'll address this as part of the rtc::Optional<> change.

+      }

[pthatcher1, 2016/03/12 01:21:03]

Actually, I think this isn't necessary.  If the per-stream settings are lower
than the global settings, the per-stream settings will still take effect.  We
may need the other per-stream min, but not this one.

[skvlad, 2016/03/15 21:18:18]

Looks like you are right: the global minimum bitrate is the lower bound on what
the bandwidth estimator will report. If the encoder is configured with a lower
maximum bandwidth, it will be respecting its lower limit.

       kv.second->SetSendParameters(changed_params);
     }
     if (changed_params.codec) {
       // Update receive feedback parameters from new codec.
       LOG(LS_INFO)
           << "SetFeedbackOptions on all the receive streams because the send "
              "codec has changed.";
       for (auto& kv : receive_streams_) {
         RTC_DCHECK(kv.second != nullptr);
         kv.second->SetFeedbackParameters(HasNack(send_codec_->codec),
                                          HasRemb(send_codec_->codec),
                                          HasTransportCc(send_codec_->codec));
       }
     }
   }
   send_params_ = params;
   return true;
 }
+webrtc::RTCRtpParameters WebRtcVideoChannel2::GetRtpParameters(uint32_t ssrc) {
+  rtc::CritScope stream_lock(&stream_crit_);
+  auto it = send_streams_.find(ssrc);
+  if (it == send_streams_.end()) {
+    LOG(LS_WARNING) << "Attempting to get RTP parameters for stream with ssrc "
+                    << ssrc << " which doesn't exist.";
+    return webrtc::RTCRtpParameters();
+  }
+
+  return it->second->rtp_parameters();
+}
+
+bool WebRtcVideoChannel2::SetRtpParameters(
+    uint32_t ssrc,
+    const webrtc::RTCRtpParameters& parameters) {
+  rtc::CritScope stream_lock(&stream_crit_);
+  auto it = send_streams_.find(ssrc);
+  if (it == send_streams_.end()) {
+    LOG(LS_WARNING) << "Attempting to set RTP parameters for stream with ssrc "
+                    << ssrc << " which doesn't exist.";
+    return false;
+  }
+
+  if (parameters.encodings.size() != 1) {
+    LOG(LS_WARNING)
+        << "Attempting to set RTP parameters without exactly 1 encoding";
+    return false;
+  }
+
+  it->second->set_rtp_parameters(parameters);

[pthatcher1, 2016/03/12 01:21:03]

I think we should have a SetRtpParameters on the send stream that does the code
below and sets rtp_parameters_ rather than doing that here.

[Taylor Brandstetter, 2016/03/12 01:57:06]

I would say the same thing, but the send stream doesn't have access to the
current send_params_. We can resolve this easily if we do the refactoring I
describe above though.

[pthatcher1, 2016/03/14 16:26:50]

I think adding a new state variable to the SendStream makes sense.  Do you
really think we can do it in a followup CL rather than this one?

+  ChangedSendParameters changed_params;
+  changed_params.max_bandwidth_bps =
+      rtc::Optional<int>(MinPositive(bitrate_config_.max_bitrate_bps,

[pthatcher1, 2016/03/12 01:21:03]

I don't think we need the min with bitrate_config_.max_bitrate_bps.  I think we
just need the min of the last RtpParameters and th elast ChangedSendParameters
passed into a stream.

[Taylor Brandstetter, 2016/03/12 01:57:06]

It shouldn't be the last ChangedSendParameters, but the last
VideoSendParameters. The last ChangedSendParameters could come from this very
method.

+                                     parameters.encodings[0].max_bitrate_bps));
+  it->second->SetSendParameters(changed_params);
+  return true;
+}
 
 bool WebRtcVideoChannel2::GetChangedRecvParameters(
     const VideoRecvParameters& params,
     ChangedRecvParameters* changed_params) const {
   if (!ValidateCodecFormats(params.codecs) ||
       !ValidateRtpExtensions(params.extensions)) {
     return false;
   }
 
   // Handle receive codecs.
@@ skipping 640 matching lines @@
       capturer_(nullptr),
       external_encoder_factory_(external_encoder_factory),
       stream_(nullptr),
       parameters_(config, send_params.options, max_bitrate_bps, codec_settings),
       pending_encoder_reconfiguration_(false),
       allocated_encoder_(nullptr, webrtc::kVideoCodecUnknown, false),
       capturer_is_screencast_(false),
       sending_(false),
       muted_(false),
       first_frame_timestamp_ms_(0),
-      last_frame_timestamp_ms_(0) {
+      last_frame_timestamp_ms_(0),
+      rtp_parameters_(webrtc::RTCRtpParameters::CreateDefault()) {
   parameters_.config.rtp.max_packet_size = kVideoMtu;
   parameters_.conference_mode = send_params.conference_mode;
 
   sp.GetPrimarySsrcs(&parameters_.config.rtp.ssrcs);
   sp.GetFidSsrcs(parameters_.config.rtp.ssrcs,
                  &parameters_.config.rtp.rtx.ssrcs);
   parameters_.config.rtp.c_name = sp.cname;
   parameters_.config.rtp.extensions = rtp_extensions;
   parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size
                                          ? webrtc::RtcpMode::kReducedSize
@@ skipping 1027 matching lines @@
         rtx_mapping[video_codecs[i].codec.id] !=
             fec_settings.red_payload_type) {
       video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id];
     }
   }
 
   return video_codecs;
 }
 
 }  // namespace cricket