Re: [PATCH 5/6] dsound: rework ugly mixer logic

[Andrew Eikum]

Mostly good cleanup in this one. Some thoughts below...

On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote:
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index feef787..7817b88 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -73,10 +73,8 @@ struct DirectSoundDevice
> - DWORD                       writelead, buflen, state, playpos, mixpos;
> - DWORD                       writelead, buflen, aclen, fraglen, state, playpos, pad;

If you're introducing new variables, surely you can come up with
something more descriptive than "aclen." Something like
"ac_buffer_frames," maybe? I'm a big fan of units on my variable
names so mistakes like "pos_bytes = ac_frames;" are obvious.

Similar for "pad," maybe something like "in_mmdev_bytes" (Is that
actually used any more after your patches? You could re-use it if
not.)
> @@ -209,7 +207,6 @@ HRESULT DSOUND_PrimaryCreate(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryDestroy(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryPlay(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryStop(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> -HRESULT DSOUND_PrimaryGetPosition(DirectSoundDevice *device, LPDWORD playpos, LPDWORD writepos) DECLSPEC_HIDDEN;

Good riddance.
> @@ -682,147 +623,100 @@ static void DSOUND_PerformMix(DirectSoundDevice *device)
>    	LeaveCriticalSection(&device->mixlock);
>    	return;
>    }
> -
> - to_mix_frags = device->prebuf - (pad * device->pwfx->nBlockAlign + device->fraglen - 1) / device->fraglen;
> -
> - to_mix_bytes = to_mix_frags * device->fraglen;
> -
> - if(device->in_mmdev_bytes > 0){
> -
> DWORD delta_bytes = min(to_mix_bytes, device->in_mmdev_bytes);
> -
> device->in_mmdev_bytes -= delta_bytes;
> -
> device->playing_offs_bytes += delta_bytes;
> -
> device->playing_offs_bytes %= device->buflen;
> - block = device->pwfx->nBlockAlign;

Bleh. Do we really need to alias that?
> - if (maxq > device->fraglen * 3)
> -
> maxq = device->fraglen * 3;
> -

This seems to be replacing ds_snd_queue_max, right? Why remove the
configurability? Why 3 instead of the old default of 10? This should
be a separate patch at least.
> -
>
> writepos = (device->playpos + pad) % device->buflen;
>
> if (device->priolevel != DSSCL_WRITEPRIMARY) {
> -
> BOOL recover = FALSE, all_stopped = FALSE;
> -
> DWORD playpos, writepos, writelead, maxq, prebuff_max, prebuff_left, size1, size2;
> -
> LPVOID buf1, buf2;
> -
> BOOL all_stopped = FALSE;
> int nfiller;
> -
> BOOL native = device->normfunction == normfunctions[4];
> -
> DWORD bpp = device->pwfx->wBitsPerSample>>3;

Again, do we need to alias that?
> -static DWORD DSOUND_fraglen(DirectSoundDevice *device)
> -{
> - REFERENCE_TIME period;
> - HRESULT hr;
> - DWORD ret;
> -
> - hr = IAudioClient_GetDevicePeriod(device->client, &period, NULL);
> - if(FAILED(hr)){
> -
>    /* just guess at 10ms */
> -
>    WARN("GetDevicePeriod failed: %08x\n", hr);
> -
>    ret = MulDiv(device->pwfx->nBlockAlign, device->pwfx->nSamplesPerSec, 100);
> - }else
> -
>    ret = MulDiv(device->pwfx->nSamplesPerSec * device->pwfx->nBlockAlign, period, 10000000);
> -
> - ret -= ret % device->pwfx->nBlockAlign;
> - return ret;
>
> -}
> -

...
> - device->fraglen = MulDiv(device->pwfx->nSamplesPerSec, period, 10000000) * device->pwfx->nBlockAlign;

This should be a separate patch. I don't have an argument /against/
it, but why do you prefer 10ms over whatever the driver prefers?

Andrew