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.

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

demux: Support inactive cross-pipeline sinks #8619

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

andyross
Copy link
Contributor

[Split out from #8571 for separate review]

It may happen that a demux is configured to write to a buffer in a stopped pipeline (consider echo cancellation when the mic is not in use: the output reference stream has nowhere to go). This doesn't work in the tree currently; the general pipeline design is to try to turn on pipes that are required, but that's incomplete for the case (again, echo cancellation) where the pipelines are oriented in different directions and in any case is undesirable as a microphone stream shouldn't be required to be active for playback to work.

Instead, detect the situation at PRE_START time, configure the output buffers overrun_permitted (as there may not be a reader), and drop the unconfigured streams dynamically at process() time, as the target pipeline may be started up at arbitrary moments. When it starts, we'll begin emitting output at the first process callback as desired.

It may happen that a demux is configured to write to a buffer in a
stopped pipeline (consider echo cancellation when the mic is not in
use: the output reference stream has nowhere to go).  This doesn't
work in the tree currently; the general pipeline design is to try to
turn on pipes that are required, but that's incomplete for the case
(again, echo cancellation) where the pipelines are oriented in
different directions and in any case is undesirable as a microphone
stream shouldn't be required to be active for playback to work.

Instead, detect the situation at PRE_START time, configure the output
buffers overrun_permitted (as there may not be a reader), and drop the
unconfigured streams dynamically at process() time, as the target
pipeline may be started up at arbitrary moments.  When it starts,
we'll begin emitting output at the first process callback as desired.

Signed-off-by: Andy Ross <andyross@google.com>
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

One may argue the need to run the capture pipe was/is a feature not a bug, but I can certainly see value in being more flexible w.r.t. a stopped pipeline.

Added a few reviewers. CI coverage for this especially in IPC3 demux is not great, so hopefully we can get some IPC3 eyes on this. Looks good based on code review.

Copy link
Contributor

@btian1 btian1 left a comment

Choose a reason for hiding this comment

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

should use two patches?

  1. add sinks_stream[i]
  2. add demux trigger

@andyross
Copy link
Contributor Author

should use two patches?

IMHO no? There's no working/bisectable configuration that results from just one of them, you need both to get working output.

@kv2019i kv2019i merged commit 33ef5f6 into thesofproject:main Dec 18, 2023
40 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants