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

[Maarten Lankhorst]

Op 22-10-12 17:42, Andrew Eikum schreef:
> On Sat, Oct 20, 2012 at 12:03:55AM +0200, Maarten Lankhorst wrote:
>> 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.
>>
> Fair enough, I hadn't thought of that. I still think "aclen" is a
> crummy name :P
>
>> And.. get out of my bikeshed!
>>
> Well, it does lead to errors (bbbf72ddcb1202) and I don't see any harm
> in being explicit. It'll future-proof it, in case future variables
> have some other unit.
>
> In any case, not a patch-killer, of course.
>
>>>> +	block = device->pwfx->nBlockAlign;
>>> Bleh. Do we really need to alias that?
>> It's my bikeshed to paint! :P
>>
> My objection is it makes the code harder to read, which is something
> we really don't need in dsound. For an even worse example, see the
> w/wi/wfe/wfe2/wfe3 web in your 3rd patch.
Yeah got tired of that too and fixed it in the atomic reopendevice rework, it uses
better descriptions instead, and returns a newly allocated WAVEFORMATEX
which is more clear for use.

In this patch wfe2 is the format returned from IsFormatSupported, wfe3 is the
temporary float format I use for testing.

In the atomic reopendevice I thought about killing an extra copy from that step,
but it's not worth it. If the allocation fails it's handled correctly, and the
reallocation of device->buffer would very likely fail, too.

>> 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.
>>
> Okay, thanks for explaining. I was going to give the behavior changes
> a closer look after we figure out patch 3 and I get a chance to run my
> tests.
>
That's actually the easiest patch, I pretend primary format is 22050 / 8 / 2 like windows,
until setformat is called and then it copies the actual wfx from the current audioclient
if priority != writeprimary, else it attempts to set it through creating a new iaudioclient
with the specified format. Switching to and from primary priolevel will also re-create the
audioclient, and if not writeprimary the preferred format will be float since it saves us a
step in the mixer.

~Maarten