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

[Maarten Lankhorst]

Op 19-10-12 15:54, Andrew Eikum schreef:
> 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.)
However what I do makes sense here, since all of the units in dsound right
now are bytes, unless that gets changed. So I stick to them.
I'm all for changing it to frames, but it was out of the scope for this patch.

If you want to do it like that it's better to go all the way and introduce helpers
for all of wine's drivers and dsound/winmm to use instead. This would improve
readability a lot more instead of reinventing the wheel everywhere..

And.. get out of my bikeshed!

>> @@ -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?
It's my bikeshed to paint! :P

Followup patch I didn't send in yet on removed this entirely and just consolidated both cases.

>
>> +	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.
No this does not replace ds_snd_queue_max, I always fill up the buffer to full,
but I'm supposed to get a event every period, so instead of always filling the
buffer to full I spread it out over multiple periods so hopefully applications
have some time to catch up. This gets rid of clipping at the start.

See also below why..
>> +	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?
A followup patch removes it entirely and just mixes directly to the buffer for the native
case (most common, only oss4 and (maybe, just guessing?) coreaudio don't support
float, so I didn't want to touch that case even more.

>> -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?
I guess from the earlier comment you didn't notice I filled the complete buffer?

I also refer you to GetStreamLatency where I was getting the period from,
each time the event is fired you should write at least fraglen data.

I'm being generous here and write 3 periods in the mixer, since I expect
that wine will not always keep up. But this does not replace ds_snd_hel_frags
in any way and instead I just fill the whole buffer that's given to me.

I'm hoping that by limiting to only 3 periods the cpu usage will be smoothed out
more over time, because after all we would already be more than 3 periods behind
which leaves us in a serious chance of underrunning already..

Please take it from me: IT'S OK TO FAIL THOSE MMDEVAPI PERIOD TESTS,
BUT PLEASE DON'T LIE ABOUT THE HARDWARE CAPABILITIES REGARDING
TO MINIMUM PERIOD, DEFAULT PERIOD AND STREAM LATENCY.

Well.. default period can be 10 ms if you want, and minimum period is below 10 ms.
Helps pass some tests, but I degress..

For example Skyrim with a 36 ms stream latency will just buffer in more data to
compensate instead. But it can't do it if you report that it's fine to feed data every 5 ms.
There's no reason for dsound to be special, so I'd rather have it follow mmdevapi
behavior more closely here..

~Maarten