New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow building with sndio support on more systems than just OpenBSD #4486

Merged
merged 1 commit into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@t6
Contributor

t6 commented Jul 14, 2018

Sndio also supports FreeBSD and Linux.

Show outdated Hide outdated CMakeLists.txt Outdated
Allow building with sndio support on more systems than just OpenBSD
Sndio also supports FreeBSD and Linux.

Signed-off-by: Tobias Kortkamp <t@tobik.me>

@PhysSong PhysSong changed the base branch from stable-1.2 to master Jul 18, 2018

@PhysSong PhysSong changed the base branch from master to stable-1.2 Jul 18, 2018

@PhysSong PhysSong changed the base branch from stable-1.2 to master Jul 22, 2018

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 8, 2018

Member

Please do not enable this blindly without some evidence that it actually works. I'll quote #3596:

Initially, I had just enabled RoarAudio (adds sdio.h) so we could test sndio via Ubuntu (via Travis). Unfortunately, RoarAudio won't compile, and it's not just us.

Member

tresf commented Aug 8, 2018

Please do not enable this blindly without some evidence that it actually works. I'll quote #3596:

Initially, I had just enabled RoarAudio (adds sdio.h) so we could test sndio via Ubuntu (via Travis). Unfortunately, RoarAudio won't compile, and it's not just us.

@t6

This comment has been minimized.

Show comment
Hide comment
@t6

t6 Aug 8, 2018

Contributor

What does sndio have to do with RoarAudio? The claim in #3596 that sndio is only available on OpenBSD is bogus. If you want to test this on Ubuntu install their libsndio-dev package.

Here's a build log from building LMMS 1.2.0-rc6 on FreeBSD with this patch applied: https://people.freebsd.org/~tobik/logs/lmms-1.2.0.r6,2.log

Contributor

t6 commented Aug 8, 2018

What does sndio have to do with RoarAudio? The claim in #3596 that sndio is only available on OpenBSD is bogus. If you want to test this on Ubuntu install their libsndio-dev package.

Here's a build log from building LMMS 1.2.0-rc6 on FreeBSD with this patch applied: https://people.freebsd.org/~tobik/logs/lmms-1.2.0.r6,2.log

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 8, 2018

Member

The claim in #3596 that sndio is only available on OpenBSD is bogus.

It's a description, not a claim. The message has since been removed from our stable-1.2 branch.

What does sndio have to do with RoarAudio

Our CI environment uses Ubuntu 14.04 and for that platform libsndio-dev is only provided as a virtual package though libroar-dev, which would not compile at time of adding that comment.

Here's a build log from building LMMS 1.2.0-rc6 on FreeBSD with this patch applied:

Thanks for the log and for the link to the bionic mirror. Please understand this will break builds on any 14.04-based machines that have libroar-dev installed. Broken builds eventually turn into bug reports and/or support questions, so it's sane behavior to guard against them.

At the time of writing this, 14.04 is old enough that most desktop users have moved away from it, so this just needs to be rebased and merged... but before anyone does that...

changed the base branch from stable-1.2 to master 17 days ago

@PhysSong can you explain why this PR was removed from our stable branch? If you have no objections, let's put it back on stable and merge it.

Member

tresf commented Aug 8, 2018

The claim in #3596 that sndio is only available on OpenBSD is bogus.

It's a description, not a claim. The message has since been removed from our stable-1.2 branch.

What does sndio have to do with RoarAudio

Our CI environment uses Ubuntu 14.04 and for that platform libsndio-dev is only provided as a virtual package though libroar-dev, which would not compile at time of adding that comment.

Here's a build log from building LMMS 1.2.0-rc6 on FreeBSD with this patch applied:

Thanks for the log and for the link to the bionic mirror. Please understand this will break builds on any 14.04-based machines that have libroar-dev installed. Broken builds eventually turn into bug reports and/or support questions, so it's sane behavior to guard against them.

At the time of writing this, 14.04 is old enough that most desktop users have moved away from it, so this just needs to be rebased and merged... but before anyone does that...

changed the base branch from stable-1.2 to master 17 days ago

@PhysSong can you explain why this PR was removed from our stable branch? If you have no objections, let's put it back on stable and merge it.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 8, 2018

Member

So this compiles and bundles just fine on Ubuntu 18.04.

I have a support question...

