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.
/trunk/channels/chan_sip.c
Diff Revision 4 Diff Revision 5
[20] 17532 lines
[+20] [+] static void change_redirecting_information(struct sip_pvt *p, struct sip_request *req, struct ast_party_redirecting *redirecting, int set_call_forward)
17533
			ast_free(redirecting->from.name);
17533
			ast_free(redirecting->from.name);
17534
		}
17534
		}
17535
		ast_debug(3, "Got redirecting from name %s\n", redirecting_from_name);
17535
		ast_debug(3, "Got redirecting from name %s\n", redirecting_from_name);
17536
		redirecting->from.name = redirecting_from_name;
17536
		redirecting->from.name = redirecting_from_name;
17537
	}
17537
	}
17538
	redirecting->from.tag = (char *) p->cid_tag;
17538
	if (!ast_strlen_zero(p->cid_tag)) {

    
   
17539
		if (redirecting->from.tag) {

    
   
17540
			ast_free(redirecting->from.tag);

    
   
17541
		}

    
   
17542
		redirecting->from.tag = ast_strdup(p->cid_tag);

    
   
17543
		if (redirecting->to.tag) {

    
   
17544
			ast_free(redirecting->to.tag);

    
   
17545
		}

    
   
17546
		redirecting->to.tag = ast_strdup(p->cid_tag);

    
   
17547
	}
17539
	if (!ast_strlen_zero(redirecting_to_number)) {
17548
	if (!ast_strlen_zero(redirecting_to_number)) {
17540
		if (redirecting->to.number) {
17549
		if (redirecting->to.number) {
17541
			ast_free(redirecting->to.number);
17550
			ast_free(redirecting->to.number);
17542
		}
17551
		}
17543
		ast_debug(3, "Got redirecting to number %s\n", redirecting_to_number);
17552
		ast_debug(3, "Got redirecting to number %s\n", redirecting_to_number);
[+20] [20] 4 lines
[+20] static void change_redirecting_information(struct sip_pvt *p, struct sip_request *req, struct ast_party_redirecting *redirecting, int set_call_forward)
17548
			ast_free(redirecting->to.name);
17557
			ast_free(redirecting->to.name);
17549
		}
17558
		}
17550
		ast_debug(3, "Got redirecting to name %s\n", redirecting_from_number);
17559
		ast_debug(3, "Got redirecting to name %s\n", redirecting_from_number);
17551
		redirecting->to.name = redirecting_to_name;
17560
		redirecting->to.name = redirecting_to_name;
17552
	}
17561
	}
17553
	redirecting->to.tag = (char *) p->cid_tag;

   
17554
	redirecting->reason = reason;
17562
	redirecting->reason = reason;
17555
}
17563
}
17556

    
   
17564

   
17557
/*! \brief Parse 302 Moved temporalily response
17565
/*! \brief Parse 302 Moved temporalily response
17558
	\todo XXX Doesn't redirect over TLS on sips: uri's.
17566
	\todo XXX Doesn't redirect over TLS on sips: uri's.
[+20] [20] 368 lines
[+20] [+] static void handle_response_invite(struct sip_pvt *p, int resp, const char *rest, struct sip_request *req, int seqno)
17927
			ast_log(LOG_WARNING, "Unable to cancel SIP destruction.  Expect bad things.\n");
17935
			ast_log(LOG_WARNING, "Unable to cancel SIP destruction.  Expect bad things.\n");
17928
		if (!req->ignore && p->owner) {
17936
		if (!req->ignore && p->owner) {
17929
			struct ast_party_redirecting redirecting = {{0,},};
17937
			struct ast_party_redirecting redirecting = {{0,},};
17930
			change_redirecting_information(p, req, &redirecting, FALSE);
17938
			change_redirecting_information(p, req, &redirecting, FALSE);
17931
			ast_channel_queue_redirecting_update(p->owner, &redirecting);
17939
			ast_channel_queue_redirecting_update(p->owner, &redirecting);

    
   
17940
			ast_party_redirecting_free(&redirecting);
17932
			sip_handle_cc(p, req, AST_CC_CCNR);
17941
			sip_handle_cc(p, req, AST_CC_CCNR);
17933
		}
17942
		}
17934
		check_pendings(p);
17943
		check_pendings(p);
17935
		break;
17944
		break;
17936

    
   
17945

   
[+20] [20] 963 lines
[+20] [+] static void handle_response(struct sip_pvt *p, int resp, const char *rest, struct sip_request *req, int seqno)
18900
				case 305: /* Use Proxy */
