GNOME Bugzilla – Bug 658517
[ghostpad] Peer query does not work with empty GstPlaySinkVideoConvert
Last modified: 2011-11-03 08:51:22 UTC
Created attachment 195943 [details] [review] Fix query for the case there is no explicit target While trying to retrieve the vaDisplay using a custome query, I found that it actually didn't work if the query goes across a element like an empty GstPlaySinkVideoConvert. In this case, the element is a GstBin with two ghost pad but no element inside. The query chain stops in gst_proxy_pad_query_default() because the target is NULL. The attached patch retrieves the target using gst_proxy_pad_get_internal() and gst_pad_get_peer().
I'm not sure how this makes a difference. The peer of the internal pad should be equivalent to the target if I'm not missing anything. Also you could use gst_pad_peer_query() :)
Is this for the case when the sinkpad ghostpad gets the proxypad of the srcpad ghostpad set as target? This then makes the srcpad target-less and causes problems? If that's the case the correct solution would be to add something to gst_ghost_pad_set_target() that makes sure that ghostpads that are connected to each other have each other's proxypads as target.
(In reply to comment #2) > Is this for the case when the sinkpad ghostpad gets the proxypad of the srcpad > ghostpad set as target? This then makes the srcpad target-less and causes > problems? Yes. > > If that's the case the correct solution would be to add something to > gst_ghost_pad_set_target() that makes sure that ghostpads that are connected to > each other have each other's proxypads as target. Ok, so essentially, we should do something special when the target is set to NULL ? My concern was about _get_target(), should it return NULL or some proxypad ?
gst_ghost_pad_set_target(ghostpad, proxypad) should check if the proxypad belongs to another ghostpad and if it does it should set the target pointer in the other ghostpad. Same for setting the target to NULL. This way the old query code will work and both ghost pads know their target
I've been looking into this issue and I think I found where the confusion comes from. When we call gst_ghost_pad_set_target(), what we actually do is to link the new pad to the ghostpad internal pad. If the link succeed, the end result is that the peer of the ghost pad internal pad is set as the new pad (and vis-versa). What I'm trying to make you realise is that the peer of the ghost pad internal pad is the ghost pad target. Storing this target again inside the ghost pad is redundant, and error prone. My suggestion here is to drop the target reference from the proxy pad implementation and use a really accurate target, which is the internal pad peer. This should remove some confusion in the ghostpad implementation cause by redundancy.
Created attachment 199974 [details] [review] Don't cache internal proxy pad target Fix this issue by not caching the target, and using the well tested peer handling of GstPad.
Created attachment 199975 [details] [review] Fix memory leak in basetransform While fixing this issue I found a reference leak in base transform.
Created attachment 199976 [details] [review] Don't cache internal proxy pad target (compilation fix) Sorry, went a little fast on this one, I removed the dispose/finalize method or GstProxyPad because it ended up empty, but forgot to remove the static declaration. This version fixes that.
Comment on attachment 199975 [details] [review] Fix memory leak in basetransform This was already fixed differently in GIT.
commit 391568efde9b737b9c7358110680e06cd62ae0fb Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Oct 25 17:26:50 2011 -0400 ghostpad: Don't cache internal proxy pad target The internal proxy pad target is simply a cache of the internal proxy pad peer. This patch uses the well implement GstPad peer handling to obtain the target. This fixes issues with target not being set in both direction when two ghostpads are linked together (empty bin). https://bugzilla.gnome.org/show_bug.cgi?id=658517