If we're going to toggle this on for all distros now, how is this backend used? I've tried rsnd/0 but it doesn't appear to work. The best listing I could find was here: http://man.openbsd.org/sndio#DEVICE_NAMES.

Tagging @devnexen too as he created the original PR which added sndio support.

image

Also @t6 just an FYI - In Ubuntu 18.04 there's a typo in FindSndio.cmake, probably fixed upstream already, but benign.

CMake Warning (dev) in cmake/modules/FindSndio.cmake: line 29:
  A logical block opening [...] closes [...] with mis-matching arguments.
# cmake/modules/FindSndio.cmake: line 29:

  if(SNDIO_FOUND)
      set(SNDIO_INCLUDE_DIRS "${SNDIO_INCLUDE_DIR}")
      set(SNDIO_LIBRARIES "${SNDIO_LIBRARY}")
- endif(HAVE_SNDIO)
+ endif(SNDIO_FOUND)
Member

tresf commented Aug 8, 2018

So this compiles and bundles just fine on Ubuntu 18.04.

I have a support question...

If we're going to toggle this on for all distros now, how is this backend used? I've tried rsnd/0 but it doesn't appear to work. The best listing I could find was here: http://man.openbsd.org/sndio#DEVICE_NAMES.

Tagging @devnexen too as he created the original PR which added sndio support.

image

Also @t6 just an FYI - In Ubuntu 18.04 there's a typo in FindSndio.cmake, probably fixed upstream already, but benign.

CMake Warning (dev) in cmake/modules/FindSndio.cmake: line 29:
  A logical block opening [...] closes [...] with mis-matching arguments.
# cmake/modules/FindSndio.cmake: line 29:

  if(SNDIO_FOUND)
      set(SNDIO_INCLUDE_DIRS "${SNDIO_INCLUDE_DIR}")
      set(SNDIO_LIBRARIES "${SNDIO_LIBRARY}")
- endif(HAVE_SNDIO)
+ endif(SNDIO_FOUND)
@devnexen

This comment has been minimized.

Show comment
Hide comment
@devnexen

devnexen Aug 8, 2018

Contributor

I do not know how it works under Linux but is device naming similar as OpenBSD ? Does not know if it brings a different experience than with Alsa neither.

Contributor

devnexen commented Aug 8, 2018

I do not know how it works under Linux but is device naming similar as OpenBSD ? Does not know if it brings a different experience than with Alsa neither.

@devnexen

This comment has been minimized.

Show comment
Hide comment
@devnexen

devnexen Aug 8, 2018

Contributor

I forgot to mention is sndiod installed ?

Contributor

devnexen commented Aug 8, 2018

I forgot to mention is sndiod installed ?

@t6

This comment has been minimized.

Show comment
Hide comment
@t6

t6 Aug 8, 2018

Contributor

If we're going to toggle this on for all distros now, how is this backend used? I've tried rsnd/0 but it doesn't appear to work.

For starters disable all other audio daemons. Sndio on Linux needs exclusive access to the audio device when used locally. AFAIK pasuspender should temporarily disable PulseAudio. rsnd/0 is the first audio device (it translates to the hw:0 ALSA device) but it might not be what you need on your hardware. So try rsnd/1, rsnd/2, etc. too.

