Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125523004:
Fix bug where a connection switch causes BWE to be set to zero. (Closed)
|
Created:
4 years, 5 months ago by stefan-webrtc Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix bug where a connection switch causes BWE to be set to zero.
BUG=webrtc:6076
R=sprang@webrtc.org
Committed: https://crrev.com/be40296ccef50dd79312c6fce533bfa36b7063dc
Cr-Commit-Position: refs/heads/master@{#13414}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved fix to call.cc and added a better test. #
Total comments: 4
Patch Set 3 : Comments addressed. #
Messages
Total messages: 24 (8 generated)
stefan@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2125523004/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right): https://codereview.webrtc.org/2125523004/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:151: if (bitrate_bps == -1) { If -1 has a special meaning, isn't there a risk to introduce new bugs if people forget to handle that when making changes to the bitrate controller? If we are changing the network, do we want to keep the bitrate estimate from the old network, or reset it to the original default value?
I have improved the fix and also added a test which verifies the actual issue that caused this to happen. https://codereview.webrtc.org/2125523004/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right): https://codereview.webrtc.org/2125523004/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:151: if (bitrate_bps == -1) { On 2016/07/05 13:55:36, terelius wrote: > If -1 has a special meaning, isn't there a risk to introduce new bugs if people > forget to handle that when making changes to the bitrate controller? > > If we are changing the network, do we want to keep the bitrate estimate from the > old network, or reset it to the original default value? Good point, the correct behavior is to change to the original default value, I think.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2125523004/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2125523004/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:584: // no point in remembering for the future. I still find it a bit strange to keep the old estimate when switching network. Is this the intended behavior, or should there be a TODO here? https://codereview.webrtc.org/2125523004/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2125523004/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1145: call_->GetStats().send_bandwidth_bps > kStartBitrateBps) { I don't understand this. The code seems (almost) equivalent to a single if-statement if (call_->GetStats().send_bandwidth_bps > kStartBitrateBps) { observation_complete_.Set(); }
https://codereview.webrtc.org/2125523004/diff/20001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2125523004/diff/20001/webrtc/call/call.cc#newco... webrtc/call/call.cc:584: // no point in remembering for the future. On 2016/07/07 15:48:38, terelius wrote: > I still find it a bit strange to keep the old estimate when switching network. > Is this the intended behavior, or should there be a TODO here? Oh, nvm. Please ignore this comment.
Comments addressed.
https://codereview.webrtc.org/2125523004/diff/20001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2125523004/diff/20001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1145: call_->GetStats().send_bandwidth_bps > kStartBitrateBps) { On 2016/07/07 15:48:38, terelius wrote: > I don't understand this. The code seems (almost) equivalent to a single > if-statement > if (call_->GetStats().send_bandwidth_bps > kStartBitrateBps) { > observation_complete_.Set(); > } You are right. This is a complicated way of saying what you suggest. It's an artifact of how I initially wrote the test. I will simply remove the last wait as it isn't providing much value.
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
lgtm
terelius: I will submit this as is, but if you have any more comments I'll be happy to do a follow-up.
The CQ bit was checked by stefan@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Fix bug where a connection switch causes BWE to be set to zero. BUG=webrtc:6076 ========== to ========== Fix bug where a connection switch causes BWE to be set to zero. BUG=webrtc:6076 R=sprang@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/be40296ccef50dd79312c6fce... ==========
Description was changed from ========== Fix bug where a connection switch causes BWE to be set to zero. BUG=webrtc:6076 R=sprang@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/be40296ccef50dd79312c6fce... ========== to ========== Fix bug where a connection switch causes BWE to be set to zero. BUG=webrtc:6076 R=sprang@webrtc.org Committed: https://crrev.com/be40296ccef50dd79312c6fce533bfa36b7063dc Cr-Commit-Position: refs/heads/master@{#13414} ==========
Committed patchset #3 (id:40001) manually as be40296ccef50dd79312c6fce533bfa36b7063dc (presubmit successful).
Patchset 3 (id:??) landed as https://crrev.com/be40296ccef50dd79312c6fce533bfa36b7063dc Cr-Commit-Position: refs/heads/master@{#13414}
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_gn_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
