Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417723005:
Prevent BWE rampdowns without new loss reports. (Closed)
|
Created:
5 years, 2 months ago by pbos-webrtc Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrevent BWE rampdowns without new loss reports.
Before this change, UpdateEstimate would repeatedly decrease bitrate
even though there's no fresh corresponding RTCP loss report, triggering
multiple reactions to a single indication of high packet loss.
BUG=webrtc:5101
R=stefan@webrtc.org
Committed: https://crrev.com/b7edb88ae2bb65b3b7ce3be31dd708470baa3575
Cr-Commit-Position: refs/heads/master@{#10374}
Patch Set 1 #
Total comments: 18
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : comments #
Messages
Total messages: 25 (7 generated)
PTAL
The CQ bit was checked by pbos@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/1417723005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417723005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
mflodman@webrtc.org changed reviewers: + mflodman@webrtc.org
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:121: accumulate_expected_packets_ += number_of_packets; Old, but I'd like to change name on this variable, to me accumulate(d) sounds more like number of packets from the start. Can we change to something like since last? https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:188: if (time_last_receiver_block_ms_ != -1) { I'd like to discuss if we should check already here or not. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:212: if (!has_decreased_since_last_fraction_loss_ && You can use 'accumulate_expected_packets_' instead of a new bool.
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:123: // Report loss if the total report is based on sufficiently many packets. Maybe change this comment to explain the if-statement and not what comes after it. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:188: if (time_last_receiver_block_ms_ != -1) { On 2015/10/22 07:06:57, mflodman wrote: > I'd like to discuss if we should check already here or not. Are you talking about whether we should check for a receiver report before we allow any ramp-up to start (current behaviour), or whether we should only allow a ramp-up when receiving a new receiver report? Current behavior is to ramp-up in small steps over a receiver report interval. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc:75: EXPECT_GT(bitrate_bps, kMinBitrateBps); GE
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:188: if (time_last_receiver_block_ms_ != -1) { On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > On 2015/10/22 07:06:57, mflodman wrote: > > I'd like to discuss if we should check already here or not. > > Are you talking about whether we should check for a receiver report before we > allow any ramp-up to start (current behaviour), or whether we should only allow > a ramp-up when receiving a new receiver report? > > Current behavior is to ramp-up in small steps over a receiver report interval. Yes. But also, what happens if we're in an increasing state without REMB negotiated and we stop receiving RR? I see the point when having REMB as a cap and that is the normal state for us, but I think it would be interesting to quickly discuss what we think is the right behavior. I'll admit though it was a while since I dug deep into this code...
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:188: if (time_last_receiver_block_ms_ != -1) { On 2015/10/22 11:22:29, mflodman wrote: > On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > > On 2015/10/22 07:06:57, mflodman wrote: > > > I'd like to discuss if we should check already here or not. > > > > Are you talking about whether we should check for a receiver report before we > > allow any ramp-up to start (current behaviour), or whether we should only > allow > > a ramp-up when receiving a new receiver report? > > > > Current behavior is to ramp-up in small steps over a receiver report interval. > > Yes. But also, what happens if we're in an increasing state without REMB > negotiated and we stop receiving RR? I see the point when having REMB as a cap > and that is the normal state for us, but I think it would be interesting to > quickly discuss what we think is the right behavior. > > I'll admit though it was a while since I dug deep into this code... > > Yes, if we don't have REMB this isn't great. Overall, I think we need some mechanism to handle the case where we don't receive feedback, for instance 0-2 seconds since feedback: continue as normal 2-4 seconds since feedback: no increase 4-10 seconds since feedback: ramp-down towards min bitrate But let's discuss!
feedback
PTAL https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:121: accumulate_expected_packets_ += number_of_packets; On 2015/10/22 07:06:58, mflodman wrote: > Old, but I'd like to change name on this variable, to me accumulate(d) sounds > more like number of packets from the start. Can we change to something like > since last? Done. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:123: // Report loss if the total report is based on sufficiently many packets. On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > Maybe change this comment to explain the if-statement and not what comes after > it. Done. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:188: if (time_last_receiver_block_ms_ != -1) { On 2015/10/22 11:28:06, stefan-webrtc (holmer) wrote: > On 2015/10/22 11:22:29, mflodman wrote: > > On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > > > On 2015/10/22 07:06:57, mflodman wrote: > > > > I'd like to discuss if we should check already here or not. > > > > > > Are you talking about whether we should check for a receiver report before > we > > > allow any ramp-up to start (current behaviour), or whether we should only > > allow > > > a ramp-up when receiving a new receiver report? > > > > > > Current behavior is to ramp-up in small steps over a receiver report > interval. > > > > Yes. But also, what happens if we're in an increasing state without REMB > > negotiated and we stop receiving RR? I see the point when having REMB as a cap > > and that is the normal state for us, but I think it would be interesting to > > quickly discuss what we think is the right behavior. > > > > I'll admit though it was a while since I dug deep into this code... > > > > > > Yes, if we don't have REMB this isn't great. Overall, I think we need some > mechanism to handle the case where we don't receive feedback, for instance > 0-2 seconds since feedback: continue as normal > 2-4 seconds since feedback: no increase > 4-10 seconds since feedback: ramp-down towards min bitrate > > But let's discuss! Sure, I put a TODO for now. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:212: if (!has_decreased_since_last_fraction_loss_ && On 2015/10/22 07:06:57, mflodman wrote: > You can use 'accumulate_expected_packets_' instead of a new bool. Nope, accumulate_expected_packets_ gets reset every time last_fraction_lost_ gets updated, and not after UpdateEstimate succeeds. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc:75: EXPECT_GT(bitrate_bps, kMinBitrateBps); On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > GE GT or the next check doesn't verify anything (we could be hitting min bitrate after the first UpdateEstimate and that's why we're not going down further).
lgtm https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc:75: EXPECT_GT(bitrate_bps, kMinBitrateBps); On 2015/10/22 13:16:06, pbos-webrtc wrote: > On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > > GE > > GT or the next check doesn't verify anything (we could be hitting min bitrate > after the first UpdateEstimate and that's why we're not going down further). I see, good point. Add a comment :) https://codereview.webrtc.org/1417723005/diff/20001/webrtc/modules/bitrate_co... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/20001/webrtc/modules/bitrate_co... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:123: // Don't generate a loss rate until it corresponds to enough packets. nit: Maybe "...based on enough packets"?
comments
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc:75: EXPECT_GT(bitrate_bps, kMinBitrateBps); On 2015/10/22 13:38:04, stefan-webrtc (holmer) wrote: > On 2015/10/22 13:16:06, pbos-webrtc wrote: > > On 2015/10/22 09:20:47, stefan-webrtc (holmer) wrote: > > > GE > > > > GT or the next check doesn't verify anything (we could be hitting min bitrate > > after the first UpdateEstimate and that's why we're not going down further). > > I see, good point. Add a comment :) Done. https://codereview.webrtc.org/1417723005/diff/20001/webrtc/modules/bitrate_co... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/20001/webrtc/modules/bitrate_co... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:123: // Don't generate a loss rate until it corresponds to enough packets. On 2015/10/22 13:38:04, stefan-webrtc (holmer) wrote: > nit: Maybe "...based on enough packets"? Done.
LGTM I think we should continue the discussion pretty soon about how to handle this. https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:121: accumulate_expected_packets_ += number_of_packets; On 2015/10/22 13:16:06, pbos-webrtc wrote: > On 2015/10/22 07:06:58, mflodman wrote: > > Old, but I'd like to change name on this variable, to me accumulate(d) sounds > > more like number of packets from the start. Can we change to something like > > since last? > > Done. Thanks! https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:212: if (!has_decreased_since_last_fraction_loss_ && On 2015/10/22 13:16:05, pbos-webrtc wrote: > On 2015/10/22 07:06:57, mflodman wrote: > > You can use 'accumulate_expected_packets_' instead of a new bool. > > Nope, accumulate_expected_packets_ gets reset every time last_fraction_lost_ > gets updated, and not after UpdateEstimate succeeds. I was unclear in this comment, but we can avoid the bool if we use 'accumulate_expected+packets_' where I suggested in my comment above and reset it after the Update function.
https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1417723005/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:212: if (!has_decreased_since_last_fraction_loss_ && On 2015/10/22 14:34:34, mflodman wrote: > On 2015/10/22 13:16:05, pbos-webrtc wrote: > > On 2015/10/22 07:06:57, mflodman wrote: > > > You can use 'accumulate_expected_packets_' instead of a new bool. > > > > Nope, accumulate_expected_packets_ gets reset every time last_fraction_lost_ > > gets updated, and not after UpdateEstimate succeeds. > > I was unclear in this comment, but we can avoid the bool if we use > 'accumulate_expected+packets_' where I suggested in my comment above and reset > it after the Update function. That sounds like it'd be a bit harder to understand though?
The CQ bit was checked by pbos@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/1417723005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417723005/40001
The CQ bit was unchecked by pbos@webrtc.org
The CQ bit was checked by pbos@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/1417723005/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417723005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417723005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b7edb88ae2bb65b3b7ce3be31dd708470baa3575 Cr-Commit-Position: refs/heads/master@{#10374} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
