[PATCH] winepulse: Add pulse driver, v8

[Chris Robinson]

On Tuesday, February 28, 2012 5:32:13 PM Maarten Lankhorst wrote:
> + * This is basically the same as the pa_threaded_mainloop implementation,
> + * but that cannot be used because it uses pthread_create directly
> + *
> + * pa_threaded_mainloop_(un)lock -> pthread_mutex_(un)lock
> + * pa_threaded_mainloop_signal -> pthread_cond_signal
> + * pa_threaded_mainloop_wait -> pthread_cond_wait

I'm curious why you're using pthreads. Doesn't WinAPI have anything comparable 
to pthread_cond_wait?

> +static void pulse_probe_settings(void) {
...
> +        ret = pa_stream_connect_playback(stream, NULL, &attr,
> +        PA_STREAM_START_CORKED|PA_STREAM_FIX_RATE|PA_STREAM_FIX_FORMAT|PA_S
> TREAM_FIX_CHANNELS|PA_STREAM_EARLY_REQUESTS, NULL, NULL);

Is it necessary to fix the format? MSDN says that the audio service always 
works with floating-point, and GetMixFormat always specified floating-point in 
my dealings with it. I don't think it should blindly return whatever 
PulseAudio does, unless it can be shown that Windows doesn't always give 
floating-point.

> +static WAVEFORMATEX *clone_format(const WAVEFORMATEX *fmt)
> +{
> +    WAVEFORMATEX *ret;
> +    size_t size;
> +
> +    if (fmt->wFormatTag == WAVE_FORMAT_EXTENSIBLE)
> +        size = sizeof(WAVEFORMATEXTENSIBLE);
> +    else
> +        size = sizeof(WAVEFORMATEX);
> +
> +    ret = HeapAlloc(GetProcessHeap(), 0, size);

This should probably use CoTaskMemAlloc, as it's used for IsFormatSupported. 
I'm also curious if IsFormatSupported should always return a 
WAVE_FORMAT_EXTENSIBLE object.

> +static enum pa_channel_position pulse_map[] = {

This should probably be const.

> +        if (fmt->wFormatTag == WAVE_FORMAT_IEEE_FLOAT)
> +            This->ss.format = PA_SAMPLE_FLOAT32LE;
...
> +        if (IsEqualGUID(&wfe->SubFormat, &KSDATAFORMAT_SUBTYPE_IEEE_FLOAT))
> +            This->ss.format = PA_SAMPLE_FLOAT32LE;

You likely should check that the format specifies 32 bits, and not 64 or 
something different like that.


This isn't an all-inclusive lookover, just some things that caught my eye. 
Also, does this driver fail to load if it can't connect to a pulse server?