It might be easier to test this with aucat -i /dev/urandom -f rsnd/1 first (if this works you'll hear some whitenoise, aucat is part of the sndio-tools package on Ubuntu) instead of trying it in LMMS.

Contributor

t6 commented Aug 8, 2018

If we're going to toggle this on for all distros now, how is this backend used? I've tried rsnd/0 but it doesn't appear to work.

For starters disable all other audio daemons. Sndio on Linux needs exclusive access to the audio device when used locally. AFAIK pasuspender should temporarily disable PulseAudio. rsnd/0 is the first audio device (it translates to the hw:0 ALSA device) but it might not be what you need on your hardware. So try rsnd/1, rsnd/2, etc. too.

It might be easier to test this with aucat -i /dev/urandom -f rsnd/1 first (if this works you'll hear some whitenoise, aucat is part of the sndio-tools package on Ubuntu) instead of trying it in LMMS.

@devnexen

This comment has been minimized.

Show comment
Hide comment
@devnexen

devnexen Aug 8, 2018

Contributor

Ok so I built all under debian and seems to work tried couple of seconds with rsnd/0 device in my side.

Contributor

devnexen commented Aug 8, 2018

Ok so I built all under debian and seems to work tried couple of seconds with rsnd/0 device in my side.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 9, 2018

Member

I forgot to mention is sndiod installed ?

No, it wasn't but I managed to get it to work without it.

Sndio on Linux needs exclusive access to the audio device when used locally. AFAIK pasuspender should temporarily disable PulseAudio.

Thanks and spot-on. sndio works as long as pasuspender prefixes the LMMS command.

For the installed version:

pasuspender -- lmms

For the compiled version:

pasuspender -- ./lmms

For the AppImage version:

pasuspender -- ./lmms-1.2.0-rc6.32-linux-x86_64.AppImage

rsnd/0 is the first audio device

That's perfect, I'm in a VM, so there's only one back-end. Fortunately, once PA is suspended, sndio just works without specifying the device. This is ideal.

So I think this is ready for merge to stable-1.2 if @PhysSong agrees. Thanks for the support assistance. The only downside is we can't ship it with our official AppImage because our build system for 1.2.0 is still 14.04 which doesn't provide the proper library. This will change once we start releasing on newer distros, starting with 1.3.0.

From a performance testing perspective, we ship a very resource intensive demo unfa - Spoken and it starts to under-run on virtual hardware about half through where SDL is able to keep up. This isn't a comprehensive test. We'll have more real-world examples trickle in once this is merged.

Member

tresf commented Aug 9, 2018

I forgot to mention is sndiod installed ?

No, it wasn't but I managed to get it to work without it.

Sndio on Linux needs exclusive access to the audio device when used locally. AFAIK pasuspender should temporarily disable PulseAudio.

Thanks and spot-on. sndio works as long as pasuspender prefixes the LMMS command.

For the installed version:

pasuspender -- lmms

For the compiled version:

pasuspender -- ./lmms

For the AppImage version:

pasuspender -- ./lmms-1.2.0-rc6.32-linux-x86_64.AppImage

rsnd/0 is the first audio device

That's perfect, I'm in a VM, so there's only one back-end. Fortunately, once PA is suspended, sndio just works without specifying the device. This is ideal.

So I think this is ready for merge to stable-1.2 if @PhysSong agrees. Thanks for the support assistance. The only downside is we can't ship it with our official AppImage because our build system for 1.2.0 is still 14.04 which doesn't provide the proper library. This will change once we start releasing on newer distros, starting with 1.3.0.

From a performance testing perspective, we ship a very resource intensive demo unfa - Spoken and it starts to under-run on virtual hardware about half through where SDL is able to keep up. This isn't a comprehensive test. We'll have more real-world examples trickle in once this is merged.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 9, 2018

Member

So I think this is ready for merge to stable-1.2 if @PhysSong agrees

What I was afraid of was that stable-1.2 is on a feature freeze. However, I'd like to listen to opinions from other devs. If they agree on merging into stable-1.2, then I won't block that.

Member

PhysSong commented Aug 9, 2018

So I think this is ready for merge to stable-1.2 if @PhysSong agrees

What I was afraid of was that stable-1.2 is on a feature freeze. However, I'd like to listen to opinions from other devs. If they agree on merging into stable-1.2, then I won't block that.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 17, 2018

Member

This will change once we start releasing on newer distros, starting with 1.3.0.

I think we can add libsndio-dev into our CircleCI docker image.

Member

PhysSong commented Aug 17, 2018

This will change once we start releasing on newer distros, starting with 1.3.0.

I think we can add libsndio-dev into our CircleCI docker image.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 17, 2018

Member

I think we can add libsndio-dev into our CircleCI docker image.

Sounds good. I still think this belongs on stable-1.2, so you can toggle on libsndio-dev in Cirlce-CI anytime.

Member

tresf commented Aug 17, 2018

I think we can add libsndio-dev into our CircleCI docker image.

Sounds good. I still think this belongs on stable-1.2, so you can toggle on libsndio-dev in Cirlce-CI anytime.

@tresf tresf changed the base branch from master to stable-1.2 Aug 27, 2018

@tresf tresf merged commit 4bb6586 into LMMS:stable-1.2 Aug 27, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@t6

This comment has been minimized.

Show comment
Hide comment
@t6

t6 Aug 27, 2018

Contributor

Thank you, @tresf!

Contributor

t6 commented Aug 27, 2018

Thank you, @tresf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment