From 981d0c02de7229edcaf2e31843df4899692779d1 Mon Sep 17 00:00:00 2001 From: Havard Graff Date: Thu, 16 Apr 2020 16:47:50 +0200 Subject: [PATCH] rtpjitterbuffer: don't use RTX packets in rate-calc and reset-logic The problem was this: Due to the highly irregular arrival of RTX-packet the max-misorder variable could be pushed very low. (-10). If you then at some point get a big in the sequence-numbers (62 in the test) you end up sending RTX-requests for some of those packets, and then if the sender answers those requests, you are going to get a bunch of RTX-packets arriving. (-13 and then 5 more packets in the test) Now, if max-misorder is pushed very low at this point, these RTX-packets will trigger the handle_big_gap_buffer() logic, and because they arriving so neatly in order, (as they would, since they have been requested like that), the gst_rtp_jitter_buffer_reset() will be called, and two things will happen: 1. priv->next_seqnum will be set to the first RTX packet 2. the 5 RTX-packet will be pushed into the chain() function However, at this point, these RTX-packets are no longer valid, the jitterbuffer has already pushed lost-events for these, so they will now be dropped on the floor, and never make it to the waiting loop-function. And, since we now have a priv->next_seqnum that will never arrive in the loop-function, the jitterbuffer is now stalled forever, and will not push out another buffer. The proposed fixes: 1. Don't use RTX in calculation of the packet-rate. 2. Don't use RTX in large-gap logic, as they are likely to be dropped. --- gst/rtpmanager/gstrtpjitterbuffer.c | 13 ++++++++----- tests/check/elements/rtpjitterbuffer.c | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c index 37fcc77f40..f0571f4a78 100644 --- a/gst/rtpmanager/gstrtpjitterbuffer.c +++ b/gst/rtpmanager/gstrtpjitterbuffer.c @@ -2946,16 +2946,19 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent, expected = priv->next_in_seqnum; - packet_rate = - gst_rtp_packet_rate_ctx_update (&priv->packet_rate_ctx, seqnum, rtptime); + /* don't update packet-rate based on RTX, as those arrive highly unregularly */ + if (!is_rtx) { + packet_rate = gst_rtp_packet_rate_ctx_update (&priv->packet_rate_ctx, + seqnum, rtptime); + GST_TRACE_OBJECT (jitterbuffer, "updated packet_rate: %d", packet_rate); + } max_dropout = gst_rtp_packet_rate_ctx_get_max_dropout (&priv->packet_rate_ctx, priv->max_dropout_time); max_misorder = gst_rtp_packet_rate_ctx_get_max_misorder (&priv->packet_rate_ctx, priv->max_misorder_time); - GST_TRACE_OBJECT (jitterbuffer, - "packet_rate: %d, max_dropout: %d, max_misorder: %d", packet_rate, + GST_TRACE_OBJECT (jitterbuffer, "max_dropout: %d, max_misorder: %d", max_dropout, max_misorder); timer = rtp_timer_queue_find (priv->timers, seqnum); @@ -3021,7 +3024,7 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent, } /* Special handling of large gaps */ - if ((gap != -1 && gap < -max_misorder) || (gap >= max_dropout)) { + if (!is_rtx && ((gap != -1 && gap < -max_misorder) || (gap >= max_dropout))) { gboolean reset = handle_big_gap_buffer (jitterbuffer, buffer, pt, seqnum, gap, max_dropout, max_misorder); if (reset) { diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c index f055b22432..99cc57ca0c 100644 --- a/tests/check/elements/rtpjitterbuffer.c +++ b/tests/check/elements/rtpjitterbuffer.c @@ -3126,6 +3126,29 @@ GST_START_TEST (test_multiple_lost_do_not_stall) GST_END_TEST; +GST_START_TEST (test_reset_using_rtx_packets_does_not_stall) +{ + GstHarness *h = gst_harness_new ("rtpjitterbuffer"); + BufferArrayCtx bufs[] = { + /* *INDENT-OFF* */ + { 1, 1 * TEST_RTP_TS_DURATION, FALSE, 2000000}, + { 62, 62 * TEST_RTP_TS_DURATION, FALSE, 0}, + { -13, -13 * TEST_RTP_TS_DURATION, TRUE, 10000}, + { 1, 1 * TEST_RTP_TS_DURATION, TRUE, 0}, + { 1, 1 * TEST_RTP_TS_DURATION, TRUE, 0}, + { 1, 1 * TEST_RTP_TS_DURATION, TRUE, 0}, + { 1, 1 * TEST_RTP_TS_DURATION, TRUE, 0}, + { 1, 1 * TEST_RTP_TS_DURATION, TRUE, 0}, + /* *INDENT-ON* */ + }; + + g_object_set (h->element, "latency", 400, + "do-retransmission", TRUE, "do-lost", TRUE, "max-misorder-time", 1, NULL); + fail_unless (check_for_stall (h, bufs, G_N_ELEMENTS (bufs))); + gst_harness_teardown (h); +} + +GST_END_TEST; static Suite * rtpjitterbuffer_suite (void) @@ -3196,6 +3219,8 @@ rtpjitterbuffer_suite (void) tcase_add_test (tc_chain, test_reset_timers_does_not_stall); tcase_add_test (tc_chain, test_multiple_lost_do_not_stall); + tcase_add_test (tc_chain, test_reset_using_rtx_packets_does_not_stall); + return s; }