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 <kra...@redhat.com> (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");
