Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774553002:
Drop the 16kHz sample rate restriction on AECM and zero out higher bands (Closed)
|
Created:
4 years, 9 months ago by aluebs-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDrop the 16kHz sample rate restriction on AECM and zero out higher bands
The restriction has been removed completely and AECM now supports any
number of higher bands. But this has been achieved by always zeroing out the
higher bands, instead of applying a constant gain which is the average over half
of the lower band (like it is done for the AEC), because that would be
non-trivial to implement and we don't want to spend too much time on AECM, since
we want to get rid of it in the long term anyway.
R=peah@webrtc.org, solenberg@webrtc.org, tina.legrand@webrtc.org
Committed: https://crrev.com/f687d53aabee0523ce6e9e0636163af8df120e41
Cr-Commit-Position: refs/heads/master@{#11931}
Patch Set 1 #Patch Set 2 : Fixed unittest #
Total comments: 4
Patch Set 3 : Fix error message #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 33 (8 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
On 2016/03/07 16:09:53, aluebs-webrtc wrote: Nice changes! Have you tested this in an apprtc mobile call yet?
On 2016/03/08 06:17:57, peah-webrtc wrote: > On 2016/03/07 16:09:53, aluebs-webrtc wrote: > > Nice changes! > Have you tested this in an apprtc mobile call yet? No, but I have run audioproc with an aecdump that before was crashing (because it had a sample rate of 32kHz) and it seems to be working as expected. I will test it with the appRTCDemo app and report back.
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
Nice, but I don't know the code well enough to provide a confident approval.
Fixed the unittest. PTAL.
I'm far less familiar with the code than Henrik, so I leave it to Per for the final approval. https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:323: LOG(LS_ERROR) << "AECM only supports 16 kHz or lower sample rates"; I guess this error message needs to be revisited. https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:2025: EXPECT_LE(num_bad_chunks, kMaxNumBadChunks); Indentation.
https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:323: LOG(LS_ERROR) << "AECM only supports 16 kHz or lower sample rates"; On 2016/03/08 16:35:59, turaj wrote: > I guess this error message needs to be revisited. Done. https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right): https://codereview.webrtc.org/1774553002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_unittest.cc:2025: EXPECT_LE(num_bad_chunks, kMaxNumBadChunks); On 2016/03/08 16:35:59, turaj wrote: > Indentation. Done.
Tested with peah on an AppRTCDemo call that this change doesn't break the aecm and it seems it might even slightly improve it because of splitting filter used instead of the downsampler.
On 2016/03/09 12:51:59, aluebs-webrtc wrote: > Tested with peah on an AppRTCDemo call that this change doesn't break the aecm > and it seems it might even slightly improve it because of splitting filter used > instead of the downsampler. Truly Awesome! lgtm
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774553002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774553002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4006)
aluebs@webrtc.org changed reviewers: + solenberg@webrtc.org, tina.legrand@webrtc.org
Apparently I need some more owner's approval :) tlegrand: data/audio_processing/output_data_fixed.pb solenberg: webrtc/voice_engine/transmit_mixer.cc
On 2016/03/09 13:47:34, aluebs-webrtc wrote: > Apparently I need some more owner's approval :) > tlegrand: > data/audio_processing/output_data_fixed.pb > solenberg: > webrtc/voice_engine/transmit_mixer.cc If I understand this right, we still have a 16 kHz restriction for AECM, but it is now handled inside of APM? If that is the case, do you mind updating the CL description? I first thought that this CL removed the restriction completely, and changed AECM to work support up to 48 kHz.
lgtm for transmit_mixer.cc https://codereview.webrtc.org/1774553002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1774553002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:209: for (size_t band = 1u; band < audio->num_bands(); ++band) { do you really need the 'u' in 1u?
Really nice change! LTGM, if you update the description, and given that the need for a new output_data_fixed.pb file was expected.
Well, the restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. I added that to the description. The new output_data_fixed.pb file was only needed to add tests for 32kHz and 48kHz on the fixed point interface, that before were skipped completely because of the AECM restriction. https://codereview.webrtc.org/1774553002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/echo_control_mobile_impl.cc (right): https://codereview.webrtc.org/1774553002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/echo_control_mobile_impl.cc:209: for (size_t band = 1u; band < audio->num_bands(); ++band) { On 2016/03/09 14:58:21, the sun wrote: > do you really need the 'u' in 1u? I am never sure if it is needed for VisualStudio compiler, as with ".f" for floats, so I rather always use it by default, since it doesn't cost anything and maybe avoid some headache :)
On 2016/03/09 15:00:59, tlegrand-webrtc wrote: > Really nice change! > > LTGM, if you update the description, and given that the need for a new > output_data_fixed.pb file was expected. I am going to need and "LGTM" on top of that "LTGM" ;)
On 2016/03/09 15:27:49, aluebs-webrtc wrote: > On 2016/03/09 15:00:59, tlegrand-webrtc wrote: > > Really nice change! > > > > LTGM, if you update the description, and given that the need for a new > > output_data_fixed.pb file was expected. > > I am going to need and "LGTM" on top of that "LTGM" ;) Hehe, not sure I should give it before you update the description. :) The think you wrote above is perfect. Can you add that to the CL description? It's good to have there when searching through changes in WebRTC. LGTM
Description was changed from ========== Drop the 16kHz sample rate restriction on AECM ========== to ========== Drop the 16kHz sample rate restriction on AECM and zero out higher bands The restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. ==========
On 2016/03/09 15:34:29, tlegrand-webrtc wrote: > On 2016/03/09 15:27:49, aluebs-webrtc wrote: > > On 2016/03/09 15:00:59, tlegrand-webrtc wrote: > > > Really nice change! > > > > > > LTGM, if you update the description, and given that the need for a new > > > output_data_fixed.pb file was expected. > > > > I am going to need and "LGTM" on top of that "LTGM" ;) > > Hehe, not sure I should give it before you update the description. :) > The think you wrote above is perfect. Can you add that to the CL description? > It's good to have there when searching through changes in WebRTC. LGTM I had updated the one-liner descrption with "...and zero out higher bands", but I agree that having a more detailed description makes a lot of sense. Done now :)
Message was sent while issue was closed.
Description was changed from ========== Drop the 16kHz sample rate restriction on AECM and zero out higher bands The restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. ========== to ========== Drop the 16kHz sample rate restriction on AECM and zero out higher bands The restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. R=peah@webrtc.org, solenberg@webrtc.org, tina.legrand@webrtc.org Committed: https://crrev.com/f687d53aabee0523ce6e9e0636163af8df120e41 Cr-Commit-Position: refs/heads/master@{#11931} ==========
Message was sent while issue was closed.
Description was changed from ========== Drop the 16kHz sample rate restriction on AECM and zero out higher bands The restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. R=peah@webrtc.org, solenberg@webrtc.org, tina.legrand@webrtc.org Committed: https://crrev.com/f687d53aabee0523ce6e9e0636163af8df120e41 Cr-Commit-Position: refs/heads/master@{#11931} ========== to ========== Drop the 16kHz sample rate restriction on AECM and zero out higher bands The restriction has been removed completely and AECM now supports any number of higher bands. But this has been achieved by always zeroing out the higher bands, instead of applying a constant gain which is the average over half of the lower band (like it is done for the AEC), because that would be non-trivial to implement and we don't want to spend too much time on AECM, since we want to get rid of it in the long term anyway. R=peah@webrtc.org, solenberg@webrtc.org, tina.legrand@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f687d53aabee0523ce6e9e063... ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f687d53aabee0523ce6e9e0636163af8df120e41 Cr-Commit-Position: refs/heads/master@{#11931}
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as f687d53aabee0523ce6e9e0636163af8df120e41 (presubmit successful).
Message was sent while issue was closed.
On 2016/03/09 15:38:15, aluebs-webrtc wrote: > Committed patchset #3 (id:40001) manually as > f687d53aabee0523ce6e9e0636163af8df120e41 (presubmit successful). Breaks Android it looks like. See your own try jobs and https://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/1781893002/ by perkj@webrtc.org. The reason for reverting is: Breaks Android it looks like. See your own try jobs and https://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%....
Message was sent while issue was closed.
On 2016/03/10 00:22:12, perkj_webrtc wrote: > On 2016/03/09 15:38:15, aluebs-webrtc wrote: > > Committed patchset #3 (id:40001) manually as > > f687d53aabee0523ce6e9e0636163af8df120e41 (presubmit successful). > > Breaks Android it looks like. > See your own try jobs and > https://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%... You are right. I overlooked that because of some flaky bots. Sorry about that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
