ao_sndio: add this audio output again

[rozhuk-im]

Changes:

• rewrite to use new internal MPV API;
• code refactoring;
• fix buffers size calculations;
• buffer set to auto;
• reset() - clean/reinit device only after errors;

[rozhuk-im]

There is something wrong with channel mapping: 6 channels some times like mono.
2 channels - ok.

[ericonr]

I'm currently doing a bit of a different approach in https://github.com/ericonr/mpv/tree/sndio , and should probably PR it for discussion, then.

[rozhuk-im]

I do not use sndio so feel free to continue development :)

At this time there is 2 known for me issues:

1. Some times (on fast rewind, pause-play) it may freeze on full buf (I suspect), device reopen required to fix it (sndio specific). Workaround in reset() handle this, but it freeze on sio_stop() before it fail.
   May be sndio API improperly used or this is some bug in sndio wrapper on FreeBSD.

2. Something wrong in init code: looks like it map center to left while 6->2 conversion (mpv backend specific).
   I have no plans to fix it.

[ericonr]

Re. 1, I can definitely test it. I didn't do anything with fast rewind, only pause-play and seeking.

Re. 2, I will look into it.

I will open a PR with my version as an alternative to this one, hopefully the maintainers can take a look and decide what they prefer.

[rozhuk-im]

Re. 1, I can definitely test it. I didn't do anything with fast rewind, only pause-play and seeking.

Yes, I mean seeking, but with mouse wheel scrolling.

[ericonr]

Yes, I mean seeking, but with mouse wheel scrolling.

Has been working fine.

[noiseless]

Thank you for your work!
Your patch works fine for me (OpenBSD-current, amd64).

Some times (on fast rewind, pause-play) it may freeze on full buf (I suspect), device reopen required to fix it (sndio specific).

I couldn't reproduce it. Maybe I was doing something wrong.
I don't observe any audio problems after fast rewind and I don't observe any debugging log entries.

[rozhuk-im]

Possible that this is some problem with sndio wrapper on FreeBSD.

[avih]

I will open a PR with my version as an alternative to this one, hopefully the maintainers can take a look and decide what they prefer.

You two seems to know sndio better than current maintainers, and we clearly cannot take more than one version, so I think it would be best for everyone if you two @ericonr and @rozhuk-im could join forces and produce together only one version (it does look like both of you also comment on eachother's PR, so you're already halfway there? ;) ).

[rozhuk-im]

@avih Before this week even sndio wrapper was not installed on my system :)

IMHO, for now, better to merge this PR: it is good base to continue improve code and add features.

[ericonr]

Possible that this is some problem with sndio wrapper on FreeBSD.

I can ask around, but I don't think the issue with requiring a device reset (and possibly disconnect and reconnect) was ever spotted, since the previous code never did anything like that.

Does it make sense to add this code, since it's apparently (there could be people having this issue, they just didn't speak up) only for the FreeBSD version of sndio?

[michaelforney]

I think this needs to be p->par.bufsz. sndio won't start playing until bufsz is filled (see this recent clarification in the manual: https://marc.info/?l=openbsd-tech&m=160650041114362&w=2), and since appbufsz < bufsz, it never starts playing for me.

Or maybe, this should be left at appbufsz, and we should use p->par.bufsz instead of ao->device_buffer when calculating free_samples below.

[rozhuk-im]

With p->par.bufsz does not work: GUI/Video freezes, looks like write() block, but sound - ok.
As I understand appbufsz - buf size available for unblocked write, and it can be set. bufsz - R/O internal allocated memory.

[michaelforney]

I am testing your PR, but sound does not play for me, it just hangs at 0:00. This is because the buffer is never filled up to bufsz, so sndio never starts draining the buffer.

If I change line 301 (state->free_samples) to use p->par.bufsz, or if I change this line to use p->par.bufsz, the issue goes away.

[rozhuk-im]

p->par.appbufsz = ((p->par.rate * 80) / 1000); /* 80ms buffer. */ can you try increase it to 800?
ao->device_buffer = p->par.bufsz / 2; - work for me, is ~82ms, so looks like it > appbufsz.

[rozhuk-im]

If I set ao->device_buffer to p->par.appbufsz + p->par.round - 85,3ms still works.
I suspect that shdio wait to fill appbufsz and then starts, at least on OpenBSD, but FreeBSD wrapper starts after first sio_write().
ao->device_buffer = p->par.appbufsz + p->par.round; - probably will allow both sndio implementations work.

