At Wed, 14 Nov 2007 23:10:02 +0300, Stas Sergeev wrote:
Hi.
Takashi Iwai wrote:
The problem is that the condition of poll() is indeed broken without the hack. That is, it's no problem of snd_pcm_write_areas(). snd_pcm_write_areas() just calls snd_pcm_wait(), which calls nothing but poll(). If your program calls poll() by itself, you would see the similar problem.
Yes. But I am not sure how should it behave. For example, if the prog sets avail_min=1, then it should expect the snd_pcm_wait() to no longer work. But I think the write() should not be affected. Yes, snd_pcm_write_areas() just calls snd_pcm_wait(), but should it? Should write() depend on avail_min, or only snd_pcm_wait() should?
Both at the end. It defines the wake-up condition of poll(). If the space gets short by write, it needs to sleep until the space defined by avail_min becomes available again. That's what snd_pcm_wait() for. So, there shouldn't be difference between the logic of write-block and snd_pcm_wait().
If write() should depend on avail_min (which doesn't look logical to me), then this does imply mpg123 is doing the wrong thing by setting avail_min=1? And so, what would be the fix?
avail_min=1 makes really little sense. But CPU hog is the problem of the system implementation in practice, so we should fix alsa-lib not to trigger it. The previous hack was to fix this issue at the cost of fragileness against partial writes. It hits libao and mpg123 with small number of periods.
True, it should be fixed in a better way.
Unfortunately, I have no idea how's the better fix should look like. My patch implements exactly the logic I had in mind: leave snd_pcm_wait() to depend on avail_min, but make writei() to not depend on it (use period_size instead). Since you don't agree with that logic, please let me know what logic you prefer.
What I don't like is the place you fix it. This is not much better than the old hack, or it's even worse because it adds a hack in the common function not in the plugin.
Furthermore, as you said earlier, avail_min doesn't really work as expected. It gets "rounded" to period_size anyway, because that's where the interrupt arrives. Taking that into account, I can propose the following: deprecate avail_min and always set it to period_size, no matter what the program thinks. That will pretty much return the old behaveour (the hack), although this time - without the underruns. But I don't suppose you are going to like also this approach. :)
Hm, maybe that's no bad option. For the driver, avail_min=1 is almost as same as avail_min=period_size. The apps using avail_min=1 must be the code with dumb write without poll (otherwise it cannot work well), so the behavior of apps would be as same as before. And above all, the fix would be really easy like the patch below.
BTW, could you provide a bit more information how to reproduce the bug?
Simply play some mp3 with mpg123, run "top".
(Or, a test-case code would be best, as you know ;)
Well, stripping down the mpg123 code is not a rocket science, I can do that. But I am a bit busy at the moment, this will have to wait a few days. But I think there are really no problems reproducing it. :)
Then which version of mpg123 with which configuration? Your machine is not my machine, there are very different versions of mpg123. As you already know, a small detail makes great difference. So, be precise about the test case :)
thanks,
Takashi
diff -r 95e6e03f2e9d src/pcm/pcm.c --- a/src/pcm/pcm.c Mon Nov 12 12:01:16 2007 +0100 +++ b/src/pcm/pcm.c Thu Nov 15 11:50:05 2007 +0100 @@ -5577,6 +5577,12 @@ int snd_pcm_sw_params_set_avail_min(snd_ #endif { assert(pcm && params); + /* Fix avail_min if it's below period size. The period_size + * defines the minimal wake-up timing accuracy, so it doesn't + * make sense to set below that. + */ + if (val < pcm->period_size) + val = pcm->period_size; params->avail_min = val; return 0; }