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

[Eduard Permyakov]

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?