Resolve issues with choppy sound when using res_timing_pthread.

The situation that caused this problem was when continuous mode was being
turned on and off while a rate was set for a timing interface.  A very easy
way to replicate this bug was to do a Playback() from behind a Local channel.
In this scenario, a rate gets set on the channel for doing file playback.
At the same time, continuous mode gets turned on and off about every 20 ms
as frames get queued on to the PBX side channel from the other side of the
Local channel.

Essentially, this module treated continuous mode and a set rate as mutually
exclusive states for the timer to be in.  When I dug deep enough, I observed
the following pattern:

   1) Set timer to tick every 20 ms.
   2) Wait almost 20 ms ...
   3) Continuous mode gets turned on for a queued up frame
   4) Continuous mode gets turned off
   5) The timer goes back to its tick per 20 ms. state but starts counting
      at 0 ms.
   6) Goto step 2.

Sometimes, res_timing_pthread would make it 20 ms and produce a timer tick,
but not most of the time.  This is what produced the choppy sound (or sometimes
no sound at all).

Now, the module treats continuous mode and a set rate as completely independent
timer modes.  They can be enabled and disabled independently of each other and
things work as expected.


(closes issue #14412)
Reported by: dome
Patches:
      issue14412.diff.txt uploaded by russell (license 2)
      issue14412-1.6.1.0.diff.txt uploaded by russell (license 2)
Tested by: DennisD, russell


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@198146 65c4cc65-6c06-0410-ace0-fbb531ad65f3

res/res_timing_pthread.c

@@ -75,7 +75,6 @@ enum {
 enum pthread_timer_state {
 	TIMER_STATE_IDLE,
 	TIMER_STATE_TICKING,
-	TIMER_STATE_CONTINUOUS,
 };
 
 struct pthread_timer {
@@ -85,13 +84,15 @@ struct pthread_timer {
 	/*! Interval in ms for current rate */
 	unsigned int interval;
 	unsigned int tick_count;
+	unsigned int pending_ticks;
 	struct timeval start;
+	unsigned int continuous:1;
 };
 
 static void pthread_timer_destructor(void *obj);
 static struct pthread_timer *find_timer(int handle, int unlinkobj);
-static void write_byte(int wr_fd);
-static void read_pipe(int rd_fd, unsigned int num, int clear);
+static void write_byte(struct pthread_timer *timer);
+static void read_pipe(struct pthread_timer *timer, unsigned int num);
 
 /*!
  * \brief Data for the timing thread
@@ -148,23 +149,6 @@ static void pthread_timer_close(int handle)
 	ao2_ref(timer, -1);
 }
 
-static void set_state(struct pthread_timer *timer)
-{
-	unsigned int rate = timer->rate;
-
-	if (rate) {
-		timer->state = TIMER_STATE_TICKING;
-		timer->interval = roundf(1000.0 / ((float) rate));
-		timer->start = ast_tvnow();
-	} else {
-		timer->state = TIMER_STATE_IDLE;
-		timer->interval = 0;
-		timer->start = ast_tv(0, 0);
-	}
-
-	timer->tick_count = 0;
-}
-
 static int pthread_timer_set_rate(int handle, unsigned int rate)
 {
 	struct pthread_timer *timer;
@@ -175,17 +159,24 @@ static int pthread_timer_set_rate(int handle, unsigned int rate)
 	}
 
 	if (rate > MAX_RATE) {
-		ast_log(LOG_ERROR, "res_timing_pthread only supports timers at a max rate of %d / sec\n",
-			MAX_RATE);
+		ast_log(LOG_ERROR, "res_timing_pthread only supports timers at a "
+				"max rate of %d / sec\n", MAX_RATE);
 		errno = EINVAL;
 		return -1;
 	}
 
 	ao2_lock(timer);
-	timer->rate = rate;
-	if (timer->state != TIMER_STATE_CONTINUOUS) {
-		set_state(timer);
+
+	if ((timer->rate = rate)) {
+		timer->interval = roundf(1000.0 / ((float) rate));
+		timer->start = ast_tvnow();
+		timer->state = TIMER_STATE_TICKING;
+	} else {
+		timer->interval = 0;
+		timer->start = ast_tv(0, 0);
+		timer->state = TIMER_STATE_IDLE;
 	}
+	timer->tick_count = 0;
 
 	ao2_unlock(timer);
 
@@ -204,12 +195,9 @@ static void pthread_timer_ack(int handle, unsigned int quantity)
 		return;
 	}
 
-	if (timer->state == TIMER_STATE_CONTINUOUS) {
-		/* Leave the pipe alone, please! */
-		return;
-	}
-
-	read_pipe(timer->pipe[PIPE_READ], quantity, 0);
+	ao2_lock(timer);
+	read_pipe(timer, quantity);
+	ao2_unlock(timer);
 
 	ao2_ref(timer, -1);
 }
@@ -224,8 +212,10 @@ static int pthread_timer_enable_continuous(int handle)
 	}
 
 	ao2_lock(timer);
