[PATCH] winepulse: Add pulse driver, v8

[Maarten Lankhorst]

Hey Chris,

Op 29-02-12 06:58, Chris Robinson schreef:
> 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?
I'm literally following the pulseaudio mainloop here. If you look at
their definitions they will map to those. Pulseaudio library
on win32 was having their own weird version of pthread_cond_*
which would go through wineserver multiple times for each
signalling. The extra context switches would have been a sure
way to kill any hope of efficiency and decreased the chances of
the scheduler doing things right. :)

pulseaudio/src/pulsecore/mutex-win32.c for reference

>> +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.
I'd have to recheck on my windows pc, to see if that's the case.
However if it is I don't see a problem with always specifying float
and removing the fix flag for it.

>> +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.
Aye indeed, seems to be an old copy/paste bug too. Was fixed on 27 Apr 2011 in winealsa.

>> +static enum pa_channel_position pulse_map[] = {
> This should probably be const.
Yes. :)

>> +        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.
Yes indeed, I've moved those checks to a separate function now,
Initialize was getting too long. I should probably use it for
IsFormatSupported, but likely PulseAudio supports any sane
format you throw at it, although I haven't tested the limits...

Is it really IsFormatSupported's job to deal with a WAVEFORMATEX struct
with only cbSize and wFormatTag and it will get out something sane all the
time, no matter how stupid the input?

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?


Yeah, mmdevapi checks priority, I set it to 0 if driver is unavailable, 3 if it is.
See AUDDRV_GetPriority

Thanks for reviewing,
~Maarten