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

[Reimar Döffinger]

Hello,
This should be added to the documentation, too.

On Fri, Dec 06, 2013 at 12:40:20AM -0500, Brad Smith wrote:

> +if test "$_sndio" = yes ; then                                                                                                                                                                                                                 

Lots of trailing whitespace, also in the .c file.
If I remember right, it is not even possible to commit
before this is fixed.
To be consistent with FFmpeg, 4 spaces indentation would
be somewhat preferable over tabs, too.

> +static ao_info_t info = {
> +	"sndio audio output",

Maybe it would be good to mention something more, like that it's related
to (Open)BSD?

> +	struct af_to_par {
> +		int format, bits, sig, le;
> +	} af_to_par[] = {
> +		{AF_FORMAT_U8,	    8, 0, 0},
> +		{AF_FORMAT_S8,      8, 1, 0},
> +		{AF_FORMAT_U16_LE, 16, 0, 1},
> +		{AF_FORMAT_U16_BE, 16, 0, 0},
> +		{AF_FORMAT_S16_LE, 16, 1, 1},
> +		{AF_FORMAT_S16_BE, 16, 1, 0},
> +		{AF_FORMAT_U24_LE, 16, 0, 1},
> +		{AF_FORMAT_U24_BE, 24, 0, 0},
> +		{AF_FORMAT_S24_LE, 24, 1, 1},
> +		{AF_FORMAT_S24_BE, 24, 1, 0},
> +		{AF_FORMAT_U32_LE, 32, 0, 1},
> +		{AF_FORMAT_U32_BE, 32, 0, 0},
> +		{AF_FORMAT_S32_LE, 32, 1, 1},
> +		{AF_FORMAT_S32_BE, 32, 1, 0}
> +	}, *p;

This should be "static const".
However I suspect you shouldn't need it at all.
Look into (found in use in other places):
af_fmt2bits
(fmt & AF_FORMAT_SIGN_MASK) == AF_FORMAT_US
(fmt & AF_FORMAT_END_MASK) == AF_FORMAT_LE

> +	if (par.bits == 8 && par.bps == 1) {
> +		format = par.sig ? AF_FORMAT_S8 : AF_FORMAT_U8;	
> +	} else if (par.bits == 16 && par.bps == 2) {
> +		format = par.sig ? 
> +		    (par.le ? AF_FORMAT_S16_LE : AF_FORMAT_S16_BE) :
> +		    (par.le ? AF_FORMAT_U16_LE : AF_FORMAT_U16_BE);
> +	} else if ((par.bits == 24 || par.msb) && par.bps == 3) {
> +		format = par.sig ? 
> +		    (par.le ? AF_FORMAT_S24_LE : AF_FORMAT_S24_BE) :
> +		    (par.le ? AF_FORMAT_U24_LE : AF_FORMAT_U24_BE);
> +	} else if ((par.bits == 32 || par.msb) && par.bps == 4) {
> +		format = par.sig ? 
> +		    (par.le ? AF_FORMAT_S32_LE : AF_FORMAT_S32_BE) :
> +		    (par.le ? AF_FORMAT_U32_LE : AF_FORMAT_U32_BE);

Could either be built using the reverse of the functionality above,
or by searching through a list of formats you want to allow.


> +	pfds = malloc(sizeof(struct pollfd) * sio_nfds(hdl));

I'd suggest calloc instead.
Also sizeof(*pfds) is preferred over sizeof(struct pollfd) in MPlayer.

> +bad_free:
> +	free(pfds);
> +bad_close:
> +	sio_close(hdl);
> +	hdl = 0;
> +	return 0;

One label is enough if you make sure pfds is initialized to NULL.

> +static int get_space(void)
> +{
> +	int bufused, revents, n;
> +
> +	/*
> +	 * call poll() and sio_revents(), so the
> +	 * delay counter is updated
> +	 */
> +	n = sio_pollfd(hdl, pfds, POLLOUT);
> +	while (poll(pfds, n, 0) < 0 && errno == EINTR)
> +		; /* nothing */
> +	revents = sio_revents(hdl, pfds);
> +	return par.bufsz * par.pchan * par.bps - delay;

That has a bit of a smell of black magic.
I guess there is no easier/more obvious way?
Also, how do the callbacks work, would it be safer to
mark delay "volatile"? Or does sndio make sure everything
is done correctly?

> +	int n;
> +
> +	n = sio_write(hdl, data, len);

Are you sure sndio will support if if you e,g.
write 1 byte but the sample format is 6 channel U32
or anything strange like that?
And also without performance issues?
I think most aos round to a good block size.

> +	delay += n;
> +	if (flags & AOPLAY_FINAL_CHUNK)
> +		reset();

You definitely shouldn't do that! You explicitly throw away all
the last data, possibly multiple seconds of audio! (Well,
with the buffer size you have probably 0.5s max, but that is bad
enough).
Also, in the uninit function you do not use the "immediate" argument,
that can't be right.
Depending on its value you should either let the ao finish playing the
audio or stop directly.

> +static float get_delay(void)
> +{
> +	return (float)delay / (par.bps * par.pchan * par.rate);

Implementations where get_delay and get_space do things significantly
differently are usually broken.
If get_space needs to do all those special things to get correct values,
get_delay should do that as well or you will get bad A-V sync.

> +/*
> + * stop playing, keep buffers (for pause)
> + */
> +static void audio_pause(void)
> +{
> +	reset();

I don't think those generic comments have much of a value in
general, but you should at least adjust them when the code does
the exact opposite of what the comment says!
Also reading the documentation I don't think it is correct.
On pause you should only do stop, the resume should only happen
once there is data again, otherwise sndio might misunderstand this
as a buffer underrun case.

> +/*
> + * resume playing, after audio_pause()
> + */
> +static void audio_resume(void)
> +{
> +	int n, count, todo;
> +
> +	/*
> +	 * 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.
> +	 */
> +	todo = par.bufsz * par.pchan * par.bps;
> +	while (todo > 0) {
> +		count = todo;
> +		if (count > SILENCE_NMAX)
> +			count = SILENCE_NMAX;
> +		n = sio_write(hdl, silence, count);
> +		if (n == 0)
> +			break;
> +		todo -= n;
> +		delay += n;
> +	}

We have mp_ao_resume_refill, why not use that?