Review Board 1.7.16


Fix transcode_via_sln and increase utility of PLC

Review Request #622 - Created April 22, 2010 and submitted

Mark Michelson
/trunk
Reviewers
asterisk-dev
Asterisk
The problem here is a bit complex, so try to bear with me...

It was noticed by a Digium customer that generic PLC (as configured in
codecs.conf) did not appear to actually be having any sort of benefit when
packet loss was introduced on an RTP stream. I reproduced this issue myself
by streaming a file across an RTP stream and dropping approx. 5% of the
RTP packets. I saw no real difference between when PLC was enabled or disabled
when using wireshark to analyze the RTP streams.

After analyzing what was going on, it became clear that one of the problems
faced was that when running my tests, the translation paths were being set
up in such a way that PLC could not possibly work as expected. To illustrate,
if packets are lost on channel A's read stream, then we expect that PLC will
be applied to channel B's write stream. The problem is that generic PLC can
only be done when there is a translation path that moves from some codec to
SLINEAR. When I would run my tests, I found that every single time, read
and write translation paths would be set up on channel A instead of channel
B. There appeared to be no real way to predict which channel the translation
paths would be set up on.

This is where Kevin swooped in to let me know about the transcode_via_sln
option in asterisk.conf. It is supposed to work by placing a read translation
path on both channels from the channel's rawreadformat to SLINEAR. It also
will place a write translation path on both channels from SLINEAR to the
channel's rawwriteformat. Using this option allows one to predictably set up
translation paths on all channels. There are two problems with this, though.
First and foremost, the transcode_via_sln option did not appear to be working
properly when I was placing a SIP call between two endpoints which did not
share any common formats. Second, even if this option were to work, for PLC
to be applied, there had to be a write translation path that would go from
some format to SLINEAR. It would not work properly if the starting format
of translation was SLINEAR.

The one-line change presented in this review request in chan_sip.c fixed the
first issue for me. The problem was that in sip_request_call, the
jointcapability of the outbound channel was being set to the format passed to
sip_request_call. This is nativeformats of the inbound channel. Because of this,
when ast_channel_make_compatible was called by app_dial, both channels already
had compatibly read and write formats. Thus, no translation path was set up at
the time. My change is to set the jointcapability of the sip_pvt created during
sip_request_call to the intersection of the inbound channel's nativeformats and
the configured peer capability that we determined during the earlier call to
create_addr. Doing this got the translation paths set up as expected when using
transcode_via_sln.

The changes presented in channel.c fixed the second issue for me. First and
foremost, when Asterisk is started, we'll read codecs.conf to see the value of
the genericplc option. If this option is set, and ast_write is called for a
frame with no data, then we will attempt to fill in the missing samples for
the frame. The implementation uses a channel datastore for maintaining the
PLC state and for creating a buffer to store PLC samples in. Even when we
receive a frame with data, we'll call plc_rx so that the PLC state will have
knowledge of the previous voice frame, which it can use as a basis for when
it comes time to actually do a PLC fill-in.

So, reviewers, now I ask for your help. First off, there's the one line change
in chan_sip that I have put in. Is it right? By my logic it seems correct, but
I'm sure someone can tell me why it is not going to work. This is probably the
change I'm least concerned about, though. What concerns me much more is the
set of changes in channel.c. First off, am I even doing it right? When I run
tests, I can clearly see that when PLC is activated, I see a significant increase
in RTP traffic where I would expect it to be. However, in my humble opinion, the
audio sounds kind of crappy whenever the PLC fill-in is done. It sounds worse to
me than when no PLC is used at all. I need someone to review the logic I have used
to be sure that I'm not misusing anything. As far as I can see my pointer arithmetic
is correct, and my use of AST_FRIENDLY_OFFSET should be correct as well, but I'm
sure someone can point out somewhere where I've done something incorrectly.

As I was writing this review request up, I decided to give the code a test run under
valgrind, and I find that for some reason, calls to plc_rx are causing some invalid
reads. Apparently I'm reading past the end of a buffer somehow. I'll have to dig around
a bit to see why that is the case. If it's obvious to someone reviewing, speak up!

