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

jack2 on darwin fails when client name exceeds 27 characters #474

Open
hlolli opened this issue Jun 5, 2019 · 29 comments
Open

jack2 on darwin fails when client name exceeds 27 characters #474

hlolli opened this issue Jun 5, 2019 · 29 comments

Comments

@hlolli
Copy link

hlolli commented Jun 5, 2019

There seems to be a difference to how long the client name can be from linux to darwin. I'm able to exceed client names with well over 32 characters on linux. But on OsX I get Cannot open 0iiiiiiiiiiiiiiiiiiiiiiiii12 client but deleting 1 character here (the number 2) it works just fine. To be more precise, I believe the limit is somewhere hard set to 32 with the clientName:inputPort (colon input-port). I compiled jack2 from develop branch last week. If it were possible, I'd love to see the clientName limit a bit higher 128 or preferably 256.

I believe the error is coming from here https://github.com/jackaudio/jack2/blob/develop/common/JackLibClient.cpp#L124-L127

@falkTX
Copy link
Member

falkTX commented Jun 5, 2019

Most developers that use jack are not running osx, so these things take a while until they are noticed.
Since you can see the issue directly, do you mind creating a PR with a fix please?
Thanks

@hlolli
Copy link
Author

hlolli commented Jun 5, 2019

I'll try few things, see if it gets fixed. Happy to contribute if I can :)

@hlolli
Copy link
Author

hlolli commented Jun 5, 2019

Btw, I relize now that I did your Carla workshop in Berlin last summer 💯

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

Before I PR something silly, I think I've located the error to the fact that 32+ characters actually cause the semaphores to crash on darwin:

