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@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);}@@ -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 (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);}@@ -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;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