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

New API methods not backported to headers repository (Jack1) #628

Open
gnif opened this issue Aug 19, 2020 · 39 comments
Open

New API methods not backported to headers repository (Jack1) #628

gnif opened this issue Aug 19, 2020 · 39 comments

Comments

@gnif
Copy link

gnif commented Aug 19, 2020

The following defines have not been defined in the shared headers repository making it impossible to determine the jack version (1 or 2) at runtime based on the existence of the symbols in the linked library.

jack2/common/jack/jack.h

Lines 63 to 77 in ba28ffa

void
jack_get_version(
int *major_ptr,
int *minor_ptr,
int *micro_ptr,
int *proto_ptr) JACK_OPTIONAL_WEAK_EXPORT;
/**
* Call this function to get version of the JACK, in form of a string
*
* @return Human readable string describing JACK version being used.
*
*/
const char *
jack_get_version_string(void) JACK_OPTIONAL_WEAK_EXPORT;

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

jack_get_version was a jack2 addition that was never pushed to jack1, intentionally.
Such call should be marked deprecated.

@falkTX falkTX closed this as completed Aug 19, 2020
@gnif
Copy link
Author

gnif commented Aug 19, 2020

Sure, that sounds fine, but it should at the minimum be declared in the JACK1 header even if it's just a weak symbol that is never resolved. This then avoids the need to use dlfcn as @kmatheussen is doing to determine version. Instead one could simply do:

if (jack_get_version)
  version = 2;

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

Or alternatively, simply do not use this function. It brings nothing of value to the application or the user.

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@gnif
Copy link
Author

gnif commented Aug 19, 2020

Or alternatively, simply do not use this function.

I am not looking to use the function, but rather checking for its existence to determine if JACK1 or JACK2 is in use. If you provide some other way to detect which version of JACK is in use at runtime, then this problem goes away.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

I understand that, but really seems the wrong solution.
Once pipewire becomes more common-place, this version string will be completely meaningless. I dare say it is already meaningless as it is.

Clients must not care what the jack server version is.
If the client changes behaviour depending on server version, the client is buggy and should be fixed instead.

jack clients target an API, not a specific version of the server/library.
clients need to be able to cope with any server version, or server rewrite, or even new codebase for the server.

if there is some question in regards to the API, that should be clarified instead. (please open a ticket for a question if that is the case)

@gnif
Copy link
Author

gnif commented Aug 19, 2020

Again, we are not trying to determine what version the server is, we don't care what the version string is, the goal here is to determine which client library is in use. As shown by my other issue (#627) there are some critical differences that break the application implementing it depending on which is in use.

If the client changes behaviour depending on server version, the client is buggy and should be fixed instead.

Correct, the flaw exists in the jack client library, so how do we detect which versions on deployed systems are buggy or not? How can we be backwards compatible with broken libraries?

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

The client library is tied to the server version though, a mismatch of client/server is not something that works in the core JACK design.

Best approach here is to fix JACK and try to get people with the updated version.

For instance, Radium crashes when using jack1 because, jack1 by default throws out clients that spends too much time. This can happen, for instance if a plugin misbehaves or a user adds too many plugins, the kernel is not configured properly, too audio programs are running at once, etc. etc.

On this case, you can simply inform the user and provide an action to reconnect to JACK.
LMMS and MusE already do this, maybe others. You can also have a link to some documentation explaining the situation.

it's far more efficient use of time to simply show a warning if using jack1, than to fix Radium to be able to handle behavior which hardly anyone experience

This is a wrong assumption.
Just because it is the behaviour right now, does not mean it will stay that way forever.
The way jack2 is more permissive in terms of "bad" clients (that take too long to process) is something jack2 might eventually change (adopt from jack1). I particularly dislike the fact that jack2 never kills clients no matter what, as it is easy for a rogue client to mess up the entire graph.

