virtual_oss: Port to base

[christos]

Summary

This patch diverges quite a bit from the current upstream [1] in a few
ways:

1. virtual_oss(8), virtual_bt_speaker(8) and virtual_oss_cmd(8) are actually separate programs.
2. Backends (lib/virtual_oss) are built as separate shared libraries and we dlopen() them in virtual_oss(8) and virtual_bt_speaker(8) on demand.
3. virtual_equalizer(8) and the sndio and bluetooth backends are built as ports, because they depend on third-party libraries.
4. Use newer libav API in bluetooth backend (see HAVE_LIBAV ifdefs) to address compiler errors.

[1] https://github.com/freebsd/virtual_oss

Sponsored by: The FreeBSD Foundation

[christos]

Unhook sndio backend and virtual_equalizer from builds, build them as ports
instead (see D52365 and D52366) as they depend on third-party libraries, and
are also optional.

[christos]

lib/virtual_oss/bt/Makefile

13

I need to address this. Basically we have some parts that depend on some FFMPEG third-party libraries (see LDFLAGS below). Maybe I could also package this into a port. Any ideas?

[christos]

libexec/rc/rc.d/virtual_oss

6

Not really sure how to handle this either. This is only relevant if we have sndio support built.

52

Ditto, but for virtual_equalizer support.

[christos]

Changes:

• Because voss_has_synchronization cannot be used from inside the backends, add a pointer to it in voss_load_backend(). A bit hacky, but it does the job for now.
• Pass a prefix argument to voss_load_backend(), since we have backends that live in /usr/lib (built by src), and others in /usr/local/lib (built by ports, see D52365). I do not really like this though...

TODO:

• I am still thinking of a clean way to handle HAVE_FFMPEG in the bluetooth code. Essentially this adds AAC support, but depends on gstreamer1-libav. Making those code pieces a shared library is possible, but I think it'll complicate things a bit.
• virtual_oss_wait() is needed by some backends, but for now we get an error because this symbol is defined by virtual_oss(8), so the shared libs cannot use it. Defining it in the library code is also not easily possible, as virtual_oss_wait() uses variables from virtual_oss(8) (voss_dsp_samples and voss_dsp_sample_rate).
• rc script inline comments (see below).

[arrowd]

lib/virtual_oss/sndio/Makefile

9

Do not hardcode "/usr/local", use ${LOCALBASE} instead. This will be provided by ports framework via MAKE_ENV.

libexec/rc/rc.d/virtual_oss

15

Use $(sysctl -n user.localbase) here instead.

usr.sbin/virtual_oss/virtual_equalizer/Makefile

10

${LOCALBASE}

[kevans]

usr.sbin/virtual_oss/virtual_equalizer/Makefile

10

${LOCALBASE:U/usr/local} to 'just work' when you're trying to build it not as part of the ports framework (for, e.g., local testing).

[christos]

Fix libav compiler errors, use newer API. Not tested however.

Changes: https://reviews.freebsd.org/P670

[christos]

Use ${LOCALBASE:U/usr/local} in Makefiles, and $(sysctl -n user.locabase) in rc
script.

[christos]

Unhook bluetooth backend from build. See D52426.

[christos]

Build sndio backend with libsamplerate to address build failure in
https://reviews.freebsd.org/D52365#1197605.

[arrowd]

Making world from scratch with this change applied results in

===> lib/virtual_oss/null (install)
install -U  -s -o root -g wheel -m 444  -C -S  voss_null.so /usr/obj/usr/src/amd64.amd64/tmp/usr/lib/virtual_oss/
install: /usr/obj/usr/src/amd64.amd64/tmp/usr/lib/virtual_oss: No such file or directory
*** [_libinstall] Error code 71

for me.

[arrowd]

For now I just fooled the buildsystem by creating those dirs manually.

usr.sbin/virtual_oss/virtual_bt_speaker/bt_speaker.c

98

/usr/src/usr.sbin/virtual_oss/virtual_bt_speaker/bt_speaker.c:97:26: error: cast from 'const char *' to 'bdaddr_t *' drops const qualifier [-Werror,-Wcast-qual]
   97 |             __DECONST(bdaddr_p, NG_HCI_BDADDR_ANY),
      |                                 ^
