After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748041 - rtpjitterbuffer: Too early requested retransmission for future packets
rtpjitterbuffer: Too early requested retransmission for future packets
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-17 11:41 UTC by Sebastian Dröge (slomo)
Modified: 2015-04-27 14:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WIP rtpjitterbuffer: Don't just take the last packet spacing into account but take a running average (3.04 KB, patch)
2015-04-22 17:42 UTC, Sebastian Dröge (slomo)
rejected Details | Review

Description Sebastian Dröge (slomo) 2015-04-17 11:41:51 UTC
Related to one of the patches in bug #739868


Currently rtpjitterbuffer requests retransmissions for future packets (i.e. seqnums following the last received one), based on estimating when they *should* arrive. This clearly does not work well, as the patch in #739868 to disable it shows, and also when I run a RTX pipeline here over loopback it is requesting lots of retransmissions for future packets that just arrive shortly after (and then a bit later the retransmission).

This wastes bandwidth for nothing, but together with our suboptimal RTCP handling right now (see e.g. bug #746543) this also wastes early RTCP packets that we might've actually needed for other things later.
Comment 1 Sebastian Dröge (slomo) 2015-04-22 17:07:22 UTC
Funnily rtpjitterbuffer also considers the normal (a bit late transmission after doing RTX) receival of that packet as an RTX success. It's not :P
Comment 2 Sebastian Dröge (slomo) 2015-04-22 17:42:48 UTC
Created attachment 302170 [details] [review]
WIP rtpjitterbuffer: Don't just take the last packet spacing into account but take a running average

Currently implements 3 different ways, the currently enabled one works best
here but still requests a retransmission every now and then here shortly
before the packet arrives.
Comment 3 Sebastian Dröge (slomo) 2015-04-22 18:01:45 UTC
Review of attachment 302170 [details] [review]:

::: gst/rtpmanager/gstrtpjitterbuffer.c
@@ +2026,3 @@
+      /* Allow 10% delay */
+
+      new_packet_spacing *= 1.1;

This should probably be a property if this is considered the right thing to do, and it's related to rtx-delay
Comment 4 Sebastian Dröge (slomo) 2015-04-22 18:37:19 UTC
Discussed this with Wim, and this is what we came up with. Works fine in my tests, let's see what happens.


commit edcc5be297f03c0152ebabbba1fe0bfad8a5131a
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Apr 22 20:24:20 2015 +0200

    rtpjitterbuffer: When request retransmissions for future packets, consider the packet spacing in the extra delay
    
    We now take the maximum of 2*jitter and 0.5*packet_spacing for the extra
    delay. If jitter is very low, this should prevent unnecessary retransmission
    requests to some degree.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748041

commit 3fe8ceff14bb7847d867c743b4a5df8ba221a80d
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Apr 22 19:41:07 2015 +0200

    rtpjitterbuffer: Take a running average of the packet spacings instead of just the latest
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748041
Comment 5 Sebastian Dröge (slomo) 2015-04-27 14:48:28 UTC
commit 91c8688ed7c694c84173a26982643b167a4185ca
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Apr 27 16:36:27 2015 +0200

    rtpjitterbuffer: Fix RTX unit test
    
    The calculations were a bit off everywhere, even before the changes done
    recently to the delay for RTX of expected future packets. It only worked by
    accident, but now the calculations are all correct again. Hopefully.