[-,extplug,refinement,from,hw_params,1/1] pcm: extplug: Update slave PCM if extplug slave params were changed

Commit Message

Timo Wischer Dec. 6, 2018, 2:31 p.m. UTC

From: Timo Wischer <[e-mail]>

from extplug hw_params() callback function.

This feature is for example required in the following use case:
- A filter plugin supports only 1-8 channels. Therefore it calls
snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 1,
8) to provide the salve PCM parameters limited to 1-8 channels to the
user application
- The user application requests 2 channels. But in this case the slave
PCM will be configured with 1 channel because it is the first valid
configuration
- To update the salve PCM
snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 2,
2) could be called from the extplug hw_params() callback function

Without this patch the call to snd_pcm_extplug_set_slave_param_minmax()
would not have any effect. With this patch the slave device will also be
configured with 2 channels and no down mixing has to be done by the
extplug.

This new feature can also be used for some specific dependencies. For
example a down mixing extplug supports only 2to1 and 4to2 channel down
mixing. Therefore the salve PCM has to be opened with 2 channels if the
user application requests 4 channels.

Signed-off-by: Timo Wischer <[e-mail]>

Comments

Takashi Iwai Dec. 7, 2018, 11:03 a.m. UTC | #1

On Thu, 06 Dec 2018 15:31:43 +0100,
<[e-mail]> wrote:
> 
> From: Timo Wischer <[e-mail]>
> 
> from extplug hw_params() callback function.
> 
> This feature is for example required in the following use case:
> - A filter plugin supports only 1-8 channels. Therefore it calls
> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 1,
> 8) to provide the salve PCM parameters limited to 1-8 channels to the
> user application

Which condition is supposed for this constraint (1-8 channels)?  If
this is about the input to the extplug itself, it should be set via
snd_pcm_extplug_set_param_minmax() instead.

The snd_pcm_extplug_set_slave_param_minmax() is used to restrict the
output from extplug (i.e. the condition to be passed to the slave).
This is needed only when the slave PCM can't have the proper input
constraints.  So usually this sets to a single condition or alike.


Takashi

