http://bugs.winehq.org/show_bug.cgi?id=28723
--- Comment #43 from Alexey Loukianov mooroon2@mail.ru 2011-11-11 11:28:20 CST --- (In reply to comment #42)
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
Main problem here is that your ALSA period is bigger than MMDev period. As I had already mentioned you should expect underruns in such case. Probably we should not try to get of ALSA period setup during init but try to set it to be equal to mmdev period instead.
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).
Having three mmdev periods in ALSA buffer won't help in case (3*mmdev_period_frames < alsa_period_frames) == TRUE. What can be done when calculating write_limit is to also take into account the alsa_period timeframe. Easiest way is to take period=max(alsa,mmdev), but it would add excess latency which might be avoided by doing more precise calculations. I would see what can be done on this topic and post a new patch with proposed changes when done.
This fixes the problem for me ... 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?
The code you had written is exactly what I've been writing about in previous paragraph. I'm not too content with using 3*period while it should be enough to have 2*period - extra latency is not a good thing and it should be avoided if possible. 3*period would be especially harmful if we don't set ALSA period and it would be auto-set by ALSA to something really big.
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.
OK. I just had removed it from there as with the modifications I made GetCurrentPadding had been no longer relaying on any data from ALSA. Maybe it would be reasonable to move snd_pcm_state() trace into alsa_write_buffer and/or to timer callback handler - IMO there it would be more logical place to call ALSA stuff.
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.
Sure, I just had used the patch you had submitted as-is, without any attempts to make it more pretty. Having comments by preprocessor ifs in final patchset is no-no :-). I would clean it up in case we would head on "don't set ALSA period road" or uncomment it back in case "set ALSA period to match mmdev period" road would be chosen instead.
I know they're taken from my hackish test patch...
Sure it would be cleaned up. Just had been using your quick hackish patch as-is while testing.
(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.
This change is correct, but mmdev hitting xruns with my patch in + 5ms period - is wrong. My tests show that most likely xruns I hit are yet again caused by ALSA period ending up being bigger than mmdev period. Going to test more thoroughly today.
Thank you for review, I would come back with a new patchset for more :-).