Audio: Optimize and fix TDFB direction calculation #9169

ShriramShastry · 2024-05-27T11:32:35Z

This check-in refines the TDFB direction calculation, addressing both
performance and correctness:

Fixes:
Correct infinite loop in max_mic_distance by fixing loop conditions.
Optimizations:
Fix off-by-one error in line_array_mode_check ensuring all checks
for co-linearity among microphone locations are performed.

singalsu · 2024-05-27T15:39:05Z

It's not clear if you want to keep #9160, or want to include the previous change into this PR (and close #9160). I think separate PR is better, so you should keep #9160 and remove the first commit from this PR.

src/audio/tdfb/tdfb_direction.c

singalsu · 2024-06-07T08:14:12Z

src/audio/tdfb/tdfb_direction.c

@@ -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++) {


Put this into a separate commit. Since this would be a bug fix. Previous change is an optimization for initialize code.

btw, I think there's also a typo in the comment in line 158 - it should be y(i), not y(1)

singalsu · 2024-06-07T08:23:38Z

src/audio/tdfb/tdfb_direction.c

@@ -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);


Good catch, without the Q1.15 multiply shifts this indeed fits the int32_t type. Please put this also to to own commit.

lgirdwood

@ShriramShastry LGTM, but needs @singalsu to approve. Btw, what is the MCPS or % speed up ?

lyakh · 2024-06-12T12:25:37Z

src/audio/tdfb/tdfb_direction.c

-		/* 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)


this commit mostly reverts the previous one. It seems quite broken

Done ! Please take a look. Thanks

lgirdwood

@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.

src/audio/tdfb/tdfb_direction.c

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>

lyakh

code changes look good now, just need to improve comments

src/audio/tdfb/tdfb_direction.c

lyakh · 2024-06-24T06:20:06Z

src/audio/tdfb/tdfb_direction.c

@@ -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++) {


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

lyakh · 2024-06-25T06:13:59Z

src/audio/tdfb/tdfb_direction.c

-		/* 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
+		 */


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

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

Process: investigate GitHub code review replacements zephyrproject-rtos/zephyr#39194

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

@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.

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.

I think you missed the last sentence:

JUST A WARNING that this issue will keep coming again and again.

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/

lgirdwood

Lets wait for @singalsu to return before we merge.

singalsu · 2024-07-19T08:24:12Z

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.

singalsu · 2024-07-19T08:56:39Z

src/audio/tdfb/tdfb_direction.c

-		/* 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
+		 */


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.

singalsu · 2024-07-19T08:58:03Z

src/audio/tdfb/tdfb_direction.c

@@ -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)


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.

singalsu

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.

singalsu · 2024-07-19T09:58:37Z

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.

kv2019i · 2024-08-05T07:42:35Z

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 !

ShriramShastry requested a review from lyakh May 27, 2024 12:15

ShriramShastry marked this pull request as ready for review May 27, 2024 12:15

ShriramShastry requested review from singalsu, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners May 27, 2024 12:15

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch 2 times, most recently from 62dc1ef to e219c00 Compare May 28, 2024 07:00

ShriramShastry mentioned this pull request May 28, 2024

Audio: Use generic saturation logic for improved efficiency #9170

Closed

singalsu requested changes Jun 7, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch 2 times, most recently from 95175bd to 049c690 Compare June 11, 2024 12:35

lgirdwood reviewed Jun 11, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch from 049c690 to 2a328dc Compare June 11, 2024 13:46

ShriramShastry requested a review from singalsu June 12, 2024 03:14

lyakh requested changes Jun 12, 2024

View reviewed changes

lgirdwood reviewed Jun 12, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch 2 times, most recently from 3e7956e to 136faf7 Compare June 21, 2024 06:11

ShriramShastry requested a review from lyakh June 21, 2024 06:14

lyakh requested changes Jun 21, 2024

View reviewed changes

src/audio/tdfb/tdfb_direction.c Show resolved Hide resolved

ShriramShastry added 2 commits June 23, 2024 19:40

Audio: Bugfix: Optimize inner loop in max_mic_distance calculation

77daf1a

Start inner loop from i+1 to halve iterations and eliminate redundant checks. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>

Audio: Bugfix: Correct loop boundary in line_array_mode_check

8adadca

Adjust loop boundary to ensure correct number of elements are processed. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch from 136faf7 to 8767df4 Compare June 23, 2024 15:53

ShriramShastry requested a review from lyakh June 23, 2024 16:20

lyakh reviewed Jun 24, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_tdfb_direction_calc branch from 8767df4 to ee2bdab Compare June 24, 2024 08:08

ShriramShastry requested a review from lyakh June 24, 2024 08:10

lyakh approved these changes Jun 25, 2024

View reviewed changes

lgirdwood reviewed Jun 25, 2024

View reviewed changes

lgirdwood added this to the v2.11 milestone Jun 25, 2024

singalsu reviewed Jul 19, 2024

View reviewed changes

singalsu approved these changes Jul 19, 2024

View reviewed changes

kv2019i merged commit a4c44ad into thesofproject:main Aug 5, 2024
42 of 46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: Optimize and fix TDFB direction calculation #9169

Audio: Optimize and fix TDFB direction calculation #9169

ShriramShastry commented May 27, 2024 •

edited

Loading

singalsu commented May 27, 2024

singalsu Jun 7, 2024

lyakh Jun 24, 2024

singalsu Jun 7, 2024

lgirdwood left a comment

lyakh Jun 12, 2024

ShriramShastry Jun 21, 2024

lgirdwood left a comment

lyakh left a comment

lyakh Jun 24, 2024

lyakh Jun 25, 2024

marc-hb Jul 19, 2024 •

edited

Loading

lyakh Jul 19, 2024 •

edited

Loading

singalsu Jul 19, 2024

marc-hb Jul 20, 2024 •

edited

Loading

marc-hb Jul 20, 2024 •

edited

Loading

lgirdwood left a comment

singalsu commented Jul 19, 2024

singalsu Jul 19, 2024

singalsu Jul 19, 2024

singalsu left a comment

singalsu commented Jul 19, 2024

kv2019i commented Aug 5, 2024

Audio: Optimize and fix TDFB direction calculation #9169

Audio: Optimize and fix TDFB direction calculation #9169

Conversation

ShriramShastry commented May 27, 2024 • edited Loading

singalsu commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

lyakh Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

singalsu commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu left a comment

Choose a reason for hiding this comment

singalsu commented Jul 19, 2024

kv2019i commented Aug 5, 2024

ShriramShastry commented May 27, 2024 •

edited

Loading

marc-hb Jul 19, 2024 •

edited

Loading

lyakh Jul 19, 2024 •

edited

Loading

marc-hb Jul 20, 2024 •

edited

Loading

marc-hb Jul 20, 2024 •

edited

Loading