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.

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

Add JACK Audio support #3396

Merged
merged 13 commits into from Jul 2, 2018

Conversation

@buscher
Copy link
Contributor

commented Apr 14, 2018

Hello,

I took the jack patch from #137 and tried to fix/advance it.

Main Changes:

  • Added stereo output support
  • Configurable output devices (Mono/Stereo). We can talk about the names, as strictly speaking "Mono" or "Stereo" is not an output device.
  • EDIT: Jack port auto connect is disabled by default and can be enabled in ~/.config/Mumble/Mumble.conf
    [jack] autoconnect=true
  • EDIT: the jack server starts by default on mumble start (if not already running). This can be disabled in ~/.config/Mumble/Mumble.conf
    [jack] startserver=false

Bugfixes:

  • Jack input/output was always running (even if not selected) and causing crashes when interacting with a different input/output source. This fixes the crash when changing the output/input from jack to alsa and vice versa.
  • Fixed possible crash on shutdown
  • Fixed possible crash in case of jackd reconfiguration (for example the buffer size)

Those changes were tested with:

  • jack-1.9.11_rc1 and jack-1.9.12 (-> jack2)
  • switchting/mixing alsa&jack input/output source
  • several runs in debug mode with ASAN support (not in this pull request)

UPDATE:

  • Added autconnect (default off) and autostart (default on)
  • Added more verbose error checking
@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

Do I have to care about the failing mac build?
/Users/travis/.travis/job_stages: line 166: shell_session_update: command not found
hardly seems related to my changes, or?

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Apr 15, 2018

Hello,

Thank you very much for your hard work!

Don't worry about the macOS build, its failure is not related to your changes.

I triggered a new build right now: https://travis-ci.org/mumble-voip/mumble/jobs/366461124

@mkrautz

This comment has been minimized.

Copy link
Member

commented Apr 22, 2018

Hi, I had also started updating it (as you can see from the last comments on #137)

https://github.com/mkrautz/mumble/compare/jack_v2

I don't recall how much changed in my branch, though!

@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

Hm... pretty nice. You got some more errors checks and a nice config trick ;-)
I could incorporate the checks and bring back (with a config option) the auto connect & autostart.

Would that make it "more likely" to get merged? :)
Or what is the best way to proceed here? I don’t want 2 jack patches fighting over a merge :D

@mkrautz

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Please incorporate as much as you can from my branch. There is no competition. Since you picked up the feature, your PR is on track to get merged more than my branch. I’ll take a closer look once time permits.

@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2018

I included your jack port autoconnect and verbose error checking :)

@@ -754,7 +754,11 @@ int AudioInput::encodeCELTFrame(short *psSource, EncodingOutputBuffer& buffer) {

cCodec->celt_encoder_ctl(ceEncoder, CELT_SET_PREDICTION(0));

#if defined(CELT_SET_VBR) && !defined(CELT_SET_VBR_RATE)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Apr 29, 2018

Member

This should not be part of this PR.

Mumble requires at least CELT 0.7 for compatibility with other clients. It can also be built against CELT 0.11.

If you need preprocessor magic like this, something is wrong in your build environment.

Mumble ships with CELT 0.7 and CELT 0.11 in Git or in its source tarballs.

You can ensure you use the bundled verisons by passing CONFIG+=bundled-celt to qmake.

This comment has been minimized.

Copy link
@buscher

buscher Apr 29, 2018

Author Contributor

You are right, I removed it :)

@buscher buscher force-pushed the buscher:mumble-jack branch from e596580 to e814995 Apr 29, 2018

@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Can I do anything to speed up a possible review? :)

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

@buscher Rebase your branch, so that the Mac OS build on Travis CI doesn't fail anymore. :)

@buscher buscher force-pushed the buscher:mumble-jack branch from e814995 to d89f5bc Jul 1, 2018

@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Done. What else can I do? :)

#include "Global.h"


static JackAudioSystem * jasys = NULL;

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

We usually append the asterisk to the variable's name.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::close_jack() {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra space.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Sorry, I don't really understand this one.
Do you want me to make an extra space? where? or is it about the extra new empty line?
or is there an extra space (which I do not seem to find) which I should remove?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

