Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793913008:
Add PeerConnectionInterface::UpdateCallBitrate. (Closed)
|
Created:
3 years, 8 months ago by Zach Stein Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, the sun, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd PeerConnectionInterface::UpdateCallBitrate.
This gives clients more control of the bandwidth estimator.
Subsumed by https://codereview.chromium.org/2838233002/
BUG=webrtc:7395
Patch Set 1 #Patch Set 2 : Fix proxy typo, invoke call_w on worker thread, and add a simple unit test. The test currently fail… #Patch Set 3 : masking approach #Patch Set 4 : remove unused member #Patch Set 5 : add an error message and clarify test #
Total comments: 9
Patch Set 6 : sticky config mask #Patch Set 7 : add a todo about de-duplicating bitrate mask structs #
Total comments: 25
Patch Set 8 : minor fixes - still 2 todos in call #
Total comments: 6
Patch Set 9 : Move BitrateParameters validation to pc and use RTCError, clamp SetBitrate's max, other verbage/rea… #Patch Set 10 : move start DCHECK in SetBitrateConfig with other DCHECKs #
Total comments: 18
Patch Set 11 : Force update if current bitrate is set. Remove big lambda. Improve parameter validation. #
Total comments: 2
Messages
Total messages: 21 (4 generated)
Description was changed from ========== Add PeerConnectionInterface::UpdateCallBitrate. This allows clients more control of the bandwidth estimator. BUG=NONE ========== to ========== Add PeerConnectionInterface::UpdateCallBitrate. This gives clients more control of the bandwidth estimator. BUG=webrtc:7395 ==========
zstein@webrtc.org changed reviewers: + deadbeef@webrtc.org
What do you think of this approach? We could also keep the mask around as a private member of Call if we also wanted to apply it to future calls to SetBitrateConfig (via WebRtcVideoChannel2::SetSendParameters).
zstein@webrtc.org changed reviewers: + stefan@webrtc.org
Could you take a look at the changes to call Stefan? Right now, calls from the peer connection API will not be considered in future calls to the video channel's SetSendParameters.
This still needs to address the conflicts between the current invocations of "SetBitrateConfig", right? Or is the idea that applications should just call "UpdateBitrateConfig" again after SetRemoteDescription? Aside from that, this looks good. https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: }; Couldn't one struct be used by both PeerConnection and Call? https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... webrtc/pc/peerconnectioninterface_unittest.cc:3295: pc_->UpdateCallBitrate(update); This test doesn't have any expectations, so is it mainly to verify that nothing crashes? I'd mention that in a comment. Can you also add a TODO to add a more meaningful test for this? I don't think there's any easy way right now; the best we could do would probably be allowing a "FakeCall" to be injected into PeerConnection (which we do in other places).
On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > This still needs to address the conflicts between the current invocations of > "SetBitrateConfig", right? Or is the idea that applications should just call > "UpdateBitrateConfig" again after SetRemoteDescription? > > Aside from that, this looks good. > > https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h > File webrtc/call/call.h (right): > > https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h#newcode82 > webrtc/call/call.h:82: }; > Couldn't one struct be used by both PeerConnection and Call? > > https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... > File webrtc/pc/peerconnectioninterface_unittest.cc (right): > > https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... > webrtc/pc/peerconnectioninterface_unittest.cc:3295: > pc_->UpdateCallBitrate(update); > This test doesn't have any expectations, so is it mainly to verify that nothing > crashes? I'd mention that in a comment. > > Can you also add a TODO to add a more meaningful test for this? I don't think > there's any easy way right now; the best we could do would probably be allowing > a "FakeCall" to be injected into PeerConnection (which we do in other places). I think that whether or not UpdateCallBitrate overrides SetRemoteDescription depends on the expected uses of SetRemoteDescription, which I don't have strong intuitions for. If a call to SetRemoteDescription is likely to change the active network, I think the client should have to call UpdateCallBitrate again (if the old bitrate was tied to the properties of the old network configuration). If a call to SetRemoteDescription is not likely to change the active network, the client should not have to call UpdateCallBitrate again. Which of these sounds right to you (or am I thinking of SetRemoteDescription incorrectly)?
https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: }; On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > Couldn't one struct be used by both PeerConnection and Call? I did it this way because I wasn't sure where that struct should go. I am assuming that Call is an implementation detail of PeerConnection, so PeerConnectionInterface should not depend on Call and Call should not depend on PeerConnectionInterface. The struct could go in base instead, so that both could depend on it, but that seemed like overkill to me (I'm not sure this struct is generally useful enough to be in base). The high-level dependency structure is still pretty fuzzy to me though, so let me know what you think! https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... webrtc/pc/peerconnectioninterface_unittest.cc:3295: pc_->UpdateCallBitrate(update); On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > This test doesn't have any expectations, so is it mainly to verify that nothing > crashes? I'd mention that in a comment. > > Can you also add a TODO to add a more meaningful test for this? I don't think > there's any easy way right now; the best we could do would probably be allowing > a "FakeCall" to be injected into PeerConnection (which we do in other places). The comment on 3293-3294 was my attempt to explain the lack of EXPECTs. Should I just put the TODO after this test case?
> I think that whether or not UpdateCallBitrate overrides SetRemoteDescription > depends on the expected uses of SetRemoteDescription, which I don't have strong > intuitions for. If a call to SetRemoteDescription is likely to change the active > network, I think the client should have to call UpdateCallBitrate again (if the > old bitrate was tied to the properties of the old network configuration). If a > call to SetRemoteDescription is not likely to change the active network, the > client should not have to call UpdateCallBitrate again. Which of these sounds > right to you (or am I thinking of SetRemoteDescription incorrectly)? SetRemoteDescription is called whenever a new offer/answer exchange is done. Which could happen for many reasons; the remote description could even be exactly the same as the previously-set one. So we can't assume it's tied to a network change. And I think it would be unexpected if every time it's called, it clears some previously-set, seemingly-unrelated configuration. https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: }; On 2017/04/09 21:09:01, Zach Stein wrote: > On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > > Couldn't one struct be used by both PeerConnection and Call? > > I did it this way because I wasn't sure where that struct should go. > > I am assuming that Call is an implementation detail of PeerConnection, so > PeerConnectionInterface should not depend on Call and Call should not depend on > PeerConnectionInterface. The struct could go in base instead, so that both could > depend on it, but that seemed like overkill to me (I'm not sure this struct is > generally useful enough to be in base). The high-level dependency structure is > still pretty fuzzy to me though, so let me know what you think! PeerConnectionInterface (being the "public interface") shouldn't depend on Call, but Call can depend on things in the public interface. So I'd move BitrateConfig to some header file in api/ (either peerconnectioninterface.h, or something else that makes sense). Maybe it's overkill, but I prefer cutting down on duplicated structures/classes; we already have a lot of them. https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnectioni... webrtc/pc/peerconnectioninterface_unittest.cc:3295: pc_->UpdateCallBitrate(update); On 2017/04/09 21:09:01, Zach Stein wrote: > On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > > This test doesn't have any expectations, so is it mainly to verify that > nothing > > crashes? I'd mention that in a comment. > > > > Can you also add a TODO to add a more meaningful test for this? I don't think > > there's any easy way right now; the best we could do would probably be > allowing > > a "FakeCall" to be injected into PeerConnection (which we do in other places). > > The comment on 3293-3294 was my attempt to explain the lack of EXPECTs. > > Should I just put the TODO after this test case? That would be fine.
https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: }; On 2017/04/10 04:59:51, Taylor Brandstetter wrote: > On 2017/04/09 21:09:01, Zach Stein wrote: > > On 2017/04/07 03:29:12, Taylor Brandstetter wrote: > > > Couldn't one struct be used by both PeerConnection and Call? > > > > I did it this way because I wasn't sure where that struct should go. > > > > I am assuming that Call is an implementation detail of PeerConnection, so > > PeerConnectionInterface should not depend on Call and Call should not depend > on > > PeerConnectionInterface. The struct could go in base instead, so that both > could > > depend on it, but that seemed like overkill to me (I'm not sure this struct is > > generally useful enough to be in base). The high-level dependency structure is > > still pretty fuzzy to me though, so let me know what you think! > > PeerConnectionInterface (being the "public interface") shouldn't depend on Call, > but Call can depend on things in the public interface. So I'd move BitrateConfig > to some header file in api/ (either peerconnectioninterface.h, or something else > that makes sense). Maybe it's overkill, but I prefer cutting down on duplicated > structures/classes; we already have a lot of them. But will we not want to have this also for ORTC at some point? In that case I wouldn't expect ORTC to include peerconnectioninterface.h? Also, I think there are tests in other repositories that build on Call instead of PeerConnection to be more light weight. https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnection.... webrtc/pc/peerconnection.cc:1254: LOG_F(LS_ERROR) << "media_controller_ is NULL."; Can we simply DCHECK on this instead, or is it possible for a user to provoke this?
Here is my first attempt at making PeerConnectionInterface::SetCallBitrate sticky. I haven't de-duplicated the bitrate mask structs yet or improved the testing story. Please take a look. https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/80001/webrtc/pc/peerconnection.... webrtc/pc/peerconnection.cc:1254: LOG_F(LS_ERROR) << "media_controller_ is NULL."; On 2017/04/10 13:26:58, stefan-webrtc wrote: > Can we simply DCHECK on this instead, or is it possible for a user to provoke > this? The user can only hit this if they call UpdateCallBitrate before Initialize - so it shouldn't ever happen, but if we only have a DCHECK we could dereference a nullptr, so I think this is the right thing to do.
https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:731: // Parameters that are not set will defer to implementation defined values. "or values from x-google-[min/start/max]-bitrate, or b=AS"? Probably worth mentioning, as long as it works that way. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:735: rtc::Optional<int> max_bitrate_bps; I think it's worth describing the behavior of min/start/max at a high level here. For example, how changes to "start" will restart bandwidth estimation (I believe that's what we found). https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:738: // Updates the Call level bitrate parameters. I wouldn't say "Call level", since someone just using the PeerConnection API won't know what that means. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:953: RTC_DCHECK_GE(*mask.min_bitrate_bps, 0); In general we don't assert as a result of bad input at the API level. Could the method either return an error (see "RTCError"), or do the clamping I've seen elsewhere with BitrateConfigs? https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:971: const Config::BitrateConfig& bitrate_config = config_.bitrate_config; It seems unintuitive that config_.bitrate_config isn't the one actually being used. I'd prefer reversing it, personally, and storing the "from SDP" BitrateConfig in a separate member variable. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:979: bitrate_config.max_bitrate_bps); I'm not sure the "mask" should completely override the max value that came from SDP. In general, the PeerConnection APIs operate within the envelope negotiated by SDP. So if a "b=AS" bandwidth limit is present, it should be honored even if the application called SetCallBitrate. Meaning, take the minimum of the two maximums here. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:983: masked_bitrate_config_.start_bitrate_bps || Should this be the same condition as in SetBitrateConfig, including "|| bitrate_config.start_bitrate_bps <= 0"? May be worth making a helper function. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h#newco... webrtc/call/call.h:79: // instead (and move BitrateParameters to base/ or api/). Just FYI, it would be api/ since it's used in the API. The long term plan is for api/ to not depend on any other directories. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h#newco... webrtc/call/call.h:155: virtual void SetBitrateConfigMask( Just an idea/bikeshed: calling these methods something like "SetSdpBitrateConfig" and "SetApplicationBitrateConfig" may make it more clear what the two represent. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1254: LOG_F(LS_ERROR) << "media_controller_ is NULL."; Is this ever expected to happen? If not, you can just "RTC_DCHECK(media_controller_)". https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection.h File webrtc/pc/peerconnection.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.h:149: void SetCallBitrate_w(const BitrateParameters& bitrate); The "invoked on worker thread" method can be private. Or you could just have one method that invokes itself if needed.
Thanks for the detailed feedback so far. I am working on the 2 TODOs in call.cc , but most of the style changes are here. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:731: // Parameters that are not set will defer to implementation defined values. On 2017/04/12 01:33:41, Taylor Brandstetter wrote: > "or values from x-google-[min/start/max]-bitrate, or b=AS"? Probably worth > mentioning, as long as it works that way. I was being intentionally vague to avoid this getting out of sync in the future. I meant for implementation defined values to include all the things we do now (static constants, SDP, and codec parameters), but I see that it would be easy to interpret as only being constants. Can you think of a more concise way to say "the implementation is free to choose whatever values it wants for parameters that are not set here?" https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:735: rtc::Optional<int> max_bitrate_bps; On 2017/04/12 01:33:41, Taylor Brandstetter wrote: > I think it's worth describing the behavior of min/start/max at a high level > here. For example, how changes to "start" will restart bandwidth estimation (I > believe that's what we found). Done. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:738: // Updates the Call level bitrate parameters. On 2017/04/12 01:33:41, Taylor Brandstetter wrote: > I wouldn't say "Call level", since someone just using the PeerConnection API > won't know what that means. Done. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:979: bitrate_config.max_bitrate_bps); On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > I'm not sure the "mask" should completely override the max value that came from > SDP. In general, the PeerConnection APIs operate within the envelope negotiated > by SDP. So if a "b=AS" bandwidth limit is present, it should be honored even if > the application called SetCallBitrate. Meaning, take the minimum of the two > maximums here. We don't know that bitrate_config.max_bitrate_bps was set by the SDP here - it might have come from a codec param (but maybe that is the code that should be changed). https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h#newco... webrtc/call/call.h:79: // instead (and move BitrateParameters to base/ or api/). On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > Just FYI, it would be api/ since it's used in the API. The long term plan is for > api/ to not depend on any other directories. Acknowledged. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.h#newco... webrtc/call/call.h:155: virtual void SetBitrateConfigMask( On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > Just an idea/bikeshed: calling these methods something like > "SetSdpBitrateConfig" and "SetApplicationBitrateConfig" may make it more clear > what the two represent. It felt wrong to talk about SDP here, so I tried to go with more generic terms. I don't feel too strongly though. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1254: LOG_F(LS_ERROR) << "media_controller_ is NULL."; On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > Is this ever expected to happen? If not, you can just > "RTC_DCHECK(media_controller_)". Done. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection.h File webrtc/pc/peerconnection.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.h:149: void SetCallBitrate_w(const BitrateParameters& bitrate); On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > The "invoked on worker thread" method can be private. Or you could just have one > method that invokes itself if needed. Done.
https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:731: // Parameters that are not set will defer to implementation defined values. On 2017/04/13 00:26:36, Zach Stein wrote: > On 2017/04/12 01:33:41, Taylor Brandstetter wrote: > > "or values from x-google-[min/start/max]-bitrate, or b=AS"? Probably worth > > mentioning, as long as it works that way. > > I was being intentionally vague to avoid this getting out of sync in the future. > I meant for implementation defined values to include all the things we do now > (static constants, SDP, and codec parameters), but I see that it would be easy > to interpret as only being constants. Can you think of a more concise way to say > "the implementation is free to choose whatever values it wants for parameters > that are not set here?" If you were being intentionally vague, "implementation defined" sounds good. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:979: bitrate_config.max_bitrate_bps); On 2017/04/13 00:26:36, Zach Stein wrote: > On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > > I'm not sure the "mask" should completely override the max value that came > from > > SDP. In general, the PeerConnection APIs operate within the envelope > negotiated > > by SDP. So if a "b=AS" bandwidth limit is present, it should be honored even > if > > the application called SetCallBitrate. Meaning, take the minimum of the two > > maximums here. > > We don't know that bitrate_config.max_bitrate_bps was set by the SDP here - it > might have come from a codec param (but maybe that is the code that should be > changed). The codec params also come from SDP (from "a=fmtp" lines). I do understand your point that both "x-google-max-bitrate" and "b=AS" end up condensed to a single value, but "x-google-max-bitrate" is non-standard, and more of a corner case, so we should optimize for the "b=AS" case if anything. https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:733: // Changing start will reset the current bitrate estimate. nit: I'd still prefer a more thorough description. For example: "SetBitrate can be used to control the bandwidth allocated for all RTP streams sent by this PeerConnection, within the bounds of other limits that may be imposed (for example, by "b=TIAS" in SDP). |min_bitrate_bps| and |max_bitrate_bps|, if set, clamp the allocated bandwidth, which typically would come from the bandwidth estimator. For example, if |max_bitrate_bps| is set to 2Mbps, the bandwidth allocated for all RTP streams will not exceed 2Mbps, even if the bandwidth estimator predicts that more bandwidth is available. |start_bitrate_bps|, if set, resets the current bandwidth allocation, which may increase or decrease as the bandwidth estimator continues to operate. If SetBitrate is called a second time, and |start_bitrate_bps| is different from the previous value, the bandwidth allocation will be reset to the provided value. Otherwise (if |start_bitrate_bps| is unchanged), it will have no effect." https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:736: rtc::Optional<int> start_bitrate_bps; While writing the above description, I was reminded that "start_bitrate_bps" isn't the most intuitive interface. It's called "start", but it could be used in the middle of a call. And the rule that it only has an effect if it changes means that if an application wants to restart bandwidth estimation from the same point it last started, it would need to do something like: BitrateParameters new_params; // Needs to be different from the previous value to have an effect. new_params.start_bitrate_bps.emplace(old_start_bitrate+1); pc->SetBitrate(new_params); What do you think about changing it to "current_bitrate_bps", and changing the behavior to "reset current bitrate *any time* it's set", not just when it's different from the previous value? Hopefully I'm making sense. https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:741: virtual void SetBitrate(const BitrateParameters& bitrate) = 0; nit: "SetBitrateParameters"?
Please take another look. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:953: RTC_DCHECK_GE(*mask.min_bitrate_bps, 0); On 2017/04/12 01:33:41, Taylor Brandstetter wrote: > In general we don't assert as a result of bad input at the API level. Could the > method either return an error (see "RTCError"), or do the clamping I've seen > elsewhere with BitrateConfigs? Done. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:971: const Config::BitrateConfig& bitrate_config = config_.bitrate_config; On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > It seems unintuitive that config_.bitrate_config isn't the one actually being > used. I'd prefer reversing it, personally, and storing the "from SDP" > BitrateConfig in a separate member variable. Done. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:979: bitrate_config.max_bitrate_bps); On 2017/04/13 22:16:13, Taylor Brandstetter wrote: > On 2017/04/13 00:26:36, Zach Stein wrote: > > On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > > > I'm not sure the "mask" should completely override the max value that came > > from > > > SDP. In general, the PeerConnection APIs operate within the envelope > > negotiated > > > by SDP. So if a "b=AS" bandwidth limit is present, it should be honored even > > if > > > the application called SetCallBitrate. Meaning, take the minimum of the two > > > maximums here. > > > > We don't know that bitrate_config.max_bitrate_bps was set by the SDP here - it > > might have come from a codec param (but maybe that is the code that should be > > changed). > > The codec params also come from SDP (from "a=fmtp" lines). I do understand your > point that both "x-google-max-bitrate" and "b=AS" end up condensed to a single > value, but "x-google-max-bitrate" is non-standard, and more of a corner case, so > we should optimize for the "b=AS" case if anything. I didn't realize that the codec params also came from SDP. https://codereview.webrtc.org/2793913008/diff/120001/webrtc/call/call.cc#newc... webrtc/call/call.cc:983: masked_bitrate_config_.start_bitrate_bps || On 2017/04/12 01:33:42, Taylor Brandstetter wrote: > Should this be the same condition as in SetBitrateConfig, including "|| > bitrate_config.start_bitrate_bps <= 0"? May be worth making a helper function. Done. https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:736: rtc::Optional<int> start_bitrate_bps; On 2017/04/13 22:16:13, Taylor Brandstetter wrote: > While writing the above description, I was reminded that "start_bitrate_bps" > isn't the most intuitive interface. It's called "start", but it could be used in > the middle of a call. And the rule that it only has an effect if it changes > means that if an application wants to restart bandwidth estimation from the same > point it last started, it would need to do something like: > > BitrateParameters new_params; > // Needs to be different from the previous value to have an effect. > new_params.start_bitrate_bps.emplace(old_start_bitrate+1); > pc->SetBitrate(new_params); > > What do you think about changing it to "current_bitrate_bps", and changing the > behavior to "reset current bitrate *any time* it's set", not just when it's > different from the previous value? Hopefully I'm making sense. I like current better than start, but I would prefer not to change SetBweBitrates to interpret the same value as a reset in this change and adding 1 to start in peerconnection.cc seems too hacky to me.
Looks good. My only real remaining concern is that, if we're planning to change the semantics of "current_bitrate_bps", it would be better to do that before the API becomes public. Also, it would be nice to have some more test coverage. Though I'm not seeing an easy way to do that without more work on test infrastructure; maybe stefan@ will have a suggestion when he gets back. https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:736: rtc::Optional<int> start_bitrate_bps; On 2017/04/18 22:54:50, Zach Stein wrote: > On 2017/04/13 22:16:13, Taylor Brandstetter wrote: > > While writing the above description, I was reminded that "start_bitrate_bps" > > isn't the most intuitive interface. It's called "start", but it could be used > in > > the middle of a call. And the rule that it only has an effect if it changes > > means that if an application wants to restart bandwidth estimation from the > same > > point it last started, it would need to do something like: > > > > BitrateParameters new_params; > > // Needs to be different from the previous value to have an effect. > > new_params.start_bitrate_bps.emplace(old_start_bitrate+1); > > pc->SetBitrate(new_params); > > > > What do you think about changing it to "current_bitrate_bps", and changing the > > behavior to "reset current bitrate *any time* it's set", not just when it's > > different from the previous value? Hopefully I'm making sense. > > I like current better than start, but I would prefer not to change > SetBweBitrates to interpret the same value as a reset in this change and adding > 1 to start in peerconnection.cc seems too hacky to me. When you say you'd prefer not changing SetBweBitrates in this change, you mean you'd rather do it in a standalone CL? That seems like a good idea. It would be preferable to land that CL before this one, so that the PeerConnection API has the right semantics the first time it's introduced. Otherwise applications could start using it, then be surprised when its behavior changes. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:187: // |base_bitrate_config_| and |bitrate_config_mask_| and udpates the Typo, "udpates" https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:962: const rtc::Optional<int> mask_max = bitrate_config_mask_.max_bitrate_bps; nit: Can be const ref. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:974: } nit: I think this code could be simplified. Example: masked.max_bitrate_bps = MinPositive( base_bitrate_config_.max_bitrate_bps base_bitrate_config_mask_.max_bitrate_bps.value_or(-1)); Though, if you think this is less readable, what you have is good. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:978: masked.start_bitrate_bps == current.start_bitrate_bps && Same comment as before: shouldn't this be checking the same conditions as in SetBitrateConfig, on line 928? In fact, those checks should just be moved here, since having them in both places would be redundant. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1239: return factory_->worker_thread()->Invoke<RTCError>( To avoid the extra indentation, and because the chromium style guide prohibits storing capturing lambdas, could this be changed to: if (!factory_->worker_thread()->IsCurrent()) { return factory_->worker_thread()->Invoke<RTCError>( RTC_FROM_HERE, rtc::Bind(&PeerConnection::SetBitrate, this, bitrate)); } // rest of stuff https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1244: if (has_min && *bitrate.min_bitrate_bps <= 0) { Isn't "min == 0" ok? https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1245: return RTCError(RTCErrorType::INVALID_PARAMETER, Can you use the "RETURN_AND_LOG_ERROR" macro, so that these errors will appear in the log as well? https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnectioninterface_unittest.cc:3296: } Can you add a basic test for the errors?
https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2793913008/diff/140001/webrtc/api/peerconnectio... webrtc/api/peerconnectioninterface.h:736: rtc::Optional<int> start_bitrate_bps; On 2017/04/19 01:06:53, Taylor Brandstetter wrote: > On 2017/04/18 22:54:50, Zach Stein wrote: > > On 2017/04/13 22:16:13, Taylor Brandstetter wrote: > > > While writing the above description, I was reminded that "start_bitrate_bps" > > > isn't the most intuitive interface. It's called "start", but it could be > used > > in > > > the middle of a call. And the rule that it only has an effect if it changes > > > means that if an application wants to restart bandwidth estimation from the > > same > > > point it last started, it would need to do something like: > > > > > > BitrateParameters new_params; > > > // Needs to be different from the previous value to have an effect. > > > new_params.start_bitrate_bps.emplace(old_start_bitrate+1); > > > pc->SetBitrate(new_params); > > > > > > What do you think about changing it to "current_bitrate_bps", and changing > the > > > behavior to "reset current bitrate *any time* it's set", not just when it's > > > different from the previous value? Hopefully I'm making sense. > > > > I like current better than start, but I would prefer not to change > > SetBweBitrates to interpret the same value as a reset in this change and > adding > > 1 to start in peerconnection.cc seems too hacky to me. > > When you say you'd prefer not changing SetBweBitrates in this change, you mean > you'd rather do it in a standalone CL? That seems like a good idea. It would be > preferable to land that CL before this one, so that the PeerConnection API has > the right semantics the first time it's introduced. Otherwise applications could > start using it, then be surprised when its behavior changes. Looking at SetBweBitrates again, I think it actually sets current/start as long as it is > 0, so I can make this change in Call in this CL. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:187: // |base_bitrate_config_| and |bitrate_config_mask_| and udpates the On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > Typo, "udpates" Done. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:962: const rtc::Optional<int> mask_max = bitrate_config_mask_.max_bitrate_bps; On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > nit: Can be const ref. Done. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:974: } On 2017/04/19 01:06:53, Taylor Brandstetter wrote: > nit: I think this code could be simplified. Example: > > masked.max_bitrate_bps = MinPositive( > base_bitrate_config_.max_bitrate_bps > base_bitrate_config_mask_.max_bitrate_bps.value_or(-1)); > > Though, if you think this is less readable, what you have is good. Didn't know about MinPositive (defined in mediachannel.h). I also considered: if (mask_has_max && base_has_max) std::min(*mask_max, base_max); else masked_max.value_or(base_max); But I think what I have is most readable. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:978: masked.start_bitrate_bps == current.start_bitrate_bps && On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > Same comment as before: shouldn't this be checking the same conditions as in > SetBitrateConfig, on line 928? In fact, those checks should just be moved here, > since having them in both places would be redundant. I was avoiding changing the behavior of SetBitrateConfig when SetBitrateConfigMask is not called since I don't see where it is tested. The idea here was to avoid calling SetBweBitrates if there is no mask or the mask sets the same value as was set by SetBitrateConfig (BitrateParameters enforces 0 <= min <= current <= max for set parameters - I should add that constraint to BitrateConfigMask). I will revisit changing SetBitrateConfig. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1239: return factory_->worker_thread()->Invoke<RTCError>( On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > To avoid the extra indentation, and because the chromium style guide prohibits > storing capturing lambdas, could this be changed to: > > if (!factory_->worker_thread()->IsCurrent()) { > return factory_->worker_thread()->Invoke<RTCError>( > RTC_FROM_HERE, rtc::Bind(&PeerConnection::SetBitrate, this, bitrate)); > } > // rest of stuff Done. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1244: if (has_min && *bitrate.min_bitrate_bps <= 0) { On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > Isn't "min == 0" ok? This makes setting all of the parameters to 0 legal, but I doubt this matters much in practice (the values will be clamped to higher values in at least one place later on). https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1245: return RTCError(RTCErrorType::INVALID_PARAMETER, On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > Can you use the "RETURN_AND_LOG_ERROR" macro, so that these errors will appear > in the log as well? Done. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/pc/peerconnection... webrtc/pc/peerconnectioninterface_unittest.cc:3296: } On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > Can you add a basic test for the errors? Done. https://codereview.webrtc.org/2793913008/diff/200001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/200001/webrtc/call/call.cc#newc... webrtc/call/call.cc:935: same_max) { (bitrate_config.max_bitrate_bps == -1 || same_max)? https://codereview.webrtc.org/2793913008/diff/200001/webrtc/call/call.cc#newc... webrtc/call/call.cc:999: RTCError Call::UpdateCurrentBitrateConfig(bool force_update) { Looking in to adding Call tests next.
https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:974: } On 2017/04/20 20:48:00, Zach Stein wrote: > On 2017/04/19 01:06:53, Taylor Brandstetter wrote: > > nit: I think this code could be simplified. Example: > > > > masked.max_bitrate_bps = MinPositive( > > base_bitrate_config_.max_bitrate_bps > > base_bitrate_config_mask_.max_bitrate_bps.value_or(-1)); > > > > Though, if you think this is less readable, what you have is good. > > Didn't know about MinPositive (defined in mediachannel.h). I also considered: > > if (mask_has_max && base_has_max) > std::min(*mask_max, base_max); > else > masked_max.value_or(base_max); > > But I think what I have is most readable. Acknowledged. https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:978: masked.start_bitrate_bps == current.start_bitrate_bps && On 2017/04/20 20:48:00, Zach Stein wrote: > On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > > Same comment as before: shouldn't this be checking the same conditions as in > > SetBitrateConfig, on line 928? In fact, those checks should just be moved > here, > > since having them in both places would be redundant. > > I was avoiding changing the behavior of SetBitrateConfig when > SetBitrateConfigMask is not called since I don't see where it is tested. > The idea here was to avoid calling SetBweBitrates if there is no mask or the > mask sets the same value as was set by SetBitrateConfig (BitrateParameters > enforces 0 <= min <= current <= max for set parameters - I should add that > constraint to BitrateConfigMask). > I will revisit changing SetBitrateConfig. But I believe it does change the behavior of SetBitrateConfig when SetBitrateConfigMask isn't called. Before, calling it with {0, 1000, -1}, then {0, -1, -1} would only call SetBweBitrates once. But now it will call it twice. I think ultimately what you want is to not call SetBweBitrates when nothing's changing. And "start <= 0" means "start's not changing", so it should be included in the boolean expression. Then the boolean expression here becomes the same as the one in SetBitrateConfig, so the one in SetBitrateConfig is redundant and can be removed.
On 2017/04/21 10:14:08, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... > webrtc/call/call.cc:974: } > On 2017/04/20 20:48:00, Zach Stein wrote: > > On 2017/04/19 01:06:53, Taylor Brandstetter wrote: > > > nit: I think this code could be simplified. Example: > > > > > > masked.max_bitrate_bps = MinPositive( > > > base_bitrate_config_.max_bitrate_bps > > > base_bitrate_config_mask_.max_bitrate_bps.value_or(-1)); > > > > > > Though, if you think this is less readable, what you have is good. > > > > Didn't know about MinPositive (defined in mediachannel.h). I also considered: > > > > if (mask_has_max && base_has_max) > > std::min(*mask_max, base_max); > > else > > masked_max.value_or(base_max); > > > > But I think what I have is most readable. > > Acknowledged. > > https://codereview.webrtc.org/2793913008/diff/180001/webrtc/call/call.cc#newc... > webrtc/call/call.cc:978: masked.start_bitrate_bps == current.start_bitrate_bps > && > On 2017/04/20 20:48:00, Zach Stein wrote: > > On 2017/04/19 01:06:54, Taylor Brandstetter wrote: > > > Same comment as before: shouldn't this be checking the same conditions as in > > > SetBitrateConfig, on line 928? In fact, those checks should just be moved > > here, > > > since having them in both places would be redundant. > > > > I was avoiding changing the behavior of SetBitrateConfig when > > SetBitrateConfigMask is not called since I don't see where it is tested. > > The idea here was to avoid calling SetBweBitrates if there is no mask or the > > mask sets the same value as was set by SetBitrateConfig (BitrateParameters > > enforces 0 <= min <= current <= max for set parameters - I should add that > > constraint to BitrateConfigMask). > > I will revisit changing SetBitrateConfig. > > But I believe it does change the behavior of SetBitrateConfig when > SetBitrateConfigMask isn't called. > > Before, calling it with {0, 1000, -1}, then {0, -1, -1} would only call > SetBweBitrates once. But now it will call it twice. > > I think ultimately what you want is to not call SetBweBitrates when nothing's > changing. And "start <= 0" means "start's not changing", so it should be > included in the boolean expression. > > Then the boolean expression here becomes the same as the one in > SetBitrateConfig, so the one in SetBitrateConfig is redundant and can be > removed. SetBweBitrates is only called once in that case because SetBitrateConfig would return before calling UpdateCurrentBitrateConfig. I have another round of changes with better error handling and tests for Call that I will share on Monday :)
On 2017/04/21 23:49:20, Zach Stein wrote: > SetBweBitrates is only called once in that case because SetBitrateConfig would > return before calling UpdateCurrentBitrateConfig. Oh, right. Not sure what I was thinking. Anyway; I agree we shouldn't change the behavior of SetBitrateConfig. But we can still consolidate the "config unchanged" check without changing the behavior. Or, did you mean you wanted to avoid changing the code too much since it's missing test coverage? In that case, I look forward to seeing the tests you're working on
Description was changed from ========== Add PeerConnectionInterface::UpdateCallBitrate. This gives clients more control of the bandwidth estimator. BUG=webrtc:7395 ========== to ========== Add PeerConnectionInterface::UpdateCallBitrate. This gives clients more control of the bandwidth estimator. Subsumed by https://codereview.chromium.org/2838233002/ BUG=webrtc:7395 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
