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));
>
> ------------------------------------------------------------------------
>
>
>
