Add support for sndio #647

lanodan · 2021-09-16T03:15:20Z

New attempt to get sndio into portaudio, sadly I couldn't get the comments to the diff on Assembla for some reason.

Closes: #220

RossBencina · 2021-09-23T00:13:05Z

@Ianodan: Please clarify: are you saying that you have not made the changes requested at Assembla?

philburk · 2021-09-23T00:17:33Z

We merged a PR last week, Add Sun/NetBSD hostapi #645
I thought that included support for sndio. Maybe not.

Is there any overlap between these two implementations? Do we need both?

lanodan · 2021-09-23T10:56:28Z

@Ianodan: Please clarify: are you saying that you have not made the changes requested at Assembla?

I cannot see the comments that were made on Assembla for the PR done some years ago by someone else, so no.

We merged a PR last week, Add Sun/NetBSD hostapi #645
I thought that included support for sndio. Maybe not.

Is there any overlap between these two implementations? Do we need both?

There is no relation between Sun/NetBSD audio and sndio.
sndio is from OpenBSD but works on multiple operating systems (OpenBSD, NetBSD, FreeBSD, Linux, …) as it isn't a kernel-level audio system but an audio server (like JACK or PulseAudio).
As for the need for it, there is an ALSA compatibility layer but it's rather limited (lack of (sub)device selection, which is annoying for audio recording purposes).

philburk

Thanks for resurrecting this code. I have not given a it a full review. But I did notice some stylistic issues.

README.md

src/hostapi/sndio/pa_sndio.c

RossBencina · 2021-10-28T00:38:20Z

We are interested in this contribution, but we will not accept it as-is. Our basic formatting conventions need to be adhered to (e.g. use spaces not tabs, formatting of function declarations) see: https://github.com/PortAudio/portaudio/wiki/ImplementationStyleGuidelines . Beyond this, I think the review comments to-date are reasonable, but perhaps there is some room for compromise in the style of variable names (inch vs. inputChannelCount for example).

philburk · 2021-10-28T00:39:46Z

Please let us know if you are interested in updating the code style.

Also please let us know if you are willing to be an "owner" of this code and make bug fixes if needed.

loveJesus · 2021-11-06T23:31:47Z

God is good,
Just wanted to say thanks for the work on portaudio and this patch. It is working for me on OpenBSD 7.0, at least to the point of using through the pyaudio package. In case it helps anyone: to compile, i needed to

doas pkg_add automake autoconf gmake libtool
# The following were the versions installed and are needed on OpenBSD. 
export AUTOMAKE_VERSION=1.16
export AUTOCONF_VERSION=2.71 
autoreconf -ri
./configure --without-audioio --without-oss --with-sndio
gmake clean  #(instead of make)
gmake 
doas gmake install
#Likely there is a better way to do the following
doas cp include/portaudio.h /usr/local/include 
export CFLAGS="-I/usr/local/include"
# Now can pip install pyaudio

Hope that covers things and did not forget anything.

philburk · 2021-11-12T01:24:23Z

@lanodan - Please let us know if you would like to keep working on this. Please see comments above.

lanodan · 2021-11-12T12:04:15Z

@lanodan - Please let us know if you would like to keep working on this. Please see comments above.

Yes, I do want to keep working on it. AFAIK I just addressed all the comments above.

please let us know if you are willing to be an "owner" of this code and make bug fixes if needed.

I am willing to maintain this code as I need it to work and would gladly welcome co-owners, specially ones which are from non-Linux to cover more systems (as I've yet to get actually working audio in QEMU).

As for the code style, I pushed a .clang-format file to avoid mismatched indentation settings for at least the sndio module, could be moved into the sndio module folder.
Variable renaming is a bit tedious but I think it's much better to have it be similar enough to other modules.

.github/workflows/cmake.yml

CMakeLists.txt

RossBencina · 2021-11-18T01:32:31Z

It's looking much more consistent with our other code. Could you please also make a global find and replace of sndio_stream -> sndioStream and pa_stream -> paStream.

RossBencina · 2021-11-18T01:40:54Z

src/hostapi/sndio/pa_sndio.c

+        par->bps = 3; /* paInt24 is packed format */
+        break;
+    case paInt16:
+    case paFloat32:


I'm not sure whether this is a bug, but ideally float32 should be mapped to 24 or 32 bit IO (assuming that sndio doesn't support float IO natively).

