Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800033003:
Fixing sample-rate dependent band-split filter issues in AEC3 (Closed)
|
Created:
3 years, 8 months ago by peah-webrtc Modified:
3 years, 8 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, 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. |
DescriptionFixing sample-rate dependent band-split filter issues in AEC3
This CL ensures that the number of bands
for the render side matches that for the capture side
when AEC3 is active. Without this, there was problems
when the render rate is different from the capture rate.
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2800033003
Cr-Commit-Position: refs/heads/master@{#17586}
Committed: https://chromium.googlesource.com/external/webrtc/+/2ce640fada9d045f3161b122cab9fff50f256da8
Patch Set 1 #
Total comments: 3
Patch Set 2 : Changed conditional according to reviewer comments. #Patch Set 3 : Reverted the changes relating to the number of bands used internally inside AEC3 #
Messages
Total messages: 22 (12 generated)
Description was changed from ========== Fixing platform dependent band-split filter issues in AEC3 BUG= ========== to ========== Fixing platform dependent band-split filter issues in AEC3 The band split filter in the audio processing module has a scheme that is platform dependent. The code for setting the number of bands in AEC3 did not match this, which caused issues on ARM platforms for some sample rates. That is addressed by one part of this CL. The other part of this CL ensures that the number of bands for the render side matches that for the capture side when AEC3 is active. Without this, there was problems when the render rate is different from the capture rate. BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
hlundin@: Could you please take a look at this CL?
LGTM with one comment. https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/aec3_common.h (right): https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/aec3_common.h:74: return std::min(2, static_cast<size_t>( I think this would be more readable as return static_cast<size_t>(sample_rate_hz == 8000 ? 1 : 2);
https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/aec3_common.h (right): https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/aec3_common.h:74: return std::min(2, static_cast<size_t>( On 2017/04/07 09:55:06, hlundin-webrtc wrote: > I think this would be more readable as > return static_cast<size_t>(sample_rate_hz == 8000 ? 1 : 2); My bad. How about return static_cast<size_t>(sample_rate_hz <= 16000 ? 1 : 2);
Thanks for the comments! https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/aec3_common.h (right): https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/aec3_common.h:74: return std::min(2, static_cast<size_t>( On 2017/04/07 09:56:35, hlundin-webrtc wrote: > On 2017/04/07 09:55:06, hlundin-webrtc wrote: > > I think this would be more readable as > > return static_cast<size_t>(sample_rate_hz == 8000 ? 1 : 2); > > My bad. How about > return static_cast<size_t>(sample_rate_hz <= 16000 ? 1 : 2); Awesome! Much better! Done.
Thanks for the comments! https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec3/aec3_common.h (right): https://codereview.webrtc.org/2800033003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec3/aec3_common.h:74: return std::min(2, static_cast<size_t>( On 2017/04/07 09:56:35, hlundin-webrtc wrote: > On 2017/04/07 09:55:06, hlundin-webrtc wrote: > > I think this would be more readable as > > return static_cast<size_t>(sample_rate_hz == 8000 ? 1 : 2); > > My bad. How about > return static_cast<size_t>(sample_rate_hz <= 16000 ? 1 : 2); Awesome! Much better! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2800033003/#ps20001 (title: "Changed conditional according to reviewer 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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/23147)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2800033003/#ps40001 (title: "Reverted the changes relating to the number of bands used internally inside AEC3")
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 peah@webrtc.org
Description was changed from ========== Fixing platform dependent band-split filter issues in AEC3 The band split filter in the audio processing module has a scheme that is platform dependent. The code for setting the number of bands in AEC3 did not match this, which caused issues on ARM platforms for some sample rates. That is addressed by one part of this CL. The other part of this CL ensures that the number of bands for the render side matches that for the capture side when AEC3 is active. Without this, there was problems when the render rate is different from the capture rate. BUG=webrtc:6018 ========== to ========== Fixing sample-rate dependent band-split filter issues in AEC3 This CL ensures that the number of bands for the render side matches that for the capture side when AEC3 is active. Without this, there was problems when the render rate is different from the capture rate. BUG=webrtc:6018 ==========
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": 1491561467017260,
"parent_rev": "ea44ad4efcafef6e583a700a5637c232fe1e32a8", "commit_rev":
"2ce640fada9d045f3161b122cab9fff50f256da8"}
Message was sent while issue was closed.
Description was changed from ========== Fixing sample-rate dependent band-split filter issues in AEC3 This CL ensures that the number of bands for the render side matches that for the capture side when AEC3 is active. Without this, there was problems when the render rate is different from the capture rate. BUG=webrtc:6018 ========== to ========== Fixing sample-rate dependent band-split filter issues in AEC3 This CL ensures that the number of bands for the render side matches that for the capture side when AEC3 is active. Without this, there was problems when the render rate is different from the capture rate. BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2800033003 Cr-Commit-Position: refs/heads/master@{#17586} Committed: https://chromium.googlesource.com/external/webrtc/+/2ce640fada9d045f3161b122c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/2ce640fada9d045f3161b122c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
