-
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 sink checks in crossover_s32_default #9178
Audio: Optimize sink checks in crossover_s32_default #9178
Conversation
aeb61cb
to
f32aebb
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.
ok, this optimisation avoids checking inactive sinks for each frame on each channel. So yes, it should save some time when there are inactive sinks. But please restore the original index calculation
} | ||
|
||
idx += nch; | ||
} |
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.
indentation
for (i = 0; i < frames; i++) { | ||
x = audio_stream_read_frag_s32(source_stream, idx); | ||
/* Read source */ | ||
x = audio_stream_read_frag_s32(source_stream, ch + i * nch); |
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.
you replaced an idx += nch
addition with an addition and a multiplication ch + i * nch
- subject to any compiler optimisations. Depending on those optimisations this might or might not end up generating different code, but I personally don't think this would be faster. I'd keep the original addition, personally, I prefer this form:
for (i = 0, idx = ch; i < frames; i++, idx += nch) {
but that might be a matter of taste. The original code was run-time equivalent to this, don't think your version is an improvement.
x = audio_stream_read_frag_s32(source_stream, idx); | ||
/* Read source */ | ||
x = audio_stream_read_frag_s32(source_stream, ch + i * nch); | ||
/* Apply crossover split */ |
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.
does this comment add any information?
idx); | ||
/* Write output to active sinks */ | ||
for (j = 0; j < active_sinks; j++) { | ||
y = audio_stream_write_frag_s32(sink_stream[j], ch + i * nch); |
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.
and now you have this calculation twice? No, please, remove this.
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.
Thank you for your code review feedback. I reconsidered the index increment approach and agree that incrementing the index directly within the loop construct is both stylistically clearer and potentially more efficient because it eliminates unnecessary multiplication. Regarding the comment, I recognise that it is redundant and should be removed for clarity. I'll make these changes accordingly.
f32aebb
to
4e0131a
Compare
4e0131a
to
66b65fb
Compare
for (i = 0; i < frames; i++) { | ||
/* Iterate over frames */ | ||
for (i = 0, idx = ch; i < frames; i++, idx += nch) { | ||
/* Read source */ |
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.
indentation - there's a spurious space in front of the comment
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, Thanks.
issue addressed, thanks, would be nice to clean up indentation though
66b65fb
to
451f364
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.
A few minor style comments inline, but the optimization itself seems good and gets and check out of the inner loop.
451f364
to
e9c1e3a
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.
Best to split this PR in patches that each have one change. Easiest way to do this quickly is to git reset
on to origin/main HEAD (this will leave all your changes as a diff and not as patches), then use git gui
to individually pick lines for each patch.
for (j = 0; j < num_sinks; j++) { | ||
if (bsinks[j]) | ||
sink_stream[active_sinks++] = bsinks[j]->data; | ||
} | ||
|
||
/* Process for each channel */ | ||
for (ch = 0; ch < nch; ch++) { | ||
idx = ch; | ||
/* Set current crossover state */ | ||
state = &cd->state[ch]; | ||
for (i = 0; i < frames; i++) { | ||
/* Iterate over frames */ | ||
for (i = 0, idx = ch; i < frames; i++, idx += nch) { | ||
/* Read source */ | ||
x = audio_stream_read_frag_s32(source_stream, idx); | ||
cd->crossover_split(*x, out, state); | ||
|
||
for (j = 0; j < num_sinks; j++) { | ||
if (!bsinks[j]) | ||
continue; | ||
sink_stream = bsinks[j]->data; | ||
y = audio_stream_write_frag_s32(sink_stream, | ||
idx); | ||
/* Write output to active sinks */ | ||
for (j = 0; j < active_sinks; j++) { | ||
y = audio_stream_write_frag_s32(sink_stream[j], idx); | ||
/* Copy processed data to sink */ |
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.
Best to split this, as this part is more than the commit message. I would make this a second patch that states the reason and any performance improvement data.
@@ -225,36 +225,29 @@ static void crossover_s32_default(struct comp_data *cd, | |||
int32_t num_sinks, | |||
uint32_t frames) | |||
{ | |||
/* Array to hold active sink streams; initialized to null */ |
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.
all this commit does is delete the comments you added, please check your commits to make sure they make sense before pushing them.
e9c1e3a
to
a41f27d
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.
all my previous comments are still valid, please fix them
a41f27d
to
833cfb6
Compare
Optimize the function by pre-initializing active sinks and adjusting loops for better performance and readability. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>
Add Doxygen comments to describe the parameters and functionality of the crossover_s32_default function. This helps in understanding the function's flow and the purpose of each section. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>
833cfb6
to
d000d67
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.
Thank you for reviewing the change. I addressed the review comments. Please have a look.
Optimize
crossover_s32_default
by pre-determining the active sinks before processing frames. This check-in reduces redundant null checks for each frame and channel, leading to performance improvements in processing audio streams with multiple sinks.