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