Created attachment 52483 [details] [review]
patch2

make it works for me (tm)

I can replicate the problem so i've tried to take a look.. btw, i'm not really
an audio expert!

from gstalsa.c:
 /* FIXME: We should use snd_pcm_hw_params_set_rate instead of 
     * snd_pcm_hw_params_set_rate_near here. Unfortunately alsa fails in that case
     * far more often and seems to handle this quite well. (Example: ENS1371 
     * driver on alsalib 1.0.5, kernel 2.6.6-mm5). If it sets far too wrong sample
     * rates, we need to revert back to snd_pcm_hw_params_set_rate or check the
     * rate that was set.
     */
Removing this change make it works for me.. (the previous patch does that).
If someone want to test/comment the patch! thanks!

This requires our probing to be ok. Did you check this on multiple pieces of
hardware, and for each with all of plughw:X, hw:X and dmix?

of course not :) i don't think i've fixed the problem, i've just attached a
patch that make alsasink works for me w/o specifying the device property. Maybe
this can be useful to someone else that has the knowledge to fix this bug.. or
maybe i've just lost some time.

There seem to be two different APIs for snd_pcm_hw_params_set_rate_near(). On my
system, I have a prototypes in /usr/include/alsa/pcm.h like this:

int snd_pcm_hw_params_set_rate_near(snd_pcm_t *pcm, snd_pcm_hw_params_t *params,
unsigned int *val, int *dir);

and in /usr/include/alsa/pcm_old.h like this:

unsigned int snd_pcm_hw_params_set_rate_near(snd_pcm_t *pcm, snd_pcm_hw_params_t
*params, unsigned int val, int *dir);

where pcm_old.h has a head saying "Old 0.9.x API" and the libasound2 version is
1.0.9 (snd_pcm_hw_params_set_rate() has the same prototype for both btw). 

Our configure.ac check only requires >= 0.9.1 though, so maybe that's somewhere
where we screw up as well?

Cheers
 -Tim

I've looked into this a bit and it looks to me like there are basically two
kinds of problems:

 (a) probing the 'default' device often returns useless values (if there's a
     plug layer it will claim to support any format it can convert into a
     supported hardware format, which is more or less anything; oh, and it
     reports values like max_channels=10000 and stuff like that; probing a
     specified device (hw:0, dmix:0) gives more realistic and useful results)

 (b) I think there's something not quite kosher in how we use period_size.
     This pipeline for example doesn't work right 

        sinesrc ! audio/x-raw-int,rate=22050 ! alsasink device=default

     It gives intermittent/distorted sound output on both my desktop 
     (via6x6 card) and the laptop (intel8x0).

     Using the same value for period-size that is returned by 

       snd_pcm_hw_params_get_period_size()

     makes things work for me. This value tends to be lower than the value
     of 8192 that we use as default value (this is usually the case for other
     rates/configurations as well).
     I'm not really sure what the significance of this value is, but maybe
     we should have a -1 default value here (=use what's returned by the above
     function) and only configure a fixed value if the user explicitely set it.


I've got somewhat ugly patches for both of these that need some cleaning up and
will put together a small test application that tests a variety of output
formats, so we can easily test these and other possible fixes on a variety of
hardware.

Just as a random update.

Created attachment 53329 [details] [review]
patch #1 - improve probing

This patch should improve probing a lot when using the "default" device. In
many use cases (e.g. where default maps to hw:0 or dmix:0) it should give us
much better and more narrowly-defined caps.

Okay, I've committed a modified version of the patch above, so that the extra
probing can be switched off using the 'advanced-probing' property.

2005-10-31  Tim-Philipp Muller  <tim at centricular dot net>

        * ext/alsa/gstalsa.c: (gst_alsa_class_init),
        (gst_alsa_set_property), (gst_alsa_get_property),
        (gst_alsa_get_caps), (gst_alsa_open_audio_device),
        (gst_snd_pcm_info_get_real_device), (gst_alsa_open_audio):
        * ext/alsa/gstalsa.h:
          When the default device is being used, try to probe the caps
          of the underlying device instead if possible. This should give
          us more narrowly defined caps that are closer to the hardware's
          capabilities. This is enabled by default, but can be switched
          off via the new 'advanced-probing' property (#315121).


I hope that this, in combination with the fixes from bug #318767 and bug
#318273, should solve a lot of problems people have been having with the
GStreamer alsasink.


Some of the above debian.org bugs are not GStreamer bugs though, but due to
broken pipelines. When decoding ogg/vorbis files, the result with be float
audio. audioscale cannot process float audio, so you get that 'failed to set
caps' error. It should work though if you put an 'audioconvert' right after spider.


Note also that the patch will give more narrowly defined caps, which means that
having audioconvert/audioscale in the pipeline (in the right order) is even more
crucial than it was before. For example, on my debian sid dmix is used by
default, so with the above patch the caps accepted will only be

  audio/x-raw-int, width=(int)16, depth=(int)16, signed=(boolean)true,
endianness=(int)1234, rate=(int)48000, channels=(int)2

so even something like

  gst-launch-0.8 sinesrc ! alsasink

won't work anymore without at least an audioconvert in the middle (to convert
from the 1 channel sinesrc produces to the 2 channels alsasink wants).

This might be a bit of a hassle in the short term, but any applications or
pipelines that don't work after the changes use broken pipelines anyway and need
to be fixed.



Please give these changes a spin and let me know how it goes.

Cheers
 -Tim

Tim, your changes certainly fixed a bunch of problems, thanks.

A new bug was introduced with respect to handling of the buffer-size parameter for the alsasink and was reported against quolibet in Debian as bug http://bugs.debian.org/344864.

David Riebenbauer was kind enough to write a patch which does the following:
"Essentially this changes period_size and period_count to
desired_period_size and desired_period_count in the getter and setter
for buffer_size."

Created attachment 56568 [details] [review]
use desired_period_size and desired_period_count in getter and setter for buffer-size

Patch from comment #10 applied, many thanks:

2006-01-08  Tim-Philipp Müller  <tim at centricular dot net>

        * ext/alsa/gstalsa.c: (gst_alsa_set_property),
        (gst_alsa_get_property):
          Must use desired_period_size and desired_period_count here after
          last round of fixes, otherwise we'll get a division-by-zero when
          setting the buffer-size property (#315121, patch by: David
          Riebenbauer).

Appears to be fixed. Please reopen if issues remain.