[alsa-devel] rate plugin issue
Hi,
I committed your patch now on HG tree, as it's likely OK from the possible breakage on other apps, too.
Anyway, I swear that I'll never read any mails during my next vacation. That was so exhausting to be dragged to a lengthy discussion for band-aiding a badly designed compoment... :)
Takashi
Hello.
Takashi Iwai wrote:
I committed your patch now on HG tree, as it's likely OK from the possible breakage on other apps, too.
OK, first I have to apologize for not providing a test-case - its still in my todo if you need it. There are still a few things unclear, like, for example, how it fixes the portaudio, if, as you say, such an apps are not touching the code in question.
Anyway... as I said already, there are few more problems that were hidden before and are not any more. (I really tried to warn you :) For example, mpg123 will now consume 100% of CPU because of the nasty loop in snd_pcm_write_areas()... I am using something like the attached patch, but maybe you can come up with the better solution. Quick summary: mpg123 sets avail_min=1, so the loop spins without any rest. Before, the value was "adjusted", so it didn't spin that nasty way.
Anyway, I swear that I'll never read any mails during my next vacation. That was so exhausting to be dragged to a lengthy discussion for band-aiding a badly designed compoment... :)
But now, unfortunately, you won't get away from that. :) The hack used to cover more problems...
At Mon, 12 Nov 2007 22:25:17 +0300, Stas Sergeev wrote:
Hello.
Takashi Iwai wrote:
I committed your patch now on HG tree, as it's likely OK from the possible breakage on other apps, too.
OK, first I have to apologize for not providing a test-case - its still in my todo if you need it. There are still a few things unclear, like, for example, how it fixes the portaudio, if, as you say, such an apps are not touching the code in question.
Anyway... as I said already, there are few more problems that were hidden before and are not any more. (I really tried to warn you :) For example, mpg123 will now consume 100% of CPU because of the nasty loop in snd_pcm_write_areas()... I am using something like the attached patch, but maybe you can come up with the better solution. Quick summary: mpg123 sets avail_min=1, so the loop spins without any rest. Before, the value was "adjusted", so it didn't spin that nasty way.
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.
Anyway, I swear that I'll never read any mails during my next vacation. That was so exhausting to be dragged to a lengthy discussion for band-aiding a badly designed compoment... :)
But now, unfortunately, you won't get away from that. :) The hack used to cover more problems...
True, it should be fixed in a better way.
thanks,
Takashi
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? 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?
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. 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. :)
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. :)
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; }
Hi.
Takashi Iwai wrote:
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.
That depends on what logic does the fix imply. My logic was that the writei() should not depend on avail_min, and just always use period_size. Your logic is that they both should respect avail_min when it is >period_size, and from that point of view, my fix is of course wrong and at the wrong place.
And above all, the fix would be really easy like the patch below.
Your patch works very well. Actually, much better than mine, for the reasons I can't explain (there is probably another similar loop somewhere, otherwise they would work in the similar way). Just a few questions: 1. Do you want to "round" avail_min only when it is < period_size, or maybe you want something like this instead: --- avail_min += period_size - 1; avail_min -= avail_min % period_size; --- so that it is always rounded up? 2. What is it really good for? Why would the one want avail_min != period_size? Now, after you disallow avail_min<period_size, it makes even less sense - why would someone set avail_min>period_size, instead of increasing the period_size itself? So maybe something like this is also possible: --- /* there seem to be no reason to set avail_min, * period_size is enough */ avail_min = period_size; ---
Then which version of mpg123 with which configuration?
But I am not sure if you tried _any_ mpg123 with any configuration. :) Well, mine is mpg123-0.60-1.fc5.rf, running on top of Fedora8. --- High Performance MPEG 1.0/2.0/2.5 Audio Player for Layers 1, 2 and 3 version 0.60; written and copyright by Michael Hipp and others free software (LGPL/GPL) without any warranty but with best wishes --- But your patch works very well, so perhaps you don't even need to test it with mpg123 any more. :)
Thanks!
At Sat, 17 Nov 2007 17:59:53 +0300, Stas Sergeev wrote:
And above all, the fix would be really easy like the patch below.
Your patch works very well. Actually, much better than mine, for the reasons I can't explain (there is probably another similar loop somewhere, otherwise they would work in the similar way). Just a few questions:
- Do you want to "round" avail_min only
when it is < period_size, or maybe you want something like this instead:
avail_min += period_size - 1; avail_min -= avail_min % period_size;
so that it is always rounded up?
A good question. I'm a kind of conservative about that. avail_min itself doesn't guarantee the timing accuracy, so it's no driver/system problem even if the app is woken up slightly after the exact time.
But it's very unlikely that this round-up would break anything, though.
- What is it really good for? Why would
the one want avail_min != period_size? Now, after you disallow avail_min<period_size, it makes even less sense - why would someone set avail_min>period_size, instead of increasing the period_size itself? So maybe something like this is also possible:
/* there seem to be no reason to set avail_min,
- period_size is enough */
avail_min = period_size;
Changing period_size isn't allowed arbitrarilly because it's a hardware constraint. OTOH, avail_min is the software parameter. So avail_min > period_size makes sense in some cases. In my patch I disallowed the case avail_min < period_size because it doesn't work *practically*.
Then which version of mpg123 with which configuration?
But I am not sure if you tried _any_ mpg123 with any configuration. :)
Well, one of my versions on my machine has even no ALSA support.
thanks,
Takashi
Hi.
Takashi Iwai wrote:
Changing period_size isn't allowed arbitrarilly because it's a hardware constraint. OTOH, avail_min is the software parameter. So avail_min > period_size makes sense in some cases. In my patch I disallowed the case avail_min < period_size because it doesn't work *practically*.
OK, I see, thank you. Now it seems the patch is still not applied. Just to make sure, is it because you are still waiting for the test-case, or just haven't got around to apply it yet? I am asking to make sure that I am not responsible for the delay. :)
At Tue, 20 Nov 2007 00:09:12 +0300, Stas Sergeev wrote:
Hi.
Takashi Iwai wrote:
Changing period_size isn't allowed arbitrarilly because it's a hardware constraint. OTOH, avail_min is the software parameter. So avail_min > period_size makes sense in some cases. In my patch I disallowed the case avail_min < period_size because it doesn't work *practically*.
OK, I see, thank you. Now it seems the patch is still not applied. Just to make sure, is it because you are still waiting for the test-case, or just haven't got around to apply it yet? I am asking to make sure that I am not responsible for the delay. :)
Sorry, no, it's just because I've been busy for other things. Will be applied today soon.
thanks,
Takashi
Takashi Iwai wrote:
Sorry, no, it's just because I've been busy for other things. Will be applied today soon.
So is this patch supposed to fix the OSS sample-rate conversion plug-in? Has this patch been pushed to kernel.org yet? I had to disable CONFIG_SND_PCM_OSS_PLUGINS because it was causing underruns.
At Tue, 18 Dec 2007 09:43:38 -0600, Timur Tabi wrote:
Takashi Iwai wrote:
Sorry, no, it's just because I've been busy for other things. Will be applied today soon.
So is this patch supposed to fix the OSS sample-rate conversion plug-in? Has this patch been pushed to kernel.org yet? I had to disable CONFIG_SND_PCM_OSS_PLUGINS because it was causing underruns.
The patch was for alsa-lib, so it has nothing to do with kernel OSS emulation. Your problem should come from a difference place.
Takashi
At Mon, 12 Nov 2007 22:25:17 +0300, Stas Sergeev wrote:
Anyway... as I said already, there are few more problems that were hidden before and are not any more. (I really tried to warn you :) For example, mpg123 will now consume 100% of CPU because of the nasty loop in snd_pcm_write_areas()...
BTW, could you provide a bit more information how to reproduce the bug? (Or, a test-case code would be best, as you know ;)
Takashi
participants (3)
-
Stas Sergeev -
Takashi Iwai -
Timur Tabi