Re: [PATCH v6] dsound: Commit next audio chunk between play cursor and write cursor to playing.

[Andrew Eikum]

Signed-off-by: Andrew Eikum

On Tue, Sep 14, 2021 at 01:05:18PM +0300, Eduard Permyakov wrote:
> This region of the audio buffer is forbidden to be written to by the
> DirectSound specification. The documentation states: "The write cursor
> is the point after which it is safe to write data into the buffer. The
> block between the play cursor and the write cursor is already committed
> to be played, and cannot be changed safely." However, some applications
> still do this, which has lead to audio glitches only when using the Wine
> DirectSound implementation. Experiments showed that the native DirctSound
> implementation will still play the old audio the first time around when the
> buffer region gets overwritten. Use an approach of copying the next forbidden
> region into a "committed buffer" to add the same behavior to the Wine
> implementation.
>
> Out of performance considerations, only copy data to the committed buffer
> when we detect that an overwrite is possible (i.e. the current mixing
> region of the buffer gets locked).
> Signed-off-by: Eduard Permyakov
>
> dlls/dsound/buffer.c         | 66 ++++++++++++++++++++++++++++++++++--
>  dlls/dsound/dsound_convert.c | 33 +++++++++---------
>  dlls/dsound/dsound_private.h | 11 ++++--
>  dlls/dsound/mixer.c          | 48 ++++++++++++++++++++------
>  4 files changed, 125 insertions(+), 33 deletions(-)
>
> diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c
> index dc3b54906ce..c595b19dd63 100644
> --- a/dlls/dsound/buffer.c
> +++ b/dlls/dsound/buffer.c
> @@ -101,6 +101,27 @@ static int __cdecl notify_compar(const void *l, const void *r)
>      return 1;
>  }
>
> +static void commit_next_chunk(IDirectSoundBufferImpl *dsb)
> +{
> - void *dstbuff = dsb->committedbuff, *srcbuff = dsb->buffer->memory;
> - DWORD srcoff = dsb->sec_mixpos, srcsize = dsb->buflen, cpysize = dsb->writelead;
> -
> - if(dsb->state != STATE_PLAYING)
> -
>    return;
> -
> - if(cpysize > srcsize - srcoff) {
> -
>    DWORD overflow = cpysize - (srcsize - srcoff);
> -
>    memcpy(dstbuff, (BYTE*)srcbuff + srcoff, srcsize - srcoff);
> -
>    memcpy((BYTE*)dstbuff + (srcsize - srcoff), srcbuff, overflow);
> - }else{
> -
>    memcpy(dstbuff, (BYTE*)srcbuff + srcoff, cpysize);
> - }
> -
> - dsb->use_committed = TRUE;
> - dsb->committed_mixpos = 0;
> - TRACE("committing %u bytes from offset %u\n", dsb->writelead, dsb->sec_mixpos);
>
> +}
> -
>
> static HRESULT WINAPI IDirectSoundNotifyImpl_SetNotificationPositions(IDirectSoundNotify *iface,
>          DWORD howmuch, const DSBPOSITIONNOTIFY *notify)
>  {
> @@ -244,6 +265,7 @@ static HRESULT WINAPI IDirectSoundBufferImpl_SetFrequency(IDirectSoundBuffer8 *i
>  {
>          IDirectSoundBufferImpl *This = impl_from_IDirectSoundBuffer8(iface);
>    DWORD oldFreq;
> -
>
> void *newcommitted;
>
> TRACE("(%p,%d)\n",This,freq);
>
> @@ -274,6 +296,13 @@ static HRESULT WINAPI IDirectSoundBufferImpl_SetFrequency(IDirectSoundBuffer8 *i
>    	This->freqAdjustDen = This->device->pwfx->nSamplesPerSec;
>    	This->nAvgBytesPerSec = freq * This->pwfx->nBlockAlign;
>    	DSOUND_RecalcFormat(This);
> -
> -
> newcommitted = HeapReAlloc(GetProcessHeap(), 0, This->committedbuff, This->writelead);
> -
> if(!newcommitted) {
> -
> 	ReleaseSRWLockExclusive(&This->lock);
> -
> 	return DSERR_OUTOFMEMORY;
> -
> }
> -
> This->committedbuff = newcommitted;
>
> }
>
> ReleaseSRWLockExclusive(&This->lock);
>
> @@ -319,6 +348,8 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Stop(IDirectSoundBuffer8 *iface)
>    if (This->state == STATE_PLAYING || This->state == STATE_STARTING)
>    {
>    	This->state = STATE_STOPPED;
> -
> This->use_committed = FALSE;
> -
> This->committed_mixpos = 0;
> DSOUND_CheckEvent(This, 0, 0);
> }
>
> @@ -503,8 +534,10 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock(IDirectSoundBuffer8 *iface, DW
>
> if (writecursor+writebytes <= This->buflen) {
>    	*(LPBYTE*)lplpaudioptr1 = This->buffer->memory+writecursor;
> -
> if (This->sec_mixpos >= writecursor && This->sec_mixpos < writecursor + writebytes && This->state == STATE_PLAYING)
> -
> if (This->sec_mixpos >= writecursor && This->sec_mixpos < writecursor + writebytes && This->state == STATE_PLAYING) {
> WARN("Overwriting mixing position, case 1\n");
> -
> 	commit_next_chunk(This);
> -
> }
> *audiobytes1 = writebytes;
> if (lplpaudioptr2)
> 	*(LPBYTE*)lplpaudioptr2 = NULL;
>
> @@ -519,16 +552,20 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock(IDirectSoundBuffer8 *iface, DW
>    	*(LPBYTE*)lplpaudioptr1 = This->buffer->memory+writecursor;
>    	*audiobytes1 = This->buflen-writecursor;
>    	This->buffer->lockedbytes += *audiobytes1;
> -
> if (This->sec_mixpos >= writecursor && This->sec_mixpos < writecursor + writebytes && This->state == STATE_PLAYING)
> -
> if (This->sec_mixpos >= writecursor && This->sec_mixpos < writecursor + writebytes && This->state == STATE_PLAYING) {
> WARN("Overwriting mixing position, case 2\n");
> -
> 	commit_next_chunk(This);
> -
> }
> if (lplpaudioptr2)
> 	*(LPBYTE*)lplpaudioptr2 = This->buffer->memory;
> if (audiobytes2) {
> 	*audiobytes2 = writebytes-(This->buflen-writecursor);
> 	This->buffer->lockedbytes += *audiobytes2;
> }
> -
> if (audiobytes2 && This->sec_mixpos < remainder && This->state == STATE_PLAYING)
> -
> if (audiobytes2 && This->sec_mixpos < remainder && This->state == STATE_PLAYING) {
> WARN("Overwriting mixing position, case 3\n");
> -
> 	commit_next_chunk(This);
> -
> }
> TRACE("Locked %p(%i bytes) and %p(%i bytes) writecursor=%d\n", *(LPBYTE*)lplpaudioptr1, *audiobytes1, lplpaudioptr2 ? *(LPBYTE*)lplpaudioptr2 : NULL, audiobytes2 ? *audiobytes2: 0, writecursor);
> }
>
> @@ -552,6 +589,9 @@ static HRESULT WINAPI IDirectSoundBufferImpl_SetCurrentPosition(IDirectSoundBuff
>    newpos -= newpos%This->pwfx->nBlockAlign;
>    This->sec_mixpos = newpos;
> - This->use_committed = FALSE;
> - This->committed_mixpos = 0;
> - /* at this point, do not attempt to reset buffers, mess with primary mix position,
>           or anything like that to reduce latency. The data already prebuffered cannot be changed */
>
> @@ -1081,6 +1121,12 @@ HRESULT secondarybuffer_create(DirectSoundDevice *device, const DSBUFFERDESC *ds
>    /* calculate fragment size and write lead */
>    DSOUND_RecalcFormat(dsb);
> - dsb->committedbuff = HeapAlloc(GetProcessHeap(), 0, dsb->writelead);
> - if(!dsb->committedbuff) {
> -
> IDirectSoundBuffer8_Release(&dsb->IDirectSoundBuffer8_iface);
> -
> return DSERR_OUTOFMEMORY;
> - }
> - if (dsb->dsbd.dwFlags & DSBCAPS_CTRL3D) {
>   dsb->ds3db_ds3db.dwSize = sizeof(DS3DBUFFER);
>   dsb->ds3db_ds3db.vPosition.x = 0.0;
>
> @@ -1135,6 +1181,7 @@ void secondarybuffer_destroy(IDirectSoundBufferImpl *This)
>  HeapFree(GetProcessHeap(), 0, This->notifies);
>  HeapFree(GetProcessHeap(), 0, This->pwfx);
> -
>
> HeapFree(GetProcessHeap(), 0, This->committedbuff);
>
> if (This->filters) {
>     int i;
>
> @@ -1157,6 +1204,7 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
>  {
>      IDirectSoundBufferImpl *dsb;
>      HRESULT hres = DS_OK;
> -
>
> VOID *committedbuff;
> TRACE("(%p,%p,%p)\n", device, ppdsb, pdsb);
>
> dsb = HeapAlloc(GetProcessHeap(),0,sizeof(*dsb));
>
> @@ -1166,6 +1214,13 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
>          return DSERR_OUTOFMEMORY;
>      }
> -
>
> committedbuff = HeapAlloc(GetProcessHeap(),0,pdsb->writelead);
> -
>
> if (committedbuff == NULL) {
> -
>    HeapFree(GetProcessHeap(),0,dsb);
> -
>    *ppdsb = NULL;
> -
>    return DSERR_OUTOFMEMORY;
> -
>
> }
> -
>
> AcquireSRWLockShared(&pdsb->lock);
>
> CopyMemory(dsb, pdsb, sizeof(*dsb));
>
> @@ -1175,6 +1230,7 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
>      ReleaseSRWLockShared(&pdsb->lock);
>  if (dsb->pwfx == NULL) {
> -
>    HeapFree(GetProcessHeap(),0,committedbuff);
>    HeapFree(GetProcessHeap(),0,dsb);
>    *ppdsb = NULL;
>    return DSERR_OUTOFMEMORY;
>
> @@ -1192,6 +1248,9 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
>      dsb->notifies = NULL;
>      dsb->nrofnotifies = 0;
>      dsb->device = device;
> -
>
> dsb->committedbuff = committedbuff;
> -
>
> dsb->use_committed = FALSE;
> -
>
> dsb->committed_mixpos = 0;
> DSOUND_RecalcFormat(dsb);
>
> InitializeSRWLock(&dsb->lock);
>
> @@ -1202,6 +1261,7 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
>          list_remove(&dsb->entry);
>          dsb->buffer->ref--;
>          HeapFree(GetProcessHeap(),0,dsb->pwfx);
> -
>    HeapFree(GetProcessHeap(),0,dsb->committedbuff);
>    HeapFree(GetProcessHeap(),0,dsb);
>    dsb = NULL;
> }else
>
> diff --git a/dlls/dsound/dsound_convert.c b/dlls/dsound/dsound_convert.c
> index 4735178a9a8..67344565860 100644
> --- a/dlls/dsound/dsound_convert.c
> +++ b/dlls/dsound/dsound_convert.c
> @@ -55,26 +55,25 @@ WINE_DEFAULT_DEBUG_CHANNEL(dsound);
>  #define le32(x) (x)
>  #endif
>
> -static float get8(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +static float get8(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
> - const BYTE* buf = dsb->buffer->memory;
> - buf += pos + channel;
> - const BYTE *buf = base + channel;
> return (buf[0] - 0x80) / (float)0x80;
>
> }
>
> -static float get16(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +static float get16(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
> - const BYTE* buf = dsb->buffer->memory;
> - const SHORT *sbuf = (const SHORT*)(buf + pos + 2 * channel);
> - const BYTE *buf = base + 2 * channel;
> - const SHORT *sbuf = (const SHORT*)(buf);
> SHORT sample = (SHORT)le16(*sbuf);
> return sample / (float)0x8000;
>
> }
>
> -static float get24(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +static float get24(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
>      LONG sample;
> - const BYTE* buf = dsb->buffer->memory;
> - buf += pos + 3 * channel;
> - const BYTE *buf = base + 3 * channel;
> - /* The next expression deliberately has an overflow for buf[2] >= 0x80,
>       this is how negative values are made.
>     */
>
> @@ -82,32 +81,32 @@ static float get24(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
>      return sample / (float)0x80000000U;
>  }
>
> -static float get32(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +static float get32(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
> - const BYTE* buf = dsb->buffer->memory;
> - const LONG *sbuf = (const LONG*)(buf + pos + 4 * channel);
> - const BYTE *buf = base + 4 * channel;
> - const LONG *sbuf = (const LONG*)(buf);
> LONG sample = le32(*sbuf);
> return sample / (float)0x80000000U;
>
> }
>
> -static float getieee32(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +static float getieee32(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
> - const BYTE* buf = dsb->buffer->memory;
> - const float *sbuf = (const float*)(buf + pos + 4 * channel);
> - const BYTE *buf = base + 4 * channel;
> - const float *sbuf = (const float*)(buf);
> /* The value will be clipped later, when put into some non-float buffer */
> return *sbuf;
>
> }
>
> const bitsgetfunc getbpp[5] = {get8, get16, get24, get32, getieee32};
>
> -float get_mono(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> +float get_mono(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel)
>  {
>      DWORD channels = dsb->pwfx->nChannels;
>      DWORD c;
>      float val = 0;
>      /* XXX: does Windows include LFE into the mix? */
>      for (c = 0; c < channels; c++)
> -
>    val += dsb->get_aux(dsb, pos, c);
> -
>    val += dsb->get_aux(dsb, base, c);
> val /= channels;
>    return val;
>
> }
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index bdb9ebee544..9ec96504a50 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -43,7 +43,7 @@ typedef struct IDirectSoundBufferImpl        IDirectSoundBufferImpl;
>  typedef struct DirectSoundDevice             DirectSoundDevice;
>
> /* dsound_convert.h */
> -typedef float (*bitsgetfunc)(const IDirectSoundBufferImpl *, DWORD, DWORD);
> +typedef float (*bitsgetfunc)(const IDirectSoundBufferImpl *, BYTE *, DWORD);
>  typedef void (*bitsputfunc)(const IDirectSoundBufferImpl *, DWORD, DWORD, float);
>  extern const bitsgetfunc getbpp[5] DECLSPEC_HIDDEN;
>  void putieee32(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel, float value) DECLSPEC_HIDDEN;
> @@ -153,7 +153,12 @@ struct IDirectSoundBufferImpl
>      LONG64                      freqAccNum;
>      /* used for mixing */
>      DWORD                       sec_mixpos;
> -
> - /* Holds a copy of the next 'writelead' bytes, to be used for mixing. This makes it
> -
> * so that these bytes get played once even if this region of the buffer gets overwritten,
> -
> * which is more in-line with native DirectSound behavior. */
> - BOOL                        use_committed;
> - LPVOID                      committedbuff;
> - DWORD                       committed_mixpos;
> /* IDirectSoundNotify fields */
> LPDSBPOSITIONNOTIFY         notifies;
> int                         nrofnotifies;
>
> @@ -171,7 +176,7 @@ struct IDirectSoundBufferImpl
>      struct list entry;
>  };
>
> -float get_mono(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel) DECLSPEC_HIDDEN;
> +float get_mono(const IDirectSoundBufferImpl *dsb, BYTE *base, DWORD channel) DECLSPEC_HIDDEN;
>  void put_mono2stereo(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel, float value) DECLSPEC_HIDDEN;
>  void put_mono2quad(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel, float value) DECLSPEC_HIDDEN;
>  void put_stereo2quad(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel, float value) DECLSPEC_HIDDEN;
> diff --git a/dlls/dsound/mixer.c b/dlls/dsound/mixer.c
> index 124d1e4fba1..e9d8a6e3cdd 100644
> --- a/dlls/dsound/mixer.c
> +++ b/dlls/dsound/mixer.c
> @@ -276,22 +276,35 @@ void DSOUND_CheckEvent(const IDirectSoundBufferImpl *dsb, DWORD playpos, int len
>  }
>
> static inline float get_current_sample(const IDirectSoundBufferImpl *dsb,
> -
>    DWORD mixpos, DWORD channel)
> -
>    BYTE *buffer, DWORD buflen, DWORD mixpos, DWORD channel)
>
> {
> - if (mixpos >= dsb->buflen && !(dsb->playflags & DSBPLAY_LOOPING))
> - if (mixpos >= buflen && !(dsb->playflags & DSBPLAY_LOOPING))
>     return 0.0f;
> - return dsb->get(dsb, mixpos % dsb->buflen, channel);
> - return dsb->get(dsb, buffer + (mixpos % buflen), channel);
>
> }
>
> static UINT cp_fields_noresample(IDirectSoundBufferImpl *dsb, UINT count)
>  {
>      UINT istride = dsb->pwfx->nBlockAlign;
>      UINT ostride = dsb->device->pwfx->nChannels * sizeof(float);
> - UINT committed_samples = 0;
> DWORD channel, i;
> - for (i = 0; i < count; i++)
> -
> - if(dsb->use_committed) {
> -
>    committed_samples = (dsb->writelead - dsb->committed_mixpos) / istride;
> -
>    committed_samples = committed_samples <= count ? committed_samples : count;
> - }
> -
> - for (i = 0; i < committed_samples; i++)
> -
>    for (channel = 0; channel < dsb->mix_channels; channel++)
> -
>        dsb->put(dsb, i * ostride, channel, get_current_sample(dsb, dsb->committedbuff,
> -
>            dsb->writelead, dsb->committed_mixpos + i * istride, channel));
> -
> - for (; i < count; i++)
>     for (channel = 0; channel < dsb->mix_channels; channel++)
> -
>        dsb->put(dsb, i * ostride, channel, get_current_sample(dsb,
> -
>                dsb->sec_mixpos + i * istride, channel));
> -
>        dsb->put(dsb, i * ostride, channel, get_current_sample(dsb, dsb->buffer->memory,
> -
>            dsb->buflen, dsb->sec_mixpos + i * istride, channel));
> - return count;
>
> }
>
> @@ -300,6 +313,7 @@ static UINT cp_fields_resample(IDirectSoundBufferImpl *dsb, UINT count, LONG64 *
>      UINT i, channel;
>      UINT istride = dsb->pwfx->nBlockAlign;
>      UINT ostride = dsb->device->pwfx->nChannels * sizeof(float);
> -
>
> UINT committed_samples = 0;
>
> LONG64 freqAcc_start = *freqAccNum;
> LONG64 freqAcc_end = freqAcc_start + count * dsb->freqAdjustNum;
>
> @@ -326,16 +340,24 @@ static UINT cp_fields_resample(IDirectSoundBufferImpl *dsb, UINT count, LONG64 *
>      fir_copy = dsb->device->cp_buffer;
>      intermediate = fir_copy + fir_cachesize;
> -
>
> if(dsb->use_committed) {
> -
>    committed_samples = (dsb->writelead - dsb->committed_mixpos) / istride;
> -
>    committed_samples = committed_samples <= required_input ? committed_samples : required_input;
> -
>
> }
>
> /* Important: this buffer MUST be non-interleaved
> - if you want -msse3 to have any effect.
> - This is good for CPU cache effects, too.
>
> */
> itmp = intermediate;
> - for (channel = 0; channel < channels; channel++)
> -
>    for (i = 0; i < required_input; i++)
> -
>        *(itmp++) = get_current_sample(dsb,
> -
>                dsb->sec_mixpos + i * istride, channel);
> -
>
> for (channel = 0; channel < channels; channel++) {
> -
>    for (i = 0; i < committed_samples; i++)
> -
>        *(itmp++) = get_current_sample(dsb, dsb->committedbuff,
> -
>            dsb->writelead, dsb->committed_mixpos + i * istride, channel);
> -
>    for (; i < required_input; i++)
> -
>        *(itmp++) = get_current_sample(dsb, dsb->buffer->memory,
> -
>                dsb->buflen, dsb->sec_mixpos + i * istride, channel);
> -
>
> }
>
> for(i = 0; i < count; ++i) {
>     UINT int_fir_steps = (freqAcc_start + i * dsb->freqAdjustNum) * dsbfirstep / dsb->freqAdjustDen;
>
> @@ -389,6 +411,12 @@ static void cp_fields(IDirectSoundBufferImpl *dsb, UINT count, LONG64 *freqAccNu
>      }
>  dsb->sec_mixpos = ipos;
> -
> - if(dsb->use_committed) {
> -
>    dsb->committed_mixpos += adv * dsb->pwfx->nBlockAlign;
> -
>    if(dsb->committed_mixpos >= dsb->writelead)
> -
>        dsb->use_committed = FALSE;
> - }
>
> }
> /**
>
> 2.25.1