GNOME Bugzilla – Bug 746543
rtpsession: Properly implement T_rr_interval and allow sending multiple early feedback packets in a row
Last modified: 2015-06-02 09:42:31 UTC
T_rr_interval and T_rr and Tmin are currently the same value inside rtpsession, which is also not correct according to the RFC. In feedback profiles, Tmin should be 1s in the beginning, and 0 after the first RTCP packet. T_rr should be the currently calculated interval when we would send the next regular RTCP packet, but it can be suppressed if the next regular RTCP packet would be earlier than T_rr_interval since the last one *and* no feedback packets are pending. And if a regular RTCP packet is suppressed because of that, we are allowed to send an early RTCP packet again now. This way it would be possible to have a regular RTCP packet every T_rr_interval, and in between multiple early RTCP packets (which are smaller, even more when implementing reduced size RTCP). And because they are smaller and we thus use less bandwidth for RTCP currently, the scheduling interval for regular RTCP packets can become smaller, which then allows us to send early feedback even more often. This is related to bug #738319.
The current code has the implicit assumption that RTCP packets are mostly all equal in size. If you implement reduced size rtcp to report more frequently, you need to also remember to make the IP & UDP headers into account.
I think this can be implemented quite easily actually, without introducing new properties or anything. - rtcp-min-interval is ignored if feedback is pending for the actual timeouts and we only take the bandwidth calculations into account (also see bug #747863), unless the last non-early RTCP packet is more than rtcp-min-interval in the past (in which case we would schedule earlier). Exception to this is for the very first RTCP packet, which would be at 1s if feedback is pending and otherwise at the time the current code calculates (rtcp-min-interval/2). - on every timeout, we send RTCP if the last non-early RTCP packet is more than rtcp-min-interval in the past, or feedback is pending (in which case we consider this RTCP packet as early feedback). Otherwise we suppress (and allow early feedback again). This would cause us to only send RTCP every rtcp-min-interval if there is no feedback pending, and otherwise send RTCP as often as bandwidth calculations allow (if there is actual feedback pending). As we always reconsider the next timeout when feedback is to be sent, this would also not cause scheduled timeouts to be too far in the future if no feedback was pending last time we scheduled the timeout. Also this would be a first step to reduced-size RTCP, as we would now already have to store the time since the last non-early RTCP packet. Which with reduced-size RTCP would be exactly the time since the last compound (non-reduced size) RTCP packet. And whenever we send a non-early RTCP packet, we would have to make sure that we really send a compound RTCP packet. Additionally we would have to take IP/UDP headers into account and not consider all RTCP packets of approximately equal size as Olivier said in comment 1. Any comments here, potential problems?
Quotes from comment 2 "..., unless the last non-early RTCP packet is more than rtcp-min-interval in the past (in which case we would schedule earlier)." Is this referring to the case when there are no feedback messages, but more than one T_rr_int has past since T_rr_last? Also, is that rtcp-min-interval the randomized value [0.5,1.5] times T_rr_int, i.e. the equivalent to T_rr_interval in the RTCP spec. That randomization is there to prevent clock synchronization in multi-party cases. "Exception to this is for the very first RTCP packet, which would be at 1s if feedback is pending and otherwise at the time the current code calculates (rtcp-min-interval/2)." How are this dealing with the unicast rule? The RFC allows the initial delay to be zero in case the session is unicast. Note that there are some clarifications in an IETF draft that is fairly mature. I expect WG last call occur in the next three months. Please see: Section 5.2 of https://datatracker.ietf.org/doc/draft-ietf-avtcore-rtp-multi-stream/ "- on every timeout, we send RTCP if the last non-early RTCP packet is more than rtcp-min-interval in the past, or feedback is pending (in which case we consider this RTCP packet as early feedback). Otherwise we suppress (and allow early feedback again)." This appear correct, assuming that one has calculated the transmission timeout when sending the early packet according to bullet 6 in Section 3.5.2 of RFC 4585: "R then MUST set allow_early = FALSE, MUST recalculate tn = tp + 2*T_rr, and MUST set tp to the previous tn." "Also this would be a first step to reduced-size RTCP, as we would now already have to store the time since the last non-early RTCP packet. Which with reduced-size RTCP would be exactly the time since the last compound (non-reduced size) RTCP packet. And whenever we send a non-early RTCP packet, we would have to make sure that we really send a compound RTCP packet. Additionally we would have to take IP/UDP headers into account and not consider all RTCP packets of approximately equal size as Olivier said in comment 1." Yes, you do need to introduce the T_rr_last parameter and use that correctly. I do note that already today early sent minimal Feedback Packets, even without reduced size RTCP is supposed to not update T_rr_last. But, I guess current implementation send full compounds with all SDES items etc. I would note that if you already today are not sampling the actual incoming and outgoing RTCP packet sizes to form the value of avg_rtcp_size the implementation will not maintain bandwidth targets when there are non-symmetric number of SSRCs between the local and remote endpoint(s).
One more thing - I think T_rr_int is part of signalling as well, so it should be possible to configure it. It wasn't clear if it would be configurable from what was written.
Created attachment 302859 [details] [review] rtpsession: Add property for selecting feedback RTP profile (RFC4585) And modify our RTCP scheduling algorithm accordingly. We now can send more RTCP packets if needed for feedback, but will throttle full RTCP packets by rtcp-min-interval (t-rr-int from RFC4585). In non-feedback mode, rtcp-min-interval is Tmin from RFC3550, which is statically set to 1s or 0s by RFC4585. Tmin defines how often we should send RTCP packets at most.
Created attachment 302860 [details] [review] rtpsession: Only put RRs and full SDES into regular RTCP packets If we may suppress the packet due to the rules of RFC4585 (i.e. when below the t-rr-int), we can send a smaller RTCP packet without RRs and full SDES. In theory we could even send a minimal RTCP packet according to RFC5506, but we don't support that yet.
Created attachment 302861 [details] [review] rtpsession: Add property for selecting feedback RTP profile (RFC4585) And modify our RTCP scheduling algorithm accordingly. We now can send more RTCP packets if needed for feedback, but will throttle full RTCP packets by rtcp-min-interval (t-rr-int from RFC4585). In non-feedback mode, rtcp-min-interval is Tmin from RFC3550, which is statically set to 1s or 0s by RFC4585. Tmin defines how often we should send RTCP packets at most.
Created attachment 302862 [details] [review] rtpsession: Keep track of tp/tn and t_rr_last separately Otherwise we can't properly schedule RTCP in feedback profiles as we need to distinguish the time when we last checked for sending RTCP (tp) but might have suppressed it, and the time when we last actually sent a non-early RTCP packet. This together with the other changes should now properly implement RTCP scheduling according to RFC4585, and especially allow us to send feedback packets a lot if needed but only send regular RTCP packets every once in a while.
Created attachment 302863 [details] [review] rtpsession: Only put RRs and full SDES into regular RTCP packets If we may suppress the packet due to the rules of RFC4585 (i.e. when below the t-rr-int), we can send a smaller RTCP packet without RRs and full SDES. In theory we could even send a minimal RTCP packet according to RFC5506, but we don't support that yet.
Question now with the new property is if that should be a generic "profile" property with an enum type like GstRTSPProfile (AVP, AVPF, ...) or if it's ok like that? And setting this on rtpbin is a bit tricky now, unless we give the sessions inside rtpbin distinct names :)
Created attachment 302869 [details] [review] rtspsrc: Set feedback to TRUE/FALSE depending on the used profile (AVP vs AVPF)
Another problem is that this uses quite some CPU and has lots of wakeups on the RTCP threads now (depending on the RTP bitrate). It might make sense to only set the timer to rtcp-min-interval (t-rr-int) unless something is requesting early feedback, but getting the calculations for all the things right doesn't seem trivial (like when to reset allow_early to TRUE). Will look at that tomorrow. RTCP thread used ~1% CPU here for an HD h264 stream with quite high bitrate. It's probably much worse on embedded devices, or even worse on devices that run on battery ;)
Thanks for your reply Magnus :) That was really helpful to understand that I'm on the right track here. (In reply to Magnus Westerlund from comment #3) > I would note that if you already today are not sampling the actual incoming > and outgoing RTCP packet sizes to form the value of avg_rtcp_size the > implementation will not maintain bandwidth targets when there are > non-symmetric number of SSRCs between the local and remote endpoint(s). We actually calculate the average size of RTCP packets and use that for the bandwidth/interval calculations. Not sure what Olivier was referring to actually :) We only don't consider the UDP/IP packet overhead in the calculations.
(In reply to Sebastian Dröge (slomo) from comment #13) > We only don't consider the UDP/IP packet overhead in the calculations. I'm not sure if there is a way to do that automatically, maybe we should add a "packet-overhead" property to rtpsession and default to the size of ipv4+udp header. Maybe libnice should export the value, as it knows if TURN is used, and if ipv4 or ipv6 are used, and nicesrc could push it downstream as a custom sticky event. Or maybe it doesn't matter and a fixed number is enough.
(In reply to Olivier Crête from comment #14) > (In reply to Sebastian Dröge (slomo) from comment #13) > > We only don't consider the UDP/IP packet overhead in the calculations. > > I'm not sure if there is a way to do that automatically, maybe we should add > a "packet-overhead" property to rtpsession and default to the size of > ipv4+udp header. Maybe libnice should export the value, as it knows if TURN > is used, and if ipv4 or ipv6 are used, and nicesrc could push it downstream > as a custom sticky event. Or maybe it doesn't matter and a fixed number is > enough. My guess that it doesn't matter that much, because the RTP packets will also have this overhead. It's a bit less (in percentage) than for RTCP (RTP packets usually are a bit bigger) though. We'll see if it matters :)
Created attachment 303661 [details] [review] rtpsession: Add property for selecting RTP profile (AVP/AVPF/etc) And modify our RTCP scheduling algorithm accordingly. We now can send more RTCP packets if needed for feedback, but will throttle full RTCP packets by rtcp-min-interval (t-rr-int from RFC4585). In non-feedback mode, rtcp-min-interval is Tmin from RFC3550, which is statically set to 1s or 0s by RFC4585. Tmin defines how often we should send RTCP packets at most.
Created attachment 303662 [details] [review] rtpsession: Keep track of tp/tn and t_rr_last separately Otherwise we can't properly schedule RTCP in feedback profiles as we need to distinguish the time when we last checked for sending RTCP (tp) but might have suppressed it, and the time when we last actually sent a non-early RTCP packet. This together with the other changes should now properly implement RTCP scheduling according to RFC4585, and especially allow us to send feedback packets a lot if needed but only send regular RTCP packets every once in a while.
Created attachment 303663 [details] [review] rtpsession: Only put RRs and full SDES into regular RTCP packets If we may suppress the packet due to the rules of RFC4585 (i.e. when below the t-rr-int), we can send a smaller RTCP packet without RRs and full SDES. In theory we could even send a minimal RTCP packet according to RFC5506, but we don't support that yet.
Created attachment 303664 [details] [review] rtspsrc: Set RTP profile on the rtpsession objects
Created attachment 303740 [details] [review] rtpsession: Add property for selecting RTP profile (AVP/AVPF/etc) And modify our RTCP scheduling algorithm accordingly. We now can send more RTCP packets if needed for feedback, but will throttle full RTCP packets by rtcp-min-interval (t-rr-int from RFC4585). In non-feedback mode, rtcp-min-interval is Tmin from RFC3550, which is statically set to 1s or 0s by RFC4585. Tmin defines how often we should send RTCP packets at most.
Created attachment 303741 [details] [review] rtpsession: Keep track of tp/tn and t_rr_last separately Otherwise we can't properly schedule RTCP in feedback profiles as we need to distinguish the time when we last checked for sending RTCP (tp) but might have suppressed it, and the time when we last actually sent a non-early RTCP packet. This together with the other changes should now properly implement RTCP scheduling according to RFC4585, and especially allow us to send feedback packets a lot if needed but only send regular RTCP packets every once in a while.
Created attachment 303742 [details] [review] rtpsession: Only put RRs and full SDES into regular RTCP packets If we may suppress the packet due to the rules of RFC4585 (i.e. when below the t-rr-int), we can send a smaller RTCP packet without RRs and full SDES. In theory we could even send a minimal RTCP packet according to RFC5506, but we don't support that yet.
Created attachment 303743 [details] [review] rtpbin: Add rtp-profile property for setting the default profile of newly created sessions
Created attachment 303744 [details] [review] rtspsrc: Set RTP profile on the rtpsession objects
Created attachment 303745 [details] [review] examples: Set RTP profile to AVPF for rtpaux examples
Created attachment 303746 [details] [review] rtpsession: Only go through the RTCP generation when we have to send regular RTCP or feedback is pending Otherwise we will copy hash tables, create RTCP packets, etc. just for throwing things away again many times per second. Reduces CPU usage of the RTCP thread a bit, but it would be better to not wake up the RTCP thread at all. CPU usage is still around 1.5% instead of 0.3% when comparing to non-feedback profiles.
To improve upon the last patch, I'm going to rewrite the timeout scheduling a bit now. We would only schedule a timeout by default for every MAX(rtcp-min-interval, calculated-rtcp-min-interval)... unless feedback is requested to be sent. Then we would schedule earlier, based on the calculated-rtcp-min-interval and the last time we sent something. Based on that we would also decide if we can do early feedback or not. That way we would only wake up the RTCP threads when something is actually to be sent (or when a forward reconsideration is happening). The bad thing about this is that it's not exactly the same as in the spec anymore then. The behaviour will be the same, but the algorithm is different.
(In reply to Sebastian Dröge (slomo) from comment #27) > To improve upon the last patch, I'm going to rewrite the timeout scheduling > a bit now. We would only schedule a timeout by default for every > MAX(rtcp-min-interval, calculated-rtcp-min-interval)... unless feedback is > requested to be sent. > > Then we would schedule earlier, based on the calculated-rtcp-min-interval > and the last time we sent something. Based on that we would also decide if > we can do early feedback or not. > > That way we would only wake up the RTCP threads when something is actually > to be sent (or when a forward reconsideration is happening). > > > The bad thing about this is that it's not exactly the same as in the spec > anymore then. The behaviour will be the same, but the algorithm is different. I have to ask about what you refer to as a "forward reconsideration" is that for the cases when the reverse reconsideration algorithm tests if it is going to transmit and come up with a transmission interval larger than what resulted in this wakeup? If that is the case, I am fine. Secondly, I hope the "rtcp-min-interval" above will be the currently randomized value, i.e. T_rr_current_interval from RFC 4585. If not you disable the timer self synchronization protection. Another effect this could have, is that as it is not suppressing a number of steps with the normally calculated and randomized value is that it will change the transmission distribution for when being suppressed. The resulting behavior according to the spec, can result in that the actual RTCP interval becomes up to 2.73*T_rr_int. I do consider it a bug, but we haven't gotten around to do anything towards updating the AVPF specification in this regard. So, from that perspective, using the randomized T_rr_interval is fine. What I am uncertain is what behavior you will see when rtcp-min-interval and calculated-rtcp-min-interval are almost equal in size, at least their determenistic part. As you will choose the longer, at least you will not use more bandwidth than is assigned. Thus, I expect this to be fine from that aspect. Further, I hope you ensure that if FB packets are needed to be sent that you perform the evaluation if allowed_early should be reset at that time.
Comment on attachment 303746 [details] [review] rtpsession: Only go through the RTCP generation when we have to send regular RTCP or feedback is pending Not really making that much difference and I have a better patch almost finished.
Created attachment 304219 [details] [review] rtpsession: Add property for selecting RTP profile (AVP/AVPF/etc) And modify our RTCP scheduling algorithm accordingly. We now can send more RTCP packets if needed for feedback, but will throttle full RTCP packets by rtcp-min-interval (t-rr-int from RFC4585). In non-feedback mode, rtcp-min-interval is Tmin from RFC3550, which is statically set to 1s or 0s by RFC4585. Tmin defines how often we should send RTCP packets at most.
Created attachment 304220 [details] [review] rtpsession: Keep track of tp/tn and t_rr_last separately Otherwise we can't properly schedule RTCP in feedback profiles as we need to distinguish the time when we last checked for sending RTCP (tp) but might have suppressed it, and the time when we last actually sent a non-early RTCP packet. This together with the other changes should now properly implement RTCP scheduling according to RFC4585, and especially allow us to send feedback packets a lot if needed but only send regular RTCP packets every once in a while.
Created attachment 304221 [details] [review] rtpsession: Only put RRs and full SDES into regular RTCP packets If we may suppress the packet due to the rules of RFC4585 (i.e. when below the t-rr-int), we can send a smaller RTCP packet without RRs and full SDES. In theory we could even send a minimal RTCP packet according to RFC5506, but we don't support that yet.
Created attachment 304222 [details] [review] rtpbin: Add rtp-profile property for setting the default profile of newly created sessions
Created attachment 304223 [details] [review] rtspsrc: Set RTP profile on the rtpsession objects
Created attachment 304224 [details] [review] examples: Set RTP profile to AVPF for rtpaux examples
Created attachment 304225 [details] [review] rtpsession: Remove useless goto
Created attachment 304226 [details] [review] rtpsession: Only schedule a timer when we actually have to send RTCP Otherwise we will have 10s-100s of thread wakeups in feedback profiles, create RTCP packets, etc. just to suppress them in 99% of the cases (i.e. if no feedback is actually pending and no regular RTCP has to be sent). This improves CPU usage and battery life quite a lot.
All pushed