diff mbox

[updated] pcm: rate: Add capability to pass configuration node to plugins

Message ID a66a494e-3bd6-c813-6786-ca9deceabdc0@IEE.org (mailing list archive)
State New, archived
Headers show

Commit Message

Alan Young Feb. 15, 2017, 12:28 p.m. UTC 
On 14/02/17 07:30, Alan Young wrote:
> 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?

Here is an updated patch with a more robust check.

Alan.

Comments

Takashi Iwai Feb. 17, 2017, 4:53 p.m. UTC | #1 
On Wed, 15 Feb 2017 13:28:38 +0100,
Alan Young wrote:
> 
> On 14/02/17 07:30, Alan Young wrote:
> > 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?
> 
> Here is an updated patch with a more robust check.

I still doubt whether it works as you expected...

>  	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 && !isdigit(*id))
> +				break; /* not a simple array of types */
>  		}

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.

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. 

BTW, one another thing:
> @@ -1386,7 +1414,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
>  #else
>  	type = "linear";
>  	open_func = SND_PCM_RATE_PLUGIN_ENTRY(linear);
> -	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops);
> +	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops, 0);

Is this really correct?  I thought SND_PCM_RATE_PLUGIN_ENTRY() refers
to the old style?


thanks,

Takashi
Alan Young Feb. 17, 2017, 5:44 p.m. UTC | #2 
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;
         }

> BTW, one another thing:
>> >@@ -1386,7 +1414,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
>> >  #else
>> >  	type = "linear";
>> >  	open_func = SND_PCM_RATE_PLUGIN_ENTRY(linear);
>> >-	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops);
>> >+	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops, 0);
> Is this really correct?  I thought SND_PCM_RATE_PLUGIN_ENTRY() refers
> to the old style?
>

Yes, left over from earlier version where I just changed the old func's 
signature.

Alan.
Takashi Iwai Feb. 17, 2017, 5:59 p.m. UTC | #3 
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
Alan Young Feb. 21, 2017, 12:02 p.m. UTC | #4 
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) {
				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;
				}
			}
		}
Takashi Iwai Feb. 21, 2017, 12:39 p.m. UTC | #5 
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
diff mbox

Patch

From e27a479d48e86313b868c1f23261f7dcdf68bf82 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" then that 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.

The existing behaviour of using the first (usable) plain string value,
regardless of parameter name, within the configuration node as the
converter name is unchanged.

Signed-off-by: Alan Young <consult.awy@gmail.com>
---
 include/pcm_rate.h |  5 ++++-
 src/pcm/pcm_rate.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/include/pcm_rate.h b/include/pcm_rate.h
index 4d70df2..da278ac 100644
--- a/include/pcm_rate.h
+++ b/include/pcm_rate.h
@@ -120,11 +120,14 @@  typedef struct snd_pcm_rate_ops {
 typedef int (*snd_pcm_rate_open_func_t)(unsigned int version, void **objp,
 					snd_pcm_rate_ops_t *opsp);
 
+typedef int (*snd_pcm_rate_open_conf_func_t)(unsigned int version, void **objp,
+					snd_pcm_rate_ops_t *opsp, const snd_config_t *conf);
+
 /**
  * Define the object entry for external PCM rate-converter plugins
  */
 #define SND_PCM_RATE_PLUGIN_ENTRY(name) _snd_pcm_rate_##name##_open
-
+#define SND_PCM_RATE_PLUGIN_CONF_ENTRY(name) _snd_pcm_rate_##name##_open_conf
 
 #ifndef DOC_HIDDEN
 /* old rate_ops for protocol version 0x010001 */
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index cbb7618..90a00fb 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1249,6 +1249,9 @@  const snd_config_t *snd_pcm_rate_get_default_converter(snd_config_t *root)
 }
 
 #ifdef PIC
