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

Updated structures and functions for setting the max bitrate limit to take rtc::Optional<int> instead of special constants (0 or -1).

BUG=

[skvlad, 2016-03-17 20:09:47]

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

[skvlad, 2016-03-17 20:09:48]

This change updates most of the code related to bandwidth limits to use
rtc::Optional<int> to represent the limit.

After this change, an Optional<int> with no value represents no limit, a
positive value sets a limit, and a negative or zero value is an error.

This replaces various special case constants previously used to represent no
limit (0 and -1).

[pthatcher1, 2016-03-17 21:51:29]

https://codereview.webrtc.org/1813763005/diff/1/webrtc/api/mediacontroller.cc
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/api/mediacontroller.cc...
webrtc/api/mediacontroller.cc:68: }
Can this be unset?

If not, why have it be optional?

https://codereview.webrtc.org/1813763005/diff/1/webrtc/base/optional_ios.h
File webrtc/base/optional_ios.h (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/base/optional_ios.h#ne...
webrtc/base/optional_ios.h:30: }
Can we put this in optional.h?  Is the dependency on ostream that bad?

Also, a name like "_ios" makes it sound like iOS.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call.h#newcode82
webrtc/call.h:82: } bitrate_config;
Should we just use kMaxBandwidth here and not have it be optional?

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc#newcode229
webrtc/call/call.cc:229: congestion_controller_->SetBweBitrates(
Can you make the -1 a named constant, perhaps kBandwidthUnset?

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc#newcode230
webrtc/call/call.cc:230: config_.bitrate_config.min_bitrate_bps,
This could be more clear by using optional:: value_or.

[skvlad, 2016-03-18 00:49:17]

Updated with code review feedback.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/api/mediacontroller.cc
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/api/mediacontroller.cc...
webrtc/api/mediacontroller.cc:68: }
On 2016/03/17 21:51:28, pthatcher1 wrote:
> Can this be unset?
> 
> If not, why have it be optional?
This value will become unset if a video stream is added with no bitrate limit.
I'm not that familiar with the codebase, but the behavior seems to be like this:
1. The Call gets created, and the maximum bitrate limit for the BWE is set at 2
Mbps.
2. When a video stream gets created, the max bitrate from codec parameters - if
any - overrides this limit. If it is not set - which is probably the common case
- this will clear the max bitrate in the call.
3. If there is a b= parameter in the remote description, it will override the
bitrate limit from the codec.

This probably also means that for audio-only calls with no limit in SDP the
bandwidth will remain capped at 2 Mbps.

So in summary - it can and most likely will be unset after the call starts.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/base/optional_ios.h
File webrtc/base/optional_ios.h (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/base/optional_ios.h#ne...
webrtc/base/optional_ios.h:30: }
On 2016/03/17 21:51:28, pthatcher1 wrote:
> Can we put this in optional.h?  Is the dependency on ostream that bad?
> 
> Also, a name like "_ios" makes it sound like iOS.

Ok - moved it to optional.h. The ostream dependency is nothing to worry about.
My original motivation for keeping it separate was that this operator is only
used for logging and is not really related to core functionality - it's not even
a member of the Optional class.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call.h#newcode82
webrtc/call.h:82: } bitrate_config;
On 2016/03/17 21:51:28, pthatcher1 wrote:
> Should we just use kMaxBandwidth here and not have it be optional?

