Repository navigation
mixer: add logic to maintain data flow when sources are paused - #5628
Conversation
There was a problem hiding this comment.
This fills the amount the sink can fit. Have you checked how much is frames in practice, is it 48 for for 1 ms scheduling? Or does first copy when all sources become inactive push a lot more silence to sink buffer? It might increase buffering delay if some source would become again active. But on this side it's hard to know what is the just sufficient amount to prevent xrun so this is a safe assumption.
There was a problem hiding this comment.
@singalsu This is a good question. The existing code to output silence uses this same logic, i.e. it will as much silence as the sink can fit. I used the same approach here. While running sof-test that trigger this path, it's still usually 48 frames this routine write. So we'd write more if we are really running behind and then it probably makes sense to write more silence to avoid a DAI XRUNs. When the upstream components (that provide data to mixer) are started, there is probably risk of some extra delays in the initial copy() iterations of those components (shouldn't happen but the risk is still there), so definitely not a good time to run low on buffering towards the DAI(s).
There was a problem hiding this comment.
yes, see my comment above - the MIN() above is bogus, so the normal silence playback does the same
|
@RanderWang Can you double-check this as well. This should leave IPC4 unaffected, but good to double-check. |
Yes please @RanderWang - can you test on Windows ? |
@plbossart is there a reason we do this ? I'm assuming it's to support a codec that needs a running BCLK/FRAME. Logic would be a lot simpler if we reused the STOP flow (this now can emit the early/late BCLK and FRAME if needed).. |
There was a problem hiding this comment.
unrelated to this PR, but actually this isn't needed here - frames is equal to INT32_MAX here, so, the result is pretty obvious
There was a problem hiding this comment.
yes, see my comment above - the MIN() above is bogus, so the normal silence playback does the same
sure, I reviewed the PR and confirm it will not affect IPC4 path |
|
@lyakh @RanderWang @singalsu New version uploaded, please review. This now reuses a single code block for all silence generation scenarios and drops the unnecessary moving of the static function (not sure why I had that in V1). I kept this small and isolated and did not go about changing the name of "num_mix_sources". |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
see if you want to change comp_info() to comp_dbg() but approving regardless, that's a minor thing
In a topology where mixer is connected to multiple source pipelines, the mixer<->DAI downstream pipeline for playback is kept running when sources are put to PAUSED state. This design means that mixer_copy() needs to continue to produce output samples as the DAI is kept running as well. There is existing code to generate silence when all sources are PAUSED. A problem remains in the sequence when sources are RELEASED. On the first iteration after RELEASE, the source state can be changed (so mixer sees non-zero num_mix_sources). But the sources may not have any data available on the first cycle. This can lead to XRUNs observed at the DAI as the mixer is not continuously producing data in ACTIVE state. Address this issue by adding a "sources_inactive" state to mixer. When all sources are inactive, mixer enters this state. When sources are resumed, mixer will note this state, and continue to generate silence (on its output) until at least one source starts generating audio data. This ensures continuous audio output to the mixer downstream pipeline. The change is not sufficient on its own, but is part of the fixes to address: thesofproject#5469 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
In a topology where mixer is connected to multiple source pipelines, the
mixer<->DAI downstream pipeline for playback is kept running when
sources are put to PAUSED state.
This design means that mixer_copy() needs to continue to produce output
samples as the DAI is kept running as well. There is existing code to
generate silence when all sources are PAUSED.
A problem remains in the sequence when sources are RELEASED. On the
first iteration after RELEASE, the source state can be changed (so mixer
sees non-zero num_mix_sources). But the sources may not have any data
available on the first cycle. This can lead to XRUNs observed at the DAI
as the mixer is not continuously producing data in ACTIVE state.
Address this issue by adding a "sources_inactive" state to mixer. When
all sources are inactive, mixer enters this state. When sources are
resumed, mixer will note this state, and continue to generate silence
(on its output) until at least one source starts generating audio data.
This ensures continuous audio output to the mixer downstream pipeline.
The change is not sufficient on its own, but is part of the fixes
to address: #5469
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com