/usr/obj/usr/src/amd64.amd64/tmp/usr/include/netgraph/bluetooth/include/ng_hci.h:451:39: note: expanded from macro 'NG_HCI_BDADDR_ANY'
  451 | #define NG_HCI_BDADDR_ANY       ((bdaddr_p) "\000\000\000\000\000\000")
      |                                             ^
1 error generated.

[arrowd]

All right, figured the installation problem. The following diff should be added to the review:

diff --git a/etc/mtree/BSD.lib32.dist b/etc/mtree/BSD.lib32.dist
index a736a7d58b66..8a3fe9a34714 100644
--- a/etc/mtree/BSD.lib32.dist
+++ b/etc/mtree/BSD.lib32.dist
@@ -21,5 +21,7 @@
         ..
         pkgconfig
         ..
+        virtual_oss
+        ..
     ..
 ..
diff --git a/etc/mtree/BSD.usr.dist b/etc/mtree/BSD.usr.dist
index 7d3a72f43fa1..265f4cfe8753 100644
--- a/etc/mtree/BSD.usr.dist
+++ b/etc/mtree/BSD.usr.dist
@@ -101,6 +101,8 @@
         ..
         ossl-modules
         ..
+        virtual_oss
+        ..
     ..
     libdata
         ldscripts

[christos]

In D52308#1198586, @arrowd wrote:

All right, figured the installation problem. The following diff should be added to the review:

diff --git a/etc/mtree/BSD.lib32.dist b/etc/mtree/BSD.lib32.dist
index a736a7d58b66..8a3fe9a34714 100644
--- a/etc/mtree/BSD.lib32.dist
+++ b/etc/mtree/BSD.lib32.dist
@@ -21,5 +21,7 @@
         ..
         pkgconfig
         ..
+        virtual_oss
+        ..
     ..
 ..
diff --git a/etc/mtree/BSD.usr.dist b/etc/mtree/BSD.usr.dist
index 7d3a72f43fa1..265f4cfe8753 100644
--- a/etc/mtree/BSD.usr.dist
+++ b/etc/mtree/BSD.usr.dist
@@ -101,6 +101,8 @@
         ..
         ossl-modules
         ..
+        virtual_oss
+        ..
     ..
     libdata
         ldscripts

Good catch! I always forget about those... Thanks for the valuable help. :-)

[christos]

usr.sbin/virtual_oss/virtual_bt_speaker/bt_speaker.c

98

Trying to address this in D52310.

[christos]

Address Gleb's comments.

[christos]

Get rid of __DECONST hack in virtual_bt_speaker (see update in D52310).

[christos]

Most issues have been addressed. Only ones remaining so far:

• virtual_oss_wait() is needed by some backends, but for now we get an error because this symbol is defined by virtual_oss(8), so the shared libs cannot use it. Defining it in the library code is also not easily possible, as virtual_oss_wait() uses variables from virtual_oss(8) (voss_dsp_samples and voss_dsp_sample_rate)
• rc script inline comments (see below).

[christos]

Fixed symbol resolution failures. Only thing remaining is the rc script inline
comments below.

At this point the patch should already be ready to commit if there are no
issues during testing, so feel free to test it as well.

[christos]

rc script changes:

• Fix virtual_oss path in command variable.
• Get rid of of virtual_oss_delay and sleep ${virtual_oss_delay} in virtual_oss_start() and virtual_oss_stop(). It can work without sleeping between starting/stopping configs.

[christos]

The patch is essentially finalized and IMHO ready to commit. Only thing remaining (but not really that important) are the rc script inline comments.

Also, since I'd like this to land in 15.0, I guess we'd need to bump __FreeBSD_version for stable/15, so that ports like D52593 can build only if the machine does have virtual_oss in base (related discussion there). @markj @emaste @cperciva What do you think?

[cperciva]

In D52308#1201895, @christos wrote:

The patch is essentially finalized and IMHO ready to commit. Only thing remaining (but not really that important) are the rc script inline comments.

