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

[Andrew Eikum]

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