Finally, I have one other proposal that is not reflected in my code review. Since
without transcode_via_sln set, one cannot predict or control where a translation
path will be up, it seems to me that the current practice of using PLC only when
transcoding to SLINEAR is not useful. I recommend that once it has been determined
that the method used in this code review is correct and works as expected, then
the code in translate.c that invokes PLC should be removed.
I ran a test where I would place a call from my Polycom phone through one Asterisk box
to a second one. The second box would play back the demo-congrats file. I modified the
RTP code in the second box to drop approximately 5 percent of the outgoing packets. The
audio between the two Asterisk boxes was GSM. The audio between the Polycom phone and
Asterisk was ulaw.

I used wireshark on the first Asterisk box to analyze the RTP streams. I noticed an increase
from an average of 2650 packets on the path from the first Asterisk box to the Polycom to
an average of 2786 packets when PLC was enabled.

Changes between revision 2 and 3

1 2 3 4 5
1 2 3 4 5

  1. /trunk/channels/chan_sip.c: Loading...
  2. /trunk/main/channel.c: Loading...
/trunk/channels/chan_sip.c
Diff Revision 2 Diff Revision 3
[20] 7989 lines
[+20] [+] static int process_sdp(struct sip_pvt *p, struct sip_request *req, int t38action)
7990
			tsin.sin_family = AF_INET;
7990
			tsin.sin_family = AF_INET;
7991
			tsin.sin_port = htons(tportno);
7991
			tsin.sin_port = htons(tportno);
7992
			memcpy(&tsin.sin_addr, thp->h_addr, sizeof(tsin.sin_addr));
7992
			memcpy(&tsin.sin_addr, thp->h_addr, sizeof(tsin.sin_addr));
7993
			ast_rtp_instance_set_remote_address(p->trtp, &tsin);
7993
			ast_rtp_instance_set_remote_address(p->trtp, &tsin);
7994
			if (debug) 
7994
			if (debug) 
7995
				ast_verbose("Peer T.140 RTP is at port %s:%d\n", ast_inet_ntoa(vsin.sin_addr), ntohs(vsin.sin_port));
7995
				ast_verbose("Peer T.140 RTP is at port %s:%d\n", ast_inet_ntoa(tsin.sin_addr), ntohs(tsin.sin_port));
7996
			if ((p->jointcapability & AST_FORMAT_T140RED)) {
7996
			if ((p->jointcapability & AST_FORMAT_T140RED)) {
7997
				p->red = 1;
7997
				p->red = 1;
7998
				ast_rtp_red_init(p->trtp, 300, red_data_pt, 2);
7998
				ast_rtp_red_init(p->trtp, 300, red_data_pt, 2);
7999
			} else {
7999
			} else {
8000
				p->red = 0;
8000
				p->red = 0;
[+20] [20] 18 lines
[+20] static int process_sdp(struct sip_pvt *p, struct sip_request *req, int t38action)
8019
					if (debug) {
8019
					if (debug) {
8020
						ast_log(LOG_DEBUG, "Peer T.38 UDPTL is set behind NAT and with destination, destination address now %s\n", ast_inet_ntoa(isin.sin_addr));
8020
						ast_log(LOG_DEBUG, "Peer T.38 UDPTL is set behind NAT and with destination, destination address now %s\n", ast_inet_ntoa(isin.sin_addr));
8021
					}
8021
					}
8022
				}
8022
				}
8023
			} else {
8023
			} else {
8024
				memcpy(&isin.sin_addr, ihp->h_addr, sizeof(sin.sin_addr));
8024
				memcpy(&isin.sin_addr, ihp->h_addr, sizeof(isin.sin_addr));
8025
			}
8025
			}
8026
			ast_udptl_set_peer(p->udptl, &isin);
8026
			ast_udptl_set_peer(p->udptl, &isin);
8027
			if (debug)
8027
			if (debug)
