[Asterisk-code-review] res_rtp_asterisk: Improve video performance in certain networks. (asterisk[certified/16.8])
Kevin Harwell
asteriskteam at digium.com
Tue Mar 3 14:40:22 CST 2020
Kevin Harwell has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13869 )
Change subject: res_rtp_asterisk: Improve video performance in certain networks.
......................................................................
res_rtp_asterisk: Improve video performance in certain networks.
The receive buffer will now grow if we end up flushing the
receive queue after not receiving the expected packet in time.
This is done in hopes that if this is encountered again the
extra buffer size will allow more time to pass and any missing
packets to be received.
The send buffer will now grow if we are asked for packets and
can't find them. This is done in hopes that the packets are
from the past and have simply been expired. If so then in
the future with the extra buffer space the packets should be
available.
Sequence number cycling has been handled so that the
correct sequence number is calculated and used in
various places, including for sorting packets and
for determining if a packet is old or not.
NACK sending is now more aggressive. If a substantial number
of missing sequence numbers are added a NACK will be sent
immediately. Afterwards once the receive buffer reaches 25%
a single NACK is sent. If the buffer continues to grow and
reaches 50% or greater a NACK will be sent for each received
future packet to aggressively ask the remote endpoint to
retransmit.
ASTERISK-28764
Change-Id: I97633dfa8a09a7889cef815b2be369f3f0314b41
---
M res/res_rtp_asterisk.c
1 file changed, 178 insertions(+), 70 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Benjamin Keith Ford: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, approved; Approved for Submit
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index c52ce2c..4f736ef 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -103,8 +103,13 @@
#define TURN_STATE_WAIT_TIME 2000
-#define DEFAULT_RTP_SEND_BUFFER_SIZE 250
-#define DEFAULT_RTP_RECV_BUFFER_SIZE 20
+#define DEFAULT_RTP_SEND_BUFFER_SIZE 250 /*!< The initial size of the RTP send buffer */
+#define MAXIMUM_RTP_SEND_BUFFER_SIZE (DEFAULT_RTP_SEND_BUFFER_SIZE + 200) /*!< Maximum RTP send buffer size */
+#define DEFAULT_RTP_RECV_BUFFER_SIZE 20 /*!< The initial size of the RTP receiver buffer */
+#define MAXIMUM_RTP_RECV_BUFFER_SIZE (DEFAULT_RTP_RECV_BUFFER_SIZE + 20) /*!< Maximum RTP receive buffer size */
+#define OLD_PACKET_COUNT 1000 /*!< The number of previous packets that are considered old */
+#define MISSING_SEQNOS_ADDED_TRIGGER 2 /*!< The number of immediate missing packets that will trigger an immediate NACK */
+#define SEQNO_CYCLE_OVER 65536 /*!< The number after the maximum allowed sequence number */
/*! Full INTRA-frame Request / Fast Update Request (From RFC2032) */
#define RTCP_PT_FUR 192
@@ -4559,10 +4564,10 @@
{
struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
int packet_len;
- int blp_index;
+ int blp_index = -1;
int current_seqno;
- int seqno;
- unsigned int fci;
+ unsigned int fci = 0;
+ size_t remaining_missing_seqno;
if (!rtp || !rtp->rtcp) {
return 0;
@@ -4573,32 +4578,50 @@
}
current_seqno = rtp->expectedrxseqno;
- seqno = rtp->lastrxseqno;
+ remaining_missing_seqno = AST_VECTOR_SIZE(&rtp->missing_seqno);
packet_len = 12; /* The header length is 12 (version line, packet source SSRC, media source SSRC) */
- /* Get the missing sequence numbers for the FCI section of the NACK request */
- for (blp_index = 0, fci = 0; current_seqno < seqno; current_seqno++, blp_index++) {
+ /* If there are no missing sequence numbers then don't bother sending a NACK needlessly */
+ if (!remaining_missing_seqno) {
+ return 0;
+ }
+
+ /* This iterates through the possible forward sequence numbers seeing which ones we
+ * have no packet for, adding it to the NACK until we are out of missing packets.
+ */
+ while (remaining_missing_seqno) {
int *missing_seqno;
+ /* On the first entry to this loop blp_index will be -1, so this will become 0
+ * and the sequence number will be placed into the packet as the PID.
+ */
+ blp_index++;
+
missing_seqno = AST_VECTOR_GET_CMP(&rtp->missing_seqno, current_seqno,
find_by_value);
+ if (missing_seqno) {
+ /* We hit the max blp size, reset */
+ if (blp_index >= 17) {
+ put_unaligned_uint32(rtcpheader + packet_len, htonl(fci));
+ fci = 0;
+ blp_index = 0;
+ packet_len += 4;
+ }
- if (!missing_seqno) {
- continue;
+ if (blp_index == 0) {
+ fci |= (current_seqno << 16);
+ } else {
+ fci |= (1 << (blp_index - 1));
+ }
+
+ /* Since we've used a missing sequence number, we're down one */
+ remaining_missing_seqno--;
}
- /* We hit the max blp size, reset */
- if (blp_index >= 17) {
- put_unaligned_uint32(rtcpheader + packet_len, htonl(fci));
- fci = 0;
- blp_index = 0;
- packet_len += 4;
- }
-
- if (blp_index == 0) {
- fci |= (current_seqno << 16);
- } else {
- fci |= (1 << (blp_index - 1));
+ /* Handle cycling of the sequence number */
+ current_seqno++;
+ if (current_seqno == SEQNO_CYCLE_OVER) {
+ current_seqno = 0;
}
}
@@ -5765,6 +5788,7 @@
int abs_send_time_id;
unsigned int now_msw = 0;
unsigned int now_lsw = 0;
+ unsigned int packets_not_found = 0;
if (!rtp->send_buffer) {
ast_debug(1, "Tried to handle NACK request, but we don't have a RTP packet storage!\n");
@@ -5797,6 +5821,7 @@
res += rtp_sendto(instance, payload->buf, payload->size, 0, &remote_address, &ice);
} else {
ast_debug(1, "Received NACK request for RTP packet with seqno %d, but we don't have it\n", pid);
+ packets_not_found++;
}
/*
* The bitmask. Denoting the least significant bit as 1 and its most significant bit
@@ -5818,6 +5843,7 @@
res += rtp_sendto(instance, payload->buf, payload->size, 0, &remote_address, &ice);
} else {
ast_debug(1, "Remote end also requested RTP packet with seqno %d, but we don't have it\n", seqno);
+ packets_not_found++;
}
}
blp >>= 1;
@@ -5825,6 +5851,15 @@
}
}
+ if (packets_not_found) {
+ /* Grow the send buffer based on how many packets were not found in the buffer, but
+ * enforce a maximum.
+ */
+ ast_data_buffer_resize(rtp->send_buffer, MIN(MAXIMUM_RTP_SEND_BUFFER_SIZE,
+ ast_data_buffer_max(rtp->send_buffer) + packets_not_found));
+ ast_debug(2, "Send buffer on RTP instance '%p' is now at maximum of %zu\n", instance, ast_data_buffer_max(rtp->send_buffer));
+ }
+
return res;
}
@@ -7597,6 +7632,11 @@
} else if (rtp->expectedrxseqno == -1 || seqno == rtp->expectedrxseqno) {
rtp->expectedrxseqno = seqno + 1;
+ /* We've cycled over, so go back to 0 */
+ if (rtp->expectedrxseqno == SEQNO_CYCLE_OVER) {
+ rtp->expectedrxseqno = 0;
+ }
+
/* If there are no buffered packets that will be placed after this frame then we can
* return it directly without duplicating it.
*/
@@ -7615,7 +7655,7 @@
/* If we don't have the next packet after this we can directly return the frame, as there is no
* chance it will be overwritten.
*/
- if (!ast_data_buffer_get(rtp->recv_buffer, seqno + 1)) {
+ if (!ast_data_buffer_get(rtp->recv_buffer, rtp->expectedrxseqno)) {
frame = ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
return AST_LIST_FIRST(&frames);
@@ -7626,9 +7666,10 @@
* to the head of the frames list and avoid having to duplicate it but this would result in out
* of order packet processing by libsrtp which we are trying to avoid.
*/
- frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, read_area, res, seqno - 1));
+ frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno));
if (frame) {
AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
+ prev_seqno = seqno;
}
/* Add any additional packets that we have buffered and that are available */
@@ -7640,7 +7681,7 @@
break;
}
- frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, payload->buf, payload->size, rtp->expectedrxseqno - 1));
+ frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, payload->buf, payload->size, prev_seqno));
ast_free(payload);
if (!frame) {
@@ -7653,11 +7694,15 @@
ast_debug(2, "Pulled buffered packet with sequence number '%d' to additionally return on RTP instance '%p'\n",
frame->seqno, instance);
AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
+ prev_seqno = rtp->expectedrxseqno;
rtp->expectedrxseqno++;
+ if (rtp->expectedrxseqno == SEQNO_CYCLE_OVER) {
+ rtp->expectedrxseqno = 0;
+ }
}
return AST_LIST_FIRST(&frames);
- } else if ((abs(seqno - rtp->expectedrxseqno) > 100) ||
+ } else if (((abs(seqno - rtp->expectedrxseqno) > 100) && timestamp > rtp->lastividtimestamp) ||
ast_data_buffer_count(rtp->recv_buffer) == ast_data_buffer_max(rtp->recv_buffer)) {
int inserted = 0;
@@ -7673,43 +7718,46 @@
rtp_write_rtcp_fir(instance, rtp, &remote_address);
}
+ /* This works by going through the progression of the sequence number retrieving buffered packets
+ * or inserting the current received packet until we've run out of packets. This ensures that the
+ * packets are in the correct sequence number order.
+ */
while (ast_data_buffer_count(rtp->recv_buffer)) {
struct ast_rtp_rtcp_nack_payload *payload;
- payload = (struct ast_rtp_rtcp_nack_payload *)ast_data_buffer_remove_head(rtp->recv_buffer);
- if (!payload) {
+ /* If the packet we received is the one we are expecting at this point then add it in */
+ if (rtp->expectedrxseqno == seqno) {
+ frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno));
+ if (frame) {
+ AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
+ prev_seqno = seqno;
+ ast_debug(2, "Inserted just received packet with sequence number '%d' in correct order on RTP instance '%p'\n",
+ seqno, instance);
+ }
+ rtp->expectedrxseqno++;
+ if (rtp->expectedrxseqno == SEQNO_CYCLE_OVER) {
+ rtp->expectedrxseqno = 0;
+ }
+ inserted = 1;
continue;
}
- /* Even when dumping the receive buffer we do our best to order things, so we ensure that the
- * packet we just received is processed in the correct order, so see if we need to insert it now.
- */
- if (!inserted) {
- int buffer_seqno;
-
- buffer_seqno = ntohl(payload->buf[0]) & 0xffff;
- if (seqno < buffer_seqno) {
- frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno));
- if (frame) {
- AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
- rtp->expectedrxseqno = seqno + 1;
- prev_seqno = seqno;
- ast_debug(2, "Inserted just received packet with sequence number '%d' in correct order on RTP instance '%p'\n",
- seqno, instance);
- }
- inserted = 1;
+ payload = (struct ast_rtp_rtcp_nack_payload *)ast_data_buffer_remove(rtp->recv_buffer, rtp->expectedrxseqno);
+ if (payload) {
+ frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, payload->buf, payload->size, prev_seqno));
+ if (frame) {
+ AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
+ prev_seqno = rtp->expectedrxseqno;
+ ast_debug(2, "Emptying queue and returning packet with sequence number '%d' from RTP instance '%p'\n",
+ frame->seqno, instance);
}
+ ast_free(payload);
}
- frame = ast_frdup(ast_rtp_interpret(instance, srtp, &addr, payload->buf, payload->size, prev_seqno));
- if (frame) {
- AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
- prev_seqno = frame->seqno;
- ast_debug(2, "Emptying queue and returning packet with sequence number '%d' from RTP instance '%p'\n",
- frame->seqno, instance);
+ rtp->expectedrxseqno++;
+ if (rtp->expectedrxseqno == SEQNO_CYCLE_OVER) {
+ rtp->expectedrxseqno = 0;
}
-
- ast_free(payload);
}
if (!inserted) {
@@ -7721,11 +7769,21 @@
frame = ast_rtp_interpret(instance, srtp, &addr, read_area, res, prev_seqno);
AST_LIST_INSERT_TAIL(&frames, frame, frame_list);
rtp->expectedrxseqno = seqno + 1;
+ if (rtp->expectedrxseqno == SEQNO_CYCLE_OVER) {
+ rtp->expectedrxseqno = 0;
+ }
ast_debug(2, "Adding just received packet with sequence number '%d' to end of dumped queue on RTP instance '%p'\n",
seqno, instance);
}
+ /* When we flush increase our chance for next time by growing the receive buffer when possible
+ * by how many packets we missed, to give ourselves a bit more breathing room.
+ */
+ ast_data_buffer_resize(rtp->recv_buffer, MIN(MAXIMUM_RTP_RECV_BUFFER_SIZE,
+ ast_data_buffer_max(rtp->recv_buffer) + AST_VECTOR_SIZE(&rtp->missing_seqno)));
+ ast_debug(2, "Receive buffer on RTP instance '%p' is now at maximum of %zu\n", instance, ast_data_buffer_max(rtp->recv_buffer));
+
/* As there is such a large gap we don't want to flood the order side with missing packets, so we
* give up and start anew.
*/
@@ -7737,7 +7795,18 @@
/* We're finished with the frames list */
ast_frame_free(AST_LIST_FIRST(&frames), 0);
- if (seqno < rtp->expectedrxseqno) {
+ /* Determine if the received packet is from the last OLD_PACKET_COUNT (1000 by default) packets or not.
+ * For the case where the received sequence number exceeds that of the expected sequence number we calculate
+ * the past sequence number that would be 1000 sequence numbers ago. If the received sequence number
+ * exceeds or meets that then it is within OLD_PACKET_COUNT packets ago. For example if the expected
+ * sequence number is 100 and we receive 65530, then it would be considered old. This is because
+ * 65535 - 1000 + 100 = 64635 which gives us the sequence number at which we would consider the packets
+ * old. Since 65530 is above that, it would be considered old.
+ * For the case where the received sequence number is less than the expected sequence number we can do
+ * a simple subtraction to see if it is 1000 packets ago or not.
+ */
+ if ((seqno < rtp->expectedrxseqno && ((rtp->expectedrxseqno - seqno) <= OLD_PACKET_COUNT)) ||
+ (seqno > rtp->expectedrxseqno && (seqno >= (65535 - OLD_PACKET_COUNT + rtp->expectedrxseqno)))) {
/* If this is a packet from the past then we have received a duplicate packet, so just drop it */
ast_debug(2, "Received an old packet with sequence number '%d' on RTP instance '%p', dropping it\n",
seqno, instance);
@@ -7750,10 +7819,12 @@
} else {
/* This is an out of order packet from the future */
struct ast_rtp_rtcp_nack_payload *payload;
- int difference;
+ int missing_seqno;
+ int remove_failed;
+ unsigned int missing_seqnos_added = 0;
- ast_debug(2, "Received an out of order packet with sequence number '%d' from the future on RTP instance '%p'\n",
- seqno, instance);
+ ast_debug(2, "Received an out of order packet with sequence number '%d' while expecting '%d' from the future on RTP instance '%p'\n",
+ seqno, rtp->expectedrxseqno, instance);
payload = ast_malloc(sizeof(*payload) + res);
if (!payload) {
@@ -7769,11 +7840,38 @@
payload->size = res;
memcpy(payload->buf, rtpheader, res);
ast_data_buffer_put(rtp->recv_buffer, seqno, payload);
- AST_VECTOR_REMOVE_CMP_ORDERED(&rtp->missing_seqno, seqno, find_by_value,
- AST_VECTOR_ELEM_CLEANUP_NOOP);
- difference = seqno - (prev_seqno + 1);
- while (difference > 0) {
+ /* If this sequence number is removed that means we had a gap and this packet has filled it in
+ * some. Since it was part of the gap we will have already added any other missing sequence numbers
+ * before it (and possibly after it) to the vector so we don't need to do that again. Note that
+ * remove_failed will be set to -1 if the sequence number isn't removed, and 0 if it is.
+ */
+ remove_failed = AST_VECTOR_REMOVE_CMP_ORDERED(&rtp->missing_seqno, seqno, find_by_value,
+ AST_VECTOR_ELEM_CLEANUP_NOOP);
+ if (!remove_failed) {
+ ast_debug(2, "Packet with sequence number '%d' on RTP instance '%p' is no longer missing\n",
+ seqno, instance);
+ }
+
+ /* The missing sequence number code works by taking the sequence number of the
+ * packet we've just received and going backwards until we hit the sequence number
+ * of the last packet we've received. While doing so we check to make sure that the
+ * sequence number is not already missing and that it is not already buffered.
+ */
+ missing_seqno = seqno;
+ while (remove_failed) {
+ missing_seqno -= 1;
+
+ /* If we've cycled backwards then start back at the top */
+ if (missing_seqno < 0) {
+ missing_seqno = 65535;
+ }
+
+ /* We've gone backwards enough such that we've hit the previous sequence number */
+ if (missing_seqno == prev_seqno) {
+ break;
+ }
+
/* We don't want missing sequence number duplicates. If, for some reason,
* packets are really out of order, we could end up in this scenario:
*
@@ -7786,24 +7884,34 @@
*
* This will prevent the duplicate from being added.
*/
- if (AST_VECTOR_GET_CMP(&rtp->missing_seqno, seqno - difference,
+ if (AST_VECTOR_GET_CMP(&rtp->missing_seqno, missing_seqno,
find_by_value)) {
- difference--;
+ continue;
+ }
+
+ /* If this packet has been buffered already then don't count it amongst the
+ * missing.
+ */
+ if (ast_data_buffer_get(rtp->recv_buffer, missing_seqno)) {
continue;
}
ast_debug(2, "Added missing sequence number '%d' to RTP instance '%p'\n",
- seqno - difference, instance);
- AST_VECTOR_ADD_SORTED(&rtp->missing_seqno, seqno - difference,
+ missing_seqno, instance);
+ AST_VECTOR_ADD_SORTED(&rtp->missing_seqno, missing_seqno,
compare_by_value);
- difference--;
+ missing_seqnos_added++;
}
- /* When our data buffer is half full we assume that the packets aren't just out of order but
- * have actually been lost. To get them back we construct and send a NACK causing the sender to
- * retransmit them.
+ /* When we add a large number of missing sequence numbers we assume there was a substantial
+ * gap in reception so we trigger an immediate NACK. When our data buffer is 1/4 full we
+ * assume that the packets aren't just out of order but have actually been lost. At 1/2
+ * full we get more aggressive and ask for retransmission when we get a new packet.
+ * To get them back we construct and send a NACK causing the sender to retransmit them.
*/
- if (ast_data_buffer_count(rtp->recv_buffer) == ast_data_buffer_max(rtp->recv_buffer) / 2) {
+ if (missing_seqnos_added >= MISSING_SEQNOS_ADDED_TRIGGER ||
+ ast_data_buffer_count(rtp->recv_buffer) == ast_data_buffer_max(rtp->recv_buffer) / 4 ||
+ ast_data_buffer_count(rtp->recv_buffer) >= ast_data_buffer_max(rtp->recv_buffer) / 2) {
int packet_len = 0;
int res = 0;
int ice;
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13869
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: certified/16.8
Gerrit-Change-Id: I97633dfa8a09a7889cef815b2be369f3f0314b41
Gerrit-Change-Number: 13869
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200303/ba641bde/attachment-0001.html>
More information about the asterisk-code-review
mailing list