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
