app_waitforsilence: Cleanup & don't treat missing frames as 'noise'

* WaitForSilence completes successfully if it receives no media in the
  specified timeout, but when acting as WaitForNoise that logic needs
  to be reversed.

* Use standard argument parsing macros and add some error checking for
  invalid values.

* The documentation indicated that the first argument to both
  WaitForSilence and WaitForNoise was required when it was not. Update
  the documentation to reflect that.

* Wrap up some behavior in structs to avoid boolean checks all over the
  place.

ASTERISK-24066 #close
Reported by: M vd S

Change-Id: I01d40adc5b63342bb5018a1bea2081a0aa191ef9

apps/app_waitforsilence.c

@@ -44,6 +44,7 @@
 
 #include "asterisk.h"
 
+#include "asterisk/app.h"
 #include "asterisk/file.h"
 #include "asterisk/channel.h"
 #include "asterisk/pbx.h"
@@ -57,7 +58,9 @@
 			Waits for a specified amount of silence.
 		</synopsis>
 		<syntax>
-			<parameter name="silencerequired" required="true" />
+			<parameter name="silencerequired">
+				<para>If not specified, defaults to <literal>1000</literal> milliseconds.</para>
+			</parameter>
 			<parameter name="iterations">
 				<para>If not specified, defaults to <literal>1</literal>.</para>
 			</parameter>
@@ -101,7 +104,9 @@
 			Waits for a specified amount of noise.
 		</synopsis>
 		<syntax>
-			<parameter name="noiserequired" required="true" />
+			<parameter name="noiserequired">
+				<para>If not specified, defaults to <literal>1000</literal> milliseconds.</para>
+			</parameter>
 			<parameter name="iterations">
 				<para>If not specified, defaults to <literal>1</literal>.</para>
 			</parameter>
@@ -125,17 +130,32 @@
 static char *app_silence = "WaitForSilence";
 static char *app_noise = "WaitForNoise";
 
