Hi Joerg,
On 06/01/2011 07:00 AM, Joerg-Cyril.Hoehle@t-systems.com wrote:
because of bug #27087, I had a look at the emerging mmdevapi code. Actually, it's the first time I read its documentation on MSDN. Here are a few comments of mine while reading the code.
Thanks for the careful review!
The handling of duration (-> buffer size) in winealsa.drv/mmdevdrv.c:AudioClient_Initialize does not perform enough validation. MSDN explicitly acknowledges that it could be 0. I believe it should be related to ALSA's buffer, or a minimum of something like 2*period_size. Or perhaps even 3*period size.
Yes. I have yet to see an application specify a 0 buffer, but we should handle it this way instead of failing. This is important and I'll see to fixing it soon.
IIRC MSDN also documents a maximum of 2s for renderer and 0.5 for capture.
These limitations might be true for exclusive mode, which isn't really implemented in Wine. Frankly, I see little reason to spend time implementing special handling of exclusive mode unless we find an app that does something that requires it.
AudioRenderClient_ReleaseBuffer does not check that written_frames is what GetBuffer returned. Use with AUDCLNT_BUFFERFLAGS_SILENT and you'll have memcpy overwrite arbitrary amounts of memory. MSDN says: AUDCLNT_E_INVALID_SIZE
Yeah, that should be checked more closely.
I believe Wine should currently include if (AUDCLNT_STREAMFLAGS_EVENTCALLBACK&& SHAREMODE_EXCLUSIVE) { FIXME("unsupported")& return E_ } because none of the buffer allocation and parameter checking specific to that mode that MSDN mentions in Initialize is implemented.
Is it really better to outright fail on that combination instead of having a (probably quite good) chance at just working? I think it would be worth having a loud FIXME when exclusive mode is requested, though.
When it (the ping pong buffer logic) will be implemented, some better logic to trigger SetEvent() will probably be needed, because I'm unsure whether CreateTimerQueueTimer() provides the required stability over time and possibly ignores a laptop's suspend/resume cycle or xruns. In exclusive mode, reception of an event is a guarantee that GetBuffer(GetBufferSize frames) will succeed.
Hmm, perhaps. It would be nice to have an application that actually uses exclusive mode, to see how it behaves with this "non-exclusive" implementation. I have yet to see an application that uses exclusive mode, though...
I suggest the values returned by AudioClient_GetDevicePeriod be adjusted to match what test.winehq shows. 10ms/3ms makes some sense, 10ms is mentioned a lot in MSDN and blogs. 3ms is small yet has a chance to work without glitch with MSDN's exclusive mode example.
These were tuned basically by hand, to values that worked well for the small number of test applications I had. I could try tweaking them to 10ms/3ms and see if it still works. I agree that being consistent with Windows would be somewhat better arbitrary values than the ones we currently have.
I can't remember seeing input validation of broken avgbytes and blockalign in the WAVEFORMAT parameter. I showed some tests some month ago that prove that unlike Wine, winmm is not disturbed by bad values. How does mmdevapi react to them?
I believe mmdevapi actually should validate those parameters, but I'm not certain. Tests would obviously clear that up.
None of the things above (except the 0 size buffer one) are really in need of urgent fixing, so I probably won't get to them for a while. I have made notes about them, though. If you would like to send patches fixing them (with tests, etc of course), please do. Notice that it's likely that the other drivers have the same problems.
Thanks again, Andrew