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
