[PATCH] pcm_rate: Do not discard slave reported delay in status result.

[PATCH] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 33 bytes --]

Similar to recent dshare patch.


[-- Attachment #2: 0001-pcm_rate-Do-not-discard-slave-reported-delay-in-stat.patch --]
[-- Type: text/x-patch, Size: 3665 bytes --]

>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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 97 bytes --]

On 17/11/16 15:24, Alan Young wrote:
>
> Similar to recent dshare patch.
>
Update with sign-off


[-- Attachment #2: 0001-pcm_rate-Do-not-discard-slave-reported-delay-in-stat.patch --]
[-- Type: text/x-patch, Size: 3716 bytes --]

>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);
+	}
 	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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai]

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

Re: [PATCH] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

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?

Re: [PATCH] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai]

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

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 97 bytes --]

On 17/11/16 15:24, Alan Young wrote:
>
> Similar to recent dshare patch.
>
Update with sign-off


[-- Attachment #2: 0001-pcm_rate-Do-not-discard-slave-reported-delay-in-stat.patch --]
[-- Type: text/x-patch, Size: 3717 bytes --]

>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);
+	}
 	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



[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai]

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

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

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.

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai]

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

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

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.

[-- Attachment #2: 0001-pcm_rate-Do-not-discard-slave-reported-delay-in-stat.patch --]
[-- Type: text/x-patch, Size: 3844 bytes --]

>From b3aacdcee7431142748d675ebc8792cb0ca02630 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 | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 6184def..f340b67 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,20 @@ 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(pcm, 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);
+					// Maybe possible to somthing similar to
+					// snd_pcm_rate_playback_internal_delay()
+					// for the capture case.
 		status->avail = snd_pcm_mmap_capture_avail(pcm);
 		status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max);
 	}
-- 
2.5.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RESEND] pcm_rate: Do not discard slave reported delay in status result.

[Takashi Iwai]

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

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

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.


[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 4466 bytes --]

>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);
 
 /**
  * Define the object entry for external PCM rate-converter plugins
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index cbb7618..c0a60d7 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1258,7 +1258,7 @@ 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;
 	snd_pcm_rate_open_func_t open_func;
@@ -1279,7 +1279,7 @@ static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
 	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);
+	err = open_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)
@@ -1292,7 +1292,7 @@ static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
 	/* try to open with the old protocol version */
 	rate->plugin_version = SND_PCM_RATE_PLUGIN_VERSION_OLD;
 	err = open_func(SND_PCM_RATE_PLUGIN_VERSION_OLD,
-			&rate->obj, &rate->ops);
+			&rate->obj, &rate->ops, NULL);
 	if (err) {
 		snd_dlobj_cache_put(open_func);
 		rate->open_func = NULL;
@@ -1353,21 +1353,21 @@ 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);
 			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;
 		}
@@ -1386,7 +1386,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 +1439,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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

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

[Alan Young]

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.

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

[Alan Young]

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.

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

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.


[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 5539 bytes --]

>From 28ce5b014c922a95f9398fad47c7c02fca94665c 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 | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 43 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..bbfe4b1 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1258,26 +1258,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 +1375,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 && strcmp(id, "0") != 0)
+				break; /* not a simple array of types */
 		}
 	} else {
 		SNDERR("Invalid type for rate converter");
@@ -1386,7 +1411,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 +1464,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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

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

[Alan Young]

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.

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

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.


[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 5782 bytes --]

>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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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

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

[Alan Young]

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.

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

[Takashi Iwai]

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

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

[Alan Young]

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;
				}
			}
		}

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

[Takashi Iwai]

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

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 42 bytes --]

Updated patch to address latest comments.

[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 6226 bytes --]

>From e15901209b4c8902c79ba1da5a025f40adb9b1b1 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>
---
 include/pcm_rate.h |  5 +++-
 src/pcm/pcm_rate.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 79 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..6f26724 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1258,26 +1258,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 && converter_conf != NULL);
+	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) {
@@ -1301,6 +1323,30 @@ static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
 }
 #endif
 
