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 341029 - Ghostpads internal and target should be linked from the beginning
Ghostpads internal and target should be linked from the beginning
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.10
Assigned To: Wim Taymans
GStreamer Maintainers
: 340420 (view as bug list)
Depends on: 331727
Reported: 2006-05-08 14:21 UTC by Edward Hervey
Modified: 2006-09-01 10:32 UTC
See Also:
GNOME target: ---
GNOME version: ---

Fix for recursive ghostpads with probes and pad-blocking (4.10 KB, patch)
2006-06-17 19:07 UTC, Edward Hervey
none Details | Review
typo fix (4.17 KB, patch)
2006-06-17 19:13 UTC, Edward Hervey
rejected Details | Review
Patch for ghostpad internal-target linking from start (25.40 KB, patch)
2006-07-08 17:18 UTC, Edward Hervey
none Details | Review
Updated version (29.82 KB, patch)
2006-07-10 11:31 UTC, Edward Hervey
none Details | Review
Updated version with testsuite fix (32.11 KB, patch)
2006-07-11 09:44 UTC, Edward Hervey
none Details | Review
Updated, threadsafety improved (35.94 KB, patch)
2006-07-11 12:11 UTC, Edward Hervey
none Details | Review
Updated version, beauty fixes (35.16 KB, patch)
2006-07-11 12:22 UTC, Edward Hervey
none Details | Review
Updated, with comments taken into account. (39.03 KB, patch)
2006-07-11 15:34 UTC, Edward Hervey
committed Details | Review
Updated design document (12.81 KB, patch)
2006-07-18 15:01 UTC, Edward Hervey
none Details | Review

Description Edward Hervey 2006-05-08 14:21:13 UTC
data/event/buffer probes and pad_blocking don't work on ghostpads of ghostpad currently.

A simple way to test that (gst-python code):
fakesink = gst.element_factory_make("fakesink")
bin1 = gst.Bin("bin1")
bin2 = gst.Bin("bin2")


