Created attachment 76483 [details] [review]
Possible patch

Make collectpads monitor (un)linking of managed pads,
and only take the linked pads into account for collection.

I doubt this is a good idea.  It looks like this would break dynamic (re)linking of pipelines.  You assume that unlinking a pad is a bad thing because it would cause the streaming to block.  You cannot know whether this is the case however.  I can unlink a pad and link it again (to some other pad) immediately (while the pad is blocked of course).  This would not pose any form of harmful delay for the  user of CollectPads.

If I understand you correctly, you propose to make CollectPads detect the unlink and exclude the pad from the collection.  It is clear to see how this is not what is generally wanted.  It would break dynamic linking performed on the source pads of all muxers and cause slightly different semantics in the case of e.g. adder.

Can you name a specific situation where the current behaviour causes problems?

(In reply to comment #2)
> I doubt this is a good idea.  It looks like this would break dynamic
> (re)linking of pipelines.  You assume that unlinking a pad is a bad thing
> because it would cause the streaming to block.  You cannot know whether this is
> the case however.  I can unlink a pad and link it again (to some other pad)
> immediately (while the pad is blocked of course).  This would not pose any form
> of harmful delay for the  user of CollectPads.

Agreed there is no harm if unlinking and then followed again by linking;
there is however blocking (harm?) if not followed by linking (see below).

> If I understand you correctly, you propose to make CollectPads detect the
> unlink and exclude the pad from the collection.  It is clear to see how this is
> not what is generally wanted.  It would break dynamic linking performed on the
> source pads of all muxers and cause slightly different semantics in the case of
> e.g. adder.

It would be (temporarily) excluded from the collection and considered again after linking.  Indeed, with this patch, there would be different semantics on dynamically unlinked *sink*pads (I suppose?); i.e. they would be (temporarily) auto-skipped (and e.g. adder would probably consider it EOS). The current auto-blocking behaviour may be 'simulated' by blocking peers of sinkpads  when performing such link-switch.
Anyway, this is admittedly convoluted and not so elegant, and not my desire either; my concern however is elsewhere (see below).

> Can you name a specific situation where the current behaviour causes problems?

I have no specific situation, but would occur e.g. when unlinking a pad from videomixer (if the stream in question no longer needs mixing in), and not linking to another pad.

In short, the 'safety' (against blocking; with indeed other effects) this patch tries to bring would probably not be needed *if*:
- (permanently) unlinking a (typically request) pad would be consistently followed by releasing this request pad
- muxing elements would properly cater for this by implementing releasing a request pad, i.e. implement a release_pad function.  If a muxer does not to this; then gstreamer core (in gst_element_release_request_pad) will just fall back to removing the pad from the element, and this pad would still be in the collectpads collection (but no longer in the element!) --> blocking, which is then known for sure not to be the intention or a good thing

It can indeed be argued that neither of the above is really responsibility of the core (and am as such hardly adamant about this patch), but a matter of 'proper programming', either in the applications or in the plugins.  So, abiding by those rules above, the current behaviour would cause no problems.  It would also at least require patches as in e.g. #374479 and #374658 (and possibly more?) to bring a few plugins up to specs/rules.

I hope this explains some concerns a bit better.
[maybe core could somehow act differently in gst_element_release_request_pad and/or the need for release_pad should be emphasized more to CollectPads users?]

(In reply to comment #3)
[...]
> I have no specific situation, but would occur e.g. when unlinking a pad from
> videomixer (if the stream in question no longer needs mixing in), and not
> linking to another pad.

Doing so would be a severe bug on the application side.  It has to release the pad it requested.  If this does not work correctly, we should look at videomixer (and adder).  It might be more complicated in the case of muxers.

> In short, the 'safety' (against blocking; with indeed other effects) this patch
> tries to bring would probably not be needed *if*:
> - (permanently) unlinking a (typically request) pad would be consistently
> followed by releasing this request pad

Application problem, see above.  "Fixing" this would only lead to more applications getting away with improper behaviour, further complicating things for everyone.

> - muxing elements would properly cater for this by implementing releasing a
> request pad, i.e. implement a release_pad function.  If a muxer does not to
> this; then gstreamer core (in gst_element_release_request_pad) will just fall
> back to removing the pad from the element, and this pad would still be in the
> collectpads collection (but no longer in the element!) --> blocking, which is
> then known for sure not to be the intention or a good thing

Sounds like a lot of muxers are broken then.  If this is the case, they should be fixed.  It's probably a bit of cut-and-paste work because there is no special base class for muxers to derive from (maybe it's time to create one?).

> It can indeed be argued that neither of the above is really responsibility of
> the core (and am as such hardly adamant about this patch), but a matter of
> 'proper programming', either in the applications or in the plugins.  So,
> abiding by those rules above, the current behaviour would cause no problems. 
> It would also at least require patches as in e.g. #374479 and #374658 (and
> possibly more?) to bring a few plugins up to specs/rules.

(mentioning the bug numbers with "bug" in front to generate correct autolinks: bug #374479 and bug #374658)

> I hope this explains some concerns a bit better.
> [maybe core could somehow act differently in gst_element_release_request_pad
> and/or the need for release_pad should be emphasized more to CollectPads
> users?]

Changing gst_element_release_request_pad cannot solve this, as it cannot know how the pad is used.  I think CollectPads documentation is clear enough (gst_collect_pads_add_pad even mentions explicitely that it will hold a reference to the pad).

Like you supposed, the problem we discuss here is not a deficiency of core but rather a bug in muxers that got replicated widely (as there is no convenient base class, where this probably belongs and could be fixed centrally).  Using CollectPads means more or less to have the pads be managed by CollectPads itself, and the default GstElement behaviour of release_pad interfers with that as you pointed out.  If a request pad is to be released, it has to be removed from the pad collection, too.  Good catch I'd say.

collectpads should not discard unlinked pads, if the muxer operates in pull mode, this would not trigger a pad block on the sinks and would make dynamic plugging impossible.
It also allows to easily switch sources on the muxer/mixer without having to block all other pads (to disallow potential muxing of the unlinked pad while reconnecting)

If a sinkpad is no longer used, it should be _unlinked and _released. It sucks that refcounting does not work here as the pad does not have any indication how it was created... that's 0.11 work.

Trying to figure out where the behaviour of muxers should be documented...