There are other jack implementations besides jack1 and jack2 (pipewire is the most common).
Assuming jack2 is the running jack server will hurt client developers in the long term.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

Again, we are not trying to determine what version the server is, we don't care what the version string is, the goal here is to determine which client library is in use. As shown by my other issue (#627) there are some critical differences that break the application implementing it depending on which is in use.

Best approach here is to fix JACK, and then try to get this fixed version to users ASAP.
That way everyone benefits.

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@gnif
Copy link
Author

gnif commented Aug 19, 2020

Best approach here is to fix JACK, and then try to get this fixed version to users ASAP.
That way everyone benefits.

That is obvious, so how do we go about detecting a user is running an older/broken version of the client libraries? How do we even inform them that there is an issue and they need to upgrade?

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@gnif
Copy link
Author

gnif commented Aug 19, 2020

If jack2 starts behaving like jack1 by default I will consider stop using jack

If jack2 starts behaving like this also it will lead to removing support from QEMU also as this kind of behaviour can not be tolerated.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

Noted.

Thing is.. even if we now add such function to jack1, linux distributions will not have it for many years...
And even with that, the JACK version is not useful as-is.

real case: jack2 is now at 1.9.14, the development version.
this version will remain until it is released, and then a new version will be set for development.
say we fix the bug that causes a crash, by next week.
then 1.9.14 is released in 1 month.
so... now your client gets the version and sees 1.9.14... what do you do? there is no way to know if the fix is there or not.

@gnif
Copy link
Author

gnif commented Aug 19, 2020

We can determine that the new get version method is not implemented and safely assume that its a version older than 1.9.14. You have to start somewhere

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

Actually, Radium crashes. It was a complicated bug. Easier just to inform about not using jack1. If jack2 starts behaving like jack1 by default I will consider stop using jack

If jack2 starts behaving like this also it will lead to removing support from QEMU also as this kind of behaviour can not be tolerated.

Sorry to say, but your application code is broken.
Clients need to be able to cope with jack shutdown callback. it is part of the API for a reason.
Just because jack2 does not call it, is meaningless here. jack2 is the bad actor, not jack1.

@gnif
Copy link
Author

gnif commented Aug 19, 2020

If we have runtime access to the client version it will at least allow us to write it to a log and when users report crashes we can correlate what changed that caused things to break, even if it's not a bug in jack itself.

Clients need to be able to cope with jack shutdown callback. it is part of the API for a reason.

Noted, which is why today a patch set has been developed to work around the bad behavior in JACK2, however changing something so fundamental so late into the game is going to cause problems for many people.

We will start getting reports of audio dropouts, even if it can recover, and won't know what the cause was because we can't tell what version of libjack is in use on the user's PC. People that had fully working systems/configurations suddenly will break and not be able to identify why.

If you really must make JACK2 evict bad/slow clients, make it an optional parameter of the server at startup.

BTW: I fully understand why you would want to evict slow clients, the Looking Glass project has the same requirement for latency reasons, but to force this on people just because you feel it's how things should be is not acceptable as many (including myself) can cope with a hung/slow client as we are not using JACK for production/studio audio.

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

We will start getting reports of audio dropouts, even if it can recover, and won't know what the cause was because we can't tell what version of libjack is in use on the user's PC.

I was kinda agreeing with you until this part.
What do dropouts have to do with the libjack version the user is running?
If you application has many dropouts, I suggest rethinking how it handles to audio so it can minimize those.

It does handle jack shutdown callback normally. But when jack1 wants to throw out the client, which it should never do BTW, a weird crash happens.

JACK is not the one crashing, your application is.
When JACK triggers the shutdown callback, no more processing will happen.
The "client" pointer is also no longer valid. Trying to do something with the client pointer after this is surely a bad idea.

Sure, that's your opinion, but it creates various problems. Why would a user like to restart the program or reconnect jack? It's fine getting a notification that the program spends too much time processing audio, but why can't the program just clean up by itself?

