|
|
Created:
4 years ago by stefan-webrtc Modified:
4 years ago Reviewers:
terelius CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse different restrictions of acked bitrate lag depending on operating point.
Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE.
BUG=webrtc:5079
Committed: https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907
Cr-Commit-Position: refs/heads/master@{#15389}
Patch Set 1 #Patch Set 2 : Comments addressed. #
Total comments: 6
Patch Set 3 : Comments addressed. #
Created: 4 years ago
Messages
Total messages: 15 (6 generated)
Description was changed from ========== Use different restrictions of lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 ========== to ========== Use different restrictions of acked bitrate lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 ==========
stefan@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:234: const uint32_t max_allowed_lag_kbps = Could we rename this max_bitrate_bps? Lag makes me think of the time lag between the network change and when we update the what the encoder target. Also, it is measured in bps. https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:238: // Don't change the bit rate if the send side is too far off. Place this comment above if-statement and other comment? https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:239: current_bitrate_bps = current_bitrate_bps_; If max_allowed_lag_kbps is the cap and we are above the cap, shouldn't we update the bitrate to max_allowed_lag_kbps? Maybe we can replace the entire if-statement with current_bitrate_bps = std::max(current_bitrate_bps, max_bitrate_bps)?
Comments addressed.
lgtm
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/...
https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc (right): https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:234: const uint32_t max_allowed_lag_kbps = On 2016/12/02 15:30:13, terelius wrote: > Could we rename this max_bitrate_bps? Lag makes me think of the time lag between > the network change and when we update the what the encoder target. Also, it is > measured in bps. Done. https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:238: // Don't change the bit rate if the send side is too far off. On 2016/12/02 15:30:13, terelius wrote: > Place this comment above if-statement and other comment? Done. https://codereview.webrtc.org/2542083003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc:239: current_bitrate_bps = current_bitrate_bps_; On 2016/12/02 15:30:13, terelius wrote: > If max_allowed_lag_kbps is the cap and we are above the cap, shouldn't we update > the bitrate to max_allowed_lag_kbps? > > Maybe we can replace the entire if-statement with > current_bitrate_bps = std::max(current_bitrate_bps, max_bitrate_bps)? Done as discussed.
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480695349983490, "parent_rev": "02aa2292dd44e417cf16146860d19bafdfa19fe1", "commit_rev": "30179ccb10f932ce958a5568df08f0ebcb4efb6b"}
Message was sent while issue was closed.
Description was changed from ========== Use different restrictions of acked bitrate lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 ========== to ========== Use different restrictions of acked bitrate lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use different restrictions of acked bitrate lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 ========== to ========== Use different restrictions of acked bitrate lag depending on operating point. Before this the BWE was allowed to operate freely up to 100 kbps. This isn't a good idea for audio BWE. BUG=webrtc:5079 Committed: https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907 Cr-Commit-Position: refs/heads/master@{#15389} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5932149c9aeaa7679ad0bc3183047766832ca907 Cr-Commit-Position: refs/heads/master@{#15389}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2547113002/ by deadbeef@webrtc.org. The reason for reverting is: Appears to cause a regression to RampUpTest.SendSideAudioOnlyUpDownUpRtx: https://build.chromium.org/p/client.webrtc.perf/builders/Android32%20Tests%20.... |