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

[Brad Smith]

On 06/12/13 3:10 PM, Reimar Döffinger wrote:
> Hello,
> This should be added to the documentation, too.

Ok.

> 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.

Ok, this is a BSD style of code with regard to tabs vs spaces. I
will pass on your feedback to Alexandre to see about having fixes
and such incorporated.

Just FYI this is the same backend I was attempting to submit here

http://marc.info/?l=mplayer-dev-eng&w=2&r=1&s=sndio&q=b

I am not the author but I am someone who helps to maintain the OpenBSD
MPlayer port/package. The intention is to have the backend go through
the feedback / fixes / submit for further review loop until it is in
good enough shape to be commited and so it is not sitting in our ports
tree. This is the only thing we have in our ports tree of any
significance for MPlayer.

>> +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?
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.