After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 658517 - [ghostpad] Peer query does not work with empty GstPlaySinkVideoConvert
[ghostpad] Peer query does not work with empty GstPlaySinkVideoConvert
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-09-08 01:52 UTC by Nicolas Dufresne (ndufresne)
Modified: 2011-11-03 08:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix query for the case there is no explicit target (1.30 KB, patch)
2011-09-08 01:52 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
Don't cache internal proxy pad target (16.88 KB, patch)
2011-10-25 21:28 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
Fix memory leak in basetransform (795 bytes, patch)
2011-10-25 21:30 UTC, Nicolas Dufresne (ndufresne)
rejected Details | Review
Don't cache internal proxy pad target (compilation fix) (17.18 KB, patch)
2011-10-25 21:43 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2011-09-08 01:52:58 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().
Comment 1 Sebastian Dröge (slomo) 2011-09-08 12:49:29 UTC
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() :)
Comment 2 Sebastian Dröge (slomo) 2011-09-08 13:00:02 UTC
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.
Comment 3 Nicolas Dufresne (ndufresne) 2011-09-08 13:05:52 UTC
(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 ?
Comment 4 Sebastian Dröge (slomo) 2011-09-08 13:13:16 UTC
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
Comment 5 Nicolas Dufresne (ndufresne) 2011-10-25 00:29:01 UTC
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.
Comment 6 Nicolas Dufresne (ndufresne) 2011-10-25 21:28:45 UTC
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.
Comment 7 Nicolas Dufresne (ndufresne) 2011-10-25 21:30:38 UTC
Created attachment 199975 [details] [review]
Fix memory leak in basetransform

While fixing this issue I found a reference leak in base transform.
Comment 8 Nicolas Dufresne (ndufresne) 2011-10-25 21:43:51 UTC
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 9 Sebastian Dröge (slomo) 2011-11-03 07:52:00 UTC
Comment on attachment 199975 [details] [review]
Fix memory leak in basetransform

This was already fixed differently in GIT.
Comment 10 Sebastian Dröge (slomo) 2011-11-03 08:51:19 UTC
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