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 722560 - rtprtxreceive: cleanup orphan retransmission requests
rtprtxreceive: cleanup orphan retransmission requests
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-19 19:28 UTC by George Kiagiadakis
Modified: 2017-03-01 09:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtprtxreceive: fix potential leak of old, unassociated, association requests (1.41 KB, patch)
2017-02-28 11:16 UTC, George Kiagiadakis
committed Details | Review

Description George Kiagiadakis 2014-01-19 19:28:30 UTC
When rtpjitterbuffer requests for the first time the retransmission of a certain packet, rtprtxreceive stores the seqnum of this packet in a hash table in order to be able to associate later the retransmission stream. If the rtx packet never arrives, this hash table entry remains there until the pipeline is stopped.

This patch attempts to fix this small "leak" by looking at the timing information of the retransmission request and removing the hash table entry as soon as it knows that even if the rtx packet arrives, it will not be useful to the jitterbuffer.

The patch, together with a unit test and a small logic fix is here: http://cgit.collabora.com/git/user/gkiagia/gst-plugins-good.git/log/?h=rtxrecv-cleanup-lost-req
Comment 1 Olivier Crête 2014-01-20 16:22:50 UTC
In rtprtxreceive: cleanup stored seqnum->ssrc association when an rtx request never gets answered:

I see no locking protecting the hash table in the cleanup_seqnum_ssrc1_map() callback. Also you need to either hold a ref to the element or keep a list of pending IDs so that callback to prevent the case where the element is disposed before the callback is called (or what happen if the element is stopped and restarted before the timeout expires).
Comment 2 George Kiagiadakis 2014-01-21 15:48:52 UTC
I must have been sleeping while I was writing that function... I had the lock in my mind, but didn't type it. Anyway, it's true that I hadn't really considered the case of the element being restarted before the callback gets fired, so since I now have to maintain a list of pending removals anyway, I decided to do it in a different way, which I was thinking from the start. There is no async callback from the clock anymore. The pending removals list is checked in chain() and if it is not empty, it is iterated and the time of the removals is checked against the current clock time. If the time is later than the deadline, the association is removed. The result is the same as in the previous solution, and it's simpler.

I have edited the commits in the same branch.
Comment 3 George Kiagiadakis 2014-01-21 16:07:41 UTC
Also rebased now on top of master and fixed also here the race condition in the unit test caused by rtxsend now pushing rtx buffers from a different thread.
Comment 4 Olivier Crête 2014-08-21 21:18:38 UTC
I think it needs to be rebased again :-(
Please prod Wim or me harder when you've done it so we can merge it this time!
Comment 5 George Kiagiadakis 2017-02-28 11:16:12 UTC
Looking at this again after a long time, I'm having a hard time to understand what the values of delay, frequency and period actually mean. The unit test attached here looks like it gets them wrong, compared to a real test with rtpjitterbuffer.

In any case, rtprtxreceive seems to have some sort of request timeout functionality now, only that it is only triggered when a duplicate request (for the same seqnum, with different ssrc) arrives, so it does not really fix the "leak". But this leak, if it ever happens, it's going to be so small that it does not really matter I think...

I'm attaching a small patch below that would fix the leak, anyways. The idea is to run the check in _chain() instead of only when receiving a duplicate request.
Comment 6 George Kiagiadakis 2017-02-28 11:16:57 UTC
Created attachment 346898 [details] [review]
rtprtxreceive: fix potential leak of old, unassociated, association requests
Comment 7 George Kiagiadakis 2017-03-01 09:04:04 UTC
commit 71b63d54fe4556ddaf6626cb2275825ba4eaed32
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Tue Feb 28 13:10:50 2017 +0200

    rtprtxreceive: fix potential leak of old, unassociated, association requests
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722560