Enable setting the maximum bitrate limit in RtpSender.

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 0e9866064df142cf7d1529065a818560a674dea3..33213af6d82a15824e679a887f59cc21d6df1dad 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -790,17 +790,20 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
     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;
   }
@@ -815,6 +818,11 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
   {
     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) {
@@ -833,6 +841,43 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
   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,
@@ -1493,7 +1538,8 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
       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;