[MPlayer-dev-eng] [PATCH] Add audio support for sndio API.

[Reimar Döffinger]

On Wed, Jun 20, 2012 at 08:58:50PM -0400, Brad Smith wrote:
> +if test "$_sndio" = auto ; then
> +  cat > $TMPC << EOF
> +#include <sndio.h>
> +int main(void) { struct sio_par par; sio_initpar(&par); return 0; }
> +EOF
> +  _sndio=no
> +  cc_check -lsndio && _sndio=yes
> +fi

Can't you just use e.g.
statement_check sndio.h sio_initpar(NULL)
?

> +	if (!havevol)
> +		return CONTROL_FALSE;

That isn't quite right, it will return FALSE instead of UNKNOWN for all
instead of just the volume ones.

> +/*
> + * call-back invoked to notify of the hardware position
> + */
> +static void movecb(void *addr, int delta)
> +{
> +	delay -= delta * (int)(par.bps * par.pchan);
> +}
> +
> +/*
> + * call-back invoked to notify about volume changes
> + */
> +static void volcb(void *addr, unsigned newvol)
> +{
> +	vol = newvol;

If these are called from a different thread, things get a bit
complicated. At least you should make vol and delay volatile,
even though in theory that might not be enough on all architectures.

> +	switch (format) {
> +	case AF_FORMAT_U8:
> +		par.bits = 8;
> +		par.sig = 0;
> +		break;
> +	case AF_FORMAT_S8:
> +		par.bits = 8;
> +		par.sig = 1;
> +		break;
> +	case AF_FORMAT_U16_LE:
> +		par.bits = 16;
> +		par.sig = 0;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_S16_LE:
> +		par.bits = 16;
> +		par.sig = 1;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_U16_BE:
> +		par.bits = 16;
> +		par.sig = 0;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_S16_BE:
> +		par.bits = 16;
> +		par.sig = 1;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_U24_LE:
> +		par.bits = 24;
> +		par.bps = 3;
> +		par.sig = 0;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_S24_LE:
> +		par.bits = 24;
> +		par.bps = 3;
> +		par.sig = 1;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_U24_BE:
> +		par.bits = 24;
> +		par.bps = 3;
> +		par.sig = 0;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_S24_BE:
> +		par.bits = 24;
> +		par.bps = 3;
> +		par.sig = 1;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_U32_LE:
> +		par.bits = 32;
> +		par.sig = 0;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_U32_BE:
> +		par.bits = 32;
> +		par.sig = 0;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_S32_LE:
> +		par.bits = 32;
> +		par.sig = 1;
> +		par.le = 1;
> +		break;
> +	case AF_FORMAT_S32_BE:
> +		par.bits = 32;
> +		par.sig = 1;
> +		par.le = 0;
> +		break;
> +	case AF_FORMAT_AC3_BE:
> +	case AF_FORMAT_AC3_LE:
> +		par.bits = 16;
> +		par.sig = 1;
> +		par.le = SIO_LE_NATIVE;

Are you sure this is right and sndio always supports both?

> +	default:
> +		mp_msg(MSGT_AO, MSGL_V, "ao2: unsupported format\n");
> +		par.bits = 16;
> +		par.sig = 1;
> +		par.le = SIO_LE_NATIVE;

IMHO you should try to use a lookup table for this

> +	if (par.bits == 8 && par.bps == 1) {
> +		format = par.sig ? AF_FORMAT_S8 : AF_FORMAT_U8;	

Trailing whitespace.
Also ideally if you used a table above you should be able to reuse it
here.

> +	ao_data.format = ac3 ? AF_FORMAT_AC3_NE : format;

Ah, I guess that answers my question from above.

> +	/* avoid resampling for close rates */
> +	if ((par.rate >= rate * 0.97) && (par.rate <= rate * 1.03))

Not sure this is a good idea at all, this is 3% and can cause serious
A-V desync.
Also the inner () are not necessary.

> +	if (ao_data.samplerate != rate) {
> +		/* apparently mplayer rounds a little when resampling.
> +		 * anyway, it doesn't write quite a full buffer on the first
> +		 * write, which means libsndio never actually starts up
> +		 * because it's trying to fill the buffer.  this is
> +		 * enough for everything I have come across.
> +		 */

That should be investigated. Also it should not cause sndio to never
start up, that would mean the whole logic is brittle, audio filters
might generate quite "random" amounts of data.

> +static int play(void *data, int len, int flags)
> +{
> +	int n;
> +
> +	n = sio_write(hdl, data, len);
> +	delay += n;
> +	if (flags & AOPLAY_FINAL_CHUNK)
> +		reset();

I don't think you should reset here. Even doing nothing would
be preferable, this can lose quite a lot of audio.

> +	/*
> +	 * we want to start with buffers full, because mplayer uses
> +	 * get_space() pointer as clock, which would cause video to
> +	 * accelerate while buffers are filled. Remove this when not
> +	 * necessary anymore.
> +	 */

Doesn't sndio support proper pause?
I also though we had some more generic code for this.
Sorry, don't have time for more right now.