[alsa-devel] alsa-lib: commit ce2095c41f2891c51f5dbd28e0317200314c5a75 breaks speaker-test/pulseaudio

[Takashi Iwai]

On Wed, 19 Dec 2018 03:29:38 +0100,
Diego Viola wrote:
> 
> Hello Takashi,
> 
> I currently have an issue with speaker-test and pulseaudio, and the
> root of the problem seems to be a regression in alsa-lib.
> 
> The issue occurs when I run speaker-test, it creates endless output
> with no sound, and the output does not stop until I hit ctrl-c.
> 
> I've been investigating about this issue and concluded (through a git
> bisect) that this is a regression in alsa-lib, and this is the bad
> commit that introduces the bug:
> 
> commit ce2095c41f2891c51f5dbd28e0317200314c5a75
> Author: Takashi Iwai <tiwai at suse.de>
> Date:   Thu Mar 29 09:51:46 2018 +0200
> 
>     pcm: ioplug: Implement proper drain behavior
> 
>     This patch fixes the draining behavior of ioplug in the following
>     ways:
> 
>     - When no draining ioplug callback is defined, implement the draining
>       loop using snd_pcm_wait*() and sync with the drain finishes.
>       This is equivalent with the implementation in the kernel write().
>       Similarly as in kernel code, for non-blocking mode, it returns
>       immediately after setting DRAINING state.
> 
>     - The hw_ptr update function checks the PCM state and stops the stream
>       if the draining finishes.
> 
>     - When draining ioplug callback is defined, leave the whole draining
>       operation to it.  The callback is supposed to return -EAGAIN for
>       non-blocking case, too.
> 
>     - When an error happens during draining, it drops the stream, for a
>       safety reason.
> 
>     Signed-off-by: Takashi Iwai <tiwai at suse.de>
> 
> I've sanity checked and can confirm that going back to the previous
> commit fixes the issue.
> 
> My original bug report on the pulseaudio bug tracker:
> https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/593
> 
> Thanks, and please let me know if you need more information.

Thanks for the report.  Actually this looks rather like a bug in pulse
plugin itself.  The commit above starts the stream before calling
pa_stream_drain(), and this seems causing the problem.  IOW,
pa_stream_drain() doesn't work as advertised.

In anyway, we should fix the regression and in this case it's safer to
cover in the alsa-lib side.  Below is the fix patch.  Please give it a
try.


Takashi

-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] pcm: ioplug: Fix the regression of pulse plugin drain

The recent change to support the drain via polling caused a regression
for pulse plugin; with speaker-test -c2 -twav with pulse, it leads to
either no sounds or stall.

The only sensible behavior change in the commit wrt pulse plugin is
that now it starts the stream before calling drain callback.  This
supposed to be correct, but it seems hitting a pulse plugin bug.

The start before drain callback is only a matter of consistency, and
since this doesn't work for the single existing plugin using drain
callback, we don't need to stick with this behavior.

For addressing the regression, we check the presence of the drain
callback and start the stream only when it doesn't exist, i.e. only in
drain-via-poll mode.

Fixes: ce2095c41f28 ("pcm: ioplug: Implement proper drain behavior")
Reported-by: Diego Viola <diego.viola at gmail.com>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 src/pcm/pcm_ioplug.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 881a1a85adaf..1e25190a0f71 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -537,9 +537,11 @@ static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
 		return -EBADFD;
 	case SND_PCM_STATE_PREPARED:
 		if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
-			err = snd_pcm_ioplug_start(pcm);
-			if (err < 0)
-				goto unlock;
+			if (!io->data->callback->drain) {
+				err = snd_pcm_ioplug_start(pcm);
+				if (err < 0)
+					goto unlock;
+			}
 			io->data->state = SND_PCM_STATE_DRAINING;
 		}
 		break;
-- 
2.19.2