GNOME Bugzilla – Bug 739985
rtpsession: feedback rtcp queue grows indefinitely until session end
Last modified: 2018-11-03 14:56:05 UTC
In short, when an rtcp feedback packet is received, it is queued based on the value of rtcp_feedback_retention_window, which defaults to 2 seconds. The queue is pruned periodically in rtp_source_timeout() according to: while ((pkt = g_queue_peek_tail (src->retained_feedback)) && GST_BUFFER_TIMESTAMP (pkt) < feedback_retention_window) gst_buffer_unref (g_queue_pop_tail (src->retained_feedback)); The problem is that the buffer timestamp, set when queued, comes from the packet info's running time field: rtp_session_process_feedback(): <snip> rtp_source_retain_rtcp_packet (src, packet, pinfo->running_time) And is initialized in rtp_session_process_rtcp() with -1: update_packet_info (sess, &pinfo, FALSE, FALSE, FALSE, buffer, current_time, -1, ntpnstime); In a setup with a high quantity of NACKs, this results in a queue of significant size. It seems like pinfo->current_time might be a better field to use for the buffer timestamp, but that's speculation on my part.
Indeed there is a bug.. I'm not sure why in gst_rtp_session_chain_recv_rtcp(), we discard the receive time of the packet (ie the buffer timestamp). So we could maybe pass the running time there or I guess the other option is to use the current time. Re-reading RFC 4585, it's not clear that either interpretation is better.
It seems like the current_time might be better, only in the event there was some latency between the receive time of the packet and when the rtcp processing code received it for queuing (though that is doubtful)?
The current time is problematic, because the timestamp used to manage the window is relative to the running time. It seems better to follow the processing paradigm used for RTP and just pass the running time down, similar to how RTP packets are processed.
Created attachment 290526 [details] [review] Proposed patch for Bug 739985 Proposed patch
Review of attachment 290526 [details] [review]: I'd really like to get Wim's input on this. ::: gst/rtpmanager/gstrtpsession.c @@ +1783,2 @@ current_time = gst_clock_get_time (priv->sysclock); + get_current_times (rtpsession, &running_time, &ntpnstime); The running time should be read from the GST_BUFFER_PTS() if possible, see how it's done for RTP. Otherwise it's the same thing as the current_time mostly.
Fair enough, more input is always better. I had it coded that way originally (matching RTP), but I wasn't sure it was valid to convert the RTCP buffer's timestamp to a running time using the incoming RTP segment (rtpsession->recv_rtp_seg).
No, indeed we'd need to capture the segment from the RTCP pad too.
Created attachment 323739 [details] [review] rtpsession: Fix retaining rtcp packets This is still a problem in 1.6.3 (and master). Whenever I simulate high packet loss, e.g. using an identity element, memory grows considerably over just a few hours. Incidentally, I came up with almost exactly the same patch (not realizing this bug already existed)... This patch also captures the segment in order to calculate the running time.
Review of attachment 323739 [details] [review]: Makes sense to me ::: gst/rtpmanager/gstrtpsession.c @@ +2006,3 @@ + gst_segment_to_running_time (&rtpsession->recv_rtp_seg, GST_FORMAT_TIME, + timestamp); + ntpnstime = GST_CLOCK_TIME_NONE; Why do you set it to NONE here?
Comment on attachment 290526 [details] [review] Proposed patch for Bug 739985 The new patch seems to handle this better
The main problem I see with using the buffer PTS here instead of looking at the clock is that with RTSP/TCP we will only have a timestamp on the very first buffer and then never again. Maybe as a fallback we should look at the current time?
Review of attachment 323739 [details] [review]: ::: gst/rtpmanager/gstrtpsession.c @@ +2000,3 @@ + /* get NTP time when this packet was captured, this depends on the timestamp. */ + timestamp = GST_BUFFER_PTS (buffer); GST_BUFFER_PTS_OR_DTS(buffer) instead probably
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 323739 [details] [review] [review]: > > Makes sense to me > > ::: gst/rtpmanager/gstrtpsession.c > @@ +2006,3 @@ > + gst_segment_to_running_time (&rtpsession->recv_rtp_seg, > GST_FORMAT_TIME, > + timestamp); > + ntpnstime = GST_CLOCK_TIME_NONE; > > Why do you set it to NONE here? Not sure, this is copy/paste from gst_rtp_session_chain_recv_rtp().
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 323739 [details] [review] [review]: > > Makes sense to me > > ::: gst/rtpmanager/gstrtpsession.c > @@ +2006,3 @@ > + gst_segment_to_running_time (&rtpsession->recv_rtp_seg, > GST_FORMAT_TIME, > + timestamp); > + ntpnstime = GST_CLOCK_TIME_NONE; > > Why do you set it to NONE here? Hm, I just noticed when running in gdb that the call to gst_segment_to_running_time() actually triggers an assertion: GStreamer-CRITICAL **: gst_segment_to_running_time: assertion 'segment->format == format' failed I'm not sure, why though. Any ideas?
Note that the segment is for the RTP stream, not the RTCP one. You should catch the RTCP segment independently and use that here. I assume that's the problem here? Otherwise, both segments should be in TIME format. What are your sources?
Created attachment 324285 [details] [review] rtpsession: Fix retaining rtcp packets. This patch now also captures the segment on the rtcp pad and uses it to calculate the running time similar to the rtp pad.
Review of attachment 324285 [details] [review]: ::: gst/rtpmanager/gstrtpsession.c @@ +2021,3 @@ + /* get NTP time when this packet was captured, this depends on the timestamp. */ + timestamp = GST_BUFFER_PTS (buffer); This should be DTS? Or at least DTS_OR_PTS. Is it using PTS for RTP?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/142.