This is not a matter of opinion, the original design was made that way on purpose.

From what I understand, the big reason jack2 is more permissive in terms of "bad" clients is because of Windows and its lack of realtime capabilities which means sometimes clients can take a big amount of time to do audio processing properly again.

Note that JACK will do nothing (in terms of shutdown) if clients behave properly.

The issue here is that a single bad client can make the entire audio graph misbehave, which can result in, at least, some terrible audio/noise.
Finding that specific bad client is not always easy, depending on the user setup.

@gnif
Copy link
Author

gnif commented Aug 19, 2020

What do dropouts have to do with the libjack version the user is running?

You misunderstand, I am stating that if you make it start evicting clients due to being too slow, the recovery process will cause an audio dropout. You may not be aware, QEMU is used to run Virtual Machines that emulate sound devices. As you can imagine, being super low latency consistently is a difficult thing to do when CPUs are being shared with the VM. The occasional & rare, latency spike is to be expected.

The "client" pointer is also no longer valid. Trying to do something with the client pointer after this is surely a bad idea.

Where is this documented? Jack2 is still valid until you try to re-connect. If you don't close you get a warning that you didn't and it's "cleaning up"?

I digress though, this issue is about obtaining the jack client library version in use... surely you can see that this is such a simple thing to require if only for debugging purposes.

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

You misunderstand, I am stating that if you make it start evicting clients due to being too slow, the recovery process will cause an audio dropout.

Ah yes, that makes sense. Sorry for the confusion

You may not be aware, QEMU is used to run Virtual Machines that emulate sound devices. As you can imagine, being super low latency consistently is a difficult thing to do when CPUs are being shared with the VM. The occasional & rare, latency spike is to be expected.

I expected to be like that. This is why I said "If you application has many dropouts, I suggest rethinking how it handles to audio so it can minimize those".
Likely you are already doing so, but a "simple" ringbuffer model where you pull data if ready and just write to the jack ports should work in this case. There will be some little added latency, but in turn no xruns (in normal circumstances).
The Firefox JACK code uses a similar approach, as many others I would imagine.

The "client" pointer is also no longer valid. Trying to do something with the client pointer after this is surely a bad idea.

Where is this documented? Jack2 is still valid until you try to re-connect. If you don't close you get a warning that you didn't and it's "cleaning up"?

Actually, I was wrong. Sorry again.

https://jackaudio.org/api/types_8h.html#a1f461fe99711af7edcb1b73b1315fbb3
states that the client structure is not deallocated by jack.

but still, trying to do anything with this client pointer after this point does not seem like a good idea.

I digress though, this issue is about obtaining the jack client library version in use... surely you can see that this is such a simple thing to require if only for debugging purposes.

I get it.. likely will be done anyway since JACK headers eventually should be shared between jack1 and jack2.
But I really do not want to give the impressions that this info can be used for anything other than purely stateless information and debugging.

@gnif
Copy link
Author

gnif commented Aug 19, 2020

Likely you are already doing so, but a "simple" ringbuffer model where you pull data if ready and just write to the jack ports should work in this case.

Yes, we are already doing so which is why it is working so well. By no means am I stating we have an issue here currently.

but still, trying to do anything with this client pointer after this point does not seem like a good idea.

Calling close after open seems like a sane thing to do, just like every other library. Ie, fopen succeeds, fwrite fails because the disk is full, the application is still expected to call fclose. In no circumstances should calling close cause any kind of fault when the pointer from the application's point of view is still open.

But I really do not want to give the impressions that this info can be used for anything other than purely stateless information and debugging.

Sure, by no means do I suggest that version-specific workarounds be the norm either, but at the end of the day if there is a bug in the library and the library won't see an update for months to years on a target distribution that breaks a product so completely, yes a workaround based on version may be required.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

Clearly jack1 should not delete the client pointer by itself before the program has had a chance to clean up, that's just horrible.

