P20 selector - convert the selector module to only use the sink/source by piotrhoppeintel · Pull Request #11243 · thesofproject/sof

piotrhoppeintel · 2026-09-28T11:51:51Z

Rework the selector module to only use the sink/source api to
prepare the SOF for the full transition to pipeline 2.0.

Add release_source_and_commit_sink() to release processed source data and commit produced data to the sink. Always commit the sink, while preserving the source release error when both operations fail. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

Copilot

Copilot review overview

🟡 Changes recommended

A critical zero-frame handling issue and documentation mismatches remain unresolved.

Review effort: Lite
Findings: 1

Open (1)

Skip selector callbacks when no frames are available · New

What changed in this PR

Migrates the selector module to sink/source APIs for Pipeline 2.0 compatibility.

Changes:

Updates selector callbacks and generic processing.
Adds shared source-release/sink-commit utilities.
Adapts selector tests to the new interfaces.

File	Summary
`test/cmocka/src/audio/selector/selector_test.c`	Updated selector tests for sink/source APIs.
`src/include/sof/audio/sink_source_utils.h`	Declares the shared utility.
`src/include/sof/audio/selector.h`	Updates selector callback signatures.
`src/audio/sink_source_utils.c`	Implements source-release and sink-commit handling.
`src/audio/selector/selector.c`	Migrates processing; zero-frame handling requires correction.
`src/audio/selector/selector_generic.c`	Updates processing implementations; Doxygen parameter names need correction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

lgirdwood · 2026-09-28T19:21:25Z

@piotrhoppeintel 1 CI failure to check.

lyakh · 2026-09-29T06:32:54Z

+	int sink_ret;
+
+	src_ret = source_release_data(source, free_size);
+	sink_ret = sink_commit_buffer(sink, (src_ret ? 0 : commit_size));


superfluous internal parentheses

lyakh · 2026-09-29T06:35:18Z

 #include <limits.h>

+int release_source_and_commit_sink(struct sof_source *source, size_t free_size,
+	struct sof_sink *sink, size_t commit_size)


I'm wondering if it's small enough to be an inline?

lyakh · 2026-09-29T06:39:16Z

+	const unsigned int nch = source_get_channels(source);
+	const int16_t *src, *src_start, *src_ch;
+	int16_t *dst, *dst_start;
+	int src_samples, dst_samples;


I think @softwarecki defines these as size_t too?

Yes, but in this case, src_samples and dst_samples are passed as parameters to the source_get_data_s16() and sink_get_buffer_s16() functions, and they require an int type.

Pending PR #11193

lyakh · 2026-09-29T06:40:27Z

-		src = audio_stream_wrap(source, src + b);
-		dst = audio_stream_wrap(sink, dst + b);
-		bytes_copied += b;
+	const int frame_bytes = source_get_frame_bytes(source);


size_t? Would be good to define and document at least recommended common types for all of these...

Done. Changed the type to size_t.

lyakh · 2026-09-29T06:47:48Z

-	cd->sel_func(sel_state->dev, &sel_state->sink->stream, &sel_state->source->stream,
-		     sel_state->dev->frames);
+	ret = cd->sel_func(sel_state->dev, sink, source, sel_state->dev->frames);
+	assert_int_equal(ret, 0);


would this test be broken after the previous commit? Maybe these commits need to be merged then

This is a new case where 0 should be returned when no frames are being processed. It is related to the changes requested by Copilot.

@piotrhoppeintel sorry, this was a comment to the entire commit, not that hunk specifically.

Specifically, the changes are mainly required by the different change types, and 'source/since' comes from buffers. The 'input_stream_buffer' and 'output_stream_buffer' wrappers are no longer used, as noted above. There were no test failures; these changes are purely technical.

Rework selector processing to use the sink/source API instead of direct audio stream access, preparing the module for the pipeline 2.0 transition. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

Update the selector unit test to pass sof_source and sof_sink objects to the processing functions. Remove obsolete stream buffer wrappers and verify the processing return value. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

softwarecki

Wouldn't it be cleaner to get the buffers once in selector_process() and pass circ_buf_source/circ_buf_sink structures to the individual processing variants? That would avoid duplicating the buffer acquisition code in each processing function.

softwarecki · 2026-09-30T09:26:45Z

-	int sink_frame_bytes = audio_stream_frame_bytes(sink);
-	int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, audio_stream_get_channels(source));
-	int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, audio_stream_get_channels(sink));
+	const int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)source_get_channels(source));


unsigned int

softwarecki · 2026-09-30T09:27:07Z

-	int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, audio_stream_get_channels(sink));
+	const int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)source_get_channels(source));
+	const int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, (int)sink_get_channels(sink));
+	const int source_frame_bytes = source_get_frame_bytes(source);


softwarecki · 2026-09-30T09:27:13Z

-	int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, audio_stream_get_channels(source));
-	int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, audio_stream_get_channels(sink));
+	const int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)source_get_channels(source));
+	const int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, (int)sink_get_channels(sink));


unsigned int

softwarecki · 2026-09-30T09:28:34Z

+	const int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, (int)source_get_channels(source));
+	const int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, (int)sink_get_channels(sink));
+	const int source_frame_bytes = source_get_frame_bytes(source);
+	const unsigned int src_channels = source_get_channels(source);


Move above n_chan_source and use this value to calculate it.

softwarecki · 2026-09-30T09:28:58Z

+	const int source_frame_bytes = source_get_frame_bytes(source);
+	const unsigned int src_channels = source_get_channels(source);
+	const unsigned int dst_channels = sink_get_channels(sink);
+	const int sink_frame_bytes = sink_get_frame_bytes(sink);


Copilot AI lite review requested due to automatic review settings September 28, 2026 11:51

piotrhoppeintel requested review from dbaluta, kv2019i, lbetlej, lgirdwood, mmaka1, plbossart and tlissows as code owners September 28, 2026 11:51

Copilot started reviewing on behalf of piotrhoppeintel September 28, 2026 11:56 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread src/audio/selector/selector.c

piotrhoppeintel force-pushed the p20-selector branch from 3800a54 to ee09d51 Compare September 28, 2026 12:49

lgirdwood approved these changes Sep 28, 2026

View reviewed changes

lyakh reviewed Sep 29, 2026

View reviewed changes

piotrhoppeintel added 2 commits September 29, 2026 09:30

audio: selector: rework module to use sink/source api

3d27b5e

Rework selector processing to use the sink/source API instead of direct audio stream access, preparing the module for the pipeline 2.0 transition. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

test: selector: update unit test for sink/source API

8baa6ed

Update the selector unit test to pass sof_source and sof_sink objects to the processing functions. Remove obsolete stream buffer wrappers and verify the processing return value. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

piotrhoppeintel force-pushed the p20-selector branch from ee09d51 to 8baa6ed Compare September 29, 2026 07:30

softwarecki reviewed Sep 30, 2026

View reviewed changes

Conversation

piotrhoppeintel commented Sep 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

lgirdwood commented Sep 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softwarecki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants