Add PeerConnectionInterface::UpdateCallBitrate with call tests.

Add PeerConnectionInterface::UpdateCallBitrate with call tests.

Subsumed by https://codereview.chromium.org/2888303005/

BUG=webrtc:7395

[Taylor_Brandstetter, 2017-04-26 15:46:10]

deadbeef@chromium.org changed reviewers:
+ deadbeef@chromium.org

[Taylor_Brandstetter, 2017-04-26 15:46:11]

Approach to testing seems good, I just have some more comments about the code
and interactions between base config/mask

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call.cc#newcode1027
webrtc/call/call.cc:1027: }
I still think the code in this file could be simplified. Here's what I was
thinking:
https://codereview.webrtc.org/2844693002/diff2/1:20001/webrtc/call/call.cc

Note that I also replaced the errors in the above method with clamping

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call.h#newcode114
webrtc/call/call.h:114: static Call* Create(
Could you add a comment that this Create method is provided so that
"transport_send" can be mocked/faked for testing?

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:358: void SetCall() {
nit: "CreateCall"?

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:397: }
Could also test setting the same "start_bitrate_bps" twice and ensuring
SetBweBitrates is called twice (the "force_update") logic.

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:428: TEST_F(CallBitrateTest,
MaskStartLessThanConfigMinFails) {
I'm not sure if it should work this way. Since there's no way to get the current
config from the PC API level, how can an application prevent these errors? I'd
suggest just clamping the values if there's an inconsistency after applying the
mask.

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnection.cc#n...
webrtc/pc/peerconnection.cc:1282: call->SetBitrateConfigMask(mask);
Should check the return value (if it still has a return value in the next
revision).

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnectioninter...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnectioninter...
webrtc/pc/peerconnectioninterface_unittest.cc:3304:
TEST_F(PeerConnectionInterfaceTest, SetBitrateCurrentLtMinFails) {
nit: I'd spell out "LessThan".

[Zach Stein, 2017-05-04 22:32:44]

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call.cc#newcode1027
webrtc/call/call.cc:1027: }
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> I still think the code in this file could be simplified. Here's what I was
> thinking:
> https://codereview.webrtc.org/2844693002/diff2/1:20001/webrtc/call/call.cc
> 
> Note that I also replaced the errors in the above method with clamping

Acknowledged.

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call.h#newcode114
webrtc/call/call.h:114: static Call* Create(
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> Could you add a comment that this Create method is provided so that
> "transport_send" can be mocked/faked for testing?

Done.

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:358: void SetCall() {
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> nit: "CreateCall"?

Done.

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:397: }
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> Could also test setting the same "start_bitrate_bps" twice and ensuring
> SetBweBitrates is called twice (the "force_update") logic.

Great idea, thanks.

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:428: TEST_F(CallBitrateTest,
MaskStartLessThanConfigMinFails) {
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> I'm not sure if it should work this way. Since there's no way to get the
current
> config from the PC API level, how can an application prevent these errors? I'd
> suggest just clamping the values if there's an inconsistency after applying
the
> mask.

What do you think about having PeerConnection::SetBitrate return the clamped
values it computes instead of an RTCError? If we just clamp the values, the
client might be surprised that the parameters are not being respected when we
say everything is okay (and I'm not sure it is even easy to discover that the
parameters aren't being respected).

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnection.cc#n...
webrtc/pc/peerconnection.cc:1282: call->SetBitrateConfigMask(mask);
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> Should check the return value (if it still has a return value in the next
> revision).

Nice catch. I also added a test case.

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnectioninter...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2838233002/diff/1/webrtc/pc/peerconnectioninter...
webrtc/pc/peerconnectioninterface_unittest.cc:3304:
TEST_F(PeerConnectionInterfaceTest, SetBitrateCurrentLtMinFails) {
On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> nit: I'd spell out "LessThan".

Done.

[Taylor Brandstetter, 2017-05-05 08:04:31]

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

[Taylor Brandstetter, 2017-05-05 08:04:32]

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:428: TEST_F(CallBitrateTest,
MaskStartLessThanConfigMinFails) {
On 2017/05/04 22:32:43, Zach Stein wrote:
> On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> > I'm not sure if it should work this way. Since there's no way to get the
> current
> > config from the PC API level, how can an application prevent these errors?
I'd
> > suggest just clamping the values if there's an inconsistency after applying
> the
> > mask.
> 
> What do you think about having PeerConnection::SetBitrate return the clamped
> values it computes instead of an RTCError? If we just clamp the values, the
> client might be surprised that the parameters are not being respected when we
> say everything is okay (and I'm not sure it is even easy to discover that the
> parameters aren't being respected).

That's a good idea. RTCError was actually meant to be used for things like this,
when errors need to provide additional structured information.

However, it's still inconsistent that calling SetBitrateConfig then
SetBitrateConfigMask will return something with the clamped values, but calling
them in the reverse order will silently succeed. How does the application know
the parameters aren't being respected in that case? Surfacing the information
from SetRemoteDescription would be a little strange.

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

https://codereview.webrtc.org/2838233002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:899: // TODO(zstein):
s/same_max/bitrate_config.max_bitrate_bps == -1 || same_max/
Can you explain this TODO?

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

https://codereview.webrtc.org/2838233002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:492: 
I have some other suggestions for tests. Can you take a look here:

https://codereview.webrtc.org/2862653004/diff2/1:20001/webrtc/call/call_unitt...

[Zach Stein, 2017-05-09 00:41:26]

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:428: TEST_F(CallBitrateTest,
MaskStartLessThanConfigMinFails) {
On 2017/05/05 08:04:32, Taylor Brandstetter wrote:
> On 2017/05/04 22:32:43, Zach Stein wrote:
> > On 2017/04/26 15:46:11, Taylor_Brandstetter wrote:
> > > I'm not sure if it should work this way. Since there's no way to get the
> > current
> > > config from the PC API level, how can an application prevent these errors?
> I'd
> > > suggest just clamping the values if there's an inconsistency after
applying
> > the
> > > mask.
> > 
> > What do you think about having PeerConnection::SetBitrate return the clamped
> > values it computes instead of an RTCError? If we just clamp the values, the
> > client might be surprised that the parameters are not being respected when
we
> > say everything is okay (and I'm not sure it is even easy to discover that
the
> > parameters aren't being respected).
> 
> That's a good idea. RTCError was actually meant to be used for things like
this,
> when errors need to provide additional structured information.
> 
> However, it's still inconsistent that calling SetBitrateConfig then
> SetBitrateConfigMask will return something with the clamped values, but
calling
> them in the reverse order will silently succeed. How does the application know
> the parameters aren't being respected in that case? Surfacing the information
> from SetRemoteDescription would be a little strange.

I was thinking that inconsistency was "less bad" since we want to discourage
people from setting these values via SetRemoteDescription in the long term. It
still seems most natural to me that the call to
PC::SetBitrate/SetBitrateConfigMask should only be applied to the current remote
description instead of kept around as a global config (in which case clamping
makes less sense), but you have more established intuitions here than I do. So
the idea of returning the clamped values was just to make it more obvious in the
API that the values were clamped. It would be inconsistent with
SetRemoteDescription, but the behavior would be the same and there is no obvious
way to surface the information there...

I am planning on going forward with clamping and not returning anything from
PC::SetBitrate (we could decide to return the clamped values later instead),
unless you think we should consider this more now.

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

https://codereview.webrtc.org/2838233002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:899: // TODO(zstein):
s/same_max/bitrate_config.max_bitrate_bps == -1 || same_max/
On 2017/05/05 08:04:32, Taylor Brandstetter wrote:
> Can you explain this TODO?

I thought that setting max == -1 was intended to mean don't change anything, so
we could make the same optimization that is made with start <= 0. Looking again,
the probe controller will actually reset to a default value if you pass -1, so I
don't think we should make this change here (and I'll just delete the TODO).

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

https://codereview.webrtc.org/2838233002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:492: 
On 2017/05/05 08:04:32, Taylor Brandstetter wrote:
> I have some other suggestions for tests. Can you take a look here:
> 
>
https://codereview.webrtc.org/2862653004/diff2/1:20001/webrtc/call/call_unitt...

Thanks, these are great test cases. Did you have these passing with the clamped
version of the code? They don't pass for me (but I might have messed up a merge
somewhere).

[Taylor Brandstetter, 2017-05-09 15:51:54]

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

https://codereview.webrtc.org/2838233002/diff/1/webrtc/call/call_unittest.cc#...
webrtc/call/call_unittest.cc:428: TEST_F(CallBitrateTest,
MaskStartLessThanConfigMinFails) {
On 2017/05/09 00:41:26, Zach Stein wrote:
> I am planning on going forward with clamping and not returning anything from
> PC::SetBitrate (we could decide to return the clamped values later instead),
> unless you think we should consider this more now.

That sounds good to me. I think this is what's most consistent with existing
WebRTC APIs.

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

https://codereview.webrtc.org/2838233002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:492: 
On 2017/05/09 00:41:26, Zach Stein wrote:
> On 2017/05/05 08:04:32, Taylor Brandstetter wrote:
> > I have some other suggestions for tests. Can you take a look here:
> > 
> >
>
https://codereview.webrtc.org/2862653004/diff2/1:20001/webrtc/call/call_unitt...
> 
> Thanks, these are great test cases. Did you have these passing with the
clamped
> version of the code? They don't pass for me (but I might have messed up a
merge
> somewhere).

One is passing, but two aren't. The issue is that an old
"start_bitrate"/"current_bitrate" ends up being passed into SetBweBitrates if
SetBitrateConfig[Mask] is called with an unset/-1 value.

[Zach Stein, 2017-05-09 23:03:21]

Here is the next version with all of the tests I have passing.

[Taylor Brandstetter, 2017-05-11 04:16:15]

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:901: min > *bitrate_config_mask_.min_bitrate_bps) {
The code would be simpler if instead of "see if combined value will change, if
so pass it along", the algorithm was "compute new combined value, pass it along
if it's changed".

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:909: // TODO(zstein): Do we only want to update if the start
rate changed here?
Yes; there should be a test for that too.

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:948: !update.max_bitrate_bps) {
Getting into more corner-casey territory, but you could get past this check, but
still end up with nothing changed due to the clamping afterwards. Here's a test
for this and a fix: https://codereview.webrtc.org/2875703004/

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3352:
TEST_F(PeerConnectionInterfaceTest, SetBitrateCurrentLessThanSdpMin) {
Can you leave a comment above this test explaining what the "SDP min" refers to?

[Zach Stein, 2017-05-11 21:43:09]

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:901: min > *bitrate_config_mask_.min_bitrate_bps) {
On 2017/05/11 04:16:15, Taylor Brandstetter wrote:
> The code would be simpler if instead of "see if combined value will change, if
> so pass it along", the algorithm was "compute new combined value, pass it
along
> if it's changed".

For c->SetBitrateConfig(1,2,3); c->SetBitrateConfig(1,2,4); the second call
currently calls SetBweBitrates(1,2,4), not SetBweBitrates(1,-1,4), so it will
reset bandwidth estimation. I was trying to avoid changing that behavior but
maybe we don't care. I would guess that we want to reset bandwidth estimation if
the network properties changed sufficiently, which we don't have an indicator
for here.

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:909: // TODO(zstein): Do we only want to update if the start
rate changed here?
On 2017/05/11 04:16:15, Taylor Brandstetter wrote:
> Yes; there should be a test for that too.

Done.

Test is in this other CL (the 2 Elides tests):
https://codereview.chromium.org/2870383003/diff/1/webrtc/call/call_unittest.cc

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:948: !update.max_bitrate_bps) {
On 2017/05/11 04:16:15, Taylor Brandstetter wrote:
> Getting into more corner-casey territory, but you could get past this check,
but
> still end up with nothing changed due to the clamping afterwards. Here's a
test
> for this and a fix: https://codereview.webrtc.org/2875703004/

Maybe that is enough of an edge case that we don't care about eliding the call,
but I will take a look at your change.

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3352:
TEST_F(PeerConnectionInterfaceTest, SetBitrateCurrentLessThanSdpMin) {
On 2017/05/11 04:16:15, Taylor Brandstetter wrote:
> Can you leave a comment above this test explaining what the "SDP min" refers
to?

I'm not sure this test actually belongs here as is - it is really testing the
interaction between BitrateConfig and BitrateConfigMask, but it doesn't
explicitly do anything to call SetBitrateConfig, so it isn't very easy to
understand. Can you think of a way to make an indirect call to SetBitrateConfig?
If not, maybe we should just remove this (and rely on the Call tests).

[Taylor Brandstetter, 2017-05-11 23:14:08]

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2838233002/diff/100001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectioninterface_unittest.cc:3352:
TEST_F(PeerConnectionInterfaceTest, SetBitrateCurrentLessThanSdpMin) {
On 2017/05/11 21:43:09, Zach Stein wrote:
> On 2017/05/11 04:16:15, Taylor Brandstetter wrote:
> > Can you leave a comment above this test explaining what the "SDP min" refers
> to?
> 
> I'm not sure this test actually belongs here as is - it is really testing the
> interaction between BitrateConfig and BitrateConfigMask, but it doesn't
> explicitly do anything to call SetBitrateConfig, so it isn't very easy to
> understand. Can you think of a way to make an indirect call to
SetBitrateConfig?
> If not, maybe we should just remove this (and rely on the Call tests).

I'm fine with relying on the Call tests. But if you keep this, the appropriate
way to trigger SetBitrateConfig with a min would be calling
"SetRemoteDescription" with "x-google-min-bitrate" (could just be in a
hard-coded SDP string; there are examples if you search for it in code search).
Or you could use "b=AS" and try setting a start bitrate greater than the max,
which may be more future-proof.

https://codereview.webrtc.org/2838233002/diff/120001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2838233002/diff/120001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:956: << "nothing to update";
This could be spammy if it happens for every SDP offer/answer; can you change it
to LS_VERBOSE?

[Zach Stein, 2017-05-23 20:58:53]

https://codereview.webrtc.org/2838233002/diff/120001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2838233002/diff/120001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:956: << "nothing to update";
On 2017/05/11 23:14:08, Taylor Brandstetter wrote:
> This could be spammy if it happens for every SDP offer/answer; can you change
it
> to LS_VERBOSE?

Done.

[Zach Stein, 2017-05-30 21:55:39]

Description was changed from

==========
Add PeerConnectionInterface::UpdateCallBitrate with call tests.

BUG=webrtc:7395
==========

to

==========
Add PeerConnectionInterface::UpdateCallBitrate with call tests.

Subsumed by https://codereview.chromium.org/2888303005/

BUG=webrtc:7395
==========