* [PATCH] audio/dsound: fix invalid parameters error [not found] <20200117182621.GB13724.ref@chuwi-lt0> @ 2020-01-17 18:26 ` KJ Liew 2020-01-18 6:30 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 10+ messages in thread From: KJ Liew @ 2020-01-17 18:26 UTC (permalink / raw) To: qemu-devel 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. --- ../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; } @@ -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) { @@ -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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-01-17 18:26 ` [PATCH] audio/dsound: fix invalid parameters error KJ Liew @ 2020-01-18 6:30 ` Philippe Mathieu-Daudé 2020-01-27 1:46 ` Zoltán Kővágó 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-18 6:30 UTC (permalink / raw) To: KJ Liew, qemu-devel, Kővágó, Zoltán, Gerd Hoffmann 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) Cc'ing Zoltán who wrote 7fa9754ac88, and Gerd (the maintainer of this file): $ ./scripts/get_maintainer.pl -f audio/dsoundaudio.c Gerd Hoffmann <kraxel@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; > } > @@ -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) { > @@ -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; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-01-18 6:30 ` Philippe Mathieu-Daudé @ 2020-01-27 1:46 ` Zoltán Kővágó 2020-01-31 7:55 ` Gerd Hoffmann 2020-02-01 18:28 ` KJ Liew 0 siblings, 2 replies; 10+ messages in thread From: Zoltán Kővágó @ 2020-01-27 1:46 UTC (permalink / raw) To: Philippe Mathieu-Daudé, KJ Liew, QEMU Developers, Gerd Hoffmann [-- Attachment #1: Type: text/plain, Size: 4917 bytes --] 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? > > Cc'ing Zoltán who wrote 7fa9754ac88, and Gerd (the maintainer of this > file): > > $ ./scripts/get_maintainer.pl -f audio/dsoundaudio.c > Gerd Hoffmann <kraxel@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 [-- Attachment #2: dsound.patch --] [-- Type: text/x-patch, Size: 2389 bytes --] 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"); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-01-27 1:46 ` Zoltán Kővágó @ 2020-01-31 7:55 ` Gerd Hoffmann 2020-02-01 18:28 ` KJ Liew 1 sibling, 0 replies; 10+ messages in thread From: Gerd Hoffmann @ 2020-01-31 7:55 UTC (permalink / raw) To: Zoltán Kővágó Cc: Philippe Mathieu-Daudé, QEMU Developers, 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? 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 <kraxel@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-01-27 1:46 ` Zoltán Kővágó 2020-01-31 7:55 ` Gerd Hoffmann @ 2020-02-01 18:28 ` KJ Liew 1 sibling, 0 replies; 10+ messages in thread From: KJ Liew @ 2020-02-01 18:28 UTC (permalink / raw) To: Zoltán Kővágó Cc: Philippe Mathieu-Daudé, QEMU Developers, 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? > 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 <kraxel@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"); ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] audio/dsound: fix invalid parameters error @ 2020-02-02 23:02 Kővágó, Zoltán 2020-02-03 7:56 ` Howard Spoelstra 2020-02-06 13:38 ` Gerd Hoffmann 0 siblings, 2 replies; 10+ messages in thread From: Kővágó, Zoltán @ 2020-02-02 23:02 UTC (permalink / raw) To: qemu-devel; +Cc: Gerd Hoffmann, KJ Liew Windows (unlike wine) bails out when IDirectSoundBuffer8::Lock is called with zero length. Also, hw->pos_emul handling was incorrect when calling this function for the first time. Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> Reported-by: KJ Liew <liewkj@yahoo.com> --- I've tested this patch on wine and a borrowed Windows 8.1 laptop, I could only test audio playback, not recording. I've cross-compiled qemu using the docker image, for 64-bit. --- audio/dsound_template.h | 1 + audio/audio.c | 6 ++---- audio/dsoundaudio.c | 27 +++++++++++++++++++++++---- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/audio/dsound_template.h b/audio/dsound_template.h index 7a15f91ce5..9c5ce625ab 100644 --- a/audio/dsound_template.h +++ b/audio/dsound_template.h @@ -244,6 +244,7 @@ static int dsound_init_out(HWVoiceOut *hw, struct audsettings *as, goto fail0; } + ds->first_time = true; obt_as.endianness = 0; audio_pcm_init_info (&hw->info, &obt_as); diff --git a/audio/audio.c b/audio/audio.c index f63f39769a..cb1efc6dc5 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -1076,10 +1076,8 @@ static size_t audio_pcm_hw_run_out(HWVoiceOut *hw, size_t live) while (live) { size_t size, decr, proc; void *buf = hw->pcm_ops->get_buffer_out(hw, &size); - if (!buf) { - /* retrying will likely won't help, drop everything. */ - hw->mix_buf->pos = (hw->mix_buf->pos + live) % hw->mix_buf->size; - return clipped + live; + if (!buf || size == 0) { + break; } decr = MIN(size / hw->info.bytes_per_frame, live); diff --git a/audio/dsoundaudio.c b/audio/dsoundaudio.c index c265c0094b..bd57082a8d 100644 --- a/audio/dsoundaudio.c +++ b/audio/dsoundaudio.c @@ -53,12 +53,14 @@ typedef struct { typedef struct { HWVoiceOut hw; LPDIRECTSOUNDBUFFER dsound_buffer; + bool first_time; dsound *s; } DSoundVoiceOut; typedef struct { HWVoiceIn hw; LPDIRECTSOUNDCAPTUREBUFFER dsound_capture_buffer; + bool first_time; dsound *s; } DSoundVoiceIn; @@ -414,21 +416,32 @@ 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, wpos, act_size; size_t req_size; int err; void *ret; - hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL); + hr = IDirectSoundBuffer_GetCurrentPosition( + dsb, &ppos, ds->first_time ? &wpos : NULL); if (FAILED(hr)) { dsound_logerr(hr, "Could not get playback buffer position\n"); *size = 0; return NULL; } + if (ds->first_time) { + hw->pos_emul = wpos; + ds->first_time = false; + } + req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul); req_size = MIN(req_size, hw->size_emul - hw->pos_emul); + if (req_size == 0) { + *size = 0; + return NULL; + } + err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size, &ret, NULL, &act_size, NULL, false, ds->s); if (err) { @@ -508,18 +521,24 @@ static void *dsound_get_buffer_in(HWVoiceIn *hw, size_t *size) DSoundVoiceIn *ds = (DSoundVoiceIn *) hw; LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer; HRESULT hr; - DWORD cpos, act_size; + DWORD cpos, rpos, act_size; size_t req_size; int err; void *ret; - hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, &cpos, NULL); + hr = IDirectSoundCaptureBuffer_GetCurrentPosition( + dscb, &cpos, ds->first_time ? &rpos : NULL); if (FAILED(hr)) { dsound_logerr(hr, "Could not get capture buffer position\n"); *size = 0; return NULL; } + if (ds->first_time) { + hw->pos_emul = rpos; + ds->first_time = false; + } + req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul); req_size = MIN(req_size, hw->size_emul - hw->pos_emul); -- 2.25.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-02-02 23:02 Kővágó, Zoltán @ 2020-02-03 7:56 ` Howard Spoelstra 2020-02-03 9:18 ` Philippe Mathieu-Daudé 2020-02-06 13:38 ` Gerd Hoffmann 1 sibling, 1 reply; 10+ messages in thread From: Howard Spoelstra @ 2020-02-03 7:56 UTC (permalink / raw) To: Kővágó, Zoltán Cc: KJ Liew, qemu-devel qemu-devel, Gerd Hoffmann [-- Attachment #1: Type: text/plain, Size: 4900 bytes --] On Mon, Feb 3, 2020 at 12:02 AM Kővágó, Zoltán <dirty.ice.hu@gmail.com> wrote: > Windows (unlike wine) bails out when IDirectSoundBuffer8::Lock is called > with zero length. Also, hw->pos_emul handling was incorrect when > calling this function for the first time. > > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> > Reported-by: KJ Liew <liewkj@yahoo.com> > --- > > I've tested this patch on wine and a borrowed Windows 8.1 laptop, I > could only test audio playback, not recording. I've cross-compiled qemu > using the docker image, for 64-bit. > > --- > audio/dsound_template.h | 1 + > audio/audio.c | 6 ++---- > audio/dsoundaudio.c | 27 +++++++++++++++++++++++---- > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/audio/dsound_template.h b/audio/dsound_template.h > index 7a15f91ce5..9c5ce625ab 100644 > --- a/audio/dsound_template.h > +++ b/audio/dsound_template.h > @@ -244,6 +244,7 @@ static int dsound_init_out(HWVoiceOut *hw, struct > audsettings *as, > goto fail0; > } > > + ds->first_time = true; > obt_as.endianness = 0; > audio_pcm_init_info (&hw->info, &obt_as); > > diff --git a/audio/audio.c b/audio/audio.c > index f63f39769a..cb1efc6dc5 100644 > --- a/audio/audio.c > +++ b/audio/audio.c > @@ -1076,10 +1076,8 @@ static size_t audio_pcm_hw_run_out(HWVoiceOut *hw, > size_t live) > while (live) { > size_t size, decr, proc; > void *buf = hw->pcm_ops->get_buffer_out(hw, &size); > - if (!buf) { > - /* retrying will likely won't help, drop everything. */ > - hw->mix_buf->pos = (hw->mix_buf->pos + live) % > hw->mix_buf->size; > - return clipped + live; > + if (!buf || size == 0) { > + break; > } > > decr = MIN(size / hw->info.bytes_per_frame, live); > diff --git a/audio/dsoundaudio.c b/audio/dsoundaudio.c > index c265c0094b..bd57082a8d 100644 > --- a/audio/dsoundaudio.c > +++ b/audio/dsoundaudio.c > @@ -53,12 +53,14 @@ typedef struct { > typedef struct { > HWVoiceOut hw; > LPDIRECTSOUNDBUFFER dsound_buffer; > + bool first_time; > dsound *s; > } DSoundVoiceOut; > > typedef struct { > HWVoiceIn hw; > LPDIRECTSOUNDCAPTUREBUFFER dsound_capture_buffer; > + bool first_time; > dsound *s; > } DSoundVoiceIn; > > @@ -414,21 +416,32 @@ 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, wpos, act_size; > size_t req_size; > int err; > void *ret; > > - hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL); > + hr = IDirectSoundBuffer_GetCurrentPosition( > + dsb, &ppos, ds->first_time ? &wpos : NULL); > if (FAILED(hr)) { > dsound_logerr(hr, "Could not get playback buffer position\n"); > *size = 0; > return NULL; > } > > + if (ds->first_time) { > + hw->pos_emul = wpos; > + ds->first_time = false; > + } > + > req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul); > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > + if (req_size == 0) { > + *size = 0; > + return NULL; > + } > + > err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size, &ret, > NULL, > &act_size, NULL, false, ds->s); > if (err) { > @@ -508,18 +521,24 @@ static void *dsound_get_buffer_in(HWVoiceIn *hw, > size_t *size) > DSoundVoiceIn *ds = (DSoundVoiceIn *) hw; > LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer; > HRESULT hr; > - DWORD cpos, act_size; > + DWORD cpos, rpos, act_size; > size_t req_size; > int err; > void *ret; > > - hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, &cpos, NULL); > + hr = IDirectSoundCaptureBuffer_GetCurrentPosition( > + dscb, &cpos, ds->first_time ? &rpos : NULL); > if (FAILED(hr)) { > dsound_logerr(hr, "Could not get capture buffer position\n"); > *size = 0; > return NULL; > } > > + if (ds->first_time) { > + hw->pos_emul = rpos; > + ds->first_time = false; > + } > + > req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul); > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > -- > 2.25.0 > Hi, I tested this patch running qemu-system-ppc with MacOS 9.2 and OSX 10.5. Qemu was cross-compiled on Fedora 31 from https://github.com/mcayland/qemu/tree/screamer. Host was Windows 10. The dsound locking errors are gone, so for this test case all seems OK. Best, Howard [-- Attachment #2: Type: text/html, Size: 6212 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-02-03 7:56 ` Howard Spoelstra @ 2020-02-03 9:18 ` Philippe Mathieu-Daudé 2020-02-03 9:25 ` Howard Spoelstra 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-03 9:18 UTC (permalink / raw) To: Howard Spoelstra, Kővágó, Zoltán Cc: Gerd Hoffmann, qemu-devel qemu-devel, KJ Liew Hi Howard, On 2/3/20 8:56 AM, Howard Spoelstra wrote: > On Mon, Feb 3, 2020 at 12:02 AM Kővágó, Zoltán <dirty.ice.hu@gmail.com > <mailto:dirty.ice.hu@gmail.com>> wrote: > > Windows (unlike wine) bails out when IDirectSoundBuffer8::Lock is called > with zero length. Also, hw->pos_emul handling was incorrect when > calling this function for the first time. > > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com > <mailto:DirtY.iCE.hu@gmail.com>> > Reported-by: KJ Liew <liewkj@yahoo.com <mailto:liewkj@yahoo.com>> > --- > > I've tested this patch on wine and a borrowed Windows 8.1 laptop, I > could only test audio playback, not recording. I've cross-compiled qemu > using the docker image, for 64-bit. > > --- > audio/dsound_template.h | 1 + > audio/audio.c | 6 ++---- > audio/dsoundaudio.c | 27 +++++++++++++++++++++++---- > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/audio/dsound_template.h b/audio/dsound_template.h > index 7a15f91ce5..9c5ce625ab 100644 > --- a/audio/dsound_template.h > +++ b/audio/dsound_template.h > @@ -244,6 +244,7 @@ static int dsound_init_out(HWVoiceOut *hw, > struct audsettings *as, > goto fail0; > } > > + ds->first_time = true; > obt_as.endianness = 0; > audio_pcm_init_info (&hw->info, &obt_as); > > diff --git a/audio/audio.c b/audio/audio.c > index f63f39769a..cb1efc6dc5 100644 > --- a/audio/audio.c > +++ b/audio/audio.c > @@ -1076,10 +1076,8 @@ static size_t audio_pcm_hw_run_out(HWVoiceOut > *hw, size_t live) > while (live) { > size_t size, decr, proc; > void *buf = hw->pcm_ops->get_buffer_out(hw, &size); > - if (!buf) { > - /* retrying will likely won't help, drop everything. */ > - hw->mix_buf->pos = (hw->mix_buf->pos + live) % > hw->mix_buf->size; > - return clipped + live; > + if (!buf || size == 0) { > + break; > } > > decr = MIN(size / hw->info.bytes_per_frame, live); > diff --git a/audio/dsoundaudio.c b/audio/dsoundaudio.c > index c265c0094b..bd57082a8d 100644 > --- a/audio/dsoundaudio.c > +++ b/audio/dsoundaudio.c > @@ -53,12 +53,14 @@ typedef struct { > typedef struct { > HWVoiceOut hw; > LPDIRECTSOUNDBUFFER dsound_buffer; > + bool first_time; > dsound *s; > } DSoundVoiceOut; > > typedef struct { > HWVoiceIn hw; > LPDIRECTSOUNDCAPTUREBUFFER dsound_capture_buffer; > + bool first_time; > dsound *s; > } DSoundVoiceIn; > > @@ -414,21 +416,32 @@ 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, wpos, act_size; > size_t req_size; > int err; > void *ret; > > - hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL); > + hr = IDirectSoundBuffer_GetCurrentPosition( > + dsb, &ppos, ds->first_time ? &wpos : NULL); > if (FAILED(hr)) { > dsound_logerr(hr, "Could not get playback buffer position\n"); > *size = 0; > return NULL; > } > > + if (ds->first_time) { > + hw->pos_emul = wpos; > + ds->first_time = false; > + } > + > req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul); > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > + if (req_size == 0) { > + *size = 0; > + return NULL; > + } > + > err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size, > &ret, NULL, > &act_size, NULL, false, ds->s); > if (err) { > @@ -508,18 +521,24 @@ static void *dsound_get_buffer_in(HWVoiceIn > *hw, size_t *size) > DSoundVoiceIn *ds = (DSoundVoiceIn *) hw; > LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer; > HRESULT hr; > - DWORD cpos, act_size; > + DWORD cpos, rpos, act_size; > size_t req_size; > int err; > void *ret; > > - hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, &cpos, > NULL); > + hr = IDirectSoundCaptureBuffer_GetCurrentPosition( > + dscb, &cpos, ds->first_time ? &rpos : NULL); > if (FAILED(hr)) { > dsound_logerr(hr, "Could not get capture buffer position\n"); > *size = 0; > return NULL; > } > > + if (ds->first_time) { > + hw->pos_emul = rpos; > + ds->first_time = false; > + } > + > req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul); > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > -- > 2.25.0 > > > Hi, > > I tested this patch running qemu-system-ppc with MacOS 9.2 and OSX 10.5. > Qemu was cross-compiled on Fedora 31 from > https://github.com/mcayland/qemu/tree/screamer. Host was Windows 10. > > The dsound locking errors are gone, so for this test case all seems OK. Thanks for testing! Does that mean we can add your tag to this patch? Tested-by: Howard Spoelstra <hsp.cat7@gmail.com> Regards, Phil. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-02-03 9:18 ` Philippe Mathieu-Daudé @ 2020-02-03 9:25 ` Howard Spoelstra 0 siblings, 0 replies; 10+ messages in thread From: Howard Spoelstra @ 2020-02-03 9:25 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: KJ Liew, Gerd Hoffmann, qemu-devel qemu-devel, Kővágó, Zoltán [-- Attachment #1: Type: text/plain, Size: 6218 bytes --] Cher Philippe, On Mon, Feb 3, 2020 at 10:18 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Howard, > > On 2/3/20 8:56 AM, Howard Spoelstra wrote: > > On Mon, Feb 3, 2020 at 12:02 AM Kővágó, Zoltán <dirty.ice.hu@gmail.com > > <mailto:dirty.ice.hu@gmail.com>> wrote: > > > > Windows (unlike wine) bails out when IDirectSoundBuffer8::Lock is > called > > with zero length. Also, hw->pos_emul handling was incorrect when > > calling this function for the first time. > > > > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com > > <mailto:DirtY.iCE.hu@gmail.com>> > > Reported-by: KJ Liew <liewkj@yahoo.com <mailto:liewkj@yahoo.com>> > > --- > > > > I've tested this patch on wine and a borrowed Windows 8.1 laptop, I > > could only test audio playback, not recording. I've cross-compiled > qemu > > using the docker image, for 64-bit. > > > > --- > > audio/dsound_template.h | 1 + > > audio/audio.c | 6 ++---- > > audio/dsoundaudio.c | 27 +++++++++++++++++++++++---- > > 3 files changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/audio/dsound_template.h b/audio/dsound_template.h > > index 7a15f91ce5..9c5ce625ab 100644 > > --- a/audio/dsound_template.h > > +++ b/audio/dsound_template.h > > @@ -244,6 +244,7 @@ static int dsound_init_out(HWVoiceOut *hw, > > struct audsettings *as, > > goto fail0; > > } > > > > + ds->first_time = true; > > obt_as.endianness = 0; > > audio_pcm_init_info (&hw->info, &obt_as); > > > > diff --git a/audio/audio.c b/audio/audio.c > > index f63f39769a..cb1efc6dc5 100644 > > --- a/audio/audio.c > > +++ b/audio/audio.c > > @@ -1076,10 +1076,8 @@ static size_t audio_pcm_hw_run_out(HWVoiceOut > > *hw, size_t live) > > while (live) { > > size_t size, decr, proc; > > void *buf = hw->pcm_ops->get_buffer_out(hw, &size); > > - if (!buf) { > > - /* retrying will likely won't help, drop everything. */ > > - hw->mix_buf->pos = (hw->mix_buf->pos + live) % > > hw->mix_buf->size; > > - return clipped + live; > > + if (!buf || size == 0) { > > + break; > > } > > > > decr = MIN(size / hw->info.bytes_per_frame, live); > > diff --git a/audio/dsoundaudio.c b/audio/dsoundaudio.c > > index c265c0094b..bd57082a8d 100644 > > --- a/audio/dsoundaudio.c > > +++ b/audio/dsoundaudio.c > > @@ -53,12 +53,14 @@ typedef struct { > > typedef struct { > > HWVoiceOut hw; > > LPDIRECTSOUNDBUFFER dsound_buffer; > > + bool first_time; > > dsound *s; > > } DSoundVoiceOut; > > > > typedef struct { > > HWVoiceIn hw; > > LPDIRECTSOUNDCAPTUREBUFFER dsound_capture_buffer; > > + bool first_time; > > dsound *s; > > } DSoundVoiceIn; > > > > @@ -414,21 +416,32 @@ 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, wpos, act_size; > > size_t req_size; > > int err; > > void *ret; > > > > - hr = IDirectSoundBuffer_GetCurrentPosition(dsb, &ppos, NULL); > > + hr = IDirectSoundBuffer_GetCurrentPosition( > > + dsb, &ppos, ds->first_time ? &wpos : NULL); > > if (FAILED(hr)) { > > dsound_logerr(hr, "Could not get playback buffer > position\n"); > > *size = 0; > > return NULL; > > } > > > > + if (ds->first_time) { > > + hw->pos_emul = wpos; > > + ds->first_time = false; > > + } > > + > > req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul); > > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > > > + if (req_size == 0) { > > + *size = 0; > > + return NULL; > > + } > > + > > err = dsound_lock_out(dsb, &hw->info, hw->pos_emul, req_size, > > &ret, NULL, > > &act_size, NULL, false, ds->s); > > if (err) { > > @@ -508,18 +521,24 @@ static void *dsound_get_buffer_in(HWVoiceIn > > *hw, size_t *size) > > DSoundVoiceIn *ds = (DSoundVoiceIn *) hw; > > LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer; > > HRESULT hr; > > - DWORD cpos, act_size; > > + DWORD cpos, rpos, act_size; > > size_t req_size; > > int err; > > void *ret; > > > > - hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, &cpos, > > NULL); > > + hr = IDirectSoundCaptureBuffer_GetCurrentPosition( > > + dscb, &cpos, ds->first_time ? &rpos : NULL); > > if (FAILED(hr)) { > > dsound_logerr(hr, "Could not get capture buffer > position\n"); > > *size = 0; > > return NULL; > > } > > > > + if (ds->first_time) { > > + hw->pos_emul = rpos; > > + ds->first_time = false; > > + } > > + > > req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul); > > req_size = MIN(req_size, hw->size_emul - hw->pos_emul); > > > > -- > > 2.25.0 > > > > > > Hi, > > > > I tested this patch running qemu-system-ppc with MacOS 9.2 and OSX 10.5. > > Qemu was cross-compiled on Fedora 31 from > > https://github.com/mcayland/qemu/tree/screamer. Host was Windows 10. > > > > The dsound locking errors are gone, so for this test case all seems OK. > > Thanks for testing! > > Does that mean we can add your tag to this patch? > > > Regards, > > Phil. > > Yes, so: Tested-by: Howard Spoelstra <hsp.cat7@gmail.com> Best, Howard [-- Attachment #2: Type: text/html, Size: 8768 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] audio/dsound: fix invalid parameters error 2020-02-02 23:02 Kővágó, Zoltán 2020-02-03 7:56 ` Howard Spoelstra @ 2020-02-06 13:38 ` Gerd Hoffmann 1 sibling, 0 replies; 10+ messages in thread From: Gerd Hoffmann @ 2020-02-06 13:38 UTC (permalink / raw) To: Kővágó, Zoltán; +Cc: qemu-devel, KJ Liew On Mon, Feb 03, 2020 at 12:02:23AM +0100, Kővágó, Zoltán wrote: > Windows (unlike wine) bails out when IDirectSoundBuffer8::Lock is called > with zero length. Also, hw->pos_emul handling was incorrect when > calling this function for the first time. > > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> > Reported-by: KJ Liew <liewkj@yahoo.com> Patch queued. thanks, Gerd ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-06 13:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200117182621.GB13724.ref@chuwi-lt0>
2020-01-17 18:26 ` [PATCH] audio/dsound: fix invalid parameters error KJ Liew
2020-01-18 6:30 ` Philippe Mathieu-Daudé
2020-01-27 1:46 ` Zoltán Kővágó
2020-01-31 7:55 ` Gerd Hoffmann
2020-02-01 18:28 ` KJ Liew
2020-02-02 23:02 Kővágó, Zoltán
2020-02-03 7:56 ` Howard Spoelstra
2020-02-03 9:18 ` Philippe Mathieu-Daudé
2020-02-03 9:25 ` Howard Spoelstra
2020-02-06 13:38 ` Gerd Hoffmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.