[asterisk-commits] oej: branch group/pinefool-poor-mans-plc-1.4 r384878 - in /team/group/pinefoo...
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Mon Apr 8 07:24:46 CDT 2013
Author: oej
Date: Mon Apr 8 07:24:41 2013
New Revision: 384878
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=384878
Log:
Make PLC work properly. Now it rocks.
Modified:
team/group/pinefool-poor-mans-plc-1.4/README.pinefool-poor-mans-plc
team/group/pinefool-poor-mans-plc-1.4/main/rtp.c
Modified: team/group/pinefool-poor-mans-plc-1.4/README.pinefool-poor-mans-plc
URL: http://svnview.digium.com/svn/asterisk/team/group/pinefool-poor-mans-plc-1.4/README.pinefool-poor-mans-plc?view=diff&rev=384878&r1=384877&r2=384878
==============================================================================
--- team/group/pinefool-poor-mans-plc-1.4/README.pinefool-poor-mans-plc (original)
+++ team/group/pinefool-poor-mans-plc-1.4/README.pinefool-poor-mans-plc Mon Apr 8 07:24:41 2013
@@ -16,21 +16,33 @@
We see a lot of issues in the RTP streams when bridging two sip calls in Asterisk. If there's
packet loss coming in, it's the same going out - but in this case the sequence numbers doesn't
indicate packet loss. This causes issues with recordings sounding bad and possibly other issues
-since we have quite long phone calls.
+since we have quite long phone calls. If the packets coming in are reordered, they go out
+the same - but this time resequenced to hide the reordering. This can be very bad for some
+codecs
Investigating the PLC in Asterisk, according to Malcolm's wiki article there has to be a core
bridge with transcoding and a jitter buffer. That won't work for us.
-Looking into the RTP channel, where I've spent some time recently, I notice that we don't bother
-checking that packets are sequential, so the RTP channel doesn't bother with packet loss much. That needed to change.
+Looking into the RTP channel, where I've spent some time recently, I notice that Asterisk does not bother
+checking that packets are sequential, so the RTP channel ignores both packet loss and reordering.
+That needed to change.
I created a very simple PLC in the RTP channel for the incoming stream, following these principles:
+PLC mode (configured in sip.conf)
+==================================
- If there is packet loss, resend the previous packet. We discover this at the next packet, so we
introduce two frames at once. In most cases with small packet loss, the receiving end will sort this
out in the jitter buffer.
- If there is packet reordering, a jitter causing a packet to arrive too late, we ignore that packet
since we've already replaced it with another one.
+
+Shit-in-shit-out mode (configured in rtp.conf)
+==============================================
+- If there is packet loss, transmit RTP with gaps in sequence numbers to indicate reordering
+- If packets are reordered, transmit reordered
+
+(turn this on with donthidepacketloss=yes in rtp.conf)
Normally PLC would work hand-in-hand with a jitter buffer so we first reorder incoming RTP packets, then add the
missing ones. In this case, I can't afford introducing extra delay caused by a jitter buffer, so I needed a poor
@@ -40,10 +52,18 @@
How does it work?
================
+PLC mode:
- If we receive packets 2,3,5 - we will send 2,3,3,5 into the bridge, making sure that we get no skew.
- If we receive 2,3,5,4 we will send 2,3,3,5 and throw away #4.
-In both cases, the extra "3" will be sent at the same time as the "5" into the bridge.
+SISO mode:
+- If we receive packets 2,3,5 we will send 2,3,5
+- If we receive packets 1,3,2,4,5 we will send them as 1,3,2,4,5
+
+Before this patch:
+- If we receive packets 1,2,3,5 we will send them as 1,2,3,4
+- If we receive packets 1,3,2,4,5 we will send them as 1,2,3,4,5 - just new numbers,
+ the media is still not in order.
Looking at wireshark on the other side of the call, the outbound stream now looks better. Of course,
there's jitter but no packet loss in the stream, we still have all expected frames - for Alaw 50 RTP
@@ -55,24 +75,24 @@
Configuration
=============
-You can either enable it globally or per device in sip.conf. The SIP channel only
+PLC: You can either enable it globally or per device in sip.conf. The SIP channel only
use this for audio streams.
-In RTP.conf you can enable it for the RTP subsystem. This will affect all RTP streams,
-including video and text if you have that. In most cases, it's propably not a good idea
-but for testing.
+SISO: Enable "donthidepacketloss" in rtp.conf.
Downside
========
This is a poor man's fix. Codecs that have built-in PLC, like G.729, can propably handle it
-internally better.
+internally better. In that case the "donthidepacketloss" patch works better.
-Incoming packets 1,2,4,5 will simply be outbound 1,2,3,4 and we will have a skew - but hide
-the fact that we have packet loss on the inbound stream. In order to handle this, we will need
-to send frames in the media stream that indicate packet loss and make sure codecs understand
-it and that it affects the outbound RTP stream as well.
+At some point the PLC should automatically be disabled for a set of codecs. If you want
+to fund that work, contact me directly.
-In my case, we only have G.711a calls, so the Poor Man's Solution works well.
+Credits
+=======
+A bit thank you to Martin Festr Vit (voipmonitor.org) who has spent a lot of time
+chatting with me about possible solutions - the donthidepacketloss mode is based
+on his ideas mostly.
Reading suggestion (with audio)
===============================
@@ -88,22 +108,3 @@
And finally a description of the current PLC in Asterisk
- https://wiki.asterisk.org/wiki/display/AST/Packet+Loss+Concealment+%28PLC%29
-
-From RTP.conf
-=============
-; The RTP channels has an implementation of Packet Loss Concealment
-; that is named "Poor Man's PLC". Normally a PLC happens withing the
-; context of a jitter buffer. This PLC just copies the previous
-; packet into the stream again if a packet (or multiple) is missing.
-; If a packet arrives too late (reordered) it will be ignored.
-; This introduces a bit of jitter since we're sending two
-; packets at the same time. Hopefully the phone or gw at the
-; end of the line will have a jitter buffer and play out the media
-; properly. This PLC will make sure that Asterisk outbound RTP streams has
-; less skew and that recordings will actually have a proper amount of
-; media. Default is turned off.
-; This setting apply to ALL rtp streams in this Asterisk instance.
-; If you have video or text streams, it might not be a good idea.
-;
-; You can turn this on per device or globally in sip.conf too.
-;plc=yes
Modified: team/group/pinefool-poor-mans-plc-1.4/main/rtp.c
URL: http://svnview.digium.com/svn/asterisk/team/group/pinefool-poor-mans-plc-1.4/main/rtp.c?view=diff&rev=384878&r1=384877&r2=384878
==============================================================================
--- team/group/pinefool-poor-mans-plc-1.4/main/rtp.c (original)
+++ team/group/pinefool-poor-mans-plc-1.4/main/rtp.c Mon Apr 8 07:24:41 2013
@@ -1304,6 +1304,7 @@
seqno &= 0xffff;
timestamp = ntohl(rtpheader[1]);
ssrc = ntohl(rtpheader[2]);
+ int not_jitter = 0;
AST_LIST_HEAD_INIT_NOLOCK(&frames);
/* Force a marker bit and change SSRC if the SSRC changes */
@@ -1363,7 +1364,6 @@
if (rtp->rxcount > 1) {
int MAX_MISORDER = 100;
- int not_jitter = 0;
/* RTP sequence numbers are consecutive. Have we lost a packet? */
lostpackets = (int) seqno - (int) rtp->lastrxseqno - 1;
@@ -1384,36 +1384,47 @@
we can't compare with the previous one. Just go ahead and wait for
the next packet
*/
- if (ast_test_flag(rtp, FLAG_POORMANSPLC) && seqno < rtp->lastrxseqno && not_jitter == 0) {
- /* This is a latecomer that we've already replaced. A jitter buffer would have handled this
- properly, but in many cases we can't afford a jitterbuffer and will have to live
- with the face that the poor man's PLC already has replaced this frame and we can't
- insert it AGAIN, because that would cause negative skew.
- Henry, just ignore this late visitor. Thank you.
- */
- if (rtp_debug_test_addr(&sin)) {
- ast_verbose("Ignoring RTP from %s:%u (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6u) - jitter?\n",
- ast_inet_ntoa(sin.sin_addr), ntohs(sin.sin_port), payloadtype, seqno, timestamp,res - hdrlen);
+ if (seqno < rtp->lastrxseqno && not_jitter == 0) {
+ if (ast_test_flag(rtp, FLAG_POORMANSPLC) && seqno < rtp->lastrxseqno && not_jitter == 0) {
+ /* This is a latecomer that we've already replaced. A jitter buffer would have handled this
+ properly, but in many cases we can't afford a jitterbuffer and will have to live
+ with the face that the poor man's PLC already has replaced this frame and we can't
+ insert it AGAIN, because that would cause negative skew.
+ Henry, just ignore this late visitor. Thank you.
+ */
+ if (rtp_debug_test_addr(&sin)) {
+ ast_verbose("Ignoring RTP from %s:%u (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6u) - jitter?\n",
+ ast_inet_ntoa(sin.sin_addr), ntohs(sin.sin_port), payloadtype, seqno, timestamp,res - hdrlen);
+ }
+ return AST_LIST_FIRST(&frames) ? AST_LIST_FIRST(&frames) : &ast_null_frame;
+ } else {
+ /* No poor mans PLC. Do we care about forwarding this? */
+ if (rtp_debug_test_addr(&sin)) {
+ ast_verbose("Got late RTP from %s:%u (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6u) - jitter?\n",
+ ast_inet_ntoa(sin.sin_addr), ntohs(sin.sin_port), payloadtype, seqno, timestamp,res - hdrlen);
+ }
}
- return AST_LIST_FIRST(&frames) ? AST_LIST_FIRST(&frames) : &ast_null_frame;
+
}
}
- if (lostpackets) {
+ if (lostpackets > 0) {
if(option_debug > 2) {
ast_log(LOG_DEBUG, "**** Packet loss detected - # %d. Current Seqno %-6.6u\n", lostpackets, seqno);
}
if (ast_test_flag(rtp, FLAG_POORMANSPLC) && rtp->plcbuf != NULL) {
int i;
for (i = 0; i < lostpackets && i < plcmax; i++) {
+ struct ast_frame *new;
rtp->lastrxseqno++;
/* Fix the seqno in the frame */
rtp->plcbuf->seqno = rtp->lastrxseqno;
rtp->plcbuf->ts += rtp->plcbuf->len;
-
- AST_LIST_INSERT_TAIL(&frames, ast_frdup(rtp->plcbuf), frame_list);
+ new = ast_frdup(rtp->plcbuf);
+
+ AST_LIST_INSERT_TAIL(&frames, new, frame_list);
if (option_debug > 2) {
- ast_log(LOG_DEBUG, "**** Inserting buffer frame %d. \n", i + 1);
+ ast_log(LOG_DEBUG, "**** Inserting buffer frame %d. Seqno %d\n", i + 1, rtp->plcbuf->seqno);
}
}
} else {
@@ -1424,8 +1435,11 @@
}
}
- rtp->lastrxseqno = seqno;
-
+ /* Only change lastrxseqno if it's bigger or if we have a large gap or marker bit */
+ if (mark || not_jitter || (rtp->lastrxseqno < seqno )) {
+ rtp->lastrxseqno = seqno;
+ }
+
if (rtp->themssrc==0)
rtp->themssrc = ntohl(rtpheader[2]); /* Record their SSRC to put in future RR */
@@ -1543,7 +1557,6 @@
ast_log(LOG_DEBUG, " *** Not copying frame into plcbuf PLC=%s\n", ast_test_flag(rtp, FLAG_POORMANSPLC) ? "On" : "Off");
}
}
-
AST_LIST_INSERT_TAIL(&frames, &rtp->f, frame_list);
return AST_LIST_FIRST(&frames);
}
@@ -2890,6 +2903,7 @@
int pred;
int mark = 0;
int rate = rtp_get_rate(f->subclass) / 1000;
+ int send_seqno = rtp->seqno;
if (f->subclass == AST_FORMAT_G722) {
/* G722 is silllllllllllllllllly */
@@ -2903,12 +2917,23 @@
if (donthidepacketloss && rtp->prev_frame_seqno > 0 && f->seqno && f->seqno != (rtp->prev_frame_seqno + 1)) {
/* We have incoming packet loss and need to signal that outbound. */
unsigned int loss = f->seqno - rtp->prev_frame_seqno - 1;
+
if (f->seqno > rtp->prev_frame_seqno) {
- if (option_debug > 2) {
- ast_log(LOG_DEBUG, "**** Incoming packet loss, letting it through: %u packets\n", loss);
- }
/* Jump ahead, let the packet loss go straight through */
rtp->seqno += loss;
+ send_seqno += loss;
+ if (option_debug > 2) {
+ ast_log(LOG_DEBUG, "**** Incoming packet loss, letting it through: %d packets (Seqno %d Prev seqno %d)\n", loss, f->seqno, rtp->prev_frame_seqno);
+ }
+ }
+ if (f->seqno && f->seqno < rtp->prev_frame_seqno) {
+ /* We have a latecomer. Forward it as is but keep the seqno */
+ int diff = rtp->prev_frame_seqno - f->seqno;
+ send_seqno = rtp->seqno - diff - 1;
+ /* Keep RTP-seqno */
+ if (option_debug > 2) {
+ ast_log(LOG_DEBUG, "**** Incoming reordering, letting it through: %d packets (Seqno %d Prev seqno %d)\n", loss, f->seqno, rtp->prev_frame_seqno);
+ }
}
}
@@ -2966,7 +2991,7 @@
/* Get a pointer to the header */
rtpheader = (unsigned char *)(f->data - hdrlen);
- put_unaligned_uint32(rtpheader, htonl((2 << 30) | (codec << 16) | (rtp->seqno) | (mark << 23)));
+ put_unaligned_uint32(rtpheader, htonl((2 << 30) | (codec << 16) | (send_seqno) | (mark << 23)));
put_unaligned_uint32(rtpheader + 4, htonl(rtp->lastts));
put_unaligned_uint32(rtpheader + 8, htonl(rtp->ssrc));
@@ -2974,7 +2999,7 @@
res = sendto(rtp->s, (void *)rtpheader, f->datalen + hdrlen, 0, (struct sockaddr *)&rtp->them, sizeof(rtp->them));
if (res <0) {
if (!rtp->nat || (rtp->nat && (ast_test_flag(rtp, FLAG_NAT_ACTIVE) == FLAG_NAT_ACTIVE))) {
- ast_log(LOG_DEBUG, "RTP Transmission error of packet %d to %s:%d: %s\n", rtp->seqno, ast_inet_ntoa(rtp->them.sin_addr), ntohs(rtp->them.sin_port), strerror(errno));
+ ast_log(LOG_DEBUG, "RTP Transmission error of packet %d to %s:%d: %s\n", send_seqno, ast_inet_ntoa(rtp->them.sin_addr), ntohs(rtp->them.sin_port), strerror(errno));
} else if (((ast_test_flag(rtp, FLAG_NAT_ACTIVE) == FLAG_NAT_INACTIVE) || rtpdebug) && !ast_test_flag(rtp, FLAG_NAT_INACTIVE_NOWARN)) {
/* Only give this error message once if we are not RTP debugging */
if (option_debug || rtpdebug)
@@ -2992,11 +3017,19 @@
}
if (rtp_debug_test_addr(&rtp->them))
- ast_verbose("Sent RTP packet to %s:%u (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6u)\n",
- ast_inet_ntoa(rtp->them.sin_addr), ntohs(rtp->them.sin_port), codec, rtp->seqno, rtp->lastts,res - hdrlen);
- }
-
- increment_seqno(rtp, f);
+ ast_verbose("Sent RTP packet to %s:%u (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6u) Seq-in %-6.6u\n",
+ ast_inet_ntoa(rtp->them.sin_addr), ntohs(rtp->them.sin_port), codec, send_seqno, rtp->lastts,res - hdrlen, f->seqno);
+ }
+
+ if (send_seqno == rtp->seqno) {
+ /* Do not update if we are sending a late packet (not current seqno) to preserve jitter reordering across
+ the bridge */
+ increment_seqno(rtp, f);
+ } else {
+ if (option_debug > 2) {
+ ast_log(LOG_DEBUG, "**** Not updating seqno -sending late frame. Send_seqno %d RTP seqno %d \n", send_seqno, rtp->seqno);
+ }
+ }
return 0;
}
@@ -3139,8 +3172,10 @@
if (f->data) {
ast_rtp_raw_write(rtp, f, codec);
}
- if (f != _f)
+ if (f != _f) {
ast_frfree(f);
+ }
+
}
return 0;
More information about the asterisk-commits
mailing list