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

[Alexandre Ratchov]

On Fri, Dec 06, 2013 at 09:10:32PM +0100, Reimar Döffinger wrote:
> > +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?

the call-back is simply invoked by sio_revents(), so neither
locking nor volatile qualifier are necessary.

> > +	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?

yes, any value of 'len' will work.

> And also without performance issues?

Sure, calling sio_write() for every byte will consume a lot of CPU,
but I guess play is called for larger chunks isn't it?

> I think most aos round to a good block size.
> 

Are you suggesting to return 0 if (len < blocksize). This would
imply that play() is allowed to consime less data than the 'len'
argument, would this be ok ?

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

reset doesn't discard samples. Once samples are submitted with
sio_write() they cannot be discarted. Here, reset() just tells the
device to drain and stop. Instead of continuing to play silence.

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

yeah, but there's no way to discard samples, still we're talking
about around 250ms of audio.

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

We did it this way because poll() is expencive, and at the time
this code was written get_delay() was called very often.

> > +/*
> > + * 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!

right :), this is probably what the function is supposed to do

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

reset() won't resume playback immediately. It puts the device in a
waiting state, where playback is triggered as soon there's enough
data buffered using sio_write(). So as long as play() is not called
the device wont restart.

> > +/*
> > + * 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?

I wasn't aware of this function, i'll try it and submit a new diff
with above suggestions.

thanks for the review.

-- Alexandre