Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000063003:
Update the BWE when the network route changes. (Closed)
|
Created:
4 years, 7 months ago by honghaiz3 Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, pbos-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate the BWE when the network route changes.
BUG=
Committed: https://crrev.com/2221e1cd1dd19ae16c87c14bbea92fa62d15154d
Cr-Commit-Position: refs/heads/master@{#13021}
Patch Set 1 : #Patch Set 2 : Add unittests #
Total comments: 8
Patch Set 3 : #
Total comments: 14
Patch Set 4 : #
Total comments: 8
Patch Set 5 : Add TODO to re-create objects. #Messages
Total messages: 32 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org, stefan@webrtc.org
Hi, Can you take a first look at the CL while I am working on the tests?
Patchset #2 (id:100001) has been deleted
Added unittests
lgtm
Stefan, Can you take a look at the CL?
How will this work if we initially connect via TURN, and later switch over to p2p? Do we really want to reset BWE and ramp-up from 300 kbps in that case? https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:215: void CongestionController::FixBitrates(int* bitrate_bps, ClampBitrates https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, Is this called only on the send-side, or both send-side and receive-side? Maybe we should also reset remote_bitrate_estimator_ in case of both sides. https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:250: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, Seems incorrect to me that we don't also reset transport_feedback_adapter_.GetBitrateEstimator(). If we reset BWE we should reset both the delay based and the loss based.
On 2016/05/27 21:46:16, stefan-webrtc (holmer) wrote: > How will this work if we initially connect via TURN, and later switch over to > p2p? Do we really want to reset BWE and ramp-up from 300 kbps in that case? > I don't think a switch between TURN and not will trigger this. I think only network interface changes will. > https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... > File webrtc/modules/congestion_controller/congestion_controller.cc (right): > > https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... > webrtc/modules/congestion_controller/congestion_controller.cc:215: void > CongestionController::FixBitrates(int* bitrate_bps, > ClampBitrates > > https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... > webrtc/modules/congestion_controller/congestion_controller.cc:246: void > CongestionController::ResetBweBitrates(int bitrate_bps, > Is this called only on the send-side, or both send-side and receive-side? Maybe > we should also reset remote_bitrate_estimator_ in case of both sides. It's on both sides. But does it matter when we're using send-side BWE? > > https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... > webrtc/modules/congestion_controller/congestion_controller.cc:250: > bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, > Seems incorrect to me that we don't also reset > transport_feedback_adapter_.GetBitrateEstimator(). If we reset BWE we should > reset both the delay based and the loss based.
PTAL. Thanks! https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:215: void CongestionController::FixBitrates(int* bitrate_bps, On 2016/05/27 21:46:15, stefan-webrtc (holmer) wrote: > ClampBitrates Done. https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, On 2016/05/27 21:46:15, stefan-webrtc (holmer) wrote: > Is this called only on the send-side, or both send-side and receive-side? Maybe > we should also reset remote_bitrate_estimator_ in case of both sides. Are we using the send-side BWE? Do we still need the receive-side one? Is it just resetting the min bitrate of the remb? Added that if yes. https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:250: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, On 2016/05/27 21:46:15, stefan-webrtc (holmer) wrote: > Seems incorrect to me that we don't also reset > transport_feedback_adapter_.GetBitrateEstimator(). If we reset BWE we should > reset both the delay based and the loss based. Done.
https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, On 2016/05/27 23:20:44, honghaiz3 wrote: > On 2016/05/27 21:46:15, stefan-webrtc (holmer) wrote: > > Is this called only on the send-side, or both send-side and receive-side? > Maybe > > we should also reset remote_bitrate_estimator_ in case of both sides. > Are we using the send-side BWE? Do we still need the receive-side one? > Is it just resetting the min bitrate of the remb? Added that if yes. Receive-side BWE may still be negotiated instead of send-side, at least for the foreseeable future. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:215: void CongestionController::ClampBitrates(int* bitrate_bps, Seems like this can be a static function within this file only as it doesn't access any members of the class. Move it to the top of the file too. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, ResetBitrates() or maybe ResetBwe() to make it clear that not only bitrates are reset, also the BWE state. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:253: if (remote_bitrate_estimator_) { Remove {} https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:259: transport_feedback_adapter_.SetBitrateEstimator(rbe); Will the previous rbe be deleted? https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:261: Could you add a TODO which says "TODO(holmer): Trigger a new probe once mid-call probing is implemented."? https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller_unittest.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller_unittest.cc:138: controller_->ResetBweBitrates(-1, -1, -1); Is this a good behavior? Shouldn't we instead let it reset to the previous bitrate? So essentially this call would be a no-op except for recreating BWE objects.
PTAL. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:215: void CongestionController::ClampBitrates(int* bitrate_bps, On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > Seems like this can be a static function within this file only as it doesn't > access any members of the class. Move it to the top of the file too. Done. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > ResetBitrates() or maybe ResetBwe() to make it clear that not only bitrates are > reset, also the BWE state. Based on your comments, will it be clearer to use ResetBweAndBitrates? https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:253: if (remote_bitrate_estimator_) { On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > Remove {} Done. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:259: transport_feedback_adapter_.SetBitrateEstimator(rbe); On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > Will the previous rbe be deleted? Yes, the bitrate_estimator_ is a unique_ptr. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:261: On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > Could you add a TODO which says "TODO(holmer): Trigger a new probe once mid-call > probing is implemented."? Done. https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller_unittest.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller_unittest.cc:138: controller_->ResetBweBitrates(-1, -1, -1); On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > Is this a good behavior? Shouldn't we instead let it reset to the previous > bitrate? So essentially this call would be a no-op except for recreating BWE > objects. What do you mean by "previous bitrate"? If you mean the bitrate right before we make the call, it then would be a no-op. If you mean the bitrate before the last round of BWE, then we need to remember the history. Plus the concept of "last round of BWE" is not clear to me. What I attempt to do is to mimic the behavior of SetBweBitrates(-1, -1, -1) but after creating a set of BWE objects. So it would be the same as the state when the system starts with SetBweBitrates(-1, -1, -1). WDYT?
https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, On 2016/05/31 16:58:03, honghaiz3 wrote: > On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > > ResetBitrates() or maybe ResetBwe() to make it clear that not only bitrates > are > > reset, also the BWE state. > > Based on your comments, will it be clearer to use ResetBweAndBitrates? That works https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller_unittest.cc (right): https://codereview.webrtc.org/2000063003/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller_unittest.cc:138: controller_->ResetBweBitrates(-1, -1, -1); On 2016/05/31 16:58:03, honghaiz3 wrote: > On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > > Is this a good behavior? Shouldn't we instead let it reset to the previous > > bitrate? So essentially this call would be a no-op except for recreating BWE > > objects. > > What do you mean by "previous bitrate"? > If you mean the bitrate right before we make the call, it then would be a no-op. > > If you mean the bitrate before the last round of BWE, then we need to remember > the history. > Plus the concept of "last round of BWE" is not clear to me. > > What I attempt to do is to mimic the behavior of SetBweBitrates(-1, -1, -1) but > after creating a set > of BWE objects. So it would be the same as the state when the system starts with > SetBweBitrates(-1, -1, -1). > WDYT? I see. I was thinking that if we want to reset only the max and min, then it would be good to be able to pass in -1 as start bitrate. But I guess that is what it already does, I may have just misunderstood. https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:251: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, I wonder if we instead should simply recreate the bitrate controller object to avoid having a new method. WDYT? Sorry for not bringing this up earlier. https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:255: remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); Here too, shouldn't we recreate this object as well?
https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:251: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > I wonder if we instead should simply recreate the bitrate controller object to > avoid having a new method. WDYT? Sorry for not bringing this up earlier. No we cannot because some of the objects in congestion controller is used by other objects. For example, the pacer is used by AudioSendStream (and probably videosendstream too). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Plus, the pacer (an object in CongestionController) probably should not be replaced because it contains the pending packet queue. https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:255: remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > Here too, shouldn't we recreate this object as well? Cannot for the same reason. It was used by the videoreceivestream. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:251: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, On 2016/06/01 02:17:34, honghaiz3 wrote: > On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > > I wonder if we instead should simply recreate the bitrate controller object to > > avoid having a new method. WDYT? Sorry for not bringing this up earlier. > > No we cannot because some of the objects in congestion controller is used by > other objects. > For example, the pacer is used by AudioSendStream (and probably videosendstream > too). > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > Plus, the pacer (an object in CongestionController) probably should not be > replaced because it contains the pending packet queue. I was referring to bitrate_controller_ and not congestion controller. I think it should be possible to do if we remove the GetBitrateController() method. Feel free to add a TODO instead if it turns out to be too much work. https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:255: remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); On 2016/06/01 02:17:34, honghaiz3 wrote: > On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > > Here too, shouldn't we recreate this object as well? > > Cannot for the same reason. > It was used by the videoreceivestream. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... I see, could you add a TODO which says "Recreate this object once the remote bitrate estimator no longer is exposed outside CongestionController."
PTAL. https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:246: void CongestionController::ResetBweBitrates(int bitrate_bps, On 2016/05/30 06:47:03, stefan-webrtc (holmer) wrote: > On 2016/05/27 23:20:44, honghaiz3 wrote: > > On 2016/05/27 21:46:15, stefan-webrtc (holmer) wrote: > > > Is this called only on the send-side, or both send-side and receive-side? > > Maybe > > > we should also reset remote_bitrate_estimator_ in case of both sides. > > Are we using the send-side BWE? Do we still need the receive-side one? > > Is it just resetting the min bitrate of the remb? Added that if yes. > > Receive-side BWE may still be negotiated instead of send-side, at least for the > foreseeable future. Acknowledged. https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:251: bitrate_controller_->ResetBitrates(bitrate_bps, min_bitrate_bps, On 2016/06/01 07:38:36, stefan-webrtc (holmer) wrote: > On 2016/06/01 02:17:34, honghaiz3 wrote: > > On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > > > I wonder if we instead should simply recreate the bitrate controller object > to > > > avoid having a new method. WDYT? Sorry for not bringing this up earlier. > > > > No we cannot because some of the objects in congestion controller is used by > > other objects. > > For example, the pacer is used by AudioSendStream (and probably > videosendstream > > too). > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Plus, the pacer (an object in CongestionController) probably should not be > > replaced because it contains the pending packet queue. > > I was referring to bitrate_controller_ and not congestion controller. I think it > should be possible to do if we remove the GetBitrateController() method. Feel > free to add a TODO instead if it turns out to be too much work. Ah sorry that I mis-read it. It had similar issues because the pointer was used by VideoSendStream here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... There the created bandwidth observer contains a pointer to the bitrate_controller_. Add a TODO https://codereview.webrtc.org/2000063003/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:255: remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps); On 2016/06/01 07:38:36, stefan-webrtc (holmer) wrote: > On 2016/06/01 02:17:34, honghaiz3 wrote: > > On 2016/05/31 17:27:49, stefan-webrtc (holmer) wrote: > > > Here too, shouldn't we recreate this object as well? > > > > Cannot for the same reason. > > It was used by the videoreceivestream. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > I see, could you add a TODO which says "Recreate this object once the remote > bitrate estimator no longer is exposed outside CongestionController." Done.
lgtm
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000063003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000063003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2000063003/#ps180001 (title: "Add TODO to re-create objects. ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000063003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000063003/180001
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Update the BWE when the network route changes. BUG= ========== to ========== Update the BWE when the network route changes. BUG= Committed: https://crrev.com/2221e1cd1dd19ae16c87c14bbea92fa62d15154d Cr-Commit-Position: refs/heads/master@{#13021} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2221e1cd1dd19ae16c87c14bbea92fa62d15154d Cr-Commit-Position: refs/heads/master@{#13021}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:180001) has been created in https://codereview.webrtc.org/2030283002/ by guidou@webrtc.org. The reason for reverting is: Speculative revert due to failure in Mac Tester FYI bot. See https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds... Will reland if the revert doesn't fix the bot..
Message was sent while issue was closed.
On 2016/06/03 06:24:36, guidou wrote: > A revert of this CL (patchset #5 id:180001) has been created in > https://codereview.webrtc.org/2030283002/ by mailto:guidou@webrtc.org. > > The reason for reverting is: Speculative revert due to failure in Mac Tester FYI > bot. See > https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds... > > Will reland if the revert doesn't fix the bot.. The revert worked. Please find out why it broke the bot before relanding. |
