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 1 and 2

1 2 3 4 5
1 2 3 4 5

  1. /trunk/main/channel.c: Loading...
/trunk/main/channel.c
Diff Revision 1 Diff Revision 2
[20] 3931 lines
[+20] [+] int ast_write_video(struct ast_channel *chan, struct ast_frame *fr)
3932
	/* A buffer in which to store SLIN PLC
3932
	/* A buffer in which to store SLIN PLC
3933
	 * samples generated by the generic PLC
3933
	 * samples generated by the generic PLC
3934
	 * functionality in plc.c
3934
	 * functionality in plc.c
3935
	 */
3935
	 */
3936
	int16_t *samples_buf;
3936
	int16_t *samples_buf;
3937
	/* The current size of the samples_buf. */
3937
	/* The current number of samples in the
3938
	size_t samples_buf_size;
3938
	 * samples_buf

    
   
3939
	 */

    
   
3940
	size_t num_samples;
3939
	plc_state_t plc_state;
3941
	plc_state_t plc_state;
3940
};
3942
};
3941

    
   
3943

   
3942
static void plc_ds_destroy(void *data)
3944
static void plc_ds_destroy(void *data)
3943
{
3945
{
[+20] [20] 7 lines
[+20] [+] static struct ast_datastore_info plc_ds_info = {
3951
	.destroy = plc_ds_destroy,
3953
	.destroy = plc_ds_destroy,
3952
};
3954
};
3953

    
   
3955

   
3954
static void adjust_frame_for_plc(struct ast_channel *chan, struct ast_frame *frame, struct ast_datastore *datastore)
3956
static void adjust_frame_for_plc(struct ast_channel *chan, struct ast_frame *frame, struct ast_datastore *datastore)
3955
{
3957
{
3956
	size_t new_size = frame->samples + AST_FRIENDLY_OFFSET;
3958
	int num_new_samples = frame->samples + AST_FRIENDLY_OFFSET;
3957
	struct plc_ds *plc = datastore->data;
3959
	struct plc_ds *plc = datastore->data;

    
   
3960

   
3958
	/* First, we need to be sure that our buffer is large enough to accomodate
3961
	/* First, we need to be sure that our buffer is large enough to accomodate
3959
	 * the samples we need to fill in
3962
	 * the samples we need to fill in. This will likely only occur on the first

    
   
3963
	 * frame we write.
3960
	 */
3964
	 */
3961
	if (plc->samples_buf_size < new_size) {
3965
	if (plc->num_samples < num_new_samples) {
3962
		ast_free(plc->samples_buf);
3966
		ast_free(plc->samples_buf);
3963
		plc->samples_buf = ast_calloc(new_size, sizeof(*plc->samples_buf));
3967
		plc->samples_buf = ast_calloc(num_new_samples, sizeof(*plc->samples_buf));
3964
		if (!plc->samples_buf) {
3968
		if (!plc->samples_buf) {
3965
			ast_channel_datastore_remove(chan, datastore);
3969
			ast_channel_datastore_remove(chan, datastore);
3966
			ast_datastore_free(datastore);
3970
			ast_datastore_free(datastore);
3967
			return;
3971
			return;
3968
		}
3972
		}
3969
		plc->samples_buf_size = new_size;
3973
		plc->num_samples = num_new_samples;
3970
	}
3974
	}
3971

    
   
3975

   

    
   
3976
	if (frame->datalen == 0) {
3972
	plc_fillin(&plc->plc_state, plc->samples_buf + AST_FRIENDLY_OFFSET, frame->samples);
3977
		plc_fillin(&plc->plc_state, plc->samples_buf + AST_FRIENDLY_OFFSET, frame->samples);
3973
	frame->data.ptr = plc->samples_buf;
3978
		frame->data.ptr = plc->samples_buf;
3974
	frame->datalen = new_size;
3979
		frame->datalen = num_new_samples * 2;
3975
	frame->offset = AST_FRIENDLY_OFFSET;
3980
	}

    
   
3981
	plc_rx(&plc->plc_state, plc->samples_buf + AST_FRIENDLY_OFFSET, frame->samples);
3976
}
3982
}
3977

    
   
3983

   
3978
static void apply_plc(struct ast_channel *chan, struct ast_frame *frame)
3984
static void apply_plc(struct ast_channel *chan, struct ast_frame *frame)
3979
{
3985
{
3980
	struct ast_datastore *datastore;
3986
	struct ast_datastore *datastore;
3981
	struct plc_ds *plc;
3987
	struct plc_ds *plc;
3982

    
   
3988

   
3983
	datastore = ast_channel_datastore_find(chan, &plc_ds_info, NULL);
3989
	datastore = ast_channel_datastore_find(chan, &plc_ds_info, NULL);
3984
	if (datastore) {
3990
	if (datastore) {
3985
		plc = datastore->data;
3991
		plc = datastore->data;
3986
		if (frame->datalen == 0) {

   
3987
			adjust_frame_for_plc(chan, frame, datastore);
3992
		adjust_frame_for_plc(chan, frame, datastore);
3988
		}

   
3989
		plc_rx(&plc->plc_state, (int16_t *) frame->data.ptr + AST_FRIENDLY_OFFSET, frame->samples);

   
3990
		return;
3993
		return;
3991
	}
3994
	}
3992

    
   
3995

   
3993
	datastore = ast_datastore_alloc(&plc_ds_info, NULL);
3996
	datastore = ast_datastore_alloc(&plc_ds_info, NULL);
3994
	if (!datastore) {
3997
	if (!datastore) {
[+20] [20] 4 lines
[+20] static void apply_plc(struct ast_channel *chan, struct ast_frame *frame)
3999
		ast_datastore_free(datastore);
4002
		ast_datastore_free(datastore);
4000
		return;
4003
		return;
4001
	}
4004
	}
4002
	datastore->data = plc;
4005
	datastore->data = plc;
4003
	ast_channel_datastore_add(chan, datastore);
4006
	ast_channel_datastore_add(chan, datastore);
4004
	if (frame->datalen == 0) {

   
4005
		adjust_frame_for_plc(chan, frame, datastore);
4007
	adjust_frame_for_plc(chan, frame, datastore);
4006
	}
4008
}
4007
	plc_rx(&plc->plc_state, (int16_t *) frame->data.ptr + AST_FRIENDLY_OFFSET, frame->samples);

   
4008
}

   
4009

    
   
4009

   
4010
int ast_write(struct ast_channel *chan, struct ast_frame *fr)
4010
int ast_write(struct ast_channel *chan, struct ast_frame *fr)
4011
{
4011
{
4012
	int res = -1;
4012
	int res = -1;
4013
	struct ast_frame *f = NULL;
4013
	struct ast_frame *f = NULL;
[+20] [20] 3676 lines
  1. /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.