[MPlayer-dev-eng] [PATCH] Add sndio input support

[Reimar Döffinger]

On Wed, Mar 12, 2014 at 12:35:30AM -0400, Brad Smith wrote:
> +    par.appbufsz = ai->req_samplerate;	/* 1 sec */
> +
> +   if (!sio_setpar(ai->sndio.hdl, &par) || !sio_getpar(ai->sndio.hdl, &par)) {
> +	mp_msg(MSGT_TV, MSGL_ERR, "could not configure sndio audio");
> +	return -1;
> +    }

Broken indentation, and no tabs would be preferable.

> +    if ((ai->sndio.hdl = sio_open(ai->sndio.device, SIO_REC, 0)) == NULL) {

I really dislike packing everything into the if.
IMHO it only hurts readability, introduces a significant risk of bugs
(just check how often people forgot the () around the assignment in
FFmpeg) and only saves a single line of code.

> -#ifdef CONFIG_SUN_AUDIO
> +#if defined(CONFIG_SUN_AUDIO) && !defined(CONFIG_SNDIO_AUDIO)

Why these kind of changes? Why can they not co-exist?

> +#if defined(CONFIG_SNDIO_AUDIO)

For consistency with existing code I think it would be slightly
nicer to use #ifdef where possible.

> @@ -643,12 +695,76 @@
>  priv->dspsamplesize = 16;
>  priv->dspstereo = 1;
>  priv->dspspeed = 44100;
> +#if !defined(CONFIG_SNDIO_AUDIO)
>  priv->dspfmt = AFMT_S16_LE;
> +#endif

While it would be unused, just leaving that variable in the struct
anyway would avoid the need for this ifdef for example.

> +//priv->par.round = FRAGSIZE / (priv->dspsamplesize/8 * (priv->dspstereo + 1));
> +priv->par.round = priv->dspspeed / 100; /* 10 ms */

Will everything work fine if this is not a multiple of something like "sample size *
channels"?

> +if(priv->dspready == TRUE)

I can't see this TRUE/FALSE used anywhere else in the file.
I think it should just use standard C 0/1 etc.

> -#ifdef CONFIG_SUN_AUDIO
> +#if defined(CONFIG_SNDIO_AUDIO)
> +bytesavail = priv->realpos - priv->dspbytesread;
> +if(bytesavail > priv->appbufsz)
> +    bytesavail = priv->appbufsz;
> +if(bytesavail < priv->round)
> +    bytesavail = priv->round;

This looks like it should use e.g. av_clip (from libavutil).