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 port id to port name to avoid collisions #5

Merged
merged 1 commit into from Sep 24, 2019
Merged

Add port id to port name to avoid collisions #5

merged 1 commit into from Sep 24, 2019

Conversation

romsom
Copy link
Contributor

@romsom romsom commented Sep 24, 2019

This patch changes the pattern how jack port names are derived from alsa
port names when using unique port names. It adds the port id after the
':' to make sure port names are really unique, even when the alsa ports
are not (or too long).
This fixes problems with the ploytec gm5 which has up to 5 in/out ports
and quite long client/port names.

This patch changes the pattern how jack port names are derived from alsa
port names when using unique port names. It adds the port id after the
':' to make sure port names are really unique, even when the alsa ports
are not (or too long).
This fixes problems with the ploytec gm5 which has up to 5 in/out ports
and quite long client/port names.
@dvzrv
Copy link
Collaborator

dvzrv commented Sep 24, 2019

@romsom thanks for your pull request.

I have some questions to be able to understand it though: Is this just a readibility fix, or does it prevent actual name collision?
Do the names (generated by a2j_port_fill_name()) get truncated, because they are too long (as described in your use-case) and therefore collide or do the port names, as retrieved by snd_seq_port_info_get_name(), get truncated?

@romsom
Copy link
Contributor Author

romsom commented Sep 24, 2019

Thanks for picking up further development for a2jmidid! :)

The limitation in length comes from g_max_jack_port_name_size.
If the sum of the components of the string are too long, snprintf will only write g_max_jack_port_name_size bytes into port_ptr->name.
If ports names have a common prefix (e.g. they differ only in an index at the last characters of the alsa port name), that can cause different ports to have the same name (port_ptr->name).

Later in a2j_port_create that name will be used to register a jack port with jack_port_register, which requires unique port names per client:
http://jackaudio.org/files/docs/html/group__PortFunctions.html#ga3e21d145c3c82d273a889272f0e405e7

If they are not unique further jack_port_create will fail and only one port will have been created.

That happend to me with the GM5x5x5, so I added the (unique) port id at an earlier position in the name.

@dvzrv
Copy link
Collaborator

dvzrv commented Sep 24, 2019

Thanks for the explanation!

@dvzrv dvzrv merged commit aeee960 into jackaudio:master Sep 24, 2019
@nedko
Copy link
Contributor

nedko commented Aug 20, 2022

This commit breaks existing sessions by changing port names. The commit will be kept out of https://github.com/LADI/a2jmidid until there is non-breaking way to achieve the same. In theory at least the existing port uniqueness feature should have fixed the issue without this commit.

@romsom
Copy link
Contributor Author

romsom commented Sep 6, 2022

What "port uniqueness feature" is that so I can verify with my setup?
Has it been added recently?
I can't see how this problem could be fixed without changing the port name scheme. Allowing longer port names would help, but the length can't be controlled by the client and there could always be port names which are too long for a specific maximum length.

How about we only include the index for clients with more than one port (inputs and outputs handled separately of course)?
That way only sessions with such clients would break.
The only full mitigation I can think of, is providing a way to patch port names in sessions to a format which doesn't clash.

@nedko
Copy link
Contributor

nedko commented Sep 6, 2022

The "port uniqueness feature" is enabled when make_unique is true. It is from 2008 and you are tweaking it.

A hypothetical to-be-made change-set can turn the make_unique and g_disable_port_uniqueness variables from bool to 3-state uint/enum along with extended in backward compatible way command-line and D-Bus interface. This way existing sessions will not break unless the user enables the extended uniqueness (via the new extended configuration interface), which can be done via semi-automatic upgrade process of the used a2jmidid (D-Bus or as command-line string) configuration, including changing of a2jmidid port names in session data (ladish or not).

UPDATE: ladishd actually uses the a2jmidid's map_jack_port_to_alsa() D-Bus method : https://github.com/LADI/ladish/blob/stable/daemon/virtualizer.c#L616 and thus should not be affected by this. Session managers that don't use the dynamic mapping functionality will still break tho.

@romsom
Copy link
Contributor Author

romsom commented Sep 6, 2022

I can look into the 3-state option, but maybe a clean break will motivate other session managers to use map_jack_port_to_alsa().
Breaking existing setups is always a call for anger. I know I hate it when it happens to me. But on the other hand there are setups which don't work at all with the old behavior.

How about making the old behavior a fall-back option instead of the default? So those who have setups that rely on static port names in the old format can still make it work easily, but we don't hinder adoption of the IMO better format by hiding it behind a non-default option.

Let me know what you think and what you need to be able to merge the format change.

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

Successfully merging this pull request may close these issues.

None yet

3 participants