[alsa-devel] SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwai<tiwai at suse.de> wrote:
> At Sat, 15 Aug 2009 23:40:36 -0400,
> Jon Smirl wrote:
>>
>> On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirl<jonsmirl at gmail.com> wrote:
>> > void
>> > bfio_synch_stop(void)
>> > {
>> >    int n;
>> >
>> >    if (base_handle == NULL) {
>> >        return;
>> >    }
>> >    FOR_IN_AND_OUT {
>> >        for (n = 0; n < n_handles[IO]; n++) {
>>
>> I added:
>> snd_pcm_nonblock(handles[IO][n], 0)
>> snd_pcm_drain(handles[IO][n])
>> snd_pcm_nonblock(handles[IO][n], SND_PCM_NONBLOCK )
>>
>> >            snd_pcm_close(handles[IO][n]);
>> >        }
>> >    }
>> > }
>>
>> This is not working correctly.
>> snd_pcm_nonblock(handles[IO][n], 0)
>> It does not remove O_NONBLOCK for some unknown reason.
>>
>> I added printf() to snd_pcm_hw_nonblock()
>> The fcntl is not getting an error.
>>       if (fcntl(fd, F_SETFL, flags) < 0) {
>> Flags being set are 2 (O_RDWR).
>>
>> But when I get over to snd_pcm_pre_drain_init(), I get the -EAGAIN error.
>> static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream,
>> int state)
>> {
>>       printk("snd_pcm_pre_drain_init\n");
>>       if (substream->f_flags & O_NONBLOCK)
>>               return -EAGAIN;
>>       printk("snd_pcm_pre_drain_init 1\n");
>>       substream->runtime->trigger_master = substream;
>>       return 0;
>> }
>> So I have to conclude that fcntl(fd, F_SETFL, flags) is not removing
>> the O_NONBLOCK flag.
>
> Yeah, you found a long-standing bug :)
>
> Honestly, I think the current designed behavior is just annoying.
> An ioctl may be blocked, thus there is no real merit to return -EAGAIN
> with DRAIN ioctl.

Brutefir is a server type app, so pulseaudio should be having trouble
with this too.

Brutefir is processing audio asynchronously while monitoring for
network traffic and doing background FIR computations. The FIR
operation is heavily pipelined so it is using a 2MB ALSA buffer.

So what happens when the source stream ends? Brutefir wants to close
(or mute) the device when the stream is finished.  What brutefir wants
to do is sit in a loop calling drain() until it stops returning
EAGAIN.  That way it can keep monitoring the network for incoming
traffic.  When drain stops returning EAGAIN it is safe to close the
device.  An alternative solution is to use a thread with a blocking
drain(). But brutefir is not currently threaded.

The current brutefir code just ignores the drain state and chops off
the last 2MB of the stream. That's probably because streams don't end
very often.

I noticed this because I was sending test files one at a time to
brutefir instead of streaming. My test files would play a couple of
seconds and then get chopped off.

So what does pulse do when it runs out of input data? Does it feed the
device zeros or close/mute it?



>
> So, my preferred solution is simply to remove the O_NONBLOCK check
> in the code path above.
>
> Another solution would be a patch like below (totally untested)...
>
> Comments?
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index d89c816..5d6b9ad 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1343,8 +1343,6 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream,
>
>  static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state)
>  {
> -       if (substream->f_flags & O_NONBLOCK)
> -               return -EAGAIN;
>        substream->runtime->trigger_master = substream;
>        return 0;
>  }
> @@ -1404,7 +1402,8 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream);
>  * After this call, all streams are supposed to be either SETUP or DRAINING
>  * (capture only) state.
>  */
> -static int snd_pcm_drain(struct snd_pcm_substream *substream)
> +static int snd_pcm_drain(struct snd_pcm_substream *substream,
> +                        struct file *file)
>  {
>        struct snd_card *card;
>        struct snd_pcm_runtime *runtime;
> @@ -1412,6 +1411,14 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream)
>        int result = 0;
>        int i, num_drecs;
>        struct drain_rec *drec, drec_tmp, *d;
> +       int f_flags;
> +
> +       if (file)
> +               f_flags = file->f_flags;
> +       else
> +               f_flags = substream->f_flags;
> +       if (f_flags & O_NONBLOCK)
> +               return -EAGAIN;
>
>        card = substream->pcm->card;
>        runtime = substream->runtime;
> @@ -2556,7 +2563,7 @@ static int snd_pcm_common_ioctl1(struct file *file,
>                return snd_pcm_hw_params_old_user(substream, arg);
>  #endif
>        case SNDRV_PCM_IOCTL_DRAIN:
> -               return snd_pcm_drain(substream);
> +               return snd_pcm_drain(substream, file);
>        case SNDRV_PCM_IOCTL_DROP:
>                return snd_pcm_drop(substream);
>        case SNDRV_PCM_IOCTL_PAUSE:
>



-- 
Jon Smirl
jonsmirl at gmail.com