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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #4 from Antonio Ospite <ao2 at ao2.it> ---
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #3)
> 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 :)
>

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.

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