+
+#include <ctype.h> /* for isdigit() */
+
 static int is_builtin_plugin(const char *type)
 {
 	return strcmp(type, "linear") == 0;
@@ -1258,26 +1261,48 @@  static const char *const default_rate_plugins[] = {
 	"speexrate", "linear", NULL
 };
 
-static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
+static int rate_open_func(snd_pcm_rate_t *rate, const char *type, const snd_config_t *converter_conf, int verbose)
 {
-	char open_name[64], lib_name[128], *lib = NULL;
+	char open_name[64], open_conf_name[64], lib_name[128], *lib = NULL;
 	snd_pcm_rate_open_func_t open_func;
+	snd_pcm_rate_open_conf_func_t open_conf_func;
 	int err;
 
 	snprintf(open_name, sizeof(open_name), "_snd_pcm_rate_%s_open", type);
+	snprintf(open_conf_name, sizeof(open_conf_name), "_snd_pcm_rate_%s_open_conf", type);
 	if (!is_builtin_plugin(type)) {
 		snprintf(lib_name, sizeof(lib_name),
 				 "%s/libasound_module_rate_%s.so", ALSA_PLUGIN_DIR, type);
 		lib = lib_name;
 	}
+
+	rate->rate_min = SND_PCM_PLUGIN_RATE_MIN;
+	rate->rate_max = SND_PCM_PLUGIN_RATE_MAX;
+	rate->plugin_version = SND_PCM_RATE_PLUGIN_VERSION;
+
+	open_conf_func = snd_dlobj_cache_get(lib, open_conf_name, NULL, verbose);
+	if (open_conf_func) {
+		err = open_conf_func(SND_PCM_RATE_PLUGIN_VERSION,
+				     &rate->obj, &rate->ops, converter_conf);
+		if (!err) {
+			rate->plugin_version = rate->ops.version;
+			if (rate->ops.get_supported_rates)
+				rate->ops.get_supported_rates(rate->obj,
+							      &rate->rate_min,
+							      &rate->rate_max);
+			rate->open_func = open_conf_func;
+			return 0;
+		} else {
+			snd_dlobj_cache_put(open_conf_func);
+			return err;
+		}
+	}
+
 	open_func = snd_dlobj_cache_get(lib, open_name, NULL, verbose);
 	if (!open_func)
 		return -ENOENT;
 
 	rate->open_func = open_func;
-	rate->rate_min = SND_PCM_PLUGIN_RATE_MIN;
-	rate->rate_max = SND_PCM_PLUGIN_RATE_MAX;
-	rate->plugin_version = SND_PCM_RATE_PLUGIN_VERSION;
 
 	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops);
 	if (!err) {
@@ -1353,23 +1378,26 @@  int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
 	if (!converter) {
 		const char *const *types;
 		for (types = default_rate_plugins; *types; types++) {
-			err = rate_open_func(rate, *types, 0);
+			err = rate_open_func(rate, *types, NULL, 0);
 			if (!err) {
 				type = *types;
 				break;
 			}
 		}
 	} else if (!snd_config_get_string(converter, &type))
-		err = rate_open_func(rate, type, 1);
+		err = rate_open_func(rate, type, NULL, 1);
 	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 && !isdigit(*id))
+				break; /* not a simple array of types */
 		}
 	} else {
 		SNDERR("Invalid type for rate converter");
@@ -1386,7 +1414,7 @@  int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
 #else
 	type = "linear";
 	open_func = SND_PCM_RATE_PLUGIN_ENTRY(linear);
-	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops);
+	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops, 0);
 	if (err < 0) {
 		snd_pcm_free(pcm);
 		free(rate);
@@ -1439,6 +1467,11 @@  pcm.name {
 	converter [ STR1 STR2 ... ]	# optional
 				# Converter type, default is taken from
 				# defaults.pcm.rate_converter
+	# or
+	converter {		# optional
+		name STR	# Convertor type
+		xxx yyy		# optional convertor-specific configuration
+	}
 }
 \endcode
 
-- 
2.5.5