[Asterisk-code-review] res_rtp_asterisk.c: Send RTCP as compound packets. (...asterisk[master])
George Joseph
asteriskteam at digium.com
Tue Sep 17 09:26:19 CDT 2019
George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/12865 )
Change subject: res_rtp_asterisk.c: Send RTCP as compound packets.
......................................................................
res_rtp_asterisk.c: Send RTCP as compound packets.
According to RFC3550, ALL RTCP packets must be sent in a compond packet
of at least two individual packets, including SR/RR and SDES. REMB,
FIR, and NACK were not following this format, and as a result, would
fail the packet check in ast_rtcp_interpret. This was found from writing
unit tests for RTCP. The browser would accept the way we were
constructing these RTCP packets, but when sending directly from one
Asterisk instance to another, the above mentioned problem would occur.
Change-Id: Ieb140e9c22568a251a564cd953dd22cd33244605
---
M res/res_rtp_asterisk.c
1 file changed, 98 insertions(+), 35 deletions(-)
Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved; Approved for Submit
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 5da095e..e96223f 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -215,7 +215,6 @@
static struct ast_ha *stun_blacklist = NULL;
static ast_rwlock_t stun_blacklist_lock = AST_RWLOCK_INIT_VALUE;
-
/*! \brief Pool factory used by pjlib to allocate memory. */
static pj_caching_pool cachingpool;
@@ -4500,6 +4499,41 @@
return len;
}
+/* Lock instance before calling this if it isn't already
+ *
+ * If successful, the overall packet length is returned
+ * If not, then 0 is returned
+ */
+static int ast_rtcp_generate_compound_prefix(struct ast_rtp_instance *instance, unsigned char *rtcpheader,
+ struct ast_rtp_rtcp_report *report, int *sr)
+{
+ int packet_len = 0;
+ int res;
+
+ /* Every RTCP packet needs to be sent out with a SR/RR and SDES prefixing it.
+ * At the end of this function, rtcpheader should contain both of those packets,
+ * and will return the length of the overall packet. This can be used to determine
+ * where further packets can be inserted in the compound packet.
+ */
+ res = ast_rtcp_generate_report(instance, rtcpheader, report, sr);
+
+ if (res == 0 || res == 1) {
+ ast_debug(1, "Failed to generate %s report!\n", sr ? "SR" : "RR");
+ return 0;
+ }
+
+ packet_len += res;
+
+ res = ast_rtcp_generate_sdes(instance, rtcpheader + packet_len, report);
+
+ if (res == 0 || res == 1) {
+ ast_debug(1, "Failed to generate SDES!\n");
+ return 0;
+ }
+
+ return packet_len + res;
+}
+
static int ast_rtcp_generate_nack(struct ast_rtp_instance *instance, unsigned char *rtcpheader)
{
struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
@@ -4590,19 +4624,9 @@
ao2_lock(instance);
rtcpheader = bdata;
- res = ast_rtcp_generate_report(instance, rtcpheader, rtcp_report, &sr);
+ res = ast_rtcp_generate_compound_prefix(instance, rtcpheader, rtcp_report, &sr);
if (res == 0 || res == 1) {
- ast_debug(1, "Failed to add %s report to RTCP packet!\n", sr ? "SR" : "RR");
- goto cleanup;
- }
-
- packet_len += res;
-
- res = ast_rtcp_generate_sdes(instance, rtcpheader + packet_len, rtcp_report);
-
- if (res == 0 || res == 1) {
- ast_debug(1, "Failed to add SDES to RTCP packet!\n");
goto cleanup;
}
@@ -4920,11 +4944,16 @@
static void rtp_write_rtcp_fir(struct ast_rtp_instance *instance, struct ast_rtp *rtp, struct ast_sockaddr *remote_address)
{
- unsigned int *rtcpheader;
- char bdata[1024];
- int len = 20;
+ unsigned char *rtcpheader;
+ unsigned char bdata[1024];
+ int packet_len = 0;
+ int fir_len = 20;
int ice;
int res;
+ int sr;
+ RAII_VAR(struct ast_rtp_rtcp_report *, rtcp_report,
+ ast_rtp_rtcp_report_alloc(rtp->themssrc_valid ? 1 : 0),
+ ao2_cleanup);
if (!rtp || !rtp->rtcp) {
return;
@@ -4948,26 +4977,46 @@
rtp->rtcp->firseq = 0;
}
- rtcpheader = (unsigned int *)bdata;
- rtcpheader[0] = htonl((2 << 30) | (4 << 24) | (RTCP_PT_PSFB << 16) | ((len/4)-1));
- rtcpheader[1] = htonl(rtp->ssrc);
- rtcpheader[2] = htonl(rtp->themssrc);
- rtcpheader[3] = htonl(rtp->themssrc); /* FCI: SSRC */
- rtcpheader[4] = htonl(rtp->rtcp->firseq << 24); /* FCI: Sequence number */
- res = rtcp_sendto(instance, (unsigned int *)rtcpheader, len, 0, rtp->bundled ? remote_address : &rtp->rtcp->them, &ice);
+ rtcpheader = bdata;
+
+ ao2_lock(instance);
+ res = ast_rtcp_generate_compound_prefix(instance, rtcpheader, rtcp_report, &sr);
+
+ if (res == 0 || res == 1) {
+ ao2_unlock(instance);
+ return;
+ }
+
+ packet_len += res;
+
+ put_unaligned_uint32(rtcpheader + packet_len + 0, htonl((2 << 30) | (4 << 24) | (RTCP_PT_PSFB << 16) | ((fir_len/4)-1)));
+ put_unaligned_uint32(rtcpheader + packet_len + 4, htonl(rtp->ssrc));
+ put_unaligned_uint32(rtcpheader + packet_len + 8, htonl(rtp->themssrc));
+ put_unaligned_uint32(rtcpheader + packet_len + 12, htonl(rtp->themssrc)); /* FCI: SSRC */
+ put_unaligned_uint32(rtcpheader + packet_len + 16, htonl(rtp->rtcp->firseq << 24)); /* FCI: Sequence number */
+ res = rtcp_sendto(instance, (unsigned int *)rtcpheader, packet_len + fir_len, 0, rtp->bundled ? remote_address : &rtp->rtcp->them, &ice);
if (res < 0) {
ast_log(LOG_ERROR, "RTCP FIR transmission error: %s\n", strerror(errno));
+ } else {
+ ast_rtcp_calculate_sr_rr_statistics(instance, rtcp_report, rtp->bundled ? *remote_address : rtp->rtcp->them, ice, sr);
}
+
+ ao2_unlock(instance);
}
static void rtp_write_rtcp_psfb(struct ast_rtp_instance *instance, struct ast_rtp *rtp, struct ast_frame *frame, struct ast_sockaddr *remote_address)
{
struct ast_rtp_rtcp_feedback *feedback = frame->data.ptr;
- unsigned int *rtcpheader;
- char bdata[1024];
- int len = 24;
+ unsigned char *rtcpheader;
+ unsigned char bdata[1024];
+ int remb_len = 24;
int ice;
int res;
+ int sr = 0;
+ int packet_len = 0;
+ RAII_VAR(struct ast_rtp_rtcp_report *, rtcp_report,
+ ast_rtp_rtcp_report_alloc(rtp->themssrc_valid ? 1 : 0),
+ ao2_cleanup);
if (feedback->fmt != AST_RTP_RTCP_FMT_REMB) {
ast_debug(1, "Provided an RTCP feedback frame of format %d to write on RTP instance '%p' but only REMB is supported\n",
@@ -4993,17 +5042,32 @@
return;
}
- rtcpheader = (unsigned int *)bdata;
- rtcpheader[0] = htonl((2 << 30) | (AST_RTP_RTCP_FMT_REMB << 24) | (RTCP_PT_PSFB << 16) | ((len/4)-1));
- rtcpheader[1] = htonl(rtp->ssrc);
- rtcpheader[2] = htonl(0); /* Per the draft this should always be 0 */
- rtcpheader[3] = htonl(('R' << 24) | ('E' << 16) | ('M' << 8) | ('B')); /* Unique identifier 'R' 'E' 'M' 'B' */
- rtcpheader[4] = htonl((1 << 24) | (feedback->remb.br_exp << 18) | (feedback->remb.br_mantissa)); /* Number of SSRCs / BR Exp / BR Mantissa */
- rtcpheader[5] = htonl(rtp->ssrc); /* The SSRC this feedback message applies to */
- res = rtcp_sendto(instance, (unsigned int *)rtcpheader, len, 0, rtp->bundled ? remote_address : &rtp->rtcp->them, &ice);
+ rtcpheader = bdata;
+
+ ao2_lock(instance);
+ res = ast_rtcp_generate_compound_prefix(instance, rtcpheader, rtcp_report, &sr);
+
+ if (res == 0 || res == 1) {
+ ao2_unlock(instance);
+ return;
+ }
+
+ packet_len += res;
+
+ put_unaligned_uint32(rtcpheader + packet_len + 0, htonl((2 << 30) | (AST_RTP_RTCP_FMT_REMB << 24) | (RTCP_PT_PSFB << 16) | ((remb_len/4)-1)));
+ put_unaligned_uint32(rtcpheader + packet_len + 4, htonl(rtp->ssrc));
+ put_unaligned_uint32(rtcpheader + packet_len + 8, htonl(0)); /* Per the draft, this should always be 0 */
+ put_unaligned_uint32(rtcpheader + packet_len + 12, htonl(('R' << 24) | ('E' << 16) | ('M' << 8) | ('B'))); /* Unique identifier 'R' 'E' 'M' 'B' */
+ put_unaligned_uint32(rtcpheader + packet_len + 16, htonl((1 << 24) | (feedback->remb.br_exp << 18) | (feedback->remb.br_mantissa))); /* Number of SSRCs / BR Exp / BR Mantissa */
+ put_unaligned_uint32(rtcpheader + packet_len + 20, htonl(rtp->ssrc)); /* The SSRC this feedback message applies to */
+ res = rtcp_sendto(instance, (unsigned int *)rtcpheader, packet_len + remb_len, 0, rtp->bundled ? remote_address : &rtp->rtcp->them, &ice);
if (res < 0) {
ast_log(LOG_ERROR, "RTCP PSFB transmission error: %s\n", strerror(errno));
+ } else {
+ ast_rtcp_calculate_sr_rr_statistics(instance, rtcp_report, rtp->bundled ? *remote_address : rtp->rtcp->them, ice, sr);
}
+
+ ao2_unlock(instance);
}
/*! \pre instance is locked */
@@ -7720,10 +7784,9 @@
memset(rtcpheader, 0, data_size);
- res = ast_rtcp_generate_report(instance, rtcpheader, rtcp_report, &sr);
+ res = ast_rtcp_generate_compound_prefix(instance, rtcpheader, rtcp_report, &sr);
if (res == 0 || res == 1) {
- ast_debug(1, "Failed to add %s report to NACK, stopping here\n", sr ? "SR" : "RR");
return &ast_null_frame;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/12865
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ieb140e9c22568a251a564cd953dd22cd33244605
Gerrit-Change-Number: 12865
Gerrit-PatchSet: 3
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp 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/20190917/54abbf5d/attachment-0001.html>
More information about the asterisk-code-review
mailing list