[PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[sylvain.bertrand]

once draining is done, the pcm enters the SETUP state, which ought to
be valid for snd_pcm_drain()

signed-off-by: Sylvain BERTRAND <sylvain.bertrand@legeek.net>
---

I missed this one in my previous patch because exiting with or without
an error once draining is done was producing the same result.

--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -1329,7 +1329,7 @@ int snd_pcm_drain(snd_pcm_t *pcm)
 		SNDMSG("PCM not set up");
 		return -EIO;
 	}
-	err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+	err = bad_pcm_state(pcm, P_STATE_RUNNABLE | P_STATE(SETUP));
 	if (err < 0)
 		return err;
 	/* lock handled in the callback */

Re: [PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[sylvain.bertrand]

ping?

-- 
Sylvain

Re: [PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[Jaroslav Kysela]

Dne 02. 05. 20 v 21:33 sylvain.bertrand@gmail.com napsal(a):
> once draining is done, the pcm enters the SETUP state, which ought to
> be valid for snd_pcm_drain()
> 
> signed-off-by: Sylvain BERTRAND <sylvain.bertrand@legeek.net>
> ---
> 
> I missed this one in my previous patch because exiting with or without
> an error once draining is done was producing the same result.

NAK: You should not call drain when the PCM handle is in the SETUP field. It's 
an obvious caller problem. The streaming should be active somehow.

						Jaroslav

> 
> --- a/src/pcm/pcm.c
> +++ b/src/pcm/pcm.c
> @@ -1329,7 +1329,7 @@ int snd_pcm_drain(snd_pcm_t *pcm)
>   		SNDMSG("PCM not set up");
>   		return -EIO;
>   	}
> -	err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
> +	err = bad_pcm_state(pcm, P_STATE_RUNNABLE | P_STATE(SETUP));
>   	if (err < 0)
>   		return err;
>   	/* lock handled in the callback */
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[sylvain.bertrand]

On Thu, May 14, 2020 at 03:52:25PM +0200, Jaroslav Kysela wrote:
> NAK: You should not call drain when the PCM handle is in the SETUP field.
> It's an obvious caller problem. The streaming should be active somehow.

The pb here is the non-blocking calls of the drain function: in my test case,
the first call to the drain function switches the pcm in draining state, but
the pcm will be switched to the setup state somewhen in between 2 drain function
calls! Naively, I was calling the drain function on a regular time basis to see
if the draining was finished, namely expecting 0 to be returned.

Then if I understood you well, the right way(tm) to use the drain function in
non-block mode, is to call only once the drain function, then inspect the state
of the pcm till it not anymore in the draining state. 

Am I right? Or did I miss something again?

regards,

-- 
Sylvain

[PATCH] augment snd_pcm_drain() documentation regarding its non-blocking mode usage

[sylvain.bertrand]

augment the documentation regarding the use of the snd_pcm_drain function in
non-blocking mode.

signed-off-by: Sylvain BERTRAND <sylvain.bertrand@legeek.net>
---
 src/pcm/pcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
---
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 1064044c..0d4b2930 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -1311,8 +1311,14 @@ int snd_pcm_drop(snd_pcm_t *pcm)
  * \return 0 on success otherwise a negative error code
  * \retval -ESTRPIPE a suspend event occurred
  *
- * For playback wait for all pending frames to be played and then stop
- * the PCM.
+ * For playback, in blocking mode,  wait for all pending frames to be played
+ * and then stop the PCM.
+ * For playback, in non-blocking mode, will return -EAGAIN if the pcm is still
+ * being drained at the time of the call. A note of caution: the pcm can finish
+ * draining asynchronously from a snd_pcm_draw call. The pcm will be then in
+ * SND_PCM_STATE_SETUP state which means any subsequent calls to snd_pcm_drain
+ * will fail since you cannot switch the pcm to SND_PCM_STATE_DRAINING state
+ * from SND_PCM_STATE_SETUP state.
  * For capture stop PCM permitting to retrieve residual frames.
  *
  * For stopping the PCM stream immediately, use \link ::snd_pcm_drop() \endlink

Re: [PATCH] augment snd_pcm_drain() documentation regarding its non-blocking mode usage

[sylvain.bertrand]

ping?

regards,

-- 
Sylvain

Re: [PATCH] fix snd_pcm_drain() excluding SETUP state from valid states

[Jaroslav Kysela]

Dne 14. 05. 20 v 23:06 sylvain.bertrand@gmail.com napsal(a):
> On Thu, May 14, 2020 at 03:52:25PM +0200, Jaroslav Kysela wrote:
>> NAK: You should not call drain when the PCM handle is in the SETUP field.
>> It's an obvious caller problem. The streaming should be active somehow.
> 
> The pb here is the non-blocking calls of the drain function: in my test case,
> the first call to the drain function switches the pcm in draining state, but
> the pcm will be switched to the setup state somewhen in between 2 drain function
> calls! Naively, I was calling the drain function on a regular time basis to see
> if the draining was finished, namely expecting 0 to be returned.
> 
> Then if I understood you well, the right way(tm) to use the drain function in
> non-block mode, is to call only once the drain function, then inspect the state
> of the pcm till it not anymore in the draining state.
> 
> Am I right? Or did I miss something again?

I looked to this problem again and the original patch seems more appropriate. 
The snd_pcm_drain() should return zero, if the state is SETUP, because there 
is no further work.

I applied your patch:

https://github.com/alsa-project/alsa-lib/commit/1b9104b5ff10be7f60441f622436d4f14a2a97d1

with the (sanity) optimization in:

https://github.com/alsa-project/alsa-lib/commit/0b7f1441bb82903d45a29bf83c849ca94c5b7d7e

It basically doesn't allow to call the plugin callback (otherwise we need to 
review all plugin drain callbacks, if the SETUP state is handled properly).

			Thank you for your suggestion,
						Jaroslav

> 
> regards,
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.