Another attempt to return ao_sndio to mpv

[noiseless]

This pull request is completely based on #8314 and is its continuation. In fact, this is the @rozhuk-im patch with fixes for the bugs described here by @brad0 and with improvements described here by @michaelforney . Minor cosmetic changes are also present.

The correct operation of ao_sndio from the patch was tested on OpenBSD current (not on the most current snapshot, though; I don't think it matters in principle).

[Dudemanguy]

I personally think you should just squash all of your commits together into one and note that they fix issues/problems with the original implementation.

[noiseless]

OK, thanks. I still separated the cosmetic changes into a separate commit. If it is not OK, I don't mind squashing everything in one commit, but IMHO combining code changes and cosmetic changes in one patch degrades readability.

[Dudemanguy]

To be quite frank, the commit about updating the commits could be dropped entirely imo aside from, perhaps, the copyright line (that could be just included with the bugfix/refactoring commit). Nevertheless, I do not know anything about the audio API so the following review is strictly just based on style.

[noiseless]

Apologies for requesting a review - I was actually trying to figure out why the status is still "Changes requested"..

Mistake

[noiseless]

I may have found some problems with the operation of my patch. I am investigating this issue. Perhaps there is no point in testing my patch now, if someone has such a desire.

I will be glad to hear comments about the code style or errors in it in any case.

[brad0]

I may have found some problems with the operation of my patch. I am investigating this issue. Perhaps there is no point in testing my patch now, if someone has such a desire.

I will be glad to hear comments about the code style or errors in it in any case.

How did you find these issues? Did you manage to do any testing on FreeBSD?

[noiseless]

How did you find these issues?

I discovered the problem while watching some youtube videos using mpv with yt-dlp. It consists in the fact that there are warnings of the form "Audio device underrun detected", which is accompanied by a fading of playback for a fraction of a second.

The problem does not occur with all videos and not always.
At the same time, at the moments when the problem happens, it happens precisely because of the logic I added to get_state() (I checked it).

The reasons for the error are not yet clear to me.

Did you manage to do any testing on FreeBSD?

I haven't been able to test anything on FreeBSD yet, because I don't have FreeBSD installed right now, most likely I will do this closer to the weekend.

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

Perhaps an interesting fact: this problem does not reproduce if you run mpv with "--no-video".
(no, of course I'm not suggesting this as a solution)

[ratchov]

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.

Perhaps an interesting fact: this problem does not reproduce if you run mpv with "--no-video". (no, of course I'm not suggesting this as a solution)

I managed to reproduce stuttering and the "Audio device underrun detected" messages on OpenBSD by running mpv on a very slow machine with a tiny sndiod block size (-z option). Removing the underrun checks in get_state() fixes the sound.

[FrostKnight]

I found out that there is an option in mpv that makes sndio work 100% even with mp4 files without any desync issues!

I wrote about it in another post and this is it:
mpv --audio-device=sndio yourvideomediafile.mp4

Can anyone using OpenBSD, or any BSD with sndio try this? It might work! :)

It certainly did for me! :)

I swear, desyncing doesn't happen when I add that above variable to mpv.

So yeah, sndio does in fact work even for videos with that. AFAIK.

[noiseless]

running mpv on a very slow machine with a tiny sndiod block size (-z option).

I am running sndiod without any options (-> block size has a default value), however yes, I now believe it is a performance related issue.

I accidentally had hw.setperf=0 sysctl set, as a result of which the clock frequency of my A10-5750M is reduced to 1400MHz and its performance after it is clearly insufficient for reliable multimedia processing.

After setting hw.setperf=100, I could not reproduce the problem with "Audio device underrun detected".

Without my edits in ao_sndio.c, I was getting the error

Audio/Video desynchronisation detected! Possible reasons include too slow
hardware, temporary CPU spikes, broken drivers, and broken files. Audio
position will not match to the video (see A-V status field).

instead of this one.

UPD: No, unfortunately, it has now been reproduced with hw.setperf=100. I will investigate the problem further..

[noiseless]

So that it doesn't look like nothing is happening here at all: I'm not ready to offer a final solution to this problem yet, and I'm continuing to investigate it.

Between versions 0.32.* and 0.33.*, processing of underruns (and not only) was removed from the mpv audio subsystem. Now the processing of such situations should be done directly in ao (as far as I understand).

While I'm experimenting, reading the code and trying to work out the least disgusting solution. If it exists at all and/or is needed: probably, if you run sndiod with a smaller block size or try to force mpv to use rsnd directly, the problem will not be observed (I will check this in the future).

But I am haunted by the fact that when using mpv 0.32 and mpv 0.33 with the original patch from @rozhuk-im on the same equipment, I have never observed such problems.

[ratchov]

So that it doesn't look like nothing is happening here at all: I'm not ready to offer a final solution to this problem yet, and I'm continuing to investigate it.

Between versions 0.32.* and 0.33.*, processing of underruns (and not only) was removed from the mpv audio subsystem. Now the processing of such situations should be done directly in ao (as far as I understand).

That's fine as it's is in line with sndio design: underruns are handled in lower layers and transparent, so the ao_sndio.c layer doesn't even have a mean to detect them reliably.

But I am haunted by the fact that when using mpv 0.32 and mpv 0.33 with the original patch from @rozhuk-im on the same equipment, I have never observed such problems.

BTW, do you observe the problem if you're using --audio-device=sdl ?

[noiseless]

That's fine as it's is in line with sndio design: underruns are handled in lower layers and transparent, so the ao_sndio.c layer doesn't even have a mean to detect them reliably.

Yes, thank you, I know that. For some reason I didn't finish the thought when I wrote the last message. Problems with a-b loop and twitch streams arise due to the fact that ao_sndio does not inform the higher layer that EOF has been reached (here) and it does not call reset(). To do this, I set p->playing to false, as recommended in internal.h.

BTW, do you observe the problem if you're using --audio-device=sdl ?

With ao=sdl I didn't check at all. It's difficult for me to use it, because as far as I can observe with it, the audio lags behind the video by a few milliseconds, and after moving forward/backward through the video - apparently, sometimes for a longer time. I am building an mpv with ao_openal enabled and using it for comparison.

It seemed to me that everything was OK with ao=openal, but now that I decided to double-check it, I seem to have managed to reproduce the problem with ao=openal as well (for this I needed to create a significant excess load on the system).

Apparently, this is really a performance-related problem. I probably need to soften the conditions of my check (so that small xruns do not lead to an immediate reset()) and this will be a good enough solution.

I will do it soon. Thanks for the idea!

[ratchov]

Thank you @noiseless for looking at this. Last few days, I used your branch a lot (OpenBSD + above mentioned changes) and sound is reliable. The underruns detected by mpv that @noiseless mentioned don't seem to be related to sndio. Are there any other pending problems?

[noiseless]

The underruns detected by mpv that @noiseless mentioned don't seem to be related to sndio.

Yes, these "underruns" are not a consequence of problems with sndio, but a consequence of the false-positive triggering of my checks.

In any case, these were false and redundant checks triggering, so I complicated the logic of processing such events: now we inform the higher level about the probable EOF/underrun only if the check worked 5 times in a row; the conditions of the check itself have also softened.

I have been actively using this code for the last two days and have not experienced any problems, including redundant "Audio device underrun detected" messages (now they only appear when the system is highly overloaded, that is, as with the same as with ao_openal).

I think that if we are talking about OpenBSD, then this code is ready for use/testing.

For FreeBSD, I was also going to check why the workaround was needed and, perhaps, bring this logic back.

[ratchov]

If there is not enough CPU for mpv, it will simply miss the deadline and audio/video will pause during few blocks. Then (once there's enough CPU again) audio/video will resume in sync. It happens as if ^Z was pressed and then the process resumed. We can't do better if there's not enough CPU. Simple problem, simple solution.

IMHO trying to handle underruns defeats the whole purpose of sndio: keeping the code/design simple. If a reset is needed, then there's something we're missing or maybe a bug to fix somewhere else. I'll help with it if I manage to reproduce it. Hints to reproduce it are welcome.

[noiseless]

In my recent edits, I'm just trying to avoid having to report underruns. Initially, I added my checks to the code to inform mpv that audio playback was completed, since otherwise ao continued to play silence indefinitely.

However, later I discovered that my code also leads to redundant messages "Audio device underrun detected", and this behavior is erroneous - with other ao's, such messages appear when the system is significantly more heavily loaded.

In my recent edits, I try not to report false underruns and report EOF. The fact that my code generally causes messages about underruns is a side effect and a consequence of the fact that mpv does not know how to distinguish EOF from underrun.

In a good way, I need some kind of api to understand that EOF has come, there is no such api in mpv now, so I have to guess in such a dubious way.
I'm not trying to process xrun's instead of sndio, just the opposite.

[noiseless]

Since, perhaps, there has already been some confusion:

• This pull request is based on ao_sndio: add this audio output again #8314 and is an attempt to fix the problems described here.
• The problems I am referring to arise due to the fact that ao_sndio does not enter the playing=false state at the moment when EOF is reached.
• To fix this, I added checks that were designed to report EOF, which led to calling the required reset().
• However, after a while I found that the playback began to freeze, this was accompanied by the errors "Audio device underrun detected". These errors were caused by the logic I added earlier.
• My last commit is an attempt to report only EOF and not report false underruns.

(If the system is overloaded, such messages can still occur, apparently, this should no longer be considered a problem.)

[noiseless]

If a reset is needed, then there's something we're missing or maybe a bug to fix somewhere else. I'll help with it if I manage to reproduce it. Hints to reproduce it are welcome.

I have added logic that can eventually lead to messages about "underrun" to report that playback is complete. I am sure that in all other cases ao_sndio should not affect the logic of mpv operation, including in the case of real underruns.

Unfortunately, we do not have the ability to make checks similar to these:

• https://github.com/mpv-player/mpv/blob/master/audio/out/ao_alsa.c#L1013
• https://github.com/mpv-player/mpv/blob/master/audio/out/ao_openal.c#L368
• https://github.com/mpv-player/mpv/blob/master/audio/out/ao_oss.c#L362

due to the fact that sndio does not have a "paused/stopped" state (or any other state that could be checked). Mpv 0.33.* also does not have the ability to perform a similar check (like this).

Theoretically, I could go to layer violation and check the playback status the same way audioctl does (that is, bypassing sndiod; like this), however, at times when "a-b loop" or twitch streams hang, audioctl shows an increase in play.bytes and a stable zero value of play.errors, that is, such an option would not help me anyway.

Of course, I do not consider the method I am currently using to solve problems with "a-b loop" and streams to be good and/or correct. But I don't have any good ideas yet on how to do it better.

Perhaps we somehow incorrectly calculate free/queued samples and/or delay and this leads to the fact that playback does not stop when it should (in the last call to audio_write before the infinite loop in get_state, you can see that samples!=0).

But, if this is the case, then it is still unclear to me exactly where the error may be.

The only advantages of my solution are that it works, corrects errors with "a-b loop" and streams on twitch, and also does not spam with "Audio device underrun detected" messages in console, except in situations when the system is heavily overloaded. In other aspects, this is a dubious workaround, alas.

If someone has an understanding of how to properly handle the hang of the "a-b loop" and twitch streams, I would be grateful for the advice.

[ratchov]

Could you provide step-by-step instructions on how to reproduce the playback freezes? I start suspecting a sndio bug, so I'd like to manage to reproduce the freezes and understand what's going on.

The need for handling underruns suggests something is wrong outside the ao module.

[noiseless]

@Dudemanguy, sorry to address you directly. I hope I'm not interrupting too much.

Just wanted to clarify, are there any other issues with my patch that are preventing it from being included in the master?

I checked the current versions of mpv with this patch work under OpenBSD, Linux, and FreeBSD. Everything works correctly.

Yes, there is probably much more in this patch that could be improved (for example, I read the mailing list messages from @sizeofvoid about the inconvenience while seeking video; the details, however, I do not know yet), but I think we can do that after the code is in the main branch.

Other ao, after all, are not perfect either.

In ao_sdl the sound lags behind the video (another confirmation of the problem).
In ao_oss loop function does not work, in the same way as it was broken in ao_sndio before my fixes.

There are definitely no comparable problems in ao_sndio right now.

[Dudemanguy]

I'm not qualified to deeply review audio code, but since there is allegedly not any functional issue with the patch maybe someone else will review it. The PR is missing meson support though. It would be nice to add that.

[noiseless]

The PR is missing meson support though. It would be nice to add that.

I'll do it soon, thank you.

[ratchov]

On Sun, Dec 26, 2021 at 09:36:21AM -0800, Andrew Krasavin wrote: @Dudemanguy, sorry to address you directly. I hope I'm not interrupting too much. Just wanted to clarify, are there any other issues with my patch that are preventing it from being included in the master?

As far as I know there are no issues. As your diff is in -current, people will start using it and will report any problems. Just wait 1-2 weeks until people upgrade and shake it. Some would say controls are sluggish, but this is not related to your diff or to mpv, that will be handled at system level.

I checked the current versions of mpv with this patch work under OpenBSD, Linux, and FreeBSD. Everything works correctly. Yes, there is probably much more in this patch that could be improved (for example, I read the mailing list messages from @sizeofvoid about the inconvenience while seeking video; the details, however, I do not know yet), but I think we can do that after the code is in the main branch. Other ao, after all, are not perfect either. In ao_sdl the sound lags behind the video (another confirmation of the problem ). In ao_oss loop function does not work, in the same way as it was broken in ao_sndio before my fixes. There are definitely no comparable problems in ao_sndio right now.

I'd say this is a big step forward if problems pop-up, they will be quickly fixed.

[noiseless]

As far as I know there are no issues. As your diff is in -current,
people will start using it and will report any problems. Just wait 1-2
weeks until people upgrade and shake it.

I agree that it takes time for something to be confirmed. But it's been almost two weeks now.

In any case, I have been asked to provide meson support. I will definitely do that, however, I will finish some other work first.
In the proverbial 14 days I can probably get it done.

Some would say controls are sluggish, but this is not related to your
diff or to mpv, that will be handled at system level.

I'll keep that in mind, thank you. Unfortunately, the only thing I know about this problem is that it seems to exist, that's all.

I'd say this is a big step forward if problems pop-up, they will be
quickly fixed.

I'm not trying to say that there is any "step forward" at all.
My point was that even if there are bugs in the current implementation of ao_sndio (and there probably are), they can be fixed after the merge of the patch into the trunk. The bugs in the other ao I referenced are examples of the fact that if a bug is found, ao_sndio will not be the only imperfect ao in the repository.

No, I wasn't trying to say something like "look how good my ao_sndio is and how bad ao_oss and ao_sdl are" (although ao_sdl, does have a serious problem, in my opinion).

I don't consider myself well qualified on the subject or anything like that. No.
I made a fix for ao_sndio by @rozhuk-im for the only reason: I've been using OpenBSD and mpv for too long and am too used to them. It's easier to find a way to fix the problem than to change the player or the OS.
I haven't found anyone else willing to fix it for some time, so I decided to try it myself.
There is no other reason.

As for fixing bugs quickly, if someone finds them and reports them to me - I can't promise it will be fast, but I will definitely do my best to figure out the problem and fix it.
And yes, I am open to criticism. And if someone more qualified decides to replace me, that's fine with me too.So far, no one has informed me of this intention, so I'm doing what I can and what I think is right.

[noiseless]

The PR is missing meson support though. It would be nice to add that.

@Dudemanguy, done. Tested on linux, I will check on openbsd in the evening.

[noiseless]

Small nit but could you alphabetize this (put it after sdl2_audio)?

Oh, sorry, that's what I originally intended to do, and I can't answer why I forgot about it. Anyway - fixed.
Thanks for the remark.

I will check on openbsd in the evening

Done, build is successful.

[rozhuk-im]

But maybe someone will read this and want to try to figure out the problem? (@rozhuk-im - maybe it will be interesting to you?).

Yes, I will try to fix OSS, thanks for pointing.

[Dudemanguy]

@noiseless: Could you squash your 3 commits together?

[noiseless]

@noiseless: Could you squash your 3 commits together?

Done.

[Dudemanguy]

I'm not really qualified to review this since I don't know the mpv audio API very well but the code looks fine to me at least.

[noiseless]

I'm not really qualified to review this since I don't know the mpv audio API very well but the code looks fine to me at least.

Thank you!
I don't mind if someone else from mpv looks at my code before you decide to accept/not to accept it into the main branch.

Perhaps you can recommend someone to me for this purpose?

[Dudemanguy]

The code seems fine and this works OK for me with a little bit of testing. I'll let this sit for a while if anyone else wants to comment on it but I think it's fine to merge.

[Dudemanguy]

Oops sorry, I just realize this has a merge conflict with the master branch (probably because of pipewire being merged). Could you rebase?

[noiseless]

Could you rebase?

Done, thanks.

[Dudemanguy]

Thanks. I'll let this sit for a bit.

[Dudemanguy]

Thanks for everyone's hard work on this.

[noiseless]

Thanks for everyone's hard work on this.

Thank you very much!

@Dudemanguy @brad0 @ratchov @rozhuk-im - once again I want to say thank you all very much - without you and your help it would have been immeasurably more difficult for me.