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