> - The user application requests 2 channels. But in this case the slave
> PCM will be configured with 1 channel because it is the first valid
> configuration
> - To update the salve PCM
> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 2,
> 2) could be called from the extplug hw_params() callback function
> 
> Without this patch the call to snd_pcm_extplug_set_slave_param_minmax()
> would not have any effect. With this patch the slave device will also be
> configured with 2 channels and no down mixing has to be done by the
> extplug.
> 
> This new feature can also be used for some specific dependencies. For
> example a down mixing extplug supports only 2to1 and 4to2 channel down
> mixing. Therefore the salve PCM has to be opened with 2 channels if the
> user application requests 4 channels.
> 
> Signed-off-by: Timo Wischer <[e-mail]>
> 
> diff --git a/src/pcm/pcm_extplug.c b/src/pcm/pcm_extplug.c
> index 1f887c5..44afadb 100644
> --- a/src/pcm/pcm_extplug.c
> +++ b/src/pcm/pcm_extplug.c
> @@ -26,6 +26,7 @@
>   *
>   */
>    
> +#include <stdbool.h>
>  #include "pcm_local.h"
>  #include "pcm_plugin.h"
>  #include "pcm_extplug.h"
> @@ -43,6 +44,7 @@ typedef struct snd_pcm_extplug_priv {
>  	snd_pcm_extplug_t *data;
>  	struct snd_ext_parm params[SND_PCM_EXTPLUG_HW_PARAMS];
>  	struct snd_ext_parm sparams[SND_PCM_EXTPLUG_HW_PARAMS];
> +	bool sparams_changed;
>  } extplug_priv_t;
>  
>  static const int hw_params_type[SND_PCM_EXTPLUG_HW_PARAMS] = {
> @@ -60,18 +62,49 @@ static const unsigned int excl_parbits[SND_PCM_EXTPLUG_HW_PARAMS] = {
>  					 SND_PCM_HW_PARBIT_FRAME_BITS),
>  };
>  
> +static bool snd_ext_parm_equal(const struct snd_ext_parm * const a,
> +			      const struct snd_ext_parm * const b)
> +{
> +	if (a != b || a->active != b->active || a->num_list != b->num_list)
> +		return false;
> +
> +	if (a->num_list > 0) {
> +		for (unsigned int i = 0; i++; i < a->num_list) {
> +			if (a->list[i] != b->list[i])
> +				return false;
> +		}
> +	} else {
> +		if (a->min != b->min || a->max != b->max)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int snd_ext_parm_set(struct snd_ext_parm *parm, unsigned int min,
> +			    unsigned int max, unsigned int num_list,
> +			    unsigned int *list)
> +{
> +	const struct snd_ext_parm new_parm = {
> +		.min = min,
> +		.max = max,
> +		.num_list = num_list,
> +		.list = list,
> +		.active = 1,
> +	};
> +	const int changed = snd_ext_parm_equal(parm, &new_parm) ? 0 : 1;
> +
> +	free(parm->list);
> +	*parm = new_parm;
> +	return changed;
> +}
> +
>  /*
>   * set min/max values for the given parameter
>   */
>  int snd_ext_parm_set_minmax(struct snd_ext_parm *parm, unsigned int min, unsigned int max)
>  {
> -	parm->num_list = 0;
> -	free(parm->list);
> -	parm->list = NULL;
> -	parm->min = min;
> -	parm->max = max;
> -	parm->active = 1;
> -	return 0;
> +	return snd_ext_parm_set(parm, min, max, 0, NULL);
>  }
>  
>  /*
> @@ -92,11 +125,7 @@ int snd_ext_parm_set_list(struct snd_ext_parm *parm, unsigned int num_list, cons
>  	memcpy(new_list, list, sizeof(*new_list) * num_list);
>  	qsort(new_list, num_list, sizeof(*new_list), val_compar);
>  
> -	free(parm->list);
> -	parm->num_list = num_list;
> -	parm->list = new_list;
> -	parm->active = 1;
> -	return 0;
> +	return snd_ext_parm_set(parm, 0, 0, num_list, new_list);
>  }
>  
>  void snd_ext_parm_clear(struct snd_ext_parm *parm)
> @@ -291,29 +320,37 @@ static int snd_pcm_extplug_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params
>   */
>  static int snd_pcm_extplug_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
>  {
> -
>  	extplug_priv_t *ext = pcm->private_data;
>  	snd_pcm_t *slave = ext->plug.gen.slave;
> -	int err = snd_pcm_hw_params_slave(pcm, params,
> -					  snd_pcm_extplug_hw_refine_cchange,
> -					  snd_pcm_extplug_hw_refine_sprepare,
> -					  snd_pcm_extplug_hw_refine_schange,
> -					  snd_pcm_generic_hw_params);
> -	if (err < 0)
> -		return err;
> -	ext->data->slave_format = slave->format;
> -	ext->data->slave_subformat = slave->subformat;
> -	ext->data->slave_channels = slave->channels;
> -	ext->data->rate = slave->rate;
> -	INTERNAL(snd_pcm_hw_params_get_format)(params, &ext->data->format);
> -	INTERNAL(snd_pcm_hw_params_get_subformat)(params, &ext->data->subformat);
> -	INTERNAL(snd_pcm_hw_params_get_channels)(params, &ext->data->channels);
> -
> -	if (ext->data->callback->hw_params) {
> -		err = ext->data->callback->hw_params(ext->data, params);
> +
> +	do {
> +		ext->sparams_changed = false;
> +
> +		int err = snd_pcm_hw_params_slave(pcm, params,
> +						  snd_pcm_extplug_hw_refine_cchange,
> +						  snd_pcm_extplug_hw_refine_sprepare,
> +						  snd_pcm_extplug_hw_refine_schange,
> +						  snd_pcm_generic_hw_params);
>  		if (err < 0)
>  			return err;
> -	}
> +		ext->data->slave_format = slave->format;
> +		ext->data->slave_subformat = slave->subformat;
> +		ext->data->slave_channels = slave->channels;
> +		ext->data->rate = slave->rate;
> +		INTERNAL(snd_pcm_hw_params_get_format)(
> +				params, &ext->data->format);
> +		INTERNAL(snd_pcm_hw_params_get_subformat)(
> +				params, &ext->data->subformat);
> +		INTERNAL(snd_pcm_hw_params_get_channels)(
> +				params, &ext->data->channels);
> +
> +		if (ext->data->callback->hw_params) {
> +			err = ext->data->callback->hw_params(ext->data, params);
> +			if (err < 0)
> +				return err;
> +		}
> +	} while (ext->sparams_changed);
> +
>  	return 0;
>  }
>  
> @@ -411,6 +448,7 @@ static void clear_ext_params(extplug_priv_t *ext)
>  	int i;
>  	for (i = 0; i < SND_PCM_EXTPLUG_HW_PARAMS; i++) {
>  		snd_ext_parm_clear(&ext->params[i]);
> +		ext->sparams_changed |= ext->sparams[i].active;
>  		snd_ext_parm_clear(&ext->sparams[i]);
>  	}
>  }
> @@ -637,7 +675,9 @@ parameters are sample format and channels.
>  To define the constraints of the slave PCM configuration, use
>  either #snd_pcm_extplug_set_slave_param_minmax() and
>  #snd_pcm_extplug_set_slave_param_list().  The arguments are as same
> -as former functions.
> +as former functions. Both functions can also be called from the hw_params
> +callback. This can be used to further limit the configuration of the slave
> +device depending on the configuration of the user device.
>  
>  To clear the parameter constraints, call #snd_pcm_extplug_params_reset()
>  function. 
> @@ -768,11 +808,17 @@ void snd_pcm_extplug_params_reset(snd_pcm_extplug_t *extplug)
>  int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, unsigned int num_list, const unsigned int *list)
>  {
>  	extplug_priv_t *ext = extplug->pcm->private_data;
> +	int changed = 0;
> +
>  	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
>  		SNDERR("EXTPLUG: invalid parameter type %d", type);
>  		return -EINVAL;
>  	}
> -	return snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> +	changed = snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> +	if (changed > 0)
> +		ext->sparams_changed = true;
> +
> +	return changed;
>  }
>  
>  /**
> @@ -790,6 +836,8 @@ int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, u
>  int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type, unsigned int min, unsigned int max)
>  {
>  	extplug_priv_t *ext = extplug->pcm->private_data;
> +	int changed = 0;
> +
>  	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
>  		SNDERR("EXTPLUG: invalid parameter type %d", type);
>  		return -EINVAL;
> @@ -798,7 +846,11 @@ int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type,
>  		SNDERR("EXTPLUG: invalid parameter type %d", type);
>  		return -EINVAL;
>  	}
> -	return snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> +	changed = snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> +	if (changed > 0)
> +		ext->sparams_changed = true;
> +
> +	return changed;
>  }
>  
>  /**
> -- 
> 2.7.4
>

Timo Wischer Dec. 7, 2018, 11:45 a.m. UTC | #2

On 12/7/18 12:03, Takashi Iwai wrote:
> On Thu, 06 Dec 2018 15:31:43 +0100,
> <[e-mail]> wrote:
>> From: Timo Wischer <[e-mail]>
>>
>> from extplug hw_params() callback function.
>>
>> This feature is for example required in the following use case:
>> - A filter plugin supports only 1-8 channels. Therefore it calls
>> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 1,
>> 8) to provide the salve PCM parameters limited to 1-8 channels to the
>> user application
> Which condition is supposed for this constraint (1-8 channels)?  If
> this is about the input to the extplug itself, it should be set via
> snd_pcm_extplug_set_param_minmax() instead.
>
> The snd_pcm_extplug_set_slave_param_minmax() is used to restrict the
> output from extplug (i.e. the condition to be passed to the slave).
> This is needed only when the slave PCM can't have the proper input
> constraints.  So usually this sets to a single condition or alike.


When using snd_pcm_extplug_set_param_minmax() it will ignore the 
capabilities of the slave device. For example the salve device supports 
only 2 channels but the extplug supports 1-8 channels and uses 
snd_pcm_extplug_set_param_minmax(CHANNELS, 1, 8).
So the user application can open the device with 8 channels and the 
extplug has to down mix to 2 channels. But I do not want to support down 
mixing in my extplug. I only want to limit the capabilities of the slave 
device with the limitations of my extplug and forward this to the user 
application.

PS: currently I am investigating in an issue of this patch in case of 
MMAP access where snd_pcm_hw_params() fails with EBUSY because 
snd_pcm_mmap() is called twice. Therefore please do not merge this 
version of the patch.

But anyway I am interested whether you are fine with this approach, now 
or I have to find another approach.

Best regards

Timo

>
>
> Takashi
>
>> - The user application requests 2 channels. But in this case the slave
>> PCM will be configured with 1 channel because it is the first valid
>> configuration
>> - To update the salve PCM
>> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 2,
>> 2) could be called from the extplug hw_params() callback function
>>
>> Without this patch the call to snd_pcm_extplug_set_slave_param_minmax()
>> would not have any effect. With this patch the slave device will also be
>> configured with 2 channels and no down mixing has to be done by the
>> extplug.
>>
>> This new feature can also be used for some specific dependencies. For
>> example a down mixing extplug supports only 2to1 and 4to2 channel down
>> mixing. Therefore the salve PCM has to be opened with 2 channels if the
>> user application requests 4 channels.
>>
>> Signed-off-by: Timo Wischer <[e-mail]>
>>
>> diff --git a/src/pcm/pcm_extplug.c b/src/pcm/pcm_extplug.c
>> index 1f887c5..44afadb 100644
>> --- a/src/pcm/pcm_extplug.c
>> +++ b/src/pcm/pcm_extplug.c
>> @@ -26,6 +26,7 @@
>>    *
>>    */
>>     
>> +#include <stdbool.h>
>>   #include "pcm_local.h"
>>   #include "pcm_plugin.h"
>>   #include "pcm_extplug.h"
>> @@ -43,6 +44,7 @@ typedef struct snd_pcm_extplug_priv {
>>   	snd_pcm_extplug_t *data;
>>   	struct snd_ext_parm params[SND_PCM_EXTPLUG_HW_PARAMS];
>>   	struct snd_ext_parm sparams[SND_PCM_EXTPLUG_HW_PARAMS];
>> +	bool sparams_changed;
>>   } extplug_priv_t;
>>   
>>   static const int hw_params_type[SND_PCM_EXTPLUG_HW_PARAMS] = {
>> @@ -60,18 +62,49 @@ static const unsigned int excl_parbits[SND_PCM_EXTPLUG_HW_PARAMS] = {
>>   					 SND_PCM_HW_PARBIT_FRAME_BITS),
>>   };
>>   
>> +static bool snd_ext_parm_equal(const struct snd_ext_parm * const a,
>> +			      const struct snd_ext_parm * const b)
>> +{
>> +	if (a != b || a->active != b->active || a->num_list != b->num_list)
>> +		return false;
>> +
>> +	if (a->num_list > 0) {
>> +		for (unsigned int i = 0; i++; i < a->num_list) {
>> +			if (a->list[i] != b->list[i])
>> +				return false;
>> +		}
>> +	} else {
>> +		if (a->min != b->min || a->max != b->max)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static int snd_ext_parm_set(struct snd_ext_parm *parm, unsigned int min,
>> +			    unsigned int max, unsigned int num_list,
>> +			    unsigned int *list)
>> +{
>> +	const struct snd_ext_parm new_parm = {
>> +		.min = min,
>> +		.max = max,
>> +		.num_list = num_list,
>> +		.list = list,
>> +		.active = 1,
>> +	};
>> +	const int changed = snd_ext_parm_equal(parm, &new_parm) ? 0 : 1;
>> +
>> +	free(parm->list);
>> +	*parm = new_parm;
>> +	return changed;
>> +}
>> +
>>   /*
>>    * set min/max values for the given parameter
>>    */
>>   int snd_ext_parm_set_minmax(struct snd_ext_parm *parm, unsigned int min, unsigned int max)
>>   {
>> -	parm->num_list = 0;
>> -	free(parm->list);
>> -	parm->list = NULL;
>> -	parm->min = min;
>> -	parm->max = max;
>> -	parm->active = 1;
>> -	return 0;
>> +	return snd_ext_parm_set(parm, min, max, 0, NULL);
>>   }
>>   
>>   /*
>> @@ -92,11 +125,7 @@ int snd_ext_parm_set_list(struct snd_ext_parm *parm, unsigned int num_list, cons
>>   	memcpy(new_list, list, sizeof(*new_list) * num_list);
>>   	qsort(new_list, num_list, sizeof(*new_list), val_compar);
>>   
>> -	free(parm->list);
>> -	parm->num_list = num_list;
>> -	parm->list = new_list;
>> -	parm->active = 1;
>> -	return 0;
>> +	return snd_ext_parm_set(parm, 0, 0, num_list, new_list);
>>   }
>>   
>>   void snd_ext_parm_clear(struct snd_ext_parm *parm)
>> @@ -291,29 +320,37 @@ static int snd_pcm_extplug_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params
>>    */
>>   static int snd_pcm_extplug_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
>>   {
>> -
>>   	extplug_priv_t *ext = pcm->private_data;
>>   	snd_pcm_t *slave = ext->plug.gen.slave;
>> -	int err = snd_pcm_hw_params_slave(pcm, params,
>> -					  snd_pcm_extplug_hw_refine_cchange,
>> -					  snd_pcm_extplug_hw_refine_sprepare,
>> -					  snd_pcm_extplug_hw_refine_schange,
>> -					  snd_pcm_generic_hw_params);
>> -	if (err < 0)
>> -		return err;
>> -	ext->data->slave_format = slave->format;
>> -	ext->data->slave_subformat = slave->subformat;
>> -	ext->data->slave_channels = slave->channels;
>> -	ext->data->rate = slave->rate;
>> -	INTERNAL(snd_pcm_hw_params_get_format)(params, &ext->data->format);
>> -	INTERNAL(snd_pcm_hw_params_get_subformat)(params, &ext->data->subformat);
>> -	INTERNAL(snd_pcm_hw_params_get_channels)(params, &ext->data->channels);
>> -
>> -	if (ext->data->callback->hw_params) {
>> -		err = ext->data->callback->hw_params(ext->data, params);
>> +
>> +	do {
>> +		ext->sparams_changed = false;
>> +
>> +		int err = snd_pcm_hw_params_slave(pcm, params,
>> +						  snd_pcm_extplug_hw_refine_cchange,
>> +						  snd_pcm_extplug_hw_refine_sprepare,
>> +						  snd_pcm_extplug_hw_refine_schange,
>> +						  snd_pcm_generic_hw_params);
>>   		if (err < 0)
>>   			return err;
>> -	}
>> +		ext->data->slave_format = slave->format;
>> +		ext->data->slave_subformat = slave->subformat;
>> +		ext->data->slave_channels = slave->channels;
>> +		ext->data->rate = slave->rate;
>> +		INTERNAL(snd_pcm_hw_params_get_format)(
>> +				params, &ext->data->format);
>> +		INTERNAL(snd_pcm_hw_params_get_subformat)(
>> +				params, &ext->data->subformat);
>> +		INTERNAL(snd_pcm_hw_params_get_channels)(
>> +				params, &ext->data->channels);
>> +
>> +		if (ext->data->callback->hw_params) {
>> +			err = ext->data->callback->hw_params(ext->data, params);
>> +			if (err < 0)
>> +				return err;
>> +		}
>> +	} while (ext->sparams_changed);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -411,6 +448,7 @@ static void clear_ext_params(extplug_priv_t *ext)
>>   	int i;
>>   	for (i = 0; i < SND_PCM_EXTPLUG_HW_PARAMS; i++) {
>>   		snd_ext_parm_clear(&ext->params[i]);
>> +		ext->sparams_changed |= ext->sparams[i].active;
>>   		snd_ext_parm_clear(&ext->sparams[i]);
>>   	}
>>   }
>> @@ -637,7 +675,9 @@ parameters are sample format and channels.
>>   To define the constraints of the slave PCM configuration, use
>>   either #snd_pcm_extplug_set_slave_param_minmax() and
>>   #snd_pcm_extplug_set_slave_param_list().  The arguments are as same
>> -as former functions.
>> +as former functions. Both functions can also be called from the hw_params
>> +callback. This can be used to further limit the configuration of the slave
>> +device depending on the configuration of the user device.
>>   
>>   To clear the parameter constraints, call #snd_pcm_extplug_params_reset()
>>   function.
>> @@ -768,11 +808,17 @@ void snd_pcm_extplug_params_reset(snd_pcm_extplug_t *extplug)
>>   int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, unsigned int num_list, const unsigned int *list)
>>   {
>>   	extplug_priv_t *ext = extplug->pcm->private_data;
>> +	int changed = 0;
>> +
>>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
>>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
>>   		return -EINVAL;
>>   	}
>> -	return snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
>> +	changed = snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
>> +	if (changed > 0)
>> +		ext->sparams_changed = true;
>> +
>> +	return changed;
>>   }
>>   
>>   /**
>> @@ -790,6 +836,8 @@ int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, u
>>   int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type, unsigned int min, unsigned int max)
>>   {
>>   	extplug_priv_t *ext = extplug->pcm->private_data;
>> +	int changed = 0;
>> +
>>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
>>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
>>   		return -EINVAL;
>> @@ -798,7 +846,11 @@ int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type,
>>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
>>   		return -EINVAL;
>>   	}
>> -	return snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
>> +	changed = snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
>> +	if (changed > 0)
>> +		ext->sparams_changed = true;
>> +
>> +	return changed;
>>   }
>>   
>>   /**
>> -- 
>> 2.7.4
>>

Takashi Iwai Dec. 7, 2018, 11:52 a.m. UTC | #3

On Fri, 07 Dec 2018 12:45:38 +0100,
Timo Wischer wrote:
> 
> On 12/7/18 12:03, Takashi Iwai wrote:
> > On Thu, 06 Dec 2018 15:31:43 +0100,
> > <[e-mail]> wrote:
> >> From: Timo Wischer <[e-mail]>
> >>
> >> from extplug hw_params() callback function.
> >>
> >> This feature is for example required in the following use case:
> >> - A filter plugin supports only 1-8 channels. Therefore it calls
> >> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 1,
> >> 8) to provide the salve PCM parameters limited to 1-8 channels to the
> >> user application
> > Which condition is supposed for this constraint (1-8 channels)?  If
> > this is about the input to the extplug itself, it should be set via
> > snd_pcm_extplug_set_param_minmax() instead.
> >
> > The snd_pcm_extplug_set_slave_param_minmax() is used to restrict the
> > output from extplug (i.e. the condition to be passed to the slave).
> > This is needed only when the slave PCM can't have the proper input
> > constraints.  So usually this sets to a single condition or alike.
> 
> 
> When using snd_pcm_extplug_set_param_minmax() it will ignore the
> capabilities of the slave device. For example the salve device
> supports only 2 channels but the extplug supports 1-8 channels and
> uses snd_pcm_extplug_set_param_minmax(CHANNELS, 1, 8).
> So the user application can open the device with 8 channels and the
> extplug has to down mix to 2 channels. But I do not want to support
> down mixing in my extplug. I only want to limit the capabilities of
> the slave device with the limitations of my extplug and forward this
> to the user application.

Well, if you just need some additional constraints that is specific to
the explug, keep the constraints linked to slave, but apply the
additional refine on top of it.  That should make things much easier,
I suppose.


Takashi

> 
> PS: currently I am investigating in an issue of this patch in case of
> MMAP access where snd_pcm_hw_params() fails with EBUSY because
> snd_pcm_mmap() is called twice. Therefore please do not merge this
> version of the patch.
> 
> But anyway I am interested whether you are fine with this approach,
> now or I have to find another approach.
> 
> Best regards
> 
> Timo
> 
> >
> >
> > Takashi
> >
> >> - The user application requests 2 channels. But in this case the slave
> >> PCM will be configured with 1 channel because it is the first valid
> >> configuration
> >> - To update the salve PCM
> >> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 2,
> >> 2) could be called from the extplug hw_params() callback function
> >>
> >> Without this patch the call to snd_pcm_extplug_set_slave_param_minmax()
> >> would not have any effect. With this patch the slave device will also be
> >> configured with 2 channels and no down mixing has to be done by the
> >> extplug.
> >>
> >> This new feature can also be used for some specific dependencies. For
> >> example a down mixing extplug supports only 2to1 and 4to2 channel down
> >> mixing. Therefore the salve PCM has to be opened with 2 channels if the
> >> user application requests 4 channels.
> >>
> >> Signed-off-by: Timo Wischer <[e-mail]>
> >>
> >> diff --git a/src/pcm/pcm_extplug.c b/src/pcm/pcm_extplug.c
> >> index 1f887c5..44afadb 100644
> >> --- a/src/pcm/pcm_extplug.c
> >> +++ b/src/pcm/pcm_extplug.c
> >> @@ -26,6 +26,7 @@
> >>    *
> >>    */
> >>     +#include <stdbool.h>
> >>   #include "pcm_local.h"
> >>   #include "pcm_plugin.h"
> >>   #include "pcm_extplug.h"
> >> @@ -43,6 +44,7 @@ typedef struct snd_pcm_extplug_priv {
> >>   	snd_pcm_extplug_t *data;
> >>   	struct snd_ext_parm params[SND_PCM_EXTPLUG_HW_PARAMS];
> >>   	struct snd_ext_parm sparams[SND_PCM_EXTPLUG_HW_PARAMS];
> >> +	bool sparams_changed;
> >>   } extplug_priv_t;
> >>     static const int hw_params_type[SND_PCM_EXTPLUG_HW_PARAMS] = {
> >> @@ -60,18 +62,49 @@ static const unsigned int excl_parbits[SND_PCM_EXTPLUG_HW_PARAMS] = {
> >>   					 SND_PCM_HW_PARBIT_FRAME_BITS),
> >>   };
> >>   +static bool snd_ext_parm_equal(const struct snd_ext_parm * const
> >> a,
> >> +			      const struct snd_ext_parm * const b)
> >> +{
> >> +	if (a != b || a->active != b->active || a->num_list != b->num_list)
> >> +		return false;
> >> +
> >> +	if (a->num_list > 0) {
> >> +		for (unsigned int i = 0; i++; i < a->num_list) {
> >> +			if (a->list[i] != b->list[i])
> >> +				return false;
> >> +		}
> >> +	} else {
> >> +		if (a->min != b->min || a->max != b->max)
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +static int snd_ext_parm_set(struct snd_ext_parm *parm, unsigned int min,
> >> +			    unsigned int max, unsigned int num_list,
> >> +			    unsigned int *list)
> >> +{
> >> +	const struct snd_ext_parm new_parm = {
> >> +		.min = min,
> >> +		.max = max,
> >> +		.num_list = num_list,
> >> +		.list = list,
> >> +		.active = 1,
> >> +	};
> >> +	const int changed = snd_ext_parm_equal(parm, &new_parm) ? 0 : 1;
> >> +
> >> +	free(parm->list);
> >> +	*parm = new_parm;
> >> +	return changed;
> >> +}
> >> +
> >>   /*
> >>    * set min/max values for the given parameter
> >>    */
> >>   int snd_ext_parm_set_minmax(struct snd_ext_parm *parm, unsigned int min, unsigned int max)
> >>   {
> >> -	parm->num_list = 0;
> >> -	free(parm->list);
> >> -	parm->list = NULL;
> >> -	parm->min = min;
> >> -	parm->max = max;
> >> -	parm->active = 1;
> >> -	return 0;
> >> +	return snd_ext_parm_set(parm, min, max, 0, NULL);
> >>   }
> >>     /*
> >> @@ -92,11 +125,7 @@ int snd_ext_parm_set_list(struct snd_ext_parm *parm, unsigned int num_list, cons
> >>   	memcpy(new_list, list, sizeof(*new_list) * num_list);
> >>   	qsort(new_list, num_list, sizeof(*new_list), val_compar);
> >>   -	free(parm->list);
> >> -	parm->num_list = num_list;
> >> -	parm->list = new_list;
> >> -	parm->active = 1;
> >> -	return 0;
> >> +	return snd_ext_parm_set(parm, 0, 0, num_list, new_list);
> >>   }
> >>     void snd_ext_parm_clear(struct snd_ext_parm *parm)
> >> @@ -291,29 +320,37 @@ static int snd_pcm_extplug_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params
> >>    */
> >>   static int snd_pcm_extplug_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
> >>   {
> >> -
> >>   	extplug_priv_t *ext = pcm->private_data;
> >>   	snd_pcm_t *slave = ext->plug.gen.slave;
> >> -	int err = snd_pcm_hw_params_slave(pcm, params,
> >> -					  snd_pcm_extplug_hw_refine_cchange,
> >> -					  snd_pcm_extplug_hw_refine_sprepare,
> >> -					  snd_pcm_extplug_hw_refine_schange,
> >> -					  snd_pcm_generic_hw_params);
> >> -	if (err < 0)
> >> -		return err;
> >> -	ext->data->slave_format = slave->format;
> >> -	ext->data->slave_subformat = slave->subformat;
> >> -	ext->data->slave_channels = slave->channels;
> >> -	ext->data->rate = slave->rate;
> >> -	INTERNAL(snd_pcm_hw_params_get_format)(params, &ext->data->format);
> >> -	INTERNAL(snd_pcm_hw_params_get_subformat)(params, &ext->data->subformat);
> >> -	INTERNAL(snd_pcm_hw_params_get_channels)(params, &ext->data->channels);
> >> -
> >> -	if (ext->data->callback->hw_params) {
> >> -		err = ext->data->callback->hw_params(ext->data, params);
> >> +
> >> +	do {
> >> +		ext->sparams_changed = false;
> >> +
> >> +		int err = snd_pcm_hw_params_slave(pcm, params,
> >> +						  snd_pcm_extplug_hw_refine_cchange,
> >> +						  snd_pcm_extplug_hw_refine_sprepare,
> >> +						  snd_pcm_extplug_hw_refine_schange,
> >> +						  snd_pcm_generic_hw_params);
> >>   		if (err < 0)
> >>   			return err;
> >> -	}
> >> +		ext->data->slave_format = slave->format;
> >> +		ext->data->slave_subformat = slave->subformat;
> >> +		ext->data->slave_channels = slave->channels;
> >> +		ext->data->rate = slave->rate;
> >> +		INTERNAL(snd_pcm_hw_params_get_format)(
> >> +				params, &ext->data->format);
> >> +		INTERNAL(snd_pcm_hw_params_get_subformat)(
> >> +				params, &ext->data->subformat);
> >> +		INTERNAL(snd_pcm_hw_params_get_channels)(
> >> +				params, &ext->data->channels);
> >> +
> >> +		if (ext->data->callback->hw_params) {
> >> +			err = ext->data->callback->hw_params(ext->data, params);
> >> +			if (err < 0)
> >> +				return err;
> >> +		}
> >> +	} while (ext->sparams_changed);
> >> +
> >>   	return 0;
> >>   }
> >>   @@ -411,6 +448,7 @@ static void clear_ext_params(extplug_priv_t
> >> *ext)
> >>   	int i;
> >>   	for (i = 0; i < SND_PCM_EXTPLUG_HW_PARAMS; i++) {
> >>   		snd_ext_parm_clear(&ext->params[i]);
> >> +		ext->sparams_changed |= ext->sparams[i].active;
> >>   		snd_ext_parm_clear(&ext->sparams[i]);
> >>   	}
> >>   }
> >> @@ -637,7 +675,9 @@ parameters are sample format and channels.
> >>   To define the constraints of the slave PCM configuration, use
> >>   either #snd_pcm_extplug_set_slave_param_minmax() and
> >>   #snd_pcm_extplug_set_slave_param_list().  The arguments are as same
> >> -as former functions.
> >> +as former functions. Both functions can also be called from the hw_params
> >> +callback. This can be used to further limit the configuration of the slave
> >> +device depending on the configuration of the user device.
> >>     To clear the parameter constraints, call
> >> #snd_pcm_extplug_params_reset()
> >>   function.
> >> @@ -768,11 +808,17 @@ void snd_pcm_extplug_params_reset(snd_pcm_extplug_t *extplug)
> >>   int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, unsigned int num_list, const unsigned int *list)
> >>   {
> >>   	extplug_priv_t *ext = extplug->pcm->private_data;
> >> +	int changed = 0;
> >> +
> >>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
> >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> >>   		return -EINVAL;
> >>   	}
> >> -	return snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> >> +	changed = snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> >> +	if (changed > 0)
> >> +		ext->sparams_changed = true;
> >> +
> >> +	return changed;
> >>   }
> >>     /**
> >> @@ -790,6 +836,8 @@ int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, u
> >>   int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type, unsigned int min, unsigned int max)
> >>   {
> >>   	extplug_priv_t *ext = extplug->pcm->private_data;
> >> +	int changed = 0;
> >> +
> >>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
> >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> >>   		return -EINVAL;
> >> @@ -798,7 +846,11 @@ int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type,
> >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> >>   		return -EINVAL;
> >>   	}
> >> -	return snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> >> +	changed = snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> >> +	if (changed > 0)
> >> +		ext->sparams_changed = true;
> >> +
> >> +	return changed;
> >>   }
> >>     /**
> >> -- 
> >> 2.7.4
> >>
>

Takashi Iwai Dec. 7, 2018, 1:21 p.m. UTC | #4

On Fri, 07 Dec 2018 12:52:30 +0100,
Takashi Iwai wrote:
> 
> On Fri, 07 Dec 2018 12:45:38 +0100,
> Timo Wischer wrote:
> > 
> > On 12/7/18 12:03, Takashi Iwai wrote:
> > > On Thu, 06 Dec 2018 15:31:43 +0100,
> > > <[e-mail]> wrote:
> > >> From: Timo Wischer <[e-mail]>
> > >>
> > >> from extplug hw_params() callback function.
> > >>
> > >> This feature is for example required in the following use case:
> > >> - A filter plugin supports only 1-8 channels. Therefore it calls
> > >> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 1,
> > >> 8) to provide the salve PCM parameters limited to 1-8 channels to the
> > >> user application
> > > Which condition is supposed for this constraint (1-8 channels)?  If
> > > this is about the input to the extplug itself, it should be set via
> > > snd_pcm_extplug_set_param_minmax() instead.
> > >
> > > The snd_pcm_extplug_set_slave_param_minmax() is used to restrict the
> > > output from extplug (i.e. the condition to be passed to the slave).
> > > This is needed only when the slave PCM can't have the proper input
> > > constraints.  So usually this sets to a single condition or alike.
> > 
> > 
> > When using snd_pcm_extplug_set_param_minmax() it will ignore the
> > capabilities of the slave device. For example the salve device
> > supports only 2 channels but the extplug supports 1-8 channels and
> > uses snd_pcm_extplug_set_param_minmax(CHANNELS, 1, 8).
> > So the user application can open the device with 8 channels and the
> > extplug has to down mix to 2 channels. But I do not want to support
> > down mixing in my extplug. I only want to limit the capabilities of
> > the slave device with the limitations of my extplug and forward this
> > to the user application.
> 
> Well, if you just need some additional constraints that is specific to
> the explug, keep the constraints linked to slave, but apply the
> additional refine on top of it.  That should make things much easier,
> I suppose.

But actually I'd like to see exactly what's failing before going
further.  Could you give a simple test case?  You can write a dummy
extplug plugin that just shows the behavior, for example.


thanks,

Takashi

> 
> 
> Takashi
> 
> > 
> > PS: currently I am investigating in an issue of this patch in case of
> > MMAP access where snd_pcm_hw_params() fails with EBUSY because
> > snd_pcm_mmap() is called twice. Therefore please do not merge this
> > version of the patch.
> > 
> > But anyway I am interested whether you are fine with this approach,
> > now or I have to find another approach.
> > 
> > Best regards
> > 
> > Timo
> > 
> > >
> > >
> > > Takashi
> > >
> > >> - The user application requests 2 channels. But in this case the slave
> > >> PCM will be configured with 1 channel because it is the first valid
> > >> configuration
> > >> - To update the salve PCM
> > >> snd_pcm_extplug_set_slave_param_minmax(SND_PCM_EXTPLUG_HW_CHANNELS, 2,
> > >> 2) could be called from the extplug hw_params() callback function
> > >>
> > >> Without this patch the call to snd_pcm_extplug_set_slave_param_minmax()
> > >> would not have any effect. With this patch the slave device will also be
> > >> configured with 2 channels and no down mixing has to be done by the
> > >> extplug.
> > >>
> > >> This new feature can also be used for some specific dependencies. For
> > >> example a down mixing extplug supports only 2to1 and 4to2 channel down
> > >> mixing. Therefore the salve PCM has to be opened with 2 channels if the
> > >> user application requests 4 channels.
> > >>
> > >> Signed-off-by: Timo Wischer <[e-mail]>
> > >>
> > >> diff --git a/src/pcm/pcm_extplug.c b/src/pcm/pcm_extplug.c
> > >> index 1f887c5..44afadb 100644
> > >> --- a/src/pcm/pcm_extplug.c
> > >> +++ b/src/pcm/pcm_extplug.c
> > >> @@ -26,6 +26,7 @@
> > >>    *
> > >>    */
> > >>     +#include <stdbool.h>
> > >>   #include "pcm_local.h"
> > >>   #include "pcm_plugin.h"
> > >>   #include "pcm_extplug.h"
> > >> @@ -43,6 +44,7 @@ typedef struct snd_pcm_extplug_priv {
> > >>   	snd_pcm_extplug_t *data;
> > >>   	struct snd_ext_parm params[SND_PCM_EXTPLUG_HW_PARAMS];
> > >>   	struct snd_ext_parm sparams[SND_PCM_EXTPLUG_HW_PARAMS];
> > >> +	bool sparams_changed;
> > >>   } extplug_priv_t;
> > >>     static const int hw_params_type[SND_PCM_EXTPLUG_HW_PARAMS] = {
> > >> @@ -60,18 +62,49 @@ static const unsigned int excl_parbits[SND_PCM_EXTPLUG_HW_PARAMS] = {
> > >>   					 SND_PCM_HW_PARBIT_FRAME_BITS),
> > >>   };
> > >>   +static bool snd_ext_parm_equal(const struct snd_ext_parm * const
> > >> a,
> > >> +			      const struct snd_ext_parm * const b)
> > >> +{
> > >> +	if (a != b || a->active != b->active || a->num_list != b->num_list)
> > >> +		return false;
> > >> +
> > >> +	if (a->num_list > 0) {
> > >> +		for (unsigned int i = 0; i++; i < a->num_list) {
> > >> +			if (a->list[i] != b->list[i])
> > >> +				return false;
> > >> +		}
> > >> +	} else {
> > >> +		if (a->min != b->min || a->max != b->max)
> > >> +			return false;
> > >> +	}
> > >> +
> > >> +	return true;
> > >> +}
> > >> +
> > >> +static int snd_ext_parm_set(struct snd_ext_parm *parm, unsigned int min,
> > >> +			    unsigned int max, unsigned int num_list,
> > >> +			    unsigned int *list)
> > >> +{
> > >> +	const struct snd_ext_parm new_parm = {
> > >> +		.min = min,
> > >> +		.max = max,
> > >> +		.num_list = num_list,
> > >> +		.list = list,
> > >> +		.active = 1,
> > >> +	};
> > >> +	const int changed = snd_ext_parm_equal(parm, &new_parm) ? 0 : 1;
> > >> +
> > >> +	free(parm->list);
> > >> +	*parm = new_parm;
> > >> +	return changed;
> > >> +}
> > >> +
> > >>   /*
> > >>    * set min/max values for the given parameter
> > >>    */
> > >>   int snd_ext_parm_set_minmax(struct snd_ext_parm *parm, unsigned int min, unsigned int max)
> > >>   {
> > >> -	parm->num_list = 0;
> > >> -	free(parm->list);
> > >> -	parm->list = NULL;
> > >> -	parm->min = min;
> > >> -	parm->max = max;
> > >> -	parm->active = 1;
> > >> -	return 0;
> > >> +	return snd_ext_parm_set(parm, min, max, 0, NULL);
> > >>   }
> > >>     /*
> > >> @@ -92,11 +125,7 @@ int snd_ext_parm_set_list(struct snd_ext_parm *parm, unsigned int num_list, cons
> > >>   	memcpy(new_list, list, sizeof(*new_list) * num_list);
> > >>   	qsort(new_list, num_list, sizeof(*new_list), val_compar);
> > >>   -	free(parm->list);
> > >> -	parm->num_list = num_list;
> > >> -	parm->list = new_list;
> > >> -	parm->active = 1;
> > >> -	return 0;
> > >> +	return snd_ext_parm_set(parm, 0, 0, num_list, new_list);
> > >>   }
> > >>     void snd_ext_parm_clear(struct snd_ext_parm *parm)
> > >> @@ -291,29 +320,37 @@ static int snd_pcm_extplug_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params
> > >>    */
> > >>   static int snd_pcm_extplug_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
> > >>   {
> > >> -
> > >>   	extplug_priv_t *ext = pcm->private_data;
> > >>   	snd_pcm_t *slave = ext->plug.gen.slave;
> > >> -	int err = snd_pcm_hw_params_slave(pcm, params,
> > >> -					  snd_pcm_extplug_hw_refine_cchange,
> > >> -					  snd_pcm_extplug_hw_refine_sprepare,
> > >> -					  snd_pcm_extplug_hw_refine_schange,
> > >> -					  snd_pcm_generic_hw_params);
> > >> -	if (err < 0)
> > >> -		return err;
> > >> -	ext->data->slave_format = slave->format;
> > >> -	ext->data->slave_subformat = slave->subformat;
> > >> -	ext->data->slave_channels = slave->channels;
> > >> -	ext->data->rate = slave->rate;
> > >> -	INTERNAL(snd_pcm_hw_params_get_format)(params, &ext->data->format);
> > >> -	INTERNAL(snd_pcm_hw_params_get_subformat)(params, &ext->data->subformat);
> > >> -	INTERNAL(snd_pcm_hw_params_get_channels)(params, &ext->data->channels);
> > >> -
> > >> -	if (ext->data->callback->hw_params) {
> > >> -		err = ext->data->callback->hw_params(ext->data, params);
> > >> +
> > >> +	do {
> > >> +		ext->sparams_changed = false;
> > >> +
> > >> +		int err = snd_pcm_hw_params_slave(pcm, params,
> > >> +						  snd_pcm_extplug_hw_refine_cchange,
> > >> +						  snd_pcm_extplug_hw_refine_sprepare,
> > >> +						  snd_pcm_extplug_hw_refine_schange,
> > >> +						  snd_pcm_generic_hw_params);
> > >>   		if (err < 0)
> > >>   			return err;
> > >> -	}
> > >> +		ext->data->slave_format = slave->format;
> > >> +		ext->data->slave_subformat = slave->subformat;
> > >> +		ext->data->slave_channels = slave->channels;
> > >> +		ext->data->rate = slave->rate;
> > >> +		INTERNAL(snd_pcm_hw_params_get_format)(
> > >> +				params, &ext->data->format);
> > >> +		INTERNAL(snd_pcm_hw_params_get_subformat)(
> > >> +				params, &ext->data->subformat);
> > >> +		INTERNAL(snd_pcm_hw_params_get_channels)(
> > >> +				params, &ext->data->channels);
> > >> +
> > >> +		if (ext->data->callback->hw_params) {
> > >> +			err = ext->data->callback->hw_params(ext->data, params);
> > >> +			if (err < 0)
> > >> +				return err;
> > >> +		}
> > >> +	} while (ext->sparams_changed);
> > >> +
> > >>   	return 0;
> > >>   }
> > >>   @@ -411,6 +448,7 @@ static void clear_ext_params(extplug_priv_t
> > >> *ext)
> > >>   	int i;
> > >>   	for (i = 0; i < SND_PCM_EXTPLUG_HW_PARAMS; i++) {
> > >>   		snd_ext_parm_clear(&ext->params[i]);
> > >> +		ext->sparams_changed |= ext->sparams[i].active;
> > >>   		snd_ext_parm_clear(&ext->sparams[i]);
> > >>   	}
> > >>   }
> > >> @@ -637,7 +675,9 @@ parameters are sample format and channels.
> > >>   To define the constraints of the slave PCM configuration, use
> > >>   either #snd_pcm_extplug_set_slave_param_minmax() and
> > >>   #snd_pcm_extplug_set_slave_param_list().  The arguments are as same
> > >> -as former functions.
> > >> +as former functions. Both functions can also be called from the hw_params
> > >> +callback. This can be used to further limit the configuration of the slave
> > >> +device depending on the configuration of the user device.
> > >>     To clear the parameter constraints, call
> > >> #snd_pcm_extplug_params_reset()
> > >>   function.
> > >> @@ -768,11 +808,17 @@ void snd_pcm_extplug_params_reset(snd_pcm_extplug_t *extplug)
> > >>   int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, unsigned int num_list, const unsigned int *list)
> > >>   {
> > >>   	extplug_priv_t *ext = extplug->pcm->private_data;
> > >> +	int changed = 0;
> > >> +
> > >>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
> > >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> > >>   		return -EINVAL;
> > >>   	}
> > >> -	return snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> > >> +	changed = snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
> > >> +	if (changed > 0)
> > >> +		ext->sparams_changed = true;
> > >> +
> > >> +	return changed;
> > >>   }
> > >>     /**
> > >> @@ -790,6 +836,8 @@ int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, u
> > >>   int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type, unsigned int min, unsigned int max)
> > >>   {
> > >>   	extplug_priv_t *ext = extplug->pcm->private_data;
> > >> +	int changed = 0;
> > >> +
> > >>   	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
> > >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> > >>   		return -EINVAL;
> > >> @@ -798,7 +846,11 @@ int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type,
> > >>   		SNDERR("EXTPLUG: invalid parameter type %d", type);
> > >>   		return -EINVAL;
> > >>   	}
> > >> -	return snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> > >> +	changed = snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
> > >> +	if (changed > 0)
> > >> +		ext->sparams_changed = true;
> > >> +
> > >> +	return changed;
> > >>   }
> > >>     /**
> > >> -- 
> > >> 2.7.4
> > >>
> >

Patch

diff --git a/src/pcm/pcm_extplug.c b/src/pcm/pcm_extplug.c
index 1f887c5..44afadb 100644
--- a/src/pcm/pcm_extplug.c
+++ b/src/pcm/pcm_extplug.c
@@ -26,6 +26,7 @@ 
  *
  */
   
