[PATCH 03/10] winmm: Implement waveOut* on top of MMDevAPI

[Andrew Eikum]

On 07/25/2011 06:52 AM, Joerg-Cyril.Hoehle at 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