GNOME Bugzilla – Bug 772740
rtpbin: receiving RTP bundle support
Last modified: 2017-05-03 18:50:01 UTC
As per the draft-ietf-mmusic-sdp-bundle-negotiation-33 [0] draft IETF spec, some changes would be needed in rtpbin to properly receive bundled RTP streams. I'll attach a patch. [0] https://datatracker.ietf.org/doc/draft-ietf-mmusic-sdp-bundle-negotiation/
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 (¶ms[0], GST_TYPE_ELEMENT); + g_value_set_object (¶ms[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 (¶ms[0], GST_TYPE_ELEMENT); > + g_value_set_object (¶ms[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)