[PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

snd_pcm_avail_update() can call snd_pcm_jack_stop() but
snd_pcm_jack_stop() should not be called by the JACK thread.
It should only be called by the thread how has called
snd_pcm_jack_start().

In addition the execution of snd_pcm_avail_update() can take a while.
Therefore it should not be called by the JACK thread to not block this
thread.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index e3df4d2..b643d93 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -67,7 +67,7 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 
 	if (io->state == SND_PCM_STATE_RUNNING ||
 	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
-		avail = snd_pcm_avail_update(io->pcm);
+		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
 		if (avail >= 0 && avail < jack->min_avail) {
 			while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf))
 				;
@@ -84,7 +84,7 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
 	snd_pcm_sframes_t avail;
 	snd_pcm_jack_t *jack = io->private_data;
 
-	avail = snd_pcm_avail_update(io->pcm);
+	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
 	if (avail < 0 || avail >= jack->min_avail) {
 		write(jack->fd, &buf, 1);
 		return 1;
-- 
2.7.4

Re: [PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[Takashi Iwai]

On Tue, 03 Jul 2018 13:58:06 +0200,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> snd_pcm_avail_update() can call snd_pcm_jack_stop() but
> snd_pcm_jack_stop() should not be called by the JACK thread.
> It should only be called by the thread how has called
> snd_pcm_jack_start().
> 
> In addition the execution of snd_pcm_avail_update() can take a while.
> Therefore it should not be called by the JACK thread to not block this
> thread.
> 
> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

With that change, avail cannot be a negative, as
snd_pcm_ioplug_avail() returns snd_pcm_uframes_t.
A couple of checks are redundant and confuse readers as if it's still
a signed value.


Takashi


> 
> diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
> index e3df4d2..b643d93 100644
> --- a/jack/pcm_jack.c
> +++ b/jack/pcm_jack.c
> @@ -67,7 +67,7 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
>  
>  	if (io->state == SND_PCM_STATE_RUNNING ||
>  	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
> -		avail = snd_pcm_avail_update(io->pcm);
> +		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
>  		if (avail >= 0 && avail < jack->min_avail) {
>  			while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf))
>  				;
> @@ -84,7 +84,7 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
>  	snd_pcm_sframes_t avail;
>  	snd_pcm_jack_t *jack = io->private_data;
>  
> -	avail = snd_pcm_avail_update(io->pcm);
> +	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
>  	if (avail < 0 || avail >= jack->min_avail) {
>  		write(jack->fd, &buf, 1);
>  		return 1;
> -- 
> 2.7.4
> 

[PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

snd_pcm_avail_update() can call snd_pcm_jack_stop() but
snd_pcm_jack_stop() should not be called by the JACK thread.
It should only be called by the thread how has called
snd_pcm_jack_start().

In addition the execution of snd_pcm_avail_update() can take a while.
Therefore it should not be called by the JACK thread to not block this
thread.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index e3df4d2..5a57e13 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -62,13 +62,13 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
 static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 {
 	static char buf[32];
-	snd_pcm_sframes_t avail;
+	snd_pcm_uframes_t avail;
 	snd_pcm_jack_t *jack = io->private_data;
 
 	if (io->state == SND_PCM_STATE_RUNNING ||
 	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
-		avail = snd_pcm_avail_update(io->pcm);
-		if (avail >= 0 && avail < jack->min_avail) {
+		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
+		if (avail < jack->min_avail) {
 			while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf))
 				;
 			return 1;
@@ -81,11 +81,11 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
 {
 	static char buf[1];
-	snd_pcm_sframes_t avail;
+	snd_pcm_uframes_t avail;
 	snd_pcm_jack_t *jack = io->private_data;
 
-	avail = snd_pcm_avail_update(io->pcm);
-	if (avail < 0 || avail >= jack->min_avail) {
+	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
+	if (avail >= jack->min_avail) {
 		write(jack->fd, &buf, 1);
 		return 1;
 	}
-- 
2.7.4

Re: [PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[Takashi Iwai]

On Tue, 03 Jul 2018 14:23:59 +0200,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> snd_pcm_avail_update() can call snd_pcm_jack_stop() but
> snd_pcm_jack_stop() should not be called by the JACK thread.
> It should only be called by the thread how has called
> snd_pcm_jack_start().
> 
> In addition the execution of snd_pcm_avail_update() can take a while.
> Therefore it should not be called by the JACK thread to not block this
> thread.
> 
> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

Please put "v2" into the subject prefix, and give a brief information
what changed from v1 to v2.  And, don't attach to the old mail
thread.  As of this post, it's utterly confusing which patch is which
version!


thanks,

Takashi

> 
> diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
> index e3df4d2..5a57e13 100644
> --- a/jack/pcm_jack.c
> +++ b/jack/pcm_jack.c
> @@ -62,13 +62,13 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
>  static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
>  {
>  	static char buf[32];
> -	snd_pcm_sframes_t avail;
> +	snd_pcm_uframes_t avail;
>  	snd_pcm_jack_t *jack = io->private_data;
>  
>  	if (io->state == SND_PCM_STATE_RUNNING ||
>  	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
> -		avail = snd_pcm_avail_update(io->pcm);
> -		if (avail >= 0 && avail < jack->min_avail) {
> +		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
> +		if (avail < jack->min_avail) {
>  			while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf))
>  				;
>  			return 1;
> @@ -81,11 +81,11 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
>  static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
>  {
>  	static char buf[1];
> -	snd_pcm_sframes_t avail;
> +	snd_pcm_uframes_t avail;
>  	snd_pcm_jack_t *jack = io->private_data;
>  
> -	avail = snd_pcm_avail_update(io->pcm);
> -	if (avail < 0 || avail >= jack->min_avail) {
> +	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
> +	if (avail >= jack->min_avail) {
>  		write(jack->fd, &buf, 1);
>  		return 1;
>  	}
> -- 
> 2.7.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

[PATCH - JACK IO plug v2 0/3] snd_pcm_drain support

[twischer]

Now in a clean manner as composed patches.


pcm: ioplug: Provide avail helper function
---------
No changes


jack: Avoid call to snd_pcm_avail_update() from JACK thread
---------
v2 uses snd_pcm_uframes_t as type for avail and does not check for avail < 0
becasue snd_pcm_ioplug_avail() will never retrun a value < 0


jack: Update poll_fd also in draining state
---------
This patch is depending on [1]. Therefore it was rebased on top of [1].
Additionally the commit message was extended.

v1 of this patch can be found in [2].

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-July/137775.html
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-July/137769.html

[PATCH - JACK IO plug 1/1] pcm: ioplug: Provide avail helper function for plugins

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

This function can be called without calling snd_pcm_avail_update().

The call to snd_pcm_avail_update() can take some time.
Therefore some developers would not like to call it from a real-time
context (e.g. from JACK client context).

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
index c1310e3..b16fc8b 100644
--- a/include/pcm_ioplug.h
+++ b/include/pcm_ioplug.h
@@ -235,6 +235,9 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n
 int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
 
 /* calucalte the available frames */
+snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
+				       const snd_pcm_uframes_t hw_ptr,
+				       const snd_pcm_uframes_t appl_ptr);
 snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
 					  const snd_pcm_uframes_t hw_ptr,
 					  const snd_pcm_uframes_t appl_ptr);
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 4d44ae2..6d52c27 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -1221,6 +1221,21 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state)
  * \param ioplug the ioplug handle
  * \param hw_ptr hardware pointer in frames
  * \param appl_ptr application pointer in frames
+ * \return available frames for the application
+ */
+snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
+				       const snd_pcm_uframes_t hw_ptr,
+				       const snd_pcm_uframes_t appl_ptr)
+{
+	return __snd_pcm_avail(ioplug->pcm, hw_ptr, appl_ptr);
+}
+
+/**
+ * \brief Get the available frames. This function can be used to calculate the
+ * the available frames before calling #snd_pcm_avail_update()
+ * \param ioplug the ioplug handle
+ * \param hw_ptr hardware pointer in frames
+ * \param appl_ptr application pointer in frames
  * \return available frames for the hardware
  */
 snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
@@ -1230,8 +1245,9 @@ snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
 	/* available data/space which can be transferred by the user
 	 * application
 	 */
-	const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm,
-							     hw_ptr, appl_ptr);
+	const snd_pcm_uframes_t user_avail = snd_pcm_ioplug_avail(ioplug,
+								  hw_ptr,
+								  appl_ptr);
 
 	if (user_avail > ioplug->pcm->buffer_size) {
 		/* there was an Xrun */
-- 
2.7.4

[PATCH - JACK IO plug 1/2] jack: Avoid call to snd_pcm_avail_update() from JACK thread

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

snd_pcm_avail_update() can call snd_pcm_jack_stop() but
snd_pcm_jack_stop() should not be called by the JACK thread.
It should only be called by the thread how has called
snd_pcm_jack_start().

In addition the execution of snd_pcm_avail_update() can take a while.
Therefore it should not be called by the JACK thread to not block this
thread.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index e3df4d2..5a57e13 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -62,13 +62,13 @@ static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io);
 static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 {
 	static char buf[32];
-	snd_pcm_sframes_t avail;
+	snd_pcm_uframes_t avail;
 	snd_pcm_jack_t *jack = io->private_data;
 
 	if (io->state == SND_PCM_STATE_RUNNING ||
 	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
-		avail = snd_pcm_avail_update(io->pcm);
-		if (avail >= 0 && avail < jack->min_avail) {
+		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
+		if (avail < jack->min_avail) {
 			while (read(io->poll_fd, &buf, sizeof(buf)) == sizeof(buf))
 				;
 			return 1;
@@ -81,11 +81,11 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
 {
 	static char buf[1];
-	snd_pcm_sframes_t avail;
+	snd_pcm_uframes_t avail;
 	snd_pcm_jack_t *jack = io->private_data;
 
-	avail = snd_pcm_avail_update(io->pcm);
-	if (avail < 0 || avail >= jack->min_avail) {
+	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
+	if (avail >= jack->min_avail) {
 		write(jack->fd, &buf, 1);
 		return 1;
 	}
-- 
2.7.4

[PATCH - JACK IO plug 2/2] jack: Update poll_fd also in draining state

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

to support snd_pcm_drain for the JACK IO plugin.
With this changes there will be an poll_fd event in DRAINING state even if
the min_avail was not yet reached. Otherwise the application would never
recognize that all samples were processed by JACK.
In addition the JACK real-time thread is also processing when in DRAINING
state and not only when in RUNNING or PREPARE state.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index 5a57e13..98a6f7e 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -66,6 +66,7 @@ static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
 	snd_pcm_jack_t *jack = io->private_data;
 
 	if (io->state == SND_PCM_STATE_RUNNING ||
+	    io->state == SND_PCM_STATE_DRAINING ||
 	    (io->state == SND_PCM_STATE_PREPARED && io->stream == SND_PCM_STREAM_CAPTURE)) {
 		avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
 		if (avail < jack->min_avail) {
@@ -85,7 +86,12 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
 	snd_pcm_jack_t *jack = io->private_data;
 
 	avail = snd_pcm_ioplug_avail(io, jack->hw_ptr, io->appl_ptr);
-	if (avail >= jack->min_avail) {
+	/* In draining state poll_fd is used to wait
+	 * till all pending frames are played.
+	 * Therefore it has to be guarantee that a poll event is also generated
+	 * if the buffer contains less than min_avail frames
+	 */
+	if (avail >= jack->min_avail || io->state == SND_PCM_STATE_DRAINING) {
 		write(jack->fd, &buf, 1);
 		return 1;
 	}
@@ -161,7 +167,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 		jack->areas[channel].step = jack->sample_bits;
 	}
 
-	if (io->state == SND_PCM_STATE_RUNNING) {
+	if (io->state == SND_PCM_STATE_RUNNING ||
+	    io->state == SND_PCM_STATE_DRAINING) {
 		snd_pcm_uframes_t hw_ptr = jack->hw_ptr;
 		const snd_pcm_uframes_t hw_avail = snd_pcm_ioplug_hw_avail(io, hw_ptr,
 									   io->appl_ptr);
-- 
2.7.4

Re: [PATCH - JACK IO plug v2 0/3] snd_pcm_drain support

[Takashi Iwai]

On Tue, 03 Jul 2018 15:59:20 +0200,
<twischer@de.adit-jv.com> wrote:
> 
> 
> Now in a clean manner as composed patches.

Thanks, now applied all patches.

But just another reminder: don't forget to put "v2" to each patch
subject line, too, not only in the cover letter.


Takashi

Re: [PATCH - JACK IO plug 1/1] pcm: ioplug: Provide avail helper function for plugins

[Takashi Sakamoto]

Iwai-san,

On Jul 3 2018 22:59, twischer@de.adit-jv.com wrote:
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> This function can be called without calling snd_pcm_avail_update().
> 
> The call to snd_pcm_avail_update() can take some time.
> Therefore some developers would not like to call it from a real-time
> context (e.g. from JACK client context).
> 
> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
> 
> diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
> index c1310e3..b16fc8b 100644
> --- a/include/pcm_ioplug.h
> +++ b/include/pcm_ioplug.h
> @@ -235,6 +235,9 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n
>   int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
>   
>   /* calucalte the available frames */
> +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
> +				       const snd_pcm_uframes_t hw_ptr,
> +				       const snd_pcm_uframes_t appl_ptr);
>   snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
>   					  const snd_pcm_uframes_t hw_ptr,
>   					  const snd_pcm_uframes_t appl_ptr);
> diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
> index 4d44ae2..6d52c27 100644
> --- a/src/pcm/pcm_ioplug.c
> +++ b/src/pcm/pcm_ioplug.c
> @@ -1221,6 +1221,21 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state)
>    * \param ioplug the ioplug handle
>    * \param hw_ptr hardware pointer in frames
>    * \param appl_ptr application pointer in frames
> + * \return available frames for the application
> + */
> +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
> +				       const snd_pcm_uframes_t hw_ptr,
> +				       const snd_pcm_uframes_t appl_ptr)
> +{
> +	return __snd_pcm_avail(ioplug->pcm, hw_ptr, appl_ptr);
> +}
> +
> +/**
> + * \brief Get the available frames. This function can be used to calculate the
> + * the available frames before calling #snd_pcm_avail_update()
> + * \param ioplug the ioplug handle
> + * \param hw_ptr hardware pointer in frames
> + * \param appl_ptr application pointer in frames
>    * \return available frames for the hardware
>    */
>   snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
> @@ -1230,8 +1245,9 @@ snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
>   	/* available data/space which can be transferred by the user
>   	 * application
>   	 */
> -	const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm,
> -							     hw_ptr, appl_ptr);
> +	const snd_pcm_uframes_t user_avail = snd_pcm_ioplug_avail(ioplug,
> +								  hw_ptr,
> +								  appl_ptr);
>   
>   	if (user_avail > ioplug->pcm->buffer_size) {
>   		/* there was an Xrun */

I have a question about maintenance policy of this library to update 
version script for GNU Linker (ld) when introducing new symbols. The 
version script is not updated since node name as 'ALSA_0.9.7'. The newly
intduced symbol, 'snd_pcm_ioplug_hw_avail' is in node name of
'ALSA_0.9'.

```
$ readelf -s src/.libs/libasound.so.2.0.0 | grep snd_pcm_ioplug_hw_avail
   1254: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13 
snd_pcm_ioplug_hw_avail@@ALSA_0.9
   3840: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13 
snd_pcm_ioplug_hw_avail
```

The last node name is 'ALSA_1.1.6':

```
$ tail -n 10 src/Versions.in
   global:

     @SYMBOL_PREFIX@alsa_lisp_*;
} ALSA_0.9.5;

ALSA_1.1.6 {
   global:

     @SYMBOL_PREFIX@snd_dlopen;
} ALSA_0.9.7;
```

Practically this brings no issue, but theoretically the newly introduced 
symbol should be in a new node name next to ALSA_1.1.6. I'm not so 
strict in this point, but it's better to decide maintenance policy 
(because I've added some APIs in recent years).


Regards

Takashi Sakamoto

Re: [PATCH - JACK IO plug 1/1] pcm: ioplug: Provide avail helper function for plugins

[Takashi Iwai]

On Wed, 04 Jul 2018 02:34:47 +0200,
Takashi Sakamoto wrote:
> 
> Iwai-san,
> 
> On Jul 3 2018 22:59, twischer@de.adit-jv.com wrote:
> > From: Timo Wischer <twischer@de.adit-jv.com>
> >
> > This function can be called without calling snd_pcm_avail_update().
> >
> > The call to snd_pcm_avail_update() can take some time.
> > Therefore some developers would not like to call it from a real-time
> > context (e.g. from JACK client context).
> >
> > Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
> >
> > diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
> > index c1310e3..b16fc8b 100644
> > --- a/include/pcm_ioplug.h
> > +++ b/include/pcm_ioplug.h
> > @@ -235,6 +235,9 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n
> >   int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
> >     /* calucalte the available frames */
> > +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
> > +				       const snd_pcm_uframes_t hw_ptr,
> > +				       const snd_pcm_uframes_t appl_ptr);
> >   snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
> >   					  const snd_pcm_uframes_t hw_ptr,
> >   					  const snd_pcm_uframes_t appl_ptr);
> > diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
> > index 4d44ae2..6d52c27 100644
> > --- a/src/pcm/pcm_ioplug.c
> > +++ b/src/pcm/pcm_ioplug.c
> > @@ -1221,6 +1221,21 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state)
> >    * \param ioplug the ioplug handle
> >    * \param hw_ptr hardware pointer in frames
> >    * \param appl_ptr application pointer in frames
> > + * \return available frames for the application
> > + */
> > +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
> > +				       const snd_pcm_uframes_t hw_ptr,
> > +				       const snd_pcm_uframes_t appl_ptr)
> > +{
> > +	return __snd_pcm_avail(ioplug->pcm, hw_ptr, appl_ptr);
> > +}
> > +
> > +/**
> > + * \brief Get the available frames. This function can be used to calculate the
> > + * the available frames before calling #snd_pcm_avail_update()
> > + * \param ioplug the ioplug handle
> > + * \param hw_ptr hardware pointer in frames
> > + * \param appl_ptr application pointer in frames
> >    * \return available frames for the hardware
> >    */
> >   snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
> > @@ -1230,8 +1245,9 @@ snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
> >   	/* available data/space which can be transferred by the user
> >   	 * application
> >   	 */
> > -	const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm,
> > -							     hw_ptr, appl_ptr);
> > +	const snd_pcm_uframes_t user_avail = snd_pcm_ioplug_avail(ioplug,
> > +								  hw_ptr,
> > +								  appl_ptr);
> >     	if (user_avail > ioplug->pcm->buffer_size) {
> >   		/* there was an Xrun */
> 
> I have a question about maintenance policy of this library to update
> version script for GNU Linker (ld) when introducing new symbols. The
> version script is not updated since node name as 'ALSA_0.9.7'. The
> newly
> intduced symbol, 'snd_pcm_ioplug_hw_avail' is in node name of
> 'ALSA_0.9'.
> 
> ```
> $ readelf -s src/.libs/libasound.so.2.0.0 | grep snd_pcm_ioplug_hw_avail
>   1254: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13
> snd_pcm_ioplug_hw_avail@@ALSA_0.9
>   3840: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13
> snd_pcm_ioplug_hw_avail
> ```
> 
> The last node name is 'ALSA_1.1.6':
> 
> ```
> $ tail -n 10 src/Versions.in
>   global:
> 
>     @SYMBOL_PREFIX@alsa_lisp_*;
> } ALSA_0.9.5;
> 
> ALSA_1.1.6 {
>   global:
> 
>     @SYMBOL_PREFIX@snd_dlopen;
> } ALSA_0.9.7;
> ```
> 
> Practically this brings no issue, but theoretically the newly
> introduced symbol should be in a new node name next to ALSA_1.1.6. I'm
> not so strict in this point, but it's better to decide maintenance
> policy (because I've added some APIs in recent years).

Yes, that's an open question.

I myself am no big fan of the versioned symbols.  This has been a PITA
for many applications.  The versioned smybols is useful only if the
function signature may change.  But what's the difference from
renaming the function, then?

So we've stopped putting the new symbols into the new versioned
section; the snd_dlopen was an exception because it really changed the
signature.


thanks,

Takashi

Re: [PATCH - JACK IO plug 1/1] pcm: ioplug: Provide avail helper function for plugins

[Takashi Sakamoto]

Hi,

On Jul 4 2018 15:21, Takashi Iwai wrote:
> On Wed, 04 Jul 2018 02:34:47 +0200,
> Takashi Sakamoto wrote:
>>
>> Iwai-san,
>>
>> On Jul 3 2018 22:59, twischer@de.adit-jv.com wrote:
>>> From: Timo Wischer <twischer@de.adit-jv.com>
>>>
>>> This function can be called without calling snd_pcm_avail_update().
>>>
>>> The call to snd_pcm_avail_update() can take some time.
>>> Therefore some developers would not like to call it from a real-time
>>> context (e.g. from JACK client context).
>>>
>>> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
>>>
>>> diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
>>> index c1310e3..b16fc8b 100644
>>> --- a/include/pcm_ioplug.h
>>> +++ b/include/pcm_ioplug.h
>>> @@ -235,6 +235,9 @@ int snd_pcm_ioplug_set_param_list(snd_pcm_ioplug_t *io, int type, unsigned int n
>>>    int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
>>>      /* calucalte the available frames */
>>> +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
>>> +				       const snd_pcm_uframes_t hw_ptr,
>>> +				       const snd_pcm_uframes_t appl_ptr);
>>>    snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
>>>    					  const snd_pcm_uframes_t hw_ptr,
>>>    					  const snd_pcm_uframes_t appl_ptr);
>>> diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
>>> index 4d44ae2..6d52c27 100644
>>> --- a/src/pcm/pcm_ioplug.c
>>> +++ b/src/pcm/pcm_ioplug.c
>>> @@ -1221,6 +1221,21 @@ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state)
>>>     * \param ioplug the ioplug handle
>>>     * \param hw_ptr hardware pointer in frames
>>>     * \param appl_ptr application pointer in frames
>>> + * \return available frames for the application
>>> + */
>>> +snd_pcm_uframes_t snd_pcm_ioplug_avail(const snd_pcm_ioplug_t * const ioplug,
>>> +				       const snd_pcm_uframes_t hw_ptr,
>>> +				       const snd_pcm_uframes_t appl_ptr)
>>> +{
>>> +	return __snd_pcm_avail(ioplug->pcm, hw_ptr, appl_ptr);
>>> +}
>>> +
>>> +/**
>>> + * \brief Get the available frames. This function can be used to calculate the
>>> + * the available frames before calling #snd_pcm_avail_update()
>>> + * \param ioplug the ioplug handle
>>> + * \param hw_ptr hardware pointer in frames
>>> + * \param appl_ptr application pointer in frames
>>>     * \return available frames for the hardware
>>>     */
>>>    snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
>>> @@ -1230,8 +1245,9 @@ snd_pcm_uframes_t snd_pcm_ioplug_hw_avail(const snd_pcm_ioplug_t * const ioplug,
>>>    	/* available data/space which can be transferred by the user
>>>    	 * application
>>>    	 */
>>> -	const snd_pcm_uframes_t user_avail = __snd_pcm_avail(ioplug->pcm,
>>> -							     hw_ptr, appl_ptr);
>>> +	const snd_pcm_uframes_t user_avail = snd_pcm_ioplug_avail(ioplug,
>>> +								  hw_ptr,
>>> +								  appl_ptr);
>>>      	if (user_avail > ioplug->pcm->buffer_size) {
>>>    		/* there was an Xrun */
>>
>> I have a question about maintenance policy of this library to update
>> version script for GNU Linker (ld) when introducing new symbols. The
>> version script is not updated since node name as 'ALSA_0.9.7'. The
>> newly
>> intduced symbol, 'snd_pcm_ioplug_hw_avail' is in node name of
>> 'ALSA_0.9'.
>>
>> ```
>> $ readelf -s src/.libs/libasound.so.2.0.0 | grep snd_pcm_ioplug_hw_avail
>>    1254: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13
>> snd_pcm_ioplug_hw_avail@@ALSA_0.9
>>    3840: 000000000009bfe0    40 FUNC    GLOBAL DEFAULT   13
>> snd_pcm_ioplug_hw_avail
>> ```
>>
>> The last node name is 'ALSA_1.1.6':
>>
>> ```
>> $ tail -n 10 src/Versions.in
>>    global:
>>
>>      @SYMBOL_PREFIX@alsa_lisp_*;
>> } ALSA_0.9.5;
>>
>> ALSA_1.1.6 {
>>    global:
>>
>>      @SYMBOL_PREFIX@snd_dlopen;
>> } ALSA_0.9.7;
>> ```
>>
>> Practically this brings no issue, but theoretically the newly
>> introduced symbol should be in a new node name next to ALSA_1.1.6. I'm
>> not so strict in this point, but it's better to decide maintenance
>> policy (because I've added some APIs in recent years).
> 
> Yes, that's an open question.
> 
> I myself am no big fan of the versioned symbols.  This has been a PITA
> for many applications.  The versioned smybols is useful only if the
> function signature may change.  But what's the difference from
> renaming the function, then?
> 
> So we've stopped putting the new symbols into the new versioned
> section; the snd_dlopen was an exception because it really changed the
> signature.

Nowadays I have no positive reason to use versioned symbols as you said.
I'm OK and thanks for your explanation.


Takashi Sakamoto