Hi,
I spent several evenings reviewing the code, here are my findings.
The Good
+ You did it!
+ outer layer (wave*) uses hwave, inner layer uses device parameter (except WINMM_Pause/Reset which break this separation).
+ ValidateAndLock pattern
+ Invoking driver callbacks outside of critical sections, when all slots are in a sane state again.
+ Removed overhead of pass-through mapper in case of direct format match. However, this may make computations of position and DoneHeaders more complicated. Indeed, I have not yet understood the logic in MarkDoneHeaders and associated queues.
Trivia
- Some more comments are needed. - Why does WINMM_Pause call IAC_Stop yet set device->stopped = FALSE?
- Document that g_device_handles MUST ONLY be modified within the player thread (otherwise MsgWait operates on undefined data), demonstrating that (part of) wave*Open must be handled there.
- Express units of entities in WINMM_DEVICE, like you do with last_clock_pos
- if(wait == 1 - WAIT_OBJECT_0) should be WAIT_OBJECT_0 + 1 (same by chance) Same with if(wait == g_devhandle_count - WAIT_OBJECT_0)
- The caller of WINMM_BeginPlaying supplying the device argument will have locked it already, thus Enter/LeaveCS therein is superfluous. Generally, functions that take a WINMM_Device* parameter should receive it locked by the caller.
- Memory leak in WINMM_PrepareHeader in case of acmStreamPrepareHeader error
- WOD_Open/WID_Open:cb_info is unused, memcpy(&cb_info,...) useless. Same for WOD/WID_Close
- WINMM_OpenDevice passed_fmt HeapAlloc is unchecked Prefer a stack-allocated WAVEFORMATEX and set passed_fmt = &stack_var
- Prefer C structure copy over memcpy
Logic
- I don't see a protection against waveInClose(waveOUThandle); It would be trivial to add a flag to WINMM_GetDeviceFromHWAVE except in WINMM_Pause/Reset.
- WINMM_Pause/Reset should take a device pointer, not hwave (which would additionally resolve the above point).
- WAVERR_STILLPLAYING is unknown to waveoutClose. It's not a bug, it's a feature? (close would imply stop like the MCI?? but how to unprepare the headers in such a case? IMHO the WINMM API is not designed for a close while playing and only STILLPLAYING makes sense)
- nBlockAlign is not sanitized and copied into device->bytes_per_frame. I mentioned long ago tests I wrote that supply incorrect avgbytes and blockalign to winmm yet don't disturb playback in native. IIRC I did not submit them because Wine would not work correctly with them.
- device->bytes_per_frame is misdefined and/or misused. It's initialised from the original format (e.g. MP3) but compared in PushData via queue_frames with PCM frames from IAC::GetBuffer.
Design
- Too bad out_caps.dwSupport is now set within winmm, not the ALSA/OSS/CoreAudio drivers. I loose hope that one day WAVECAPS_VOLUME will be set according to the host OS caps.
- How to avoid code duplication among WINMM_MapDevice and msacm32.drv/wavemap.c:wodOpen?
- WAVE_FORMAT_DIRECT is missing. It is said to prevent use of ACM (de)compressors. My understanding (untested) is that ACM transforms (e.g. frequency, channels) would still be used.
- WINMM_TryDeviceMapping opens the ACM twice, once with ACM_STREAMOPENF_QUERY, then for real. I recommend to open only once, because opening the ACM is an extremely expensive operation, known for delays. All drivers are loaded and queried in turn. I believe this is why people can't seem to Valgrind the winmm/wave and other tests: it just takes sooo much time repeatedly loading and unloading all the drivers!
- It may not be a good idea to handle all of wave*Open within the data push thread. Open may take ages, during which no data will be supplied to concurrently playing or recording sounds. Xruns are likely.
- Questionable use of BUFFERFLAGS_SILENT in WOD_PushData. When no data is available now, the buffer is flooded with silence, hence data submitted shortly afterwards will be delayed by as much (PulseAudio will accumulate 2 seconds of buffer data...), not by a minimal amount.
- The old drivers used ALSA_NearMatch(rate,pcm_rate). There's nothing like this currently in winmm to prevent using a very different rate. My observation is that snd_pcm_set_rate_near() will happily yield 48000 when asked for 8000! Such a variation should be refused by winmm (accepted by dsound?), or a rate converter be silently plugged in. However the code currently ignores ALSA's actual rate and performs all computations using the original one.
- Why do you use EVENTCALLBACK? Wine's mmdevapi (currently) signals events periodically from a timer. Winmm could do that itself.
- I'm not sure what to think about the observation that waveOutWrite returns an error when BeginPlaying fails yet the header was inserted into the queue. The app may expect the header and data to be reusable for something else because of the error, while Wine may play it from a subsequent BeginPlaying and hit undefined memory.
Well, BeginPlaying as of today always returns OK, but the flaw remains. BeginPlaying should be considered a performance optimization hint (start playing fast instead of after the next timer tick) and therefore not need a return value. After enqueuing, NOERROR should be the only return value.
- What about mapping some AUDCLNT_E_* to MMSYSERR_*?
Regards, Jörg Höhle