On 07/25/2011 06:52 AM, Joerg-Cyril.Hoehle@t-systems.com wrote:
I spent several evenings reviewing the code, here are my findings.
Thanks! Lots of good comments here. I've made notes about your suggestions to come back to later. Some specific responses follow...
- Some more comments are needed.
- Why does WINMM_Pause call IAC_Stop yet set device->stopped = FALSE?
You're right, the usage of stopped is confused. I need to examine it more closely and figure out what it was intended to do.
- Express units of entities in WINMM_DEVICE, like you do with last_clock_pos
Hmm, I think they all have units. bytes_per_frame is in bytes, samples_per_sec is in samples, ofs_bytes is in bytes, played_frames is in frames (although "samples vs frames" is a bit ambiguous). :)
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
All little things worth fixing. Patches welcome, if you would like the credit.
- 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)
I just wrote a quick little test, and it looks like you're right. It does also let you close a device without unpreparing all of the headers.
- 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.
You're right. It's treated as if all data is PCM, which it shouldn't.
- 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!
Good idea.
- 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.
Theoretically possible, but probably not worth the effort unless we find an app which actually does a lot of opening and closing.
- 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.
Are you sure? BUFFERFLAGS_SILENT shouldn't silence the whole buffer, just insert avail_frames worth of silence, which is what was intended. We should only accumulate 2 seconds if there is a 2 second underrun from WinMM's client. That seems like sane behavior to me.
- Why do you use EVENTCALLBACK? Wine's mmdevapi (currently) signals events periodically from a timer. Winmm could do that itself.
I guess. I don't really see the benefit. Callback mode is easier to work with, I think, since we know the driver is ready for data.
Andrew