GNOME Bugzilla – Bug 719938
rtpbin: allow dynamic RTP/RTCP encoders and decoders
Last modified: 2014-01-08 09:24:23 UTC
rtpbin currently only works with rtp packets. It would be great if rtpbin allowed to hook encoders and decoders so it could be used with srtp or dtls-srtp.
Created attachment 263627 [details] [review] rtpbin dynamic encoders/decoder (#1) First attempt to start discussions and request for comments. From the commit log: rtpbin: allow dynamic RTP/RTCP encoders/decoders * gst/rtpmanager/gstrtpbin.[ch]: four new action signals have been added (request-rtp-encoder, request-rtp-decoder, request-rtcp-encoder and request-rtcp-decoder). The user will be able to provide encoders or decoders dynamically. The encoders must follow the srtpenc API and the decoders the srtpdec API. Having separate signals for RTP and RTCP allows the user to use different encoders/decoders or provide the same one (e.g. that would be the case for srtpenc). Also, rtpbin now allows application/x-srtp in its pads.
Review of attachment 263627 [details] [review]: ::: gst/rtpmanager/gstrtpbin.c @@ +2523,3 @@ + g_value_set_object (&ret, NULL); + + g_signal_emitv (args, gst_rtp_bin_signals[signal], 0, &ret); Why is this not just g_signal_emit (session->bin, gst_rtp_bin_signals[signal], session->id, &ret); ? @@ +2547,3 @@ + } else { + GST_DEBUG_OBJECT (session->bin, "adding requested encoder %p", encoder); + gst_bin_add (GST_BIN_CAST (session->bin), encoder); This should check the return value. @@ +2862,3 @@ + GstPadTemplate *etempl; + + session->rtp_dec_queue = gst_element_factory_make ("queue", NULL); Why did you add a queue? @@ +2870,3 @@ + GST_DEBUG_OBJECT (rtpbin, "linking RTP decoder"); + decsink = gst_element_get_static_pad (decoder, "rtp_sink"); + gst_pad_link_full (qsrc, decsink, GST_PAD_LINK_CHECK_NOTHING); The decoder is application provided, this should probably do full checking, and check the return value. @@ +2876,3 @@ + decsrc = gst_element_get_static_pad (decoder, "rtp_src"); + gst_pad_link_full (decsrc, session->recv_rtp_sink, + GST_PAD_LINK_CHECK_NOTHING); Same here. @@ +2885,3 @@ + templ = gst_pad_template_new (GST_PAD_TEMPLATE_NAME_TEMPLATE (templ), + GST_PAD_TEMPLATE_DIRECTION (templ), GST_PAD_REQUEST, + GST_PAD_TEMPLATE_CAPS (etempl)); Creating a fake template looks like a hack. @@ +3151,3 @@ + GstPadTemplate *etempl; + + session->rtp_enc_queue = gst_element_factory_make ("queue", NULL); Again why the queue ?
Thanks for the review! (In reply to comment #2) > Review of attachment 263627 [details] [review]: > > ::: gst/rtpmanager/gstrtpbin.c > @@ +2523,3 @@ > + g_value_set_object (&ret, NULL); > + > + g_signal_emitv (args, gst_rtp_bin_signals[signal], 0, &ret); > > Why is this not just g_signal_emit (session->bin, gst_rtp_bin_signals[signal], > session->id, &ret); ? > True. > @@ +2547,3 @@ > + } else { > + GST_DEBUG_OBJECT (session->bin, "adding requested encoder %p", encoder); > + gst_bin_add (GST_BIN_CAST (session->bin), encoder); > > This should check the return value. > OK. > @@ +2862,3 @@ > + GstPadTemplate *etempl; > + > + session->rtp_dec_queue = gst_element_factory_make ("queue", NULL); > > Why did you add a queue? > I was doing some performance test and it seemed to perform better. I was just working on that right now, because I was not completely sure. > @@ +2870,3 @@ > + GST_DEBUG_OBJECT (rtpbin, "linking RTP decoder"); > + decsink = gst_element_get_static_pad (decoder, "rtp_sink"); > + gst_pad_link_full (qsrc, decsink, GST_PAD_LINK_CHECK_NOTHING); > > The decoder is application provided, this should probably do full checking, and > check the return value. > OK. > @@ +2876,3 @@ > + decsrc = gst_element_get_static_pad (decoder, "rtp_src"); > + gst_pad_link_full (decsrc, session->recv_rtp_sink, > + GST_PAD_LINK_CHECK_NOTHING); > > Same here. > OK. > @@ +2885,3 @@ > + templ = gst_pad_template_new (GST_PAD_TEMPLATE_NAME_TEMPLATE (templ), > + GST_PAD_TEMPLATE_DIRECTION (templ), GST_PAD_REQUEST, > + GST_PAD_TEMPLATE_CAPS (etempl)); > > Creating a fake template looks like a hack. > Will look into that. > @@ +3151,3 @@ > + GstPadTemplate *etempl; > + > + session->rtp_enc_queue = gst_element_factory_make ("queue", NULL); > > Again why the queue ? Same as before, I'll check again.
Created attachment 263629 [details] [review] rtpbin dynamic encoders/decoder (#2) Just implemented all the changes suggested in comment 2. About the fake template I initially used, it was before I updated the rtpbin static caps. Anyway, not necessary at all and an awful hack.
Review of attachment 263629 [details] [review]: Apart for the two little details, I think we're pretty much there. Wim? ::: gst/rtpmanager/gstrtpbin.c @@ +2517,3 @@ + if (!gst_bin_add (GST_BIN_CAST (session->bin), encoder)) + goto add_failed; + gst_element_sync_state_with_parent (encoder); Would probably make sense to check this return value also maybe ? @@ +2849,3 @@ + GST_DEBUG_OBJECT (rtpbin, "linking RTP decoder"); + decsink = gst_element_get_static_pad (decoder, "rtp_sink"); + decsrc = gst_element_get_static_pad (decoder, "rtp_src"); Should check if the pads exist.
(In reply to comment #5) > Review of attachment 263629 [details] [review]: > Thanks for the review again. > Apart for the two little details, I think we're pretty much there. Wim? > > ::: gst/rtpmanager/gstrtpbin.c > @@ +2517,3 @@ > + if (!gst_bin_add (GST_BIN_CAST (session->bin), encoder)) > + goto add_failed; > + gst_element_sync_state_with_parent (encoder); > > Would probably make sense to check this return value also maybe ? > I'll check the return value but only report a warning. > @@ +2849,3 @@ > + GST_DEBUG_OBJECT (rtpbin, "linking RTP decoder"); > + decsink = gst_element_get_static_pad (decoder, "rtp_sink"); > + decsrc = gst_element_get_static_pad (decoder, "rtp_src"); > > Should check if the pads exist. OK. I'll send a patch tomorrow. If everyone else is fine with this I'll add a bit of documentation.
Created attachment 263695 [details] [review] rtpbin dynamic encoders/decoder (#3) Added suggestions in comment 5 plus a bit of documentation.
looks very neat. I have a question. For the encoder you have numbered sink and source pads that need to be linked to the session. Is this so that you can have 1 element linking to multiple sessions? Won't it make sense to have this for the decoder as well then?
This is needed for the encoder, because we need to count how many bytes were encoded with the same key to expire it. The same key can be shared across many sessions, hence the multi-pad API. For the decoder, it doesn't matter, so srtpdec, you just put one per session. The only reason rtp & rtcp are together in the same srtpdec instance is that it performs RTP/RTCP demuxing, otherwise would have made it a simple elemen with a single src and sink pad.
commit 47c65fc269c0f83f710b88b446d8430fcd7ca231 Author: Aleix Conchillo Flaqué <aleix@oblong.com> Date: Thu Dec 5 15:53:52 2013 -0800 rtpbin: allow dynamic RTP/RTCP encoders/decoders * gst/rtpmanager/gstrtpbin.[ch]: four new action signals have been added (request-rtp-encoder, request-rtp-decoder, request-rtcp-encoder and request-rtcp-decoder). The user will be able to provide encoders or decoders dynamically. The encoders must follow the srtpenc API and the decoders the srtpdec API. Having separate signals for RTP and RTCP allows the user to use different encoders/decoders or provide the same one (e.g. that would be the case for srtpenc). Also, rtpbin now allows application/x-srtp in its pads. https://bugzilla.gnome.org/show_bug.cgi?id=719938
Please write unit tests next time: commit 76e4cbc7538be36ec9929c1af54b5515220b5224 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 15:16:09 2013 +0100 tests: add unit test for encoder element commit 05c8edc17499d9abc17956b964be536298ec9264 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 15:15:43 2013 +0100 rtpbin: fix memory leaks commit bcd1589a913735eba546968a95344f6dcfb6de63 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 15:03:34 2013 +0100 tests: fix leak commit 9345c2280a427d34911cbf75d8b40aa388482af5 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 15:00:50 2013 +0100 rtpbin: expect the pads on the encoders Don't use request pads for the encoder elements, the signal handler should request the pads and make sure they are available with the right name. commit cbc80d10dd4ce955af616eee34ceb549cf4754c5 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 14:56:07 2013 +0100 rtpbin: request-rtp-encoder are no action signals The request-rtp-encoder signals are not action signals so mark them correctly and use an accumulator to collect the result value.
There is still a problem: you are supposed to share the encoder elements between sessions but the list of encoder elements is kept per session. This then make it add the encoder element multiple times to the bin, which will fail.
commit 3f3b2d0886a6581c4e590cf132f49fe41275bb30 Author: Wim Taymans <wtaymans@redhat.com> Date: Mon Dec 30 16:28:57 2013 +0100 rtpbin: handle multiple encoder instances Keep track of elements that are added to multiple sessions and make sure we only add them to the rtpbin once and that we clean them when no session refers to them anymore.
Just got back from vacations. This is great. Thank you for the extra work! Will do UT next time.
Created attachment 265570 [details] [review] remove list of decoders Forgot to remove list of decoders in this commit: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/rtpmanager/gstrtpbin.c?id=ae22c958819abd75f0da25892ceb1ae84476ba1b
Comment on attachment 265570 [details] [review] remove list of decoders commit 441f286e28bf79859e2b4df21645c11d4e944657 Author: Aleix Conchillo Flaqué <aleix@oblong.com> Date: Tue Jan 7 12:13:51 2014 -0800 rtpbin: remove unused list of decoders remove list of decoders, which are already handled by the list of elements. https://bugzilla.gnome.org/show_bug.cgi?id=719938