the issue here is
in case of rtsp, it is adding audio and video streams in two different decodebin

0:00:02.279963303 18739 0xb10020f0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element decodebin0 to bin uridecodebin0
0:00:02.325272571 18739 0xb10020f0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element rtph263pdepay0 to bin decodebin0
0:00:02.334828185 18739 0xb10020f0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element h263parse0 to bin decodebin0
0:00:02.374257166 18739 0xb10020f0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element avdec_h263-0 to bin decodebin0

0:00:02.610085720 18739 0xb6114ec0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element decodebin1 to bin uridecodebin0
0:00:02.633267848 18739 0xb6114ec0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element rtpmp4gdepay0 to bin decodebin1
0:00:02.646833310 18739 0xb6114ec0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element aacparse0 to bin decodebin1
0:00:02.693690282 18739 0xb6114ec0 DEBUG          GST_PARENTAGE gstbin.c:1302:gst_bin_add: adding element avdec_aac0 to bin decodebin1


So it always shows the latest added streams' topology.

probably we should merge the structures for both the stream topologies.
Not sure if this is the right way to proceed

Created attachment 310151 [details] [review]
merge topologies from multiple decodebins

Tried to fix the issue by adding all the different topologies into a dummy container and show them..


Before the change it was like
Topology:
  unknown: application/x-rtp
    audio: MPEG-4 AAC

After this change it shows as below
Topology:
  container: (null)
    unknown: application/x-rtp
      video: ITU H.26n
    unknown: application/x-rtp
      audio: MPEG-4 AAC

Not sure if there is any other simpler way to do it..


Please check if this is fine.
We can probably live without this change as well :)...

Review of attachment 310151 [details] [review]:

I think the merging should better take place inside uridecodebin

::: gst-libs/gst/pbutils/gstdiscoverer.c
@@ +1233,3 @@
 
