GNOME Bugzilla – Bug 711084
rtpmanager: add new rtprtxsend and rtprtxreceive elements for retransmission
Last modified: 2014-01-03 20:19:43 UTC
Using RFC 4588 http://www.ietf.org/rfc/rfc4588.txt
Created attachment 258456 [details] [review] rtpmanager: add new rtprtxsend element
Created attachment 258457 [details] [review] rtpmanager: add new rtprtxreceive element
Created attachment 258458 [details] [review] rtpjitterbuffer: add ssrc and payload-type to GstRTPRetransmissionRequest
Created attachment 258459 [details] [review] tests/check: add rtprtx::test_push_forward_seq
Created attachment 258460 [details] [review] tests/check: add rtprtx::test_drop_one_sender unit test
Created attachment 258461 [details] [review] tests/check: add rtprtx::test_drop_multiple_sender unit test
Created attachment 258756 [details] [review] rtpmanager: add new rtprtxsend element
Created attachment 258757 [details] [review] rtpmanager: add new rtprtxreceive element
Created attachment 258758 [details] [review] rtpjitterbuffer: add ssrc and payload-type to GstRTPRetransmissionRequest
Created attachment 258759 [details] [review] rtpsession/gstrtpsession: set ssrc in custom upstream event GstRTPRetransmissionRequest
Created attachment 258760 [details] [review] tests/check: add rtprtx::test_push_forward_seq
Created attachment 258761 [details] [review] tests/check: add rtprtx::test_drop_one_sender unit test
Created attachment 258762 [details] [review] tests/check: add rtprtx::test_drop_multiple_sender unit test
Created attachment 258764 [details] [review] rtprtxsend/rtprtxreceive: generate gtk doc
Created attachment 259100 [details] [review] rtpmanager: add new rtprtxsend element
Created attachment 259101 [details] [review] rtpmanager: add new rtprtxreceive element
Created attachment 259102 [details] [review] rtpjitterbuffer: add ssrc and payload-type to GstRTPRetransmissionRequest
Created attachment 259103 [details] [review] rtpsession/gstrtpsession: set ssrc in custom upstream event GstRTPRetransmissionRequest
Created attachment 259104 [details] [review] tests/check: add rtprtx::test_push_forward_seq
Created attachment 259105 [details] [review] tests/check: add rtprtx::test_drop_one_sender unit test
Created attachment 259106 [details] [review] tests/check: add rtprtx::test_drop_multiple_sender unit test
Created attachment 259107 [details] [review] rtprtxsend/rtprtxreceive: generate gtk doc
Created attachment 259108 [details] [review] tests/check: Add rtprtx::test_rtxsender_packet_retention
Created attachment 259109 [details] [review] rtprtxsend: keep important buffer information in a private structure
Created attachment 259110 [details] [review] rtprtxsend: Handle the max_size_time property
Created attachment 259111 [details] [review] tests/check: Add unit test for rtxsend's max_size_time property
Created attachment 259112 [details] [review] rtprtxsend: retransmit packets in the same order as the rtx requests
Created attachment 259113 [details] [review] rtprtxsend: use a GSequence to implement the buffer queue
Created attachment 259114 [details] [review] rtprtxsend: use a realistic limit for the value of max-size-packets
Created attachment 259115 [details] [review] tests/check: add rtprtx::test_rtxreceive_data_reconstruction
Created attachment 259116 [details] [review] rtpsession: bump threshold for T_dither_max activation from 2 to 3
Created attachment 259117 [details] [review] doc: add design for rtp retransmission
Created attachment 259215 [details] [review] rtpsession: determine if the session is doing point-to-point
Created attachment 259244 [details] [review] rtpmanager: add new rtprtxreceive element
Created attachment 259245 [details] [review] rtpsession: determine if the session is doing point-to-point
could you please make a git repository available somewhere for this many patches?
And squash together all commits that add the new elements, keeping the ones that change existing code separately (but still squash them if it makes sense).
There was already a URL in the whiteboard of the bug, but it was a bit outdated (and also contained commits reported in other bug reports). As requested: http://cgit.collabora.com/git/user/gkiagia/gst-plugins-good.git/log/?h=rtx
Some quick comments: - The rtx elements don't have request pads so it's not possible to select session or SSRC multiplexing. - The rtpbin has a request srcpad now, this breaks ABI.
(In reply to comment #39) > Some quick comments: > > - The rtx elements don't have request pads so it's not possible to select > session or SSRC multiplexing. They do not support session multiplexing atm. When it is implemented, we would have an additional request pad on each of those elements, which would be the trigger for switching to session multiplexing (if it is requested, do session-multiplexing, otherwise do ssrc-multiplexing) > - The rtpbin has a request srcpad now, this breaks ABI. Can you elaborate? I don't understand this. rtpbin has not been changed here.
(In reply to comment #40) > > - The rtpbin has a request srcpad now, this breaks ABI. > > Can you elaborate? I don't understand this. rtpbin has not been changed here. Found it, you are actually talking about a change in rtpbin, which is part of https://bugzilla.gnome.org/show_bug.cgi?id=711087 This breaks ABI indeed...
(In reply to comment #41) > (In reply to comment #40) > > > - The rtpbin has a request srcpad now, this breaks ABI. > > > > Can you elaborate? I don't understand this. rtpbin has not been changed here. > > Found it, you are actually talking about a change in rtpbin, which is part of > https://bugzilla.gnome.org/show_bug.cgi?id=711087 > > This breaks ABI indeed... Fixed that one (see the other report or my rtx-full [1] branch) [1]. http://cgit.collabora.com/git/user/gkiagia/gst-plugins-good.git/log/?h=rtx-full
Created attachment 260304 [details] [review] rtpmanager: add new rtprtxsend / rtprtxreceive elements
Created attachment 260305 [details] [review] rtpjitterbuffer: add ssrc and payload-type to GstRTPRetransmissionRequest
Created attachment 260306 [details] [review] tests/check: add rtprtx::test_push_forward_seq
Comment on attachment 260305 [details] [review] rtpjitterbuffer: add ssrc and payload-type to GstRTPRetransmissionRequest > if (G_UNLIKELY (priv->clock_rate == -1)) { > /* no clock rate given on the caps, try to get one with the signal */ > if (gst_rtp_jitter_buffer_get_clock_rate (jitterbuffer, >@@ -2460,6 +2468,8 @@ do_expected_timeout (GstRtpJitterBuffer * jitterbuffer, TimerData * timer, > event = gst_event_new_custom (GST_EVENT_CUSTOM_UPSTREAM, > gst_structure_new ("GstRTPRetransmissionRequest", > "seqnum", G_TYPE_UINT, (guint) timer->seqnum, >+ "ssrc", G_TYPE_UINT, (guint) priv->last_ssrc, >+ "payload-type", G_TYPE_UINT, (guint) priv->last_pt, > "running-time", G_TYPE_UINT64, timer->rtx_base, > "delay", G_TYPE_UINT, GST_TIME_AS_MSECONDS (delay), > "retry", G_TYPE_UINT, timer->num_rtx_retry, The SSRC is already added by the ssrcdemux element I'm not sure why you would want the pt to be there, it could theoretically change for each packet and is thus not so interesting to transmit because it isn't related to the packet that got lost.
(In reply to comment #46) > (From update of attachment 260305 [details] [review]) > > if (G_UNLIKELY (priv->clock_rate == -1)) { > > /* no clock rate given on the caps, try to get one with the signal */ > > if (gst_rtp_jitter_buffer_get_clock_rate (jitterbuffer, > >@@ -2460,6 +2468,8 @@ do_expected_timeout (GstRtpJitterBuffer * jitterbuffer, TimerData * timer, > > event = gst_event_new_custom (GST_EVENT_CUSTOM_UPSTREAM, > > gst_structure_new ("GstRTPRetransmissionRequest", > > "seqnum", G_TYPE_UINT, (guint) timer->seqnum, > >+ "ssrc", G_TYPE_UINT, (guint) priv->last_ssrc, > >+ "payload-type", G_TYPE_UINT, (guint) priv->last_pt, > > "running-time", G_TYPE_UINT64, timer->rtx_base, > > "delay", G_TYPE_UINT, GST_TIME_AS_MSECONDS (delay), > > "retry", G_TYPE_UINT, timer->num_rtx_retry, > > The SSRC is already added by the ssrcdemux element Right. > I'm not sure why you would want the pt to be there, it could theoretically > change > for each packet and is thus not so interesting to transmit because it isn't > related to the packet that got lost. The pt is generally needed to be stored in rtprtxreceive so that the rtp packets can be fully reconstructed out of rtx packets, since rtx packets do not carry the original pt. Looking at the code though, it seems we can also store that internally in rtprtxreceive and discard this patch completely.
Created attachment 261457 [details] [review] rtprtxreceive: associate payload type internally, without depending on the rtx event to contain it This patch obsoletes the changes in jitterbuffer mentioned above by associating the payload type internally in rtprtxreceive.
Created attachment 261461 [details] [review] rtprtxsend: Add an rtx-ssrc property to allow external control of the ssrc
Created attachment 262964 [details] [review] rtprtxsend: Allow SSRC-multiplexing and multiple payload types in the original stream
Created attachment 263901 [details] [review] rtprtxsend: do not keep history of packets with an unknown payload type
Created attachment 263902 [details] [review] rtprtxreceive: modify to use a payload-type map like rtprtxsend
Created attachment 263903 [details] [review] test/check: Verify rtprtxsend::ssrc-map property works as expected
Created attachment 263904 [details] [review] rtprtxreceive: cleanup stored seqnum->ssrc association when an rtx request never gets answered
Created attachment 263905 [details] [review] test/check: add test to verify that rtprtxreceive cleans up orphan rtx requests Note that this unit test fails with: gstcheck.c:75:F:general:test_rtxreceive_cleanup_unanswered_requests:0: Unexpected critical/warning: static type 'GstElementFactory' unreferenced too often I don't know how to silence this. It can be ignored safely, though.
Review of attachment 259245 [details] [review]: In the section for "RFC 4585 section 3.5.2 step 4", probably should also allow early feedback in ptp mode. ::: gst/rtpmanager/rtpsession.c @@ +1341,3 @@ + data.new_addr = NULL; + g_hash_table_foreach (sess->ssrcs[sess->mask_idx], + (GHFunc) compare_rtp_source_addr, (gpointer) & data); May want to use g_hash_table_find() instead to stop iterating fast if the answer is FALSE, in that case, the session may be big (very big). Or maybe use g_hash_table_iter_*(). @@ +1344,3 @@ + is_doing_rtp_ptp = data.is_doing_ptp; + + /* same but about rtcp */ No need to check rtcp if is_doing_rtp_ptp is FALSE
Review of attachment 259117 [details] [review]: New Makefile.am needs to be added to configure.in
Created attachment 264077 [details] [review] rtpmanager: add new rtprtxsend / rtprtxreceive elements Squash commits...
Created attachment 264079 [details] [review] rtpsession: set ssrc in custom upstream event GstRTPRetransmissionRequest (just reworded the commit message)
Created attachment 264080 [details] [review] tests/check: add rtprtx::test_push_forward_seq squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Created attachment 264081 [details] [review] tests/check: add rtprtx::test_drop_one_sender unit test squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Created attachment 264082 [details] [review] tests/check: add rtprtx::test_drop_multiple_sender unit test squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Created attachment 264083 [details] [review] tests/check: Add rtprtx::test_rtxsender_packet_retention squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Created attachment 264084 [details] [review] tests/check: Add unit test for rtxsend's max_size_time property squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Created attachment 264085 [details] [review] tests/check: add rtprtx::test_rtxreceive_data_reconstruction squash changes from commits that were squashed into "add new rtprtxsend / rtprtxreceive elements"
Review of attachment 264077 [details] [review]: Some comments, nothing major ::: gst/rtpmanager/gstrtprtxreceive.c @@ +231,3 @@ + GstRtpRtxReceive *rtx = GST_RTP_RTX_RECEIVE (object); + + gst_rtp_rtx_receive_reset (rtx); This just empties tables that you'll destroy just below, not necessary. @@ +233,3 @@ + gst_rtp_rtx_receive_reset (rtx); + + if (rtx->ssrc2_ssrc1_map) { No need for these checks, finalize() is never called once and only once. @@ +506,3 @@ + GUINT_TO_POINTER (payload_type), NULL, NULL); + + g_mutex_unlock (&rtx->lock); Why do you unlock, then access the rtx_pt_map hash table, then relock ? @@ +527,3 @@ + if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map, + GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) { + GST_DEBUG use GST_DEBUG_OBJECT (not the only place) @@ +657,3 @@ + gst_structure_free (rtx->pending_rtx_pt_map); + rtx->pending_rtx_pt_map = g_value_dup_boxed (value); + rtx->rtx_pt_map_changed = TRUE; Like in rtxrecv, why the whole rtx_pt_map_changed dance? You hold the same clock, you can change it here directly. ::: gst/rtpmanager/gstrtprtxreceive.h @@ +46,3 @@ + GstPad *srcpad; + + GMutex lock; Why the new lock? why not the object lock ? ::: gst/rtpmanager/gstrtprtxsend.c @@ +199,3 @@ + +static void +gst_rtp_rtx_send_reset (GstRtpRtxSend * rtx, gboolean full) Unused _full() parameter. That said, it should probably do the reset on FLUSH also. @@ +455,3 @@ + s = gst_caps_get_structure (caps, 0); + gst_structure_get_uint (s, "ssrc", &ssrc); + data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc); Here you access ssrc_data without first taking the lock @@ +456,3 @@ + gst_structure_get_uint (s, "ssrc", &ssrc); + data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc); + gst_structure_get_int (s, "clock-rate", &data->clock_rate); Shouldn't the queue be flushed if the caps change ? @@ +536,3 @@ + /* get needed data from GstRtpRtxSend */ + ssrc = gst_rtp_buffer_get_ssrc (&rtp); + data = gst_rtp_rtx_send_get_ssrc_data (rtx, ssrc); Accessing the ssrc_data without the lock held. You probably need to create the new packets in _chain() before releasing the lock there. @@ +551,3 @@ + /* If payload type is not set through SDP/property then + * just bump the value */ + if (fmtp < 96) I know <96 isn't value, but we should still allow any value between 0 and 127. @@ +636,3 @@ + + /* add current rtp buffer to queue history */ + item = g_new0 (BufferQueueItem, 1); g_slice_new0(), helps reduce fragmentation @@ +738,3 @@ + gst_structure_free (rtx->pending_rtx_pt_map); + rtx->pending_rtx_pt_map = g_value_dup_boxed (value); + rtx->rtx_pt_map_changed = TRUE; Why do the rtx_pt_map_changed dance if you hold the same lock here? ::: gst/rtpmanager/gstrtprtxsend.h @@ +46,3 @@ + GstPad *srcpad; + + GMutex lock; Why the new lock? Why not just use the GST_OBJECT_LOCK() ?
Review of attachment 263904 [details] [review]: Looks good to me
This is partially pushed now with the following changes: - only pushed the RTX element commits - modified for the rtpbin AUX element handling - Did not commit the SSRC-seqnum association timeout, this does not need a jitterbuffer event and can be done from the RTX element itself because it knows the timeout and can use the incomming timestamps to see elapsed time. - did not commit GstTask on srcpad, it failed the unit test for me and the task should only push the RTX packets and should push the original stream in the same thread. I have the following comments: - I kept the src/sink pads, if we later implement session-multiplex we need to add a request rtx pad or add a property to make such a pad. You will need to have a bin to make an element with the right padnames for rtpbin. I tried with request pads with session numbers (src_%u) etc but that was overly complicated. about the RTXSender: - a hashtable lookup is done for each RTP packet to get the SSRC specific info. I considered removing this but kept it in the end, we can optimize this with a simple 1 item cache if needed. I'm not sure why exactly we would need to support SSRC multiplexing in the RTXsender.. - a second hashtable lookup is done to get the payload type. pt -> RTX pt mapping should be done with a simple array (there are only 127 pt-s possible) - for each RTP packet g_sequence_append is done, which does a rebalance of a tree(!) In case the queue exceeds its limits, a couple more rebalances are done(..). A GSequence is the wrong datastructure for this, you need a simple dynamic ringbuffer to keep packets around. If you need packet with seqnum M, you look at the seqnum (N) of the head of the ringbuffer and packet M is (M - N) positions in the ringbuffer. O(1) insertion and O(1) lookup that can be done without dynamic allocations when the ringbuffer reached its correct size. - making the RTX packets looks very inefficient and seems to do a copy of all the packet parts separately, surely doing one memcpy of everything is faster. Better would even be to not copy anything and _ref (or share) the parts that don't change... - limiting the size of the queue in time does not need the rtptime fields of the packets, just check the timestamps on the buffers. Similar remarks about the hashtables in rtxreceive.