[michaelforney]

I've been doing some testing with the three sndio PRs (@rozhuk-im push 9e3ae91 and 0ef7db7, @ericonr push, @ericonr pull), and I think I'm starting to understand some of the issues here. I'm on Linux using sndio via alsa, both with sndiod (snd/*) and raw alsa (rsnd/*). It's possible that people testing on other operating systems (e.g. OpenBSD or FreeBSD) have slightly different issues.

1. Audio not starting

There was a recent clarification in the manual saying that we need to write up bufsz frames before playback starts: openbsd/src@b710ff0

In my testing, audio seems to start playing exactly when appbufsz samples are written, but these aren't reported via onmove until we start to write more samples (not sure about the exact number). This causes us to think the buffer is full and stop writing samples, resulting in a hang after the first block is played. If the start of the file is silent, this sounds like it just hung immediately. In all cases I tested, bufsz was sufficiently high to start playing audio and start getting move callbacks so that we could write more samples.

2. Hang on seek

I believe this is caused by

    /* Do actual reset only on errors. */
    if (p->playing)
        return;

During seek, mpv calls reset(), and then sets p->streaming = false. If the audio buffer happened to be full at this point, the subsequent ao_play_data returns early since there is no space available. This means that it does not restart the AO, leaving p->streaming = false. In playthread, it then sleeps until space is available, but since p->streaming = false, it uses a timeout of INFINITY, blocking forever.

I don't think the reset can be avoided like you are trying to do. @ericonr's push version does a full sio_stop(); sio_start() on reset, which solves the problem, but does cause tiny blips of silence during seek since the buffer is flushed and doesn't start playing again until it is full.

---

I think the main decision we need to make is whether to go with a push-based or pull-based design. With push-based, the root of the problem seems to be that mpv's push API expects us to be able to tell it how many samples can be written without blocking, which is at odds with sndio's semantics:

Note that sio_write() might block even if there is buffer space left; using the buffer usage to guess if sio_write() would block is false and leads to unreliable programs – consider using poll(2) for this.

However, it seems like it mostly works with device_buffer = p->par.bufsz and sio_stop(); sio_start() on reset.

With a pull-based design, there is less to worry about. @ericonr's current pull version still does blocking writes, but I think this only affects cancellation (and it could be made to use non-blocking I/O).