if ((fSemaphore = sem_open(fName, O_CREAT | O_RDWR, 0777, value)) == (sem_t*)SEM_FAILED) {

I researched unnamed semaphores, which would make sense because the sem_t* fSemaphore is a unique pointer (don't see anywhere where a semaphore is opened from a different process, but I could be wrong). But didn't find any solution. Alternative would be just to substring the semaphore name, but it could be super dangerous if two processes open the same semaphore. How would you suggest to go about solving this, if solving this at all? @falkTX

@sletz
Copy link
Member

sletz commented Jun 7, 2019 via email

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

hash the string on darwin maybe?

@kmatheussen
Copy link
Contributor

kmatheussen commented Jun 7, 2019 via email

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

I'm not sure, I'm actually a linux user and on mac I don't have the gdb (I'm sure there's an alternative). I think the sem_open takes the whole string as it is, and just crashes (returns a (sem_t*)SEM_FAILED)

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

given that I remove the sprintf statement, but let me look again, I've been reverting this file so many times...

@kmatheussen
Copy link
Contributor

kmatheussen commented Jun 7, 2019 via email

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

sorry for that noise, I just deleted it again, the sprintf statement, and it successfully runs with a 100+ char jack_client name. I must have been out of coffee 2 days ago :)

I'll PR that deletion

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019

maybe keep the js_ append?

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019 via email

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019 via email

hlolli added a commit to hlolli/jack2 that referenced this issue Jun 7, 2019
@falkTX
Copy link
Member

falkTX commented Jun 7, 2019

might be worth investigating if using the client uuid for the semaphore name is an option or not.
it is just an integer that gets incremented for every new client, so it will be small in size (string-wise)

hlolli added a commit to hlolli/jack2 that referenced this issue Jun 7, 2019
@hlolli
Copy link
Author

hlolli commented Jun 7, 2019 via email

@hlolli
Copy link
Author

hlolli commented Jun 7, 2019 via email

@falkTX
Copy link
Member

falkTX commented Oct 16, 2020

this is fixed now on 1.9.15/16, please give it a try.
the semaphore is based on the client name, but not restricted by it. at least in theory

@HaHeho
Copy link

HaHeho commented Oct 16, 2020

The new limitation seems to be 63 characters. Can somebody else confirm?

With 64 and above, my Python implementation still crashes in the same ungracious manner as before. But at least there is also a clear error message.

"0123456789012345678901234567890123456789012345678901234567890123" is too long to be used as a JACK client name.
Please use 63 characters or less
JackShmReadWritePtr1::~JackShmReadWritePtr1 - Init not done for -1, skipping unlock
Traceback (most recent call last):
  ...
jack.JackOpenError: Error initializing "0123456789012345678901234567890123456789012345678901234567890123": <jack.Status 0x21: failure, server_error>

In that way, I still have to catch and truncate extended names myself. Is that reasonable?

If this confirmed, I assume we could also modify the Python API to do the truncation, so any downstream implementation does not have to care?!

@falkTX
Copy link
Member

falkTX commented Oct 17, 2020

Is JACK crashing or your application? (due to expecting to open a client and not dealing with the failure)

You can use jack_client_name_size before creating the client, see https://jackaudio.org/api/group__ClientFunctions.html#gab8b16ee616207532d0585d04a0bd1d60

I did not check this, but it should be returning 63 on macOS (otherwise it is a bug)

@HaHeho
Copy link

HaHeho commented Oct 17, 2020

Is JACK crashing or your application? (due to expecting to open a client and not dealing with the failure)

Just the application.

You can use jack_client_name_size before creating the client, see https://jackaudio.org/api/group__ClientFunctions.html#gab8b16ee616207532d0585d04a0bd1d60

I did not check this, but it should be returning 63 on macOS (otherwise it is a bug)

Good to know. I will check on how to access the API function in order to truncate names during startup.

@h1z1
Copy link

h1z1 commented Nov 6, 2020

Can you clarify if exceeding this limit is meant to be an error or is jackd supposed to truncate it internally?

It is a hard fail in JackInternalClient and JackLibClient for example.

ffmpeg appears to assume otherwise as it blindly uses the current source url as a name by default

    /* Register as a JACK client, using the context url as client name. */
    self->client = jack_client_open(context->url, JackNullOption, &status);
    if (!self->client) {
        av_log(context, AV_LOG_ERROR, "Unable to register as a JACK client\n");
        return AVERROR(EIO);
    }

Though it has a fallback, cubeb which is the backend for audio in Firefox also assumes that:

  const char * jack_client_name = "cubeb";
  if (context_name)
    jack_client_name = context_name;


  ctx->jack_client = api_jack_client_open(jack_client_name,
                                          JackNoStartServer,
                                          NULL);

@falkTX
Copy link
Member

falkTX commented Nov 6, 2020

This is meant to be a hard fail yes.
The client name must not be greater than the value returned by jack_client_name_size()

Documentation for jack_client_open says:

client_name: of at most jack_client_name_size() characters

applications need to be aware of that.
carla for example takes care of this in multi-client mode by always generating a name that is very likely to be unique, within the limits of the client name size. see https://github.com/falkTX/Carla/blob/master/source/backend/engine/CarlaEngine.cpp#L1048

@h1z1
Copy link

h1z1 commented Nov 6, 2020

Based on the crude searching I did yesterday, Carla is a rarity. Most applications do not. The assume jack will truncate it.

@falkTX
Copy link
Member

falkTX commented Nov 6, 2020

In some cases yes, but that is a bad assumption to make.
Linux and macOS should be able to cope with this, but Windows might be the last one needing some attention.

Even then, if application does not take the client name size into consideration, it likely will also not take into account that the name assigned to it by jack might be different from what was requested (this is why an open flag exists for asking a unique name)

@h1z1
Copy link

h1z1 commented Nov 6, 2020

From what I can find they don't though.. not even what I'd consider the the three major media players in Linux do

App         | Lib       | jack_client_open  | jack_client_name_size | file
--------------------------------------------------------------------------

audacity    | Portaudio | yes               | yes
lib-src/portaudio-v19/src/hostapi/jack/pa_jack.cjack_client_open
========================================================
PaJack_SetClientName->jack_client_name_size

Carla   | own   | yes | Yes
============================

Patchage | own  | yes | NO
============================

freqtweak  | own | yes |  NO
============================
   const char *server_name = NULL;

baudline | own | yes | no
============================

jackd-jamin | own | yes | NO
============================
  - has specific warn that jackd might change the name
  char *client_name = NULL;
  client = jack_client_open(client_name, JackNullOption, &status);

alsa jackplugin | own | yes | NO
============================
  - has warning
  char jack_client_name[32];

libsdl | own | yes | NO
============================
 - hard coded to "SDL"
 - warning re unimplemented name api

 client = JACK_jack_client_open("SDL", JackNoStartServer, &status, NULL);

OBS | own | yes | NO
============================
 char *device;


ffmpeg | libavdevice | yes | NO
==================================
  libavdevice/jack.c: self->client = jack_client_open(context->url, JackNullOption, &status);


mpv-native | own | yes | NO
============================
char *client_name;
  p->client = jack_client_open(p->opts->client_name, open_options, NULL);


mpv-libavdevice | libavdevice | yes | NO
=========================================

VLC | own | yes | NO
========================================================
    p_sys->p_jack_client = jack_client_open( psz_name,
                 JackNullOption | JackNoStartServer,
                 NULL );

GStreamer | own | yes | NO
========================================================
    src_client = jack_client_open ("src_client", JackNoStartServer, &status);


@falkTX
Copy link
Member

falkTX commented Nov 6, 2020

not all of those need to care though. if the default client name is just "mpv" or something small, they will not run into issues.
the problem is only when it allows user input without checking further.

@h1z1
Copy link

h1z1 commented Nov 6, 2020

Indeed and it's mpv's case I noticed it. You can specify the name with --jack-name and --jack-port. Firefox is able to cause some serious instability in both jackd and attached clients too.

Couple other weird things about this, can you confirm the intent behind JackUseExactName given the above?

 @param client_name of at most jack_client_name_size() characters.
 * The name scope is local to each server.  Unless forbidden by the
 * @ref JackUseExactName option, the server will modify this name to create a unique variant, if needed.

I can't find any reference to jack_client_name_size being used in the example applications either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants