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

[Reimar Döffinger]

On Mon, Dec 09, 2013 at 10:07:49AM +0100, Alexandre Ratchov wrote:
> > >>> +    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.

Sorry, but it is really frustrating when you spend time to make
sure things work properly, even mostly for the more limited ones
like SDL and then comes along an API where even that basic stuff
isn't possible.

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

Well, if I combine those two I get in my head:
sio_close frees all resources (i.e. also play buffers).
Play buffers will continue to be played after it returns.

I'd say obviously that can't be what it does (I sure hope so!!!).
So what does it do? Does it not actually free the resources?
Or does it not continue to play after returning?
Or does it wait until all buffers have been played and only
then return?
Also related: what happens if you SIGKILL an application that has
e.g. 10 seconds of audio buffered (or whatever the maximum you
can have buffered is)?

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

Honestly, I can't figure out why you call stop/start ever at all.
>From the documentation it sounds like the only thing they do is disable
the overrun detection. But since by default overrun doesn't do anything
the documentation makes me conclude these functions do absolutely
nothing so there is no point in using them at all, ever (except an
initial start I guess).

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

When MPlayer was completely broken for these things we only got a few
complaints per year. So if your users are all used to a rather bad
experience they might not even realize that it shouldn't be like that.
I'd have to test myself to see how bad it really is, I might easily miss
a few things.
For example audio_resume to me seems to behave as if the buffers were
discarded. But if that isn't the case, it will actually fill in too much
silence if the pause was less than the few 100 ms the buffers are large
(which can mostly happen when frame-stepping very fast).

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

Yes, that it can _neither_ pause _nor_ discard, that is rather unique,
in a bad way.
However the discard issue is the worse of the two.
For example a pure audio player ideally should decode a few seconds of
audio at a time and pass it on to the audio driver/hardware, to minimize
wakeups and save power (MPlayer isn't quite there yet).
>From what I can tell if you do that with this API, this means your users
can't even switch songs without waiting for seconds!
I would have said that this is quite obviously rather ridiculous...