I am currently thinking the pull-based version (#8347) is the right way to go. My only concern is that sndio may be playing over the network, and since pause is emulated by silence, there will be continuous network activity when paused. Either way, it is probably a good idea to get some feedback from sndio developers to make sure we're on the right track.

@michaelforney can you test 9e3ae91 and 0ef7db7 ?

With 9e3ae91 the audio always starts, but I occasionally see hangs on seeks. It happens fairly often with rsnd, and sometimes it also hangs when resuming from pause. With snd, if I increase the sndiod buffer size I also get hangs on seeks. If I leave the sndiod buffer size at the default, I get the choppiness described by @noiseless. I think this is just because the buffer size (20ms) is too small.

With 0ef7db7 the audio does not start with sndiod, but does start with rsnd, and does not seem to hang when seeking or pausing.

[rozhuk-im]

With a pull-based design, there is less to worry about.

It just hides that writes some times blocked from mpv code.
Actual problem is that SNDIO implementations have different behavior.

I look into sndio-oss wrapper lib, I can make ideal mpv sndio backend for this, but IMHO this wrapper in some parts does not respect sndio API behavior that described in sndio manual.

[rozhuk-im]

With these changes:

• buffer size set to auto, this should fix "sound is not continuous", at least it was reproduced on FreeBSD+sndiod and fixed now
• return pause()
• buffer size = appbufsz
• a bit logic changed to call poll staff on play/getstate/pause (probably this will fix "when appbufsz samples are written, but these aren't reported via onmove until we start to write more samples")

Hope this will work in most sndio implementations.
These changes return back freezes on FreeBSD with sndiod running.

@noiseless , @michaelforney if you have time, please test.

[noiseless]

Now (35e8fa3) playback also starts immediately, but the sound is still not continuous. At the same time, I can say that the frequency of audio interruptions has become higher, and the pauses between fragments of the audio track are shorter, which makes the overall sound better (but still not as it should).

[ao/sndio] result: stereo
[ao/sndio] bufsz = 17280, appbufsz = 9600, round = 480
[ao/sndio] device buffer: 9600 samples.
[ao/sndio] using soft-buffer of 9600 samples.

[michaelforney]

It just hides that writes some times blocked from mpv code.
Actual problem is that SNDIO implementations have different behavior.

I don't quite understand what you mean here, could you try rephrasing?

The push API relies on free_samples to tell mpv when to write more audio data. This is explicitly discouraged in the mpv manual ("leads to unreliable programs"). The pull API version uses sio_poll to determine when to write more audio data, which is what is suggested in the manual.

@noiseless , @michaelforney if you have time, please test.

With the default sndiod buffer and block size (-b 7680 -z 960), I get choppiness during playback (seems to be constantly underrunning). I believe this is the same issue that @noiseless is hitting. With -b 7680 -z 3840, the audio plays one block and then stops (see my analysis of issue 1 in my earlier comment).

If I modify your PR with

-    ao->device_buffer = p->par.appbufsz;
+    ao->device_buffer = p->par.bufsz;

it seems to fix both problems, and works with sndiod and raw device access.

[ericonr]

https://sndio.org/tips.html#section_10_2 also mentions that using appbufsz instead of bufsz is wrong.

[noiseless]

With the patch from @michaelforney, I no longer see any problems with the sound. Everything works. Thanks!

[rozhuk-im]

Remove pause: reset + start do same.
Set: ao->device_buffer = p->par.bufsz.
Fix: reset does not set playing = false;
Force set appbufsz = 25ms on FreeBSD, without this some times sio_write() blocks and frames dropped.

Hope this fix issues from prev messages.

[noiseless]

Built mpv with 1ff2838.
Everything works as it should without any additional code modifications.

[noiseless]

Is there any hope that the code will be accepted into the main branch? I use mpv with this patch and everything works for me (OpenBSD current + mpv 0.33.0).

[phkrl]

Works fine with Artix Linux too (mpv 0.33.0).
The advantage of native sndio output over openal with sndio is significant when sndio is used to stream audio over network: when mpv is paused with sndio output, network traffic is stopped whereas openal output still streams data (even without playing audio actually)

[CounterPillow]

I think I found a bug

EDIT: Oh wait, left to right, so this isn't actually an issue. NEVERMIND

[michaelforney]

Carrying over some comments from @ericonr's closed pull request that also apply here.

[michaelforney]

We need to adjust the ao fields to match the negotiated parameters here. If the sndio device only supports certain channel counts and sample rates, it may not choose the preferred parameters and conversion is needed.

I noticed this when testing with a USB DAC that only supports 2 channels and certain sample rates when trying to play files with strange sample rates or that are mono or 5.1. It would stall or underrun because mpv and sndio end up using different parameters.

I think the following should be sufficient:

/* use negotiated parameters */
ao->samplerate = p->par.rate;
ao->channels = sndio_layouts[p->par.pchan];

ao->format is already taken care of below.

[michaelforney]

Most of mpv uses talloc for allocation.

    p->pfd = talloc_array_ptrtype(p, p->pfd, sio_nfds(p->hdl));

You don't have to worry about freeing it or checking for errors since it gets freed automatically when p does, and it aborts on failure.

[michaelforney]

Why MP_NUM_CHANNELS + 1 instead of just using the implicit array size (sndio_layouts[])?

[michaelforney]

This loop could be restructured a bit to be simpler:

    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;
            break;
        }
    }

[michaelforney]

As @ericonr pointed out in his PR, this condition doesn't make sense. It's comparing a number of bits to a number of bytes, so it is never false. I'm curious what the intention was in the original code.

Anyway, I think you can just remove it. The sndio docs say the default for bps is "the smallest power of two large enough to hold bits", which is exactly what we want.

[FrostKnight]

I should mention, that mplayer added something in one of their versions, as did smplayer to fix the video a/v problem... the desync issues in mp4 video/audio files.

i am not sure if smplayer and mplayer use the same solution, also, smplayer can use mpv instead of mplayer, and mp4's run flawlessly, thanks to a certain option, I mentioned it as a possible fix:
#9205
"A: add the synchronization option from smplayer under general and audio called:
Audio/Video auto synchronization"
I neglected to mention, that is in preferences, but you get the idea, I hope.