It appears that the kMaxBandwidth value = 2Mbps is only effective for audio-only
calls. When a video stream gets created, it will either change to the value from
the SDP b= line, or will become unset. See my previous comment on the
MediaController.cc file for more details.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc#newcode229
webrtc/call/call.cc:229: congestion_controller_->SetBweBitrates(
On 2016/03/17 21:51:28, pthatcher1 wrote:
> Can you make the -1 a named constant, perhaps kBandwidthUnset?

Done.

https://codereview.webrtc.org/1813763005/diff/1/webrtc/call/call.cc#newcode230
webrtc/call/call.cc:230: config_.bitrate_config.min_bitrate_bps,
On 2016/03/17 21:51:28, pthatcher1 wrote:
> This could be more clear by using optional:: value_or.

Done. I didn't notice this very convenient method before.

[stefan-webrtc, 2016-03-18 08:28:40]

Would it be an option to make this parameter required instead of Optional, with
a default value of std::numeric_limits<int>::max()? Or is there some other
benefit we want from making it Optional?

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:2552: int b = 0;
I'd prefer a better name than "b".

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:560:
bitrate_config.max_bitrate_bps.value_or(kBitrateUnlimited);
Can't we do the same thing on line 229?

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
File webrtc/media/engine/simulcast.h (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
webrtc/media/engine/simulcast.h:55: int max_framerate);
Any plan on making other parameters optional? Seems like max_qp could be
optional for instance.

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
What is an optional optional? This looks super weird to me.

[pthatcher1, 2016-03-18 16:45:16]

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:560:
bitrate_config.max_bitrate_bps.value_or(kBitrateUnlimited);
On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> Can't we do the same thing on line 229?

+1

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
File webrtc/media/engine/simulcast.h (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
webrtc/media/engine/simulcast.h:55: int max_framerate);
On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> Any plan on making other parameters optional? Seems like max_qp could be
> optional for instance.

I think it makes sense to make a lot of these things Optional, but perhaps just
make it a TODO for now.  This CL is already pretty big.

[pthatcher1, 2016-03-18 16:49:16]

On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> Would it be an option to make this parameter required instead of Optional,
with
> a default value of std::numeric_limits<int>::max()? Or is there some other
> benefit we want from making it Optional?

I noticed code in simulcast.cc like this:

int bitrate_left_bps = max_bitrate_bps - GetTotalMaxBitrateBps(streams);
streams.back().max_bitrate_bps += bitrate_left_bps;
if (bitrate_left_bps > 0) {
  streams.back().max_bitrate_bps += bitrate_left_bps;
}

If the max_bitrate_bps there is a huge number, then the last stream is going to
get a huge number minus some smaller numbers, which is still a huge number. 
That might be OK, but I'm kind of worried about other places where using "huge
number" instead of "unset" might cause issues.  For example, if there's
something that says "if the application asked for a max bitrate higher than X,
do something more aggressive", and then we end up doing the aggressive thing by
default.

> 
> https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc
> File webrtc/api/webrtcsdp.cc (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc#n...
> webrtc/api/webrtcsdp.cc:2552: int b = 0;
> I'd prefer a better name than "b".
> 
> https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc#newco...
> webrtc/call/call.cc:560:
> bitrate_config.max_bitrate_bps.value_or(kBitrateUnlimited);
> Can't we do the same thing on line 229?
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
> File webrtc/media/engine/simulcast.h (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
> webrtc/media/engine/simulcast.h:55: int max_framerate);
> Any plan on making other parameters optional? Seems like max_qp could be
> optional for instance.
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.h (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.h:194:
rtc::Optional<rtc::Optional<int>>
> max_bandwidth_bps;
> What is an optional optional? This looks super weird to me.

It does seem weird at first.  But it actually makes sense as the first optional
means "did this change?" and the second is "if it change, what did it change
to?".  So you could change from unset to set or set to unset or not change at
all.  The alternative is to have a bool for "set" or "changed".  But that's just
reinventing rtc::Optional.

[skvlad, 2016-03-18 18:01:43]

On 2016/03/18 16:49:16, pthatcher1 wrote:
> On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > Would it be an option to make this parameter required instead of Optional,
> with
> > a default value of std::numeric_limits<int>::max()? Or is there some other
> > benefit we want from making it Optional?
> 
> I noticed code in simulcast.cc like this:
> 
> int bitrate_left_bps = max_bitrate_bps - GetTotalMaxBitrateBps(streams);
> streams.back().max_bitrate_bps += bitrate_left_bps;
> if (bitrate_left_bps > 0) {
>   streams.back().max_bitrate_bps += bitrate_left_bps;
> }
> 
> If the max_bitrate_bps there is a huge number, then the last stream is going
to
> get a huge number minus some smaller numbers, which is still a huge number. 
> That might be OK, but I'm kind of worried about other places where using "huge
> number" instead of "unset" might cause issues.  For example, if there's
> something that says "if the application asked for a max bitrate higher than X,
> do something more aggressive", and then we end up doing the aggressive thing
by
> default.

In addition to what Peter said, there are two more reasons why we treat no limit
differently from a really high limit:
 - the audio stream (which does not have RtpParameters wired up yet) uses a
default bitrate if the limit is not set - such as 64kbps for Opus. However, if
you set the limit to say 100kbps, it will configure the encoder with a 100kbps
bitrate.
 - the Javascript API represents the bitrate limit as an optional 64-bit
unsigned integer, while internally we use a 32-bit signed value. The
std::numeric_limits<int>::max() value will have no special meaning after
conversion - so Chrome would have to do something extra to recognize that
special value and replace it with a null, and vice-versa. rtc::Optional allows
for a more direct mapping.


> >
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
> > webrtc/media/engine/webrtcvideoengine2.h:194:
> rtc::Optional<rtc::Optional<int>>
> > max_bandwidth_bps;
> > What is an optional optional? This looks super weird to me.
> 
> It does seem weird at first.  But it actually makes sense as the first
optional
> means "did this change?" and the second is "if it change, what did it change
> to?".  So you could change from unset to set or set to unset or not change at
> all.  The alternative is to have a bool for "set" or "changed".  But that's
just
> reinventing rtc::Optional.

Do you think if we add a type alias - like rtc::Change<T> - for this particular
case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will the
overhead of the new type make it harder to read?

[skvlad, 2016-03-18 18:01:52]

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:2552: int b = 0;
On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> I'd prefer a better name than "b".

Replaced with "bandwidth_value" ("bandwidth" is used for the unparsed string).

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:560:
bitrate_config.max_bitrate_bps.value_or(kBitrateUnlimited);
On 2016/03/18 16:45:15, pthatcher1 wrote:
> On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > Can't we do the same thing on line 229?
> 
> +1
Done. Sorry, missed this.

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
File webrtc/media/engine/simulcast.h (right):

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/simul...
webrtc/media/engine/simulcast.h:55: int max_framerate);
On 2016/03/18 16:45:16, pthatcher1 wrote:
> On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > Any plan on making other parameters optional? Seems like max_qp could be
> > optional for instance.
> 
> I think it makes sense to make a lot of these things Optional, but perhaps
just
> make it a TODO for now.  This CL is already pretty big.

It looks like this function gets definite values for all the other parameters
(width/height/max_qp/max_framerate). The caller, however, uses some defaults.
I've added a TODO there.

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> What is an optional optional? This looks super weird to me.

As Peter said, this structure represents the diff from the previous values - and
it uses rtc::Optional<> with no value to indicate that there was no change.
The bandwidth limit parameter itself is optional - so it can not change at all
(outer rtc::Optional has no value), or it can change to a new limit
(rtc::Optional with rtc::Optional with a new value), or it can become unset
after it was set before - in which case it will be an rtc::Optional
(representing the diff) with an rtc::Optional (representing the setting) with no
value (no limit).

Do you think if we add a type alias - like rtc::Change<T> - for this particular
case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will the
overhead of the new type make it harder to read?

[pthatcher1, 2016-03-21 17:41:13]

lgtm

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
On 2016/03/18 18:01:52, skvlad wrote:
> On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > What is an optional optional? This looks super weird to me.
> 
> As Peter said, this structure represents the diff from the previous values -
and
> it uses rtc::Optional<> with no value to indicate that there was no change.
> The bandwidth limit parameter itself is optional - so it can not change at all
> (outer rtc::Optional has no value), or it can change to a new limit
> (rtc::Optional with rtc::Optional with a new value), or it can become unset
> after it was set before - in which case it will be an rtc::Optional
> (representing the diff) with an rtc::Optional (representing the setting) with
no
> value (no limit).
> 
> Do you think if we add a type alias - like rtc::Change<T> - for this
particular
> case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will the
> overhead of the new type make it harder to read?

I think that would make it less clear about what's going on.

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

A question for Peter: Has the W3C working group decided what calling
setParameters with a null or absent maxBitrate means? We're interpreting it to
mean "remove the limit", which I think is the right thing (since JS would have
to explicitly remove it from the getParameters output). But we should probably
make this clear in the spec if it isn't already.

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
On 2016/03/21 17:41:13, pthatcher1 wrote:
> On 2016/03/18 18:01:52, skvlad wrote:
> > On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > > What is an optional optional? This looks super weird to me.
> > 
> > As Peter said, this structure represents the diff from the previous values -
> and
> > it uses rtc::Optional<> with no value to indicate that there was no change.
> > The bandwidth limit parameter itself is optional - so it can not change at
all
> > (outer rtc::Optional has no value), or it can change to a new limit
> > (rtc::Optional with rtc::Optional with a new value), or it can become unset
> > after it was set before - in which case it will be an rtc::Optional
> > (representing the diff) with an rtc::Optional (representing the setting)
with
> no
> > value (no limit).
> > 
> > Do you think if we add a type alias - like rtc::Change<T> - for this
> particular
> > case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will
the
> > overhead of the new type make it harder to read?
> 
> I think that would make it less clear about what's going on.  

I think just a comment above this line explaining the semantics of each
"Optional" would suffice.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional_unit...
File webrtc/base/optional_unittest.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional_unit...
webrtc/base/optional_unittest.cc:516: }
Again, impressed with how thorough your unit tests are.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:214: RTC_DCHECK_GT(*config.bitrate_config.max_bitrate_bps,
0);
Is a max of 0 not allowed?

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:542: if (bitrate_config.max_bitrate_bps)
nit: as long as you're modifying this line, might as well add { } s

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:733: params.max_bandwidth_bps == 0 ?
-1 : params.max_bandwidth_bps);
Are we taking away the behavior where 0 uncaps max bitrate here?

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2390: if (!bps)
nit: {}s

