[asterisk-commits] seanbright: branch group/1.6.1-maintenance r264496 - in /team/group/1.6.1-mai...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Wed May 19 17:35:48 CDT 2010


Author: seanbright
Date: Wed May 19 17:35:44 2010
New Revision: 264496

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=264496
Log:
Merged revisions 264452 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/trunk

........
  r264452 | mmichelson | 2010-05-19 17:29:08 -0400 (Wed, 19 May 2010) | 86 lines
  
  Fix transcode_via_sln option with SIP calls and improve PLC usage.
  
  From reviewboard:
  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.
  
  Review: https://reviewboard.asterisk.org/r/622/
........

Modified:
    team/group/1.6.1-maintenance/   (props changed)
    team/group/1.6.1-maintenance/channels/chan_sip.c
    team/group/1.6.1-maintenance/include/asterisk/_private.h
    team/group/1.6.1-maintenance/include/asterisk/options.h
    team/group/1.6.1-maintenance/main/asterisk.c
    team/group/1.6.1-maintenance/main/channel.c
    team/group/1.6.1-maintenance/main/loader.c

Propchange: team/group/1.6.1-maintenance/
------------------------------------------------------------------------------
Binary property 'trunk-merged' - no diff available.

Modified: team/group/1.6.1-maintenance/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/channels/chan_sip.c?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/channels/chan_sip.c (original)
+++ team/group/1.6.1-maintenance/channels/chan_sip.c Wed May 19 17:35:44 2010
@@ -22294,7 +22294,7 @@
 	printf("Setting up to call extension '%s' at '%s'\n", ext ? ext : "<none>", host);
 #endif
 	p->prefcodec = oldformat;				/* Format for this call */
-	p->jointcapability = oldformat;
+	p->jointcapability = oldformat & p->capability;
 	sip_pvt_lock(p);
 	tmpc = sip_new(p, AST_STATE_DOWN, host);	/* Place the call */
 	if (global_callevents)

Modified: team/group/1.6.1-maintenance/include/asterisk/_private.h
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/include/asterisk/_private.h?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/include/asterisk/_private.h (original)
+++ team/group/1.6.1-maintenance/include/asterisk/_private.h Wed May 19 17:35:44 2010
@@ -69,4 +69,11 @@
  */
 void ast_process_pending_reloads(void);
 
+/*!
+ * \brief Reload genericplc configuration value from codecs.conf
+ *
+ * Implementation is in main/channel.c
+ */
+int ast_plc_reload(void);
+
 #endif /* _ASTERISK__PRIVATE_H */

Modified: team/group/1.6.1-maintenance/include/asterisk/options.h
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/include/asterisk/options.h?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/include/asterisk/options.h (original)
+++ team/group/1.6.1-maintenance/include/asterisk/options.h Wed May 19 17:35:44 2010
@@ -84,6 +84,8 @@
 	AST_OPT_FLAG_DEBUG_FILE = (1 << 23),
 	/*! There is a per-file verbose setting */
 	AST_OPT_FLAG_VERBOSE_FILE = (1 << 24),
+	/*! Generic PLC */
+	AST_OPT_FLAG_GENERIC_PLC = (1 << 30),
 };
 
 /*! These are the options that set by default when Asterisk starts */
@@ -117,6 +119,7 @@
 #define ast_opt_mute			ast_test_flag(&ast_options, AST_OPT_FLAG_MUTE)
 #define ast_opt_dbg_file		ast_test_flag(&ast_options, AST_OPT_FLAG_DEBUG_FILE)
 #define ast_opt_verb_file		ast_test_flag(&ast_options, AST_OPT_FLAG_VERBOSE_FILE)
+#define ast_opt_generic_plc         ast_test_flag(&ast_options, AST_OPT_FLAG_GENERIC_PLC)
 
 extern struct ast_flags ast_options;
 

Modified: team/group/1.6.1-maintenance/main/asterisk.c
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/main/asterisk.c?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/main/asterisk.c (original)
+++ team/group/1.6.1-maintenance/main/asterisk.c Wed May 19 17:35:44 2010
@@ -431,6 +431,7 @@
 	ast_cli(a->fd, "  Transcode via SLIN:          %s\n", ast_test_flag(&ast_options, AST_OPT_FLAG_TRANSCODE_VIA_SLIN) ? "Enabled" : "Disabled");
 	ast_cli(a->fd, "  Internal timing:             %s\n", ast_test_flag(&ast_options, AST_OPT_FLAG_INTERNAL_TIMING) ? "Enabled" : "Disabled");
 	ast_cli(a->fd, "  Transmit silence during rec: %s\n", ast_test_flag(&ast_options, AST_OPT_FLAG_TRANSMIT_SILENCE) ? "Enabled" : "Disabled");
+	ast_cli(a->fd, "  Generic PLC:                 %s\n", ast_test_flag(&ast_options, AST_OPT_FLAG_GENERIC_PLC) ? "Enabled" : "Disabled");
 
 	ast_cli(a->fd, "\n* Subsystems\n");
 	ast_cli(a->fd, "  -------------\n");

Modified: team/group/1.6.1-maintenance/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/main/channel.c?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/main/channel.c (original)
+++ team/group/1.6.1-maintenance/main/channel.c Wed May 19 17:35:44 2010
@@ -3389,6 +3389,86 @@
 	return res;
 }
 
