[PATCH v5] dsound: Commit next audio chunk between play cursor and write cursor to playing.
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(a)codeweavers.com> --- dlls/dsound/buffer.c | 48 +++++++++++++++++++++++++++++++++--- dlls/dsound/dsound_convert.c | 33 ++++++++++++------------- dlls/dsound/dsound_private.h | 11 ++++++--- dlls/dsound/mixer.c | 48 ++++++++++++++++++++++++++++-------- 4 files changed, 107 insertions(+), 33 deletions(-) diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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) { @@ -503,8 +521,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 +539,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 +1105,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 +1165,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 +1188,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 +1198,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 +1214,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 +1232,7 @@ HRESULT IDirectSoundBufferImpl_Duplicate( dsb->notifies = NULL; dsb->nrofnotifies = 0; dsb->device = device; + dsb->committedbuff = committedbuff; DSOUND_RecalcFormat(dsb); InitializeSRWLock(&dsb->lock); @@ -1202,6 +1243,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
This is looking pretty good. I'm going to run it through my set of dsound test applications, and there's a little bit of feedback below. On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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;
Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be?
@@ -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;
I think use_committed (and committed_mixpos?) should be initialized in Duplicate. What about SetCurrentPosition? Andrew
On Mon, Sep 13, 2021 at 10:33:38AM -0500, Andrew Eikum wrote:
This is looking pretty good. I'm going to run it through my set of dsound test applications
Bad news, it causes a crash in the Darwinia 2 demo for me :( $ md5sum darwinia-demo2.exe 3f5d3fd40db15dd1a7e51891f385c57f darwinia-demo2.exe https://www.moddb.com/games/darwinia/downloads/darwinia-windows-demo Andrew
On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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;
Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be?
@@ -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;
I think use_committed (and committed_mixpos?) should be initialized in Duplicate. What about SetCurrentPosition?
Andrew
On 2021-09-13 6:49 p.m., Andrew Eikum wrote:
On Mon, Sep 13, 2021 at 10:33:38AM -0500, Andrew Eikum wrote:
This is looking pretty good. I'm going to run it through my set of dsound test applications
Bad news, it causes a crash in the Darwinia 2 demo for me :(
$ md5sum darwinia-demo2.exe 3f5d3fd40db15dd1a7e51891f385c57f darwinia-demo2.exe
https://www.moddb.com/games/darwinia/downloads/darwinia-windows-demo
Andrew
Fixed in the newest version of the patch. I did not consider a case where an existing buffer's frequency (and consequently writelead) could be changed.
On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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; Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be?
If the committed_mixpos is greater than 0 and less than dsb->writelead, this means that a previously committed chunk has not had time to be played completely. This essentially means that the app is overwriting a single buffer region in rapid succession. I think this is an extremely exotic case. Since there is only a fixed amount of space for the committed chunk, we would need to discard either part of the "old" committed chunk, or part of the "new" committed chunk. For simplicity of the code, I have kept it such that any part of the "old" committed chunk is discarded in such a case.
@@ -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; I think use_committed (and committed_mixpos?) should be initialized in Duplicate. What about SetCurrentPosition?
Andrew
On Tue, Sep 14, 2021 at 01:14:14PM +0300, Eduard Permyakov wrote:
On 2021-09-13 6:49 p.m., Andrew Eikum wrote:
On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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; Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be?
If the committed_mixpos is greater than 0 and less than dsb->writelead, this means that a previously committed chunk has not had time to be played completely. This essentially means that the app is overwriting a single buffer region in rapid succession. I think this is an extremely exotic case. Since there is only a fixed amount of space for the committed chunk, we would need to discard either part of the "old" committed chunk, or part of the "new" committed chunk. For simplicity of the code, I have kept it such that any part of the "old" committed chunk is discarded in such a case.
My concern was more about resetting the value to zero than the audio data which gets mixed. Won't that result in pulling too many samples from the committed buffer if the application overwrites the buffer while committed_mixpos is non-zero, because the value gets reset to zero? It's possible I missed something. Andrew
On 2021-09-14 10:24 p.m., Andrew Eikum wrote:
On Tue, Sep 14, 2021 at 01:14:14PM +0300, Eduard Permyakov wrote:
On 2021-09-13 6:49 p.m., Andrew Eikum wrote:
On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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; Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be? If the committed_mixpos is greater than 0 and less than dsb->writelead, this means that a previously committed chunk has not had time to be played completely. This essentially means that the app is overwriting a single buffer region in rapid succession. I think this is an extremely exotic case. Since there is only a fixed amount of space for the committed chunk, we would need to discard either part of the "old" committed chunk, or part of the "new" committed chunk. For simplicity of the code, I have kept it such that any part of the "old" committed chunk is discarded in such a case.
My concern was more about resetting the value to zero than the audio data which gets mixed. Won't that result in pulling too many samples from the committed buffer if the application overwrites the buffer while committed_mixpos is non-zero, because the value gets reset to zero? It's possible I missed something.
Andrew
I think there is no problem with this case. When reading from the committed buffer, there is logic that guarantees that we cannot overread it. In the case of overwriting the committed buffer when 1/2 of it has been consumed, what will happen is that after only 1/2 of the "old" committed samples have played, the "new" committed samples will start playing. I think this makes sense for the case when there is overwriting. The previous values of committed_mixpos and use_committed do not affect the behavior of what will happen after the chunk is committed. Independent of whether we were reading samples from an "old" committed buffer or from the main buffer, we will read from the beginning of the newly overwritten committed buffer next time mixing takes place. Or is there a specific sequence you had in mind which could be problematic?
On Wed, Sep 15, 2021 at 10:57:18AM +0300, Eduard Permyakov wrote:
On 2021-09-14 10:24 p.m., Andrew Eikum wrote:
On Tue, Sep 14, 2021 at 01:14:14PM +0300, Eduard Permyakov wrote:
On 2021-09-13 6:49 p.m., Andrew Eikum wrote:
On Fri, Sep 10, 2021 at 06:36:23PM +0300, Eduard Permyakov wrote:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index dc3b54906ce..63468732e1d 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -101,6 +101,24 @@ 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(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; Is it right to always reset this to zero? Is it possible an app could write to the committed chunk while committed_mixpos is non-zero, and so reset committed_mixpos when it shouldn't be? If the committed_mixpos is greater than 0 and less than dsb->writelead, this means that a previously committed chunk has not had time to be played completely. This essentially means that the app is overwriting a single buffer region in rapid succession. I think this is an extremely exotic case. Since there is only a fixed amount of space for the committed chunk, we would need to discard either part of the "old" committed chunk, or part of the "new" committed chunk. For simplicity of the code, I have kept it such that any part of the "old" committed chunk is discarded in such a case.
My concern was more about resetting the value to zero than the audio data which gets mixed. Won't that result in pulling too many samples from the committed buffer if the application overwrites the buffer while committed_mixpos is non-zero, because the value gets reset to zero? It's possible I missed something.
Andrew
I think there is no problem with this case. When reading from the committed buffer, there is logic that guarantees that we cannot overread it. In the case of overwriting the committed buffer when 1/2 of it has been consumed, what will happen is that after only 1/2 of the "old" committed samples have played, the "new" committed samples will start playing. I think this makes sense for the case when there is overwriting. The previous values of committed_mixpos and use_committed do not affect the behavior of what will happen after the chunk is committed. Independent of whether we were reading samples from an "old" committed buffer or from the main buffer, we will read from the beginning of the newly overwritten committed buffer next time mixing takes place. Or is there a specific sequence you had in mind which could be problematic?
Ok, I've re-read the code and I think I understand now. I'll give the latest version another run through my test programs. Andrew
participants (2)
-
Andrew Eikum -
Eduard Permyakov