It would be nice if this fix could be directly implemented into mpv even if its an optional command which only happens when sndio is enabled.

Hyperbola is one of a few linux distros with sndio as their main audio.

I recall artix and devuan had sndio at one point, not sure if they still do.

[brad0]

I put this into our OpenBSD package for the last 3 weeks but there are some issues with the backend as is.

The feedback we received was...

"The loop function in mpv stopped working in the last two weeks.

If it reaches the end of a file it stops playing.

If I use the 'l' key to mark certain parts of a track, it stops playing at
the second marker."

and

"I tested mpv-0.3.11p1 and twitch.tv livestreams with youtube-dl have a
regression. The same 15 second ad plays at the beginning and then it is
paused indefinitely."

[noiseless]

"I tested mpv-0.3.11p1 and twitch.tv livestreams with youtube-dl have a
regression. The same 15 second ad plays at the beginning and then it is
paused indefinitely."

I'm not sure if this is a problem with mpv and/or sndio. I've never used twitch, but I checked youtube live streams - I couldn't reproduce the error.

I am using mpv 0.33.1 with this patch on OpenBSD-current and yt-dlp instead of youtube-dl.

A randomly selected livestream on youtube is simply played, after pressing "play/pause" button - too. The other two, also randomly selected, are also simply played. It seems to me that either more information is needed here about the way to reproduce the problem; or it should be looked for elsewhere, not in the mpv.

UPD: I checked the same streams from youtube-dl - the problem also does not reproduce. Probably need more information and/or examples.

[noiseless]

The loop function in mpv stopped working in the last two weeks.
If it reaches the end of a file it stops playing.
If I use the 'l' key to mark certain parts of a track, it stops playing at the second marker.

I can confirm it. The problem exists and is trivially reproduced.

[brad0]

I'm not sure if this is a problem with mpv and/or sndio. I've never used twitch, but I checked youtube live streams - I couldn't reproduce the error.

The problem goes away when just switching the audio backend.

[noiseless]

The problem goes away when just switching the audio backend.

I'm sorry, I missed a message in the mailing list with examples of problems. For the history:

run 1:
$ mpv https://www.twitch.tv/tastelesstv
 (+) Video --vid=1 (h264 1920x1080)
 (+) Audio --aid=1 (aac 2ch 44100Hz)
AO: [sndio] 44100Hz stereo 2ch s16
VO: [gpu] 1920x1080 yuv420p
AV: 00:00:13 / 00:00:14 (92%) A-V:  0.000 Cache: 0.5s/287KB
[ffmpeg/demuxer] mpegts: Packet corrupt (stream = 1, dts = 6748380).
AV: 00:00:14 / 04:07:12 (0%) A-V:  0.000 Cache: 14817s/3MB
Invalid audio PTS: 15.000091 -> 14828.363000
Reset playback due to audio timestamp reset.
(...) AV: 00:00:00 / 04:07:30 (0%) A-V:  0.000 Cache: 21s/16MB

run 2:
$ mpv https://www.twitch.tv/tastelesstv
 (+) Video --vid=1 (h264 1920x1080)
 (+) Audio --aid=1 (aac 2ch 44100Hz)
AO: [sndio] 44100Hz stereo 2ch s16
VO: [gpu] 1920x1080 yuv420p
AV: 00:00:14 / 04:09:15 (0%) A-V:  0.000 Cache: 14940s/1MB
Invalid audio PTS: 15.000091 -> 14953.440333
Reset playback due to audio timestamp reset.
[ffmpeg/video] h264: co located POCs unavailable
[ffmpeg/video] h264: co located POCs unavailable
(...) AV: 00:00:00 / 04:09:27 (0%) A-V:  0.000 Cache: 13s/10MB

(source)

I managed to reproduce the problem, however, with a different channel (the channel from the example is not available), but it does not matter.

[noiseless]

Apparently, I fixed the problem and returned the behavior of the sndio backend to what it was in mpv 0.32. If I don't find any new problems, I will publish a patch and/or pull request later tonight (MSK).

[noiseless]

Patches that fix bugs with loop/twitch:

1. 57d7e44 (it's not a fact that this patch is necessary, but it's somewhat easier and more convenient for me with it)
2. 260bbe6