<p>Jenkins2 <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/8773">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_rtp_asterisk: Add support for receiving and handling NACK requests.<br><br>Adds the ability to receive and handle incoming NACK requests if<br>retransmissions are enabled. If retransmissions are enabled, a data<br>buffer is allocated that stores packets being sent. If a NACK request<br>is received, the packet requested for retransmission is sent if it is<br>still in the buffer. In the same request, if any of the following 16<br>packets are marked as not received, those will be sent as well if<br>available, as outlined in RFC4585.<br><br>Also changes RTCP RR and SR to use media source SSRC instead of packet<br>source SSRC when determining which instance to use for RTCP reports.<br><br>For more information, refer to the wiki page:<br>https://wiki.asterisk.org/wiki/display/AST/WebRTC+User+Experience+Improvements<br><br>ASTERISK-27806 #close<br><br>Change-Id: I7f7f124af3b9d5d2fd9cffc6ba8cb48a6fff06ec<br>---<br>M include/asterisk/rtp_engine.h<br>M res/res_pjsip_sdp_rtp.c<br>M res/res_rtp_asterisk.c<br>3 files changed, 170 insertions(+), 10 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h<br>index 8f044ce..3426b2a 100644<br>--- a/include/asterisk/rtp_engine.h<br>+++ b/include/asterisk/rtp_engine.h<br>@@ -292,10 +292,14 @@<br> #define AST_RTP_RTCP_SR 200<br> /*! Receiver Report */<br> #define AST_RTP_RTCP_RR 201<br>+/*! Transport Layer Feed Back (From RFC4585 also RFC5104) */<br>+#define AST_RTP_RTCP_RTPFB 205<br> /*! Payload Specific Feed Back (From RFC4585 also RFC5104) */<br>-#define AST_RTP_RTCP_PSFB 206<br>+#define AST_RTP_RTCP_PSFB 206<br> <br> /* Common RTCP feedback message types */<br>+/*! Generic NACK (From RFC4585 also RFC5104) */<br>+#define AST_RTP_RTCP_FMT_NACK 1<br> /*! Picture loss indication (From RFC4585) */<br> #define AST_RTP_RTCP_FMT_PLI 1<br> /*! Full INTRA-frame Request (From RFC5104) */<br>diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c<br>index 11ab726..f6fb17d 100644<br>--- a/res/res_pjsip_sdp_rtp.c<br>+++ b/res/res_pjsip_sdp_rtp.c<br>@@ -1096,6 +1096,9 @@<br> <br> attr = pjmedia_sdp_attr_create(pool, "rtcp-fb", pj_cstr(&stmp, "* goog-remb"));<br> pjmedia_sdp_attr_add(&media->attr_count, media->attr, attr);<br>+<br>+ attr = pjmedia_sdp_attr_create(pool, "rtcp-fb", pj_cstr(&stmp, "* nack"));<br>+ pjmedia_sdp_attr_add(&media->attr_count, media->attr, attr);<br> }<br> <br> /*! \brief Function which negotiates an incoming media stream */<br>diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c<br>index c889037..4ac20d5 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -71,6 +71,7 @@<br> #include "asterisk/smoother.h"<br> #include "asterisk/uuid.h"<br> #include "asterisk/test.h"<br>+#include "asterisk/data_buffer.h"<br> #ifdef HAVE_PJPROJECT<br> #include "asterisk/res_pjproject.h"<br> #endif<br>@@ -91,6 +92,8 @@<br> #define DEFAULT_TURN_PORT 3478<br> <br> #define TURN_STATE_WAIT_TIME 2000<br>+<br>+#define DEFAULT_RTP_BUFFER_SIZE 250<br> <br> /*! Full INTRA-frame Request / Fast Update Request (From RFC2032) */<br> #define RTCP_PT_FUR 192<br>@@ -373,6 +376,8 @@<br> <br> struct rtp_red *red;<br> <br>+ struct ast_data_buffer *send_buffer; /*!< Buffer for storing sent packets for retransmission */<br>+<br> #ifdef HAVE_PJPROJECT<br> ast_cond_t cond; /*!< ICE/TURN condition for signaling */<br> <br>@@ -507,6 +512,12 @@<br> unsigned char buf_data[64000]; /*!< buffered primary data */<br> int hdrlen;<br> long int prev_ts;<br>+};<br>+<br>+/*! \brief Structure for storing RTP packets for retransmission */<br>+struct ast_rtp_rtcp_nack_payload {<br>+ size_t size; /*!< The size of the payload */<br>+ unsigned char buf[0]; /*!< The payload data */<br> };<br> <br> AST_LIST_HEAD_NOLOCK(frame_list, ast_frame);<br>@@ -3675,6 +3686,11 @@<br> rtp->red = NULL;<br> }<br> <br>+ /* Destroy the send buffer if it was being used */<br>+ if (rtp->send_buffer) {<br>+ ast_data_buffer_free(rtp->send_buffer);<br>+ }<br>+<br> ao2_cleanup(rtp->lasttxformat);<br> ao2_cleanup(rtp->lastrxformat);<br> ao2_cleanup(rtp->f.subclass.format);<br>@@ -4369,7 +4385,7 @@<br> } else {<br> /* This is the first frame with sequence number we've seen, so start keeping track */<br> rtp->expectedseqno = frame->seqno + 1;<br>- }<br>+ }<br> } else {<br> rtp->expectedseqno = -1;<br> }<br>@@ -4383,13 +4399,27 @@<br> /* If we know the remote address construct a packet and send it out */<br> if (!ast_sockaddr_isnull(&remote_address)) {<br> int hdrlen = 12, res, ice;<br>+ int packet_len = frame->datalen + hdrlen;<br> unsigned char *rtpheader = (unsigned char *)(frame->data.ptr - hdrlen);<br> <br> put_unaligned_uint32(rtpheader, htonl((2 << 30) | (codec << 16) | (seqno) | (mark << 23)));<br> put_unaligned_uint32(rtpheader + 4, htonl(rtp->lastts));<br> put_unaligned_uint32(rtpheader + 8, htonl(rtp->ssrc));<br> <br>- if ((res = rtp_sendto(instance, (void *)rtpheader, frame->datalen + hdrlen, 0, &remote_address, &ice)) < 0) {<br>+ /* If retransmissions are enabled, we need to store this packet for future use */<br>+ if (rtp->send_buffer) {<br>+ struct ast_rtp_rtcp_nack_payload *payload;<br>+<br>+ payload = ast_malloc(sizeof(*payload) + packet_len);<br>+ if (payload) {<br>+ payload->size = packet_len;<br>+ memcpy(payload->buf, rtpheader, packet_len);<br>+ ast_data_buffer_put(rtp->send_buffer, rtp->seqno, payload);<br>+ }<br>+ }<br>+<br>+ res = rtp_sendto(instance, (void *)rtpheader, packet_len, 0, &remote_address, &ice);<br>+ if (res < 0) {<br> if (!ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_NAT) || (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_NAT) && (ast_test_flag(rtp, FLAG_NAT_ACTIVE) == FLAG_NAT_ACTIVE))) {<br> ast_debug(1, "RTP Transmission error of packet %d to %s: %s\n",<br> rtp->seqno,<br>@@ -5145,8 +5175,8 @@<br> }<br> <br> /*! \pre instance is locked */<br>-static struct ast_rtp_instance *rtp_find_instance_by_ssrc(struct ast_rtp_instance *instance,<br>- struct ast_rtp *rtp, unsigned int ssrc)<br>+static struct ast_rtp_instance *__rtp_find_instance_by_ssrc(struct ast_rtp_instance *instance,<br>+ struct ast_rtp *rtp, unsigned int ssrc, int source)<br> {<br> int index;<br> <br>@@ -5158,8 +5188,9 @@<br> /* Find the bundled child instance */<br> for (index = 0; index < AST_VECTOR_SIZE(&rtp->ssrc_mapping); ++index) {<br> struct rtp_ssrc_mapping *mapping = AST_VECTOR_GET_ADDR(&rtp->ssrc_mapping, index);<br>+ unsigned int mapping_ssrc = source ? ast_rtp_get_ssrc(mapping->instance) : mapping->ssrc;<br> <br>- if (mapping->ssrc_valid && mapping->ssrc == ssrc) {<br>+ if (mapping->ssrc_valid && mapping_ssrc == ssrc) {<br> return mapping->instance;<br> }<br> }<br>@@ -5169,6 +5200,20 @@<br> return instance;<br> }<br> return NULL;<br>+}<br>+<br>+/*! \pre instance is locked */<br>+static struct ast_rtp_instance *rtp_find_instance_by_packet_source_ssrc(struct ast_rtp_instance *instance,<br>+ struct ast_rtp *rtp, unsigned int ssrc)<br>+{<br>+ return __rtp_find_instance_by_ssrc(instance, rtp, ssrc, 0);<br>+}<br>+<br>+/*! \pre instance is locked */<br>+static struct ast_rtp_instance *rtp_find_instance_by_media_source_ssrc(struct ast_rtp_instance *instance,<br>+ struct ast_rtp *rtp, unsigned int ssrc)<br>+{<br>+ return __rtp_find_instance_by_ssrc(instance, rtp, ssrc, 1);<br> }<br> <br> static const char *rtcp_payload_type2str(unsigned int pt)<br>@@ -5201,6 +5246,69 @@<br> break;<br> }<br> return str;<br>+}<br>+<br>+/*! \pre instance is locked */<br>+static int ast_rtp_rtcp_handle_nack(struct ast_rtp_instance *instance, unsigned int *nackdata, unsigned int position,<br>+ unsigned int length)<br>+{<br>+ struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);<br>+ int res = 0;<br>+ int blp_index;<br>+ int packet_index;<br>+ int ice;<br>+ struct ast_rtp_rtcp_nack_payload *payload;<br>+ unsigned int current_word;<br>+ unsigned int pid; /* Packet ID which refers to seqno of lost packet */<br>+ unsigned int blp; /* Bitmask of following lost packets */<br>+ struct ast_sockaddr remote_address = { {0,} };<br>+<br>+ if (!rtp->send_buffer) {<br>+ ast_debug(1, "Tried to handle NACK request, but we don't have a RTP packet storage!\n");<br>+ return res;<br>+ }<br>+<br>+ ast_rtp_instance_get_remote_address(instance, &remote_address);<br>+<br>+ /*<br>+ * We use index 3 because with feedback messages, the FCI (Feedback Control Information)<br>+ * does not begin until after the version, packet SSRC, and media SSRC words.<br>+ */<br>+ for (packet_index = 3; packet_index < length; packet_index++) {<br>+ current_word = ntohl(nackdata[position + packet_index]);<br>+ pid = current_word >> 16;<br>+ /* We know the remote end is missing this packet. Go ahead and send it if we still have it. */<br>+ payload = (struct ast_rtp_rtcp_nack_payload *)ast_data_buffer_get(rtp->send_buffer, pid);<br>+ if (payload) {<br>+ res += rtp_sendto(instance, payload->buf, payload->size, 0, &remote_address, &ice);<br>+ } else {<br>+ ast_debug(1, "Received NACK request for RTP packet with seqno %d, but we don't have it\n", pid);<br>+ }<br>+ /*<br>+ * The bitmask. Denoting the least significant bit as 1 and its most significant bit<br>+ * as 16, then bit i of the bitmask is set to 1 if the receiver has not received RTP<br>+ * packet (pid+i)(modulo 2^16). Otherwise, it is set to 0. We cannot assume bits set<br>+ * to 0 after a bit set to 1 have actually been received.<br>+ */<br>+ blp = current_word & 0xFF;<br>+ blp_index = 1;<br>+ while (blp) {<br>+ if (blp & 1) {<br>+ /* Packet (pid + i)(modulo 2^16) is missing too. */<br>+ unsigned int seqno = (pid + blp_index) % 65536;<br>+ payload = (struct ast_rtp_rtcp_nack_payload *)ast_data_buffer_get(rtp->send_buffer, seqno);<br>+ if (payload) {<br>+ res += rtp_sendto(instance, payload->buf, payload->size, 0, &remote_address, &ice);<br>+ } else {<br>+ ast_debug(1, "Remote end also requested RTP packet with seqno %d, but we don't have it\n", seqno);<br>+ }<br>+ }<br>+ blp >>= 1;<br>+ blp_index++;<br>+ }<br>+ }<br>+<br>+ return res;<br> }<br> <br> /*<br>@@ -5243,6 +5351,7 @@<br> #define RTCP_RR_BLOCK_WORD_LENGTH 6<br> #define RTCP_HEADER_SSRC_LENGTH 2<br> #define RTCP_FB_REMB_BLOCK_WORD_LENGTH 4<br>+#define RTCP_FB_NACK_BLOCK_WORD_LENGTH 2<br> <br> static struct ast_frame *ast_rtcp_interpret(struct ast_rtp_instance *instance, const unsigned char *rtcpdata, size_t size, struct ast_sockaddr *addr)<br> {<br>@@ -5319,6 +5428,8 @@<br> unsigned int ssrc_valid;<br> unsigned int length;<br> unsigned int min_length;<br>+ /*! Always use packet source SSRC to find the rtp instance unless explicitly told not to. */<br>+ unsigned int use_packet_source = 1;<br> <br> struct ast_json *message_blob;<br> RAII_VAR(struct ast_rtp_rtcp_report *, rtcp_report, NULL, ao2_cleanup);<br>@@ -5341,8 +5452,19 @@<br> /* fall through */<br> case RTCP_PT_RR:<br> min_length += (rc * RTCP_RR_BLOCK_WORD_LENGTH);<br>+ use_packet_source = 0;<br> break;<br> case RTCP_PT_FUR:<br>+ break;<br>+ case AST_RTP_RTCP_RTPFB:<br>+ switch (rc) {<br>+ case AST_RTP_RTCP_FMT_NACK:<br>+ min_length += RTCP_FB_NACK_BLOCK_WORD_LENGTH;<br>+ break;<br>+ default:<br>+ break;<br>+ }<br>+ use_packet_source = 0;<br> break;<br> case RTCP_PT_PSFB:<br> switch (rc) {<br>@@ -5392,12 +5514,15 @@<br> }<br> rtcp_report->reception_report_count = rc;<br> <br>- ssrc = ntohl(rtcpheader[i + 1]);<br>+ ssrc = ntohl(rtcpheader[i + 2]);<br> rtcp_report->ssrc = ssrc;<br> break;<br> case RTCP_PT_FUR:<br> case RTCP_PT_PSFB:<br> ssrc = ntohl(rtcpheader[i + 1]);<br>+ break;<br>+ case AST_RTP_RTCP_RTPFB:<br>+ ssrc = ntohl(rtcpheader[i + 2]);<br> break;<br> case RTCP_PT_SDES:<br> case RTCP_PT_BYE:<br>@@ -5417,7 +5542,15 @@<br> <br> /* Determine the appropriate instance for this */<br> if (ssrc_valid) {<br>- child = rtp_find_instance_by_ssrc(transport, transport_rtp, ssrc);<br>+ /*<br>+ * Depending on the payload type, either the packet source or media source<br>+ * SSRC is used.<br>+ */<br>+ if (use_packet_source) {<br>+ child = rtp_find_instance_by_packet_source_ssrc(transport, transport_rtp, ssrc);<br>+ } else {<br>+ child = rtp_find_instance_by_media_source_ssrc(transport, transport_rtp, ssrc);<br>+ }<br> if (child && child != transport) {<br> /*<br> * It is safe to hold the child lock while holding the parent lock.<br>@@ -5438,7 +5571,7 @@<br> }<br> <br> if (ssrc_valid && rtp->themssrc_valid) {<br>- if (ssrc != rtp->themssrc) {<br>+ if (ssrc != rtp->themssrc && use_packet_source) {<br> /*<br> * Skip over this RTCP record as it does not contain the<br> * correct SSRC. We should not act upon RTCP records<br>@@ -5580,6 +5713,24 @@<br> transport_rtp->f.delivery.tv_usec = 0;<br> transport_rtp->f.src = "RTP";<br> f = &transport_rtp->f;<br>+ break;<br>+ case AST_RTP_RTCP_RTPFB:<br>+ switch (rc) {<br>+ case AST_RTP_RTCP_FMT_NACK:<br>+ /* If retransmissions are not enabled ignore this message */<br>+ if (!rtp->send_buffer) {<br>+ break;<br>+ }<br>+<br>+ if (rtcp_debug_test_addr(addr)) {<br>+ ast_verbose("Received generic RTCP NACK message\n");<br>+ }<br>+<br>+ ast_rtp_rtcp_handle_nack(instance, rtcpheader, position, length);<br>+ break;<br>+ default:<br>+ break;<br>+ }<br> break;<br> case RTCP_PT_FUR:<br> /* Handle RTCP FUR as FIR by setting the format to 4 */<br>@@ -5981,7 +6132,7 @@<br> ssrc = ntohl(rtpheader[2]);<br> <br> /* Determine the appropriate instance for this */<br>- child = rtp_find_instance_by_ssrc(instance, rtp, ssrc);<br>+ child = rtp_find_instance_by_packet_source_ssrc(instance, rtp, ssrc);<br> if (!child) {<br> /* Neither the bundled parent nor any child has this SSRC */<br> return &ast_null_frame;<br>@@ -6614,6 +6765,8 @@<br> }<br> } else if (property == AST_RTP_PROPERTY_ASYMMETRIC_CODEC) {<br> rtp->asymmetric_codec = value;<br>+ } else if (property == AST_RTP_PROPERTY_RETRANS_SEND) {<br>+ rtp->send_buffer = ast_data_buffer_alloc(ast_free_ptr, DEFAULT_RTP_BUFFER_SIZE);<br> }<br> }<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/8773">change 8773</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/8773"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I7f7f124af3b9d5d2fd9cffc6ba8cb48a6fff06ec </div>
<div style="display:none"> Gerrit-Change-Number: 8773 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>