+    if (dc->priv->current_topology) {
+      if (g_list_length (dc->priv->current_topology) > 1) {

> 1? Not > 0? You need to merge once there already is one topology and a new one arrives, no?

@@ +1238,3 @@
+            g_object_new (GST_TYPE_DISCOVERER_CONTAINER_INFO, NULL);
+
+        dc->priv->current_info->stream_info = (GstDiscovererStreamInfo *) cont;

Now if another topology arrives afterwards, you would add this container here and the new topology into yet another container. I think it would be better to merge them into the same container instead.

(In reply to Sebastian Dröge (slomo) from comment #3)
> Review of attachment 310151 [details] [review] [review]:
> 
> I think the merging should better take place inside uridecodebin
> 
I am guessing that topology is used only in case of discover? If that is the case i felt, it is not that important to make the changes in uridecodebin..
But yeah that will help in consistency..


> ::: gst-libs/gst/pbutils/gstdiscoverer.c
> @@ +1233,3 @@
>  
> +    if (dc->priv->current_topology) {
> +      if (g_list_length (dc->priv->current_topology) > 1) {
> 
> > 1? Not > 0? You need to merge once there already is one topology and a new one arrives, no?
> 
> @@ +1238,3 @@
> +            g_object_new (GST_TYPE_DISCOVERER_CONTAINER_INFO, NULL);
> +
> +        dc->priv->current_info->stream_info = (GstDiscovererStreamInfo *)
> cont;
> 
> Now if another topology arrives afterwards, you would add this container
> here and the new topology into yet another container. I think it would be
> better to merge them into the same container instead.

In ideal cases, discoverer_collect will be called after all the topologies are received. The reason i am checking for >1 is, a new container should be created only if there are more than one topology.

(In reply to Vineeth from comment #4)
> (In reply to Sebastian Dröge (slomo) from comment #3)
> > Review of attachment 310151 [details] [review] [review] [review]:
> > 
> > I think the merging should better take place inside uridecodebin
> > 
> I am guessing that topology is used only in case of discover? If that is the
> case i felt, it is not that important to make the changes in uridecodebin..
> But yeah that will help in consistency..

We don't know what else it using it :) 

> > ::: gst-libs/gst/pbutils/gstdiscoverer.c
> > @@ +1233,3 @@
> >  
> > +    if (dc->priv->current_topology) {
> > +      if (g_list_length (dc->priv->current_topology) > 1) {
> > 
> > > 1? Not > 0? You need to merge once there already is one topology and a new one arrives, no?
> > 
> > @@ +1238,3 @@
> > +            g_object_new (GST_TYPE_DISCOVERER_CONTAINER_INFO, NULL);
> > +
> > +        dc->priv->current_info->stream_info = (GstDiscovererStreamInfo *)
> > cont;
> > 
> > Now if another topology arrives afterwards, you would add this container
> > here and the new topology into yet another container. I think it would be
> > better to merge them into the same container instead.
> 
> In ideal cases, discoverer_collect will be called after all the topologies
> are received. The reason i am checking for >1 is, a new container should be
> created only if there are more than one topology.

What does "ideal case" mean here? Is it always called after everything is collected right before exposing the information or can it be callde multiple times? :)

i could not find any cases where another new topology arrives after discoverer_collect is called. 
from the code, discover collect will be called only for EOS, ASYNC_DONE.
Or even if it is called before, it checks if stream is available, which will not be available until a pad is added.

i maybe wrong :)

Seems ok to do it like that then, but still the message merging should happen in uridecodebin :) It's the place where there are multiple decodebins and its always the bins job to merge messages of the child elements as required.

Created attachment 310324 [details] [review]
uridecodebin: merge topologies from multiple decodebin

I have made the changes in uridecodebin..

Here i am just ignoring the message posted from decodebin and instead as soon as all the decodebin elements are added, getting the topology for each decodebin and merging all of them into a structure and posting the message.

Have a little doubt if i am calling it in the right place..

Please review :)

Review of attachment 310324 [details] [review]:

I think this is not the right approach. In uridecodebin (and playbin for that matter too, it can have two uridecodebin) you should catch the messages with the topology. But only forward the message with the topology once all decodebins have posted theirs, and the forwarded message then contains all topologies merged together.

It should still all be triggered by messages, just that you aggregate them. Similar to what is done with the buffering messages.

(In reply to Sebastian Dröge (slomo) from comment #9)
> Review of attachment 310324 [details] [review] [review]:
> 
> I think this is not the right approach. In uridecodebin (and playbin for
> that matter too, it can have two uridecodebin) you should catch the messages
> with the topology. But only forward the message with the topology once all
> decodebins have posted theirs, and the forwarded message then contains all
> topologies merged together.
> 
> It should still all be triggered by messages, just that you aggregate them.
> Similar to what is done with the buffering messages.

I was not sure when to post the message after aggregating.

I mean how will uridecodebin come to know that there are no more decodebin messages being posted??.

no_more_pads_full is the function where we know if there are no more decodebins being added. But the element message is being posted after this.


One way i can think of is,
keep aggregating and keep posting the message..
This way once first decodebin message gets posted it will be forwarded..
when the second decodebin posts message, then both first and second will be aggregated and forwarded.



Please guide if there is any better way to know when uridecodebin will be sure that no more messages will be posted from decodebin


PS: I tried posting on paused_to_playing state change.. But that seems to be too late and discoverer doesn't get the message.

Created attachment 312002 [details] [review]
alternate method to aggregate toplologies.

As mentioned in the previous comment, 
this is another method by which we can aggregate and post the message.


The best way is post a single message after aggregating all the messages if that is possible. But i am not able to find any such place where all this works :)

I think it should work the following way: you post the first aggregated message once each decodebin has posted a message, from then on you post an aggregated message whenever a decodebin tells you about any changes.
When uridecodebin goes back to READY, and then PAUSED/PLAYING again, you wait again until each decodebin has posted the message.

Hi, do you have any progress here? I could pick up work on it if you're not interested anymore, but if you're still planning to finish this patch, just let me know :)

Hi i wont be able to work on this particular issue at least for a week or two..
please free to work on it if you are interested :)..

Now it works even worse. RTSP detects neither the audio nor the video:

vivia@erato /u/l/s/g/h/g/examples (master)> ./test-video
stream ready at rtsp://127.0.0.1:8554/test

vivia@erato /u/l/s/g/h/gst-plugins-bad (master) [141]> 
gst-discoverer-1.0 rtsp://127.0.0.1:8554/test
Analyzing rtsp://127.0.0.1:8554/test
Done discovering rtsp://127.0.0.1:8554/test

Vivia, that last issue is fixed (https://bugzilla.gnome.org/show_bug.cgi?id=754178).

Regarding this specific issue, I'm actually unsure what the best thing to do is to be honest.

What you propose, Sebastian, means that we will always get a bogus top-level "container" that isn't actually a container.

Even if we start special-casing things, like in that case, it seems weird to me to say that the streams discovered for a rtsp uri are *contained* by something, doesn't it ?

So unless we decide that a protocol is a "container", we need to figure out how to let the discoverer know that the decoded uri maps to several streams / containers IMHO.

Curious to know your thoughts, I'm willing to work on this, just not sure what end result we're actually after, apart from "We want to see the information for all the streams" :)

(In reply to Mathieu Duponchelle from comment #16)

> Even if we start special-casing things, like in that case, it seems weird to
> me to say that the streams discovered for a rtsp uri are *contained* by
> something, doesn't it ?

It seems not too far fetched to say that the streams are "contained" in the SDP from the server, but yes we have no actual pad for this. So discoverer would indeed need a way to have several top-level streams.

Created attachment 354539 [details] [review]
uridecodebin: aggregate topology messages

Here's yet another take on this, as Sebastian asked previously, this patch only posts a topology once all the decodebins have posted theirs, then posts a new, correctly updated topology every time a decodebin posts a new topology (the old topology is replaced with the new one).

As for the whole "container" issue, I'm afraid we can't do much without breaking an unfortunate amount of programs calling that API.

The previously proposed patch had all discoveries wrapped in a dummy container, this new patch at least retains the old behavior for the standard case where a single topology was obtained and propagated upstream, the only case where we do have a dummy container is the previously-broken case, such as the one this issue was about.

Here's an example output:

 meh   master  ~  devel  sandbox  gst-build-master  GST_DEBUG=0 gst-discoverer-1.0 -v rtsp://184.72.239.149/vod/mp4:BigBuckBunny_115k.mov
Analyzing rtsp://184.72.239.149/vod/mp4:BigBuckBunny_115k.mov
Done discovering rtsp://184.72.239.149/vod/mp4:BigBuckBunny_115k.mov

Topology:
  container: text/x-uri
    unknown: application/x-rtp, media=(string)video, payload=(int)97, clock-rate=(int)90000, encoding-name=(string)H264, packetization-mode=(string)1, profile-level-id=(string)42C01E, sprop-parameter-sets=(string)"Z0LAHtkDxWhAAAADAEAAAAwDxYuS\,aMuMsg\=\=", a-framesize=(string)240-160, a-sdplang=(string)en, a-cliprect=(string)"0\,0\,160\,240", a-framerate=(string)24.0, ssrc=(uint)695662262, clock-base=(uint)0, seqnum-base=(uint)1, npt-start=(guint64)0, npt-stop=(guint64)596480000000, play-speed=(double)1, play-scale=(double)1
      video: video/x-h264, stream-format=(string)avc, alignment=(string)au, codec_data=(buffer)0142c01effe100156742c01ed903c56840000003004000000c03c58b9201000468cb8cb2, level=(string)3, profile=(string)constrained-baseline, width=(int)240, height=(int)160, framerate=(fraction)0/1, interlace-mode=(string)progressive, parsed=(boolean)true
        Tags:
          video codec: H.264 (Constrained Baseline Profile)
        
        Codec:
          video/x-h264, stream-format=(string)avc, alignment=(string)au, codec_data=(buffer)0142c01effe100156742c01ed903c56840000003004000000c03c58b9201000468cb8cb2, level=(string)3, profile=(string)constrained-baseline, width=(int)240, height=(int)160, framerate=(fraction)0/1, interlace-mode=(string)progressive, parsed=(boolean)true
        Additional info:
          None
        Stream ID: b62e9a00f1bd1881d4e436344e14b135d8ac17373100c4fe9903286bb006129a/video:0:0:RTP:AVP:97
        Width: 240
        Height: 160
        Depth: 24
        Frame rate: 0/1
        Pixel aspect ratio: 1/1
        Interlaced: false
        Bitrate: 0
        Max bitrate: 0
    unknown: application/x-rtp, media=(string)audio, payload=(int)96, clock-rate=(int)12000, encoding-name=(string)MPEG4-GENERIC, encoding-params=(string)2, profile-level-id=(string)1, mode=(string)AAC-hbr, sizelength=(string)13, indexlength=(string)3, indexdeltalength=(string)3, config=(string)1490, a-sdplang=(string)en, ssrc=(uint)969373029, clock-base=(uint)0, seqnum-base=(uint)1, npt-start=(guint64)0, npt-stop=(guint64)596480000000, play-speed=(double)1, play-scale=(double)1
      audio: audio/mpeg, mpegversion=(int)4, stream-format=(string)raw, codec_data=(buffer)1490, framed=(boolean)true, level=(string)1, base-profile=(string)lc, profile=(string)lc, rate=(int)12000, channels=(int)2
        Tags:
          audio codec: MPEG-4 AAC
        
        Codec:
          audio/mpeg, mpegversion=(int)4, stream-format=(string)raw, codec_data=(buffer)1490, framed=(boolean)true, level=(string)1, base-profile=(string)lc, profile=(string)lc, rate=(int)12000, channels=(int)2
        Additional info:
          None
        Stream ID: b9049c323800fa1dbf0c9c2f5d6dcf0e63b50fc2c5030d1c14e44a893d14e333/audio:0:0:RTP:AVP:96
        Language: <unknown>
        Channels: 2
        Sample rate: 12000
        Depth: 32
        Bitrate: 0
        Max bitrate: 0

Properties:
  Duration: 0:09:56.480000000
  Seekable: yes
  Tags: 
      audio codec: MPEG-4 AAC
      video codec: H.264 (Constrained Baseline Profile)

 meh   master  ~  devel  sandbox  gst-build-master 

I'd argue that container with `text/x-uri` sort of makes sense a weird way, but I'm a bit biased, I guess follow-up patches could be made to have gst-discoverer interpret this as a newly-defined `GST_DISCOVERER_STREAM_COLLECTION_INFO` or somesuch, but I'm afraid this could break existing users of the API, thoughts on that ?

Review of attachment 354539 [details] [review]:

I also wonder if you shouldn't drop the counter and just iterate over the whoel list of decodebins every time, this is probably less error-prone.

::: gst/playback/gsturidecodebin.c
@@ +118,3 @@
   guint64 ring_buffer_max_size; /* 0 means disabled */
+
+  gint pending_topologies;

How is this counter locked?

@@ +2467,3 @@
+      sttype = gst_structure_get_name_id (structure);
+
+      if (sttype == topology_structure_name) {

Why not just use gst_message_has_name() ?

@@ +2471,3 @@
+        GstElement *element = GST_ELEMENT (GST_MESSAGE_SRC (msg));
+
+        gst_message_unref (msg);

After this structure is no longer valid, you need to keep the message alive... it owns the GstStructure

@@ +2483,3 @@
+
+        g_object_set_data (G_OBJECT (element), "topology",
+            gst_structure_copy (structure));

Since you free'd msg, the GstStructure pointer is not valid anymore.

Also, I think that will leak the structure.. Mabye you want to use g_object_set_data_full() instead. Then you can also drop the gst_structure_free() in remove_decoders().

Created attachment 354714 [details] [review]
uridecodebin: aggregate topology messages

This is based on Matthieu's commit, I tried to simplify it a bit. The only thing I'm not sure about is the text/x-uri as the container name. I'd love ot have somethign like network/rtsp.

Created attachment 354715 [details] [review]
uridecodebin: aggregate topology messages

Same as my previous patch, but does "application/<uri's protocol>" as the caps for the pseudo-container, what do you think?

Review of attachment 354715 [details] [review]:

This looks like a good improvement, there was indeed no need for ->pending_topologies

::: gst/playback/gsturidecodebin.c
@@ +2461,3 @@
+        gboolean has_all_topo = TRUE;
+
+        if (dec->pending || (dec->decodebins && dec->decodebins->next != NULL)) {

Why do we need to check for dec->pending here ?

The idea of using such a "pseudo" container makes sense IMHO, didn't look at the implementation though. Would be nice to get this fixed, thanks for working on it.

(In reply to Mathieu Duponchelle from comment #24)
> Why do we need to check for dec->pending here ?

Because we need for the no-more-pads from the source I think, otherwise we get an empty topology for RTSP. I think it's basically the same thing that was done with the redundant pending_topologies.

(In reply to Olivier Crête from comment #26)
> (In reply to Mathieu Duponchelle from comment #24)
> > Why do we need to check for dec->pending here ?
> 
> Because we need for the no-more-pads from the source I think, otherwise we
> get an empty topology for RTSP. I think it's basically the same thing that
> was done with the redundant pending_topologies.

I don't understand that, where do we "get an empty topology" ? In discoverer ? If so, how can it be empty, as what now triggers uuridecodebin to post a topology is getting a (presumably non-empty) topology from one of its decodebins.

As we increment ->pending along with prepending to the list of decodebins, I think checking for it and for "all_topos" is redundant ?

> As we increment ->pending along with prepending to the list of decodebins, I
> think checking for it and for "all_topos" is redundant ?

Actually I just realized pending was also incremented when the source has dynamic pads, which is our case here, and this protects us from a race where the first decodebin would post a topology before the source had exposed all its pads, leading to a not empty but still incomplete topology, accepted commit now then :D

Review of attachment 354715 [details] [review]:

No more comments :)

Merged

commit bfba2134377b2864a6dbf4f7c13967ec603c037e
Author: Mathieu Duponchelle <mathieu.duponchelle@collabora.com>
Date:   Tue Jun 27 02:21:22 2017 +0200

    uridecodebin: aggregate topology messages
    
    This makes it possible for GstDiscoverer to work with sources that
    have multiple source pads and hence will trigger the creation of multiple
    decodebin instances such as rtspsrc.
    
    Based on the work of Vineeth TM <vineeth.tm@samsung.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754178