18909
				case 305: /* Use Proxy */
18901
				if (p->owner) {
18910
				if (p->owner) {
18902
					struct ast_party_redirecting redirecting = {{0,},};
18911
					struct ast_party_redirecting redirecting = {{0,},};
18903
					change_redirecting_information(p, req, &redirecting, TRUE);
18912
					change_redirecting_information(p, req, &redirecting, TRUE);
18904
					ast_channel_set_redirecting(p->owner, &redirecting);
18913
					ast_channel_set_redirecting(p->owner, &redirecting);

    
   
18914
					ast_party_redirecting_free(&redirecting);
18905
				}
18915
				}
18906
					/* Fall through */
18916
					/* Fall through */
18907
				case 486: /* Busy here */
18917
				case 486: /* Busy here */
18908
				case 600: /* Busy everywhere */
18918
				case 600: /* Busy everywhere */
18909
				case 603: /* Decline */
18919
				case 603: /* Decline */
[+20] [20] 1578 lines
[+20] [+] static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, int debug, int seqno, struct sockaddr_in *sin, int *recount, const char *e, int *nounlock)
20488
		ast_verbose("Ignoring this INVITE request\n");
20498
		ast_verbose("Ignoring this INVITE request\n");
20489

    
   
20499

   
20490
	if (!p->lastinvite && !req->ignore && !p->owner) {
20500
	if (!p->lastinvite && !req->ignore && !p->owner) {
20491
		/* This is a new invite */
20501
		/* This is a new invite */
20492
		/* Handle authentication if this is our first invite */
20502
		/* Handle authentication if this is our first invite */
20493
		struct ast_party_redirecting redirecting = {{0,},};

   
20494
		int cc_recall_core_id = -1;
20503
		int cc_recall_core_id = -1;
20495
		set_pvt_allowed_methods(p, req);
20504
		set_pvt_allowed_methods(p, req);
20496
		res = check_user(p, req, SIP_INVITE, e, XMIT_RELIABLE, sin);
20505
		res = check_user(p, req, SIP_INVITE, e, XMIT_RELIABLE, sin);
20497
		if (res == AUTH_CHALLENGE_SENT) {
20506
		if (res == AUTH_CHALLENGE_SENT) {
20498
			p->invitestate = INV_COMPLETED;		/* Needs to restart in another INVITE transaction */
20507
			p->invitestate = INV_COMPLETED;		/* Needs to restart in another INVITE transaction */
[+20] [20] 59 lines
[+20] static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, int debug, int seqno, struct sockaddr_in *sin, int *recount, const char *e, int *nounlock)
20558
			}
20567
			}
20559
			res = 0;
20568
			res = 0;
20560
			goto request_invite_cleanup;
20569
			goto request_invite_cleanup;
20561
		}
20570
		}
20562
		gotdest = get_destination(p, NULL, &cc_recall_core_id);	/* Get destination right away */
20571
		gotdest = get_destination(p, NULL, &cc_recall_core_id);	/* Get destination right away */
20563
		change_redirecting_information(p, req, &redirecting, FALSE); /*Will return immediately if no Diversion header is present */

   
20564
		extract_uri(p, req);			/* Get the Contact URI */
20572
		extract_uri(p, req);			/* Get the Contact URI */
20565
		build_contact(p);			/* Build our contact header */
