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

Andrew Eikum aeikum at codeweavers.com
Mon Oct 22 10:42:49 CDT 2012


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.

> 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.

Andrew



More information about the wine-devel mailing list