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

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

[Andrew Eikum]

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