[PATCH v4] dsound: Commit next audio chunk between play cursor and write cursor to playing.
Andrew Eikum
aeikum at codeweavers.com
Thu Sep 9 08:57:13 CDT 2021
On Wed, Sep 08, 2021 at 04:19:40PM +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 <epermyakov at codeweavers.com>
> ---
> dlls/dsound/buffer.c | 51 +++++++++++++++++++++++++++++++++---
> dlls/dsound/dsound_convert.c | 33 ++++++++++++++++-------
> dlls/dsound/dsound_private.h | 6 ++++-
> dlls/dsound/mixer.c | 1 +
> 4 files changed, 77 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c
> index dc3b54906ce..12c8345c4a8 100644
> --- a/dlls/dsound/buffer.c
> +++ b/dlls/dsound/buffer.c
> @@ -101,6 +101,25 @@ static int __cdecl notify_compar(const void *l, const void *r)
> return 1;
> }
>
> +static void copy_circular(VOID *dstbuff, VOID *srcbuff, DWORD srcoff, DWORD srcsize, DWORD cpysize)
> +{
> + DWORD overflow;
> + if(cpysize > srcsize - srcoff) {
> + 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);
> + }
> +}
> +
> +static void commit_next_chunk(IDirectSoundBufferImpl *dsb)
> +{
> + copy_circular(dsb->committedbuff, dsb->buffer->memory, dsb->sec_mixpos, dsb->buflen, dsb->writelead);
As this is the only user of copy_circular, I'm not sure it adds much
to break it out into its own function.
> + dsb->use_committed = TRUE;
> + 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)
> {
> @@ -503,8 +522,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 +540,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);
> }
>
> @@ -1081,6 +1106,13 @@ 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;
> + }
> + FillMemory(dsb->committedbuff, dsb->writelead, dsbd->lpwfxFormat->wBitsPerSample == 8 ? 128 : 0);
Since we're only using the buffer after committing to it, is it
necessary to initialize its contents?
> +
> if (dsb->dsbd.dwFlags & DSBCAPS_CTRL3D) {
> dsb->ds3db_ds3db.dwSize = sizeof(DS3DBUFFER);
> dsb->ds3db_ds3db.vPosition.x = 0.0;
> @@ -1135,6 +1167,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 +1190,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 +1200,14 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
> return DSERR_OUTOFMEMORY;
> }
>
> + committedbuff = HeapAlloc(GetProcessHeap(),0,pdsb->writelead);
> + if (committedbuff == NULL) {
> + HeapFree(GetProcessHeap(),0,dsb);
> + *ppdsb = NULL;
> + return DSERR_OUTOFMEMORY;
> + }
> + FillMemory(committedbuff, pdsb->writelead, pdsb->pwfx->wBitsPerSample == 8 ? 128 : 0);
Here, too.
> +
> AcquireSRWLockShared(&pdsb->lock);
>
> CopyMemory(dsb, pdsb, sizeof(*dsb));
> @@ -1175,6 +1217,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 +1235,7 @@ HRESULT IDirectSoundBufferImpl_Duplicate(
> dsb->notifies = NULL;
> dsb->nrofnotifies = 0;
> dsb->device = device;
> + dsb->committedbuff = committedbuff;
> DSOUND_RecalcFormat(dsb);
>
> InitializeSRWLock(&dsb->lock);
> @@ -1202,6 +1246,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..76c36b607c7 100644
> --- a/dlls/dsound/dsound_convert.c
> +++ b/dlls/dsound/dsound_convert.c
> @@ -55,17 +55,30 @@ WINE_DEFAULT_DEBUG_CHANNEL(dsound);
> #define le32(x) (x)
> #endif
>
> +static BYTE *buff_byte(const IDirectSoundBufferImpl *dsb, DWORD offset)
> +{
> + if(!dsb->use_committed)
> + return dsb->buffer->memory + offset;
> +
> + if(offset >= dsb->sec_mixpos && offset < dsb->sec_mixpos + dsb->writelead) {
> + return (BYTE*)dsb->committedbuff + (offset - dsb->sec_mixpos);
> + }else if(offset < (dsb->sec_mixpos + dsb->writelead) % dsb->buflen) {
> + return (BYTE*)dsb->committedbuff + (dsb->buflen - dsb->sec_mixpos + offset);
> + }else{
> + return dsb->buffer->memory + offset;
Style nit-pick, I find the else-after-return to be very ugly.
> + }
> +}
> +
> static float get8(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> {
> - const BYTE* buf = dsb->buffer->memory;
> - buf += pos + channel;
> + const BYTE *buf = buff_byte(dsb, pos + channel);
These functions execute in a very tight loop. Could we factor out the
buffer selection to the call site instead of inside the loop?
> return (buf[0] - 0x80) / (float)0x80;
> }
>
> static float get16(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> {
> - const BYTE* buf = dsb->buffer->memory;
> - const SHORT *sbuf = (const SHORT*)(buf + pos + 2 * channel);
> + const BYTE *buf = buff_byte(dsb, pos + 2 * channel);
> + const SHORT *sbuf = (const SHORT*)(buf);
> SHORT sample = (SHORT)le16(*sbuf);
> return sample / (float)0x8000;
> }
> @@ -73,8 +86,8 @@ static float get16(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> static float get24(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> {
> LONG sample;
> - const BYTE* buf = dsb->buffer->memory;
> - buf += pos + 3 * channel;
> + const BYTE *buf = buff_byte(dsb, pos + 3 * channel);
> +
> /* The next expression deliberately has an overflow for buf[2] >= 0x80,
> this is how negative values are made.
> */
> @@ -84,16 +97,16 @@ static float get24(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
>
> static float get32(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD channel)
> {
> - const BYTE* buf = dsb->buffer->memory;
> - const LONG *sbuf = (const LONG*)(buf + pos + 4 * channel);
> + const BYTE *buf = buff_byte(dsb, pos + 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)
> {
> - const BYTE* buf = dsb->buffer->memory;
> - const float *sbuf = (const float*)(buf + pos + 4 * channel);
> + const BYTE *buf = buff_byte(dsb, pos + 4 * channel);
> + const float *sbuf = (const float*)(buf);
> /* The value will be clipped later, when put into some non-float buffer */
> return *sbuf;
> }
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index bdb9ebee544..48f83315180 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -153,7 +153,11 @@ 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;
> /* IDirectSoundNotify fields */
> LPDSBPOSITIONNOTIFY notifies;
> int nrofnotifies;
> diff --git a/dlls/dsound/mixer.c b/dlls/dsound/mixer.c
> index 124d1e4fba1..5cc1f70afdb 100644
> --- a/dlls/dsound/mixer.c
> +++ b/dlls/dsound/mixer.c
> @@ -389,6 +389,7 @@ static void cp_fields(IDirectSoundBufferImpl *dsb, UINT count, LONG64 *freqAccNu
> }
>
> dsb->sec_mixpos = ipos;
> + dsb->use_committed = FALSE;
> }
>
> /**
> --
> 2.25.1
>
>
Andrew
More information about the wine-devel
mailing list