Op 03-12-12 14:59, joerg-cyril.hoe...@t-systems.com schreef: > Hi, > > Maarten Lankhorst queried: >> Bump, anything wrong with this patch? > Here's my 0.0$ ... (standard DSound disclaimer here...) > > Using mmdevapi's events in DSound is basically TRT. > Then, the DSound mixer will be synchronized with mmdevapi writes. > > >> + /* ALSA is retarded, add a timeout.. */ >> + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); > How does that comment help the reader of the code? > >> + if (period_ms <= 15) >> + device->sleeptime = period_ms * 5 / 2; >> + else >> + device->sleeptime = period_ms * 3 / 2; > I expect such threshold functions to be continuous or at least monotonic. > A sawtooth is certainly unexpected. What is its purpose? > > What I would understand is a comment in the code saying that as of 2012, > all 3 wine*.drv mmdevapi drivers have a bug that they stop sending events > after IAudioClient_Stop. Therefore add a short timeout to the wait. Yeah, but in a followup commit I'm going to stop sending IAudioClient_Stop entirely. :-)
> If that's the reason for the timeout, then period_ms * 1.5 is > perfectly fine. There's no reason for a particular threshold. Note > that neither 1.5 nor 2.5 give you regular spacing around the time of > transition from playing to stopped. I just didn't want to make the timeout too short in case no processing is done yet. Alsa's native period is ~ 22ms (1024 samples / 44100 or 48000) with dmix despite claiming it to be otherwise.. I thought about doing something more complicated to make it smoother, but I'm really just increases it >2 buffers to compensate for jitter on lower latencies, so if it the timeout would be beyond 20 ms I don't care any more.. > > + IAudioClient_GetStreamLatency(device->client, &period); > + period_ms = (period + 9999) / 10000; > If IAC_Stop is the reason for the sleep time out, then it's obvious > that GetStreamLatency has no business here, rather than the device period. > > > I could understand a use of GetStreamLatency when it comes to computing > a reasonable size of the primary buffer. Got it right.. and this is a perfect valid use of IAudioClient GetStreamLatency here, the device could also be dead (AUDCLNT_E_DEVICE_INVALIDATED), in which case events are probably not fired any more. GetStreamLatency is also used for calculating the period size, see http://msdn.microsoft.com/en-us/library/windows/desktop/dd370874%28v=vs.85%29.aspx I don't think it's used as such in this commit yet, but in the mixer rework it's used it to calculate fragment length. > > + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime); > + ... if (ret) > WaitFor* return values are defined in terms of WAIT_OBJECT_0, WAIT_TIMEOUT > and WAIT_FAILED etc., not zero or non-zero. Except WAIT_OBJECT_0 is defined as 0. > >> + device->sleepev = CreateEventW(0, 0, 0, 0); >> + device->thread = CreateThread(0, 0, DSOUND_mixthread, device, 0, 0); > I haven't checked the overall style of DSound, but I prefer FALSE and > TRUE be used for things advertised as BOOL. > (I tend to write NULL for null pointer initialisation. > Note that I easily read "if (foo) ..." for boolean and pointer types; > I don't expect "if (foo == TRUE) or (foo != NULL) ...") > > > BTW, I still believe that mixing and resampling would find their best place > in mmdevapi, not DSound. Well some way for dsound and winmm to use them both, so winmm can do resampling internally.