[alsa-devel] 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;
 }