GNOME Bugzilla – Bug 738687
midi: add alsamidisrc, an ALSA MIDI sequencer source
Last modified: 2015-10-02 20:39:58 UTC
Created attachment 288752 [details] [review] add alsamidisrc, an ALSA MIDI sequencer source Hi, in the attached patch I am proposing a prototype for an ALSA MIDI sequencer source. The implementation is based on a previous experiment of mine with appsrc[1] which was in turn based on aseqdump[2] and amidicat[3]. The code works, and it could even be good enough for my vocoder[4] use case, but I feel it'd be nice to have this element upstream and there are still some issues to deal with before that could happen: - I am using a GstBufferList hack to push more buffers in the same "polling interval" to the source pad, that is in the create() callback; should I use threads and queues? - The polling interval I am using with the alsa code is quite arbitrary, I set it to be "less than the buffer duration", but maybe a more efficient approach can be used. For now I tried to keep the code as simple as possible, just to show the functionality, any suggestions for improvements, or even better _patches_ are more than welcome. The patch is against the 1.4 branch. Ciao, Antonio [1] http://git.ao2.it/gst-aseq-appsrc.git/ [2] http://git.alsa-project.org/?p=alsa-utils.git;a=tree;f=seq/aseqdump [3] http://krellan.com/amidicat/ [4] http://ao2.it/109
Review of attachment 288752 [details] [review]: ::: gst/midi/alsamidisrc.c @@ +261,3 @@ + + G_OBJECT_CLASS (parent_class)->finalize (object); +} looks like you can remove this vmethod completely @@ +325,3 @@ + + local_data = g_malloc (size); + memcpy (local_data, data, size); g_memdup() @@ +331,3 @@ + g_free)); + + if (alsamidisrc->dump) { this is for debugging, I'd remove the property and use GST_MEMDUMP instead. @@ +353,3 @@ +push_tick_buffer (GstAlsaMidiSrc * alsamidisrc, GstBufferList * buffer_list) +{ + alsamidisrc->buffer[0] = 0xf9; use a constant for 0xf9 and/or add a comment. @@ +393,3 @@ + /* ENOENT indicates an event that is not a MIDI message, silently skip it */ + if (-ENOENT == size_ev) { + GST_WARNING ("Warning: Received non-MIDI message"); GST_WARNING_OBJECT (src, ...) also for GST_ERROR and co. @@ +469,3 @@ + snd_midi_event_no_status (alsamidisrc->parser, 1); + + alsamidisrc->buffer = malloc (DEFAULT_BUFSIZE); g_malloc, please use g_malloc/g_free everywhere for consistency @@ +471,3 @@ + alsamidisrc->buffer = malloc (DEFAULT_BUFSIZE); + if (alsamidisrc->buffer == NULL) { + ret = -ENOMEM; unused assigment? @@ +486,3 @@ + +err_free_buffer: + free (alsamidisrc->buffer); g_free
Created attachment 304902 [details] [review] midi: add an ALSA MIDI sequencer source v2 Hi, here is a v2 of the patch, I addressed all the comments in the review (thanks Stefan, BTW) and added some other cleanups. Changes since v1: - Fix some typos in the documentation - Improve wording in the documentation - Remove the finalize() vmethod, there are no resources to free - Use g_memdup() instead of g_malloc()+memcpy() - Use Glib for memory management: g_strdup() g_realloc(), g_malloc(), g_free() - Use the _OBJECT variants of GST_ERROR, GST_WARNING and GST_DEBUG - Define GST_CAT_DEFAULT before calling GST_{ERROR,WARNING,DEBUG}_OBJECT - Remove the "dump" property and use GST_MEMDUMP_OBJECT() - Do not set ret = -ENOMEM in gst_alsa_midi_src_start() as it won't be used - Clarify where the midi tick message 0xf9 comes from - Improve the comments about the gst_pad_push_list() hack - Do not use alloca(), the memory is going to be used after the return - Reorder the cleanup sequence in gst_alsa_midi_src_stop() - Simplify parse_ports() by using g_strsplit() instead of strchr() - Do not include "stdlib.h" and "string.h" anymore, they are not needed - Rename labels starting with err_ to use the error_ prefix, for consistency - Rename error_free_ports labels to the more descriptive error_free_seq_ports Please let me know if you test it. I would still like some comments about the choice of DEFAULT_POLL_TIMEOUT_MS, and the usage of a GstBufferList in gst_alsa_midi_src_create(). Thanks, Antonio
Review of attachment 304902 [details] [review]: To me the use of buffer_list makes send, since a poll with data can contain multiple events. This is maybe not the optimal way to do it, but we should optimize this is someone complains :) ::: gst/midi/alsamidisrc.c @@ +125,3 @@ + alsamidisrc->port_count = g_strv_length (ports_list); + alsamidisrc->seq_ports = + g_malloc (alsamidisrc->port_count * sizeof (snd_seq_addr_t)); g_new (snd_seq_addr_t, alsamidisrc->port_count); @@ +126,3 @@ + alsamidisrc->seq_ports = + g_malloc (alsamidisrc->port_count * sizeof (snd_seq_addr_t)); + if (!alsamidisrc->seq_ports) { this won't happen, if g_malloc or g_new fails the program gets terminated (yes, really) If you want to handle the case where the port_count is so large, you can use g_try_new/g_try_malloc above. @@ +181,3 @@ + if (ret < 0) + /* warning */ + GST_WARNING_OBJECT (alsamidisrc, "Cannot connect from port %d:%d - %s", "Cannot connect to ..."? @@ +360,3 @@ + snd_seq_poll_descriptors (alsamidisrc->seq, alsamidisrc->pfds, + alsamidisrc->npfds, POLLIN); + ret = poll (alsamidisrc->pfds, alsamidisrc->npfds, DEFAULT_POLL_TIMEOUT_MS); Maybe this deserves a comment. If I understand right you are using the a poll(,,DEFAULT_POLL_TIMEOUT_MS) that times-out to push the tick buffer? The kernel will round-up the timeout, so this is not going to give you precise timing? Also why do we have to do it. Is this to keep the link 'alive'? If so please mention in the comment. As another comment - can we use GstPoll here? This increses the compatibility, but arguably this is linux only anyway. Maybe take a look at GstPoll and if it simplifies the code, switch.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #3) > Review of attachment 304902 [details] [review] [review]: > > To me the use of buffer_list makes send, since a poll with data can contain > multiple events. This is maybe not the optimal way to do it, but we should > optimize this is someone complains :) > OK, thanks. > ::: gst/midi/alsamidisrc.c [...] > @@ +126,3 @@ > + alsamidisrc->seq_ports = > + g_malloc (alsamidisrc->port_count * sizeof (snd_seq_addr_t)); > + if (!alsamidisrc->seq_ports) { > > this won't happen, if g_malloc or g_new fails the program gets terminated > (yes, really) If you want to handle the case where the port_count is so > large, you can use g_try_new/g_try_malloc above. > I used g_try_new() and g_try_malloc() and kept the current cleanup paths as they are. > @@ +181,3 @@ > + if (ret < 0) > + /* warning */ > + GST_WARNING_OBJECT (alsamidisrc, "Cannot connect from port %d:%d - > %s", > > "Cannot connect to ..."? It is input "from" the ALSA MIDI sequencer device, the function used is snd_seq_connect_from() and the alsa code (aseqdump, arecordmidi) uses this same message in the error paths after calling this function, so I left as it is. > @@ +360,3 @@ > + snd_seq_poll_descriptors (alsamidisrc->seq, alsamidisrc->pfds, > + alsamidisrc->npfds, POLLIN); > + ret = poll (alsamidisrc->pfds, alsamidisrc->npfds, > DEFAULT_POLL_TIMEOUT_MS); > > Maybe this deserves a comment. If I understand right you are using the a > poll(,,DEFAULT_POLL_TIMEOUT_MS) that times-out to push the tick buffer? The > kernel will round-up the timeout, so this is not going to give you precise > timing? Also why do we have to do it. Is this to keep the link 'alive'? If > so please mention in the comment. > I added a comment for that in the new version, if it is not clear enough, please let me know. > As another comment - can we use GstPoll here? This increses the > compatibility, but arguably this is linux only anyway. Maybe take a look at > GstPoll and if it simplifies the code, switch. I did not see any particular advantage in using GstPoll, so I left the current polling mechanism as it is. New version coming right away.
Created attachment 308898 [details] [review] midi: add an ALSA MIDI sequencer source v3 rebased on master Hi, here is a v3 of the patch. The changes since v2 are: - Use g_try_new() instead of g_malloc() when allocating ports - Use g_try_malloc() in other places to actually use the current cleanup paths - Add a comment explaining how/why the file descriptors are polled - Improve wording and formatting for some comments - Rebase against the master branch (only gst/midi/Makefile.am needed some adjustments) - Update author date and add a more verbose commit message The incremental changes can be found here for a more convenient re-review: http://ao2.it/tmp/alsamidisrc_v3_incremental/ I hope this can still make it into 1.6. Thanks a lot, Antonio
Please excuse, this Bugzilla here has no voting feature, so I'd like to use a comment to vote for the feature: There is an unlimited number of devices, keyboards, sequencers, hardware controllers, you name it, that talk MIDI which can be fun to interact and toy with, combine audio, video etc. with. Actually, I dearly would like to also record the stream to a file, but I fear that patch has yet to be produced… Considering this a start though ;)
I think it is good to go. I just played with it and it works nicely. I'll do another look over the code and then push it unless anyone objects.
Stefan: please wait until after 1.6 is out, git is currently frozen.
Mark the patch as accept-commit-after-freeze once it's considered ready to go. That way we know easily what all needs to be merged once 1.6 is out :)
btw. here is a one-liner to test vkeybd & gst-launch-1.0 -v alsamidisrc ports=128:0 ! fluiddec ! audioconvert ! autoaudiosink
This unfortunately needs more work. 1) the patch can't add the plugin to gst/midi. It should probably go to ext/alsamidi. 2) you will need to add a copy of gst-plugin-base/m4/gst-alsa.m4 to the patch for gst-plugins.bad
Or maybe it should just go into the existing alsa plugin in -base? Any reason not to do that?
Well, our rules are: first to bad, then after a few cycles send a move request. Since this element won't be auto plugged I see no big issue by having it is base, if everyone is happy with how it is implemented.
After some IRC discussion, we decided to push this to base. I'll take care of this now.
commit caf7b6674b95d17a46d275ff7f6861026a33f7f9 Author: Stefan Sauer <ensonic@users.sf.net> Date: Thu Oct 1 21:53:20 2015 +0200 docs: add alsamidisrc to docs commit 2c7ed422921f9c946a3ece4445352f382aab24ea Author: Antonio Ospite <ao2@ao2.it> Date: Thu Oct 1 21:43:21 2015 +0200 midi: add an ALSA MIDI sequencer source The alsamidisrc element allows to get input event from ALSA MIDI sequencer devices, and possibly convert them to sound using some downstream element like fluiddec. Fixes #738687
Thanks again Stefan :)