|
From: | Zoltán Kővágó |
Subject: | Re: [PATCH] audio/dsound: fix invalid parameters error |
Date: | Mon, 27 Jan 2020 02:46:58 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.9) Goanna/4.4 Interlink/52.9.7282 |
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 <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
dsound.patch
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |