Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492343002:
Echo Suppressor code simplification using the InverseFft function (#5) (Closed)
|
Created:
5 years ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@ESUP_refactoring4_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFurther refactored the echo suppressor code:
-Extended the InverseFft function to be more generally
applicable.
-Included the previous external extra scaling into the
preexisting InverseFft call.
-Moved the updating of aec->delayEstCtr to where it is
actually used.
-Refactored the output production and comfort noise
addition using the InverseFft function.
-Removed the if-statements checking the value of the
constant flagHbandCn as any value different from 1 would
crash the program. Also removed the constant
The changes have been tested for bitexactness.
BUG=webrtc:5201
Committed: https://crrev.com/0bc176b99b770f2fa3dd94d54553ab635df6930d
Cr-Commit-Position: refs/heads/master@{#11054}
Patch Set 1 : Replaced the inverse ffts with InverseFft #
Total comments: 21
Patch Set 2 : Removed extra empty lines #Patch Set 3 : Changed name of the InverseFft method #Patch Set 4 : Changed the scaling of the ScaledInverseFft #
Depends on Patchset: Messages
Total messages: 25 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Further refactoring BUG= ========== to ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant BUG= ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, minyue@webrtc.org
Description was changed from ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant BUG= ========== to ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG= ==========
Description was changed from ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG= ========== to ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG=webrtc:5201 ==========
Thanks Per. Here are my comments https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:47: static const int flagHbandCn = 1; // flag for adding comfort noise in H band I am not sure if this should be removed https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], To me, inverseFft is a well defined term (although there are various versions of it). We, at least, need to make sure that a == iFFt(Fft(a)) holds in a simple way. Other factors than scale and conjugations should belong to post-processing. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1196: InverseFft(efw, fft, 2.0f / PART_LEN2, 1); you can still use InverseFft of old version and add additional processing, and comments on why is the additional processing. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core_mips.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core_mips.c:277: if (aec->num_bands > 1 && flagHbandCn == 1) { I am not sure if this should be removed.
https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:47: static const int flagHbandCn = 1; // flag for adding comfort noise in H band On 2015/12/04 09:10:10, minyue-webrtc wrote: > I am not sure if this should be removed I thought first that this broke the code if not being set to 1, but I see now that it was wrong. Still though, this flag complicates the code, and I cannot see why we should suddenly want to add comfort noise in the upper band. I'm open to keep it if there is a good usecase for keeping it. But I cannot see why we ever use it as a tuning parameter so I think we should remove it in the interest to simplify the code. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 09:10:10, minyue-webrtc wrote: > To me, inverseFft is a well defined term (although there are various versions of > it). We, at least, need to make sure that > a == iFFt(Fft(a)) holds in a simple way. > > Other factors than scale and conjugations should belong to post-processing. Good point! Could we rename it such that it becomes more appropriate. As it is now, the settable scaling allows for quite a lot of code size reduction (due to being able to use this function) as well as optimizations (due to not having to do the scaling a second time). Regarding the conjugate, I think that it should actually be there in all places, but I have to verify that. Would ScaledInverseFft() be an ok name? https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1196: InverseFft(efw, fft, 2.0f / PART_LEN2, 1); On 2015/12/04 09:10:10, minyue-webrtc wrote: > you can still use InverseFft of old version and add additional processing, and > comments on why is the additional processing. The problem with that, is that I'll then need to do an additional outer scaling to get the scaling correct. Furthermore, in this case the conjugate is also needed to get it fine. Since this is the case in almost all places where the InverseFft operations are used, I'd rather create a function that is not an exact InverseFft but a scaled variant thereof. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core_mips.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core_mips.c:277: if (aec->num_bands > 1 && flagHbandCn == 1) { On 2015/12/04 09:10:10, minyue-webrtc wrote: > I am not sure if this should be removed. Please see the response for aec_core.c about this.
https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:47: static const int flagHbandCn = 1; // flag for adding comfort noise in H band On 2015/12/04 09:54:39, peah-webrtc wrote: > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > I am not sure if this should be removed > > I thought first that this broke the code if not being set to 1, but I see now > that it was wrong. > > Still though, this flag complicates the code, and I cannot see why we should > suddenly want to add comfort noise in the upper band. > > I'm open to keep it if there is a good usecase for keeping it. But I cannot see > why we ever use it as a tuning parameter so I think we should remove it in the > interest to simplify the code. I will let you decide. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 09:54:39, peah-webrtc wrote: > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > To me, inverseFft is a well defined term (although there are various versions > of > > it). We, at least, need to make sure that > > a == iFFt(Fft(a)) holds in a simple way. > > > > Other factors than scale and conjugations should belong to post-processing. > > Good point! Could we rename it such that it becomes more appropriate. > > As it is now, the settable scaling allows for quite a lot of code size reduction > (due to being able to use this function) as well as optimizations (due to not > having to do the scaling a second time). > > Regarding the conjugate, I think that it should actually be there in all places, > but I have to verify that. > > > Would ScaledInverseFft() be an ok name? I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1196: InverseFft(efw, fft, 2.0f / PART_LEN2, 1); On 2015/12/04 09:54:39, peah-webrtc wrote: > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > you can still use InverseFft of old version and add additional processing, and > > comments on why is the additional processing. > > The problem with that, is that I'll then need to do an additional outer scaling > to get the scaling correct. Furthermore, in this case the conjugate is also > needed to get it fine. > > Since this is the case in almost all places where the InverseFft operations are > used, I'd rather create a function that is not an exact InverseFft but a scaled > variant thereof. SGTM
https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 10:05:19, minyue-webrtc wrote: > On 2015/12/04 09:54:39, peah-webrtc wrote: > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > To me, inverseFft is a well defined term (although there are various > versions > > of > > > it). We, at least, need to make sure that > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > Other factors than scale and conjugations should belong to post-processing. > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > As it is now, the settable scaling allows for quite a lot of code size > reduction > > (due to being able to use this function) as well as optimizations (due to not > > having to do the scaling a second time). > > > > Regarding the conjugate, I think that it should actually be there in all > places, > > but I have to verify that. > > > > > > Would ScaledInverseFft() be an ok name? > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. The point with adding scale to this function is that you can scale while you are traversing the data. An external scale factor forces you to traverse it twice, right? https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1055: Remove extra line. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1072: Remove extra line.
https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (left): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:47: static const int flagHbandCn = 1; // flag for adding comfort noise in H band On 2015/12/04 10:05:19, minyue-webrtc wrote: > On 2015/12/04 09:54:39, peah-webrtc wrote: > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > I am not sure if this should be removed > > > > I thought first that this broke the code if not being set to 1, but I see now > > that it was wrong. > > > > Still though, this flag complicates the code, and I cannot see why we should > > suddenly want to add comfort noise in the upper band. > > > > I'm open to keep it if there is a good usecase for keeping it. But I cannot > see > > why we ever use it as a tuning parameter so I think we should remove it in the > > interest to simplify the code. > > I will let you decide. Acknowledged. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 11:48:06, hlundin-webrtc wrote: > On 2015/12/04 10:05:19, minyue-webrtc wrote: > > On 2015/12/04 09:54:39, peah-webrtc wrote: > > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > > To me, inverseFft is a well defined term (although there are various > > versions > > > of > > > > it). We, at least, need to make sure that > > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > > > Other factors than scale and conjugations should belong to > post-processing. > > > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > > > As it is now, the settable scaling allows for quite a lot of code size > > reduction > > > (due to being able to use this function) as well as optimizations (due to > not > > > having to do the scaling a second time). > > > > > > Regarding the conjugate, I think that it should actually be there in all > > places, > > > but I have to verify that. > > > > > > > > > Would ScaledInverseFft() be an ok name? > > > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. > > The point with adding scale to this function is that you can scale while you are > traversing the data. An external scale factor forces you to traverse it twice, > right? Yes, that is correct. The scaling differs upon the purpose, and by including an arbitrary scaling into this function we are able to do all scaling operations at once at all places where it is used. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 10:05:19, minyue-webrtc wrote: > On 2015/12/04 09:54:39, peah-webrtc wrote: > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > To me, inverseFft is a well defined term (although there are various > versions > > of > > > it). We, at least, need to make sure that > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > Other factors than scale and conjugations should belong to post-processing. > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > As it is now, the settable scaling allows for quite a lot of code size > reduction > > (due to being able to use this function) as well as optimizations (due to not > > having to do the scaling a second time). > > > > Regarding the conjugate, I think that it should actually be there in all > places, > > but I have to verify that. > > > > > > Would ScaledInverseFft() be an ok name? > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. I don't think that is a good solution as we would end up with more code. Also, looking at it again it seems like all calls that are made to this function actually use the same scaling (2/PART_LEN2) so the other function would then never be used. There could also be a point to hardcode the scaling into ScaledInverseFft but I like the fact that the scaling is made obvious when being an input parameter to the function. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1055: On 2015/12/04 11:48:06, hlundin-webrtc wrote: > Remove extra line. Done. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1072: On 2015/12/04 11:48:06, hlundin-webrtc wrote: > Remove extra line. Done. https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1196: InverseFft(efw, fft, 2.0f / PART_LEN2, 1); On 2015/12/04 10:05:19, minyue-webrtc wrote: > On 2015/12/04 09:54:39, peah-webrtc wrote: > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > you can still use InverseFft of old version and add additional processing, > and > > > comments on why is the additional processing. > > > > The problem with that, is that I'll then need to do an additional outer > scaling > > to get the scaling correct. Furthermore, in this case the conjugate is also > > needed to get it fine. > > > > Since this is the case in almost all places where the InverseFft operations > are > > used, I'd rather create a function that is not an exact InverseFft but a > scaled > > variant thereof. > > SGTM Acknowledged.
lgtm
https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float freq_data[2][PART_LEN1], On 2015/12/04 22:52:20, peah-webrtc wrote: > On 2015/12/04 10:05:19, minyue-webrtc wrote: > > On 2015/12/04 09:54:39, peah-webrtc wrote: > > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > > To me, inverseFft is a well defined term (although there are various > > versions > > > of > > > > it). We, at least, need to make sure that > > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > > > Other factors than scale and conjugations should belong to > post-processing. > > > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > > > As it is now, the settable scaling allows for quite a lot of code size > > reduction > > > (due to being able to use this function) as well as optimizations (due to > not > > > having to do the scaling a second time). > > > > > > Regarding the conjugate, I think that it should actually be there in all > > places, > > > but I have to verify that. > > > > > > > > > Would ScaledInverseFft() be an ok name? > > > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. > > I don't think that is a good solution as we would end up with more code. Also, > looking at it again it seems like all calls that are made to this function > actually use the same scaling (2/PART_LEN2) so the other function would then > never be used. > > There could also be a point to hardcode the scaling into ScaledInverseFft but I > like the fact that the scaling is made obvious when being an input parameter to > the function. You probably still need to, at least, rename the function. And also consider hardcode the scaling and add some reason for it.
On 2015/12/08 12:36:23, minyue-webrtc wrote: > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/aec/aec_core.c (right): > > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/aec/aec_core.c:817: static void InverseFft(float > freq_data[2][PART_LEN1], > On 2015/12/04 22:52:20, peah-webrtc wrote: > > On 2015/12/04 10:05:19, minyue-webrtc wrote: > > > On 2015/12/04 09:54:39, peah-webrtc wrote: > > > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > > > To me, inverseFft is a well defined term (although there are various > > > versions > > > > of > > > > > it). We, at least, need to make sure that > > > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > > > > > Other factors than scale and conjugations should belong to > > post-processing. > > > > > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > > > > > As it is now, the settable scaling allows for quite a lot of code size > > > reduction > > > > (due to being able to use this function) as well as optimizations (due to > > not > > > > having to do the scaling a second time). > > > > > > > > Regarding the conjugate, I think that it should actually be there in all > > > places, > > > > but I have to verify that. > > > > > > > > > > > > Would ScaledInverseFft() be an ok name? > > > > > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > > > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. > > > > I don't think that is a good solution as we would end up with more code. Also, > > looking at it again it seems like all calls that are made to this function > > actually use the same scaling (2/PART_LEN2) so the other function would then > > never be used. > > > > There could also be a point to hardcode the scaling into ScaledInverseFft but > I > > like the fact that the scaling is made obvious when being an input parameter > to > > the function. > > You probably still need to, at least, rename the function. And also consider > hardcode the scaling and add some reason for it. Thanks! I have renamed the method. But I would prefer not to hardcode the scaling, as I think it makes sense to have it as an explicit parameter to the method, as the construction of the input is matched to the scaling applied. Is that ok?
On 2015/12/16 13:23:47, peah-webrtc wrote: > On 2015/12/08 12:36:23, minyue-webrtc wrote: > > > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/aec/aec_core.c (right): > > > > > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/aec/aec_core.c:817: static void > InverseFft(float > > freq_data[2][PART_LEN1], > > On 2015/12/04 22:52:20, peah-webrtc wrote: > > > On 2015/12/04 10:05:19, minyue-webrtc wrote: > > > > On 2015/12/04 09:54:39, peah-webrtc wrote: > > > > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > > > > To me, inverseFft is a well defined term (although there are various > > > > versions > > > > > of > > > > > > it). We, at least, need to make sure that > > > > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > > > > > > > Other factors than scale and conjugations should belong to > > > post-processing. > > > > > > > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > > > > > > > As it is now, the settable scaling allows for quite a lot of code size > > > > reduction > > > > > (due to being able to use this function) as well as optimizations (due > to > > > not > > > > > having to do the scaling a second time). > > > > > > > > > > Regarding the conjugate, I think that it should actually be there in all > > > > places, > > > > > but I have to verify that. > > > > > > > > > > > > > > > Would ScaledInverseFft() be an ok name? > > > > > > > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a != > > > > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. > > > > > > I don't think that is a good solution as we would end up with more code. > Also, > > > looking at it again it seems like all calls that are made to this function > > > actually use the same scaling (2/PART_LEN2) so the other function would then > > > never be used. > > > > > > There could also be a point to hardcode the scaling into ScaledInverseFft > but > > I > > > like the fact that the scaling is made obvious when being an input parameter > > to > > > the function. > > > > You probably still need to, at least, rename the function. And also consider > > hardcode the scaling and add some reason for it. > > Thanks! I have renamed the method. But I would prefer not to hardcode the > scaling, as I think it makes sense to have it as an explicit parameter to the > method, as the construction of the input is matched to the scaling applied. Is > that ok? I think it is fine if a scale of 1.0f corresponds to a precise inverse of FFT.
On 2015/12/16 14:16:55, minyue-webrtc wrote: > On 2015/12/16 13:23:47, peah-webrtc wrote: > > On 2015/12/08 12:36:23, minyue-webrtc wrote: > > > > > > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > > > File webrtc/modules/audio_processing/aec/aec_core.c (right): > > > > > > > > > https://codereview.webrtc.org/1492343002/diff/20001/webrtc/modules/audio_proc... > > > webrtc/modules/audio_processing/aec/aec_core.c:817: static void > > InverseFft(float > > > freq_data[2][PART_LEN1], > > > On 2015/12/04 22:52:20, peah-webrtc wrote: > > > > On 2015/12/04 10:05:19, minyue-webrtc wrote: > > > > > On 2015/12/04 09:54:39, peah-webrtc wrote: > > > > > > On 2015/12/04 09:10:10, minyue-webrtc wrote: > > > > > > > To me, inverseFft is a well defined term (although there are various > > > > > versions > > > > > > of > > > > > > > it). We, at least, need to make sure that > > > > > > > a == iFFt(Fft(a)) holds in a simple way. > > > > > > > > > > > > > > Other factors than scale and conjugations should belong to > > > > post-processing. > > > > > > > > > > > > Good point! Could we rename it such that it becomes more appropriate. > > > > > > > > > > > > As it is now, the settable scaling allows for quite a lot of code size > > > > > reduction > > > > > > (due to being able to use this function) as well as optimizations (due > > to > > > > not > > > > > > having to do the scaling a second time). > > > > > > > > > > > > Regarding the conjugate, I think that it should actually be there in > all > > > > > places, > > > > > > but I have to verify that. > > > > > > > > > > > > > > > > > > Would ScaledInverseFft() be an ok name? > > > > > > > > > > I think we may keep InverseFft() as it was (unless it is wrong, i.e., a > != > > > > > InverseFft(Fft(a))), and add ScaledInverseFft() on top of it. > > > > > > > > I don't think that is a good solution as we would end up with more code. > > Also, > > > > looking at it again it seems like all calls that are made to this function > > > > actually use the same scaling (2/PART_LEN2) so the other function would > then > > > > never be used. > > > > > > > > There could also be a point to hardcode the scaling into ScaledInverseFft > > but > > > I > > > > like the fact that the scaling is made obvious when being an input > parameter > > > to > > > > the function. > > > > > > You probably still need to, at least, rename the function. And also consider > > > hardcode the scaling and add some reason for it. > > > > Thanks! I have renamed the method. But I would prefer not to hardcode the > > scaling, as I think it makes sense to have it as an explicit parameter to the > > method, as the construction of the input is matched to the scaling applied. Is > > that ok? > > I think it is fine if a scale of 1.0f corresponds to a precise inverse of FFT. That for sure makes a lot sense! I uploaded a new patchset which behaves like that.
lgtm
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/1492343002/#ps80001 (title: "Changed the scaling of the ScaledInverseFft")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492343002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492343002/80001
Message was sent while issue was closed.
Description was changed from ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG=webrtc:5201 ========== to ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG=webrtc:5201 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG=webrtc:5201 ========== to ========== Further refactored the echo suppressor code: -Extended the InverseFft function to be more generally applicable. -Included the previous external extra scaling into the preexisting InverseFft call. -Moved the updating of aec->delayEstCtr to where it is actually used. -Refactored the output production and comfort noise addition using the InverseFft function. -Removed the if-statements checking the value of the constant flagHbandCn as any value different from 1 would crash the program. Also removed the constant The changes have been tested for bitexactness. BUG=webrtc:5201 Committed: https://crrev.com/0bc176b99b770f2fa3dd94d54553ab635df6930d Cr-Commit-Position: refs/heads/master@{#11054} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0bc176b99b770f2fa3dd94d54553ab635df6930d Cr-Commit-Position: refs/heads/master@{#11054} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
