Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503713003:
Smooth BWE and pass it to Audio Network Adaptor. (Closed)
|
Created:
4 years, 1 month ago by michaelt Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSmooth BWE and pass it to Audio Network Adaptor.
BUG=webrtc:6443, webrtc:6303
Committed: https://crrev.com/2fedf9c69aa13752312c346e2f99e9df88aa6467
Cr-Commit-Position: refs/heads/master@{#15257}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Response to comments. #
Total comments: 2
Patch Set 3 : Response to comments. #
Total comments: 5
Patch Set 4 : Resonse to comments. #Patch Set 5 : Rebased #Patch Set 6 : Rebased #
Dependent Patchsets: Messages
Total messages: 46 (28 generated)
The CQ bit was checked by michaelt@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/...
Description was changed from ========== Smooth BWE for ANA. BUG=webrtc:6443 ========== to ========== Smooth BWE for ANA. BUG=webrtc:6443 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
Hi, Here a quite simple impl. for the BWE smoothing.
lgtm from my side, but I think it's mostly up to minyue in this directory.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1315: bitrate_bps_smoothed_.AddSample(bitrate_bps); will it obtain time constant here? if so, does the default time constant matter at all? https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:3242: void Channel::OnUplinkBandwidthUpdated(int bitrate_bps) { I don't think we need to add this function. put the call to encoder in SetBitRate https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:553: SmoothingFilterImpl bitrate_bps_smoothed_; I hope to call this uplink_bandwidth_estimator and write comment like "we estimate the uplink bandwidth by smoothing out the target bit rate" at the proper place in SetBitrate.
https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:553: SmoothingFilterImpl bitrate_bps_smoothed_; On 2016/11/15 14:36:03, minyue-webrtc wrote: > I hope to call this > > uplink_bandwidth_estimator and write comment like "we estimate the uplink > bandwidth by smoothing out the target bit rate" at the proper place in > SetBitrate. I would prefer not using uplink bandwidth, as it's rather the average/smoothed bitrate allocation for this audio stream. It would be good not to mix concepts.
https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:553: SmoothingFilterImpl bitrate_bps_smoothed_; On 2016/11/15 14:41:58, stefan-webrtc (holmer) wrote: > On 2016/11/15 14:36:03, minyue-webrtc wrote: > > I hope to call this > > > > uplink_bandwidth_estimator and write comment like "we estimate the uplink > > bandwidth by smoothing out the target bit rate" at the proper place in > > SetBitrate. > > I would prefer not using uplink bandwidth, as it's rather the average/smoothed > bitrate allocation for this audio stream. It would be good not to mix concepts. ok. Then please add "we give smoothed bitrate allocation to audio network adaptor as the uplink bandwidth" when we call the unlink bandwidth method of ANA.
Description was changed from ========== Smooth BWE for ANA. BUG=webrtc:6443 ========== to ========== Smooth BWE and pass it to Audio Network Adaptor. BUG=webrtc:6443, webrtc:6303 ==========
https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1315: bitrate_bps_smoothed_.AddSample(bitrate_bps); Yes. Not really but we have to set it anyway. https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:3242: void Channel::OnUplinkBandwidthUpdated(int bitrate_bps) { Ok https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:553: SmoothingFilterImpl bitrate_bps_smoothed_; On 2016/11/16 08:56:45, minyue-webrtc wrote: > On 2016/11/15 14:41:58, stefan-webrtc (holmer) wrote: > > On 2016/11/15 14:36:03, minyue-webrtc wrote: > > > I hope to call this > > > > > > uplink_bandwidth_estimator and write comment like "we estimate the uplink > > > bandwidth by smoothing out the target bit rate" at the proper place in > > > SetBitrate. > > > > I would prefer not using uplink bandwidth, as it's rather the average/smoothed > > bitrate allocation for this audio stream. It would be good not to mix > concepts. > > ok. Then please add "we give smoothed bitrate allocation to audio network > adaptor as the uplink bandwidth" when we call the unlink bandwidth method of > ANA. Done.
https://codereview.webrtc.org/2503713003/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:551: SmoothingFilterImpl bitrate_bps_smoothed_; better bitrate_bps_smoothed_ -> bitrate_smoother_
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2503713003/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2503713003/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:551: SmoothingFilterImpl bitrate_bps_smoothed_; On 2016/11/17 12:04:02, minyue-webrtc wrote: > better bitrate_bps_smoothed_ -> bitrate_smoother_ Done.
https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... File webrtc/base/smoothing_filter.cc (right): https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... webrtc/base/smoothing_filter.cc:64: filter_.UpdateBase(exp(1.0f / time_constant_ms_)); is this needed. Don't we always decide to updateBase in AddSample? https://codereview.webrtc.org/2503713003/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2503713003/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:899: // The time constant of the bitrate smoother will be set on every Bitrate smoother can be initialized with arbitrary time constant (0 used here). The actual time constant will be ...
https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... File webrtc/base/smoothing_filter.cc (right): https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... webrtc/base/smoothing_filter.cc:64: filter_.UpdateBase(exp(1.0f / time_constant_ms_)); Just in the case the filter is not initialized. https://codereview.webrtc.org/2503713003/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2503713003/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:899: // The time constant of the bitrate smoother will be set on every On 2016/11/21 09:18:24, minyue-webrtc wrote: > Bitrate smoother can be initialized with arbitrary time constant (0 used here). > The actual time constant will be ... Done.
lgtm https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... File webrtc/base/smoothing_filter.cc (right): https://codereview.webrtc.org/2503713003/diff/60001/webrtc/base/smoothing_fil... webrtc/base/smoothing_filter.cc:64: filter_.UpdateBase(exp(1.0f / time_constant_ms_)); On 2016/11/21 09:48:32, michaelt wrote: > Just in the case the filter is not initialized. > I see... BTW, you mean "it IS initialized" don't you?
but you need to wait until the common_audio/smoothing_filter is landed.
Yes
The CQ bit was checked by michaelt@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/2503713003/#ps80001 (title: "Resonse to comments.")
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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12662) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12682) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15030)
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12664) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/20405)
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by michaelt@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: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2503713003/#ps120001 (title: "Rebased")
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": 120001, "attempt_start_ts": 1480328037743130,
"parent_rev": "fd9754300f5e031066e9fc930514b26ce1001ed7", "commit_rev":
"6a73186a389ce4f7b6035d3b939a672855dc54e7"}
Message was sent while issue was closed.
Description was changed from ========== Smooth BWE and pass it to Audio Network Adaptor. BUG=webrtc:6443, webrtc:6303 ========== to ========== Smooth BWE and pass it to Audio Network Adaptor. BUG=webrtc:6443, webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Smooth BWE and pass it to Audio Network Adaptor. BUG=webrtc:6443, webrtc:6303 ========== to ========== Smooth BWE and pass it to Audio Network Adaptor. BUG=webrtc:6443, webrtc:6303 Committed: https://crrev.com/2fedf9c69aa13752312c346e2f99e9df88aa6467 Cr-Commit-Position: refs/heads/master@{#15257} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2fedf9c69aa13752312c346e2f99e9df88aa6467 Cr-Commit-Position: refs/heads/master@{#15257} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