Also please leave a comment explaining why it works even though it may look odd. That will save you from having people ask questions about the code in the future.

Looks like this was a bug I inherited, it doesn't looks like sndio supports floats so I removed the case paFloat32: line for now, will try to fix it properly.

Mapped it to 32bit IO, not entirely sure what would be the best way to test or be sure which to pick.

This is based on current modifications to portaudio done in OpenBSD. Closes: PortAudio#220

RossBencina · 2022-01-27T01:46:43Z

src/hostapi/sndio/pa_sndio.c

+    PaSndioHostApiRepresentation *sndioHostApi;
+    PaDeviceInfo *info;
+    struct sio_hdl *hdl;
+    char *audiodevices;


should be audioDevices

Fixed in #878

RossBencina · 2022-01-27T01:52:19Z

src/hostapi/sndio/pa_sndio.c

+typedef struct PaSndioStream
+{
+    PaUtilStreamRepresentation base;
+    PaUtilBufferProcessor bufproc; /* format conversion */


rename to bufferProcessor

Fixed in #878

philburk · 2022-01-27T01:45:53Z

.clang-format

@@ -0,0 +1,10 @@
+AlignTrailingComments: 'false'


This is looking very good.
This file can lead to unexpected reformatting.
Can you please move this .clang-format file into the /sndio/ folder?

Fixed in #878

philburk · 2022-01-27T01:50:48Z

src/hostapi/sndio/pa_sndio.c

+    struct sio_par par;
+    unsigned mode;
+    int inputChannelCount, outputChannelCount;
+    PaSampleFormat ifmt, ofmt, siofmt;


Please use more clear names like "inputFormat".

brad0 · 2022-10-21T04:59:35Z

configure.in

+have_sndio=no
+if test "x$with_sndio" != "xno"; then
+    AC_CHECK_LIB(sndio, sio_open, have_sndio=yes, have_sndio=no)
+fi


Should use PKG_CHECK_MODULES(SNDIO, sndio, have_sndio=yes, have_sndio=no) here.

Fixed in #878

brad0 · 2022-10-21T04:59:43Z

configure.in

@@ -382,6 +390,13 @@ case "${host_os}" in
           AC_DEFINE(PA_USE_AUDIOIO,1)
        fi

+        if [[ "$have_sndio" = "yes" -a "$with_sndio" != "no" ]] ; then
+           DLL_LIBS="$DLL_LIBS -lsndio"
+           LIBS="$LIBS -lsndio"


Should use

DLL_LIBS="$DLL_LIBS $SNDIO_LIBS" LIBS="$LIBS $SNDIO_LIBS" CFLAGS="$CFLAGS $SNDIO_CFLAGS"

here.

Fixed in #878

brad0 · 2022-10-27T04:46:22Z

BTW, I came across this ticket as I am an OpenBSD developer and was going to consider another push of a sndio backend, but came across this PR before doing so.

c64skin · 2023-01-31T08:51:21Z

Any chance of this getting merged before we die of old age?

philburk · 2024-01-19T22:40:40Z

Is anyone interested in moving this forward?
It needs to be rebased and there are unresolved issues.

brad0 · 2024-01-20T04:38:12Z

Is anyone interested in moving this forward?

We have had a PA backend in our tree for 15 years. I would very much like to see something moving forward. Even if it's not perceived to be 100% perfect. You have to start somewhere.

It needs to be rebased and there are unresolved issues.

@lanodan seems to have disappeared.

lanodan · 2024-01-20T10:24:47Z

> It needs to be rebased and there are unresolved issues. @lanodan seems to have disappeared.

Sorry for the silence, I ended up switching to pipewire for unrelated reasons to this PR (as I had it applied on my systems), feel free to take over.

lanodan force-pushed the sndio branch 2 times, most recently from e0e0c44 to 04df09a Compare September 16, 2021 03:47

philburk assigned lanodan Sep 23, 2021

philburk requested changes Oct 3, 2021

View reviewed changes

lanodan force-pushed the sndio branch from 04df09a to 807c19d Compare November 12, 2021 11:43

lanodan marked this pull request as ready for review November 12, 2021 11:49

lanodan force-pushed the sndio branch 2 times, most recently from f8a10b5 to 840a4fc Compare November 12, 2021 13:12

Be-ing reviewed Nov 13, 2021

View reviewed changes

.github/workflows/cmake.yml Outdated Show resolved Hide resolved

Be-ing reviewed Nov 13, 2021

View reviewed changes

.github/workflows/cmake.yml Outdated Show resolved Hide resolved

Be-ing reviewed Nov 13, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

RossBencina reviewed Nov 18, 2021

View reviewed changes

lanodan force-pushed the sndio branch from 476365e to 083a1b1 Compare December 1, 2021 17:57

lanodan added 3 commits January 14, 2022 22:47

.clang-format: Add, will be used for src/hostapi/sndio/

0aa7b01

Add support for sndio

baaae81

This is based on current modifications to portaudio done in OpenBSD. Closes: PortAudio#220

GH Workflows: Install sndio on Ubuntu

dbe5945

lanodan force-pushed the sndio branch from 083a1b1 to dbe5945 Compare January 14, 2022 21:51

RossBencina reviewed Jan 27, 2022

View reviewed changes

philburk requested changes Jan 27, 2022

View reviewed changes

brad0 suggested changes Oct 21, 2022

View reviewed changes

lanodan closed this Jan 20, 2024

philburk mentioned this pull request Jan 21, 2024

Add support for sndio #878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sndio #647

Add support for sndio #647

lanodan commented Sep 16, 2021 •

edited

Loading

RossBencina commented Sep 23, 2021

philburk commented Sep 23, 2021

lanodan commented Sep 23, 2021 •

edited

Loading

philburk left a comment

RossBencina commented Oct 28, 2021

philburk commented Oct 28, 2021

loveJesus commented Nov 6, 2021 •

edited

Loading

philburk commented Nov 12, 2021

lanodan commented Nov 12, 2021 •

edited

Loading

RossBencina commented Nov 18, 2021

RossBencina Nov 18, 2021 •

edited

Loading

philburk Nov 18, 2021

lanodan Dec 1, 2021 •

edited

Loading

lanodan Jan 14, 2022

RossBencina Jan 27, 2022

philburk Jan 21, 2024

RossBencina Jan 27, 2022

philburk Jan 21, 2024

philburk Jan 27, 2022

philburk Jan 21, 2024

philburk Jan 27, 2022

brad0 Oct 21, 2022

brad0 Jan 21, 2024

brad0 Oct 21, 2022

brad0 Jan 21, 2024

brad0 commented Oct 27, 2022

c64skin commented Jan 31, 2023

philburk commented Jan 19, 2024

brad0 commented Jan 20, 2024

lanodan commented Jan 20, 2024 via email

Add support for sndio #647

Add support for sndio #647

Conversation

lanodan commented Sep 16, 2021 • edited Loading

RossBencina commented Sep 23, 2021

philburk commented Sep 23, 2021

lanodan commented Sep 23, 2021 • edited Loading

philburk left a comment

Choose a reason for hiding this comment

RossBencina commented Oct 28, 2021

philburk commented Oct 28, 2021

loveJesus commented Nov 6, 2021 • edited Loading

philburk commented Nov 12, 2021

lanodan commented Nov 12, 2021 • edited Loading

RossBencina commented Nov 18, 2021

RossBencina Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanodan Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brad0 commented Oct 27, 2022

c64skin commented Jan 31, 2023

philburk commented Jan 19, 2024

brad0 commented Jan 20, 2024

lanodan commented Jan 20, 2024 via email

lanodan commented Sep 16, 2021 •

edited

Loading

lanodan commented Sep 23, 2021 •

edited

Loading

loveJesus commented Nov 6, 2021 •

edited

Loading

lanodan commented Nov 12, 2021 •

edited

Loading

RossBencina Nov 18, 2021 •

edited

Loading

lanodan Dec 1, 2021 •

edited

Loading