Re: [PATCH] audio/dsound: fix invalid parameters error

[Gerd Hoffmann]

On Mon, Jan 27, 2020 at 02:46:58AM +0100, Zoltán Kővágó wrote:
> On 2020-01-18 07:30, Philippe Mathieu-Daudé wrote:
> > On 1/17/20 7:26 PM, KJ Liew wrote:
> > > QEMU Windows has broken dsound backend since the rewrite of audio API in
> > > version 4.2.0. Both playback and capture buffers failed to lock with
> > > invalid parameters error.
> > 
> > Fixes: 7fa9754ac88 (dsoundaudio: port to the new audio backend api)
> 
> Hmm, I see the old code specified those parameters, but MSDN reads:
> 
> If the application passes NULL for the ppvAudioPtr2 and pdwAudioBytes2
> parameters, the lock extends no further than the end of the buffer and does
> not wrap.
> 
> Looks like this means that if the lock doesn't fit in the buffer it fails
> instead of truncating it.  I'm sure I tested the code under wine, and
> probably in a win8.1 vm too, and it worked there, maybe it's dependent on
> the windows version or sound driver?

Ping.  Any news here?  I'm busy collecting all pending audio fixes for
the next pull request ...

> 
> > 
> > Cc'ing Zoltán who wrote 7fa9754ac88, and Gerd (the maintainer of this
> > file):
> > 
> >    $ ./scripts/get_maintainer.pl -f audio/dsoundaudio.c
> >    Gerd Hoffmann <address@hidden> (maintainer:Audio)
> > 
> > > --- ../orig/qemu-4.2.0/audio/dsoundaudio.c    2019-12-12
> > > 10:20:47.000000000 -0800
> > > +++ ../qemu-4.2.0/audio/dsoundaudio.c    2020-01-17
> > > 08:05:46.783966900 -0800
> > > @@ -53,6 +53,7 @@
> > >   typedef struct {
> > >       HWVoiceOut hw;
> > >       LPDIRECTSOUNDBUFFER dsound_buffer;
> > > +    void *last_buf;
> > >       dsound *s;
> > >   } DSoundVoiceOut;
> > > @@ -414,10 +415,10 @@
> > >       DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
> > >       LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
> > >       HRESULT hr;
> > > -    DWORD ppos, act_size;
> > > +    DWORD ppos, act_size, last_size;
> > >       size_t req_size;
> > >       int err;
> > > -    void *ret;
> > > +    void *ret, *last_ret;
> > >       hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL);
> > >       if (FAILED(hr)) {
> > > @@ -426,17 +427,24 @@
> > >           return NULL;
> > >       }
> > > +    if (ppos == hw->pos_emul) {
> > > +        *size = 0;
> > > +        return ds->last_buf;
> > > +    }
> > > +
> > >       req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul);
> > >       req_size = MIN(req_size, hw->size_emul - hw->pos_emul);
> > > -    err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size,
> > > &ret, NULL,
> > > -                          &act_size, NULL, false, ds->s);
> > > +    err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size,
> > > &ret, &last_ret,
> > > +                          &act_size, &last_size, false, ds->s);
> > >       if (err) {
> > >           dolog("Failed to lock buffer\n");
> > >           *size = 0;
> > >           return NULL;
> > >       }
> > > +    ds->last_buf = g_realloc(ds->last_buf, act_size);
> > > +    memcpy(ds->last_buf, ret, act_size);
> > >       *size = act_size;
> > >       return ret;
> > >   }
> 
> I don't really understand what's happening here, why do you need that memory
> allocation and memcpy?  This function should return a buffer where the
> caller will write data, that *size = 0; when returning ds->last_buf also
> looks incorrect to me (the calling function won't write anything into it).
> 
> I'm attaching a patch with a probably better (and totally untested) way to
> do this (if someone can tell me how to copy-paste a patch into thunderbird
> without it messing up long lines, please tell me).
> 
> 
> > > @@ -445,6 +453,8 @@
> > >   {
> > >       DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
> > >       LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
> > > +    if (len == 0)
> > > +        return 0;
> > >       int err = dsound_unlock_out(dsb, buf, NULL, len, 0);
> > >       if (err) {
> 
> Msdn says "The second pointer is needed even if nothing was written to the
> second pointer." so that NULL doesn't look okay.
> 
> > > @@ -508,10 +518,10 @@
> > >       DSoundVoiceIn *ds = (DSoundVoiceIn *) hw;
> > >       LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer;
> > >       HRESULT hr;
> > > -    DWORD cpos, act_size;
> > > +    DWORD cpos, act_size, last_size;
> > >       size_t req_size;
> > >       int err;
> > > -    void *ret;
> > > +    void *ret, *last_ret;
> > >       hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, &cpos,
> > > NULL);
> > >       if (FAILED(hr)) {
> > > @@ -520,11 +530,16 @@
> > >           return NULL;
> > >       }
> > > +    if (cpos == hw->pos_emul) {
> > > +        *size = 0;
> > > +        return NULL;
> > > +    }
> > > +
> > >       req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul);
> > >       req_size = MIN(req_size, hw->size_emul - hw->pos_emul);
> > > -    err = dsound_lock_in(dscb, &hw->info, hw->pos_emul, req_size,
> > > &ret, NULL,
> > > -                         &act_size, NULL, false, ds->s);
> > > +    err = dsound_lock_in(dscb, &hw->info, hw->pos_emul, req_size,
> > > &ret, &last_ret,
> > > +                         &act_size, &last_size, false, ds->s);
> > >       if (err) {
> > >           dolog("Failed to lock buffer\n");
> > >           *size = 0;
> > > 
> 
> You're completely ignoring last_ret and last_size here.  Don't you lose
> samples here?  I think it's possible to do something like I posted above
> with output here.
> 
> Regards,
> Zoltan