Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613543003:
Fix BitrateProber to match the requested bitrate more precisely (Closed)
|
Created:
3 years, 11 months ago by Sergey Ulanov Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix BitrateProber to match the requested bitrate more precisely
Previously BirateProber was calculating delay between probes based on
the size of the previous probe. Because of that the actual sent bitrate
can deviate greatly from the target value. With this change it uses
total number of bytes in the cluster to estimate delay before each
probe.
BUG=webrtc:6952
Review-Url: https://codereview.webrtc.org/2613543003
Cr-Original-Commit-Position: refs/heads/master@{#15971}
Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c7e6ab930e7adbd5
Review-Url: https://codereview.webrtc.org/2613543003
Cr-Commit-Position: refs/heads/master@{#16127}
Committed: https://chromium.googlesource.com/external/webrtc/+/6dbbd89f1876df167cbd4c6eb8b4ade488de1fc1
Patch Set 1 #
Total comments: 7
Patch Set 2 : sync #
Total comments: 2
Patch Set 3 : . #Patch Set 4 : fix test #Patch Set 5 : Limit number of retries #
Messages
Total messages: 36 (21 generated)
Description was changed from ========== Fix BitrateProber to match the requested bitrate. Previously BirateProber was estimating delay between probes based on size of the previous probe. Because of that the actual probe bitrate can deviate greatly from the requested value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=6952 ========== to ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=6952 ==========
sergeyu@chromium.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... File webrtc/modules/pacing/bitrate_prober.cc (left): https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:115: if (elapsed_time_ms > kInactivityThresholdMs) { I couldn't see any case when this condition would be useful. As far as I can tell probes would be suspended below by kMaxProbeDelayMs before reaching 5000 ms here. Am I missing something? https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:131: probing_state_ = ProbingState::kSuspended; This logic seems to be incorrect. It could suspend probing in a middle of a cluster and leave that cluster in that state. The remaining part of that cluster would be restarted when probing is requested again, which makes little sense. I changed this code to call ResetState() when probe is delayed by more than 3 ms.
Description was changed from ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=6952 ========== to ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 ==========
lg, this makes BitrateProber both cleaner and more correct. But I realized I should have CCd you on this CL: https://codereview.chromium.org/2609113003/ I also added a short description of what I plan to do in the bug report (webrtc:6822) so you know whats up. https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:141: if (clusters_.empty()) if (clusters_.empty()) { probing_state_ = ProbingState::kSuspended; next_probe_time_ms_ = -1; } https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:151: return cluster.time_started_ms + Not super important, but can you break this calculation into maybe two steps? I think that would be cleaner. Also, I can't see why you need to cast to int?
I like what you did in your CL. I'll merge this change with trunk. https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:141: if (clusters_.empty()) On 2017/01/04 10:13:18, philipel wrote: > if (clusters_.empty()) { > probing_state_ = ProbingState::kSuspended; > next_probe_time_ms_ = -1; > } next_probe_time_ms_ is only used in kActive state and it's reset when probing is activated, so resetting it here would not make any difference https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:151: return cluster.time_started_ms + On 2017/01/04 10:13:18, philipel wrote: > Not super important, but can you break this calculation into maybe two steps? I > think that would be cleaner. Will do. > > Also, I can't see why you need to cast to int? yes, the cast is redundant
Updated, PTAL
https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/2613543003/diff/1/webrtc/modules/pacing/bitrate... webrtc/modules/pacing/bitrate_prober.cc:141: if (clusters_.empty()) On 2017/01/05 23:31:53, Sergey Ulanov wrote: > On 2017/01/04 10:13:18, philipel wrote: > > if (clusters_.empty()) { > > probing_state_ = ProbingState::kSuspended; > > next_probe_time_ms_ = -1; > > } > > next_probe_time_ms_ is only used in kActive state and it's reset when probing is > activated, so resetting it here would not make any difference Acknowledged. https://codereview.webrtc.org/2613543003/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (left): https://codereview.webrtc.org/2613543003/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:169: cluster->sent_bytes += static_cast<int>(bytes); This cast is needed for the win bots :). I realize I should have added a comment about it.
lgtm
The CQ bit was checked by philipel@webrtc.org
The CQ bit was unchecked by philipel@webrtc.org
The CQ bit was unchecked by philipel@webrtc.org
https://codereview.webrtc.org/2613543003/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (left): https://codereview.webrtc.org/2613543003/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:169: cluster->sent_bytes += static_cast<int>(bytes); On 2017/01/09 09:34:34, philipel wrote: > This cast is needed for the win bots :). I realize I should have added a comment > about it. Done.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2613543003/#ps40001 (title: ".")
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_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21485)
The CQ bit was checked by sergeyu@chromium.org
The CQ bit was unchecked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2613543003/#ps60001 (title: "fix test")
The CQ bit was checked by sergeyu@chromium.org
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": 60001, "attempt_start_ts": 1483991476994950,
"parent_rev": "06195757ac5761ef728af512eaeba261cbf0a349", "commit_rev":
"599c5011e7569a269f5521a8c7e6ab930e7adbd5"}
Message was sent while issue was closed.
Description was changed from ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 ========== to ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2613543003 Cr-Commit-Position: refs/heads/master@{#15971} Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2626473004/ by sprang@webrtc.org. The reason for reverting is: Speculative revert. Linux memcheck bot started failing a lot at the time of this cl. Doesn't look related at first glance, but we don't have another lead yet..
Message was sent while issue was closed.
Description was changed from ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2613543003 Cr-Commit-Position: refs/heads/master@{#15971} Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c... ========== to ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2613543003 Cr-Commit-Position: refs/heads/master@{#15971} Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c... ==========
Patchset #5 (id:80001) has been deleted
The problem with valgrind bots is that the test executed very slowly and as result probes repeatedly timeout. As result prober state is reset after every probe (see ResetState()). In the latest patchset limited number of retries for each probing cluster to workaround this problem.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2613543003/#ps100001 (title: "Limit number of retries")
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": 100001, "attempt_start_ts": 1484690722655370,
"parent_rev": "f7303fc435e565c9fa1c6a0cb5c5361be42873fe", "commit_rev":
"6dbbd89f1876df167cbd4c6eb8b4ade488de1fc1"}
Message was sent while issue was closed.
Description was changed from ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2613543003 Cr-Commit-Position: refs/heads/master@{#15971} Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c... ========== to ========== Fix BitrateProber to match the requested bitrate more precisely Previously BirateProber was calculating delay between probes based on the size of the previous probe. Because of that the actual sent bitrate can deviate greatly from the target value. With this change it uses total number of bytes in the cluster to estimate delay before each probe. BUG=webrtc:6952 Review-Url: https://codereview.webrtc.org/2613543003 Cr-Original-Commit-Position: refs/heads/master@{#15971} Committed: https://chromium.googlesource.com/external/webrtc/+/599c5011e7569a269f5521a8c... Review-Url: https://codereview.webrtc.org/2613543003 Cr-Commit-Position: refs/heads/master@{#16127} Committed: https://chromium.googlesource.com/external/webrtc/+/6dbbd89f1876df167cbd4c6eb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/6dbbd89f1876df167cbd4c6eb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
