Re: [PATCH] winepulse: Add pulse driver, v8
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?
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
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
participants (3)
-
Charles Davis -
Chris Robinson -
Maarten Lankhorst