+struct plc_ds {
+	/* A buffer in which to store SLIN PLC
+	 * samples generated by the generic PLC
+	 * functionality in plc.c
+	 */
+	int16_t *samples_buf;
+	/* The current number of samples in the
+	 * samples_buf
+	 */
+	size_t num_samples;
+	plc_state_t plc_state;
+};
+
+static void plc_ds_destroy(void *data)
+{
+	struct plc_ds *plc = data;
+	ast_free(plc->samples_buf);
+	ast_free(plc);
+}
+
+static struct ast_datastore_info plc_ds_info = {
+	.type = "plc",
+	.destroy = plc_ds_destroy,
+};
+
+static void adjust_frame_for_plc(struct ast_channel *chan, struct ast_frame *frame, struct ast_datastore *datastore)
+{
+	int num_new_samples = frame->samples;
+	struct plc_ds *plc = datastore->data;
+
+	/* First, we need to be sure that our buffer is large enough to accomodate
+	 * the samples we need to fill in. This will likely only occur on the first
+	 * frame we write.
+	 */
+	if (plc->num_samples < num_new_samples) {
+		ast_free(plc->samples_buf);
+		plc->samples_buf = ast_calloc(num_new_samples, sizeof(*plc->samples_buf));
+		if (!plc->samples_buf) {
+			ast_channel_datastore_remove(chan, datastore);
+			ast_datastore_free(datastore);
+			return;
+		}
+		plc->num_samples = num_new_samples;
+	}
+
+	if (frame->datalen == 0) {
+		plc_fillin(&plc->plc_state, plc->samples_buf, frame->samples);
+		frame->data.ptr = plc->samples_buf;
+		frame->datalen = num_new_samples * 2;
+	} else {
+		plc_rx(&plc->plc_state, frame->data.ptr, frame->samples);
+	}
+}
+
+static void apply_plc(struct ast_channel *chan, struct ast_frame *frame)
+{
+	struct ast_datastore *datastore;
+	struct plc_ds *plc;
+
+	datastore = ast_channel_datastore_find(chan, &plc_ds_info, NULL);
+	if (datastore) {
+		plc = datastore->data;
+		adjust_frame_for_plc(chan, frame, datastore);
+		return;
+	}
+
+	datastore = ast_datastore_alloc(&plc_ds_info, NULL);
+	if (!datastore) {
+		return;
+	}
+	plc = ast_calloc(1, sizeof(*plc));
+	if (!plc) {
+		ast_datastore_free(datastore);
+		return;
+	}
+	datastore->data = plc;
+	ast_channel_datastore_add(chan, datastore);
+	adjust_frame_for_plc(chan, frame, datastore);
+}
+
 int ast_write(struct ast_channel *chan, struct ast_frame *fr)
 {
 	int res = -1;
@@ -3503,6 +3583,10 @@
 	case AST_FRAME_VOICE:
 		if (chan->tech->write == NULL)
 			break;	/*! \todo XXX should return 0 maybe ? */
+
+		if (ast_opt_generic_plc && fr->subclass == AST_FORMAT_SLINEAR) {
+			apply_plc(chan, fr);
+		}
 
 		/* If the frame is in the raw write format, then it's easy... just use the frame - otherwise we will have to translate */
 		if (fr->subclass == chan->rawwriteformat)
@@ -4172,10 +4256,12 @@
 	}
 
 	/* if the best path is not 'pass through', then
-	   transcoding is needed; if desired, force transcode path
-	   to use SLINEAR between channels, but only if there is
-	   no direct conversion available */
-	if ((src != dst) && ast_opt_transcode_via_slin &&
+	 * transcoding is needed; if desired, force transcode path
+	 * to use SLINEAR between channels, but only if there is
+	 * no direct conversion available. If generic PLC is
+	 * desired, then transcoding via SLINEAR is a requirement
+	 */
+	if ((src != dst) && (ast_opt_generic_plc || ast_opt_transcode_via_slin) &&
 	    (ast_translate_path_steps(dst, src) != 1))
 		dst = AST_FORMAT_SLINEAR;
 	if (ast_set_read_format(from, dst) < 0) {
@@ -5554,9 +5640,27 @@
 		ast_moh_cleanup_ptr(chan);
 }
 
+int ast_plc_reload(void)
+{
+	struct ast_variable *var;
+	struct ast_flags config_flags = { 0 };
+	struct ast_config *cfg = ast_config_load2("codecs.conf", "channel", config_flags);
+	if (!cfg || cfg == CONFIG_STATUS_FILEUNCHANGED)
+		return 0;
+	for (var = ast_variable_browse(cfg, "plc"); var; var = var->next) {
+		if (!strcasecmp(var->name, "genericplc")) {
+			ast_set2_flag(&ast_options, ast_true(var->value), AST_OPT_FLAG_GENERIC_PLC);
+		}
+	}
+	ast_config_destroy(cfg);
+	return 0;
+}
+
 void ast_channels_init(void)
 {
 	ast_cli_register_multiple(cli_channel, ARRAY_LEN(cli_channel));
+
+	ast_plc_reload();
 }
 
 /*! \brief Print call group and pickup group ---*/

Modified: team/group/1.6.1-maintenance/main/loader.c
URL: http://svnview.digium.com/svn/asterisk/team/group/1.6.1-maintenance/main/loader.c?view=diff&rev=264496&r1=264495&r2=264496
==============================================================================
--- team/group/1.6.1-maintenance/main/loader.c (original)
+++ team/group/1.6.1-maintenance/main/loader.c Wed May 19 17:35:44 2010
@@ -259,6 +259,7 @@
 	{ "features",	ast_features_reload },
 	{ "dsp",	ast_dsp_reload},
 	{ "udptl",	ast_udptl_reload },
+	{ "plc",        ast_plc_reload },
 	{ NULL, 	NULL }
 };
 




More information about the asterisk-commits mailing list