Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887693002:
Avoid render resampling when there is no need for render signal analysis. (Closed)
|
Created:
3 years, 7 months ago by peah-webrtc Modified:
3 years, 7 months ago Reviewers:
ivoc CC:
webrtc-reviews_webrtc.org, AleBzk, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid render resampling when there is no need for render signal analysis.
This CL adjusts the render processing rate such to avoid resampling of the
render signal when that is not needed.
Note that to avoid acquiring more locks than needed, this should be achieved
during initialization.
BUG=webrtc:7667
Review-Url: https://codereview.webrtc.org/2887693002
Cr-Commit-Position: refs/heads/master@{#18207}
Committed: https://chromium.googlesource.com/external/webrtc/+/ce4d91527a2cfaca3a8581ae4d5883fd63a346f9
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase and fixes of issues occurring on some platforms #
Total comments: 5
Messages
Total messages: 21 (14 generated)
Description was changed from ========== Avoid render resampling when there is no need for render signal analysis. BUG= ========== to ========== Avoid render resampling when there is no need for render signal analysis. This CL adjusts the render processing rate such to avoid resampling of the render signal when that is not needed. Note that to avoid acquiring more locks than needed, this should be achieved during initialization. BUG=webrtc:7667 ==========
peah@webrtc.org changed reviewers: + ivoc@webrtc.org
Hi, Could you please take a look at this CL?
LGTM
The CQ bit was checked by peah@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: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by peah@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/...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, I missed some aspects in the first patch which broke some of the local test I have. To solve that I needed to create a patch with some changes. Could you please take another look? Note that unfortunately, the new patch includes a rebase which it was nontrivial to avoid. Therefore, to simplify the review I have made comments in the CL to the background of the code changes. https://codereview.webrtc.org/2887693002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2887693002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:224: voice_activity_detector_enabled_ || residual_echo_detector_enabled_; This change is due to a rebase. https://codereview.webrtc.org/2887693002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:226: return CaptureMultiBandProcessingActive() || This change is due to a rebase. https://codereview.webrtc.org/2887693002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:242: mobile_echo_controller_enabled_ || adaptive_gain_controller_enabled_ || This change is due to a rebase. https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:605: formats_.api_format.reverse_input_stream().num_channels()); This change was needed for this to work properly for some combinations of input channels/rate. With this, the processing rate/channels is done the same for the input rate and the number of channels, which means that no implicit resampling will be done at the de-interleave. https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1470: if (submodule_states_.RenderMultiBandSubModulesActive()) { This change was needed as with the changes in this CL the render signal is no longer resampled to a native rate if i no processing is being done. https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_unittest.cc (left): https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_unittest.cc:865: // We always force the number of reverse channels used for processing to 1. The changes in this CL breaks this test. As this requirement does not make sense so I removed the test. The reason for that it breaks is that if there is no processing, this CL changes the behavior so that the reverse channels are not deinterleaved.
Still lgtm. https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:605: formats_.api_format.reverse_input_stream().num_channels()); On 2017/05/19 08:07:46, peah-webrtc wrote: > This change was needed for this to work properly for some combinations of input > channels/rate. With this, the processing rate/channels is done the same for the > input rate and the number of channels, which means that no implicit resampling > will be done at the de-interleave. Acknowledged. https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1470: if (submodule_states_.RenderMultiBandSubModulesActive()) { On 2017/05/19 08:07:46, peah-webrtc wrote: > This change was needed as with the changes in this CL the render signal is no > longer resampled to a native rate if i no processing is being done. Acknowledged.
On 2017/05/19 08:24:22, ivoc wrote: > Still lgtm. > > https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:605: > formats_.api_format.reverse_input_stream().num_channels()); > On 2017/05/19 08:07:46, peah-webrtc wrote: > > This change was needed for this to work properly for some combinations of > input > > channels/rate. With this, the processing rate/channels is done the same for > the > > input rate and the number of channels, which means that no implicit resampling > > will be done at the de-interleave. > > Acknowledged. > > https://codereview.webrtc.org/2887693002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:1470: if > (submodule_states_.RenderMultiBandSubModulesActive()) { > On 2017/05/19 08:07:46, peah-webrtc wrote: > > This change was needed as with the changes in this CL the render signal is no > > longer resampled to a native rate if i no processing is being done. > > Acknowledged. Thanks!
The CQ bit was checked by peah@webrtc.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": 40001, "attempt_start_ts": 1495182345371650,
"parent_rev": "7f52f084219fdd8003ff0312b81a2a991ad5f572", "commit_rev":
"ce4d91527a2cfaca3a8581ae4d5883fd63a346f9"}
Message was sent while issue was closed.
Description was changed from ========== Avoid render resampling when there is no need for render signal analysis. This CL adjusts the render processing rate such to avoid resampling of the render signal when that is not needed. Note that to avoid acquiring more locks than needed, this should be achieved during initialization. BUG=webrtc:7667 ========== to ========== Avoid render resampling when there is no need for render signal analysis. This CL adjusts the render processing rate such to avoid resampling of the render signal when that is not needed. Note that to avoid acquiring more locks than needed, this should be achieved during initialization. BUG=webrtc:7667 Review-Url: https://codereview.webrtc.org/2887693002 Cr-Commit-Position: refs/heads/master@{#18207} Committed: https://chromium.googlesource.com/external/webrtc/+/ce4d91527a2cfaca3a8581ae4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/ce4d91527a2cfaca3a8581ae4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
