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

[PATCH] winepulse: Add pulse driver, v8

[Maarten Lankhorst]

Op 29-02-12 17:29, [e-mail] 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

[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

[PATCH] winepulse: Add pulse driver, v8

[Maarten Lankhorst]

Hey Joerg,

Op 01-03-12 14:39, [e-mail] 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%2...
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:
> - During mmdevapi init, MS queries a GetMixFormat from each working device and caches that.
> - 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