[Bug 707605] Need "reverse-funnel" element

[GStreamer (bugzilla.gnome.org)]

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

Thiago Sousa Santos <thiago.sousa.santos> changed:

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

--- Comment #67 from Thiago Sousa Santos <thiago.sousa.santos at collabora.co.uk> 2014-01-18 13:08:16 UTC ---
Review of attachment 264949:
 --> (https://bugzilla.gnome.org/review?bug=707605&attachment=264949)

Good job, just have minor changes noted below.

I'd also think that it should be GstStreamIdDemux instead of GstStreamidDemux.

Thanks for your patch.

::: plugins/elements/gststreamiddemux.c
@@ +147,3 @@
+    GValue * value, GParamSpec * pspec)
+{
+  GstStreamidDemux *demux = GST_STREAMID_DEMUX (object);

As you know for sure this object is a GstStreamidDemux, you can create a
GST_STREAMID_DEMUX_CAST that is just a cast and avoids type checking that
should always succeed.
This applies to other occurrences, too.

@@ +169,3 @@
+
+  return TRUE;
+}

It might be worth a note that this only works because they are forwarded right
after a new stream-id is received, so that prevents it from sending mixed
stream-ids.

Also, can this handle different streams with different caps/segments?

@@ +188,3 @@
+  GST_OBJECT_LOCK (demux);
+  if (demux->active_srcpad != NULL)
+    demux->active_srcpad = NULL;

This seems like a dead assignment

@@ +222,3 @@
+    gst_object_unref (srcpad);
+  } else {
+    GST_OBJECT_UNLOCK (demux);

Should unref the buffer to prevent a leak

@@ +278,3 @@
+      GST_OBJECT_LOCK (demux);
+      if (demux->active_srcpad != NULL)
+        demux->active_srcpad = NULL;

This is also a dead assignment

@@ +284,3 @@
+      g_object_notify (G_OBJECT (demux), "active-pad");
+    }
+  }

The locking in this block looks confusing to me, it works because this is
inside the event handler, so only one event handler can run at a time (for the
same pad).

I'd prefer having the lock before the get_srcpad_by_stream_id (and make this
function assume it should be called with the lock already) and then unlock
inside the if/else branches.

For the if, you likely will have to split the srcpad_create into 2 parts. The
first part would just create the GstPad and can be run with the lock taken. The
second one activates and adds the pad and should be run without the lock.

On the else branch, it should be the same.

Not sure if you agree with this, but it seems to make a more consistent
locking.

@@ +316,3 @@
+static void
+gst_streamid_demux_release_srcpads (const GValue * item,
+    GstStreamidDemux * demux)

Make the function name singular, it only releases 1 srcpad

-- 
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.