Skip to content

[RFC]selector: validate params in the params callback and fix prepare call… #1070

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

Closed
ranj063 wants to merge 1 commit into from
Closed

[RFC]selector: validate params in the params callback and fix prepare call… #1070

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 49 additions & 28 deletions src/audio/selector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
#include "selector.h"
#include <sof/math/numbers.h>


/**
* \brief Validates channel count and index and sets channel count.
* \details If input data is not supported trace error is displayed and
Expand Down Expand Up @@ -89,7 +88,7 @@ static int sel_set_channel_values(struct comp_data *cd, uint32_t in_channels,

if (ch_idx > (SEL_SOURCE_4CH - 1)) {
trace_selector_error("sel_set_channel_values() error: "
"ch_idx = %u", in_channels);
"ch_idx = %u", ch_idx);
return -EINVAL;
}

Expand All @@ -108,11 +107,10 @@ static int sel_set_channel_values(struct comp_data *cd, uint32_t in_channels,
static struct comp_dev *selector_new(struct sof_ipc_comp *comp)
{
struct comp_dev *dev;
struct sof_ipc_comp_selector *sel;
struct sof_ipc_comp_selector *ipc_sel =
(struct sof_ipc_comp_selector *)comp;
struct sof_ipc_comp_process *sel;
struct sof_ipc_comp_process *ipc_sel =
(struct sof_ipc_comp_process *)comp;
struct comp_data *cd;
int ret;

trace_selector("selector_new()");

Expand All @@ -122,12 +120,12 @@ static struct comp_dev *selector_new(struct sof_ipc_comp *comp)
}

dev = rzalloc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM,
COMP_SIZE(struct sof_ipc_comp_selector));
COMP_SIZE(struct sof_ipc_comp_process));
if (!dev)
return NULL;

sel = (struct sof_ipc_comp_selector *)&dev->comp;
memcpy(sel, ipc_sel, sizeof(struct sof_ipc_comp_selector));
sel = (struct sof_ipc_comp_process *)&dev->comp;
memcpy(sel, ipc_sel, sizeof(struct sof_ipc_comp_process));

cd = rzalloc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM, sizeof(*cd));
if (!cd) {
Expand All @@ -137,16 +135,6 @@ static struct comp_dev *selector_new(struct sof_ipc_comp *comp)

comp_set_drvdata(dev, cd);

/* verification of initial parameters */
ret = sel_set_channel_values(cd, ipc_sel->input_channels_count,
ipc_sel->output_channels_count,
ipc_sel->selected_channel);
if (ret < 0) {
rfree(cd);
rfree(dev);
return NULL;
}

dev->state = COMP_STATE_READY;
return dev;
}
Expand Down Expand Up @@ -175,9 +163,42 @@ static void selector_free(struct comp_dev *dev)
static int selector_params(struct comp_dev *dev)
{
struct comp_data *cd = comp_get_drvdata(dev);
struct comp_buffer *sourceb, *sinkb;
struct comp_dev *source, *sink;

trace_selector("selector_params()");

/* selector component has only 1 source and 1 sink buffer */
sourceb = list_first_item(&dev->bsource_list, struct comp_buffer,
sink_list);

sinkb = list_first_item(&dev->bsink_list, struct comp_buffer,
source_list);

/* verification of channel counts */
/*
* FIXME: where should sel_channel come from?
* from a kcontrol or ipc params
*/
ret = sel_set_channel_values(cd, sourceb->source->params.channels,

@tlauda tlauda Mar 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Source/sink params (depends on pipe direction) won't be set at this point, so your code won't work correctly. Selector component should overwrite next components' params, so it needs those in/out channels count parameters.

@ranj063 ranj063 Mar 2, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@tlauda yes you are right about this. I think we should set this in prepare() instead of params. Then the source/sink params would already be set.

@tlauda tlauda Mar 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ranj063 Getting in/out channels based on the connected components is not a great idea. The purpose of the selector component is to force the change. Let's assume that we have playback pipeline and in the middle of the pipeline we have the component, which only requires 1 channel. We would put selector before that component to get that 1 channel, but how the selector would know that it should change the out channels? Params from the host would set the host format, so basically all the components would have e.g. 2 channels.
In the long term we should think about changing component API to have something like in and out format for every comp. With such API your change would be valid and expected.

@ranj063 ranj063 Mar 3, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@tlauda params from the host would change the channels but if a component knows it only needs 1 channel, it will ignore the channel from the host params and set it to 1 anyway.

@tlauda tlauda Mar 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ranj063 Such component can support multiple formats and 1 channel on input can be just for specific topology. With your approach we transfer responsibility to that component instead of selector, which would require for that component to have additional custom field anyway.

@ranj063 ranj063 Mar 3, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@tlauda what custom field would that be? Take the detect component for example, it knows that it only needs 1 channel. and the selector can use this information from the detect comp.

@tlauda tlauda Mar 4, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ranj063 I'm thinking more about generic case, where you have components, which can work for every channel configuration, but for specific topology they require specific channel number. One of the components in the topology needs to decide, that the channels number must be reduced.

@lbetlej lbetlej Mar 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ranj063 @tlauda Regarding component input parameters: one of the component design assumptions was that output channel ID (index) will be selected in the topology w/o requirement to have a dedicated app (the latter could be supported as an optional/ additional configuration channel)

@lbetlej lbetlej Mar 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ranj063 I'll second @tlauda statement that "In the long term we should think about changing component API to have something like in and out format for every comp" for every component in and out. A classic example is a preprocessing component with AEC that accepts on the 1st input 1-4 channels from mics and 2nd one 1-2 channels of reference signal from playback path. I assume that component can be compiled for platform specific configuration.

sinkb->sink->params.channels,
cd->sel_channel);
if (ret < 0) {
trace_selector_error("selector_params() error: ",
"invalid params for selector"
"input ch count %u output ch count %u"
"selected channel %u",
sourceb->source->params.channels,
sinkb->sink->params.channels,
cd->sel_channel);

return -EINVAL;
}

/* set input/output channel counts based on source/sink params */
cd->out_channels_count = sinkb->sink->params.channels;
cd->in_channels_count = sourceb->source->params.channels;

/* rewrite channels number for other components */
if (dev->params.direction == SOF_IPC_STREAM_PLAYBACK)
dev->params.channels = cd->out_channels_count;
Expand Down Expand Up @@ -359,15 +380,6 @@ static int selector_prepare(struct comp_dev *dev)
trace_selector("selector_prepare(): sink->params.channels = %u",
sinkb->sink->params.channels);

/* set downstream buffer size */
ret = buffer_set_size(sinkb, cd->sink_period_bytes *
config->periods_sink);
if (ret < 0) {
trace_selector_error("selector_prepare() error: "
"buffer_set_size() failed");
goto err;
}

/* validate */
if (cd->sink_period_bytes == 0) {
trace_selector_error("selector_prepare() error: "
Expand All @@ -389,6 +401,15 @@ static int selector_prepare(struct comp_dev *dev)
goto err;
}

/* set downstream buffer size */
ret = buffer_set_size(sinkb, cd->sink_period_bytes *
config->periods_sink);
if (ret < 0) {
trace_selector_error("selector_prepare() error: "
"buffer_set_size() failed");
goto err;
}

cd->sel_func = sel_get_processing_function(dev);
if (!cd->sel_func) {
trace_selector_error("selector_prepare() error: "
Expand Down