20573
		build_contact(p);			/* Build our contact header */
20566

    
   
20574

   
20567
		if (p->rtp) {
20575
		if (p->rtp) {
20568
			ast_rtp_instance_set_prop(p->rtp, AST_RTP_PROPERTY_DTMF, ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_RFC2833);
20576
			ast_rtp_instance_set_prop(p->rtp, AST_RTP_PROPERTY_DTMF, ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_RFC2833);
[+20] [20] 36 lines
[+20] static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, int debug, int seqno, struct sockaddr_in *sin, int *recount, const char *e, int *nounlock)
20605

    
   
20613

   
20606
			/* Save Record-Route for any later requests we make on this dialogue */
20614
			/* Save Record-Route for any later requests we make on this dialogue */
20607
			build_route(p, req, 0);
20615
			build_route(p, req, 0);
20608

    
   
20616

   
20609
			if (c) {
20617
			if (c) {

    
   
20618
				struct ast_party_redirecting redirecting = { { 0, }, };
20610
				/* Pre-lock the call */
20619
				/* Pre-lock the call */
20611
				ast_channel_lock(c);
20620
				ast_channel_lock(c);

    
   
20621
				change_redirecting_information(p, req, &redirecting, FALSE); /*Will return immediately if no Diversion header is present */
20612
				ast_channel_set_redirecting(c, &redirecting);
20622
				ast_channel_set_redirecting(c, &redirecting);

    
   
20623
				ast_party_redirecting_free(&redirecting);
20613
			}
20624
			}
20614
		}
20625
		}
20615
	} else {
20626
	} else {
20616
		struct ast_party_redirecting redirecting = {{0,},};
20627
		struct ast_party_redirecting redirecting = {{0,},};
20617
		if (sipdebug) {
20628
		if (sipdebug) {
[+20] [20] 7 lines
[+20] static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, int debug, int seqno, struct sockaddr_in *sin, int *recount, const char *e, int *nounlock)
20625
		c = p->owner;
20636
		c = p->owner;
20626
		change_redirecting_information(p, req, &redirecting, FALSE); /*Will return immediately if no Diversion header is present */
20637
		change_redirecting_information(p, req, &redirecting, FALSE); /*Will return immediately if no Diversion header is present */
20627
		if (c) {
20638
		if (c) {
20628
			ast_channel_set_redirecting(c, &redirecting);
20639
			ast_channel_set_redirecting(c, &redirecting);
20629
		}
20640
		}

    
   
20641
		ast_party_redirecting_free(&redirecting);
20630
	}
20642
	}
20631

    
   
20643

   
20632
	/* Session-Timers */
20644
	/* Session-Timers */
20633
	if (p->sipoptions & SIP_OPT_TIMER) {
20645
	if (p->sipoptions & SIP_OPT_TIMER) {
20634
		/* The UAC has requested session-timers for this session. Negotiate
20646
		/* The UAC has requested session-timers for this session. Negotiate
[+20] [20] 6699 lines
/trunk/include/asterisk/_private.h
Diff Revision 4 Diff Revision 5
 
/trunk/include/asterisk/channel.h
Diff Revision 4 Diff Revision 5 - File Reverted
 
/trunk/include/asterisk/options.h
Diff Revision 4 Diff Revision 5
 
/trunk/main/asterisk.c
Diff Revision 4 Diff Revision 5
 
/trunk/main/channel.c
Diff Revision 4 Diff Revision 5
 
/trunk/main/loader.c
Diff Revision 4 Diff Revision 5
 
  1. /trunk/channels/chan_sip.c: Loading...
  2. /trunk/include/asterisk/_private.h: Loading...
  3. /trunk/include/asterisk/channel.h: Loading...
  4. /trunk/include/asterisk/options.h: Loading...
  5. /trunk/main/asterisk.c: Loading...
  6. /trunk/main/channel.c: Loading...
  7. /trunk/main/loader.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.