Skip to content
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.

Sign up for GitHub

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

Jump to bottom

A new AudioEngine sink for sndio #11962

Merged
merged 1 commit into from May 20, 2017
Merged

A new AudioEngine sink for sndio #11962

merged 1 commit into from May 20, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 13, 2017

Description

A new AudioEngine sink for sndio.

Motivation and Context

Sndio is an audio framework developed by the OpenBSD project. So far
it's supports Linux, DragonFly BSD, FreeBSD, and OpenBSD.

I'm currently running a patched version of the Kodi FreeBSD port with
this sink enabled. It would be nice if this can be included properly.

The pull request includes some changes (mostly adding missing
includes) that were required for building Kodi on FreeBSD. The ALSA
backend doesn't currently compile on FreeBSD.

How Has This Been Tested?

I've tested the sink on FreeBSD HEAD and Fedora 25 with Kodi compiled
from the master branch. I've been patching the FreeBSD port with
versions of this sink for the last 6 months or so. Ubuntu/Debian have
an sndio package, but on Fedora you have to install it manually.

To test it on Linux you probably need to disable PulseAudio
temporarily to free the audio device. sndiod will lock it while it
is running. Start sndiod -dd so that it runs in the foreground and
prints debug messages. Kodi was started with AE_SINK=sndio ./kodi.bin -p from the build directory.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

stefansaraev
stefansaraev reviewed Apr 13, 2017
View reviewed changes
xbmc/filesystem/SMBDirectory.cpp Outdated
@@ -31,6 +31,7 @@

#include "system.h"

#ifdef HAS_FILESYSTEM_SMB

This comment was marked as spam.

Sign in to view

fritsch
fritsch reviewed Apr 13, 2017
View reviewed changes
xbmc/cores/AudioEngine/Sinks/AESinkSNDIO.h Outdated
struct sio_hdl *m_hdl;
struct sio_par par;
ssize_t played;
ssize_t written;

This comment was marked as spam.

Sign in to view

@fritsch
Copy link
Member

fritsch commented Apr 13, 2017

@t6 please rebase and remove the commits not needed.

One thing to add: there is a currently work going on in order to remove the ugly Factory that instantiates the sink (you cannot know that). So if we would merge that one, will you help us to also port this specific sink to the upcoming factory changes?

And something also clear: You will become the maintainer of that one after review is finished, right?

And last but not least did you run time test it?

@fritsch
Copy link
Member

fritsch commented Apr 13, 2017

Also please drop the first commit - the build fixes. This should go in separately after reviewed by @wsnipex - if it needs to go in prior to the sink commit, then so be it.

In general for kodi we make "topic" PRs, e.g. adding one feature, doing exactly one fix and so on.

So in your case:

PR 1: Build fixes
PR 2: AESinkSNDIO Implement new sink for openbsd

Thanks in advance.

@fritsch
Copy link
Member

fritsch commented Apr 13, 2017

One thing I have forgotten:
It seems that https://github.com/t6/xbmc/blob/2ad118e791a181ad5261f3e052b176ce6ceb75b5/xbmc/cores/AudioEngine/AESinkFactory.cpp#L136 prioritizes sndio over alsa and therefore linux ALSA users might run into issues if they - by accident have sndio-dev dependencies installed and don't take care when building. Please change it that way that ALSA has priority.

@ghost
Copy link
Author

ghost commented Apr 13, 2017

So if we would merge that one, will you help us to also port this specific sink to the upcoming factory changes?

Sure.

You will become the maintainer of that one after review is finished, right?

Yes, of course.

And last but not least did you run time test it?

Yes.

In general for kodi we make "topic" PRs, e.g. adding one feature, doing exactly one fix and so on.

Sure, I'll open a second PR. It needs to go in after the sink though. Probably best to do it after this is in so I can properly rebase it on top of master.

https://github.com/t6/xbmc/compare/patch-sndio-cmake...t6:patch-freebsd

Please change it that way that ALSA has priority.

Done.

@MartijnKaijser
Copy link
Member

ping @fritsch

@AchimTuran
Copy link
Member

@fritsch is this PR ready for a merge?

@AchimTuran
Copy link
Member

In my view the changes in the sink factory are looking good. Furthermore the sink should not break other platforms as it is not build there.

Jenkins build this please.

@AchimTuran
Copy link
Member

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone May 20, 2017
@MartijnKaijser MartijnKaijser added Component: Audio Type: Feature non-breaking change which adds functionality v18 Leia labels May 20, 2017
@MartijnKaijser MartijnKaijser merged commit 4081cff into xbmc:master May 20, 2017
@AchimTuran
Copy link
Member

@t6
So far I see it was your first PR against our project, right? Thank you very much for your work and contribution. We hope that we can work on more stuff if you want.

uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Oct 13, 2017
multimedia/kodi: Add sndio backend
f294ef7 
This is essentially a backport of [1] which was already merged and
will hopefully be part of the next Kodi release.

[1] xbmc/xbmc#11962

PR:		220231
Approved by:	mickael.maillot@gmail.com (maintainer timeout, 3 months)


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@452024 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Oct 13, 2017
multimedia/kodi: Add sndio backend
ce16d8c 
This is essentially a backport of [1] which was already merged and
will hopefully be part of the next Kodi release.

[1] xbmc/xbmc#11962

PR:		220231
Approved by:	mickael.maillot@gmail.com (maintainer timeout, 3 months)
svmhdvn pushed a commit to svmhdvn/freebsd-ports that referenced this pull request Jan 10, 2024
multimedia/kodi: Add sndio backend
ad47daf 
This is essentially a backport of [1] which was already merged and
will hopefully be part of the next Kodi release.

[1] xbmc/xbmc#11962

PR:		220231
Approved by:	mickael.maillot@gmail.com (maintainer timeout, 3 months)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Component: Audio Type: Feature non-breaking change which adds functionality v18 Leia
Projects
None yet
Milestone
Leia 18.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fritsch @MartijnKaijser @AchimTuran @stefansaraev