GNOME Bugzilla – Bug 747922
rtpjitterbuffer/rtxreceive: Don't reset the jitterbuffer if too old RTX packets arrive
Last modified: 2015-05-18 15:51:18 UTC
See commit message
Created attachment 301636 [details] [review] rtpjitterbuffer/rtxreceive: Don't reset the jitterbuffer if too old RTX packets arrive This can happen for various reasons, but currently is likely to happen because of https://bugzilla.gnome.org/show_bug.cgi?id=747919 We should just ignore old RTX packets instead of resetting everything and then resetting soon after again because we continue receiving packets from the old position. Also see https://bugzilla.gnome.org/show_bug.cgi?id=739868
Created attachment 301637 [details] [review] rtpbuffer: Add RTX buffer flag to mark RTX packets
Created attachment 301638 [details] [review] rtpjitterbuffer/rtxreceive: Don't reset the jitterbuffer if too old RTX packets arrive This can happen for various reasons, but currently is likely to happen because of another bug, see https://bugzilla.gnome.org/show_bug.cgi?id=747919 We should just ignore old RTX packets instead of resetting everything and then resetting soon after again because we continue receiving packets from the old position. Also see https://bugzilla.gnome.org/show_bug.cgi?id=739868
Created attachment 301639 [details] [review] rtpjitterbuffer/rtxreceive: Don't reset the jitterbuffer if too old RTX packets arrive This can happen for various reasons, but currently is likely to happen because of another bug, see https://bugzilla.gnome.org/show_bug.cgi?id=747919 We should just ignore old RTX packets instead of resetting everything and then resetting soon after again because we continue receiving packets from the old position. Also don't reset all our calculations and treat such packets as if they don't exist in the statistics. Also see https://bugzilla.gnome.org/show_bug.cgi?id=739868
I think this is the wrong approach. Spurious old packets could happen for other reasons. Instead, we should fix the reset detection to, for example, wait until 2-3 sequential packets with a non-consistent sequence to be received before doing a reset.
That makes sense, but I think independent of that RTX packets shouldn't be considered by rtpjitterbuffer as "late" packets and shouldn't affect all the calculations. I'll take a look at what it would take to implement your suggestion
Ok, Wim says RTX packets should be considered like any other "late" packets :) Now for waiting until there are 2-3 sequential packets with a non-consistent sequence are received before resetting... this might also happen because of RTX. We might've requested retransmission of e.g. 5 consecutive packets and got them after receiving 100 other packets. How do we distinguish these cases? And waiting until we have a high number of consecutive packets might also be not a good idea as it delays stuff unnecessarily.
Created attachment 301745 [details] [review] rtpjitterbuffer: Look at the gap between the last popped seqnum and the current seqnum for far too old packets This way we only reset ourselves if a packet is received that is even RTP_MAX_MISORDER older than the last popped packet, instead of just the next expected packet. We might receive RTX for packets further in the past than 100 packets, and if the ringbuffer still did not pop the ones around it we can even still use them. Somewhat related to
Looking at implementing that now but it's not nice code :)
Created attachment 302169 [details] [review] rtpjitterbuffer: When detecting a huge seqnum gap, wait for 5 consecutive packets before resetting everything It might just be a late retransmission or spurious packet from elsewhere, but resetting everything would mean that we will cause a noticeable hickup. Let's get some confidence first that the sequence numbers changed for whatever reason.
Olivier, what do you think about that approach in general? It's currently untested for the case where there is really a sequence number change, anybody have some ideas how to easily test that?
I like the approach. To test a real seqnum change, just stop and restart the sender pipeline.
So we already have similar code in rtpsource and should probably make them at least behave the same. I think the code in both is not ideal though. For one, rtpsource should use gst_rtp_buffer_compare_seqnum(), currently I think it just works by accident for seqnum<expected. I think it works the same but the code makes this very hard to understand :) Then rtpsource does the following (outside probation): diff=new_seqnum - last_seqnum - if diff < 3000 (RTP_MAX_DROPOUT) just forward the packet and do nothing, unless wraparound (in which case it adds another 64k cycle). However I don't think the wraparound detection can ever trigger, as if new_seqnum<last_seqnum then the diff will be something shortly before 65536... which is clearly bigger than 3000. - if diff <= 65536 - 100 (RTP_SEQ_MOD - RTP_MAX_MISORDER) it requires two consecutive seqnums and then considers it a new stream - otherwise it just passes through the packet (diff >= 65536 - 100), considering it packet misordering. I think as a first step I'll rewrite the rtpsource code to use signed integers instead of magic, and fix up the wraparound detection. Then I think the expected behaviour should be (in jitterbuffer and rtpsource): - if -RTP_MAX_MISORDER < diff <= RTP_MAX_DROPOUT, then just consider this normal (plus wraparound detection) - otherwise - in rtpsource, require 2 consecutive seqnums before resetting anything as we do now but additionally queue up packets - in rtpjitterbuffer, require 5 consecutive seqnums and queue packets The reason for this 2 vs 5 consecutive packets is that rtpjitterbuffer might get things mixed together from multiple RTP sources (RTX), which might make things going out of order much more likely (e.g. getting a few RTX packets in a row, and also because of the probation period in rtpsource). While rtpsource is for exactly one source. And of course because it currently uses 2 and that's what seems to work well ;)
commit c60038f188a1128d5cb6e981a690e09bba91f2ad Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon May 18 17:38:31 2015 +0300 rtpsource: Queue bad packets instead of dropping them So we can send them out once we found the next, consecutive sequence number in case one is following. commit 9f18a271f31df65d4febef15af0c5be1c14a4a1a Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon May 18 17:38:14 2015 +0300 rtpsource: Use g_queue_foreach() to unref all buffers in queues commit 54e924332e2900f55298fe9f925acb9fbafff1ec Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon May 18 17:19:31 2015 +0300 rtpsource: Refactor seqnum comparison code a bit commit 1974b24ef4572ef30eb5556456a7dd29f74f4699 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon May 18 17:08:53 2015 +0300 rtpsource: Allow sequence number wraparound during probation commit 3386de7a8a765c4f750454888790415138c0805e Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon May 18 17:07:23 2015 +0300 rtpsource: Make sequence number comparison code more readable ... by using gst_rtp_buffer_compare_seqnum() and signed integers instead of implictly using effects of integer over/underflows. commit ca110fb0b838c49d31d44c2302b948de8ab01e08 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Apr 22 18:54:06 2015 +0200 rtpjitterbuffer: When detecting a huge seqnum gap, wait for 5 consecutive packets before resetting everything It might just be a late retransmission or spurious packet from elsewhere, but resetting everything would mean that we will cause a noticeable hickup. Let's get some confidence first that the sequence numbers changed for whatever reason. https://bugzilla.gnome.org/show_bug.cgi?id=747922
Comment on attachment 302169 [details] [review] rtpjitterbuffer: When detecting a huge seqnum gap, wait for 5 consecutive packets before resetting everything Committed an updated version of this, not the other one though as it's mostly useless and just complicates the code.