Re: [PATCH] winepulse: Add pulse driver, v8
Hey Joerg, Op 01-03-12 14:39, joerg-cyril.hoe...@t-systems.com schreef: > Hi, > > Maarten Lankhorst wrote: - Align buffer size to a multiple of period size >>> How can you pass the tests with that? It's wrong with both capture >>> (annoyingly IMHO) and playback. >> I only really need it for capture, rendering needs it too since the tests >> show that this is the case, >> and seems to be against the tests too that everything is a multiple of >> period. > What is the case? Where did you see that? > > Native mmdevapi does not align GetBufferSize on period boundaries,neither > render > nor capture. The current tests may not reveal it, because 500ms is a multiple > of 10ms, > but I've enough additional tests in my git and log data from testbot to be > sure about that. > > I consider it unsafe to diverge from native when rendering. Ah ok. I was under the impression it was supposed to, but I guess not then. >> but there's nothing in the code that depends on it. > Good. > >> For capture it's different, you need to keep track of packets somehow. >> [...] Having one packet that's not a period is a pain, > I felt the same pain with winecoreaudio. I think I'm going to agree (with > myself) to disagree > (with mmdevapi), as standards would say, and align capture GetBufferSize on > period boundaries. > This will considerably simplify the code. I've not changed winecoreaudio > capture yet. > > So I find it ok if winepulse does the same: > - capture buffer as multiple of periods if it simplifies packet handling, > - render buffer exactly like native, not a multiple of period (cf. MulDiv in > the tests). > > And I'll change the capture tests to ensure it asks for buffer sizes as > multiples of period, > so the divergence won't show up :-) > > BTW, please use the MulDiv computations so as to minimize differences among > the 3-4 drivers. > Or exhibit tests that prove them wrong. It is allowed to allocate a buffer with a larger than requested time, see: http://msdn.microsoft.com/en-us/library/windows/desktop/dd370875%28v=vs.85%29.aspx The comment in hnsBufferDuration indicates it may allocate a larger buffer. Also, exclusive mode rendering seems to fail the period check on my laptop. >> I'd have to recheck on my windows pc > I forgot: Don't be wary of returning GetMixFormat with 6 channels if PA's > device is 5:1. Native > does that too (we had a few logs on test.winehq with that result) > (it may be too early if dsound chokes on that, but that would be a bug in > dsound). I return the result of the pulseaudio configuration, which may indeed be 5.1. :) I should probably do another getmixformat for capturing, because while it can be 5.1 if you're using the monitor stream as source, it's not necessarily the case. For example 5.1 output to speakers with a usb headset at 32kHz rate for capturing. >> 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? > I've never seen IsFormatSupported return something else than GetMixFormat and > that is EXTENSIBLE. > For months I thought it would always return S_FALSE and GetMixFormat, no > matter how stupid the input, but: > a) I've no tested this with ADPCM or other such stuff; > b) I've not tested channel variations with >2 channels, lacking a 5:1 or such > card; > c) recent test.winehq has one Vista machine return E_UNSUPP_FORMAT > when the rate was != GetMixFormat. I have a patch in my queue to have the > render test accept that variation. That was my belief when testing too, unfortunately for dsound and winmm we will have to accept rate != GetMixFormat.rate, so that is a test I won't be able to pass with winepulse on wine. Fortunately the windows programs will not break on this, since they get the rate from GetMixFormat or IsFormatSupported. > What I believe is: > 1. During mmdevapi init, MS queries a GetMixFormat from each working device > and caches that. > 2. Later, IsFormatSupported returning GetMixFormat is a cheap operation: > clone_format(cached_mix_format). > None of our drivers work this way. Doing it would need some thinking about > dynamic reconfiguration and plug-in devices. I'm actually doing similar things. However on moving stream from old sink to new sink I'll keep returning the metrics of the initial sink. Hopefully it's not that big a problem, since pulseaudio will remember the device used for each stream. The next time the program executes it will get the updated GetMixFormat for the new sink. Winepulse does similar things for mix format actually, although maybe I should just cache the WaveFormatExtensible for capture and render instead of deriving it at runtime. ~Maarten
Re: [PATCH] winepulse: Add pulse driver, v8
Hi, Maarten Lankhorst wrote: >>> - Align buffer size to a multiple of period size >> How can you pass the tests with that? It's wrong with both capture >> (annoyingly IMHO) and playback. >I only really need it for capture, rendering needs it too since the tests >show that this is the case, >and seems to be against the tests too that everything is a multiple of period. What is the case? Where did you see that? Native mmdevapi does not align GetBufferSize on period boundaries,neither render nor capture. The current tests may not reveal it, because 500ms is a multiple of 10ms, but I've enough additional tests in my git and log data from testbot to be sure about that. I consider it unsafe to diverge from native when rendering. >but there's nothing in the code that depends on it. Good. >For capture it's different, you need to keep track of packets somehow. >[...] Having one packet that's not a period is a pain, I felt the same pain with winecoreaudio. I think I'm going to agree (with myself) to disagree (with mmdevapi), as standards would say, and align capture GetBufferSize on period boundaries. This will considerably simplify the code. I've not changed winecoreaudio capture yet. So I find it ok if winepulse does the same: - capture buffer as multiple of periods if it simplifies packet handling, - render buffer exactly like native, not a multiple of period (cf. MulDiv in the tests). And I'll change the capture tests to ensure it asks for buffer sizes as multiples of period, so the divergence won't show up :-) BTW, please use the MulDiv computations so as to minimize differences among the 3-4 drivers. Or exhibit tests that prove them wrong. >I'd have to recheck on my windows pc I forgot: Don't be wary of returning GetMixFormat with 6 channels if PA's device is 5:1. Native does that too (we had a few logs on test.winehq with that result) (it may be too early if dsound chokes on that, but that would be a bug in dsound). >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? I've never seen IsFormatSupported return something else than GetMixFormat and that is EXTENSIBLE. For months I thought it would always return S_FALSE and GetMixFormat, no matter how stupid the input, but: a) I've no tested this with ADPCM or other such stuff; b) I've not tested channel variations with >2 channels, lacking a 5:1 or such card; c) recent test.winehq has one Vista machine return E_UNSUPP_FORMAT when the rate was != GetMixFormat. I have a patch in my queue to have the render test accept that variation. What I believe is: 1. During mmdevapi init, MS queries a GetMixFormat from each working device and caches that. 2. Later, IsFormatSupported returning GetMixFormat is a cheap operation: clone_format(cached_mix_format). None of our drivers work this way. Doing it would need some thinking about dynamic reconfiguration and plug-in devices. Regards, Jörg
Re: [PATCH] winepulse: Add pulse driver, v8
Op 29-02-12 17:29, joerg-cyril.hoe...@t-systems.com schreef: > Hi, > > Chris is right about the format. The shared mode mixer ought to return > FLOAT(32), > and it always appears to return the extensible format as a consequence. > For weird reasons, wineoss may return integer formats, but that's certainly > going to cause some unexpecting app to crash. I think it's because wineoss may not always have float support because mixing float in the kernel would require special attention. >> +assert(oldpad >= This->pad); > Wasn't there a discussion some time ago that asserts are bad? > Better back out, return INVALIDATED or some such, but don't crash the app > just because there's a problem with sound. (I agree that winmm&mmdevapi > ought to contain self/consistency tests). This is an internal consistency problem, somehow we have more sound queued than we know about. I think crashing is the best thing it's a genuine bug that should never be possible, if I don't crash it will manifest itself differently, presumably in a subtle impossible to find way.. Outside of that callback, the assertion This->pad == This->bufsize_bytes - pa_stream_writable_size should hold. > It would be nice if you could attach some > WINETEST_DEBUG=2 render.log 2>&1 to bugzilla somewhere. > There is stuff that do not cause a failure yet diverges from native. > > +static void pulse_wr_callback(pa_stream *s, size_t bytes, void *userdata) > +if (This->event) > +SetEvent(This->event); > This should be the last thing in the callback. It may well cause your thread > to be kicked out by the scheduler. Oops, thanks for catching, it's why the other setevents are the last indeed. > +if (filled_one && This->event) > +SetEvent(This->event); > Don't the capture tests prove that events get in even when the buffer is > full? (please cross-check) You should get an event then, I steal the earliest full buffer. :) I don't think the full case is completely correctly handled yet. Unlike pulseaudio won't send multiples of fragsize, unlike for rendering, but since pa_stream_peek is guaranteed to return the same data until I drop it with pa_stream_drop, I suppose I could add a workaround that will only fill a multiple of periods. >> - Align buffer size to a multiple of period size > How can you pass the tests with that? It's wrong with both capture > (annoyingly IMHO) and playback. > > I don't have more time now (and know nothing about the pa_* API). > Is pa_mainloop_run the thread that dispatches the pulse_xyz_callbacks? > > I wouldn't mind a winepulse driver in 1.4.0 > I only really need it for capture, rendering needs it too since the tests show that this is the case, but there's nothing in the code that depends on it. For capture it's different, you need to keep track of packets somehow. This is why I keep the meta information in little packets until the application has come around to read them. Having one packet that's not a period is a pain, and seems to be against the tests too that everything is a multiple of period. ~Maarten
Re: [PATCH] winepulse: Add pulse driver, v8
Hi, Chris is right about the format. The shared mode mixer ought to return FLOAT(32), and it always appears to return the extensible format as a consequence. For weird reasons, wineoss may return integer formats, but that's certainly going to cause some unexpecting app to crash. >+assert(oldpad >= This->pad); Wasn't there a discussion some time ago that asserts are bad? Better back out, return INVALIDATED or some such, but don't crash the app just because there's a problem with sound. (I agree that winmm&mmdevapi ought to contain self/consistency tests). It would be nice if you could attach some WINETEST_DEBUG=2 render.log 2>&1 to bugzilla somewhere. There is stuff that do not cause a failure yet diverges from native. +static void pulse_wr_callback(pa_stream *s, size_t bytes, void *userdata) +if (This->event) +SetEvent(This->event); This should be the last thing in the callback. It may well cause your thread to be kicked out by the scheduler. +if (filled_one && This->event) +SetEvent(This->event); Don't the capture tests prove that events get in even when the buffer is full? (please cross-check) >- Align buffer size to a multiple of period size How can you pass the tests with that? It's wrong with both capture (annoyingly IMHO) and playback. I don't have more time now (and know nothing about the pa_* API). Is pa_mainloop_run the thread that dispatches the pulse_xyz_callbacks? I wouldn't mind a winepulse driver in 1.4.0 Regards, Jörg
Re: [PATCH] winepulse: Add pulse driver, v8
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
Re: [PATCH] winepulse: Add pulse driver, v8
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
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?
Re: [PATCH] winepulse: Add pulse driver, v8
Hey Dmitry, Op 28-02-12 18:42, Dmitry Timoshkov schreef: > Maarten Lankhorst wrote: > >> >From 8aa5903b1ee75a6c538d7e1d99560bcb39a47ed2 Mon Sep 17 00:00:00 2001 >> From: Maarten Lankhorst >> Date: Thu, 28 Apr 2011 09:45:18 +0200 >> Subject: [PATCH 10/10] winepulse: Add pulse driver, v8 > You forgot to fix the date, 1 Apr would be more appropriate I'd guess. > I'm surprised it used that date, but I'm no less serious about my intentions to get it included into wine. If you have any technical concerns I would be glad to hear them. ~Maarten
Re: [PATCH] winepulse: Add pulse driver, v8
Maarten Lankhorst wrote: > >From 8aa5903b1ee75a6c538d7e1d99560bcb39a47ed2 Mon Sep 17 00:00:00 2001 > From: Maarten Lankhorst > Date: Thu, 28 Apr 2011 09:45:18 +0200 > Subject: [PATCH 10/10] winepulse: Add pulse driver, v8 You forgot to fix the date, 1 Apr would be more appropriate I'd guess. -- Dmitry.