jack1 does not delete the pointer, I was wrong in my assumption.
See https://jackaudio.org/api/types_8h.html#a1f461fe99711af7edcb1b73b1315fbb3

So even if the original design is broken, it's still the correct behavior because it behaves according to the original design? :-)

Terrible way to try to prove your point.
The answer is no, if the original design was broken, jack2 should not follow it.
(and there is precedence for it, dont remember all details now but the jack1 vs jack2 port rename functions were inconsistent at some point. jack1 was wrong and jack2 was right. so jack1 was eventually fixed to follow what jack2 did)

In this case though, zombifying/shutting-down a client because it is taking longer than the client timeout set on jack server start is a feature that has been in jack (the API) since the start.
jack2 still has some leftovers from this, in the man page you can find:

       -t, --timeout int
              Set client timeout limit in milliseconds.  In realtime mode the client timeout must be smaller than the watchdog timeout (5000 msec).  (default: 500)

@kmatheussen
Copy link
Contributor

kmatheussen commented Aug 19, 2020 via email

@gnif
Copy link
Author

gnif commented Aug 19, 2020

jack1 does not delete the pointer, I was wrong in my assumption.
See https://jackaudio.org/api/types_8h.html#a1f461fe99711af7edcb1b73b1315fbb3

To be clear, jack2 does if you call jack_client_open which is what issue #627 is about.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

but still, trying to do anything with this client pointer after this point does not seem like a good idea.

Calling close after open seems like a sane thing to do, just like every other library. Ie, fopen succeeds, fwrite fails because the disk is full, the application is still expected to call fclose. In no circumstances should calling close cause any kind of fault when the pointer from the application's point of view is still open.

I do not want to go on too much on this, but while I generally agree with you, those file handling functions do not provide a "close/shutdown" callback like jack does.
if fopen needed a function to inform the program of some file access issue, this would be more comparable.
In any case, the API says that the client side still needs to call jack_client_close, so it does in fact behave like fopen and fclose yes.

Sure, by no means do I suggest that version-specific workarounds be the norm either, but at the end of the day if there is a bug in the library and the library won't see an update for months to years on a target distribution that breaks a product so completely, yes a workaround based on version may be required.

Yeah, I really don't like it, but being pragmatic I see sometimes there is just no other way.

Alternatively (and this is very ugly, even more so than reading jack client version!!) one could read the contents of /dev/shm/ on linux, and somewhat figure out the jack version from there.
"db" files there mean the jack server supports meta-data, so it is at least 1.9.12 or the jack1 variant that added that part (dont recall versions now)

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

In my opinion, everything broken design should ideally be thrown out

I agree.

and every program should include it's own version of jack to prevent mismatch between server and client.

This cannot happen and it does not scale anyway.
So that broken design needs to be thrown out ;)

@gnif
Copy link
Author

gnif commented Aug 19, 2020

I think you're confusing protocol version with library version. The fix for issue #623 would not change the protocol. It seems you're treating protocol version and library version as one and the same thing, which IMO seems very odd.

@kmatheussen

This comment has been minimized.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

how so?

protocol changes when data structure changes. library version is just the jack release, which is more or less arbitrary.

thing is, with jack2, the data structure can change even within the same protocol and jack version.
all it takes is building jack2 with a different configuration.
with or without metadata, number of max clients, number of max ports... these things change the data structure of both client and server libraries :/

@gnif
Copy link
Author

gnif commented Aug 19, 2020

how so?

Removing the following code would not affect the compatibility of the library with the server:

if (!JackGlobals::fServerRunning && fClientCount > 0) {
// Cleanup remaining clients
jack_error("Jack server was closed but clients are still allocated, cleanup...");
for (int i = 0; i < CLIENT_NUM; i++) {
JackClient* client = JackGlobals::fClientTable[i];
if (client) {
jack_error("Cleanup client ref = %d", i);
client->Close();
delete client;
}
}
// Cleanup global context
fClientCount = 0;
delete fGlobals;
fGlobals = NULL;
}