-	timer->state = TIMER_STATE_CONTINUOUS;
-	write_byte(timer->pipe[PIPE_WRITE]);
+	if (!timer->continuous) {
+		timer->continuous = 1;
+		write_byte(timer);
+	}
 	ao2_unlock(timer);
 
 	ao2_ref(timer, -1);
@@ -243,8 +233,10 @@ static int pthread_timer_disable_continuous(int handle)
 	}
 
 	ao2_lock(timer);
-	set_state(timer);
-	read_pipe(timer->pipe[PIPE_READ], 0, 1);
+	if (timer->continuous) {
+		timer->continuous = 0;
+		read_pipe(timer, 1);
+	}
 	ao2_unlock(timer);
 
 	ao2_ref(timer, -1);
@@ -261,9 +253,11 @@ static enum ast_timer_event pthread_timer_get_event(int handle)
 		return res;
 	}
 
-	if (timer->state == TIMER_STATE_CONTINUOUS) {
+	ao2_lock(timer);
+	if (timer->continuous && timer->pending_ticks == 1) {
 		res = AST_TIMING_EVENT_CONTINUOUS;
 	}
+	ao2_unlock(timer);
 
 	ao2_ref(timer, -1);
 
@@ -338,7 +332,7 @@ static int check_timer(struct pthread_timer *timer)
 {
 	struct timeval now;
 
-	if (timer->state == TIMER_STATE_IDLE || timer->state == TIMER_STATE_CONTINUOUS) {
+	if (timer->state == TIMER_STATE_IDLE) {
 		return 0;
 	}
 
@@ -347,6 +341,7 @@ static int check_timer(struct pthread_timer *timer)
 	if (timer->tick_count < (ast_tvdiff_ms(now, timer->start) / timer->interval)) {
 		timer->tick_count++;
 		if (!timer->tick_count) {
+			/* Handle overflow. */
 			timer->start = now;
 		}
 		return 1;
@@ -355,13 +350,16 @@ static int check_timer(struct pthread_timer *timer)
 	return 0;
 }
 
-static void read_pipe(int rd_fd, unsigned int quantity, int clear)
+/*!
+ * \internal
+ * \pre timer is locked
+ */
+static void read_pipe(struct pthread_timer *timer, unsigned int quantity)
 {
-	ast_assert(quantity || clear);
+	int rd_fd = timer->pipe[PIPE_READ];
 
-	if (!quantity && clear) {
-		quantity = 1;
-	}
+	ast_assert(quantity);
+	ast_assert(quantity >= timer->pending_ticks);
 
 	do {
 		unsigned char buf[1024];
@@ -376,6 +374,8 @@ static void read_pipe(int rd_fd, unsigned int quantity, int clear)
 		FD_SET(rd_fd, &rfds);
 
 		if (select(rd_fd + 1, &rfds, NULL, NULL, &timeout) != 1) {
+			ast_debug(1, "Reading not available on timing pipe, "
+					"quantity: %u\n", quantity);
 			break;
 		}
 
@@ -386,33 +386,35 @@ static void read_pipe(int rd_fd, unsigned int quantity, int clear)
 			if (errno == EAGAIN) {
 				continue;
 			}
-			ast_log(LOG_ERROR, "read failed on timing pipe: %s\n", strerror(errno));
+			ast_log(LOG_ERROR, "read failed on timing pipe: %s\n",
+					strerror(errno));
 			break;
 		}
 
-		if (clear) {
-			continue;
-		}
-
 		quantity -= res;
+		timer->pending_ticks -= res;
 	} while (quantity);
 }
 
-static void write_byte(int wr_fd)
+/*!
+ * \internal
+ * \pre timer is locked
+ */
+static void write_byte(struct pthread_timer *timer)
 {
-	do {
-		ssize_t res;
-		unsigned char x = 42;
+	ssize_t res;
+	unsigned char x = 42;
 
-		res = write(wr_fd, &x, 1);
+	do {
+		res = write(timer->pipe[PIPE_WRITE], &x, 1);
+	} while (res == -1 && errno == EAGAIN);
 
-		if (res == -1) {
-			if (errno == EAGAIN) {
-				continue;
-			}
-			ast_log(LOG_ERROR, "Error writing to timing pipe: %s\n", strerror(errno));
-		}
-	} while (0);
+	if (res == -1) {
+		ast_log(LOG_ERROR, "Error writing to timing pipe: %s\n",
+				strerror(errno));
+	} else {
+		timer->pending_ticks++;
+	}
 }
 
 static int run_timer(void *obj, void *arg, int flags)
@@ -424,11 +426,9 @@ static int run_timer(void *obj, void *arg, int flags)
 	}
 
 	ao2_lock(timer);
-
 	if (check_timer(timer)) {
-		write_byte(timer->pipe[PIPE_WRITE]);
+		write_byte(timer);
 	}
-
 	ao2_unlock(timer);
 
 	return 0;