+#include <stdbool.h>
 #include "pcm_local.h"
 #include "pcm_plugin.h"
 #include "pcm_extplug.h"
@@ -43,6 +44,7 @@  typedef struct snd_pcm_extplug_priv {
 	snd_pcm_extplug_t *data;
 	struct snd_ext_parm params[SND_PCM_EXTPLUG_HW_PARAMS];
 	struct snd_ext_parm sparams[SND_PCM_EXTPLUG_HW_PARAMS];
+	bool sparams_changed;
 } extplug_priv_t;
 
 static const int hw_params_type[SND_PCM_EXTPLUG_HW_PARAMS] = {
@@ -60,18 +62,49 @@  static const unsigned int excl_parbits[SND_PCM_EXTPLUG_HW_PARAMS] = {
 					 SND_PCM_HW_PARBIT_FRAME_BITS),
 };
 
+static bool snd_ext_parm_equal(const struct snd_ext_parm * const a,
+			      const struct snd_ext_parm * const b)
+{
+	if (a != b || a->active != b->active || a->num_list != b->num_list)
+		return false;
+
+	if (a->num_list > 0) {
+		for (unsigned int i = 0; i++; i < a->num_list) {
+			if (a->list[i] != b->list[i])
+				return false;
+		}
+	} else {
+		if (a->min != b->min || a->max != b->max)
+			return false;
+	}
+
+	return true;
+}
+
+static int snd_ext_parm_set(struct snd_ext_parm *parm, unsigned int min,
+			    unsigned int max, unsigned int num_list,
+			    unsigned int *list)
+{
+	const struct snd_ext_parm new_parm = {
+		.min = min,
+		.max = max,
+		.num_list = num_list,
+		.list = list,
+		.active = 1,
+	};
+	const int changed = snd_ext_parm_equal(parm, &new_parm) ? 0 : 1;
+
+	free(parm->list);
+	*parm = new_parm;
+	return changed;
+}
+
 /*
  * set min/max values for the given parameter
  */
 int snd_ext_parm_set_minmax(struct snd_ext_parm *parm, unsigned int min, unsigned int max)
 {
-	parm->num_list = 0;
-	free(parm->list);
-	parm->list = NULL;
-	parm->min = min;
-	parm->max = max;
-	parm->active = 1;
-	return 0;
+	return snd_ext_parm_set(parm, min, max, 0, NULL);
 }
 
 /*
@@ -92,11 +125,7 @@  int snd_ext_parm_set_list(struct snd_ext_parm *parm, unsigned int num_list, cons
 	memcpy(new_list, list, sizeof(*new_list) * num_list);
 	qsort(new_list, num_list, sizeof(*new_list), val_compar);
 
-	free(parm->list);
-	parm->num_list = num_list;
-	parm->list = new_list;
-	parm->active = 1;
-	return 0;
+	return snd_ext_parm_set(parm, 0, 0, num_list, new_list);
 }
 
 void snd_ext_parm_clear(struct snd_ext_parm *parm)
@@ -291,29 +320,37 @@  static int snd_pcm_extplug_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params
  */
 static int snd_pcm_extplug_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
 {
-
 	extplug_priv_t *ext = pcm->private_data;
 	snd_pcm_t *slave = ext->plug.gen.slave;
-	int err = snd_pcm_hw_params_slave(pcm, params,
-					  snd_pcm_extplug_hw_refine_cchange,
-					  snd_pcm_extplug_hw_refine_sprepare,
-					  snd_pcm_extplug_hw_refine_schange,
-					  snd_pcm_generic_hw_params);
-	if (err < 0)
-		return err;
-	ext->data->slave_format = slave->format;
-	ext->data->slave_subformat = slave->subformat;
-	ext->data->slave_channels = slave->channels;
-	ext->data->rate = slave->rate;
-	INTERNAL(snd_pcm_hw_params_get_format)(params, &ext->data->format);
-	INTERNAL(snd_pcm_hw_params_get_subformat)(params, &ext->data->subformat);
-	INTERNAL(snd_pcm_hw_params_get_channels)(params, &ext->data->channels);
-
-	if (ext->data->callback->hw_params) {
-		err = ext->data->callback->hw_params(ext->data, params);
+
+	do {
+		ext->sparams_changed = false;
+
+		int err = snd_pcm_hw_params_slave(pcm, params,
+						  snd_pcm_extplug_hw_refine_cchange,
+						  snd_pcm_extplug_hw_refine_sprepare,
+						  snd_pcm_extplug_hw_refine_schange,
+						  snd_pcm_generic_hw_params);
 		if (err < 0)
 			return err;
-	}
+		ext->data->slave_format = slave->format;
+		ext->data->slave_subformat = slave->subformat;
+		ext->data->slave_channels = slave->channels;
+		ext->data->rate = slave->rate;
+		INTERNAL(snd_pcm_hw_params_get_format)(
+				params, &ext->data->format);
+		INTERNAL(snd_pcm_hw_params_get_subformat)(
+				params, &ext->data->subformat);
+		INTERNAL(snd_pcm_hw_params_get_channels)(
+				params, &ext->data->channels);
+
+		if (ext->data->callback->hw_params) {
+			err = ext->data->callback->hw_params(ext->data, params);
+			if (err < 0)
+				return err;
+		}
+	} while (ext->sparams_changed);
+
 	return 0;
 }
 
