*** Bug 340420 has been marked as a duplicate of this bug. ***

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.

Created attachment 67547 [details] [review]
typo fix

Fixes a small error in the patch (forgot to convert a 'if' into a 'while')

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.

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.

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

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

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

http://bugzilla.gnome.org/attachment.cgi?id=68766&action=diff#gst/gstghostpad.c_sec10
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 http://bugzilla.gnome.org/attachment.cgi?id=68766&action=diff#gst/gstghostpad.c_sec16, 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.

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

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.

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.

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.

http://bugzilla.gnome.org/attachment.cgi?id=68770&action=diff#gst/gstghostpad.c_sec18 is not C89 afaik. (the initializers calling functions).

This looks like a refcount problem:
  GST_PAD_PAD_TEMPLATE (internal) = GST_PAD_PAD_TEMPLATE (peer);

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;
    GST_PROXY_PAD_INTERNAL (GST_PROXY_PAD_INTERNAL (pad)) = GST_PAD (pad);

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.

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. 

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.

2006-07-11  Edward Hervey  <edward@fluendo.com>

        * gst/gstbin.c: (activate_pads),
        (iterator_activate_fold_with_resync), (gst_bin_src_pads_activate),
        (gst_bin_change_state_func):
        (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_class_init),
        (gst_ghost_pad_internal_do_activate_push),
        (gst_ghost_pad_internal_do_activate_pull),
        (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),
        (gst_pad_remove_buffer_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 (!?!).

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.

Created attachment 69122 [details] [review]
Updated design document

Updated design document ... without fancy ASCII art

Fixed now.