On 17/11/16 16:47, Takashi Iwai wrote:

Looks good, but please give your sign-off like the previous one.

I did resend with sign-off.

Also, keep maintainers to Cc.

I'm not sure what you mean?

On 13/12/16 11:49, Takashi Iwai wrote:

Compare the argument you passed there and the function definition, it's obvious:)

!$$"%%! Ouch. Sorry about that. One of those things that can happen when a patch is cherry-picked and "fixed". Not really sure why it compiles with just a warning.

Fixed patch attached.

Alan.

On 08/02/17 15:36, Takashi Iwai wrote:

can't change the existing function definition. This will be broken once you mix the old version of plugin with the new system or vice versa.

I had concluded that changing the type signature was in this case safe. There is no (public) declaration of _snd_pcm_rate_/xxx/_open() function type and I think that passing unexpected additional parameters is a function is always safe.

However ...

Alternatively, try to provide another function _snd_pcm_rate_xxx_open_conf() or such. In addition, you should provide the old open function as is for now, too. Then the rate plugin can try to get and open via snd_dlobj_cache_get() for the open_conf at first, then fall back to the old open function.

I like this more. It is cleaner in some ways and avoids the need to bump the version number.

I'll work up a revised patch.

Thanks, Alan.

Here is a revised patch.

It is useful for the converter used by a rate plugin to be capable of receiving configuration. This patch enables the "converter" node of the configuration of a "type rate" plugin to be specified as a compound and passed to open func of the converter plugin.

The SND_PCM_RATE_PLUGIN_CONF_ENTRY macro is used to define a rate converter plugin entry point that takes the additional snd_config_t *conf parameter. If a rate converter plugin also supports the existing SND_PCM_RATE_PLUGIN_ENTRY entry point, or if passed conf parameter is NULL then it is up to the plugin to determine whether or not this is a fatal condition.

Alan.

On 14/02/17 07:08, Takashi Iwai wrote:

On Mon, 13 Feb 2017 18:20:01 +0100, Alan Young wrote:

...

Here is a revised patch.

It is useful for the converter used by a rate plugin to be capable of receiving configuration. This patch enables the "converter" node of the configuration of a "type rate" plugin to be specified as a compound and passed to open func of the converter plugin.

The SND_PCM_RATE_PLUGIN_CONF_ENTRY macro is used to define a rate converter plugin entry point that takes the additional snd_config_t *conf parameter. If a rate converter plugin also supports the existing SND_PCM_RATE_PLUGIN_ENTRY entry point, or if passed conf parameter is NULL then it is up to the plugin to determine whether or not this is a fatal condition.

Alan.

The changes look almost good, but one uncertain change:

...