+/*
+ * If the conf is an array of alternatives then the id of
+ * the first element will be "0" (or maybe NULL). Otherwise assume it is
+ * a structure.
+ */
+static int is_string_array(const snd_config_t *conf)
+{
+	snd_config_iterator_t i;
+
+	if (snd_config_get_type(conf) != SND_CONFIG_TYPE_COMPOUND)
+		return 0;
+
+	i = snd_config_iterator_first(conf);
+	if (i && i != snd_config_iterator_end(conf)) {
+		snd_config_t *n = snd_config_iterator_entry(i);
+		const char *id;
+		snd_config_get_id(n, &id);
+		if (id && strcmp(id, "0") != 0)
+			return 0;
+	}
+
+	return 1;
+}
+
 /**
  * \brief Creates a new rate PCM
  * \param pcmp Returns created PCM handle
@@ -1353,24 +1399,39 @@ 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);
-	else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
+		err = rate_open_func(rate, type, NULL, 1);
+	else if (is_string_array(converter)) {
 		snd_config_iterator_t i, next;
 		snd_config_for_each(i, next, converter) {
 			snd_config_t *n = snd_config_iterator_entry(i);
 			if (snd_config_get_string(n, &type) < 0)
 				break;
-			err = rate_open_func(rate, type, 0);
+			err = rate_open_func(rate, type, NULL, 0);
 			if (!err)
 				break;
 		}
+	}
+	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);
+		}
 	} else {
 		SNDERR("Invalid type for rate converter");
 		snd_pcm_free(pcm);
@@ -1439,6 +1500,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

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


[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 6294 bytes --]

>From 6eb392e6e2b5749bd8f2c67907b2a08da8cfe863 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>
---
 include/pcm_rate.h |  5 +++-
 src/pcm/pcm_rate.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 81 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..2021247 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1258,26 +1258,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 && converter_conf != NULL);
+	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) {
@@ -1301,6 +1323,30 @@ static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
 }
 #endif
 
+/*
+ * If the conf is an array of alternatives then the id of
+ * the first element will be "0" (or maybe NULL). Otherwise assume it is
+ * a structure.
+ */
+static int is_string_array(const snd_config_t *conf)
+{
+	snd_config_iterator_t i;
+
+	if (snd_config_get_type(conf) != SND_CONFIG_TYPE_COMPOUND)
+		return 0;
+
+	i = snd_config_iterator_first(conf);
+	if (i && i != snd_config_iterator_end(conf)) {
+		snd_config_t *n = snd_config_iterator_entry(i);
+		const char *id;
+		snd_config_get_id(n, &id);
+		if (id && strcmp(id, "0") != 0)
+			return 0;
+	}
+
+	return 1;
+}
+
 /**
  * \brief Creates a new rate PCM
  * \param pcmp Returns created PCM handle
@@ -1353,24 +1399,41 @@ 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);
-	else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
+		err = rate_open_func(rate, type, NULL, 1);
+	else if (is_string_array(converter)) {
 		snd_config_iterator_t i, next;
 		snd_config_for_each(i, next, converter) {
 			snd_config_t *n = snd_config_iterator_entry(i);
 			if (snd_config_get_string(n, &type) < 0)
 				break;
-			err = rate_open_func(rate, type, 0);
+			err = rate_open_func(rate, type, NULL, 0);
 			if (!err)
 				break;
 		}
+	}
+	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;
+		}
+		err = rate_open_func(rate, type, converter, 1);
 	} else {
 		SNDERR("Invalid type for rate converter");
 		snd_pcm_free(pcm);
@@ -1439,6 +1502,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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

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

[Alan Young]

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?

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

[Takashi Iwai]

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

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

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 153 bytes --]

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.

[-- Attachment #2: 0001-pcm-rate-Add-capability-to-pass-configuration-node-t.patch --]
[-- Type: text/x-patch, Size: 6333 bytes --]

>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>
---
 include/pcm_rate.h |  5 +++-
 src/pcm/pcm_rate.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 83 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..d439f4a 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1258,26 +1258,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 && converter_conf != NULL);
+	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) {
@@ -1301,6 +1323,30 @@ static int rate_open_func(snd_pcm_rate_t *rate, const char *type, int verbose)
 }
 #endif
 
+/*
+ * If the conf is an array of alternatives then the id of
+ * the first element will be "0" (or maybe NULL). Otherwise assume it is
+ * a structure.
+ */
+static int is_string_array(const snd_config_t *conf)
+{
+	snd_config_iterator_t i;
+
+	if (snd_config_get_type(conf) != SND_CONFIG_TYPE_COMPOUND)
+		return 0;
+
+	i = snd_config_iterator_first(conf);
+	if (i && i != snd_config_iterator_end(conf)) {
+		snd_config_t *n = snd_config_iterator_entry(i);
+		const char *id;
+		snd_config_get_id(n, &id);
+		if (id && strcmp(id, "0") != 0)
+			return 0;
+	}
+
+	return 1;
+}
+
 /**
  * \brief Creates a new rate PCM
  * \param pcmp Returns created PCM handle
@@ -1353,24 +1399,43 @@ 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);
-	else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
+		err = rate_open_func(rate, type, NULL, 1);
+	else if (is_string_array(converter)) {
 		snd_config_iterator_t i, next;
 		snd_config_for_each(i, next, converter) {
 			snd_config_t *n = snd_config_iterator_entry(i);
 			if (snd_config_get_string(n, &type) < 0)
 				break;
-			err = rate_open_func(rate, type, 0);
+			err = rate_open_func(rate, type, NULL, 0);
 			if (!err)
 				break;
 		}
+	}
+	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");
+			snd_pcm_free(pcm);
+			free(rate);
+			return -EINVAL;
+		}
+		err = rate_open_func(rate, type, converter, 1);
 	} else {
 		SNDERR("Invalid type for rate converter");
 		snd_pcm_free(pcm);
@@ -1439,6 +1504,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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

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