It's about the extra empty line (231).

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::init_jack() {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

int JackAudioSystem::process_callback(jack_nframes_t nframes, void *arg) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

int JackAudioSystem::srate_callback(jack_nframes_t frames, void *arg) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::allocOutputBuffer(jack_nframes_t frames) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::setNumberOfOutPorts(unsigned int ports) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

unsigned int JackAudioSystem::numberOfOutPorts() const {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

int JackAudioSystem::buffer_size_callback(jack_nframes_t frames, void *arg) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::shutdown_callback(void *arg) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioOutputRegistrar::setDeviceChoice(const QVariant &choice, Settings &s) {

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Extra empty line.

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done



void JackAudioSystem::auto_connect_ports()
{

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

The bracket should be at the end of the line above (to match the style across the file).

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

}

void JackAudioSystem::activate()
{

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

The bracket should be at the end of the line above (to match the style across the file).

This comment has been minimized.

Copy link
@buscher

buscher Jul 1, 2018

Author Contributor

Done

@davidebeatrici davidebeatrici changed the title Add jack support Add JACK Audio support Jul 1, 2018

@davidebeatrici davidebeatrici force-pushed the buscher:mumble-jack branch from e845a57 to 32988da Jul 1, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

I did some changes to the commits.

  • I set the first one's author to @feld, creator of #137.
  • I reworded the majority of them, to maintain consistency (Jack: <message>).
  • I squashed the last two ones.
static JackAudioSystem *jasys = NULL;

// jackStatusToStringList converts a jack_status_t (a flag type
// that can contain multiple Jack statuses) to q QStringList.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 1, 2018

Member

Typo: q -> a

@davidebeatrici davidebeatrici force-pushed the buscher:mumble-jack branch 2 times, most recently from d46dfdf to 67c4d89 Jul 1, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

@buscher Feel free to amend the commits, if you want to set yourself as committer.

I tested the pull request and everything works as expected, thank you very much for your awesome work!

Jack: adapt codestyle
- move asterisk near variable name
- change order of const
- remove extra empty lines
- move opening brackets to the same line as the function signature

@davidebeatrici davidebeatrici force-pushed the buscher:mumble-jack branch from 67c4d89 to a0e5e12 Jul 2, 2018

@davidebeatrici davidebeatrici force-pushed the buscher:mumble-jack branch from f48e7a0 to 702c8a4 Jul 2, 2018

@buscher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

Nah. The last commits are yours, thanks for the review. Feel free to merge ;-)

@davidebeatrici davidebeatrici merged commit 1bf549d into mumble-voip:master Jul 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Thank you again for your hard work!

@davidebeatrici davidebeatrici referenced this pull request Jul 2, 2018
@mfr-itr

This comment has been minimized.

Copy link

commented Jul 2, 2018

Nice! When do you think this would be released? I see that the last release was 1.5y ago.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

As soon as possible, it's time to release 1.3.

#2728

@AnonFuncsAreAwesome

This comment has been minimized.

Copy link

commented Jul 2, 2018

I created an AUR package with the patch applied to latest dev snapshot for the time being, just in case anyone is interested: mumble-snapshot-jack

@mfr-itr

This comment has been minimized.

Copy link

commented Jul 3, 2018

Nice, thanks

@ranomier

This comment has been minimized.

Copy link

commented Jul 3, 2018

@timbiker

This comment has been minimized.

Copy link

commented Feb 10, 2019

Hi,
Thanks a lot for this great add-on !!

Is there any way to configure the name that mumble gives to its jack connector ?
My purpoose is to feed multiple mumble channel from one PC, which has a 32 channels Jack audio running, and so that each mumble instance as a specific name which gets automatically connected by jack.plumbling ?

Thanks,
Timothé

@Natenom

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

From mumble --help:

-jn, --jackname
Set custom Jack client name.

If you do not define jn, than it is mumble, mumble-01, mumble-02, etc.

@timbiker

This comment has been minimized.

Copy link

commented Feb 10, 2019

thanks, will try as soon as I can.
(weird, I did try the --help but it did not work on my compiled version :-( )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.