8028
				ast_debug(1,"Peer T.38 UDPTL is at port %s:%d\n", ast_inet_ntoa(isin.sin_addr), ntohs(isin.sin_port));
8028
				ast_debug(1,"Peer T.38 UDPTL is at port %s:%d\n", ast_inet_ntoa(isin.sin_addr), ntohs(isin.sin_port));
8029

    
   
8029

   
[+20] [20] 2486 lines
[+20] [+] static void initreqprep(struct sip_request *req, struct sip_pvt *p, int sipmethod, const char * const explicit_uri)
10516
 			snprintf(to, sizeof(to), "<sip:%s>%s%s", p->todnid, ast_strlen_zero(p->theirtag) ? "" : ";tag=", p->theirtag);
10516
 			snprintf(to, sizeof(to), "<sip:%s>%s%s", p->todnid, ast_strlen_zero(p->theirtag) ? "" : ";tag=", p->theirtag);
10517
 		}
10517
 		}
10518
 	} else {
10518
 	} else {
10519
 		if (sipmethod == SIP_NOTIFY && !ast_strlen_zero(p->theirtag)) {
10519
 		if (sipmethod == SIP_NOTIFY && !ast_strlen_zero(p->theirtag)) {
10520
 			/* If this is a NOTIFY, use the From: tag in the subscribe (RFC 3265) */
10520
 			/* If this is a NOTIFY, use the From: tag in the subscribe (RFC 3265) */
10521
			snprintf(to, sizeof(to), "<%s%s>;tag=%s", (!strncasecmp(p->uri, "sip:", 4) ? "sip:" : ""), p->uri, p->theirtag);
10521
			snprintf(to, sizeof(to), "<%s%s>;tag=%s", (strncasecmp(p->uri, "sip:", 4) ? "sip:" : ""), p->uri, p->theirtag);
10522
 		} else if (p->options && p->options->vxml_url) {
10522
 		} else if (p->options && p->options->vxml_url) {
10523
 			/* If there is a VXML URL append it to the SIP URL */
10523
 			/* If there is a VXML URL append it to the SIP URL */
10524
 			snprintf(to, sizeof(to), "<%s>;%s", p->uri, p->options->vxml_url);
10524
 			snprintf(to, sizeof(to), "<%s>;%s", p->uri, p->options->vxml_url);
10525
 		} else
10525
 		} else
10526
 			snprintf(to, sizeof(to), "<%s>", p->uri);
10526
 			snprintf(to, sizeof(to), "<%s>", p->uri);
[+20] [20] 2979 lines
[+20] [+] static int get_destination(struct sip_pvt *p, struct sip_request *oreq, int *cc_recall_core_id)
13506
			if (!sip_cfg.allow_external_domains && (req->method == SIP_INVITE || req->method == SIP_REFER)) {
13506
			if (!sip_cfg.allow_external_domains && (req->method == SIP_INVITE || req->method == SIP_REFER)) {
13507
				ast_debug(1, "Got SIP %s to non-local domain '%s'; refusing request.\n", sip_methods[req->method].text, p->domain);
13507
				ast_debug(1, "Got SIP %s to non-local domain '%s'; refusing request.\n", sip_methods[req->method].text, p->domain);
13508
				return -2;
13508
				return -2;
13509
			}
13509
			}
13510
		}
13510
		}
13511
		/* If we have a context defined, overwrite the original context */
13511
		/* If we don't have a peer (i.e. we're a guest call),
13512
		if (!ast_strlen_zero(domain_context))
13512
		 * overwrite the original context */

    
   
13513
		if (!ast_test_flag(&p->flags[1], SIP_PAGE2_HAVEPEERCONTEXT) && !ast_strlen_zero(domain_context))
13513
			ast_string_field_set(p, context, domain_context);
13514
			ast_string_field_set(p, context, domain_context);
13514
	}
13515
	}
13515

    
   
13516

   
13516
	/* If the request coming in is a subscription and subscribecontext has been specified use it */
13517
	/* If the request coming in is a subscription and subscribecontext has been specified use it */