bin1.add_pad(gst.GhostPad("src", fakesink.get_pad("src"))
bin2.add_pad(gst.GhostPad("src", bin1.get_pad("src"))

set to play, myprobe never gets called
Comment 1 Edward Hervey 2006-05-08 14:23:07 UTC
*** Bug 340420 has been marked as a duplicate of this bug. ***
Comment 2 Edward Hervey 2006-06-17 19:07:57 UTC
Created attachment 67545 [details] [review]
Fix for recursive ghostpads with probes and pad-blocking

This patch modifies the check for ghostpad with probes and pad blocking. Previously those functions were assuming that ghostpad were ghosting real pads.

This patch modifies those checks to recursively get the target of the ghostpads until it reaches a real pad.
Comment 3 Edward Hervey 2006-06-17 19:13:48 UTC
Created attachment 67547 [details] [review]
typo fix

Fixes a small error in the patch (forgot to convert a 'if' into a 'while')
Comment 4 Edward Hervey 2006-06-23 07:12:55 UTC
That patch is a hack in fact. This is to cope with the fact that a ghostpad target and a ghostpad internal pad are not linked.

Changing the title of the bug to reflect that.
Comment 5 Edward Hervey 2006-07-08 17:18:12 UTC
Created attachment 68628 [details] [review]
Patch for ghostpad internal-target linking from start

This patch implements linking of ghostpads' internal pad and target pad from the beginning.
Also contains removal of all the ghostpad cruft for pad blocking and probes.
In order to avoid a deadlock, GstBin have to (de)activate their sink pads before doing the state change of their children (since the ghostpads can now properly block and could be holding the streamlock).

Comments are more than welcome.
Comment 6 Edward Hervey 2006-07-10 11:31:09 UTC
Created attachment 68712 [details] [review]
Updated version

Fixed behaviour of GstProxyPad's pad functions. Don't emit fatal warnings anymore, but instead return the appropriate return value.
Also, unlink target-internal when unsetting the parent of the ghostpad (happens when removing the ghostpad from a bin).
Comment 7 Edward Hervey 2006-07-11 09:44:45 UTC
Created attachment 68766 [details] [review]
Updated version with testsuite fix

Updated version.
Changes :
* don't check for floating but for existence of a parent.
* Fix the gstghostpad test. The check for refcount was wrong (although the comments on who have references was mostly correct).
Comment 8 Andy Wingo 2006-07-11 12:09:23 UTC
* It is nonobvious why the pad activation in gstbin.c has to be there, and why it's only the source pads. Probably what you want to do is add a vmethod to gstelement that gstbin can hook into, between src and sink pad activations. At the very least the copy/paste is unmaintainable.
probably needs to LOG() or something.

We use american english in our function names (colorspace and not colourspace), it should be neighbor.

You need to use get_target in, you don't have the object lock there. Also you should pay attention to the return of gst_pad_link. Also I don't even think it's necessary to check is_linked; link() or unlink() won't error if the pads aren't unlinked or linked, respectively, because there is an unavoidable race between is_linked() and the link/unlink action.

Otherwise looks fine.
Comment 9 Edward Hervey 2006-07-11 12:11:41 UTC
Created attachment 68769 [details] [review]
Updated, threadsafety improved

Updated version with better threadsafety.
* Moved the internal property down to the GstProxyPad level, in order to remove all the cruft to get the 'neighbour' pad (using parentage).
* Re-instated fatal warnings in pad functions that don't have a neighbour (should never happen).
* All access to internal/target properties of GstProxyPad are now protected by PROXY_LOCK
* Access to parent (when not possible to do otherwise) is also threadsafe (gst_object_get_parent()).
Comment 10 Edward Hervey 2006-07-11 12:22:33 UTC
Created attachment 68770 [details] [review]
Updated version, beauty fixes

Removed any mention of neighbour (it's in fact the internal pad).
Removed useless debug statements, or adjusted their level.
Comment 11 Jan Schmidt 2006-07-11 13:25:37 UTC
I'm giving this latest patch a thumbs up here. I can't see any regressions in playback when running core against the latest releases of base/good/bad/ugly/ffmpeg - so I'm happy to see this included.
Comment 12 Andy Wingo 2006-07-11 13:46:09 UTC
The bit about code copying from GstBin still merits at least a comment in the source, and much better would be to find a way to reuse that code. is not C89 afaik. (the initializers calling functions).

This looks like a refcount problem:

What's this? It seems there are refcount problems here as well. It merits a comment in any case.
    GST_PROXY_PAD_INTERNAL (pad) = internal;
Comment 13 Wim Taymans 2006-07-11 13:52:56 UTC
about the gst_iterator_fold_resync() code reuse. The main problem was (last time I retried) is the extra code you typically have to do when resyncing. I guess passing a method to call on reset would work.
Comment 14 Wim Taymans 2006-07-11 14:17:25 UTC
the function gst_ghost_pad_set_internal() should only be called twice: once in _new (to create the ghostpad + intenal pad) and once when destroying the ghostpad (set to NULL). It's currently also called in do_link() when the link fails and the old code needed to remove the internal pad. 
Comment 15 Edward Hervey 2006-07-11 15:34:14 UTC
Created attachment 68777 [details] [review]
Updated, with comments taken into account.

Padtemplate should be properly refcounted now (using gst_object_replace in gst_proxy_pad_set_target_unlocked())
gst_ghost_pad_set_internal() is split up into _init() and _dispose()

Added comment on how the refcounting works using the parent-child relation.
Comment 16 Edward Hervey 2006-07-11 16:58:11 UTC
2006-07-11  Edward Hervey  <>

        * gst/gstbin.c: (activate_pads),
        (iterator_activate_fold_with_resync), (gst_bin_src_pads_activate),
        (de)activate src pads before calling state_change on the childs.
        This is to avoid the case where a src ghostpad is blocked (holding the
        stream lock), which would block the deactivation of the ghostpad's
        target pad.
        * gst/gstghostpad.c: (gst_proxy_pad_do_query_type),
        (gst_proxy_pad_do_event), (gst_proxy_pad_do_query),
        (gst_proxy_pad_do_internal_link), (gst_proxy_pad_do_bufferalloc),
        (gst_proxy_pad_do_chain), (gst_proxy_pad_do_getrange),
        (gst_proxy_pad_do_checkgetrange), (gst_proxy_pad_do_getcaps),
        (gst_proxy_pad_do_acceptcaps), (gst_proxy_pad_do_fixatecaps),
        (gst_proxy_pad_do_setcaps), (gst_proxy_pad_set_target_unlocked),
        (gst_proxy_pad_set_target), (gst_proxy_pad_get_internal),
        (gst_proxy_pad_dispose), (gst_proxy_pad_init),
        (gst_ghost_pad_parent_set), (gst_ghost_pad_parent_unset),
        (gst_ghost_pad_do_activate_push), (gst_ghost_pad_do_activate_pull),
        (gst_ghost_pad_do_link), (gst_ghost_pad_do_unlink),
        (gst_ghost_pad_dispose), (gst_ghost_pad_new_no_target),
        (gst_ghost_pad_new), (gst_ghost_pad_set_target):
        GhostPads now create their internal GstProxyPad at creation (and not
        when they're linked, as it was being done previously).
        The internal and target pads are linked straight away.
        The data will also travel through the other pad in order to make
        pad blocking and probes non-hackish (the probe/block now really happens
        on the GhostPad and not on the target).
        * gst/gstpad.c: (gst_pad_set_blocked_async),
        (gst_pad_link_prepare), (gst_pad_push_event):
        Remove previous ghostpad cruft.
        * gst/gstutils.c: (gst_pad_add_data_probe),
        (gst_pad_add_event_probe), (gst_pad_add_buffer_probe),
        (gst_pad_remove_data_probe), (gst_pad_remove_event_probe),
        Remove previous ghost pad cruft.
        Added more detailed debug statements.
        * tests/check/gst/gstghostpad.c: (GST_START_TEST):
        Fix the testsuite for refcounting changes.
        The comments about who has references were correct, but the refcount
        being checked wasn't the same (!?!).

Comment 17 Wim Taymans 2006-07-18 14:45:15 UTC
reopening, while making unit tests the following things don't work:

- getcaps returns ANY on pad without a target (ignores template or pad caps when using fixed caps etc..)
- linking a pad without a target fails and generates a critical.

Also the design doc is not updated.
Comment 18 Edward Hervey 2006-07-18 15:01:07 UTC
Created attachment 69122 [details] [review]
Updated design document

Updated design document ... without fancy ASCII art
Comment 19 Wim Taymans 2006-09-01 10:32:57 UTC
Fixed now.