[the sun, 2016-03-29 11:20:33]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2016-03-29 11:20:34]

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2372: LOG(LS_INFO) <<
"WebRtcVoiceMediaChannel::SetMaxSendBandwidth.";
Please add:
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());

[kwiberg-webrtc, 2016-03-29 11:30:44]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2016-03-29 11:30:46]

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:140: std::ostream& operator<<(std::ostream& stream,
rtc::Optional<T> value) {
Second argument should be const ref to avoid copying.

[pthatcher1, 2016-03-29 22:06:01]

I don't know what else a null/undefined max could be other than unconstrained.

But we should probably ask the W3C list if this is worth making explicit in the
spec.

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

Addressed some code review feedback.

A question to all of the reviewers: what should be the meaning of "b=AS:0" in
SDP? Should it result in no limit on bandwidth, should the stream be stopped or
is it an error? There seems to be no mention of the meaning of zero bandwidth in
any of the relevant RFCs.
Before this change, it would uncap the bandwidth for video, or have no effect on
audio (leaving the previous limit in place). Now it will result in an error (I
will add a unit test for this behavior if we decide it is the correct one). 
What do you think should the right behavior be?

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> On 2016/03/21 17:41:13, pthatcher1 wrote:
> > On 2016/03/18 18:01:52, skvlad wrote:
> > > On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > > > What is an optional optional? This looks super weird to me.
> > > 
> > > As Peter said, this structure represents the diff from the previous values
-
> > and
> > > it uses rtc::Optional<> with no value to indicate that there was no
change.
> > > The bandwidth limit parameter itself is optional - so it can not change at
> all
> > > (outer rtc::Optional has no value), or it can change to a new limit
> > > (rtc::Optional with rtc::Optional with a new value), or it can become
unset
> > > after it was set before - in which case it will be an rtc::Optional
> > > (representing the diff) with an rtc::Optional (representing the setting)
> with
> > no
> > > value (no limit).
> > > 
> > > Do you think if we add a type alias - like rtc::Change<T> - for this
> > particular
> > > case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will
> the
> > > overhead of the new type make it harder to read?
> > 
> > I think that would make it less clear about what's going on.  
> 
> I think just a comment above this line explaining the semantics of each
> "Optional" would suffice.

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:140: std::ostream& operator<<(std::ostream& stream,
rtc::Optional<T> value) {
On 2016/03/29 11:30:45, kwiberg-webrtc wrote:
> Second argument should be const ref to avoid copying.

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:214: RTC_DCHECK_GT(*config.bitrate_config.max_bitrate_bps,
0);
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> Is a max of 0 not allowed?

No. The bitrate limit must be positive or unset. This avoids dealing with the
ambiguities of what the meaning of a 0 limit is.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:542: if (bitrate_config.max_bitrate_bps)
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> nit: as long as you're modifying this line, might as well add { } s

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:733: params.max_bandwidth_bps == 0 ?
-1 : params.max_bandwidth_bps);
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> Are we taking away the behavior where 0 uncaps max bitrate here?

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.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2372: LOG(LS_INFO) <<
"WebRtcVoiceMediaChannel::SetMaxSendBandwidth.";
On 2016/03/29 11:20:34, the sun wrote:
> Please add:
> RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2390: if (!bps)
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> nit: {}s

Done.

[skvlad, 2016-03-30 19:40:45]

Addressed some code review feedback.

A question to all of the reviewers: what should be the meaning of "b=AS:0" in
SDP? Should it result in no limit on bandwidth, should the stream be stopped or
is it an error? There seems to be no mention of the meaning of zero bandwidth in
any of the relevant RFCs.
Before this change, it would uncap the bandwidth for video, or have no effect on
audio (leaving the previous limit in place). Now it will result in an error (I
will add a unit test for this behavior if we decide it is the correct one). 
What do you think should the right behavior be?

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

https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.h:194: rtc::Optional<rtc::Optional<int>>
max_bandwidth_bps;
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> On 2016/03/21 17:41:13, pthatcher1 wrote:
> > On 2016/03/18 18:01:52, skvlad wrote:
> > > On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > > > What is an optional optional? This looks super weird to me.
> > > 
> > > As Peter said, this structure represents the diff from the previous values
-
> > and
> > > it uses rtc::Optional<> with no value to indicate that there was no
change.
> > > The bandwidth limit parameter itself is optional - so it can not change at
> all
> > > (outer rtc::Optional has no value), or it can change to a new limit
> > > (rtc::Optional with rtc::Optional with a new value), or it can become
unset
> > > after it was set before - in which case it will be an rtc::Optional
> > > (representing the diff) with an rtc::Optional (representing the setting)
> with
> > no
> > > value (no limit).
> > > 
> > > Do you think if we add a type alias - like rtc::Change<T> - for this
> > particular
> > > case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or will
> the
> > > overhead of the new type make it harder to read?
> > 
> > I think that would make it less clear about what's going on.  
> 
> I think just a comment above this line explaining the semantics of each
> "Optional" would suffice.

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:140: std::ostream& operator<<(std::ostream& stream,
rtc::Optional<T> value) {
On 2016/03/29 11:30:45, kwiberg-webrtc wrote:
> Second argument should be const ref to avoid copying.

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:214: RTC_DCHECK_GT(*config.bitrate_config.max_bitrate_bps,
0);
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> Is a max of 0 not allowed?

No. The bitrate limit must be positive or unset. This avoids dealing with the
ambiguities of what the meaning of a 0 limit is.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:542: if (bitrate_config.max_bitrate_bps)
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> nit: as long as you're modifying this line, might as well add { } s

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:733: params.max_bandwidth_bps == 0 ?
-1 : params.max_bandwidth_bps);
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> Are we taking away the behavior where 0 uncaps max bitrate here?

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.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2372: LOG(LS_INFO) <<
"WebRtcVoiceMediaChannel::SetMaxSendBandwidth.";
On 2016/03/29 11:20:34, the sun wrote:
> Please add:
> RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2390: if (!bps)
On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> nit: {}s

Done.

[the sun, 2016-03-30 19:48:57]

Description was changed from

==========
Updated structures and functions for setting the max bitrate limit to take
rtc::Optional<int> instead of special constants (0 or -1).

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

to

==========
Updated structures and functions for setting the max bitrate limit to take
rtc::Optional<int> instead of special constants (0 or -1).

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

[the sun, 2016-03-30 19:48:57]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[pthatcher1, 2016-03-30 21:19:54]

Unless the spec says otherwise, I think we should treat "b=AS:0" would be the
same as {maxBitrate: 0}. I'd lean toward interpreting that as "don't send
anything", but if it's easier to just return an error for now, I think that's OK
as well.


On 2016/03/30 19:40:45, skvlad wrote:
> Addressed some code review feedback.
> 
> A question to all of the reviewers: what should be the meaning of "b=AS:0" in
> SDP? Should it result in no limit on bandwidth, should the stream be stopped
or
> is it an error? There seems to be no mention of the meaning of zero bandwidth
in
> any of the relevant RFCs.
> Before this change, it would uncap the bandwidth for video, or have no effect
on
> audio (leaving the previous limit in place). Now it will result in an error (I
> will add a unit test for this behavior if we decide it is the correct one). 
> What do you think should the right behavior be?
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.h (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.h:194:
rtc::Optional<rtc::Optional<int>>
> max_bandwidth_bps;
> On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> > On 2016/03/21 17:41:13, pthatcher1 wrote:
> > > On 2016/03/18 18:01:52, skvlad wrote:
> > > > On 2016/03/18 08:28:40, stefan-webrtc (holmer) wrote:
> > > > > What is an optional optional? This looks super weird to me.
> > > > 
> > > > As Peter said, this structure represents the diff from the previous
values
> -
> > > and
> > > > it uses rtc::Optional<> with no value to indicate that there was no
> change.
> > > > The bandwidth limit parameter itself is optional - so it can not change
at
> > all
> > > > (outer rtc::Optional has no value), or it can change to a new limit
> > > > (rtc::Optional with rtc::Optional with a new value), or it can become
> unset
> > > > after it was set before - in which case it will be an rtc::Optional
> > > > (representing the diff) with an rtc::Optional (representing the setting)
> > with
> > > no
> > > > value (no limit).
> > > > 
> > > > Do you think if we add a type alias - like rtc::Change<T> - for this
> > > particular
> > > > case, it will make it clearer? "rtc::Change<rtc::Optional<int>>"? Or
will
> > the
> > > > overhead of the new type make it harder to read?
> > > 
> > > I think that would make it less clear about what's going on.  
> > 
> > I think just a comment above this line explaining the semantics of each
> > "Optional" would suffice.
> 
> Done.
> 
> https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h
> File webrtc/base/optional.h (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/base/optional.h#ne...
> webrtc/base/optional.h:140: std::ostream& operator<<(std::ostream& stream,
> rtc::Optional<T> value) {
> On 2016/03/29 11:30:45, kwiberg-webrtc wrote:
> > Second argument should be const ref to avoid copying.
> 
> Done.
> 
> https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
> webrtc/call/call.cc:214: RTC_DCHECK_GT(*config.bitrate_config.max_bitrate_bps,
> 0);
> On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> > Is a max of 0 not allowed?
> 
> No. The bitrate limit must be positive or unset. This avoids dealing with the
> ambiguities of what the meaning of a 0 limit is.
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/call/call.cc#newco...
> webrtc/call/call.cc:542: if (bitrate_config.max_bitrate_bps)
> On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> > nit: as long as you're modifying this line, might as well add { } s
> 
> Done.
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.cc (left):
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.cc:733: params.max_bandwidth_bps == 0 ?
> -1 : params.max_bandwidth_bps);
> On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> > Are we taking away the behavior where 0 uncaps max bitrate here?
> 
> 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.
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:2372: LOG(LS_INFO) <<
> "WebRtcVoiceMediaChannel::SetMaxSendBandwidth.";
> On 2016/03/29 11:20:34, the sun wrote:
> > Please add:
> > RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
> 
> Done.
> 
>
https://codereview.webrtc.org/1813763005/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:2390: if (!bps)
> On 2016/03/29 02:26:58, Taylor Brandstetter wrote:
> > nit: {}s
> 
> Done.

[Taylor Brandstetter, 2016-06-22 18:25:05]

lgtm

[stefan-webrtc, 2017-02-21 16:12:45]

stefan@webrtc.org changed reviewers:
- stefan@webrtc.org

[pbos-webrtc, 2017-03-16 18:13:03]

pbos@webrtc.org changed reviewers:
- pbos@webrtc.org