Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481383002:
Fix BitrateControllerImpl not to ignore BW probe results mid-call. (Closed)
|
Created:
4 years, 1 month ago by Sergey Ulanov Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix BitrateControllerImpl not to ignore BW probe results mid-call.
Previously when BitrateControllerImpl::OnDelayBasedBweResult() is
called as result of a probe it was calling
bandwidth_estimation_.SetSendBitrate(), but not
UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was
effectively ignoring probe results as it kept the old
delay_based_bitrate_bps_ value, which caps the resulting bitrate.
BUG=webrtc:6332, webrtc:6710
Committed: https://crrev.com/26b675625f150be1ebc1a2350d39e5116d214871
Cr-Commit-Position: refs/heads/master@{#15071}
Patch Set 1 #
Messages
Total messages: 19 (8 generated)
sergeyu@chromium.org changed reviewers: + stefan@webrtc.org
stefan@webrtc.org changed reviewers: + philipel@webrtc.org
Philip, could you review this and assess the severity of it? How broken is this?
Philip: ping!
lgtm Sergey, sorry for the slow review. Testing this locally it looks like the probing result is reported before UpdateDelayBasedEstimate is called, so initial probing still works, but if the delay based estimate is lower than the probing result then the problem Sergey found will occur.
So this means that mid call probing is broken in ToT? Sergey, could you file a separate bug for this and refer to that bug in the BUG= field? Assign that bug to philip so we can go ahead and figure out when the issue was introduced and whether we have to merge this fix somewhere. lgtm
On 2016/11/13 13:50:36, stefan-webrtc (holmer) wrote: > So this means that mid call probing is broken in ToT? > > Sergey, could you file a separate bug for this and refer to that bug in the BUG= > field? Assign that bug to philip so we can go ahead and figure out when the > issue was introduced and whether we have to merge this fix somewhere. > > lgtm This CL is the fix for that bug.
Description was changed from ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332 ========== to ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332,webrtc:6710 ==========
On 2016/11/13 13:50:36, stefan-webrtc (holmer) wrote: > So this means that mid call probing is broken in ToT? > > Sergey, could you file a separate bug for this and refer to that bug in the BUG= > field? Assign that bug to philip so we can go ahead and figure out when the > issue was introduced and whether we have to merge this fix somewhere. > > lgtm Opened crbug.com/webrtc/6710
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/...
The CQ bit was unchecked by sergeyu@chromium.org
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/...
Message was sent while issue was closed.
Description was changed from ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332,webrtc:6710 ========== to ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332,webrtc:6710 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332,webrtc:6710 ========== to ========== Fix BitrateControllerImpl not to ignore BW probe results mid-call. Previously when BitrateControllerImpl::OnDelayBasedBweResult() is called as result of a probe it was calling bandwidth_estimation_.SetSendBitrate(), but not UpdateDelayBasedEstimate(). As result SendSideBandwidthEstimation was effectively ignoring probe results as it kept the old delay_based_bitrate_bps_ value, which caps the resulting bitrate. BUG=webrtc:6332,webrtc:6710 Committed: https://crrev.com/26b675625f150be1ebc1a2350d39e5116d214871 Cr-Commit-Position: refs/heads/master@{#15071} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/26b675625f150be1ebc1a2350d39e5116d214871 Cr-Commit-Position: refs/heads/master@{#15071} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
