Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1865633005:
Don't always downsample to 16kHz in the reverse stream in APM (Closed)
|
Created:
4 years, 8 months ago by aluebs-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, 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. |
DescriptionDon't always downsample to 16kHz in the reverse stream in APM
The first approach landed here: https://codereview.webrtc.org/1773173002
But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/
The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression.
Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well.
BUG=webrtc:5725
TBR=tina.legrand@webrtc.org
Committed: https://crrev.com/eb3603bd5e72b3a941363c559ddea99ee71d510a
Cr-Commit-Position: refs/heads/master@{#12451}
Patch Set 1 #Patch Set 2 : Only enable when render processing #
Total comments: 2
Patch Set 3 : rebasing #Patch Set 4 : Simplify if-statement #
Messages
Total messages: 31 (10 generated)
Description was changed from ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. ========== to ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. TBR=tina.legrand@webrtc.org ==========
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
Description was changed from ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. TBR=tina.legrand@webrtc.org ========== to ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. BUG=webrtc:5725 TBR=tina.legrand@webrtc.org ==========
aluebs@webrtc.org changed reviewers: - henrik.lundin@webrtc.org, turaj@webrtc.org
This CL should land next week, but I wanted to already upload it for review to gain time.
peah, could you please test the AEC results on this CL and review. Thank you very much!
On 2016/04/08 18:27:54, aluebs-webrtc wrote: > peah, could you please test the AEC results on this CL and review. Thank you > very much! peah, PTAL.
On 2016/04/11 19:31:23, aluebs-webrtc wrote: > On 2016/04/08 18:27:54, aluebs-webrtc wrote: > > peah, could you please test the AEC results on this CL and review. Thank you > > very much! > > peah, PTAL. I'm sorry but I've not yet had time to analyze the AEC results for the CL. Will do it ASAP.
On 2016/04/12 11:56:41, peah-webrtc wrote: > On 2016/04/11 19:31:23, aluebs-webrtc wrote: > > On 2016/04/08 18:27:54, aluebs-webrtc wrote: > > > peah, could you please test the AEC results on this CL and review. Thank you > > > very much! > > > > peah, PTAL. > > I'm sorry but I've not yet had time to analyze the AEC results for the CL. Will > do it ASAP. It has been more than a week now. Could we bump the priority on this? Please let me know if I can help in any way.
On 2016/04/14 16:35:34, aluebs-webrtc wrote: > On 2016/04/12 11:56:41, peah-webrtc wrote: > > On 2016/04/11 19:31:23, aluebs-webrtc wrote: > > > On 2016/04/08 18:27:54, aluebs-webrtc wrote: > > > > peah, could you please test the AEC results on this CL and review. Thank > you > > > > very much! > > > > > > peah, PTAL. > > > > I'm sorry but I've not yet had time to analyze the AEC results for the CL. > Will > > do it ASAP. > > It has been more than a week now. Could we bump the priority on this? Please let > me know if I can help in any way. I'm sorry but peah is currently swamped with other tasks. We're trying to get another feature through the door.
On 2016/04/14 16:43:29, hlundin-webrtc wrote: > On 2016/04/14 16:35:34, aluebs-webrtc wrote: > > On 2016/04/12 11:56:41, peah-webrtc wrote: > > > On 2016/04/11 19:31:23, aluebs-webrtc wrote: > > > > On 2016/04/08 18:27:54, aluebs-webrtc wrote: > > > > > peah, could you please test the AEC results on this CL and review. Thank > > you > > > > > very much! > > > > > > > > peah, PTAL. > > > > > > I'm sorry but I've not yet had time to analyze the AEC results for the CL. > > Will > > > do it ASAP. > > > > It has been more than a week now. Could we bump the priority on this? Please > let > > me know if I can help in any way. > > I'm sorry but peah is currently swamped with other tasks. We're trying to get > another feature through the door. It would be great to at least have a way to repro, so I could test it myself.
Only enable when there is render stream processing.
On 2016/04/18 23:37:28, aluebs-webrtc wrote: > Only enable when there is render stream processing. Could you please add a rebase. I think the code change looks fine, but I'd like to be able to run my local bitexactness tests on it just to be 100% sure. As it is now, I get a rebase error when I try to apply the patch to master.
On 2016/04/19 12:46:27, peah-webrtc wrote: > On 2016/04/18 23:37:28, aluebs-webrtc wrote: > > Only enable when there is render stream processing. > > Could you please add a rebase. I think the code change looks fine, but I'd like > to be able to run my local bitexactness tests on it just to be 100% sure. > As it is now, I get a rebase error when I try to apply the patch to master. I normally wait until landing to rebase, so that people can do their reviews. But Now I uploaded the rebase, so that you can test.
On 2016/04/19 19:57:52, aluebs-webrtc wrote: > On 2016/04/19 12:46:27, peah-webrtc wrote: > > On 2016/04/18 23:37:28, aluebs-webrtc wrote: > > > Only enable when there is render stream processing. > > > > Could you please add a rebase. I think the code change looks fine, but I'd > like > > to be able to run my local bitexactness tests on it just to be 100% sure. > > As it is now, I get a rebase error when I try to apply the patch to master. > > I normally wait until landing to rebase, so that people can do their reviews. > But Now I uploaded the rebase, so that you can test. lgtm, but with the nit that I think that the if-statements around where the rev_proc_rate is selected could be simplified.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/1865633005/#ps60001 (title: "Simplify if-statement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865633005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865633005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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/1865633005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865633005/60001
Message was sent while issue was closed.
Description was changed from ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. BUG=webrtc:5725 TBR=tina.legrand@webrtc.org ========== to ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. BUG=webrtc:5725 TBR=tina.legrand@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. BUG=webrtc:5725 TBR=tina.legrand@webrtc.org ========== to ========== Don't always downsample to 16kHz in the reverse stream in APM The first approach landed here: https://codereview.webrtc.org/1773173002 But it was partially reverted, because it affected the AEC performance, here: https://codereview.webrtc.org/1867483003/ The main difference of this approach is that it doesn't use the 3-band splitting filter in the reverse stream, which seems to be the culprit of the AEC regression. Also, the 2-band splitting filter has been used for the 32kHz case for a long time without any problem, and this is expanded in the CL to cover the 48kHz case as well. BUG=webrtc:5725 TBR=tina.legrand@webrtc.org Committed: https://crrev.com/eb3603bd5e72b3a941363c559ddea99ee71d510a Cr-Commit-Position: refs/heads/master@{#12451} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/eb3603bd5e72b3a941363c559ddea99ee71d510a Cr-Commit-Position: refs/heads/master@{#12451}
Message was sent while issue was closed.
https://codereview.webrtc.org/1865633005/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1865633005/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:380: } I think that with the new changes this and the following if-statements could be simplified quite a lot.
Message was sent while issue was closed.
https://codereview.webrtc.org/1865633005/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1865633005/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:380: } On 2016/04/22 12:36:57, peah-webrtc wrote: > I think that with the new changes this and the following if-statements could be > simplified quite a lot. > But then the separation between what is temporary and what is not will be less obvious and will be harder to reference by the TODO.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/1918673002/ by tommi@webrtc.org. The reason for reverting is: Creating a revert patch set of this to see if it could be causing the issues we see on the linux_ubsan_vptr bot now. Not landing the revert just yet..
Message was sent while issue was closed.
On 2016/04/24 22:22:16, tommi-webrtc wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.webrtc.org/1918673002/ by mailto:tommi@webrtc.org. > > The reason for reverting is: Creating a revert patch set of this to see if it > could be causing the issues we see on the linux_ubsan_vptr bot now. > Not landing the revert just yet.. For posterity, this CL was found innocent :)
Message was sent while issue was closed.
On 2016/04/25 19:57:20, aluebs-webrtc wrote: > On 2016/04/24 22:22:16, tommi-webrtc wrote: > > A revert of this CL (patchset #4 id:60001) has been created in > > https://codereview.webrtc.org/1918673002/ by mailto:tommi@webrtc.org. > > > > The reason for reverting is: Creating a revert patch set of this to see if it > > could be causing the issues we see on the linux_ubsan_vptr bot now. > > Not landing the revert just yet.. > > For posterity, this CL was found innocent :) :) LGTM for the .pb files |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
