Tools: Topology: Add selector to compress playback pipeline#10618
Tools: Topology: Add selector to compress playback pipeline#10618singalsu wants to merge 2 commits intothesofproject:mainfrom
Conversation
|
| <include/components/micsel/stereo_endpoint_playback_updownmix.conf> | ||
| } | ||
| <include/formats/compr_input_audio_formats_s32_48k.conf> | ||
| <include/formats/compr_output_audio_formats_s32_48k_stereo.conf> |
There was a problem hiding this comment.
If you add the 'micsel' right after the decoder then the module-copier, src, does not need new formats as they would keep receiving stereo?
Or is it better to run with mono on them and convert to stereo as late as possible?
There was a problem hiding this comment.
As far as I understand this convert to stereo late is the only option that kernel can support (and still needs #5694). I tried early convert to stereo but kernel could not handle the formats. Early convert to stereo will be beneficial for 5.1 and 7.1, while this late convert to stereo is optimal for mono content. The first is more critical though. Ideally kernel should support both and let the approach be chosen in topology.
There was a problem hiding this comment.
you cannot swap topology based on how many channels the file you play have...
You could have 2x micsel, the first would support mono and stereo out, the second would support only stereo.
if mono is played then it would pass the micsel as mono, if 5.1/7.1 then it gets converted to stereo. Mono->stereo would be done on the second micsel down the path.
But no idea how this can be done nicely in kernel, it sounds simple, but I don't like to add much heuristics.
There was a problem hiding this comment.
Two micsel would do it but I feel it's too complicated. Early convert to stereo is the better long term solution, while this late convert works with mono and stereo well. And our offload codecs so far are mono/stereo only. I've tested 5.1 and 7.1 with patch #10554 and wav format but the patch still needs work with stream end handling.
There was a problem hiding this comment.
Pull request overview
Adds a selector (micsel) stage to the CAVS compressed playback pipeline so the playback path can be constrained/converted to stereo, and refactors repeated format blocks into reusable include files.
Changes:
- Insert
micselinto thecompr-playbackpipeline graph betweensrcandgain. - Update Intel compressed playback topology (
compr.conf) to use shared format include files and to configure the newmicselwidget/control. - Add new
include/formats/*snippets for compressed pipeline input/output format declarations (including SRC S32→48k and stereo-only output).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/topology/topology2/platform/intel/compr.conf | Refactors format declarations into includes and adds a new micsel widget/control in the compressed playback pipeline instances. |
| tools/topology/topology2/include/pipelines/cavs/compr-playback.conf | Adds micsel component to the pipeline class and routes src -> micsel -> gain. |
| tools/topology/topology2/include/formats/compr_input_audio_formats.conf | New shared input format definitions for host-copier/decoder/module-copier. |
| tools/topology/topology2/include/formats/compr_output_audio_formats.conf | New shared output format definitions for host-copier/decoder. |
| tools/topology/topology2/include/formats/compr_output_audio_formats_s32.conf | New shared S32 output format definitions (e.g., module-copier). |
| tools/topology/topology2/include/formats/compr_input_output_formats_src_s32_to_48k.conf | New SRC-specific input/output format definitions supporting mono+stereo at 48k output. |
| tools/topology/topology2/include/formats/compr_input_audio_formats_s32_48k.conf | New input format definitions intended for the post-SRC stage feeding micsel. |
| tools/topology/topology2/include/formats/compr_output_audio_formats_s32_48k_stereo.conf | New single stereo S32/48k output format definition for micsel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Object.Widget.micsel.1 { | ||
| Object.Control.bytes."1" { | ||
| name '$COMPR_2_PCM_NAME selector bytes' | ||
| <include/components/micsel/stereo_endpoint_playback_updownmix.conf> |
There was a problem hiding this comment.
The referenced include file include/components/micsel/stereo_endpoint_playback_updownmix.conf does not exist in the repository, so topology generation will fail. Add this missing include file (and ensure it defines the intended selector bytes) or switch to an existing micsel profile include (e.g., passthrough/upmix/downmix configs) that matches the desired behavior.
| <include/components/micsel/stereo_endpoint_playback_updownmix.conf> | |
| <include/components/micsel/stereo_endpoint_playback_upmix.conf> |
There was a problem hiding this comment.
same as previous, it exists
| @@ -22,6 +22,7 @@ | |||
| <include/components/pipeline.conf> | |||
| <include/components/gain.conf> | |||
| <include/components/decoder.conf> | |||
There was a problem hiding this comment.
This pipeline instantiates a src widget (src."1" {}) but does not include include/components/src.conf in the header includes. This makes the pipeline definition depend on external include order and is inconsistent with other pipeline templates that include their component classes explicitly. Add the missing src.conf include here to keep the pipeline self-contained.
| <include/components/decoder.conf> | |
| <include/components/decoder.conf> | |
| <include/components/src.conf> |
| Object.Widget.micsel.1 { | ||
| Object.Control.bytes."1" { | ||
| name '$COMPR_PCM_NAME selector bytes' | ||
| <include/components/micsel/stereo_endpoint_playback_updownmix.conf> |
There was a problem hiding this comment.
The referenced include file include/components/micsel/stereo_endpoint_playback_updownmix.conf does not exist in the repository, so topology generation will fail. Add this missing include file (and ensure it defines the intended selector bytes) or switch to an existing micsel profile include (e.g., passthrough/upmix/downmix configs) that matches the desired behavior.
| <include/components/micsel/stereo_endpoint_playback_updownmix.conf> | |
| <include/components/micsel/stereo_endpoint_playback_passthrough.conf> |
There was a problem hiding this comment.
This patch moves formats definitions for the objects in pipeline compr-playback to separate include files in new directory include/formats. It avoids duplication of same input and output formats several times and prepares to add new formats and sample rates later. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The purpose of this patch is to add conversion of possible mono output from decoder to stereo format in the selector (micsel) module. If the decoded output is stereo, the selector goes to pass-through mode with minimal impact to processing load. This patch adds the mono format to all needed objects (host-copier, decoder, module-copier, src, input of selector). Selector has single stereo, S32_LE, 48 kHz output format. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
tplg_add_selector_to_compress
branch
from
eff2fbb to
993f81e
Compare
WIP, this needs #10613 and thesofproject/linux#5694 .
The above are now merged, so I can convert this to non-draft.