rate plugin issue

rate plugin issue

[Takashi Iwai]

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

Re: rate plugin issue

[Stas Sergeev]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

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...


[-- Attachment #2: pcm.c.diff --]
[-- Type: text/x-patch, Size: 1084 bytes --]

--- pcm.c.old	2007-11-04 17:57:01.000000000 +0300
+++ pcm.c	2007-11-07 21:34:59.000000000 +0300
@@ -6465,19 +6465,35 @@
 			goto _end;
 		}
 		if ((state == SND_PCM_STATE_RUNNING &&
-		     (snd_pcm_uframes_t)avail < pcm->avail_min &&
 		     size > (snd_pcm_uframes_t)avail) ||
 		    (size >= pcm->xfer_align &&
 		     (snd_pcm_uframes_t)avail < pcm->xfer_align)) {
+			snd_pcm_sframes_t avail_min, orig_avail_min;
+			snd_pcm_sw_params_t params;
 
 			if (pcm->mode & SND_PCM_NONBLOCK) {
 				err = -EAGAIN;
 				goto _end;
 			}
 
+			avail_min = (size < pcm->period_size ?
+				size : pcm->period_size);
+			orig_avail_min = pcm->avail_min;
+			if (avail_min != pcm->avail_min) {
+				snd_pcm_sw_params_current(pcm, &params);
+				params.avail_min = avail_min;
+				snd_pcm_sw_params(pcm, &params);
+			}
+
 			err = snd_pcm_wait(pcm, -1);
 			if (err < 0)
 				break;
+
+			if (orig_avail_min != pcm->avail_min) {
+				params.avail_min = orig_avail_min;
+				snd_pcm_sw_params(pcm, &params);
+			}
+
 			goto _again;			
 		}
 		if ((snd_pcm_uframes_t) avail > pcm->xfer_align)

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: rate plugin issue

[Takashi Iwai]

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

Re: rate plugin issue

[Takashi Iwai]

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

Re: rate plugin issue

[Stas Sergeev]

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. :)

Re: rate plugin issue

[Takashi Iwai]

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;
 }

Re: rate plugin issue

[Stas Sergeev]

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!

Re: rate plugin issue

[Takashi Iwai]

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:
> 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?

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.

> 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;
> ---

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

Re: rate plugin issue

[Stas Sergeev]

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. :)

Re: rate plugin issue

[Takashi Iwai]

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

Re: rate plugin issue

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: rate plugin issue

[Takashi Iwai]

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