Skip to content
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

Jump to bottom

module_adapter: fix buffer consumption logic in simple_copy mode #6909

Closed
wants to merge 1 commit into from
Closed

module_adapter: fix buffer consumption logic in simple_copy mode #6909

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/audio/module_adapter/module_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,7 @@ int module_adapter_copy(struct comp_dev *dev)
size_t size = MAX(mod->deep_buff_bytes, mod->period_bytes);
uint32_t min_free_frames = UINT_MAX;
uint32_t num_input_buffers = 0;
unsigned int skip_list = 0;
int ret, i = 0;

comp_dbg(dev, "module_adapter_copy(): start");
Expand All @@ -652,6 +653,7 @@ int module_adapter_copy(struct comp_dev *dev)

/* check if the source dev is in the same state as the dev */
if (!source_c[i]->source || source_c[i]->source->state != dev->state) {
skip_list |= BIT(i);
i++;
continue;
}
Expand Down Expand Up @@ -730,12 +732,18 @@ int module_adapter_copy(struct comp_dev *dev)
}

if (mod->simple_copy) {
int mod_input = 0;
i = 0;
list_for_item(blist, &dev->bsource_list) {
comp_update_buffer_consume(source_c[i], mod->input_buffers[i].consumed);
/* some sources may be skipped, so skip_list must be used */
if (!(skip_list & BIT(i))) {
Copy link
Collaborator

@ranj063 ranj063 Jan 4, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this still problematic? Now you've got the wrong consumed value right? What we need to do instead is iterate through 0 -> num_source_buffers and skip those with a different state isnt it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 I'm open to better ways to do this, but wanted to keep this small to avoid conflicts with your #6836 (but I think we are anyways conflicting already).

But. but. I think this is right. The module process sees "num_input_buffers" of input buffers and reports consumed on each of them. In this loop, we iteratate over these buffers using "mod_input". The "i" variable goes over all bsources.

The consumed is updated in module_process() using the "mod->input_buffers, num_input_buffers" passed as inputs. Thus to pair right source with consumed value, we need to use "input_buffers[mod_input].consumed" as that's the buffer we passed for source "source_c[i]" and specific "i != mod_input" when any sources are skipped.

I can see values of 3+ million bytes passed as argument to comp_update_buffer_consume() when I run pause-resume tests on current main with current code and this PR fixes that. With asserts not enabled, this just silently leads to undefined behaviours.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 This is mimicking the loop on L650, but we can't do the state check here again as the state might have changed (unlikely but would be a very hard to debug problem), and rather we should stick to the decision we do on L655. If a bsource is skipped, we ignore it when calling the consume after process as well.

comp_update_buffer_consume(source_c[i],
mod->input_buffers[mod_input].consumed);
mod->input_buffers[mod_input].size = 0;
mod->input_buffers[mod_input].consumed = 0;
mod_input++;
}
buffer_release(source_c[i]);
mod->input_buffers[i].size = 0;
mod->input_buffers[i].consumed = 0;
i++;
}
buffer_release(sink_c);
Expand Down