[Bug 707605] Need "reverse-funnel" element

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #70 from HoonHee Lee <hoonh83.lee at gmail.com> 2014-01-20 07:05:33 UTC ---
Thanks for your comment.

(In reply to comment #67)
> Review of attachment 264949 [details]:
> 
> 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?

=> I don't have a unit test case for different caps/segments.
Thus, I will share to you guys about the result of unit test after I've done
for different caps/segments.

> 
> @@ +188,3 @@
> +  GST_OBJECT_LOCK (demux);
> +  if (demux->active_srcpad != NULL)
> +    demux->active_srcpad = NULL;
> 
> This seems like a dead assignment

=> Thanks, I will remove dead assignment.

> 
> @@ +222,3 @@
> +    gst_object_unref (srcpad);
> +  } else {
> +    GST_OBJECT_UNLOCK (demux);
> 
> Should unref the buffer to prevent a leak

=> Thanks. IMHO, I think unref is not required.
You can reference below description for gst_pad_push() function.

---------------------------------------------------------------------------
In all cases, success or failure, the caller loses its reference to @buffer
after calling this function.
---------------------------------------------------------------------------

> 
> @@ +278,3 @@
> +      GST_OBJECT_LOCK (demux);
> +      if (demux->active_srcpad != NULL)
> +        demux->active_srcpad = NULL;
> 
> This is also a dead assignment

=> Thanks, I will remove 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.

=> Thanks, Actually I am not clear to lock and unlock for above that.
Thus, what I want to do is that someone helps me how to make for 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

=> I will modify function name as singular.

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