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

[Alexandre Ratchov]

On Mon, Dec 09, 2013 at 09:37:52AM +0100, Reimar Döffinger wrote:
> On 09.12.2013, at 08:02, Alexandre Ratchov <alex at caoua.org> wrote:
> > On Fri, Dec 06, 2013 at 09:10:32PM +0100, Reimar Döffinger wrote:
> >> 
> >>> +    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?
> 
> There is a default block size it tries to use, so if it works it
> should be ok as it is.
> 
> >>> +    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.
> 
> Wow, someone managed to create an API more useless than OSS, that
> is quite a feat.

thanks.

> Also, the documentation contradicts itself, it says the close
> function acts like stop _and_ that it frees resources like
> buffers, but the stop documentation says it continues playing
> buffers. If you take it literally, the documentation says it
> first frees the buffers and then continues playing...

Searching for "sio_stop" in the man page gives:

"The sio_stop() function stops playback and recording and puts
the audio subsystem in the same state as after sio_open() is called.
Samples in the play buffers are not discarded, and will continue to be
played after sio_stop() returns."

and then:

"The sio_close() function closes the stream and frees all
allocated resources associated with the libsndio handle.  If the stream
is not stopped it will be stopped first as if sio_stop() is
called."

Let me know what's unclear so we can improve it.

> > Here, reset() just tells the
> > device to drain and stop. Instead of continuing to play silence.
> 
> What's the point? The device should be closed shortly after anyway...

If so, we can drop the call to reset(), right?

> >> 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.
> 
> We're talking about an A-V desync of 250ms every time someone
> seeks or switches to a different file. That's not just bad,
> that's ridiculous. I suspect not much good will happen when
> frame-stepping through a file either.

Well, it works as is, I don't see any A-V sync problems, and nobody
complained about A-V sync problems last 5 years.

> >>> +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.
> 
> Yes, it is called very often, But it _needs_ to be called often
> to get good A-V sync.
> If calling poll is too expensive you need to at least measure how
> much time passed and estimate the change in delay, I think the
> SDL ao has such code.

thanks for the pointer.

> The reset function not actually resetting is rather confusing,
> but if the API really can do only one thing there's not much that
> can be done about it. However the API unfortunately seems
> completely unsuitable for a media player.

Are you talking about the lack of a function to discard buffered
data?

-- Alexandre