Re: [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?

[PATCH] winepulse: Add pulse driver, v8

[Charles Davis]

On Feb 28, 2012, at 10:58 PM, Chris Robinson wrote:
> 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?

It does: http://msdn.microsoft.com/en-us/library/ms686301(v=vs.85).aspx

For some reason, the condition variable API just hasn't been implemented in Wine yet.

Chip

[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