Skip to content

Commit

Permalink
ao_sndio: remove this audio output
Browse files Browse the repository at this point in the history 
It was always marked as "experimental", and had inherent problems that
were never fixed. It was disabled by default, and I don't think anyone
is using it.
  • Loading branch information
wm4 committed Mar 28, 2020
1 parent 27b4bdf commit 71d218e
Show file tree
Hide file tree
Showing 5 changed files with 0 additions and 338 deletions.
8 changes: 0 additions & 8 deletions DOCS/man/ao.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,5 @@ Available audio output drivers are:
confused with RoarAudio, which is something completely
different.

``sndio``
Audio output to the OpenBSD sndio sound system

.. note:: Experimental. There are known bugs and issues.

(Note: only supports mono, stereo, 4.0, 5.1 and 7.1 channel
layouts.)

``wasapi``
Audio output to the Windows Audio Session API.
4 changes: 0 additions & 4 deletions audio/out/ao.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ extern const struct ao_driver audio_out_audiounit;
extern const struct ao_driver audio_out_coreaudio;
extern const struct ao_driver audio_out_coreaudio_exclusive;
extern const struct ao_driver audio_out_rsound;
extern const struct ao_driver audio_out_sndio;
extern const struct ao_driver audio_out_pulse;
extern const struct ao_driver audio_out_jack;
extern const struct ao_driver audio_out_openal;
Expand Down Expand Up @@ -88,9 +87,6 @@ static const struct ao_driver * const audio_out_drivers[] = {
#endif
#if HAVE_SDL2_AUDIO
&audio_out_sdl,
#endif
#if HAVE_SNDIO
&audio_out_sndio,
#endif
&audio_out_null,
#if HAVE_COREAUDIO
Expand Down
319 changes: 0 additions & 319 deletions audio/out/ao_sndio.c
View file

This file was deleted.

6 changes: 0 additions & 6 deletions wscript
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,12 +448,6 @@ audio_output_features = [
'name': '--rsound',
'desc': 'RSound audio output',
'func': check_statement('rsound.h', 'rsd_init(NULL)', lib='rsound')
}, {
'name': '--sndio',
'desc': 'sndio audio input/output',
'func': check_statement('sndio.h',
'struct sio_par par; sio_initpar(&par); const char *s = SIO_DEVANY', lib='sndio'),
'default': 'disable'
}, {
'name': '--pulse',
'desc': 'PulseAudio audio output',
Expand Down
1 change: 0 additions & 1 deletion wscript_build.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ def swift(task):
( "audio/out/ao_pulse.c", "pulse" ),
( "audio/out/ao_rsound.c", "rsound" ),
( "audio/out/ao_sdl.c", "sdl2-audio" ),
( "audio/out/ao_sndio.c", "sndio" ),
( "audio/out/ao_wasapi.c", "wasapi" ),
( "audio/out/ao_wasapi_changenotify.c", "wasapi" ),
( "audio/out/ao_wasapi_utils.c", "wasapi" ),
Expand Down

29 comments on commit 71d218e

@brad0
Copy link
Contributor

@brad0 brad0 commented on 71d218e Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this very much is used. This is our native sound API for OpenBSD.

@ratchov
Copy link
Contributor

@ratchov ratchov commented on 71d218e Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still used on OpenBSD systems. Could we fix the sndio backend bugs rather than disabling it?

@jeeb
Copy link
Member

@jeeb jeeb commented on 71d218e Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please make sure of that. As noted, it's disabled by default by how problematic it used to be.
  2. If you still feel like you require it, people are very welcome to start maintaining it if they know enough of that system and utilize it daily.

@brad0
Copy link
Contributor

@brad0 brad0 commented on 71d218e Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether it is disabled by default or not doesn't change the fact we have been using the backend for all of these years anyway.

@ratchov
Copy link
Contributor

@ratchov ratchov commented on 71d218e Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to help fixing the bug that makes it problematic. The only issue I'm aware of is the one described by the warning message in the source code "Blocking until remaining audio is played... (sndio design bug).\n".

Is there anything else? Any pointers to related discussions are welcome.

@ratchov
Copy link
Contributor

@ratchov ratchov commented on 71d218e Apr 6, 2020 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ratchov
Copy link
Contributor

@ratchov ratchov commented on 71d218e Apr 6, 2020 via email •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropping sndio will make mpv mostly unusable on openbsd. The extra latency introduced by stuff like pulseaudio will make this useless on most machines.

I don't know about bugs in sndio support, I've been using mpv on my openbsd machines for years by now, yes, with sndio support, and it just works. There are zero patches related to that, the only thing we do is enable it in the build.

Could you point us to a list of bugs to fix ?...

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, if it's only because it doesn't support all possible sound format outputs nothing on OpenBSD supports the extra formats. That's pretty much another issue entirely.

@sthen
Copy link

@sthen sthen commented on 71d218e Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"sndio is an extra layer, Pulse is an extra layer (I still suspect they simply didn't want to fix ALSA itself), there are many extra layers around" - the way you describe it, it sounds like it's "either sndio or pulse", but that's not the case.

The sound architecture on OpenBSD is either "kernel -> sndio -> software with sndio support" or "kernel -> sndio -> SDL/pulse/libao/whatever -> software without sndio support" - Pulse/SDL/etc do not talk directly to the kernel, they are an extra layer on top of sndio.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this inherent, or just because someone implemented an incomplete PoC and then nobody tried to fix it?
Pulseaudio is just another intermediate layer on top on sndio on OpenBSD... technically, it just yields more latency, which means that some video that are pushing the envelope on some machines (say full HD hevc on not quite current hw) WILL fall on the wrong side of playing smoothly.

Apparently it even printed a warning about buggy behavior on every seek.

That warning is very pedantic. In reality, you don't even notice the blocking, considering you queue at most
a fraction of a second of audio... I just did a few tries alternating with sndio/ao sdl... I do not see ANY pause when I move around in the video. Indeed, I think we're going to patch that warning away locally.

if that's the only "bug" in sndio, it's definitely not worth stopping support for that.

@Selis
Copy link

@Selis Selis commented on 71d218e Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been running mpv on Linux with sndio compiled in manually without issue for quite some time. The notion that the sndio back end is flawed seems greatly exaggerated in my opinion. The only problem encountered is the one mentioned above, that on each seek a warning in printed in the console, and mpv desyncs for a fraction of a second to catch up. In practice this is never a problem, with the exception of the repetitive seeks caused when manually stepping frames. Again, were talking about maybe a 1/4 second delay, not a deal breaker for anyone using sndio in the first place.

I'm sympathetic to @wm4's idea of developers never having to write code for any given back end, and rather target middleware libraries instead. From my experience today however, this concept is idealistic. It seems that most such layers simply introduce new limitations in terms of performance, latency, audio quality, and/or debugging. It should also be noted that PulseAudio is not such a framework, but an audio server of itself, and dropping AO support with the motivation of "use Pulse instead" is non sequitur. Some would argue that the reason sndio exist and is getting such traction is for the purpose of replacing software like PulseAudio.

For a lot of people--such as myself, and I imagine many in the *BSD community--mpv is the player of choice due to its support for features such as this one. Considering how well the AO worked, it would be sad to see it go.

@ratchov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I somewhat understand @wm4 point of view: The code is specific to an OS that mpv developers don't use, they don't even know if that code woks at all, especially given that OpenBSD doesn't run in qemu with the audio defaults. I bet wm4 got no feedback from sndio AO users, as such feedback goes to the OpenBSD mailing-lists.

@prez
Copy link

@prez prez commented on 71d218e Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also sad to see this go. sndio on Linux has technical merit - I use it because of its simplicity and network transparency features.
And it even seems that the ao was written by @leahneukirchen with at least the partial intention to be useful under Linux.
This is just a Linux user's perspective, for OpenBSD it is of course absolutely vital.
Here's hoping that the bugs wm4 mentions can be resolved, and the burden of maintaining this ao eased.

@richardpl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a lie that maintaining AO are burden, they happily coexisted for eternity until now.

@leahneukirchen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was disabled by default, and I don't think anyone is using it.

Well, it was explicitly enabled by OpenBSD ports for over 4 years (and thus used by default there), so this argument is not worth much.

@Argon-
Copy link
Member

@Argon- Argon- commented on 71d218e Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it was explicitly enabled by OpenBSD ports for over 4 years (and thus used by default there), so this argument is not worth much.

This assumes a developer has to be aware of arbitrary patches package managers apply to their projects though.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes a developer has to be aware of arbitrary patches package managers apply to their projects though.
why provide configure options if we are not supposed to use them ?

let me reframe things in the proper order: you guys tell us you want to remove some option, we do tell you that we use it.

Now you're saying that you want to remove it because it's not default ?

Pray tell, why the special treatment ?

@jeeb
Copy link
Member

@jeeb jeeb commented on 71d218e Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not me having any concrete information and you should not think this is what is going on at all (after all, I'm one of the few who so far hasn't heard a better explanation for the removal of the --thing PARAM syntax for cli parsing than the user channel's stories of "A user did --log-file A_FILE.mkv and thus one way of the user to shoot his foot off was taken out"), but I will take a guesstimate here that:

  1. Some rework of the AO infrastructure was/is planning to happen.
  2. There are modules that are seeming low-hanging-fruit in what might make that work harder/require testing and isn't in anyone's field of interest who seems to be active in the project (as far as I see, we have Linux, Windows and macOS users active in the group of people poking mpv).
  3. Thus, these modules get removed before rework and testing has to happen to ease the load.
  4. As an extra, there seems to be a general dissatisfaction with the fact that all of these minor BSD operating systems decided to go for different interfaces, instead of converging for one.

Now, of course theoretically the best course of action would be for someone to become an active person in the community, and thus at least make the testing part simpler. But of course, if that would happen right now the likelihood that something would go in before that would depend on the willingness of one to make their job harder at least until whatever the planned rework was done. After that rework would be done, any new, incoming work would then have to work with that, and the burden would not be on the reworking person, but on the person bringing in new code.

Note: This is purely just my delusional thoughts on some of the things here, and most likely have no connection to reality. Personally, I welcome people into the active community.

@jeeb
Copy link
Member

@jeeb jeeb commented on 71d218e Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm going to note that I personally would have preferred this thread to get a response with an mpv log or something similar confirming playback with the AO instead of "We totally do use it!" kind of comments somewhat earlier.

I attempt to understand the feelings, and I am not meaning this in the way that I do not trust anyone in particular here - but it unfortunately often is the case that users think they are utilizing something, while it might be something completely different in reality.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me put things that way: if it's the only concern, leave the abandoned plugins code around, commit the changes, and wait to see if there are patches to fix them.

Specifically: stuff like sndio is not enabled by default. The proper opensource way to handle it is to JUST leave it alone, possibly mark it as broken until someone jumps to fix it, and remove it after a few months.

As it stands, currently it compiles and works (probably not perfectly, but hey, on machines where audio/video is already taxing the cpu, it can be better than other outputs), so just leave it alone.

I've been working on opensource projects for quite a few years by now. in general projects that remove code beforehand are seen as hostile. Fixing things requires more investment: you have to figure out how to re-add it, re-add the glue that makes it compile before fixing it.

I could tell you how I've finally given up on contributing to gcc for instance, because the entry barrier was too steep (send a patch... wait for 3 months for somebody to tell you it was not appropriate because it missed a space before a paren, oh and btw, could you redo the patch because it doesn't apply to current... check that current actually does NOT work at all on your OS, finally figure out the extra patch you need, rince, repeat, 3 months each time).

Leaving it alone, just disabling it (which is most often trivial) announcing that it will be removed if nobody steps in to make it work better is the proper collaborative way.

What do you actually want ? a "perfect" multimedia player that works on a very small collection of backends, or a nitty-gritty real-world player that work on more backends, possibly not perfectly.

... you can actually gain support from the community. I know I use mpv all the time, and if my audio backends stop working, I would be happy to spend time to fix them so they still build.

if you start by removing them because they're not perfect, and you require me to:

  • figure out how to re-add them;
  • fix them until they're "perfect".

Then it's more likely I will give up at some point. mpv is just one amongst 100+ software projects I try to work on...

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(+) Video --vid=1 () (h264 1920x800 29.970fps)
(+) Audio --aid=1 --alang=eng () (aac 2ch 48000Hz)
libEGL warning: DRI3: Screen seems not DRI3 capable
AO: [sndio] 48000Hz stereo 2ch s16
VO: [gpu] 1920x800 yuv420p

that what you mean ? works just fine here.

@jeeb
Copy link
Member

@jeeb jeeb commented on 71d218e Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(+) Video --vid=1 () (h264 1920x800 29.970fps) (+) Audio --aid=1 --alang=eng () (aac 2ch 48000Hz)
libEGL warning: DRI3: Screen seems not DRI3 capable
AO: [sndio] 48000Hz stereo 2ch s16
VO: [gpu] 1920x800 yuv420p

that what you mean ? works just fine here.

That is on a BSD? Nice. Thank you.

I've been working on opensource projects for quite a few years by now. in general projects that remove code beforehand are seen as hostile.

I agree.

Leaving it alone, just disabling it (which is most often trivial) announcing that it will be removed if nobody steps in to make it work better is the proper collaborative way.

It was generally left alone for quite a while. It was marked as experimental according to understanding at that point in 0a44451 in 2014. If you look at the changes in the sndio AO since its initial merge in 2013, that has been quite one-sided. There was one commit in 2015 from the original mplayer patch's author, but otherwise it has been up to a single person to keep it rolling (who does not even use it). And looking at f.ex. 12d93fd there were even seeming improvements done.

While I agree that there apparently there was no communique in any of the release notes etc regarding removal (as far as I can see) or apparent messages for any of the users, but it was pretty clear that this AO had no contact surface with users, and thus became an option for removal. Additionally, there were alternative AOs that would still keep audio output on the systems going, and theoretically might even be more maintained by actual people who care about the audio system. Not saying I agree with the style 100%, but I do see where the ship was headed with this AO.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, as one data-point currently it works just fine on OpenBSD. Specifically, this is mpv-0.32.0

We have no patch to speak of.

We built the port with a lot of configure options:CONFIGURE_ARGS = --confloaddir=${SYSCONFDIR}/mpv
--confdir=${LOCALBASE}/share/examples/mpv
--mandir=${LOCALBASE}/man
--docdir=${LOCALBASE}/share/examples/mpv
--enable-cdda
--enable-dvdnav
--enable-libmpv-shared
--enable-sndio
--enable-sdl2
--disable-alsa
--disable-caca
--disable-cuda-hwaccel
--disable-egl-drm
--disable-gl-wayland
--disable-jack
--disable-rubberband
--disable-oss-audio
--disable-openal
--disable-optimize
--disable-pulse
--disable-rsound
--disable-libsmbclient
--disable-uchardet
--disable-vaapi
--disable-vaapi-drm
--disable-vaapi-x-egl
--disable-vaapi-x11
--disable-vaapi-wayland
--disable-vapoursynth
--disable-vdpau
--disable-vdpau-gl-x11
--disable-videotoolbox-gl
--disable-vulkan
--disable-wayland

Note that some of this is necessary for our pkg system (builds happen on a "shared" system so we have to be explicit as to what we don't want, otherwise we produce packages with "hidden dependencies".

To be fair, mpv defaults to
AO: [sdl] 48000Hz stereo 2ch s32

on my machine, but direct sndio is slightly less expensive, and I do know of configurations where it's a definitive win.

@marcespie
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, OpenBSD is not FreeBSD, there are major differences between the OS. We don't have any kind of alsa nor alsa compatibility layer in OpenBSD. So basically, you encourage us to create a layer to non-existent software...

@hurzl
Copy link

@hurzl hurzl commented on 71d218e Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sndio, as opposed to pulseaudio, is the only network audio system that is usable.
It was working until 0.32 on FreeBSD, no reason to remove it.

@rozhuk-im
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8314

@rozhuk-im
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSS is much better than sndio.
sndio:

  • some times may freeze on full buf, device reopen required to fix it (sndio specific)
  • something wrong in init code: looks like it map center to left while 6->2 conversion (mpv backend specific)

@hurzl
Copy link

@hurzl hurzl commented on 71d218e Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I use OSS across the network?

Please sign in to comment.