-static int do_waiting(struct ast_channel *chan, int timereqd, time_t waitstart, int timeout, int wait_for_silence) {
+struct wait_type {
-	struct ast_frame *f = NULL;
+	const char *name;
-	int dsptime = 0;
+	const char *status;
-	RAII_VAR(struct ast_format *, rfmt, NULL, ao2_cleanup);
+	int stop_on_frame_timeout;
-	int res = 0;
+	int (*func)(struct ast_dsp *, struct ast_frame *, int *);
-	struct ast_dsp *sildet;	 /* silence detector dsp */
+};
- 	time_t now;
 
-	/*Either silence or noise calc depending on wait_for_silence flag*/
+static const struct wait_type wait_for_silence = {
-	int (*ast_dsp_func)(struct ast_dsp*, struct ast_frame*, int*) =
+	.name = "silence",
-				wait_for_silence ? ast_dsp_silence : ast_dsp_noise;
+	.status = "SILENCE",
+	.stop_on_frame_timeout = 1,
+	.func = ast_dsp_silence,
+};
+
+static const struct wait_type wait_for_noise = {
+	.name = "noise",
+	.status = "NOISE",
+	.stop_on_frame_timeout = 0,
+	.func = ast_dsp_noise,
+};
+
+static int do_waiting(struct ast_channel *chan, int timereqd, time_t waitstart, int timeout, const struct wait_type *wait_for)
+{
+	RAII_VAR(struct ast_format *, rfmt, NULL, ao2_cleanup);
+	int res;
+	struct ast_dsp *sildet;
 
 	rfmt = ao2_bump(ast_channel_readformat(chan));
 	if ((res = ast_set_read_format(chan, ast_format_slin)) < 0) {
@@ -145,15 +165,13 @@ static int do_waiting(struct ast_channel *chan, int timereqd, time_t waitstart,
 
 	/* Create the silence detector */
 	if (!(sildet = ast_dsp_new())) {
-		ast_log(LOG_WARNING, "Unable to create silence detector :(\n");
+		ast_log(LOG_WARNING, "Unable to create silence detector\n");
 		return -1;
 	}
 	ast_dsp_set_threshold(sildet, ast_dsp_get_threshold_from_settings(THRESHOLD_SILENCE));
 
-	/* Await silence... */
 	for (;;) {
-		/* Start with no silence received */
+		int dsptime = 0;
-		dsptime = 0;
 
 		res = ast_waitfor(chan, timereqd);
 
@@ -165,31 +183,33 @@ static int do_waiting(struct ast_channel *chan, int timereqd, time_t waitstart,
 
 		/* We waited and got no frame; sounds like digital silence or a muted digital channel */
 		if (res == 0) {
+			if (wait_for->stop_on_frame_timeout) {
 				dsptime = timereqd;
+			}
 		} else {
 			/* Looks like we did get a frame, so let's check it out */
-			if (!(f = ast_read(chan))) {
+			struct ast_frame *f = ast_read(chan);
+			if (!f) {
 				pbx_builtin_setvar_helper(chan, "WAITSTATUS", "HANGUP");
 				break;
 			}
 			if (f->frametype == AST_FRAME_VOICE) {
-				ast_dsp_func(sildet, f, &dsptime);
+				wait_for->func(sildet, f, &dsptime);
 			}
 			ast_frfree(f);
 		}
 
-		ast_debug(1, "Got %dms %s < %dms required\n", dsptime, wait_for_silence ? "silence" : "noise", timereqd);
+		ast_debug(1, "Got %dms of %s < %dms required\n", dsptime, wait_for->name, timereqd);
 
 		if (dsptime >= timereqd) {
-			ast_verb(3, "Exiting with %dms %s >= %dms required\n", dsptime, wait_for_silence ? "silence" : "noise", timereqd);
+			ast_verb(3, "Exiting with %dms of %s >= %dms required\n", dsptime, wait_for->name, timereqd);
-			/* Ended happily with silence */
+			pbx_builtin_setvar_helper(chan, "WAITSTATUS", wait_for->status);
+			ast_debug(1, "WAITSTATUS was set to %s\n", wait_for->status);
 			res = 1;
-			pbx_builtin_setvar_helper(chan, "WAITSTATUS", wait_for_silence ? "SILENCE" : "NOISE");
-			ast_debug(1, "WAITSTATUS was set to %s\n", wait_for_silence ? "SILENCE" : "NOISE");
 			break;
 		}
 
-		if (timeout && (difftime(time(&now), waitstart) >= timeout)) {
+		if (timeout && difftime(time(NULL), waitstart) >= timeout) {
 			pbx_builtin_setvar_helper(chan, "WAITSTATUS", "TIMEOUT");
 			ast_debug(1, "WAITSTATUS was set to TIMEOUT\n");
 			res = 0;
@@ -197,61 +217,86 @@ static int do_waiting(struct ast_channel *chan, int timereqd, time_t waitstart,
 		}
 	}
 
-
 	if (rfmt && ast_set_read_format(chan, rfmt)) {
 		ast_log(LOG_WARNING, "Unable to restore format %s to channel '%s'\n", ast_format_get_name(rfmt), ast_channel_name(chan));
 	}
+
 	ast_dsp_free(sildet);
 	return res;
 }
 
-static int waitfor_exec(struct ast_channel *chan, const char *data, int wait_for_silence)
+static int waitfor_exec(struct ast_channel *chan, const char *data, const struct wait_type *wait_for)
 {
 	int res = 1;
 	int timereqd = 1000;
 	int timeout = 0;
 	int iterations = 1, i;
 	time_t waitstart;
+	char *parse;
 	struct ast_silence_generator *silgen = NULL;
 
+	AST_DECLARE_APP_ARGS(args,
+		AST_APP_ARG(timereqd);
+		AST_APP_ARG(iterations);
+		AST_APP_ARG(timeout);
+	);
+
+	parse = ast_strdupa(data);
+	AST_STANDARD_APP_ARGS(args, parse);
+
+	if (!ast_strlen_zero(args.timereqd)) {
+		if (sscanf(args.timereqd, "%30d", &timereqd) != 1 || timereqd < 0) {
+			ast_log(LOG_ERROR, "Argument '%srequired' must be an integer greater than or equal to zero.\n",
+					wait_for->name);
+			return -1;
+		}
+	}
+
+	if (!ast_strlen_zero(args.iterations)) {
+		if (sscanf(args.iterations, "%30d", &iterations) != 1 || iterations < 1) {
+			ast_log(LOG_ERROR, "Argument 'iterations' must be an integer greater than 0.\n");
+			return -1;
+		}
+	}
+
+	if (!ast_strlen_zero(args.timeout)) {
+		if (sscanf(args.timeout, "%30d", &timeout) != 1 || timeout < 0) {
+			ast_log(LOG_ERROR, "Argument 'timeout' must be an integer greater than or equal to zero.\n");
+			return -1;
+		}
+	}
+
 	if (ast_channel_state(chan) != AST_STATE_UP) {
-		res = ast_answer(chan); /* Answer the channel */
+		ast_answer(chan); /* Answer the channel */
 	}
 
-	if (!data || ( (sscanf(data, "%30d,%30d,%30d", &timereqd, &iterations, &timeout) != 3) &&
+	ast_verb(3, "Waiting %d time(s) for %dms of %s with %ds timeout\n",
-		(sscanf(data, "%30d,%30d", &timereqd, &iterations) != 2) &&
+			 iterations, timereqd, wait_for->name, timeout);
-		(sscanf(data, "%30d", &timereqd) != 1) ) ) {
-		ast_log(LOG_WARNING, "Using default value of 1000ms, 1 iteration, no timeout\n");
-	}
-
-	ast_verb(3, "Waiting %d time(s) for %d ms silence with %d timeout\n", iterations, timereqd, timeout);
 
 	if (ast_opt_transmit_silence) {
 		silgen = ast_channel_start_silence_generator(chan);
 	}
+
 	time(&waitstart);
-	res = 1;
+	for (i = 0; i < iterations && res == 1; i++) {
-	for (i=0; (i<iterations) && (res == 1); i++) {
+		res = do_waiting(chan, timereqd, waitstart, timeout, wait_for);
-		res = do_waiting(chan, timereqd, waitstart, timeout, wait_for_silence);
 	}
+
 	if (silgen) {
 		ast_channel_stop_silence_generator(chan, silgen);
 	}
 
-
+	return res > 0 ? 0 : res;
-	if (res > 0)
-		res = 0;
-	return res;
 }
 
 static int waitforsilence_exec(struct ast_channel *chan, const char *data)
 {
-	return waitfor_exec(chan, data, 1);
+	return waitfor_exec(chan, data, &wait_for_silence);
 }
 
 static int waitfornoise_exec(struct ast_channel *chan, const char *data)
 {
-	return waitfor_exec(chan, data, 0);
+	return waitfor_exec(chan, data, &wait_for_noise);
 }
 
 static int unload_module(void)
@@ -272,6 +317,4 @@ static int load_module(void)
 	return res;
 }
 
-AST_MODULE_INFO_STANDARD_EXTENDED(ASTERISK_GPL_KEY, "Wait For Silence");
+AST_MODULE_INFO_STANDARD_EXTENDED(ASTERISK_GPL_KEY, "Wait For Silence/Noise");
-
-