After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738687 - midi: add alsamidisrc, an ALSA MIDI sequencer source
midi: add alsamidisrc, an ALSA MIDI sequencer source
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.x
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-17 13:36 UTC by Antonio Ospite
Modified: 2015-10-02 20:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add alsamidisrc, an ALSA MIDI sequencer source (19.53 KB, patch)
2014-10-17 13:36 UTC, Antonio Ospite
none Details | Review
midi: add an ALSA MIDI sequencer source v2 (19.69 KB, patch)
2015-06-09 22:04 UTC, Antonio Ospite
none Details | Review
midi: add an ALSA MIDI sequencer source v3 rebased on master (20.53 KB, patch)
2015-08-07 13:37 UTC, Antonio Ospite
needs-work Details | Review

Description Antonio Ospite 2014-10-17 13:36:02 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
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-23 14:00:17 UTC
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
Comment 2 Antonio Ospite 2015-06-09 22:04:16 UTC
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
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2015-06-22 14:08:50 UTC
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.
Comment 4 Antonio Ospite 2015-08-07 13:25:33 UTC
(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.
Comment 5 Antonio Ospite 2015-08-07 13:37:15 UTC
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
Comment 6 hungerburg 2015-08-07 14:05:39 UTC
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 ;)
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2015-08-26 05:22:30 UTC
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.
Comment 8 Tim-Philipp Müller 2015-08-26 08:31:21 UTC
Stefan: please wait until after 1.6 is out, git is currently frozen.
Comment 9 Sebastian Dröge (slomo) 2015-08-26 08:50:14 UTC
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 :)
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2015-08-28 08:21:48 UTC
btw. here is a one-liner to test

vkeybd & gst-launch-1.0 -v alsamidisrc ports=128:0 ! fluiddec ! audioconvert ! autoaudiosink
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-28 13:38:52 UTC
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
Comment 12 Tim-Philipp Müller 2015-09-28 14:07:51 UTC
Or maybe it should just go into the existing alsa plugin in -base? Any reason not to do that?
Comment 13 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-28 14:33:24 UTC
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.
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2015-10-01 19:19:15 UTC
After some IRC discussion, we decided to push this to base. I'll take care of this now.
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2015-10-01 20:00:46 UTC
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
Comment 16 Antonio Ospite 2015-10-02 20:39:58 UTC
Thanks again Stefan :)