Skip to content

Commit

Permalink
Better port type handling
Browse files Browse the repository at this point in the history 
- Added inline helper methods to create ports of the correct type
     - Had to move `jack_port_uuid` to the top because we still can't use methods declared below in 2019
    - I'd like to add helper methods that automatically set the metadata but C has no method overloading?!
- Improved metadata documentation
- Registering a port with the `JACK_DEFAULT_MIDI_TYPE` will automatically change it to `JACK_DEFAULT_MESSAGE_TYPE` for forward  compatibility.
  • Loading branch information
@piegamesde
piegamesde committed Oct 12, 2019
1 parent 1c3f811 commit cf6523f
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 15 deletions.
13 changes: 12 additions & 1 deletion common/JackAPI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1094,9 +1094,20 @@ LIB_EXPORT jack_port_t* jack_port_register(jack_client_t* ext_client, const char
} else if ((port_name == NULL) || (port_type == NULL)) {
jack_error("jack_port_register called with a NULL port name or a NULL port_type");
return NULL;
}

if (strcmp(port_type, JACK_DEFAULT_AUDIO_TYPE) == 0) {
// Don't change anything
} else if (strcmp(port_type, JACK_DEFAULT_MESSAGE_TYPE) == 0) {
// Don't change anything
} else if (strcmp(port_type, JACK_DEFAULT_MIDI_TYPE) == 0) {
// Rename the old port type
port_type = JACK_DEFAULT_MESSAGE_TYPE;
} else {
return (jack_port_t *)((uintptr_t)client->PortRegister(port_name, port_type, flags, buffer_size));
jack_info("jack_port_register called with a non-default port_type: '%s'.", port_type);
}

return (jack_port_t *)((uintptr_t)client->PortRegister(port_name, port_type, flags, buffer_size));
}

