Add ao_sndio as pull driver

[ericonr]

If we are striving for the least amount of code to maintain, I believe a pull driver is the way. It requires launching a thread inside the backend, but nothing too complicated.

This is an alternative for #8320 and #8314 . I'd be ok with closing #8320 in favor of this PR.

@rozhuk-im what do you think of this solution?

[ericonr]

Would it be okay to add a macro for 1000000LL in osdeps/time.h ?

[rozhuk-im]

It works.
No freezes on seeking.

But, if in #8314 I remove pause() and make reset() empty - I got same effect, and code is more compact.
Also, if no reset()/pause() then on pause it continue play from buffer.
Default sndio buffer in FreeBSD wrapper is 200ms.

[ericonr]

Also, if no reset()/pause() then on pause it continue play from buffer.

Yes, mpv feeds it silence. Currently, the jack backend does the same thing. I like the pull driver because it gives mpv the most information possible while at the same time simplifying pause.

[rozhuk-im]

I like the pull driver because it gives mpv the most information possible while at the same time simplifying pause.

Via ao_read_data() pull sndio backend report only about free buf space and some time. This is 2 params.
Via ```get_state() push sndio backend (#8314) - 4 parameters passed.

Current push sndio backend (#8314) has no pause() callback, what can be more simple? :)

[ericonr]

@rozhuk-im

Via ```get_state() push sndio backend (#8314) - 4 parameters passed.

Yes, but 4 parameters which aren't entirely necessary. The only information that mpv needs is latency, and if we can take care of the rest inside the driver, it's much simpler. And it's the "best quality" latency information that we can give, as well.

@noiseless could you try this one out as well?

[michaelforney]

Did some more testing and noticed an issue with parameter negotiation.

Also added a few style comments. Feel free to ignore the ones you disagree with.

[michaelforney]

You could use MP_ARRAY_SIZE(af_to_par) here.

Or better yet, how about this instead:

    sio_initpar(&p->par);
    p->par.bits = 16;
    p->par.sig = 1;
    p->par.le = SIO_LE_NATIVE;
    for (i = 0; i < MP_ARRAY_SIZE(af_to_par); i++) {
        ap = &af_to_par[i];
        if (ap->format == ao->format) {
            p->par.bits = ap->bits;
            p->par.sig = ap->sig;
            if (ap->bits > 8)
                p->par.le = SIO_LE_NATIVE;
            if (ap->bits != SIO_BPS(ap->bits))
                p->par.bps = ap->bits / 8;
            break;
        }
    }

I don't think the message about format conversion is necessary.

[ericonr]

That's a bit cleaner, but I think

            if (ap->bits > 8)
                p->par.le = SIO_LE_NATIVE;

is unnecessary? Since it's set to SIO_LE_NATIVE by default.

The

 if (ap->bits != SIO_BPS(ap->bits))
                p->par.bps = ap->bits / 8;

is weird to me as well, since it's comparing a number of bytes with a number of bits.

[ericonr]

That last block was in the original code as well, which is quite interesting.

[noiseless]

@ericonr

I'm sorry I missed your message. I will try to test it tonight (MSK).

[noiseless]

Built mpv with your patch (0903658). I didn't notice any problems, everything works as expected.
OpenBSD-current, mpv 0.33.0 + patch.

[ericonr]

@noiseless thank you very much for testing :)

I've updated the PR and it should now be more correct for channel mappings.

[michaelforney]

I took a stab at making it use non-blocking I/O here:
ericonr/mpv@sndio-pull...michaelforney:sndio-pull-nonblock

I think the only real advantage it has over your current version is that if our estimate of how much to write was wrong (sndio manual says "Note that sio_write() might block even if there is buffer space left"), it moves into a cancellable state rather than blocking in sio_write.

[ericonr]

I think the only real advantage it has over your current version is that if our estimate of how much to write was wrong (sndio manual says "Note that sio_write() might block even if there is buffer space left"), it moves into a cancellable state rather than blocking in sio_write.

I don't think it would block for long; we are writing at most device_buffer, and any errors there should make it exit on its own. And using a separate thread for driving sndio is mostly to allow us some freedom in blocking if necessary.

Being in a cancellable state more often could be useful if there are cases where bufsz is big (lots of latency) and quitting the application takes too long. The UX could then be improved (perhaps?) by not allowing a big write to happen. I'm unsure if your impl would allow for that.

[brad0]

Any update on this? There is no longer any sound support for OpenBSD with 0.33.

[brad0]

ping.

[brad0]

ping.

[brad0]

Any chance of seeing this commited?

[ericonr]

This is not the right way forward, because sndio's mixing depends on the amount of active sources. Pausing a video here will still make the daemon think the source is active, and therefore other sources will be a bit more muted. It's necessary to use sio_stop and sio_start to implement a well behaved sndio client.

[lanodan]

I don't know for others but I think it would be better to have a "good enough" audio backend in mpv that could get heavily modified later than the current situation where it's either no audio at all or having to use compat-layers like alsa-sndio which probably are doomed to be worse than an okay driver (for example the lack of volume integration).