[alsa-devel] [PATCH] pcm_rate: Do not discard slave reported delay in status result.
Similar to recent dshare patch.
On Thu, 17 Nov 2016 16:24:23 +0100, Alan Young wrote:
Similar to recent dshare patch.
Looks good, but please give your sign-off like the previous one. Also, keep maintainers to Cc. thanks, Takashi
From da687e77261f5cdd3c4b373156fb68ed83d98a26 Mon Sep 17 00:00:00 2001 From: Alan Young <consult.awy@gmail.com> Date: Tue, 14 Jun 2016 10:15:01 +0100 Subject: [PATCH] pcm_rate: Do not discard slave reported delay in status result.
snd_pcm_rate_status() gets the underlying status from the slave PCM. This may contain a delay value that includes elements such as codec and other transfer delays. Use this as the base for the returned delay value, adjusted for any frames buffered locally (within the rate plugin).
Also update snd_pcm_rate_delay() similarly. --- src/pcm/pcm_rate.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 6184def..15383ae 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -559,10 +559,9 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm, pcm->channels, rate); }
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr) { snd_pcm_rate_t *rate = pcm->private_data; - snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
if (pcm->stream != SND_PCM_STREAM_PLAYBACK) return; @@ -576,6 +575,12 @@ static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) rate->hw_ptr %= pcm->boundary; }
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + snd_pcm_rate_sync_hwptr0(pcm, *rate->gen.slave->hw.ptr); +} + static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; @@ -586,10 +591,37 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) return 0; }
+static snd_pcm_uframes_t snd_pcm_rate_playback_internal_delay(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + + if (rate->appl_ptr < rate->last_commit_ptr) { + return rate->appl_ptr - rate->last_commit_ptr + pcm->boundary; + } else { + return rate->appl_ptr - rate->last_commit_ptr; + } +} + static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) { + snd_pcm_rate_t *rate = pcm->private_data; + snd_pcm_sframes_t slave_delay; + int err; + snd_pcm_rate_hwsync(pcm); - *delayp = snd_pcm_mmap_hw_avail(pcm); + + err = snd_pcm_delay(rate->gen.slave, &slave_delay); + if (err < 0) { + return err; + } + + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { + *delayp = rate->ops.input_frames(rate->obj, slave_delay) + + snd_pcm_rate_playback_internal_delay(pcm); + } else { + *delayp = rate->ops.output_frames(rate->obj, slave_delay) + + snd_pcm_mmap_capture_hw_avail(pcm); + } return 0; }
@@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status) status->state = SND_PCM_STATE_RUNNING; status->trigger_tstamp = rate->trigger_tstamp; } - snd_pcm_rate_sync_hwptr(pcm); + snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr); status->appl_ptr = *pcm->appl.ptr; status->hw_ptr = *pcm->hw.ptr; if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { - status->delay = snd_pcm_mmap_playback_hw_avail(pcm); + status->delay = rate->ops.input_frames(rate->obj, status->delay) + + snd_pcm_rate_playback_internal_delay(pcm); status->avail = snd_pcm_mmap_playback_avail(pcm); status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max); } else { - status->delay = snd_pcm_mmap_capture_hw_avail(pcm); + status->delay = rate->ops.output_frames(rate->obj, status->delay) + + snd_pcm_mmap_capture_hw_avail(pcm); status->avail = snd_pcm_mmap_capture_avail(pcm); status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max); } -- 2.5.5
_______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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 Thu, 17 Nov 2016 17:51:25 +0100, Alan Young wrote:
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.
I haven't seen it. It's about pcm_rate.c, right? And, if you resent it, put like "[PATCH v2]", "[PATCH RESEND]" or such to indicate that it's a resent patch.
Also, keep maintainers to Cc.
I'm not sure what you mean?
You need to put me to Cc in addition to alsa-devel ML. In that way, your post can reach me far more quickly, and I can catch even if it's rejected or filtered by ML by some reason. Takashi
On Thu, 17 Nov 2016 18:12:57 +0100, Alan Young wrote:
On 17/11/16 15:24, Alan Young wrote:
Similar to recent dshare patch.
Update with sign-off
Sorry for the late follow up, I've been drug by other things for too long.
From 6f93ee59846d2c0058ef702c1fa68d723bfb14f6 Mon Sep 17 00:00:00 2001 From: Alan Young <consult.awy@gmail.com> Date: Tue, 14 Jun 2016 10:15:01 +0100 Subject: [PATCH] pcm_rate: Do not discard slave reported delay in status result.
snd_pcm_rate_status() gets the underlying status from the slave PCM. This may contain a delay value that includes elements such as codec and other transfer delays. Use this as the base for the returned delay value, adjusted for any frames buffered locally (within the rate plugin).
Also update snd_pcm_rate_delay() similarly.
Signed-off-by: Alan Young <consult.awy@gmail.com> --- src/pcm/pcm_rate.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 6184def..15383ae 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -559,10 +559,9 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm, pcm->channels, rate); }
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr) { snd_pcm_rate_t *rate = pcm->private_data; - snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
if (pcm->stream != SND_PCM_STREAM_PLAYBACK) return; @@ -576,6 +575,12 @@ static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) rate->hw_ptr %= pcm->boundary; }
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + snd_pcm_rate_sync_hwptr0(pcm, *rate->gen.slave->hw.ptr); +} + static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; @@ -586,10 +591,37 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) return 0; }
+static snd_pcm_uframes_t snd_pcm_rate_playback_internal_delay(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + + if (rate->appl_ptr < rate->last_commit_ptr) { + return rate->appl_ptr - rate->last_commit_ptr + pcm->boundary; + } else { + return rate->appl_ptr - rate->last_commit_ptr; + } +} + static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) { + snd_pcm_rate_t *rate = pcm->private_data; + snd_pcm_sframes_t slave_delay; + int err; + snd_pcm_rate_hwsync(pcm); - *delayp = snd_pcm_mmap_hw_avail(pcm); + + err = snd_pcm_delay(rate->gen.slave, &slave_delay); + if (err < 0) { + return err; + } + + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { + *delayp = rate->ops.input_frames(rate->obj, slave_delay) + + snd_pcm_rate_playback_internal_delay(pcm); + } else { + *delayp = rate->ops.output_frames(rate->obj, slave_delay) + + snd_pcm_mmap_capture_hw_avail(pcm); + }
Hmm, I'm not 100% sure through a quick review whether it's the correct calculation. Maybe it's worth to give more comments either in the code or in the changelog.
return 0; }
@@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status) status->state = SND_PCM_STATE_RUNNING; status->trigger_tstamp = rate->trigger_tstamp; } - snd_pcm_rate_sync_hwptr(pcm); + snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr);
This can't work. I can fix it in my side, but OTOH, this made me wonder how you tested the patch...
status->appl_ptr = *pcm->appl.ptr; status->hw_ptr = *pcm->hw.ptr; if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { - status->delay = snd_pcm_mmap_playback_hw_avail(pcm); + status->delay = rate->ops.input_frames(rate->obj, status->delay) + + snd_pcm_rate_playback_internal_delay(pcm); status->avail = snd_pcm_mmap_playback_avail(pcm); status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max); } else { - status->delay = snd_pcm_mmap_capture_hw_avail(pcm); + status->delay = rate->ops.output_frames(rate->obj, status->delay) + + snd_pcm_mmap_capture_hw_avail(pcm); status->avail = snd_pcm_mmap_capture_avail(pcm); status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max);
Why only playback needs the special handling while the capture doesn't? Again, some comments would be helpful for better understanding your changes. thanks, Takashi
Hi, Sorry for the delay. I too got sidetracked by other work. On 28/11/16 19:11, Takashi Iwai wrote:
@@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status) status->state = SND_PCM_STATE_RUNNING; status->trigger_tstamp = rate->trigger_tstamp; } - snd_pcm_rate_sync_hwptr(pcm); + snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr); This can't work.
I can fix it in my side, but OTOH, this made me wonder how you tested the patch...
Why do you think that cannot work? I am pretty sure it is working just fine for us but I am sure that it is possible that I have missed something.
status->appl_ptr = *pcm->appl.ptr; status->hw_ptr = *pcm->hw.ptr; if (pcm->stream == SND_PCM_STREAM_PLAYBACK) { - status->delay = snd_pcm_mmap_playback_hw_avail(pcm); + status->delay = rate->ops.input_frames(rate->obj, status->delay) + + snd_pcm_rate_playback_internal_delay(pcm); status->avail = snd_pcm_mmap_playback_avail(pcm); status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max); } else { - status->delay = snd_pcm_mmap_capture_hw_avail(pcm); + status->delay = rate->ops.output_frames(rate->obj, status->delay) + + snd_pcm_mmap_capture_hw_avail(pcm); status->avail = snd_pcm_mmap_capture_avail(pcm); status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max); Why only playback needs the special handling while the capture doesn't? Again, some comments would be helpful for better understanding your changes.
I expect that it could be useful to do something similar with capture but I could not work out what would be needed. OTOH, I think that the way that capture is used, and the way the underlying audio drivers tend to deal with this issue for capture is such that it may not be of much value. I agree that comments are good. In general I was trying to stick with the pretty minimal-comment style of the existing code. Alan.
On Tue, 13 Dec 2016 12:43:04 +0100, Alan Young wrote:
Hi,
Sorry for the delay. I too got sidetracked by other work.
On 28/11/16 19:11, Takashi Iwai wrote:
@@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status) status->state = SND_PCM_STATE_RUNNING; status->trigger_tstamp = rate->trigger_tstamp; } - snd_pcm_rate_sync_hwptr(pcm); + snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr); This can't work.
I can fix it in my side, but OTOH, this made me wonder how you tested the patch...
Why do you think that cannot work? I am pretty sure it is working just fine for us but I am sure that it is possible that I have missed something.
Compare the argument you passed there and the function definition, it's obvious :) Takashi
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 Tue, 13 Dec 2016 14:05:36 +0100, Alan Young wrote:
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.
Applied now, thanks. Takashi
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_VERSION is incremented and SND_PCM_RATE_PLUGIN_VERSION_CONFIGURE is defined so that a converter plugin can test whether it should expect the extra parameter on its open func. Alan.
On Wed, 08 Feb 2017 11:50:44 +0100, Alan Young wrote:
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_VERSION is incremented and SND_PCM_RATE_PLUGIN_VERSION_CONFIGURE is defined so that a converter plugin can test whether it should expect the extra parameter on its open func.
Alan.
[1.2 <text/html; utf-8 (7bit)>]
From febb2e95682e99fee04f5853f783ba48748850b5 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 plugin open() function (SND_PCM_RATE_PLUGIN_ENTRY(XXX)).
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 | 19 ++++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/pcm_rate.h b/include/pcm_rate.h index 4d70df2..fb7ec55 100644 --- a/include/pcm_rate.h +++ b/include/pcm_rate.h @@ -38,7 +38,8 @@ extern "C" { /** * Protocol version */ -#define SND_PCM_RATE_PLUGIN_VERSION 0x010002 +#define SND_PCM_RATE_PLUGIN_VERSION 0x010003 +#define SND_PCM_RATE_PLUGIN_VERSION_CONFIGURE 0x010003
/** hw_params information for a single side */ typedef struct snd_pcm_rate_side_info { @@ -118,7 +119,7 @@ typedef struct snd_pcm_rate_ops {
/** open function type */ typedef int (*snd_pcm_rate_open_func_t)(unsigned int version, void **objp, - snd_pcm_rate_ops_t *opsp); + snd_pcm_rate_ops_t *opsp, const snd_config_t *conf);
The idea is interesting, but a devil lives always in the detail: you 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. 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. thanks, Takashi
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.
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
However ... the version number. I'll work up a revised patch. Thanks, Alan.
On 09/02/17 15:41, Alan Young wrote:
There is no (public) declaration of _snd_pcm_rate_/xxx/_open() function type
Oops, my bad - yes there is.
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 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...? thanks, Takashi
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 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.
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
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.
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 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; } } }
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
Updated patch to address latest comments.
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
Update patch. On 21/02/17 14:43, Takashi Iwai wrote:
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 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?
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 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.
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
participants (3)
-
Alan Young -
Alan Young -
Takashi Iwai