Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888303005:
Add PeerConnectionInterface::UpdateCallBitrate. (Closed)
|
Created:
3 years, 7 months ago by Zach Stein Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, the sun, tterriberry_mozilla.com, mflodman, kwiberg-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRelanding: Adds PeerConnectionInterface::UpdateCallBitrate to give clients more control of the bandwidth estimator. PeerConnection implements this method by passing a BitrateConfigMask to its associated Call, which is combined with the existing BitrateConfig and passed on to the SendSideCongestionController as necessary. The existing BitrateConfig generally comes from the x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Original-Commit-Position: refs/heads/master@{#18417}
Committed: https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b32391996607afa80
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18421}
Committed: https://chromium.googlesource.com/external/webrtc/+/4b9798024f3a940c0c6cd6327c086bf987485a5f
Patch Set 1 #Patch Set 2 : Add a missing test from Taylor's last CR. #
Total comments: 8
Patch Set 3 : update comments as suggested in review #
Total comments: 12
Patch Set 4 : Clarify some comments as suggested by Stefan. #Patch Set 5 : Duplicate MinPositive in call to avoid dependency on cricket namespace. #Patch Set 6 : Add closing namespace comment and format. #Patch Set 7 : Don't update the start_bitrate cached in config_.bitrate_config if the start_bitrate isn't changed.… #Patch Set 8 : Implement SetBitrate in PeerConnectionInterface to avoid breaking chromium mock. #
Total comments: 7
Messages
Total messages: 45 (19 generated)
zstein@webrtc.org changed reviewers: + deadbeef@webrtc.org
zstein@webrtc.org changed reviewers: + stefan@webrtc.org
PTAL
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
Some questions about the API. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:732: // 0 <= min <= current <= max should hold for set parameters. Are all 8 combinations of set/unset supported? https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: // Assumes that 0 <= min <= start <= max holds for set parameters. Would it make sense to document how they are combined? Also, this comment suggests that the name is bad. Do you expect it to become good due to future changes in how the two are combined, or should it be changed?
https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:732: // 0 <= min <= current <= max should hold for set parameters. On 2017/05/24 08:27:55, kwiberg-webrtc wrote: > Are all 8 combinations of set/unset supported? Yes. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.h#newcode82 webrtc/call/call.h:82: // Assumes that 0 <= min <= start <= max holds for set parameters. On 2017/05/24 08:27:55, kwiberg-webrtc wrote: > Would it make sense to document how they are combined? > > Also, this comment suggests that the name is bad. Do you expect it to become > good due to future changes in how the two are combined, or should it be changed? The combination is described in SetBirateConfig/SetBitrateConfigMask - I will add a reference to those methods here. The name is a holdover from a previous iteration where the values in the mask were preferred over the values in bitrate_config if they were set. So I agree that the current name isn't great, but I can't think of a better one. Any suggestions?
lgtm. CL subject/description should be updated before landing though. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:743: // Changing |current_bitrate_bps| to a new value will reset the current This comment may need to be updated. It's not just changing to a new value that will reset the current estimate, passing in *any* value will do this. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:855: // value. In practice, this value comes from the x-google-start-bitrate-codec nit: Remove hyphen between "bitrate" and "codec"
zstein@webrtc.org changed reviewers: + solenberg@webrtc.org
Patch 3 is also a rebase. Needs review by an owner of call please. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:743: // Changing |current_bitrate_bps| to a new value will reset the current On 2017/05/25 15:33:52, Taylor Brandstetter wrote: > This comment may need to be updated. It's not just changing to a new value that > will reset the current estimate, passing in *any* value will do this. Done. https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:855: // value. In practice, this value comes from the x-google-start-bitrate-codec On 2017/05/25 15:33:52, Taylor Brandstetter wrote: > nit: Remove hyphen between "bitrate" and "codec" Done.
holmer@google.com changed reviewers: + holmer@google.com
Thanks for doing this! The CL description needs to be updated. https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:969: // If these's nothing to update (min/max unchanged, no new bandwidth there's https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( I haven't seen us use the cricket namespace here before. Should it be moved to the rtc namespace and be put under webrtc/base? https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h#newcod... webrtc/call/call.h:158: // Specifying a start bitrate (>0) will reset the current bitrate estimate. I think it would be good to specify what start bitrate == 0 does too.
Description was changed from
==========
Add PeerConnectionInterface::UpdateCallBitrate.
BUG=webrtc:7395
==========
to
==========
Adds PeerConnectionInterface::UpdateCallBitrate to give clients more control of
the bandwidth estimator. PeerConnection implements this method by passing a
BitrateConfigMask to its associated Call, which is combined with the existing
BitrateConfig and passed on to the SendSideCongestionController as necessary.
The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
==========
Thanks for the feedback. https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:969: // If these's nothing to update (min/max unchanged, no new bandwidth On 2017/05/31 07:46:33, holmer wrote: > there's Done. https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/05/31 07:46:34, holmer wrote: > I haven't seen us use the cricket namespace here before. Should it be moved to > the rtc namespace and be put under webrtc/base? MinPositive is currently defined in mediachannel.h and used by webrtcvideonengine and webrtcvideoengine2 (which are in the cricket namespace). webrtc/base/mathutils.h looks most suitable to me, but that file doesn't currently use a namespace... Mind if I consider this in a later CL? Seems out of scope here (unless you see something natural to do). https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h#newcod... webrtc/call/call.h:158: // Specifying a start bitrate (>0) will reset the current bitrate estimate. On 2017/05/31 07:46:34, holmer wrote: > I think it would be good to specify what start bitrate == 0 does too. start_bitrate == 0 is disallowed by DCHECK, but it will update the value to whatever start gets clamped to. Negative values other than -1 behave similarly. I don't think adding to this comment provides much value since this is supposed to be an internal header now - reading the code is just as clear and can't get out of sync. Or maybe I have just been looking at this code for too long :)
https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/06/01 03:37:49, Zach Stein wrote: > On 2017/05/31 07:46:34, holmer wrote: > > I haven't seen us use the cricket namespace here before. Should it be moved to > > the rtc namespace and be put under webrtc/base? > > MinPositive is currently defined in mediachannel.h and used by > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket namespace). > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > currently use a namespace... Mind if I consider this in a later CL? Seems out of > scope here (unless you see something natural to do). Can either of you explain what MinPositive is supposed to do? The definition looks like this: template <typename T> static T MinPositive(T a, T b) { if (a <= 0) { return b; } if (b <= 0) { return a; } return std::min(a, b); } IOW, if both are positive, it returns the smaller one; if one is negative and one is positive, it returns the positive one (that is, the greater one); and if both are negative, it returns the second one regardless of which number is greater. This behavior seems really complex, and I can't think of a situation where it would be useful.
https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/06/01 07:50:25, kwiberg-webrtc wrote: > On 2017/06/01 03:37:49, Zach Stein wrote: > > On 2017/05/31 07:46:34, holmer wrote: > > > I haven't seen us use the cricket namespace here before. Should it be moved > to > > > the rtc namespace and be put under webrtc/base? > > > > MinPositive is currently defined in mediachannel.h and used by > > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket > namespace). > > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > > currently use a namespace... Mind if I consider this in a later CL? Seems out > of > > scope here (unless you see something natural to do). > > Can either of you explain what MinPositive is supposed to do? The definition > looks like this: > > template <typename T> > static T MinPositive(T a, T b) { > if (a <= 0) { > return b; > } > if (b <= 0) { > return a; > } > return std::min(a, b); > } > > IOW, if both are positive, it returns the smaller one; if one is negative and > one is positive, it returns the positive one (that is, the greater one); and if > both are negative, it returns the second one regardless of which number is > greater. > > This behavior seems really complex, and I can't think of a situation where it > would be useful. It's useful in situations where you're taking the minimum of two maximums, and "-1" means "no maximum set". Which does occur here and some other places. If we changed these maximums to use "rtc::Optional", MinPositive could become "MinSet" or "MinNonNull" and then maybe it would make more sense. https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.h#newcod... webrtc/call/call.h:158: // Specifying a start bitrate (>0) will reset the current bitrate estimate. On 2017/06/01 03:37:49, Zach Stein wrote: > On 2017/05/31 07:46:34, holmer wrote: > > I think it would be good to specify what start bitrate == 0 does too. > > start_bitrate == 0 is disallowed by DCHECK, but it will update the value to > whatever start gets clamped to. Negative values other than -1 behave similarly. > I don't think adding to this comment provides much value since this is supposed > to be an internal header now - reading the code is just as clear and can't get > out of sync. Or maybe I have just been looking at this code for too long :) The value of a comment here is that it tells you what the method is *designed* to do, whereas reading the code only tells you what it actually does. In my opinion, it's ideal if every method in a header file has a sufficient level of comments that its behavior is clear and tests can be written for it without looking at the implementation.
https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/06/01 03:37:49, Zach Stein wrote: > On 2017/05/31 07:46:34, holmer wrote: > > I haven't seen us use the cricket namespace here before. Should it be moved to > > the rtc namespace and be put under webrtc/base? > > MinPositive is currently defined in mediachannel.h and used by > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket namespace). > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > currently use a namespace... Mind if I consider this in a later CL? Seems out of > scope here (unless you see something natural to do). I'd prefer not using the cricket namespace here, so in that case I'd rather land this with code duplication until we have moved it to a different namespace.
https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/06/01 16:32:47, stefan-webrtc wrote: > On 2017/06/01 03:37:49, Zach Stein wrote: > > On 2017/05/31 07:46:34, holmer wrote: > > > I haven't seen us use the cricket namespace here before. Should it be moved > to > > > the rtc namespace and be put under webrtc/base? > > > > MinPositive is currently defined in mediachannel.h and used by > > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket > namespace). > > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > > currently use a namespace... Mind if I consider this in a later CL? Seems out > of > > scope here (unless you see something natural to do). > > I'd prefer not using the cricket namespace here, so in that case I'd rather land > this with code duplication until we have moved it to a different namespace. Peter pointed out that this could also be moved to the webrtc namespace in call.h. Does that make sense to you (for a future CL) or do you think somewhere in webrtc/base is better?
On 2017/06/01 20:12:44, Zach Stein wrote: > https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... > webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( > On 2017/06/01 16:32:47, stefan-webrtc wrote: > > On 2017/06/01 03:37:49, Zach Stein wrote: > > > On 2017/05/31 07:46:34, holmer wrote: > > > > I haven't seen us use the cricket namespace here before. Should it be > moved > > to > > > > the rtc namespace and be put under webrtc/base? > > > > > > MinPositive is currently defined in mediachannel.h and used by > > > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket > > namespace). > > > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > > > currently use a namespace... Mind if I consider this in a later CL? Seems > out > > of > > > scope here (unless you see something natural to do). > > > > I'd prefer not using the cricket namespace here, so in that case I'd rather > land > > this with code duplication until we have moved it to a different namespace. > > Peter pointed out that this could also be moved to the webrtc namespace in > call.h. Does that make sense to you (for a future CL) or do you think somewhere > in webrtc/base is better? That sounds like an ok option to me to start with.
LGTM provided that we follow up and move MinPositive to the webrtc or rtc namespace.
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2888303005/#ps100001 (title: "Add closing namespace comment and format.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/26611)
Could you take a quick look at the latest patch Taylor? It fixes VideoSendStreamTest.ChangingNetworkRoute, which was failing with: # Fatal error in ../../webrtc/call/call.cc, line 1102 # last system error: 1 # Check failed: config_.bitrate_config.start_bitrate_bps > 0 (-1 vs. 0)
https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:981: updated.start_bitrate_bps = cricket::MinPositive( On 2017/06/01 16:24:43, Taylor Brandstetter wrote: > On 2017/06/01 07:50:25, kwiberg-webrtc wrote: > > On 2017/06/01 03:37:49, Zach Stein wrote: > > > On 2017/05/31 07:46:34, holmer wrote: > > > > I haven't seen us use the cricket namespace here before. Should it be > moved > > to > > > > the rtc namespace and be put under webrtc/base? > > > > > > MinPositive is currently defined in mediachannel.h and used by > > > webrtcvideonengine and webrtcvideoengine2 (which are in the cricket > > namespace). > > > webrtc/base/mathutils.h looks most suitable to me, but that file doesn't > > > currently use a namespace... Mind if I consider this in a later CL? Seems > out > > of > > > scope here (unless you see something natural to do). > > > > Can either of you explain what MinPositive is supposed to do? The definition > > looks like this: > > > > template <typename T> > > static T MinPositive(T a, T b) { > > if (a <= 0) { > > return b; > > } > > if (b <= 0) { > > return a; > > } > > return std::min(a, b); > > } > > > > IOW, if both are positive, it returns the smaller one; if one is negative and > > one is positive, it returns the positive one (that is, the greater one); and > if > > both are negative, it returns the second one regardless of which number is > > greater. > > > > This behavior seems really complex, and I can't think of a situation where it > > would be useful. > > It's useful in situations where you're taking the minimum of two maximums, and > "-1" means "no maximum set". Which does occur here and some other places. > > If we changed these maximums to use "rtc::Optional", MinPositive could become > "MinSet" or "MinNonNull" and then maybe it would make more sense. Ah, OK. Negative values are "not set", and if given two negative values we don't care which one we return, because they all just mean "not set". I agree that Optional would make such logic easier to follow...
lgtm
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2888303005/#ps120001 (title: "Don't update the start_bitrate cached in config_.bitrate_config if the start_bitrate isn't changed.…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496426235480600,
"parent_rev": "ed3b986d63294d732cabe689e049b6e76d312980", "commit_rev":
"9641c133277da0d04b78782b32391996607afa80"}
Message was sent while issue was closed.
Description was changed from
==========
Adds PeerConnectionInterface::UpdateCallBitrate to give clients more control of
the bandwidth estimator. PeerConnection implements this method by passing a
BitrateConfigMask to its associated Call, which is combined with the existing
BitrateConfig and passed on to the SendSideCongestionController as necessary.
The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
==========
to
==========
Adds PeerConnectionInterface::UpdateCallBitrate to give clients more control of
the bandwidth estimator. PeerConnection implements this method by passing a
BitrateConfigMask to its associated Call, which is combined with the existing
BitrateConfig and passed on to the SendSideCongestionController as necessary.
The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18417}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.webrtc.org/2914413002/ by charujain@webrtc.org. The reason for reverting is: Broken downstream project..
Message was sent while issue was closed.
Description was changed from
==========
Adds PeerConnectionInterface::UpdateCallBitrate to give clients more control of
the bandwidth estimator. PeerConnection implements this method by passing a
BitrateConfigMask to its associated Call, which is combined with the existing
BitrateConfig and passed on to the SendSideCongestionController as necessary.
The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18417}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
==========
to
==========
Relanding: Adds PeerConnectionInterface::UpdateCallBitrate to give clients more
control of the bandwidth estimator. PeerConnection implements this method by
passing a BitrateConfigMask to its associated Call, which is combined with the
existing BitrateConfig and passed on to the SendSideCongestionController as
necessary. The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18417}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
==========
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2888303005/#ps140001 (title: "Implement SetBitrate in PeerConnectionInterface to avoid breaking chromium mock.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496437679469850,
"parent_rev": "441718ef69db9fef17ee85c197fa5e17a29df55a", "commit_rev":
"4b9798024f3a940c0c6cd6327c086bf987485a5f"}
Message was sent while issue was closed.
Description was changed from
==========
Relanding: Adds PeerConnectionInterface::UpdateCallBitrate to give clients more
control of the bandwidth estimator. PeerConnection implements this method by
passing a BitrateConfigMask to its associated Call, which is combined with the
existing BitrateConfig and passed on to the SendSideCongestionController as
necessary. The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18417}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
==========
to
==========
Relanding: Adds PeerConnectionInterface::UpdateCallBitrate to give clients more
control of the bandwidth estimator. PeerConnection implements this method by
passing a BitrateConfigMask to its associated Call, which is combined with the
existing BitrateConfig and passed on to the SendSideCongestionController as
necessary. The existing BitrateConfig generally comes from the
x-google-{min,start,max}-bitrate params in the SDP.
BUG=webrtc:7395
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Original-Commit-Position: refs/heads/master@{#18417}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9641c133277da0d04b78782b3...
Review-Url: https://codereview.webrtc.org/2888303005
Cr-Commit-Position: refs/heads/master@{#18421}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4b9798024f3a940c0c6cd6327...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/4b9798024f3a940c0c6cd6327...
Message was sent while issue was closed.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Message was sent while issue was closed.
drive-by when looking at crbug/755971 https://codereview.webrtc.org/2888303005/diff/140001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/140001/webrtc/call/call.cc#newc... webrtc/call/call.cc:956: return b; Here, |b| is not guaranteed to be positive. would this be correct? return a < 0 ? (b < 0 ? 0 : b) : (b < 0 ? 0 : std::min(a, b)); https://codereview.webrtc.org/2888303005/diff/140001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2888303005/diff/140001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:593: // TODO(zstein): not implemented nit: the TODO should document what needs to be done https://codereview.webrtc.org/2888303005/diff/140001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc (right): https://codereview.webrtc.org/2888303005/diff/140001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1245: RTCError PeerConnection::SetBitrate(const BitrateParameters& bitrate) { Instead of blocking, can this function be designed to be async? (is the return value necessary?) https://codereview.webrtc.org/2888303005/diff/140001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1256: LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, hiding return statements in macros can be dangerous. It would be good to clean that up. https://codereview.webrtc.org/2888303005/diff/140001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1277: LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, The code cost (i.e. code bloat) of each of these log statements, is actually substantial. Consider having a single log statement and just a const char* variable for the text you want to log. https://codereview.webrtc.org/2888303005/diff/140001/webrtc/pc/peerconnection... webrtc/pc/peerconnection.cc:1287: RTC_DCHECK(call_.get()); we don't DCHECK a pointer that we dereference anyway. However, if this must always be valid at the start of the function call, you could move the DCHECK there. That way you check this condition first regardless of whether you end up dereferencing the pointer or not. Could be useful since there are so many potential early return paths.
Message was sent while issue was closed.
https://codereview.webrtc.org/2888303005/diff/140001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2888303005/diff/140001/webrtc/call/call.cc#newc... webrtc/call/call.cc:956: return b; On 2017/08/17 07:32:52, tommi wrote: > Here, |b| is not guaranteed to be positive. > > would this be correct? > > return a < 0 ? (b < 0 ? 0 : b) : (b < 0 ? 0 : std::min(a, b)); Hmm. Your suggestion is the same as std::max(0, std::min(a, b)) right? Whereas the current code is equivalent to a <= 0 ? b : std::min(a, b) I guess either way, this function should be equipped with a comment that explains what it does. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
