[PATCH 03/10] winmm: Implement waveOut* on top of MMDevAPI
Joerg-Cyril.Hoehle at t-systems.com
Joerg-Cyril.Hoehle at t-systems.com
Mon Jul 25 06:52:59 CDT 2011
I spent several evenings reviewing the code, here are my findings.
+ 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.
- 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
- I don't see a protection against waveInClose(waveOUThandle);
It would be trivial to add a flag to WINMM_GetDeviceFromHWAVE except
- 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.
- 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
- 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_*?
More information about the wine-devel