@@ -411,6 +448,7 @@  static void clear_ext_params(extplug_priv_t *ext)
 	int i;
 	for (i = 0; i < SND_PCM_EXTPLUG_HW_PARAMS; i++) {
 		snd_ext_parm_clear(&ext->params[i]);
+		ext->sparams_changed |= ext->sparams[i].active;
 		snd_ext_parm_clear(&ext->sparams[i]);
 	}
 }
@@ -637,7 +675,9 @@  parameters are sample format and channels.
 To define the constraints of the slave PCM configuration, use
 either #snd_pcm_extplug_set_slave_param_minmax() and
 #snd_pcm_extplug_set_slave_param_list().  The arguments are as same
-as former functions.
+as former functions. Both functions can also be called from the hw_params
+callback. This can be used to further limit the configuration of the slave
+device depending on the configuration of the user device.
 
 To clear the parameter constraints, call #snd_pcm_extplug_params_reset()
 function. 
@@ -768,11 +808,17 @@  void snd_pcm_extplug_params_reset(snd_pcm_extplug_t *extplug)
 int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, unsigned int num_list, const unsigned int *list)
 {
 	extplug_priv_t *ext = extplug->pcm->private_data;
+	int changed = 0;
+
 	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
 		SNDERR("EXTPLUG: invalid parameter type %d", type);
 		return -EINVAL;
 	}
-	return snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
+	changed = snd_ext_parm_set_list(&ext->sparams[type], num_list, list);
+	if (changed > 0)
+		ext->sparams_changed = true;
+
+	return changed;
 }
 
 /**
@@ -790,6 +836,8 @@  int snd_pcm_extplug_set_slave_param_list(snd_pcm_extplug_t *extplug, int type, u
 int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type, unsigned int min, unsigned int max)
 {
 	extplug_priv_t *ext = extplug->pcm->private_data;
+	int changed = 0;
+
 	if (type < 0 || type >= SND_PCM_EXTPLUG_HW_PARAMS) {
 		SNDERR("EXTPLUG: invalid parameter type %d", type);
 		return -EINVAL;
@@ -798,7 +846,11 @@  int snd_pcm_extplug_set_slave_param_minmax(snd_pcm_extplug_t *extplug, int type,
 		SNDERR("EXTPLUG: invalid parameter type %d", type);
 		return -EINVAL;
 	}
-	return snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
+	changed = snd_ext_parm_set_minmax(&ext->sparams[type], min, max);
+	if (changed > 0)
+		ext->sparams_changed = true;
+
+	return changed;
 }
 
 /**