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

[KJ Liew]

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?
>

I was testing on several real Win10 machines. QEMU built with
MSYS2/mingw-w64-x86_64 GNU toolchain.

> > 
> > 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 was trying to fix the invalid parameters errors from
dsound_lock_out()/dsound_lock_in(). I have to say that I wasn't quite sure if 
the
content of buffer matters to the caller, but an assumption that safe buffer for
read/write got to be there. So I just memcpy to keep the last good
buffer.

The lock seemed to fail for dsound_lock_out()/dsound_lock_in() when
(ppos == hw->pos_emul) for _out and (cpos == hw->pos_emul) for _in.
Hence, the last_buf was returned to the caller. Since the lock did not
take place, the *size = 0 provides the hint to skip the unlock,
otherwise, dsound_unlock_out() failed for buffer that has never been
locked.

> 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). 
 
I checked out your patch. Unfortunately, it did not
address the invalid paramters errors. The console still flooded with error
messages:
dsound: Could not lock playback buffer
dsound: Reason: An invalid parameter was passed to the returning function
dsound: Failed to lock buffer

> > > @@ -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

> diff --git a/audio/dsoundaudio.c b/audio/dsoundaudio.c
> index c265c0094b..b6bc241faa 100644
> --- a/audio/dsoundaudio.c
> +++ b/audio/dsoundaudio.c
> @@ -53,6 +53,9 @@ typedef struct {
>  typedef struct {
>      HWVoiceOut hw;
>      LPDIRECTSOUNDBUFFER dsound_buffer;
> +    void *buffer1, buffer2;
> +    DWORD size1, size2;
> +    bool has_remaining;
>      dsound *s;
>  } DSoundVoiceOut;
>  
> @@ -414,10 +417,16 @@ static void *dsound_get_buffer_out(HWVoiceOut *hw, 
> size_t *size)
>      DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
>      LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
>      HRESULT hr;
> -    DWORD ppos, act_size;
> +    DWORD ppos, act_size1, act_size2;
>      size_t req_size;
>      int err;
> -    void *ret;
> +    void *ret1, *ret2;
> +
> +    if (ds->has_remaining) {
> +        ds->has_remaining = false;
> +        *size = ds->size2;
> +        return ds->buffer2;
> +    }
>  
>      hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL);
>      if (FAILED(hr)) {
> @@ -429,15 +438,20 @@ static void *dsound_get_buffer_out(HWVoiceOut *hw, 
> size_t *size)
>      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, &ret1, 
> &ret2,
> +                          &act_size1, &act_size2, false, ds->s);
>      if (err) {
>          dolog("Failed to lock buffer\n");
>          *size = 0;
>          return NULL;
>      }
>  
> -    *size = act_size;
> +    *size = act_size1;
> +    ds->buffer1 = ret1;
> +    ds->buffer2 = ret2;
> +    ds->size1 = act_size1;
> +    ds->size2 = act_size2;
> +    ds->has_remaining = ret2 != NULL;
>      return ret;
>  }
>  
> @@ -445,7 +459,16 @@ static size_t dsound_put_buffer_out(HWVoiceOut *hw, void 
> *buf, size_t len)
>  {
>      DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
>      LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
> -    int err = dsound_unlock_out(dsb, buf, NULL, len, 0);
> +    int err;
> +
> +    if (ds->has_remaining) {
> +        ds->size1 = len;
> +        hw->pos_emul = (hw->pos_emul + len) % hw->size_emul;
> +        return len;
> +    }
> +
> +    *(ds->buffer2 ? &ds->size2 : &ds->size1) = len;
> +    err = dsound_unlock_out(dsb, ds->buffer1, ds->buffer2, ds->size1, 
> ds->size2);
>  
>      if (err) {
>          dolog("Failed to unlock buffer!!\n");