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 :
> 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 :
> > 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+