GNOME Bugzilla – Bug 722560
rtprtxreceive: cleanup orphan retransmission requests
Last modified: 2017-03-01 09:05:09 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
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).
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.
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.
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!
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.
Created attachment 346898 [details] [review] rtprtxreceive: fix potential leak of old, unassociated, association requests
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