library version is just the jack release, which is more or less arbitrary.

It shouldn't be, it should be versioned independently for exactly this use case.

all it takes is building jack2 with a different configuration.
with or without metadata, number of max clients, number of max ports... these things change the data structure of both client and server libraries :/

Understood, but this is not the issue we are addressing here.

@cschoenebeck
Copy link

Sure, by no means do I suggest that version-specific workarounds be the norm either, but at the end of the day if there is a bug in the library and the library won't see an update for months to years on a target distribution that breaks a product so completely, yes a workaround based on version may be required.

Yeah, I really don't like it, but being pragmatic I see sometimes there is just no other way.

Alternatively (and this is very ugly, even more so than reading jack client version!!) one could read the contents of /dev/shm/ on linux, and somewhat figure out the jack version from there.
"db" files there mean the jack server supports meta-data, so it is at least 1.9.12 or the jack1 variant that added that part (dont recall versions now)

I agree with @gnif here; there should be a way for clients to check the version i.e. by adding jack_get_version() on JACK1 side. Of course potential issues should be fixed in JACK1/JACK2 respectively, but it is not useful to prohibit clients from taking measures by checking a version range to circumvent issues.

I would even go further and say, every public JACK API function added on one side, should be added on the other side as well, i.e. the API headers should be truly shared. That's the point of the weak API. Because once again being realistic: the status quo between JACK1 vs. JACK2 exists for more than 10 years now, it is not likely to disappear in favour of only one side any time soon.

@falkTX
Copy link
Member

falkTX commented Aug 19, 2020

every public JACK API function added on one side, should be added on the other side as well, i.e. the API headers should be truly shared

That is one of my goals.

the status quo between JACK1 vs. JACK2 exists for more than 10 years now, it is not likely to disappear in favour of only one side any time soon.

jack2 has been the favorite since quite a while now, only a small amount of people still use jack1.
as the current maintainer of both, I have stated previously that jack1 is in purely maintenance mode.
https://jackaudio.org/news/2017/12/21/jack2-v1-9-12-release-and-future-plans.html

I will mention this again soon, likely next month, where/when I plan to call for testing regarding new osx/win binaries and a bit of help for low-hanging tasks.

@gnif
Copy link
Author

gnif commented Aug 19, 2020

I have stated previously that jack1 is in purely maintenance mode.

I for one will be glad to see JACK1 be officially deprecated simply due to the confusing nature of setting up jack for beginners.

@cschoenebeck
Copy link

every public JACK API function added on one side, should be added on the other side as well, i.e. the API headers should be truly shared

That is one of my goals.

That's great! Thanks!

the status quo between JACK1 vs. JACK2 exists for more than 10 years now, it is not likely to disappear in favour of only one side any time soon.

jack2 has been the favorite since quite a while now, only a small amount of people still use jack1.

I believe it when it became reality. ;) AFAICS the situation is not (yet) much different than it was years ago. There was a common consensus in favour of JACK2 before, and was discarded later on. And the small amount of people still backing JACK1 are actually influential ones.

As long as new features are pushed onto JACK1 tree, the overall situation will not get resolved.

as the current maintainer of both, I have stated previously that jack1 is in purely maintenance mode.
https://jackaudio.org/news/2017/12/21/jack2-v1-9-12-release-and-future-plans.html

I think the following should probably be updated as well then:
https://github.com/jackaudio/jackaudio.github.com/wiki/Differences-between-jack1-and-jack2

Quote:

Jack 2 was originally planned to replace Jack 1, but this is no longer so and they are considered equivalent implementations.

@falkTX
Copy link
Member

falkTX commented Oct 12, 2020

I am reopening this, since we should strive for feature parity between jack versions.
JACK1 is no longer being worked on, but yeah, stable API/ABI an essential fix to do.

@falkTX falkTX reopened this Oct 12, 2020
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

4 participants