LIB_EXPORT int jack_port_unregister(jack_client_t* ext_client, jack_port_t* port)
Expand Down
57 changes: 47 additions & 10 deletions common/jack/jack.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,13 @@ float jack_cpu_load (jack_client_t *client) JACK_OPTIONAL_WEAK_EXPORT;
* @{
*/

/**
* @return the UUID of the jack_port_t
*
* @see jack_uuid_to_string() to convert into a string representation
*/
jack_uuid_t jack_port_uuid (const jack_port_t *port) JACK_OPTIONAL_WEAK_EXPORT;

/**
* Create a new port for the client. This is an object used for moving
* data of any type in or out of the client. Ports may be connected
Expand All @@ -723,9 +730,9 @@ float jack_cpu_load (jack_client_t *client) JACK_OPTIONAL_WEAK_EXPORT;
* The @a port_name must be unique among all ports owned by this client.
* If the name is not unique, the registration will fail.
*
* All ports have a type, which may be any non-NULL and non-zero
* length string, passed as an argument. Some port types are built
* into the JACK API, currently only JACK_DEFAULT_AUDIO_TYPE.
* All ports have a type. It is strongly encouraged to use @ref jack_port_register_audio
* for signal channels and @ref jack_port_register_message for message (MIDI, OSC, …)
* channels instead of this method.
*
* @param client pointer to JACK client structure.
* @param port_name non-empty short name for the new port (not
Expand All @@ -744,6 +751,43 @@ jack_port_t * jack_port_register (jack_client_t *client,
unsigned long flags,
unsigned long buffer_size) JACK_OPTIONAL_WEAK_EXPORT;


/**
* Create a new message port as documented in @ref jack_port_register. Use the metadata API
* to specify what type of data this port is sending/receiving. The default is "PCM" for plain
* audio. To create a port containing voltage control instead of audio, you need to call
* @code
* jack_uuid_t uuid = jack_port_uuid(port);
* jack_set_property(client, uuid, JACK_METADATA_EVENT_TYPE, "VC", "text/plain");
* @endcode
*/
inline jack_port_t * jack_port_register_audio (jack_client_t *client,
const char *port_name,
unsigned long flags,
unsigned long buffer_size) JACK_OPTIONAL_WEAK_EXPORT
{
return jack_port_register(client, port_name, JACK_DEFAULT_AUDIO_TYPE, flags, buffer_size);
}


/**
* Create a new message port as documented in @ref jack_port_register. Use the metadata API
* to specify which communication protocols this port understands. The default is "MIDI". To
* create an OSC port for example, you need to call
* @code
* jack_uuid_t uuid = jack_port_uuid(port);
* jack_set_property(client, uuid, JACK_METADATA_EVENT_TYPE, "OSC", "text/plain");
* @endcode
*/
inline jack_port_t * jack_port_register_message (jack_client_t *client,
const char *port_name,
unsigned long flags,
unsigned long buffer_size) JACK_OPTIONAL_WEAK_EXPORT
{
return jack_port_register(client, port_name, JACK_DEFAULT_MESSAGE_TYPE, flags, buffer_size);
}


/**
* Remove the port from the client, disconnecting any existing
* connections.
Expand Down Expand Up @@ -773,13 +817,6 @@ int jack_port_unregister (jack_client_t *client, jack_port_t *port) JACK_OPTIONA
*/
void * jack_port_get_buffer (jack_port_t *port, jack_nframes_t) JACK_OPTIONAL_WEAK_EXPORT;

/**
* @return the UUID of the jack_port_t
*
* @see jack_uuid_to_string() to convert into a string representation
*/
jack_uuid_t jack_port_uuid (const jack_port_t *port) JACK_OPTIONAL_WEAK_EXPORT;

/**
* @return the full name of the jack_port_t (including the @a
* "client_name:" prefix).
Expand Down
12 changes: 8 additions & 4 deletions common/jack/metadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ extern const char* JACK_METADATA_CONNECTED;
* OSC events is not a valid MIDI status byte, so MIDI clients that check the
* status byte will gracefully ignore OSC messages if the user makes an
* inappropriate connection.
*
* This property may contain values other than "MIDI" and "OSC" for custom (or
* newer) protocols. If it is not set, the contained data should be treated as "MIDI".
*/
extern const char* JACK_METADATA_EVENT_TYPES;

Expand Down Expand Up @@ -312,10 +315,11 @@ extern const char* JACK_METADATA_PORT_GROUP;
* The type of an audio signal.
*
* This property allows audio ports to be tagged with a "meaning". The value
* is a simple string. Currently, the only type is "CV", for "control voltage"
* ports. Hosts SHOULD be take care to not treat CV ports as audibile and send
* their output directly to speakers. In particular, CV ports are not
* necessarily periodic at all and may have very high DC.
* is a simple string. Currently, the only types are "PCM" for normal audio and
* "CV", for "control voltage" ports. Hosts SHOULD be take care to not treat CV ports
* as audibile and send their output directly to speakers. In particular, CV ports
* are not necessarily periodic at all and may have very high DC. Not setting this property
* implies the default value "PCM".
*/
extern const char* JACK_METADATA_SIGNAL_TYPE;

Expand Down

6 comments on commit cf6523f

@falkTX
Copy link
Member

@falkTX falkTX commented on cf6523f Oct 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misunderstood me.
jack_port_register_audio is not the way to go, but something that can receive more arguments then the port_register where we define the custom type. this will call port register, and then metadata stuff on top of it.

off-topic: lack of method overloading in C is a feature not a bug. all exported symbols have a unique name, this is what makes C-based libraries being able to be exported and used in so many other languages

@piegamesde
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. Should I keep the _register_audio helper functions nevertheless?

Automatically setting the metadata has two problems I encountered:

  • Header foo will break if I include metadata.h into jack.h for some reasons. (This should work if I include this as proper new methods instead of inline ones though)
  • Since sadly there are two different properties to set, I'll still have to check for the port_type.

@falkTX
Copy link
Member

@falkTX falkTX commented on cf6523f Oct 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to put all the new stuff in a new header file. less confusing that way.
call it message_event.h or similar, that way it is much easier for 3rd parties to know what is new - they just check a single header file.
the only exception is the macro for the "binary message" string.

we can put the new functions in this header: the event related stuff and also the inline functions (that set event type together with the port registration).

the midiport.h header can then import message_event.h (header names not finalized) so it has the data types that message a generic message event.

@piegamesde
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's assume I redo this pull request from scratch.:

  • Create a new header file called messageport.h
  • All methods in messageport.h will typedef back to midiport.h. The documentation is copied and adapted from midiport.h, as well as the method names.
  • midiport.h includes messageport.h, all struct definitions from midiport.h are moved to messageport.h. (This way we maintain ABI compability while not having new clients drag old code with them
  • The JACK_DEFAULT_MIDI_TYPE is replaced with JACK_DEFAULT_MESSAGE_TYPE
  • There are new methods in messageport.h that help creating ports with the correct metadata. Their use is optional, calling the old methods will provide sensible defaults. Since they are in a new header file and optional use, they can be their own symbols and don't have do be inline.

What do you think?

@falkTX
Copy link
Member

@falkTX falkTX commented on cf6523f Oct 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, except replacing JACK_DEFAULT_MIDI_TYPE with JACK_DEFAULT_MESSAGE_TYPE.
for legacy reasons we need to keep the JACK_DEFAULT_MIDI_TYPE

I dont think you need to redo the PR, only update the headers now.
I can squash the commits in a single one when the PR is ready

@piegamesde
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With "replace", I didn't mean replacing the symbol in the API, but if a newer server encounters a port with the type JACK_DEFAULT_MIDI_TYPE from an older client, it will silently change this to JACK_DEFAULT_MESSAGE_TYPE. But honestly I'm not sure if I should do this.

Please sign in to comment.