Skip to content

Audio: Optimize and fix TDFB direction calculation #9169

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.

Sign up for GitHub

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

Merged
kv2019i merged 3 commits into from
Aug 5, 2024
Merged

Audio: Optimize and fix TDFB direction calculation #9169

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77daf1a
Audio: Bugfix: Optimize inner loop in max_mic_distance calculation
ShriramShastry Jun 23, 2024
8adadca
Audio: Bugfix: Correct loop boundary in line_array_mode_check
ShriramShastry Jun 23, 2024
ee2bdab
Audio: Bugfix: Optimize level_update function and improve comments
ShriramShastry Jun 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/audio/tdfb/tdfb_direction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ static int16_t max_mic_distance(struct tdfb_comp_data *cd)

/* Max lag based on largest array dimension. Microphone coordinates are Q4.12 meters */
for (i = 0; i < cd->config->num_mic_locations; i++) {
for (j = 0; i < cd->config->num_mic_locations; i++) {
if (j == i)
continue;

/* Calculate distances between all possible microphone pairs to
* find the maximum distance
*/
Copy link
Collaborator

@lyakh lyakh Jun 25, 2024

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

Copy link
Collaborator

@marc-hb marc-hb Jul 19, 2024 •
edited
Loading

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.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes

Just a warning that this issue will keep coming again and again.

Copy link
Collaborator

@lyakh lyakh Jul 19, 2024 •
edited
Loading

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.

Copy link
Collaborator

@singalsu singalsu Jul 19, 2024

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.

Copy link
Collaborator

@marc-hb marc-hb Jul 20, 2024 •
edited
Loading

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.

Copy link
Collaborator

@marc-hb marc-hb Jul 20, 2024 •
edited
Loading

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/

for (j = i + 1; j < cd->config->num_mic_locations; j++) {
dx = cd->mic_locations[i].x - cd->mic_locations[j].x;
dy = cd->mic_locations[i].y - cd->mic_locations[j].y;
dz = cd->mic_locations[i].z - cd->mic_locations[j].z;
Expand Down Expand Up @@ -157,9 +157,9 @@ 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)
Copy link
Collaborator

@singalsu singalsu Jul 19, 2024

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.

*/
for (i = 0; i < num_mic_locations - 3; i++) {
Copy link
Collaborator

@singalsu singalsu Jun 7, 2024

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.

Copy link
Collaborator

@lyakh lyakh Jun 24, 2024

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)

for (i = 0; i < num_mic_locations - 2; i++) {
a = cd->mic_locations[i + 1].x - cd->mic_locations[i].x;
b = cd->mic_locations[i + 1].y - cd->mic_locations[i].y;
c = cd->mic_locations[i + 1].z - cd->mic_locations[i].z;
Expand Down Expand Up @@ -282,9 +282,9 @@ 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);
Copy link
Collaborator

@singalsu singalsu Jun 7, 2024

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.

p += ch_count;
tdfb_cinc_s16(&p, cd->direction.d_end, cd->direction.d_size);
tmp += ((int64_t)s * s);
}

/* Calculate mean square power */
Expand Down