Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269873002:
Avoid cluster set up at max bitrate at start (Closed)
DescriptionAvoid max bitrate probing when exponential probing in progress
Avoid starting the max probing when there is an exponential probing session in progress.
BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org
Committed: https://crrev.com/9b7b75324f2deb9a17c1d25292123ae1f4021d9a
Cr-Commit-Position: refs/heads/master@{#14268}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Addressed nit #Patch Set 3 : Avoid dependency on another CL and rebase #
Total comments: 5
Patch Set 4 : Rebase and add address nit #Patch Set 5 : Rebased #Patch Set 6 : Rework approach #Patch Set 7 : Fix test #
Total comments: 4
Patch Set 8 : Fix nit #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Avoid cluster set up at max bitrate at start The current code checks if last known bitrate is not 0 as a way to verify that we are in the middle of a call. However, calls are set up with a starting bitrate of 300 kbps and this results in setting up a cluster at max bitrate at the start. Caveat: There is a rare possibility that the actual measured bitrate is same as start bitrate and in such a case the invocation of probing will not occur. BUG= ========== to ========== Avoid cluster set up at max bitrate at start The current code checks if last known bitrate is not 0 as a way to verify that we are in the middle of a call. However, calls are set up with a starting bitrate of 300 kbps and this results in setting up a cluster at max bitrate at the start. Caveat: There is a rare possibility that the actual measured bitrate is same as start bitrate and in such a case the invocation of probing will not occur. BUG= ==========
isheriff@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
I noticed in my testing that there was always probing being set up for max bitrate (20 Mbps in case of remote desktop) and this was due to probing when bitrates are set. The issue is that start is set to 300 kbps and does not represent mid call. Not sure this is the best fix, but this addresses the immediate issue. (Also, probing at max seems like a separate issue that will result in probes at high bitrate for apps that just set it arbitrarily large)
lgtm with one nit https://codereview.webrtc.org/2269873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2269873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/congestion_controller.cc:236: // - we are mid-call, which we consider to be if Nit: remove the last '-' since this is no longer a list.
isheriff@chromium.org changed reviewers: + mflodman@webrtc.org
Looks like stefan went on OOO. Magnus, need LGTM on this one.
https://codereview.webrtc.org/2269873002/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2269873002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/congestion_controller.cc:242: if (last_reported_bitrate_bps_ && != 0 https://codereview.webrtc.org/2269873002/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/congestion_controller.cc:244: static_cast<uint32_t>(start_bitrate_bps_) && Seems a bit arbitrary to not allow probing if the last reported bitrate happens to be equal to the start bitrate? That could happen mid call, right?
https://codereview.chromium.org/2269873002/diff/40001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2269873002/diff/40001/webrtc/modules/congesti... webrtc/modules/congestion_controller/congestion_controller.cc:242: if (last_reported_bitrate_bps_ && On 2016/09/02 13:49:15, stefan-webrtc (holmer) wrote: > != 0 Done. https://codereview.chromium.org/2269873002/diff/40001/webrtc/modules/congesti... webrtc/modules/congestion_controller/congestion_controller.cc:244: static_cast<uint32_t>(start_bitrate_bps_) && On 2016/09/02 13:49:15, stefan-webrtc (holmer) wrote: > Seems a bit arbitrary to not allow probing if the last reported bitrate happens > to be equal to the start bitrate? That could happen mid call, right? Right, I mention this in the commit details. The current way to determine whether we are in mid-call is to check that the |last_reported_bitrate_bps| is not 0. This does not work as intended right now since it starts at |start_bitrate_bps|. This fix is avoiding the unnecessary probing at |max_bitrate_bps| (which happens to be 10 Mbps for remoting and yeti) in the beginning of a session and basically improving over the current state. The real fix should be to determine mid-call correctly (through an explicit flag perhaps).
PTAL
https://codereview.chromium.org/2269873002/diff/40001/webrtc/modules/congesti... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.chromium.org/2269873002/diff/40001/webrtc/modules/congesti... webrtc/modules/congestion_controller/congestion_controller.cc:244: static_cast<uint32_t>(start_bitrate_bps_) && On 2016/09/06 20:32:12, Irfan wrote: > On 2016/09/02 13:49:15, stefan-webrtc (holmer) wrote: > > Seems a bit arbitrary to not allow probing if the last reported bitrate > happens > > to be equal to the start bitrate? That could happen mid call, right? > > Right, I mention this in the commit details. > > The current way to determine whether we are in mid-call is to check that the > |last_reported_bitrate_bps| is not 0. This does not work as intended right now > since it starts at |start_bitrate_bps|. This fix is avoiding the unnecessary > probing at |max_bitrate_bps| (which happens to be 10 Mbps for remoting and yeti) > in the beginning of a session and basically improving over the current state. > > The real fix should be to determine mid-call correctly (through an explicit flag > perhaps). So shouldn't this be triggered if initial_probing_triggered_ is false then? I would really prefer if we did this in a nicer way...
Description was changed from
==========
Avoid cluster set up at max bitrate at start
The current code checks if last known bitrate is not 0 as a way
to verify that we are in the middle of a call. However,
calls are set up with a starting bitrate of 300 kbps and this
results in setting up a cluster at max bitrate at the start.
Caveat: There is a rare possibility that the actual measured bitrate
is same as start bitrate and in such a case the invocation of probing will
not occur.
BUG=
==========
to
==========
Avoid max bitrate probing when exponential probing in progress
Avoid starting the max probing when there is an exponential probing session in
progress.
BUG=webrtc:6332
==========
PTAL
PTAL
lgtm https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... File webrtc/modules/congestion_controller/probe_controller_unittest.cc (right): https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller_unittest.cc:57: clock_.AdvanceTimeMilliseconds(5000); Make 5000 a constant of ProbeController. https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller_unittest.cc:77: clock_.AdvanceTimeMilliseconds(5000); Use the constant here too
https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... File webrtc/modules/congestion_controller/probe_controller_unittest.cc (right): https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller_unittest.cc:57: clock_.AdvanceTimeMilliseconds(5000); On 2016/09/16 07:31:01, stefan-webrtc (holmer) wrote: > Make 5000 a constant of ProbeController. Done. https://codereview.chromium.org/2269873002/diff/120001/webrtc/modules/congest... webrtc/modules/congestion_controller/probe_controller_unittest.cc:77: clock_.AdvanceTimeMilliseconds(5000); On 2016/09/16 07:31:01, stefan-webrtc (holmer) wrote: > Use the constant here too Done.
The CQ bit was checked by isheriff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.chromium.org/2269873002/#ps140001 (title: "Fix nit")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from
==========
Avoid max bitrate probing when exponential probing in progress
Avoid starting the max probing when there is an exponential probing session in
progress.
BUG=webrtc:6332
==========
to
==========
Avoid max bitrate probing when exponential probing in progress
Avoid starting the max probing when there is an exponential probing session in
progress.
BUG=webrtc:6332
R=philipel@webrtc.org, stefan@webrtc.org
Committed:
https://chromium.googlesource.com/external/webrtc/+/9b7b75324f2deb9a17c1d2529...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 9b7b75324f2deb9a17c1d25292123ae1f4021d9a (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Avoid max bitrate probing when exponential probing in progress Avoid starting the max probing when there is an exponential probing session in progress. BUG=webrtc:6332 R=philipel@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9b7b75324f2deb9a17c1d2529... ========== to ========== Avoid max bitrate probing when exponential probing in progress Avoid starting the max probing when there is an exponential probing session in progress. BUG=webrtc:6332 R=philipel@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/9b7b75324f2deb9a17c1d25292123ae1f4021d9a Cr-Commit-Position: refs/heads/master@{#14268} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