13517
	if (req->method == SIP_SUBSCRIBE && !ast_strlen_zero(p->subscribecontext))
13518
	if (req->method == SIP_SUBSCRIBE && !ast_strlen_zero(p->subscribecontext))
[+20] [20] 216 lines
[+20] [+] static int get_refer_info(struct sip_pvt *transferer, struct sip_request *outgoing_req)
13734
		char *to = NULL, *from = NULL;
13735
		char *to = NULL, *from = NULL;
13735
		
13736
		
13736
		/* This is an attended transfer */
13737
		/* This is an attended transfer */
13737
		referdata->attendedtransfer = 1;
13738
		referdata->attendedtransfer = 1;
13738
		ast_copy_string(referdata->replaces_callid, ptr+9, sizeof(referdata->replaces_callid));
13739
		ast_copy_string(referdata->replaces_callid, ptr+9, sizeof(referdata->replaces_callid));

    
   
13740
		ast_uri_decode(referdata->replaces_callid);
13739
		if ((ptr = strchr(referdata->replaces_callid, ';'))) 	/* Find options */ {
13741
		if ((ptr = strchr(referdata->replaces_callid, ';'))) 	/* Find options */ {
13740
			*ptr++ = '\0';
13742
			*ptr++ = '\0';
13741
		}
13743
		}
13742
		
13744
		
13743
		if (ptr) {
13745
		if (ptr) {
[+20] [20] 7 lines
[+20] static int get_refer_info(struct sip_pvt *transferer, struct sip_request *outgoing_req)
13751
			ptr = to + 7;
13753
			ptr = to + 7;
13752
			if ((to = strchr(ptr, '&')))
13754
			if ((to = strchr(ptr, '&')))
13753
				*to = '\0';
13755
				*to = '\0';
13754
			if ((to = strchr(ptr, ';')))
13756
			if ((to = strchr(ptr, ';')))
13755
				*to = '\0';
13757
				*to = '\0';
13756
			ast_uri_decode(ptr);

   
13757
			ast_copy_string(referdata->replaces_callid_totag, ptr, sizeof(referdata->replaces_callid_totag));
13758
			ast_copy_string(referdata->replaces_callid_totag, ptr, sizeof(referdata->replaces_callid_totag));
13758
		}
13759
		}
13759
		
13760
		
13760
		if (from) {
13761
		if (from) {
13761
			ptr = from + 9;
13762
			ptr = from + 9;
13762
			if ((to = strchr(ptr, '&')))
13763
			if ((to = strchr(ptr, '&')))
13763
				*to = '\0';
13764
				*to = '\0';
13764
			if ((to = strchr(ptr, ';')))
13765
			if ((to = strchr(ptr, ';')))
13765
				*to = '\0';
13766
				*to = '\0';
13766
			ast_uri_decode(ptr);

   
13767
			ast_copy_string(referdata->replaces_callid_fromtag, ptr, sizeof(referdata->replaces_callid_fromtag));
13767
			ast_copy_string(referdata->replaces_callid_fromtag, ptr, sizeof(referdata->replaces_callid_fromtag));
13768
		}
13768
		}
13769
		
13769
		
13770
		if (!sip_cfg.pedanticsipchecking)
13770
		if (!sip_cfg.pedanticsipchecking)
13771
			ast_debug(2, "Attended transfer: Will use Replace-Call-ID : %s (No check of from/to tags)\n", referdata->replaces_callid );
13771
			ast_debug(2, "Attended transfer: Will use Replace-Call-ID : %s (No check of from/to tags)\n", referdata->replaces_callid );
[+20] [20] 11171 lines
[+20] [+] static struct sip_peer *build_peer(const char *name, struct ast_variable *v, struct ast_variable *alt, int realtime, int devstate_only)
24943
				ast_string_field_set(peer, cid_name, "");
24943
				ast_string_field_set(peer, cid_name, "");