Also, since I'd like this to land in 15.0, I guess we'd need to bump __FreeBSD_version for stable/15, so that ports like D52593 can build only if the machine does have virtual_oss in base (related discussion there). @markj @emaste @cperciva What do you think?

Yes, go ahead with the __FreeBSD_version bump. But please get it done soon; I'll be creating releng/15.0 in 1.5 weeks and this probably can't go in after that point.

[christos]

In D52308#1203289, @cperciva wrote:

In D52308#1201895, @christos wrote:

The patch is essentially finalized and IMHO ready to commit. Only thing remaining (but not really that important) are the rc script inline comments.

Also, since I'd like this to land in 15.0, I guess we'd need to bump __FreeBSD_version for stable/15, so that ports like D52593 can build only if the machine does have virtual_oss in base (related discussion there). @markj @emaste @cperciva What do you think?

Yes, go ahead with the __FreeBSD_version bump. But please get it done soon; I'll be creating releng/15.0 in 1.5 weeks and this probably can't go in after that point.

The patch is done on my side, once it's reviewed I will push it immediately.

[0mp]

I took a look at the rc script and I've got some suggestions.

libexec/rc/rc.d/virtual_oss

68

This could be "$1"

71

It's better to one of the logging functions here like warn().

73

Would be better to use startmsg here. This way the user has a way to mute those.

[arrowd]

I'm still getting

===> lib/virtual_oss (all)
===> lib/virtual_oss/null (all)
make[6]: /usr/obj/usr/src/amd64.amd64/lib/virtual_oss/null/.depend:1: ignoring stale .depend for /usr/obj/usr/src/amd64.amd64/tmp/usr/lib/libprivatesamplerate.a
Building shared library voss_null.so
cc -target x86_64-unknown-freebsd16.0 --sysroot=/usr/obj/usr/src/amd64.amd64/tmp -B/usr/obj/usr/src/amd64.amd64/tmp/usr/bin  -Wl,-zrelro    -fstack-protector-strong -shared -Wl,-x -Wl,--fatal-warnings -Wl,--warn-shared-textrel  -o voss_null.so.full -Wl,-soname,voss_null.so null.pico  -lprivatesamplerate 
ld: error: unable to find library -lprivatesamplerate

when trying to build this diff. It also does not apply with arc patch for me for some reason.

[emaste]

lib/Makefile

119

I think we also need something like SUBDIR_DEPEND_virtual_oss= libsamplerate
I'm trying to understand another build failure that I'm seeing though.

[christos]

Address commnets. Also got the script to an acceptable (better) state with the
help of Mateusz (0mp@) and shellcheck(1).

[christos]

In D52308#1204955, @arrowd wrote:

I'm still getting

===> lib/virtual_oss (all)
===> lib/virtual_oss/null (all)
make[6]: /usr/obj/usr/src/amd64.amd64/lib/virtual_oss/null/.depend:1: ignoring stale .depend for /usr/obj/usr/src/amd64.amd64/tmp/usr/lib/libprivatesamplerate.a
Building shared library voss_null.so
cc -target x86_64-unknown-freebsd16.0 --sysroot=/usr/obj/usr/src/amd64.amd64/tmp -B/usr/obj/usr/src/amd64.amd64/tmp/usr/bin  -Wl,-zrelro    -fstack-protector-strong -shared -Wl,-x -Wl,--fatal-warnings -Wl,--warn-shared-textrel  -o voss_null.so.full -Wl,-soname,voss_null.so null.pico  -lprivatesamplerate 
ld: error: unable to find library -lprivatesamplerate

when trying to build this diff. It also does not apply with arc patch for me for some reason.

Does @emaste 's suggestion in the inline comment fix this?

[emaste]

ld: error: unable to find library -lprivatesamplerate

Does @emaste 's suggestion in the inline comment fix this?

My comment is related to this, but I'm still getting this as well, looking..

[christos]

In D52308#1205072, @emaste wrote:

ld: error: unable to find library -lprivatesamplerate

Does @emaste 's suggestion in the inline comment fix this?

My comment is related to this, but I'm still getting this as well, looking..

Interesting, I cannot reproduce this for some reason...

