[Bug 738687] midi: add alsamidisrc, an ALSA MIDI sequencer source

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=738687

Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #304902|none                        |reviewed
             status|                            |

--- Comment #3 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> ---
Review of attachment 304902:
 --> (https://bugzilla.gnome.org/review?bug=738687&attachment=304902)

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.