else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) { snd_config_iterator_t i, next; snd_config_for_each(i, next, converter) { snd_config_t *n = snd_config_iterator_entry(i);

• 	const char *id;
  if (snd_config_get_string(n, &type) < 0)
  	break;
  

• 	err = rate_open_func(rate, type, 0);
  

• 	err = rate_open_func(rate, type, converter, 0);
  if (!err)
  	break;
  

• 	if (snd_config_get_id(n, &id) >= 0 && id && strcmp(id, "0") != 0)
  

• 		break; /* not a simple array of types */
  

What does this check exactly...?

In the case that snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND, this could be for one of two reasons:

1. Where the configuration contains something like converter {name xxx, other stuff ...}, or 2. where configuration contains something like [first second third].

In the later case, the id fields will be digit strings, with the first entry having the id "0". The test simply stops going around the loop looking for further alternatives if the first did not have id "0".

However, looking at that again now, I see that the test is flawed because it has no marker to check that it is only applied on the first iteration, so the loop would always stop after testing the second alternative. I could fix that or remove the test altogether - what do you think?

Alan.

On Fri, 17 Feb 2017 18:44:47 +0100, Alan Young wrote:

On 17/02/17 16:53, Takashi Iwai wrote:

...

The problem is that you pass always the compound object as the converter argument. As you already suggested, there are two cases: one is a string array and another is a compound with optional args. In your code, the first iteration doesn't check which type is. So it always fails if the string array is passed.

Well, it does actually work in both cases but I guess that the plugin could get passed an unexpected type of compound config converter parameter in some cases.

...

That is, the right implementation is to check whether the given compound is a string array is. If yes, it goes to the old style loop (and you can check "name" argument properly). If not, it goes with the new compound argument. That's simple enough, and more understandable the condition you used for the loop termination.

The following makes the difference more explicit What do you think?

else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
    snd_config_iterator_t i, next;
    int pos = 0, is_array = 0;
    /*
     * If the convertor compound is an array of alternatives then

the id of * the first element will be "0" (or maybe NULL). Otherwise assume it is * a structure and must have a "name" id to identify the converter type. */ snd_config_for_each(i, next, converter) { snd_config_t *n = snd_config_iterator_entry(i); const char *id; snd_config_get_id(n, &id); if (pos++ == 0 && (!id || strcmp(id, "0") == 0)) is_array = 1; if (!is_array && strcmp(id, "name") != 0) continue; if (snd_config_get_string(n, &type) < 0) break; err = rate_open_func(rate, type, is_array ? NULL : converter, 0); if (!err || !is_array) break; }

It becomes too complex by mixing up things in a single loop. Let's take it just simple like:

if (is_string_array(converter)) { snd_config_for_each(i, next, converter) { // like the old way } } else { // handle for the new compound type }

Takashi

On Tue, 21 Feb 2017 13:02:49 +0100, Alan Young wrote:

On 17/02/17 17:59, Takashi Iwai wrote:

...

It becomes too complex by mixing up things in a single loop. Let's take it just simple like:

if (is_string_array(converter)) { snd_config_for_each(i, next, converter) { // like the old way } } else { // handle for the new compound type }

Like this?

else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) { /* * If the convertor compound is an array of alternatives then the id of * the first element will be "0" (or maybe NULL). Otherwise assume it is * a structure and must have a "name" id to identify the converter type. */ snd_config_iterator_t i, next; i = snd_config_iterator_first(converter); if (i && i != snd_config_iterator_end(converter)) { snd_config_t *n = snd_config_iterator_entry(i); const char *id; snd_config_get_id(n, &id); if (!id || strcmp(id, "0") == 0) {

Make this a function, e.g. is_string_array(), as I mentioned.

		snd_config_for_each(i, next, converter) {
			n = snd_config_iterator_entry(i);
			if (snd_config_get_string(n, &type) < 0)
				break;
			err = rate_open_func(rate, type, NULL, 0);
			if (!err)
				break;
		}

	} else {
		snd_config_for_each(i, next, converter) {
			snd_config_t *n = snd_config_iterator_entry(i);
			const char *id;
			snd_config_get_id(n, &id);
			if (strcmp(id, "name") != 0)
				continue;
			if (snd_config_get_string(n, &type) < 0)
				break;
			err = rate_open_func(rate, type, converter, 0);
			if (!err)
				break;
		}

In the second case, calling the open function inside the loop makes the code unclear. The loop is only for obtaining the name string. The open func should be called outside the loop once when you get the name string (or bail out as an error if no name is found beforehand).

thanks,

Takashi

On Tue, 21 Feb 2017 15:34:44 +0100, Alan Young wrote:

• else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {

• snd_config_iterator_t i, next;
  

• snd_config_for_each(i, next, converter) {
  

• 	snd_config_t *n = snd_config_iterator_entry(i);
  

• 	const char *id;
  

• 	snd_config_get_id(n, &id);
  

• 	if (strcmp(id, "name") != 0)
  

• 		continue;
  

• 	snd_config_get_string(n, &type);
  

• 	break;
  

• }
  

• if (type) {
  

• 	err = rate_open_func(rate, type, converter, 1);
  

• }
  

The error handling is missing when no name is given. You should handle the error explicitly instead of the success.

if (!type) { SNDERR("No name is given for rate converter"); ..... return -EINVAL; }

err = rate_open_func(....);

Takashi

On Tue, 21 Feb 2017 16:04:54 +0100, Alan Young wrote:

• else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {

• snd_config_iterator_t i, next;
  

• snd_config_for_each(i, next, converter) {
  

• 	snd_config_t *n = snd_config_iterator_entry(i);
  

• 	const char *id;
  

• 	snd_config_get_id(n, &id);
  

• 	if (strcmp(id, "name") != 0)
  

• 		continue;
  

• 	snd_config_get_string(n, &type);
  

• 	break;
  

• }
  

• if (!type) {
  

• 	SNDERR("No name given for rate converter");
  

• 	return -EINVAL;
  

Resource leaks.

Takashi

On Tue, 21 Feb 2017 16:24:06 +0100, Alan Young wrote:

On 21/02/17 15:14, Takashi Iwai wrote:

...

On Tue, 21 Feb 2017 16:04:54 +0100, Alan Young wrote:

...

• else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {

• snd_config_iterator_t i, next;
  

• snd_config_for_each(i, next, converter) {
  

• 	snd_config_t *n = snd_config_iterator_entry(i);
  

• 	const char *id;
  

• 	snd_config_get_id(n, &id);
  

• 	if (strcmp(id, "name") != 0)
  

• 		continue;
  

• 	snd_config_get_string(n, &type);
  

• 	break;
  

• }
  

• if (!type) {
  

• 	SNDERR("No name given for rate converter");
  

• 	return -EINVAL;
  

Resource leaks.

Can you expand on that a little please? What leaks?

You forgot to call

snd_pcm_free(pcm); free(rate);

before returning an error.

Ideally, all these calls should be moved to a single place and we'd call like "goto error", but it's a thing to be done by another patch.

Takashi

On Tue, 21 Feb 2017 17:14:29 +0100, Alan Young wrote:

On 21/02/17 15:28, Takashi Iwai wrote:

...

You forgot to call

snd_pcm_free(pcm); free(rate);

before returning an error.

Ah, ok. Updated patch.

...

From de90c659c98ae4dac7de6eb225b22147f56a3d1c Mon Sep 17 00:00:00 2001

From: Alan Young consult.awy@gmail.com Date: Thu, 7 Apr 2016 09:15:04 +0100 Subject: [PATCH] pcm: rate: Add capability to pass configuration node to plugins

If a rate plugin uses a node (compound) instead of a plain string for its "converter", and that compound is not a simple string array, then the compound will be passed as an additional parameter to the new plugin open() function (SND_PCM_RATE_PLUGIN_CONF_ENTRY(XXX)). The previous open() function (SND_PCM_RATE_PLUGIN_ENTRY(XXX)) will be called if the CONF version is not found. It is up to the plugin to determine whether the presence of the conf parameter is mandatory.

Signed-off-by: Alan Young consult.awy@gmail.com

Applied, thanks.

Takashi