Enable setting the maximum bitrate limit in RtpSender.

Enable setting the maximum bitrate limit in RtpSender.

This change allows the application to limit the bitrate of the outgoing
audio and video streams at runtime. The API roughly follows the WebRTC
API draft, defining the RTCRtpParameters structure witn exactly one
encoding (simulcast streams are not exposed in the API for now).
(https://www.w3.org/TR/webrtc/#idl-def-RTCRtpParameters)

BUG=

Committed: https://crrev.com/dc1c62cd30e42d5bc31db347126476ca8e1b6c58
Cr-Commit-Position: refs/heads/master@{#12025}

[skvlad, 2016-03-12 00:21:03]

skvlad@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, magjed@webrtc.org, mflodman@webrtc.org, pbos@webrtc.org,
pthatcher@webrtc.org

[skvlad, 2016-03-12 00:21:04]

This change allows any WebRTC client application to control the maximum send
bitrate for audio and video at runtime at the RtpSender level. Please take a
look.

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

I still need to check the unit tests.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:19: namespace webrtc {
FYI, you'll need to make sure you don't break the Chrome build, which has a
different build file.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:21: struct RTCRtpEncodingParameters {
Can we please remove the RTC prefix and just call it RtpParameters and
RtpEncodingParameters?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:23: RTCRtpEncodingParameters() : max_bitrate_bps(-1)
{}
You can just do this and not have to define the constructor:

int max_bitrate_bps = -1;

Actually, I think it would make sense to have rtc::Optional here:

rtc::Optional<int> max_bitrate_bps;

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:26: struct RTCRtpParameters {
Can you give a comment pointing the standards doc where these are defined?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:29: static RTCRtpParameters CreateDefault() {
Do we really need this in the API?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h
File webrtc/api/rtpsender.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h#newcod...
webrtc/api/rtpsender.h:101: bool SetParameters(const RTCRtpParameters&
setParameters);
Can you please make the parameter name just "parameters"?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h#newcod...
webrtc/api/rtpsender.h:165: bool SetParameters(const RTCRtpParameters&
setParameters);
Same here

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1269: if (!voice_channel_)
{}s please

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1337: ASSERT(signaling_thread()->IsCurrent());
and here

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
webrtc/media/base/fakemediaengine.h:230:
rtp_parameters_(webrtc::RTCRtpParameters::CreateDefault()) {
It seems like building a one-encoding parameters should be defined here, not in
RtpParameters.  Actually, why do we need one encoding the parameters here?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:84: static T MinPositive(T a, T b) {
This would be more clear as the min of rtc::Optionals.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:805: // TODO(skvlad): limit
min_bitrate_bps to not exceed the lowest of all
limit => Limit

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:806: // per-SSRC bitrate limits
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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:823: max_global_bitrate,
You could make max_global_bitrate rtc::Optional<int> also.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:825: }
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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:873:
it->second->set_rtp_parameters(parameters);
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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:876:
rtc::Optional<int>(MinPositive(bitrate_config_.max_bitrate_bps,
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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1442: webrtc::RTCRtpParameters
WebRtcVoiceMediaChannel::GetRtpParameters(
Can you split this into a CL for video and a second CL for audio?  I think we'll
be able to land the video one faster that way, and it's more important.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1472: }
It feels like we should move the ApplyBitrateLimits to the send stream, perhaps
calls it SetBitrate, and pass in the already calculated min.  Actually, we
should probably mirror the video send stream and have SetParameters that does
the min calculation internal to the stream.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:2423: int local_limit) {
Why not just pass in one value and do the min() before?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1402
webrtc/pc/channel.cc:1402: return true;
Just make it a void if it can't fail.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1600
webrtc/pc/channel.cc:1600: return
media_channel()->SetSendParameters(parameters);
Why was this moved to a separate method?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1737
webrtc/pc/channel.cc:1737: return true;
same here

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1918
webrtc/pc/channel.cc:1918: }
same here

[Taylor Brandstetter, 2016-03-12 01:57:07]

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovider.h
File webrtc/api/mediastreamprovider.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovide...
webrtc/api/mediastreamprovider.h:66: virtual RTCRtpParameters
GetAudioRtpParameters(uint32_t ssrc) = 0;
Can probably make this const.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovide...
webrtc/api/mediastreamprovider.h:90: virtual RTCRtpParameters
GetVideoRtpParameters(uint32_t ssrc) = 0;
This too.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:26: struct RTCRtpParameters {
On 2016/03/12 01:21:03, pthatcher1 wrote:
> Can you give a comment pointing the standards doc where these are defined?

It's at the top of the file.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsenderinterface.h
File webrtc/api/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsenderinterface...
webrtc/api/rtpsenderinterface.h:55: virtual RTCRtpParameters GetParameters() =
0;
Can also make this const.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession_unit...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:3350: // Verify that the media channel
received the new parameters
nit: Add periods at the end of comments like this.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
webrtc/media/base/fakemediaengine.h:230:
rtp_parameters_(webrtc::RTCRtpParameters::CreateDefault()) {
On 2016/03/12 01:21:03, pthatcher1 wrote:
> It seems like building a one-encoding parameters should be defined here, not
in
> RtpParameters.  Actually, why do we need one encoding the parameters here?

To answer the question: If you call GetParameters there should be something
returned. And I think it would be much better defined in mediaengine.h, not
fakemediaengine.h.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
webrtc/media/base/fakemediaengine.h:260: rtp_parameters_ = parameters;
I know this is just a stub class, but for consistency we should probably make
rtp_parameters_ a map (on ssrc), in case the need arises later to call
Get/SetRtpParameters on multiple SSRCs. Just need to add/remove entries inside
Add/RemoveSendStream. Also, can move it to RtpHelper.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:85: if (a <= 0)
nit: {}s

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:905: virtual webrtc::RTCRtpParameters
GetRtpParameters(uint32_t ssrc) = 0;
Again, can probably be const.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:793: int max_global_bitrate = -1;
This variable can remain inside the scoped block.

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

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:824:
kv.second->rtp_parameters().encodings[0].max_bitrate_bps));
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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:873:
it->second->set_rtp_parameters(parameters);
On 2016/03/12 01:21:03, pthatcher1 wrote:
> 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.

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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:876:
rtc::Optional<int>(MinPositive(bitrate_config_.max_bitrate_bps,
On 2016/03/12 01:21:03, pthatcher1 wrote:
> 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.

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

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1072: // Read back the
parameteres and verify they have the correct value
nit: Periods at ends of comments

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3060:
TEST_F(WebRtcVideoChannel2Test,
I'd add a "TODO: Change this when we support setting parameters for multiple
encodings". May be obvious to us, but possibly not to someone else in the
future.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:843:
CannotSetRtpParametersWithIncorrectNumberOfEncodings) {
If you add a TODO comment for the other "IncorrectNumberOfEncodings" test, add
it for this one too.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1811: }
Very thorough unit tests, I'm impressed. :)

[pthatcher1, 2016-03-14 16:26:51]

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:26: struct RTCRtpParameters {
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> On 2016/03/12 01:21:03, pthatcher1 wrote:
> > Can you give a comment pointing the standards doc where these are defined?
> 
> It's at the top of the file.

Ah, could you (Vlad) move it closer to the classes?  It's a bit hard to see up
there with the license comment.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:824:
kv.second->rtp_parameters().encodings[0].max_bitrate_bps));
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> 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.

Good point to avoid reconfiguration.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:873:
it->second->set_rtp_parameters(parameters);
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> On 2016/03/12 01:21:03, pthatcher1 wrote:
> > 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.
> 
> 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.

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?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1060: void
VerifyEncoderBitrate(cricket::FakeVideoCapturer& capturer,
Should this be called SetAndExpectMaxBandwidth?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3039: //                     
             limit...
FYI, I think we should make it:
- An Optiona<int> instead of using -1.
- Unset = remove limitation (like audio)
- 0 = don't send (like active = false)
- No way to not change, except to pass the same value as last time.

[skvlad, 2016-03-15 21:18:19]

Removed changes related to AudioSendStream from this CL - I'll send them in a
separate CL to reduce the amount of changes that need to be reviewed at once.

Took care of code review comments, refactored VideoSendStream to do bitrate
limit computation internally.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovider.h
File webrtc/api/mediastreamprovider.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovide...
webrtc/api/mediastreamprovider.h:66: virtual RTCRtpParameters
GetAudioRtpParameters(uint32_t ssrc) = 0;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> Can probably make this const.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/mediastreamprovide...
webrtc/api/mediastreamprovider.h:90: virtual RTCRtpParameters
GetVideoRtpParameters(uint32_t ssrc) = 0;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> This too.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:21: struct RTCRtpEncodingParameters {
On 2016/03/12 01:21:03, pthatcher1 wrote:
> Can we please remove the RTC prefix and just call it RtpParameters and
> RtpEncodingParameters?

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:23: RTCRtpEncodingParameters() : max_bitrate_bps(-1)
{}
On 2016/03/12 01:21:03, pthatcher1 wrote:
> You can just do this and not have to define the constructor:
> 
> int max_bitrate_bps = -1;
Done.

> Actually, I think it would make sense to have rtc::Optional here:
> 
> rtc::Optional<int> max_bitrate_bps;
I'll make this change in a separate CL, as it is largely independent of this one
and affects a number of unrelated files.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:26: struct RTCRtpParameters {
On 2016/03/14 16:26:50, pthatcher1 wrote:
> On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> > On 2016/03/12 01:21:03, pthatcher1 wrote:
> > > Can you give a comment pointing the standards doc where these are defined?
> > 
> > It's at the top of the file.
> 
> Ah, could you (Vlad) move it closer to the classes?  It's a bit hard to see up
> there with the license comment.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:29: static RTCRtpParameters CreateDefault() {
On 2016/03/12 01:21:03, pthatcher1 wrote:
> Do we really need this in the API?  
Removed.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h
File webrtc/api/rtpsender.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h#newcod...
webrtc/api/rtpsender.h:101: bool SetParameters(const RTCRtpParameters&
setParameters);
On 2016/03/12 01:21:03, pthatcher1 wrote:
> Can you please make the parameter name just "parameters"?

Done. This was a silly tab-completion mistake.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsender.h#newcod...
webrtc/api/rtpsender.h:165: bool SetParameters(const RTCRtpParameters&
setParameters);
On 2016/03/12 01:21:03, pthatcher1 wrote:
> Same here

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsenderinterface.h
File webrtc/api/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/rtpsenderinterface...
webrtc/api/rtpsenderinterface.h:55: virtual RTCRtpParameters GetParameters() =
0;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> Can also make this const.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1269: if (!voice_channel_)
On 2016/03/12 01:21:03, pthatcher1 wrote:
> {}s please

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1337: ASSERT(signaling_thread()->IsCurrent());
On 2016/03/12 01:21:03, pthatcher1 wrote:
> and here

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession_unit...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/api/webrtcsession_unit...
webrtc/api/webrtcsession_unittest.cc:3350: // Verify that the media channel
received the new parameters
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> nit: Add periods at the end of comments like this.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/fakemediaen...
webrtc/media/base/fakemediaengine.h:260: rtp_parameters_ = parameters;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> I know this is just a stub class, but for consistency we should probably make
> rtp_parameters_ a map (on ssrc), in case the need arises later to call
> Get/SetRtpParameters on multiple SSRCs. Just need to add/remove entries inside
> Add/RemoveSendStream. Also, can move it to RtpHelper.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:84: static T MinPositive(T a, T b) {
On 2016/03/12 01:21:03, pthatcher1 wrote:
> This would be more clear as the min of rtc::Optionals. 

I'm going to send a separate code review for the change to replace the magic
constants (such as -1) with rtc::Optional in bitrate limiting code everywhere.
Until then, can we keep it like this?

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:85: if (a <= 0)
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> nit: {}s

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:905: virtual webrtc::RTCRtpParameters
GetRtpParameters(uint32_t ssrc) = 0;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> Again, can probably be const.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:793: int max_global_bitrate = -1;
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> This variable can remain inside the scoped block.
> 
> You can just pass "*changed_params.max_bandwidth_bps" into MinPositive.

Good catch, thanks!

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:805: // TODO(skvlad): limit
min_bitrate_bps to not exceed the lowest of all
On 2016/03/12 01:21:04, pthatcher1 wrote:
> limit => Limit

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:806: // per-SSRC bitrate limits
On 2016/03/12 01:21:04, pthatcher1 wrote:
> 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.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:823: max_global_bitrate,
On 2016/03/12 01:21:04, pthatcher1 wrote:
> You could make max_global_bitrate rtc::Optional<int> also.

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

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:824:
kv.second->rtp_parameters().encodings[0].max_bitrate_bps));
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> 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.
Added a TODO. I'll address this as part of the rtc::Optional<> change.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:825: }
On 2016/03/12 01:21:03, pthatcher1 wrote:
> 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.

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.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1060: void
VerifyEncoderBitrate(cricket::FakeVideoCapturer& capturer,
On 2016/03/14 16:26:51, pthatcher1 wrote:
> Should this be called SetAndExpectMaxBandwidth?

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1072: // Read back the
parameteres and verify they have the correct value
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> nit: Periods at ends of comments

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3060:
TEST_F(WebRtcVideoChannel2Test,
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> I'd add a "TODO: Change this when we support setting parameters for multiple
> encodings". May be obvious to us, but possibly not to someone else in the
> future.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:843:
CannotSetRtpParametersWithIncorrectNumberOfEncodings) {
On 2016/03/12 01:57:06, Taylor Brandstetter wrote:
> If you add a TODO comment for the other "IncorrectNumberOfEncodings" test, add
> it for this one too.

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1402
webrtc/pc/channel.cc:1402: return true;
On 2016/03/12 01:21:04, pthatcher1 wrote:
> Just make it a void if it can't fail.

For some reason I saw the definition of InvokeOnWorker and thought that only
methods that return bool are supported; I was blind to the existence of the
underlying thread::Invoke<T> method.
Fixed.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1600
webrtc/pc/channel.cc:1600: return
media_channel()->SetSendParameters(parameters);
On 2016/03/12 01:21:04, pthatcher1 wrote:
> Why was this moved to a separate method?

Oops, this is a leftover from an earlier version where this method was doing
more. Thanks for spotting.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1737
webrtc/pc/channel.cc:1737: return true;
On 2016/03/12 01:21:04, pthatcher1 wrote:
> same here

Done.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/pc/channel.cc#newcode1918
webrtc/pc/channel.cc:1918: }
On 2016/03/12 01:21:04, pthatcher1 wrote:
> same here

Done.

[pthatcher1, 2016-03-16 00:48:42]

Just one more simplification that I think we can make.

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1788583004/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:84: static T MinPositive(T a, T b) {
On 2016/03/15 21:18:18, skvlad wrote:
> On 2016/03/12 01:21:03, pthatcher1 wrote:
> > This would be more clear as the min of rtc::Optionals. 
> 
> I'm going to send a separate code review for the change to replace the magic
> constants (such as -1) with rtc::Optional in bitrate limiting code everywhere.
> Until then, can we keep it like this?

sure

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:801: bitrate_config_.min_bitrate_bps =
max_global_bitrate;
I think you can leave the code as it was with the code I describe below.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:803: // unified plan SDP.
I think you can remove this comment and replace it with the one mentioned in the
header file.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:807: // stream does not.
That's probably an optimization that isn't worth doing, since we'll be getting
rid of having two maximums and only having one once we move to unified plan.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1814:
rtp_parameters_.encodings[0].max_bitrate_bps, global_max_bitrate_));
I think we can just leave this code as it was with the change I mention below.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1860: return true;
I think this whole method could be more simple:


if (!ValidateRtpParameters(new_parameters)) {
  return false;
}

if (new_parameters.max_bitrate_bps != rtp_paramters_.max_bitrate_bps) {
  // The encoder will be reconfigured with the new bitrate on the next video
frame.
  pending_encoder_configuration_ = true;
}

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1919: 
I think we could simplify by just doing the following here:

// We use the minimum of the cross-stream max bitrate 
// and per-stream max bitrate.
int max_bitrate_bps = MinPositive(
  new_parameters.encodings[0].max_bitrate_bps,
  parameters_.max_bitrate_bps)
encoder_config.streams =
    CreateVideoStreams(clamped_codec, parameters_.options,
                       max_bitrate_bps, stream_count);

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:389: int global_max_bitrate_;
We have:

parameters_;
rtp_parameters_;
global_max_bitrate_;

First, I think we can remove the last one, and simply compute the min of the
other two every time we reconfigure the encoder and not store it.

Second, we should make it clear why we have two parameters, and what goes in
each.

Perhaps something like:

// parameters_ contains settings that are the same for all streams in the
MediaChannel, such as codecs and header extensions.
VideoSendStreamParameters parameters_;
// rtp_parameters_ contains settings that are unique for each stream, such as
max bitrate.
// TODO: Move ssrcs_ and ssrcs_groups_ into rtp_parameters_.
// TODO: Combine parameters_ and rtp_parameters once we have only one stream per
MediaChannel.
webrtc::RtpParameters rtp_parameters_;

[skvlad, 2016-03-16 02:29:22]

Simplified the logic in WebRtcVideoSendStream.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:801: bitrate_config_.min_bitrate_bps =
max_global_bitrate;
On 2016/03/16 00:48:42, pthatcher1 wrote:
> I think you can leave the code as it was with the code I describe below.

Done.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:803: // unified plan SDP.
On 2016/03/16 00:48:42, pthatcher1 wrote:
> I think you can remove this comment and replace it with the one mentioned in
the
> header file.

Done.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:807: // stream does not.
On 2016/03/16 00:48:42, pthatcher1 wrote:
> That's probably an optimization that isn't worth doing, since we'll be getting
> rid of having two maximums and only having one once we move to unified plan.

This optimization was in fact working in the previous CL - I forgot to remove
the TODO. I've replaced it with the simplified version now, which no longer has
the optimization.
Setting each limit to the same value it used to be still does not trigger
reconfiguration, though.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1814:
rtp_parameters_.encodings[0].max_bitrate_bps, global_max_bitrate_));
On 2016/03/16 00:48:42, pthatcher1 wrote:
> I think we can just leave this code as it was with the change I mention below.

Done.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1860: return true;
On 2016/03/16 00:48:42, pthatcher1 wrote:
> I think this whole method could be more simple:
> 
> 
> if (!ValidateRtpParameters(new_parameters)) {
>   return false;
> }
> 
> if (new_parameters.max_bitrate_bps != rtp_paramters_.max_bitrate_bps) {
>   // The encoder will be reconfigured with the new bitrate on the next video
> frame.
>   pending_encoder_configuration_ = true;
> }

Done.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1919: 
On 2016/03/16 00:48:42, pthatcher1 wrote:
> I think we could simplify by just doing the following here:
> 
> // We use the minimum of the cross-stream max bitrate 
> // and per-stream max bitrate.
> int max_bitrate_bps = MinPositive(
>   new_parameters.encodings[0].max_bitrate_bps,
>   parameters_.max_bitrate_bps)
> encoder_config.streams =
>     CreateVideoStreams(clamped_codec, parameters_.options,
>                        max_bitrate_bps, stream_count);

Done.

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1788583004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:389: int global_max_bitrate_;
On 2016/03/16 00:48:42, pthatcher1 wrote:
> We have:
> 
> parameters_;
> rtp_parameters_;
> global_max_bitrate_;
> 
> First, I think we can remove the last one, and simply compute the min of the
> other two every time we reconfigure the encoder and not store it.
> 
> Second, we should make it clear why we have two parameters, and what goes in
> each.
> 
> Perhaps something like:
> 
> // parameters_ contains settings that are the same for all streams in the
> MediaChannel, such as codecs and header extensions.
> VideoSendStreamParameters parameters_;
> // rtp_parameters_ contains settings that are unique for each stream, such as
> max bitrate.
> // TODO: Move ssrcs_ and ssrcs_groups_ into rtp_parameters_.
> // TODO: Combine parameters_ and rtp_parameters once we have only one stream
per
> MediaChannel.
> webrtc::RtpParameters rtp_parameters_;
> 

Done.

[pthatcher1, 2016-03-16 04:46:58]

lgtm 

Very nice

[skvlad, 2016-03-17 00:46:43]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2016-03-17 00:46:44]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1788583004/#ps60001 (title:
"Rebased on top of the latest master branch")

[commit-bot: I haz the power, 2016-03-17 00:46:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1788583004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1788583004/60001

[commit-bot: I haz the power, 2016-03-17 02:07:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-17 02:07:56]

Description was changed from

==========
Enable setting the maximum bitrate limit in RtpSender.

This change allows the application to limit the bitrate of the outgoing
audio and video streams at runtime. The API roughly follows the WebRTC
API draft, defining the RTCRtpParameters structure witn exactly one
encoding (simulcast streams are not exposed in the API for now).
(https://www.w3.org/TR/webrtc/#idl-def-RTCRtpParameters)

BUG=
==========

to

==========
Enable setting the maximum bitrate limit in RtpSender.

This change allows the application to limit the bitrate of the outgoing
audio and video streams at runtime. The API roughly follows the WebRTC
API draft, defining the RTCRtpParameters structure witn exactly one
encoding (simulcast streams are not exposed in the API for now).
(https://www.w3.org/TR/webrtc/#idl-def-RTCRtpParameters)

BUG=

Committed: https://crrev.com/dc1c62cd30e42d5bc31db347126476ca8e1b6c58
Cr-Commit-Position: refs/heads/master@{#12025}
==========

[commit-bot: I haz the power, 2016-03-17 02:07:57]

Patchset 4 (id:??) landed as
https://crrev.com/dc1c62cd30e42d5bc31db347126476ca8e1b6c58
Cr-Commit-Position: refs/heads/master@{#12025}