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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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.