Re: [Winmm/winealsa] Don't use asynchronous callbacks in dsound any more

[Eric Pouech]

Maarten,

nice work on Alsa (which is definitively needed !!)

a couple of comments:
- I know the winmm code (and drivers) is crippled by bad synchronisation 
tricks (like cleaning a field in a structure to signal a thread). This 
is bad (TM). So I'd suggest using here a real synchronisation object.
- I wonder if it's a good idea to create another thread for this... but 
before merging all existing drivers' threads into a single one, I'd 
suggest using the fd oriented functions & poll (see 
snd_pcm_poll_descriptors and friends) instead of snd_pcm_wait, that'll 
ease up the future work.

Maarten Lankhorst a écrit :
> Instead of using asynchronous callbacks that uses signals, use a
> seperate thread that can be cancelled, this prevents deadlock issues.
>
> Basically we use snd_pcm_wait() that tells us when enough room is free
> to commit another buffer, then we commit the previous buffer and make
> the next buffer ready.
>
> Since snd_pcm_wait() uses poll(), we don't have signals in winealsa 
> any more.
> ------------------------------------------------------------------------
>
> diff --git a/dlls/winmm/winealsa/audio.c b/dlls/winmm/winealsa/audio.c
> index 6043680..e1676ed 100644
> --- a/dlls/winmm/winealsa/audio.c
> +++ b/dlls/winmm/winealsa/audio.c
> @@ -6,6 +6,7 @@
>   * Copyright    2002 Eric Pouech
>   *              2002 Marco Pietrobono
>   *              2003 Christian Costa : WaveIn support
> + *              2006 Maarten Lankhorst
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -70,11 +71,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(wave);
>
> #ifdef HAVE_ALSA
>
> -/* internal ALSALIB functions */
> -/* FIXME:  we shouldn't be using internal functions... */
> -snd_pcm_uframes_t _snd_pcm_mmap_hw_ptr(snd_pcm_t *pcm);
> -
> -
>  /* state diagram for waveOut writing:
>   *
>   * +---------+-------------+---------------+---------------------------------+
> @@ -2627,7 +2623,6 @@ #undef EXIT_ON_ERROR
>      	snd_pcm_hw_params_free(wwo->hw_params);
>      wwo->hw_params = hw_params;
>
> -
>      return wodNotifyClient(wwo, WOM_OPEN, 0L, 0L);
>
> errexit:
> @@ -3045,11 +3040,8 @@ struct IDsDriverBufferImpl
>      DWORD                     mmap_buflen_bytes;
>      snd_pcm_uframes_t         mmap_buflen_frames;
>      snd_pcm_channel_area_t *  mmap_areas;
> -    snd_async_handler_t *     mmap_async_handler;
>      snd_pcm_uframes_t	      mmap_ppos; /* play position */
> -    
> -    /* Do we have a direct hardware buffer - SND_PCM_TYPE_HW? */
> -    int                       mmap_mode;
> +    HANDLE                    mmap_thread;
>  };
>
> static void DSDB_CheckXRUN(IDsDriverBufferImpl* pdbi)
> @@ -3076,71 +3068,66 @@ static void DSDB_CheckXRUN(IDsDriverBuff
>      }
>  }
>
> -static void DSDB_MMAPCopy(IDsDriverBufferImpl* pdbi, int mul)
> +/**
> + * The helper thread for DirectSound
> + *
> + * Basically it does an infinite loop until it is told to die
> + * 
> + * snd_pcm_wait() is a call that polls the sound buffer and waits
> + * until there is at least 1 period free before it returns.
> + *
> + * We then commit the buffer filled by the owner of this
> + * IDSDriverBuffer */
> +static DWORD CALLBACK DBSB_MMAPStart(LPVOID data)
>  {
> +    IDsDriverBufferImpl* pdbi = (IDsDriverBufferImpl*)data;
>      WINE_WAVEDEV *     wwo = &(WOutDev[pdbi->drv->wDevID]);
> -    snd_pcm_uframes_t  period_size;
> -    snd_pcm_sframes_t  avail;
> -    int err;
> -    int dir=0;
> -
> +    snd_pcm_uframes_t  frames, wanted, ofs;
>      const snd_pcm_channel_area_t *areas;
> -    snd_pcm_uframes_t     ofs;
> -    snd_pcm_uframes_t     frames;
> -    snd_pcm_uframes_t     wanted;
>
> -    if ( !pdbi->mmap_buffer || !wwo->hw_params || !wwo->pcm)
> -    	return;
> -
> -    err = snd_pcm_hw_params_get_period_size(wwo->hw_params, &period_size, &dir);
> -    avail = snd_pcm_avail_update(wwo->pcm);
> +    TRACE("\n");
>
> -    DSDB_CheckXRUN(pdbi);
> -
> -    TRACE("avail=%d, mul=%d\n", (int)avail, mul);
> -
> -    frames = pdbi->mmap_buflen_frames;
> -	
> -    EnterCriticalSection(&pdbi->mmap_crst);
> +    if (areas != pdbi->mmap_areas || areas->addr != pdbi->mmap_areas->addr)
> +        FIXME("Can't access sound driver's buffer directly.\n");
>
> -    /* we want to commit the given number of periods, or the whole lot */
> -    wanted = mul == 0 ? frames : period_size * 2;
> +    while (pdbi->mmap_thread)
> +    {
> +        snd_pcm_wait(wwo->pcm, -1);
> +        wanted = frames = pdbi->mmap_buflen_frames;
> +        DSDB_CheckXRUN(pdbi);
>
> -    snd_pcm_mmap_begin(wwo->pcm, &areas, &ofs, &frames);
> -    if (areas != pdbi->mmap_areas || areas->addr != pdbi->mmap_areas->addr)
> -        FIXME("Can't access sound driver's buffer directly.\n");	
> -
> -    /* mark our current play position */
> -    pdbi->mmap_ppos = ofs;
> -        
> -    if (frames > wanted)
> -        frames = wanted;
> -        
> -    err = snd_pcm_mmap_commit(wwo->pcm, ofs, frames);
> -	
> -    /* Check to make sure we committed all we want to commit. ALSA
> -     * only gives a contiguous linear region, so we need to check this
> -     * in case we've reached the end of the buffer, in which case we
> -     * can wrap around back to the beginning. */
> -    if (frames < wanted) {
> -        frames = wanted -= frames;
> +        EnterCriticalSection(&pdbi->mmap_crst);
>          snd_pcm_mmap_begin(wwo->pcm, &areas, &ofs, &frames);
>          snd_pcm_mmap_commit(wwo->pcm, ofs, frames);
> -    }
>
> -    LeaveCriticalSection(&pdbi->mmap_crst);
> +        /* mark our current play position */
> +        pdbi->mmap_ppos = ofs;
> +
> +        /* Check to make sure we committed all we want to commit. ALSA
> +         * only gives a contiguous linear region, so we need to check this
> +         * in case we've reached the end of the buffer, in which case we
> +         * can wrap around back to the beginning. */
> +        if (frames < wanted) {
> +            frames = wanted - frames;
> +            snd_pcm_mmap_begin(wwo->pcm, &areas, &ofs, &frames);
> +            snd_pcm_mmap_commit(wwo->pcm, ofs, frames);
> +        }
> +        LeaveCriticalSection(&pdbi->mmap_crst);
> +    }
> +    TRACE("Destroyed MMAP thread\n");
> +    return 0;
>  }
>
> -static void DSDB_PCMCallback(snd_async_handler_t *ahandler)
> +/**
> + *  Cancel running MMAP thread */
> +static void DBSB_MMAPStop(IDsDriverBufferImpl* This)
>  {
> -    int periods;
> -    /* snd_pcm_t *               handle = snd_async_handler_get_pcm(ahandler); */
> -    IDsDriverBufferImpl*      pdbi = snd_async_handler_get_callback_private(ahandler);
> -    TRACE("callback called\n");
> -    
> -    /* Commit another block (the entire buffer if it's a direct hw buffer) */
> -    periods = pdbi->mmap_mode == SND_PCM_TYPE_HW ? 0 : 1;
> -    DSDB_MMAPCopy(pdbi, periods);
> +    HANDLE Thread;
> +    TRACE("Destroying MMAP thread\n");
> +    if (This->mmap_thread == NULL) return;
> +    Thread = This->mmap_thread;
> +    This->mmap_thread = NULL;
> +    WaitForSingleObject(Thread, INFINITE);
>  }
>
> /**
> @@ -3158,21 +3145,47 @@ static int DSDB_CreateMMAP(IDsDriverBuff
>      unsigned int              bits_per_sample;
>      unsigned int              bits_per_frame;
>      int                       err;
> +    int mmap_mode;
> +
> +    mmap_mode = snd_pcm_type(wwo->pcm);
> +    pdbi->mmap_thread = NULL;
> +
> +    /* If we have a software buffer, change buffer size to only get 2
> +     *
> +     * Why 2? We want a small number so that we don't get ahead of the
> +     * DirectSound mixer. But we don't want to ever let the buffer get
> +     * completely empty - having 2 periods gives us time to commit another
> +     * period when the first expires.
> +     *
> +     * The potential for buffer underrun is high, but that's the reality
> +     * of using a translated buffer (the whole point of DirectSound is
> +     * to provide direct access to the hardware).
> +     */
>
> -    err = snd_pcm_hw_params_get_format(wwo->hw_params, &format);
> -    err = snd_pcm_hw_params_get_buffer_size(wwo->hw_params, &frames);
> -    err = snd_pcm_hw_params_get_channels(wwo->hw_params, &channels);
> -    bits_per_sample = snd_pcm_format_physical_width(format);
> -    bits_per_frame = bits_per_sample * channels;
> -    pdbi->mmap_mode = snd_pcm_type(wwo->pcm);
> -    
> -    if (pdbi->mmap_mode == SND_PCM_TYPE_HW) {
> +    if (mmap_mode == SND_PCM_TYPE_HW) {
>          TRACE("mmap'd buffer is a hardware buffer.\n");
>      }
>      else {
> +        snd_pcm_uframes_t psize;
> +        err = snd_pcm_hw_params_get_period_size(wwo->hw_params, &psize, NULL);
> +        /* Set only a buffer of 2 period sizes, to decrease latency */
> +        if (err >= 0)
> +            err = snd_pcm_hw_params_set_buffer_size_near(wwo->pcm, wwo->hw_params, &psize);
> +
> +
> +        psize *= 2;
> +        if (err < 0) {
> +            ERR("Errno %d (%s) occured when setting buffer size\n", err, strerror(errno));
> +        }
>          TRACE("mmap'd buffer is an ALSA emulation of hardware buffer.\n");
>      }
>
> +    err = snd_pcm_hw_params_get_format(wwo->hw_params, &format);
> +    err = snd_pcm_hw_params_get_buffer_size(wwo->hw_params, &frames);
> +    err = snd_pcm_hw_params_get_channels(wwo->hw_params, &channels);
> +    bits_per_sample = snd_pcm_format_physical_width(format);
> +    bits_per_frame = bits_per_sample * channels;
> +
>      if (TRACE_ON(wave))
>   ALSA_TraceParameters(wwo->hw_params, NULL, FALSE);
>
> @@ -3208,19 +3221,13 @@ static int DSDB_CreateMMAP(IDsDriverBuff
>      InitializeCriticalSection(&pdbi->mmap_crst);
>      pdbi->mmap_crst.DebugInfo->Spare[0] = (DWORD_PTR)"WINEALSA_mmap_crst";
>
> -    err = snd_async_add_pcm_handler(&pdbi->mmap_async_handler, wwo->pcm, DSDB_PCMCallback, pdbi);
> -    if ( err < 0 )
> -    {
> -	ERR("add_pcm_handler failed. reason: %s\n", snd_strerror(err));
> -	return DSERR_GENERIC;
> -    }
> -
>      return DS_OK;
>  }
>
> static void DSDB_DestroyMMAP(IDsDriverBufferImpl* pdbi)
>  {
>      TRACE("mmap buffer %p destroyed\n", pdbi->mmap_buffer);
> +    DBSB_MMAPStop(pdbi);
>      pdbi->mmap_areas = NULL;
>      pdbi->mmap_buffer = NULL;
>      pdbi->mmap_crst.DebugInfo->Spare[0] = 0;
> @@ -3254,6 +3261,7 @@ static ULONG WINAPI IDsDriverBufferImpl_
>
> if (refCount)
>   return refCount;
> +    IDsDriverBuffer_Stop(iface);
>      if (This == This->drv->primary)
>   This->drv->primary = NULL;
>      DSDB_DestroyMMAP(This);
> @@ -3372,29 +3380,13 @@ static HRESULT WINAPI IDsDriverBufferImp
>   err = snd_pcm_prepare(wwo->pcm);
>          state = snd_pcm_state(wwo->pcm);
>      }
> -    if ( state == SND_PCM_STATE_PREPARED )
> +    if ( state == SND_PCM_STATE_PREPARED && This->mmap_thread == NULL)
>      {
> -	/* If we have a direct hardware buffer, we can commit the whole lot
> -	 * immediately (periods = 0), otherwise we prime the queue with only
> -	 * 2 periods.
> -	 *
> -	 * Why 2? We want a small number so that we don't get ahead of the
> -	 * DirectSound mixer. But we don't want to ever let the buffer get
> -	 * completely empty - having 2 periods gives us time to commit another
> -	 * period when the first expires.
> -	 *
> -	 * The potential for buffer underrun is high, but that's the reality
> -	 * of using a translated buffer (the whole point of DirectSound is
> -	 * to provide direct access to the hardware).
> -         * 
> -         * A better implementation would use the buffer Lock() and Unlock()
> -         * methods to determine how far ahead we can commit, and to rewind if
> -         * necessary.
> -	 */
> -	int periods = This->mmap_mode == SND_PCM_TYPE_HW ? 0 : 2;
> -	
> -	DSDB_MMAPCopy(This, periods);
> -	err = snd_pcm_start(wwo->pcm);
> +        err = snd_pcm_start(wwo->pcm);
> +        This->mmap_thread = CreateThread(NULL, 0, DBSB_MMAPStart, (LPVOID)(DWORD)This, 0, NULL);
> +        if (This->mmap_thread == NULL)
> +            return DSERR_GENERIC;
> +        SetThreadPriority(This->mmap_thread, THREAD_PRIORITY_TIME_CRITICAL);
>      }
>      return DS_OK;
>  }
> @@ -3419,6 +3411,8 @@ static HRESULT WINAPI IDsDriverBufferImp
>      	return DS_OK;
>      }
>
> +    DBSB_MMAPStop(This);
> +
>      if ( ( err = snd_pcm_drop(wwo->pcm)) < 0 )
>      {
>     	ERR("error while stopping pcm: %s\n", snd_strerror(err));
>
> ------------------------------------------------------------------------

[Winmm/winealsa] Don't use asynchronous callbacks in dsound any more

[Maarten Lankhorst]

2006/12/25, Eric Pouech <eric.pouech(a)wanadoo.fr>:
> Maarten,
>
> nice work on Alsa (which is definitively needed !!)
>
> a couple of comments:
> - I know the winmm code (and drivers) is crippled by bad synchronisation
> tricks (like cleaning a field in a structure to signal a thread). This
> is bad (TM). So I'd suggest using here a real synchronisation object.

Well, what I have now works well enough, thread is sure to be killed,
which makes life easier. I don't see the point in making code more
complicated then it needs to be.
> - I wonder if it's a good idea to create another thread for this... but
> before merging all existing drivers' threads into a single one, I'd
> suggest using the fd oriented functions & poll (see
> snd_pcm_poll_descriptors and friends) instead of snd_pcm_wait, that'll
> ease up the future work.

I'm not sure if it's a good thing either to create a new thread for
it, but in the current driver model there are no other alternatives,
you are right it is easy enough to use poll(), but basically all I
would be doing is copy the code from snd_pcm_wait(), which does it for
me. If anyone wants to use poll() it is easy enough to copy the code
1:1 from the LGPL alsa sound library. However I saw no need for it and
to keep the code simple I just used snd_pcm_wait.

See alsa-lib/src/pcm/pcm.c for how snd_pcm_wait is implemented: It
just uses poll()
> Maarten Lankhorst a écrit :
> > Instead of using asynchronous callbacks that uses signals, use a
> > seperate thread that can be cancelled, this prevents deadlock issues.
> >
> > Basically we use snd_pcm_wait() that tells us when enough room is free
> > to commit another buffer, then we commit the previous buffer and make
> > the next buffer ready.
> > .....

[Winmm/winealsa] Don't use asynchronous callbacks in dsound any more

[Eric Pouech]

Maarten Lankhorst a écrit :
> 2006/12/25, Eric Pouech <eric.pouech(a)wanadoo.fr>:
> > Maarten,
> >
> > nice work on Alsa (which is definitively needed !!)
> >
> > a couple of comments:
> > - I know the winmm code (and drivers) is crippled by bad synchronisation
> > tricks (like cleaning a field in a structure to signal a thread). This
> > is bad (TM). So I'd suggest using here a real synchronisation object.
>
> Well, what I have now works well enough, thread is sure to be killed,
> which makes life easier. I don't see the point in making code more
> complicated then it needs to be.
> the compiler may just optimize things away, and there's no guarantee 
> that NULL:ing a handle in a struct share across threads will always do 
> what you want. For the sake of record, some of the others place do mark 
> the field as volatile to make things a bit more clearer.
> > - I wonder if it's a good idea to create another thread for this... but
> > before merging all existing drivers' threads into a single one, I'd
> > suggest using the fd oriented functions & poll (see
> > snd_pcm_poll_descriptors and friends) instead of snd_pcm_wait, that'll
> > ease up the future work.
>
> I'm not sure if it's a good thing either to create a new thread for
> it, but in the current driver model there are no other alternatives,
> you are right it is easy enough to use poll(), but basically all I
> would be doing is copy the code from snd_pcm_wait(), which does it for
> me. If anyone wants to use poll() it is easy enough to copy the code
> 1:1 from the LGPL alsa sound library. However I saw no need for it and
> to keep the code simple I just used snd_pcm_wait.
>
> See alsa-lib/src/pcm/pcm.c for how snd_pcm_wait is implemented: It
> just uses poll()
> I know, that's why I'm asking you to do it ;-)
> A+