The patch no longer applies, but it was probably not entirely correct even when it did apply.
The problem still persist though.

Review of attachment 222356 [details] [review]:

Looks almost good, I just don't know if there are any negative implications when exposing the unknown streams. Can't think of any right now

::: gst/playback/gstdecodebin2.c
@@ +1459,2 @@
   if (gst_caps_is_any (caps))
     goto any_caps;

This will now crash if caps==NULL

@@ +1460,3 @@
     goto any_caps;
 
   dpad = gst_decode_pad_new (dbin, pad, chain);

And this

@@ -1713,3 @@
-      gst_decode_bin_expose (dbin);
-    }
-    EXPOSE_UNLOCK (dbin);

You should still check if the chain is complete and expose then here

> I just don't know if there are any negative implications
> when exposing the unknown streams. Can't think of any right now

Note that this is not (should not be) done by default, but only if the "expose-all-streams" property is set explicitly to TRUE.

It is true by default, so we should add a new property? :)

Oh, my bad, I thought it was false by default. (Why is it true by default?)

Seems we are still coming short for what expose-all-streams seems
to mean. The current code has at least two seemingly analogous
do-not-expose conditions:

unknown_type: Type unknown, caps could not be fixated to a compatible
set (missing decoder ...)
discarded_type: Type known, no final caps due to failed nego (decoder
present, but stream variant not supported)

if any of these conditions are hit, the pad/stream is not exposed.
And this is where I think we have a problem. expose-all sounds like both
should be exposed no matter what, whereas the current patch only fixes(*)
the behavior for the unknown_type case.

As I see it there are likely 2 ways forward. Either we complete/fix the
patch as it is and change the expose-all-streams name to something more
intuitive like expose-unknown-streams, or we make sure the fix also
handles the discarded_type condition.

What do you think?

Edward added the expose-all-streams property a long time ago for discoverer IIRC. Maybe he can explain what his intention was :)


From my understanding both cases Reynaldo mentioned should be exposed if expose-all-streams is TRUE. As this is the default setting this might break existing code in unexpected ways as they suddenly get pads with unknown types.


The current behaviour is that if !expose-all-streams we never expose raw pads, and we never expose pads which have caps for which there is an decoder that does not output final caps. Unless I misunderstand what the code does, this doesn't make any sense at all to me :)

The goal was to avoid as quickly as possible to add unneeded decoders/streams.

If the final caps are raw video (for example), don't attempt to plug/create audio decoders.

This was added for efficient encodebin and gnonlin usages.

(In reply to comment #8)
> The goal was to avoid as quickly as possible to add unneeded decoders/streams.
> 
> If the final caps are raw video (for example), don't attempt to plug/create
> audio decoders.

The code does not do that though. Instead it never exposes 1) raw pads (as with the default raw caps, which include audio/video/text caps), or 2) pads that would be plugged to decoders that don't produce final (i.e. raw) caps.

1) Probably should check not the default raw caps but the signals and the property
2) Is dangerous because it would also not use a pad if there's a single decoder in the list that outputs only silly stuff like video/x-raw(memory:VASurface) and not video/x-raw.

I think the behaviour of that property is currently broken.


Apart from that, what this bug is about apparently is different from what the expose-all-streams property was supposed to solve. And I think we should add a second property for that so that decodebin also exposes pads with unknown types.

Edward, I'm not sure I understand you clearly enough. Can you elaborate
a bit on what do you want this property to do? Idea would be to fix
it to match the original intent and close this one for once and for all.
I should be able to take care of it.

Note that the original intent is not what this bug was about, if I understand it correctly. Still what you said should be done first, and then we can add something new but related for this bug.

The original intent is the following:

You use decodebin (or uridecodebin) in your pipeline (because you don't want to deal with figuring out demuxer, decoder, what to plug, ...).

You only want to use a certain type of stream from it. You set the caps property to the streams you're interested in (say ... just video/x-raw) and set expose-all-streams to False.

This will (or rather "should" based on sebastian's remarks):
1) Only expose pads that match that
2) *remove* all decoders and intermediary elements that are not used

This was done for a performance reason (so you don't have elements you know you'll never ever need around).

OK. Still, the case failing to achieve what the property name
seems to point to is the expose-all-streams = TRUE one. Under
this light, should I go forward and fix this so it properly
exposes the two cases discussed in comment #6?

For reference:

unknown_type: Type unknown, caps could not be fixated to a compatible
set (missing decoder ...)
discarded_type: Type known, no final caps due to failed nego (decoder
present, but stream variant not supported)

(In reply to comment #13)
> OK. Still, the case failing to achieve what the property name
> seems to point to is the expose-all-streams = TRUE one. Under
> this light, should I go forward and fix this so it properly
> exposes the two cases discussed in comment #6?

No, first of all it should be fixed to work the way how Edward envisioned it. See my comments above for the broken behaviour.

Then this bug here should be fixed, i.e. provide a new property to select that all (even unknown!) streams should be exposed. We can't do that with the existing expose-all-streams property (==TRUE) because it can easily break existing code. Nothing expects to get arbitrary unknown streams exposed on decodebin, only "raw" and known streams are expected.

Also potentially related bug #710444

Closing since this isn't an issue with decodebin3.