Created attachment 337401 [details] [review]
rtpbin: receive bundle support

The application can now instruct rtpbin to demultiplex incoming bundled
sessions and internally create the GstRtpSessions accordingly. To
enable this feature the application should request the
recv_bundle_rt{c}p_sink_%u pads.

Additionally a new signal named on-bundled-ssrc is provided and can be
used by the application to redirect the stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.

Created attachment 337402 [details]
OpenWebRTC transport agent with bundling support

Created attachment 337403 [details]
OpenWebRTC transport agent with bundling support disabled

Comment on attachment 337401 [details] [review]
rtpbin: receive bundle support

Hum this isn't the right version :)

Created attachment 337448 [details] [review]
rtpbin: receive bundle support

The application can now instruct rtpbin to demultiplex incoming
bundled sessions and internally create the GstRtpSessions accordingly. To
enable this feature the application should request the
recv_bundle_rt{c}p_sink_%u pads.

Additionally a new signal named on-bundled-ssrc is provided and can be
used by the application to redirect the stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.

I'm not sure I like the API. Especially that bundling is negotiatable.. Another option would be to keep the same pads, but once a new SSRC is discovered on an incoming pad, have a new signal that will ask "do you want me to redirect this SSRC to another session?". And some API (maybe an actial signal) that says "Move this SSRC to that session".

(In reply to Olivier Crête from comment #6)
> I'm not sure I like the API. Especially that bundling is negotiatable..
> Another option would be to keep the same pads, but once a new SSRC is
> discovered on an incoming pad, have a new signal that will ask "do you want
> me to redirect this SSRC to another session?". 

Ok this can be done without the separate recv_bundle sink pad templates indeed. I can update the patch accordingly, I think :)

I'm not sure what you mean with "do you want me to redirect this SSRC to another session?", I think the on-bundled-ssrc signal already allows something similar.

> And some API (maybe an actial
> signal) that says "Move this SSRC to that session".

When such signal would be needed? I don't understand the use-case for this.

Created attachment 337827 [details] [review]
rtpbin: receive bundle support

A new signal named on-bundled-ssrc is provided and can be
used by the application to redirect an stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.

Review of attachment 337827 [details] [review]:

Except for the little nits below, this generally looks good. I'd really appreciate if there was a unit test to make sure this doesn't get broken in the future. I think it would also be beneficial to avoid plugin the extra ssrcdemux if the signal is not connected (or if it returns 0).

::: gst/rtpmanager/gstrtpbin.c
@@ +672,3 @@
+  g_value_init (&result, G_TYPE_UINT);
+  g_value_init (&params[0], GST_TYPE_ELEMENT);
+  g_value_set_object (&params[0], rtpbin);

You need to unset the params[0] after emitting the signal otherwise you'll leak the element.

@@ +678,3 @@
+  g_signal_emitv (params,
+      gst_rtp_bin_signals[SIGNAL_ON_BUNDLED_SSRC], 0, &result);
+  if (G_VALUE_HOLDS_UINT (&result)) {

This will always be true, you just initialized it to G_TYPE_UINT earlier.

@@ +3560,3 @@
+    goto no_name;
+
+  GST_DEBUG_OBJECT (rtpbin, "finding session %d", sessid);

Use %u for debug since it's a guint.

(In reply to Olivier Crête from comment #9)
> Review of attachment 337827 [details] [review] [review]:
> 
> Except for the little nits below, this generally looks good. I'd really
> appreciate if there was a unit test to make sure this doesn't get broken in
> the future. I think it would also be beneficial to avoid plugin the extra
> ssrcdemux if the signal is not connected (or if it returns 0).
> 

With the current semantics, 0 means use the existing session. But thanks for the feedback, I'll work on a test and implement lazy ssrcdemux plugging :)

> ::: gst/rtpmanager/gstrtpbin.c
> @@ +672,3 @@
> +  g_value_init (&result, G_TYPE_UINT);
> +  g_value_init (&params[0], GST_TYPE_ELEMENT);
> +  g_value_set_object (&params[0], rtpbin);
> 
> You need to unset the params[0] after emitting the signal otherwise you'll
> leak the element.
> 

Ok!

> @@ +678,3 @@
> +  g_signal_emitv (params,
> +      gst_rtp_bin_signals[SIGNAL_ON_BUNDLED_SSRC], 0, &result);
> +  if (G_VALUE_HOLDS_UINT (&result)) {
> 
> This will always be true, you just initialized it to G_TYPE_UINT earlier.
> 

Ok!

> @@ +3560,3 @@
> +    goto no_name;
> +
> +  GST_DEBUG_OBJECT (rtpbin, "finding session %d", sessid);
> 
> Use %u for debug since it's a guint.

The rtpbin code is crippled with these %d where %u should be used. But yeah I can fix it!

Created attachment 338614 [details] [review]
rtpbin: receive bundle support

A new signal named on-bundled-ssrc is provided and can be
used by the application to redirect an stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.

Review of attachment 338614 [details] [review]:

I'd rather see num-buffers on the testsrcs than g_timeout_add(), but the rest is fine so you can push it.

Created attachment 339371 [details] [review]
rtpbin: receive bundle support

A new signal named on-bundled-ssrc is provided and can be
used by the application to redirect an stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.


Updated the test, removing the g_timeout_add()'s.
Also fixed an issue in new_bundled_src_pad_found() where the rt(c)p
funnel sink pad would be left disconnected and we'd request a new pad
instead.

The diff from the previous patch: https://bugzilla.gnome.org/attachment.cgi?oldid=338614&action=interdiff&newid=339371&headers=1

Review of attachment 339371 [details] [review]:

::: gst/rtpmanager/gstrtpbin.c
@@ +712,3 @@
+  }
+
+  gst_pad_link (src_pad, recv_rtp_sink);

Please check the return value of gst_pad_link(), if you're 100% sure it will always work, try gst_pad_link_full(,GST_PAD_LINK_CHECK_NOTHING). That is only ok if there is nothing linked on the outside yet on either side of the link.

@@ +3154,3 @@
   rtpbin = stream->bin;
 
+  GST_DEBUG ("new payload pad %u", pt);

Any reason this is not GST_DEBUG_OBJECT() ?

(In reply to Olivier Crête from comment #15)
> Review of attachment 339371 [details] [review] [review]:
> 
> ::: gst/rtpmanager/gstrtpbin.c
> @@ +712,3 @@
> +  }
> +
> +  gst_pad_link (src_pad, recv_rtp_sink);
> 
> Please check the return value of gst_pad_link(), if you're 100% sure it will
> always work, try gst_pad_link_full(,GST_PAD_LINK_CHECK_NOTHING). That is
> only ok if there is nothing linked on the outside yet on either side of the
> link.
> 

I'll check the gst_pad_link() result. The alternative can't be used, i'm afraid.

> @@ +3154,3 @@
>    rtpbin = stream->bin;
>  
> +  GST_DEBUG ("new payload pad %u", pt);
> 
> Any reason this is not GST_DEBUG_OBJECT() ?

I suppose not! Originally I was only fixing the format string :)

Created attachment 339782 [details] [review]
rtpbin: receive bundle support

A new signal named on-bundled-ssrc is provided and can be
used by the application to redirect an stream to a different
GstRtpSession or to keep the RTX stream grouped within the
GstRtpSession of the same media type.

Comment on attachment 339782 [details] [review]
rtpbin: receive bundle support

Thanks!

This was merged on https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=dcd3ce9751cdef0b5ab1fa118355f92bdfe82cb3 (>= 1.11.1)