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

[Reimar Döffinger]

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

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

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

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