[Bug 707605] Need "reverse-funnel" element

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=707605
  GStreamer | gstreamer (core) | git

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #256875|none                        |needs-work
             status|                            |

--- Comment #42 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-10-10 12:16:42 UTC ---
Review of attachment 256875:
 --> (https://bugzilla.gnome.org/review?bug=707605&attachment=256875)

::: plugins/elements/gststreamiddemux.c
@@ +173,3 @@
+
+  if (demux->stream_id_pairs == NULL)
+    demux->stream_id_pairs = g_hash_table_new (g_str_hash, g_str_equal);

Consider using
https://developer.gnome.org/glib/unstable/glib-Hash-Tables.html#g-hash-table-new-full
here, which allows you to free elements easier :)

@@ +198,3 @@
+
+  if (padname)
+    g_free (padname);

padname will always be != NULL here, and g_free() is NULL-safe anyway

@@ +214,3 @@
+
+  if (demux->active_srcpad) {
+    GST_OBJECT_LOCK (demux);

You need to lock *before* checking if active_srcpad is NULL

@@ +220,3 @@
+    gst_object_unref (srcpad);
+  } else
+    gst_buffer_unref (buf);

I think this should be an error. There should always be an active srcpad

@@ +243,3 @@
+  GST_OBJECT_UNLOCK (demux);
+  if (srcpad) {
+    GST_DEBUG_OBJECT (demux, "srcpad = %s matched", gst_pad_get_name
(srcpad));

GST_DEBUG_OBJECT (demux, "srcpad = %s:%s matched", GST_DEBUG_PAD_NAME (srcpad))

@@ +279,3 @@
+      }
+      demux->active_srcpad = active_srcpad;
+      GST_OBJECT_UNLOCK (demux);

If the active_srcpad changed, call g_object_notify() for the active-srcpad
property

@@ +287,3 @@
+      || GST_EVENT_TYPE (event) == GST_EVENT_EOS)
+    res = gst_pad_event_default (pad, parent, event);
+  else if (demux->active_srcpad) {

Use curly braces for the first if, and the else too. Easier to read :)

@@ +355,3 @@
+    g_hash_table_foreach (demux->stream_id_pairs,
+        gst_streamid_demux_free_stream_id_pairs, NULL);
+    g_hash_table_destroy (demux->stream_id_pairs);

Use g_hash_table_unref() here for sanity

::: tests/check/elements/streamiddemux.c
@@ +63,3 @@
+_fake_chain (GstPad * pad, GstObject * parent, GstBuffer * buffer)
+{
+  gst_buffer_unref (buffer);

Probably makes sense here to check if you only get only the buffers meant for
this stream here (maybe remove the randomness above and just send the same
number of buffers to each stream?)

@@ +102,3 @@
+  while (mysink_cnt < NUM_SUBSTREAMS) {
+    gst_check_setup_events_with_stream_id (td.mysrc, td.demux, caps,
+        GST_FORMAT_BYTES, g_strdup_printf ("test%d", mysink_cnt++));

You forget cleanup here, all pads and elements need to be freed.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.