http://bugs.winehq.org/show_bug.cgi?id=28723
--- Comment #42 from Andrew Eikum aeikum@codeweavers.com 2011-11-11 10:51:30 CST --- (In reply to comment #39)
Created attachment 37437 [details] Proposed patchset, try #2
The first patch gave me tons of underruns (all of the following is without PulseAudio). I have the following statistics in IAC::Initialize():
trace:alsa:AudioClient_Initialize alsa_period_frames: 470 trace:alsa:AudioClient_Initialize ALSA buffer size: 2352 frames trace:alsa:AudioClient_Initialize MMDevice period: 0x186a0 * 100ns, 220 frames trace:alsa:AudioClient_Initialize MMDevice buffer size: 2560 frames
But then I see: err:alsa:alsa_write_data avail: 1930, held_frames: 2496 trace:alsa:alsa_write_data XRun state, recovering
So it looks like ALSA is reporting an underrun when there isn't enough data in the ALSA buffer to take a full alsa_period_frames chunk (2352 - 1930 = 422 < 470). So perhaps we need to tweak the write_limit calculation to ensure there's always at least three mmdev periods _and_ three ALSA periods (three in case we get the period pattern Wine,Alsa-->Alsa,Wine).
This fixes the problem for me (should clean up the frames-in-alsa-buffer calculation redundancy, and you'll need to add the alsa_period_frames member):
- write_limit = This->mmdev_period_frames; - /* Make sure ALSA has enough samples to play till next timer event, to get - xrun-free behavior we need ALSA to have 2x mmdev_period frames to play. */ - if((This->alsa_bufsize_frames - avail) <= This->mmdev_period_frames) write_limit += This->mmdev_period_frames; + write_limit = 0; + while(This->alsa_bufsize_frames - avail + write_limit < This->mmdev_period_frames * 3 || + This->alsa_bufsize_frames - avail + write_limit < This->alsa_period_frames * 3) + write_limit += This->mmdev_period_frames;
I tested this with a few games with PulseAudio as well, and it worked. Does this continue to work with RAGE and your other test games?
Some comments on the code in your patches:
- TRACE("PCM state: %u, avail: %ld, pad: %u\n", - snd_pcm_state(This->pcm_handle), avail_frames, *out); + *out = This->held_frames; + TRACE("pad: %u\n", *out);
I think the snd_pcm_state() value is useful here, if only for debugging the PulseAudio bug. Unless you have a reason to get rid of it, I'd rather keep it in.
+#if 0 ... +endif
When you are getting ready to submit the patches, you can just remove this code. This patch also creates an unused variable which you can remove, alsa_period_us.
+ n = max(duration / 10, This->mmdev_period_rt * 3 / 10) ; /* duration converted to us or 3x our period size */ + d = 0;
I know they're taken from my hackish test patch, but please choose a better variable name than "n" (alsa_buffer_us or something). And you can get rid of "d" entirely and pass NULL instead.
+ TRACE("n: %u, d: %d\n", n, d);
We trace these values more formally below in that patch, so you can just get rid of these debug TRACEs.
(In reply to comment #41)
Changing dsound's primary.c so it don't try to set period size to pre-defined value of 50000 seems to fix the problem - no more unexpected xruns, I've got only four at expected places after ~5 minutes of RAGE gameplay.
This change is obviously correct, so there should be no problem submitting it.
specs require to ignore requested period size and use default instead if AudioClient is being opened in shared more, which is not the case with the current implementation.
This is pretty easy to fix with tests. It's on my list, but feel free to beat me to it :)