-
Notifications
You must be signed in to change notification settings - Fork 314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audio: Optimize and fix TDFB direction calculation #9169
Audio: Optimize and fix TDFB direction calculation #9169
Conversation
62dc1ef
to
e219c00
Compare
@@ -159,7 +157,8 @@ static bool line_array_mode_check(struct tdfb_comp_data *cd) | |||
* Form vector AB(a,b,c) from x(i+1) - x(i), y(i+1) - y(i), z(i+1) - z(i) | |||
* Form vector AC(d,e,f) from x(i+2) - x(i), y(i+2) - y(1), z(i+2) - z(i) | |||
*/ | |||
for (i = 0; i < num_mic_locations - 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this into a separate commit. Since this would be a bug fix. Previous change is an optimization for initialize code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I think there's also a typo in the comment in line 158 - it should be y(i)
, not y(1)
@@ -282,9 +281,10 @@ static void level_update(struct tdfb_comp_data *cd, int frames, int ch_count, in | |||
/* Calculate mean square level */ | |||
for (n = 0; n < frames; n++) { | |||
s = *p; | |||
tmp += ((int32_t)s * s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, without the Q1.15 multiply shifts this indeed fits the int32_t type. Please put this also to to own commit.
95175bd
to
049c690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShriramShastry LGTM, but needs @singalsu to approve. Btw, what is the MCPS or % speed up ?
049c690
to
2a328dc
Compare
src/audio/tdfb/tdfb_direction.c
Outdated
/* Start from i+1 halves the amount of iteration & to eliminate redundant checks */ | ||
for (j = i + 1; j < cd->config->num_mic_locations; j++) { | ||
for (j = 0; i < cd->config->num_mic_locations; i++) { | ||
if (j == i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this commit mostly reverts the previous one. It seems quite broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ! Please take a look. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShriramShastry I think its best to split PR as suggested, to help with bisection. Some of the changes are ready to merge but are blocked by opens from other changes in the PR.
3e7956e
to
136faf7
Compare
Start inner loop from i+1 to halve iterations and eliminate redundant checks. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>
Adjust loop boundary to ensure correct number of elements are processed. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>
136faf7
to
8767df4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes look good now, just need to improve comments
@@ -159,7 +157,8 @@ static bool line_array_mode_check(struct tdfb_comp_data *cd) | |||
* Form vector AB(a,b,c) from x(i+1) - x(i), y(i+1) - y(i), z(i+1) - z(i) | |||
* Form vector AC(d,e,f) from x(i+2) - x(i), y(i+2) - y(1), z(i+2) - z(i) | |||
*/ | |||
for (i = 0; i < num_mic_locations - 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I think there's also a typo in the comment in line 158 - it should be y(i)
, not y(1)
Use int32_t instead of int64_t to improve performance in level_update. Improve comments in max_mic_distance function to better explain the distance calculation between all possible microphone pairs. Correct the typo in line_array_mode_check function. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com> Address reviewer comments: Improve comments in max_mic_distance and correct typo in line_array_mode_check
8767df4
to
ee2bdab
Compare
/* Start from i+1 halves the amount of iteration & to eliminate redundant checks */ | ||
/* Calculate distances between all possible microphone pairs to | ||
* find the maximum distance | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the future: please make corrections, addressing review comments in the commit where that change is made - not in a later commit. It doesn't matter much in this case, since this is a comment, but it would make reviewing easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI @lyakh: using new commits to address review comments is the "standard" GitHub process. Using Github like plain git is very unusual, SOF (and Zephyr) are among the outliers rewriting history and using "force" pushes.
Just a warning that this issue will keep coming again and again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb do I understand you correctly, that as soon as I push a PR with whatever bugs in my commits, the "standard" / recommended way to improve that PR would be to push fixes on top? So that after a merge you get my original 5 commits with all the bugs in them and then additional 3 commits on top with fixes? If so, I strongly disagree with this process. For one it would break bisection. And in general that isn't what a peer review process is about IMHO. Recall the "original LKML" (at least as of the git era) style review process - you send an email thread with your commits, you get a ton of replies explaining how wrong you are and suggesting all the therapists you should visit to address your problems, after which you go, fix your stuff and resend the emails - the original ones are gone, no fixing on top.
But importantly - we all make mistakes, very few PRs are recognised as good enough in their first version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @lyakh, This comment change should really be in the previous commit that just added it. It's not much work to shuffle changes from commit to other and @ShriramShastry should take the time to learn to clean up patches this way and ease our review work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed the last sentence:
JUST A WARNING that this issue will keep coming again and again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for fun, here's a very curious and much more extreme view: https://fossil-scm.org/home/doc/trunk/www/rebaseharm.md
This one is very good: https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets wait for @singalsu to return before we merge.
We don't have CI validation for these features and @ShriramShastry is without DUTs to run tests. So bear with me, I need to test this manually before approve. Right now there is higher priority work for a new platform. |
/* Start from i+1 halves the amount of iteration & to eliminate redundant checks */ | ||
/* Calculate distances between all possible microphone pairs to | ||
* find the maximum distance | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @lyakh, This comment change should really be in the previous commit that just added it. It's not much work to shuffle changes from commit to other and @ShriramShastry should take the time to learn to clean up patches this way and ease our review work.
@@ -155,7 +157,7 @@ static bool line_array_mode_check(struct tdfb_comp_data *cd) | |||
|
|||
/* Cross product of vectors AB and AC is (0, 0, 0) if they are co-linear. | |||
* Form vector AB(a,b,c) from x(i+1) - x(i), y(i+1) - y(i), z(i+1) - z(i) | |||
* Form vector AC(d,e,f) from x(i+2) - x(i), y(i+2) - y(1), z(i+2) - z(i) | |||
* Form vector AC(d,e,f) from x(i+2) - x(i), y(i+2) - y(i), z(i+2) - z(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, this part of code was changed in previous commit, would fit there better. The changes so far are initialize code changes (run once) and comments changes while the next one is good and actually reduces run-time MCPS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve due to bug fix (would possibly treat a L-shape array as line array if last mic deviates from line) and a good optimization found.
Not so happy with commits structure with changes to previously added comments in next commit. But OK this time, please try to improve with those.
Adding that this PR saves when sound angle tracking is enabled in TGL 4 mic case from 134 MCPS average to 120 MPCS average. Also test tools/test/audio/tdfb_direction_test.m is passed. |
Failures in https://sof-ci.01.org/sofpr/PR9169/build5921/devicetest/index.html , but the TDFB not part of these topologies, so can' be related. Proceeding with merge, thanks @ShriramShastry ! |
This check-in refines the TDFB direction calculation, addressing both
performance and correctness:
max_mic_distance
by fixing loop conditions.line_array_mode_check
ensuring all checksfor co-linearity among microphone locations are performed.