kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE
I'm the author of amidiminder - a utility that keeps track of desired connections between ALSA Seq (MIDI) ports.
I've found a bug in the kernel part of the ALSA Sequencer, and am not sure where or how to report it.
Summary: The events: * SND_SEQ_EVENT_CLIENT_CHANGE is never sent * SND_SEQ_EVENT_PORT_CHANGE is not sent on common port changes (name and port info)
I have a lot of detail on these bugs, what other software causes them to be a problem (PureData), and where the missing code in the kernel sound/core/seq files is.
Before I dump all the details in this forum.... where is the right place for me to report and/or discuss this?
- Mark
On Sun, 24 Nov 2024 22:31:13 +0100, Mark Lentczner wrote:
I'm the author of amidiminder - a utility that keeps track of desired connections between ALSA Seq (MIDI) ports.
I've found a bug in the kernel part of the ALSA Sequencer, and am not sure where or how to report it.
Summary: The events:
- SND_SEQ_EVENT_CLIENT_CHANGE is never sent
- SND_SEQ_EVENT_PORT_CHANGE is not sent on common port changes (name and
port info)
I have a lot of detail on these bugs, what other software causes them to be a problem (PureData), and where the missing code in the kernel sound/core/seq files is.
Before I dump all the details in this forum.... where is the right place for me to report and/or discuss this?
Those events aren't created indeed. I don't remember exactly, as the implementation was really old (decades ago), but I guess it was because it'd give too much broadcasting when the event is triggered at each client / port change.
Actually it's trivial to enable those events. It's just a patch like below. But, I have to consider the result by this change and try to avoid the unnecessary event bombing before going ahead.
thanks,
Takashi
-- 8< -- --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1290,6 +1290,10 @@ static int snd_seq_ioctl_set_client_info(struct snd_seq_client *client, client->midi_version = client_info->midi_version; memcpy(client->event_filter, client_info->event_filter, 32); client->group_filter = client_info->group_filter; + + /* notify the change */ + snd_seq_system_client_ev_client_change(client->number); + return 0; }
@@ -1413,6 +1417,9 @@ static int snd_seq_ioctl_set_port_info(struct snd_seq_client *client, void *arg) if (port) { snd_seq_set_port_info(port, info); snd_seq_port_unlock(port); + /* notify the change */ + snd_seq_system_client_ev_port_change(info->addr.client, + info->addr.port); } return 0; }
Looking at the items that can be changed with client info, or port info, I think there is little likelihood that applications are going to be changing this information more than once in the lifetime of a client, or a port.
So far, my application hasn't detected any changes to port information after it starts tracking a port (though it doesn't track everything about a port.) This is certainly because the port info is given at the time of port creation. Of the several DAWs that I tested, when you change their MIDI configuration - rather than change the port info of their existing ports, they simply delete them all, and recreate the ports as needed, which is broadcasting PORT_EXIT and PORT_START events anyway. In either case, this isn't a broadcast concern as these events are relatively rare.
Client info is a bit different, as you must create your client first, then set info. So the CLIENT_START event would be followed by one or more CLIENT_CHANGE events. This is exacerbated because the helper functions for setting client name, event filter, etc... which each get the client info, set one field, and set the info back. So true, this change would cause a broadcast of a few events each time a client is created... But again, in the scheme of things, client creation is a relatively rare occurrence.
In the absence of these events, applications like mine, have to periodically poll every client info and every port info to see if anything that they care about (like client name or port name) has changed. For now, since port info changes (at least of name or capabilities) is essentially non-existent, my application doesn't bother to poll ports. And for clients, since the changes only happen right after client start, my application simply sleeps a short interval after getting CLIENT_START, before reading the client info, giving the client process time to set the info up. Clearly this isn't optimal or infalible - but in practice it works, and polling info frequently seems too high a cost.
Lastly, there IS one place in the code where PORT_CHANGE is sent. In seq_ump_client.c in the function update_port_infos(). In that case, interestingly, a port_info change is only considered if and only if the name or the capability fields are changed. That logic could be applied to your patch - but given how infrequent I suspect port (or client) changes are, it seems presumptive to be limiting what field changes an application would be interested in.
Happy to dive into any more details you may need, — Mark
On Mon, 25 Nov 2024 19:55:00 +0100, Mark Lentczner wrote:
Looking at the items that can be changed with client info, or port info, I think there is little likelihood that applications are going to be changing this information more than once in the lifetime of a client, or a port.
So far, my application hasn't detected any changes to port information after it starts tracking a port (though it doesn't track everything about a port.) This is certainly because the port info is given at the time of port creation. Of the several DAWs that I tested, when you change their MIDI configuration - rather than change the port info of their existing ports, they simply delete them all, and recreate the ports as needed, which is broadcasting PORT_EXIT and PORT_START events anyway. In either case, this isn't a broadcast concern as these events are relatively rare.
Client info is a bit different, as you must create your client first, then set info. So the CLIENT_START event would be followed by one or more CLIENT_CHANGE events. This is exacerbated because the helper functions for setting client name, event filter, etc... which each get the client info, set one field, and set the info back. So true, this change would cause a broadcast of a few events each time a client is created... But again, in the scheme of things, client creation is a relatively rare occurrence.
In the absence of these events, applications like mine, have to periodically poll every client info and every port info to see if anything that they care about (like client name or port name) has changed. For now, since port info changes (at least of name or capabilities) is essentially non-existent, my application doesn't bother to poll ports. And for clients, since the changes only happen right after client start, my application simply sleeps a short interval after getting CLIENT_START, before reading the client info, giving the client process time to set the info up. Clearly this isn't optimal or infalible - but in practice it works, and polling info frequently seems too high a cost.
Actually it's only the deliveries per subscription, hence the message transmissions aren't too high. We can optimize a bit in the case where no subscription is done to the announcement port, but that's another story.
Lastly, there IS one place in the code where PORT_CHANGE is sent. In seq_ump_client.c in the function update_port_infos(). In that case, interestingly, a port_info change is only considered if and only if the name or the capability fields are changed. That logic could be applied to your patch - but given how infrequent I suspect port (or client) changes are, it seems presumptive to be limiting what field changes an application would be interested in.
Yes, that's possible, but I don't think it's worth for complication; the call patterns of client and port info ioctls are usually for really updating the attributes from applications. OTOH, in the recent UMP code, I implemented the check because it may driven by the device firmware, hence it's not really predictable.
That being said, I'm fine to apply the change, but I hesitate to do so for the current 6.13 merge window. I'm going to submit (and likely merge) the patch for 6.14 once after 6.13 merge window is closed in the next week.
thanks,
Takashi
participants (2)
-
Mark Lentczner -
Takashi Iwai