[emaste]

Something like the patch below (applied to each virtual_oss subdir) works. It's gross, but we're already doing this in e.g. lib/googletest/gmock/Makefile

diff --git a/lib/virtual_oss/bt/Makefile b/lib/virtual_oss/bt/Makefile
index 15413b7a1f1e..3d075946f362 100644
--- a/lib/virtual_oss/bt/Makefile
+++ b/lib/virtual_oss/bt/Makefile
@@ -9,6 +9,7 @@ CFLAGS+=        -I${SRCTOP}/usr.sbin/virtual_oss/virtual_oss \
                -I${SRCTOP}/contrib/libsamplerate
 LDFLAGS+=      -lbluetooth -lsdp
 LIBADD=                samplerate
+LDFLAGS+=      -L${.OBJDIR:H:H}/libsamplerate
 
 .if defined(HAVE_LIBAV)
 CFLAGS+=       -I${LOCALBASE:U/usr/local}/include -DHAVE_LIBAV

[kevans]

In D52308#1205084, @emaste wrote:

Something like the patch below (applied to each virtual_oss subdir) works. It's gross, but we're already doing this in e.g. lib/googletest/gmock/Makefile

virtual_oss/Makefile.inc, please

diff --git a/lib/virtual_oss/bt/Makefile b/lib/virtual_oss/bt/Makefile
index 15413b7a1f1e..3d075946f362 100644
--- a/lib/virtual_oss/bt/Makefile
+++ b/lib/virtual_oss/bt/Makefile
@@ -9,6 +9,7 @@ CFLAGS+=        -I${SRCTOP}/usr.sbin/virtual_oss/virtual_oss \
                -I${SRCTOP}/contrib/libsamplerate
 LDFLAGS+=      -lbluetooth -lsdp
 LIBADD=                samplerate
+LDFLAGS+=      -L${.OBJDIR:H:H}/libsamplerate
 
 .if defined(HAVE_LIBAV)
 CFLAGS+=       -I${LOCALBASE:U/usr/local}/include -DHAVE_LIBAV

[emaste]

In D52308#1205129, @kevans wrote:

In D52308#1205084, @emaste wrote:

Something like the patch below (applied to each virtual_oss subdir) works. It's gross, but we're already doing this in e.g. lib/googletest/gmock/Makefile

virtual_oss/Makefile.inc, please

I'm thinking more this is a demonstration of the issue and we need to find a way to do this properly for virtual_oss (and convert googletest as well) rather than a cleaner version of the hack.

[christos]

Address Ed's comment.

[christos]

If it builds fine and there are no further issues I think we can commit it.

[emaste]

LDFLAGS+= -L in lib/:

lib/atf/libatf-c++/Makefile:LDFLAGS+=   -L${.OBJDIR:H}/libatf-c
lib/csu/tests/dynamiclib/Makefile:LDFLAGS+=     -Wl,-rpath,${TESTSDIR} -L${.OBJDIR:H}/dso
lib/googletest/gmock/Makefile:LDFLAGS+= -L${.OBJDIR:H}/gtest
lib/googletest/gmock_main/Makefile:LDFLAGS+=    -L${LIBGTESTDIR}
lib/googletest/gmock_main/Makefile:LDFLAGS+=    -L${LIBGMOCKDIR}
lib/googletest/gtest_main/Makefile:LDFLAGS+=    -L${LIBGTESTDIR}
lib/libc/tests/stdlib/Makefile:LDFLAGS.$t+=     -L${LIBNETBSD_OBJDIR}
lib/libc/tests/tls/Makefile:LDFLAGS.tls_dynamic_test+=  -Wl,-rpath,${TESTSDIR} -L${DSODIR}
lib/libthr/tests/dlopen/Makefile:LDFLAGS+=      -L${.OBJDIR}/dso -Wl,-rpath=${TESTDIR}
lib/libxo/encoder/csv/Makefile:LDFLAGS += -L${.OBJDIR:H:H}/libxo

So I think it's fine to use the same -L hack here, and putting it in virtual_oss/Makefile.inc at least avoid duplication.

[christos]

Address Ed's comment.