24944
			} else if (!strcasecmp(v->name, "cid_number")) {
24944
			} else if (!strcasecmp(v->name, "cid_number")) {
24945
				ast_string_field_set(peer, cid_num, v->value);
24945
				ast_string_field_set(peer, cid_num, v->value);
24946
			} else if (!strcasecmp(v->name, "context")) {
24946
			} else if (!strcasecmp(v->name, "context")) {
24947
				ast_string_field_set(peer, context, v->value);
24947
				ast_string_field_set(peer, context, v->value);

    
   
24948
				ast_set_flag(&peer->flags[1], SIP_PAGE2_HAVEPEERCONTEXT);
24948
			} else if (!strcasecmp(v->name, "subscribecontext")) {
24949
			} else if (!strcasecmp(v->name, "subscribecontext")) {
24949
				ast_string_field_set(peer, subscribecontext, v->value);
24950
				ast_string_field_set(peer, subscribecontext, v->value);
24950
			} else if (!strcasecmp(v->name, "fromdomain")) {
24951
			} else if (!strcasecmp(v->name, "fromdomain")) {
24951
				ast_string_field_set(peer, fromdomain, v->value);
24952
				ast_string_field_set(peer, fromdomain, v->value);
24952
			} else if (!strcasecmp(v->name, "usereqphone")) {
24953
			} else if (!strcasecmp(v->name, "usereqphone")) {
[+20] [20] 40 lines
[+20] static struct sip_peer *build_peer(const char *name, struct ast_variable *v, struct ast_variable *alt, int realtime, int devstate_only)
24993
							ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
24994
							ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
24994
						}
24995
						}
24995
					}
24996
					}
24996
				}
24997
				}
24997
			} else if (!strcasecmp(v->name, "defaultip")) {
24998
			} else if (!strcasecmp(v->name, "defaultip")) {
24998
				if (ast_get_ip(&peer->defaddr, v->value)) {
24999
				if (!ast_strlen_zero(v->value) && ast_get_ip(&peer->defaddr, v->value)) {
24999
					unref_peer(peer, "unref_peer: from build_peer defaultip");
25000
					unref_peer(peer, "unref_peer: from build_peer defaultip");
25000
					return NULL;
25001
					return NULL;
25001
				}
25002
				}
25002
			} else if (!strcasecmp(v->name, "permit") || !strcasecmp(v->name, "deny")) {
25003
			} else if (!strcasecmp(v->name, "permit") || !strcasecmp(v->name, "deny")) {
25003
				int ha_error = 0;
25004
				int ha_error = 0;

    
   
25005
				if (!ast_strlen_zero(v->value)) {
25004
				peer->ha = ast_append_ha(v->name, v->value, peer->ha, &ha_error);
25006
					peer->ha = ast_append_ha(v->name, v->value, peer->ha, &ha_error);

    
   
25007
				}
25005
				if (ha_error) {
25008
				if (ha_error) {
25006
					ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
25009
					ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
25007
				}
25010
				}
25008
			} else if (!strcasecmp(v->name, "contactpermit") || !strcasecmp(v->name, "contactdeny")) {
25011
			} else if (!strcasecmp(v->name, "contactpermit") || !strcasecmp(v->name, "contactdeny")) {
25009
				int ha_error = 0;
25012
				int ha_error = 0;

    
   
25013
				if (!ast_strlen_zero(v->value)) {
25010
				peer->contactha = ast_append_ha(v->name + 7, v->value, peer->contactha, &ha_error);
25014
					peer->contactha = ast_append_ha(v->name + 7, v->value, peer->contactha, &ha_error);

    
   
25015
				}
25011
				if (ha_error) {
25016
				if (ha_error) {
25012
					ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
25017
					ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
25013
				}
25018
				}
25014
			} else if (!strcasecmp(v->name, "port")) {
25019
			} else if (!strcasecmp(v->name, "port")) {
25015
				peer->portinuri = 1;
25020
				peer->portinuri = 1;
[+20] [20] 2286 lines
/trunk/main/channel.c
Diff Revision 2 Diff Revision 3
 
  1. /trunk/channels/chan_sip.c: Loading...
  2. /trunk/main/channel.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.