audiohook.c: Difference in read/write rates caused continuous buffer resets

Currently, everytime a sample rate change occurs (on read or write) the
associated factory buffers are reset. If the requested sample rate on a
read differed from that of a write then the buffers are continually reset
on every read and write. This has the side effect of emptying the buffer,
thus there being no data to read and then write to a file in the case of
call recording.

This patch fixes it so that an audiohook_list's rate always maintains the
maximum sample rate among hooks and formats. Audiohook sample rates are
only overwritten by this value when slin native compatibility is turned on.
Also, the audiohook sample rate can only overwrite the list's sample rate
when its rate is greater than that of the list or if compatibility is
turned off. This keeps the rate from constantly switching/resetting.

ASTERISK-24944 #close
Reported by: Ronald Raikes

Change-Id: Idab4dfef068a7922c09cc631dda27bc920a6c76f

2 files changed, 109 insertions, 23 deletions

diff --git a/include/asterisk/audiohook.h b/include/asterisk/audiohook.h
index 375b2dd9d..cae8cc071 100644
--- a/include/asterisk/audiohook.h
+++ b/include/asterisk/audiohook.h
@@ -63,6 +63,7 @@ enum ast_audiohook_flags {
 	AST_AUDIOHOOK_SMALL_QUEUE   = (1 << 4),
 	AST_AUDIOHOOK_MUTE_READ     = (1 << 5), /*!< audiohook should be mute frames read */
 	AST_AUDIOHOOK_MUTE_WRITE    = (1 << 6), /*!< audiohook should be mute frames written */
+	AST_AUDIOHOOK_COMPATIBLE    = (1 << 7), /*!< is the audiohook native slin compatible */
 };
 
 enum ast_audiohook_init_flags {
diff --git a/main/audiohook.c b/main/audiohook.c
index b754f2329..3e233fa2a 100644
--- a/main/audiohook.c
+++ b/main/audiohook.c
@@ -46,6 +46,8 @@ ASTERISK_REGISTER_FILE()
 #define AST_AUDIOHOOK_SYNC_TOLERANCE 100 /*!< Tolerance in milliseconds for audiohooks synchronization */
 #define AST_AUDIOHOOK_SMALL_QUEUE_TOLERANCE 100 /*!< When small queue is enabled, this is the maximum amount of audio that can remain queued at a time. */
 
+#define DEFAULT_INTERNAL_SAMPLE_RATE 8000
+
 struct ast_audiohook_translate {
 	struct ast_trans_pvt *trans_pvt;
 	struct ast_format *format;
@@ -117,7 +119,7 @@ int ast_audiohook_init(struct ast_audiohook *audiohook, enum ast_audiohook_type
 	audiohook->init_flags = init_flags;
 
 	/* initialize internal rate at 8khz, this will adjust if necessary */
-	audiohook_set_internal_rate(audiohook, 8000, 0);
+	audiohook_set_internal_rate(audiohook, DEFAULT_INTERNAL_SAMPLE_RATE, 0);
 
 	/* Since we are just starting out... this audiohook is new */
 	ast_audiohook_update_status(audiohook, AST_AUDIOHOOK_STATUS_NEW);
@@ -361,7 +363,19 @@ static struct ast_frame *audiohook_read_frame_helper(struct ast_audiohook *audio
 	struct ast_frame *read_frame = NULL, *final_frame = NULL;
 	struct ast_format *slin;
 
-	audiohook_set_internal_rate(audiohook, ast_format_get_sample_rate(format), 1);
+	/*
+	 * Update the rate if compatibility mode is turned off or if it is
+	 * turned on and the format rate is higher than the current rate.
+	 *
+	 * This makes it so any unnecessary rate switching/resetting does
+	 * not take place and also any associated audiohook_list's internal
+	 * sample rate maintains the highest sample rate between hooks.
+	 */
+	if (!ast_test_flag(audiohook, AST_AUDIOHOOK_COMPATIBLE) ||
+	    (ast_test_flag(audiohook, AST_AUDIOHOOK_COMPATIBLE) &&
+	      ast_format_get_sample_rate(format) > audiohook->hook_internal_samp_rate)) {
+		audiohook_set_internal_rate(audiohook, ast_format_get_sample_rate(format), 1);
+	}
 
 	if (!(read_frame = (direction == AST_AUDIOHOOK_DIRECTION_BOTH ?
 		audiohook_read_frame_both(audiohook, samples, read_reference, write_reference) :
@@ -425,6 +439,22 @@ struct ast_frame *ast_audiohook_read_frame_all(struct ast_audiohook *audiohook,
 static void audiohook_list_set_samplerate_compatibility(struct ast_audiohook_list *audiohook_list)
 {
 	struct ast_audiohook *ah = NULL;
+
+	/*
+	 * Anytime the samplerate compatibility is set (attach/remove an audiohook) the
+	 * list's internal sample rate needs to be reset so that the next time processing
+	 * through write_list, if needed, it will get updated to the correct rate.
+	 *
+	 * A list's internal rate always chooses the higher between its own rate and a
+	 * given rate. If the current rate is being driven by an audiohook that wanted a
+	 * higher rate then when this audiohook is removed the list's rate would remain
+	 * at that level when it should be lower, and with no way to lower it since any
+	 * rate compared against it would be lower.
+	 *
+	 * By setting it back to the lowest rate it can recalulate the new highest rate.
+	 */
+	audiohook_list->list_internal_samp_rate = DEFAULT_INTERNAL_SAMPLE_RATE;
+
 	audiohook_list->native_slin_compatible = 1;
 	AST_LIST_TRAVERSE(&audiohook_list->manipulate_list, ah, list) {
 		if (!(ah->init_flags & AST_AUDIOHOOK_MANIPULATE_ALL_RATES)) {
@@ -455,7 +485,7 @@ int ast_audiohook_attach(struct ast_channel *chan, struct ast_audiohook *audioho
 		AST_LIST_HEAD_INIT_NOLOCK(&ast_channel_audiohooks(chan)->whisper_list);
 		AST_LIST_HEAD_INIT_NOLOCK(&ast_channel_audiohooks(chan)->manipulate_list);
 		/* This sample rate will adjust as necessary when writing to the list. */
-		ast_channel_audiohooks(chan)->list_internal_samp_rate = 8000;
+		ast_channel_audiohooks(chan)->list_internal_samp_rate = DEFAULT_INTERNAL_SAMPLE_RATE;
 	}
 
 	/* Drop into respective list */
@@ -467,8 +497,11 @@ int ast_audiohook_attach(struct ast_channel *chan, struct ast_audiohook *audioho
 		AST_LIST_INSERT_TAIL(&ast_channel_audiohooks(chan)->manipulate_list, audiohook, list);
 	}
 
-
-	audiohook_set_internal_rate(audiohook, ast_channel_audiohooks(chan)->list_internal_samp_rate, 1);
+	/*
+	 * Initialize the audiohook's rate to the default. If it needs to be,
+	 * it will get updated later.
+	 */
+	audiohook_set_internal_rate(audiohook, DEFAULT_INTERNAL_SAMPLE_RATE, 1);
 	audiohook_list_set_samplerate_compatibility(ast_channel_audiohooks(chan));
 
 	/* Change status over to running since it is now attached */
@@ -766,14 +799,14 @@ static struct ast_frame *audiohook_list_translate_to_slin(struct ast_audiohook_l
 	struct ast_frame *new_frame = frame;
 	struct ast_format *slin;
 
-	/* If we are capable of maintaining doing samplerates other that 8khz, update
-	 * the internal audiohook_list's rate and higher samplerate audio arrives. By
-	 * updating the list's rate, all the audiohooks in the list will be updated as well
-	 * as the are written and read from. */
-	if (audiohook_list->native_slin_compatible) {
-		audiohook_list->list_internal_samp_rate =
-			MAX(ast_format_get_sample_rate(frame->subclass.format), audiohook_list->list_internal_samp_rate);
-	}
+	/*
+	 * If we are capable of sample rates other that 8khz, update the internal
+	 * audiohook_list's rate and higher sample rate audio arrives. If native
+	 * slin compatibility is turned on all audiohooks in the list will be
+	 * updated as well during read/write processing.
+	 */
+	audiohook_list->list_internal_samp_rate =
+		MAX(ast_format_get_sample_rate(frame->subclass.format), audiohook_list->list_internal_samp_rate);
 
 	slin = ast_format_cache_get_slin_by_rate(audiohook_list->list_internal_samp_rate);
 	if (ast_format_cmp(frame->subclass.format, slin) == AST_FORMAT_CMP_EQUAL) {
@@ -822,6 +855,36 @@ static struct ast_frame *audiohook_list_translate_to_native(struct ast_audiohook
 }
 
 /*!
+ *\brief Set the audiohook's internal sample rate to the audiohook_list's rate,
+ *       but only when native slin compatibility is turned on.
+ *
+ * \param audiohook_list audiohook_list data object
+ * \param audiohook the audiohook to update
+ * \param rate the current max internal sample rate
+ */
+static void audiohook_list_set_hook_rate(struct ast_audiohook_list *audiohook_list,
+					 struct ast_audiohook *audiohook, int *rate)
+{
+	/* The rate should always be the max between itself and the hook */
+	if (audiohook->hook_internal_samp_rate > *rate) {
+		*rate = audiohook->hook_internal_samp_rate;
+	}
+
+	/*
+	 * If native slin compatibility is turned on then update the audiohook
+	 * with the audiohook_list's current rate. Note, the audiohook's rate is
+	 * set to the audiohook_list's rate and not the given rate. If there is
+	 * a change in rate the hook's rate is changed on its next check.
+	 */
+	if (audiohook_list->native_slin_compatible) {
+		ast_set_flag(audiohook, AST_AUDIOHOOK_COMPATIBLE);
+		audiohook_set_internal_rate(audiohook, audiohook_list->list_internal_samp_rate, 1);
+	} else {
+		ast_clear_flag(audiohook, AST_AUDIOHOOK_COMPATIBLE);
+	}
+}
+
+/*!
  * \brief Pass an AUDIO frame off to be handled by the audiohook core
  *
  * \details
@@ -851,6 +914,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 	int samples;
 	int middle_frame_manipulated = 0;
 	int removed = 0;
+	int internal_sample_rate;
 
 	/* ---Part_1. translate start_frame to SLINEAR if necessary. */
 	if (!(middle_frame = audiohook_list_translate_to_slin(audiohook_list, direction, start_frame))) {
@@ -858,6 +922,19 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 	}
 	samples = middle_frame->samples;
 
+	/*
+	 * While processing each audiohook check to see if the internal sample rate needs
+	 * to be adjusted (it should be the highest rate specified between formats and
+	 * hooks). The given audiohook_list's internal sample rate is then set to the
+	 * updated value before returning.
+	 *
+	 * If slin compatibility mode is turned on then an audiohook's internal sample
+	 * rate is set to its audiohook_list's rate. If an audiohook_list's rate is
+	 * adjusted during this pass then the change is picked up by the audiohooks
+	 * on the next pass.
+	 */
+	internal_sample_rate = audiohook_list->list_internal_samp_rate;
+
 	/* ---Part_2: Send middle_frame to spy and manipulator lists.  middle_frame is guaranteed to be SLINEAR here.*/
 	/* Queue up signed linear frame to each spy */
 	AST_LIST_TRAVERSE_SAFE_BEGIN(&audiohook_list->spy_list, audiohook, list) {
@@ -872,7 +949,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 			}
 			continue;
 		}
-		audiohook_set_internal_rate(audiohook, audiohook_list->list_internal_samp_rate, 1);
+		audiohook_list_set_hook_rate(audiohook_list, audiohook, &internal_sample_rate);
 		ast_audiohook_write_frame(audiohook, direction, middle_frame);
 		ast_audiohook_unlock(audiohook);
 	}
@@ -896,7 +973,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 				}
 				continue;
 			}
-			audiohook_set_internal_rate(audiohook, audiohook_list->list_internal_samp_rate, 1);
+			audiohook_list_set_hook_rate(audiohook_list, audiohook, &internal_sample_rate);
 			if (ast_slinfactory_available(factory) >= samples && ast_slinfactory_read(factory, read_buf, samples)) {
 				/* Take audio from this whisper source and combine it into our main buffer */
 				for (i = 0, data1 = combine_buf, data2 = read_buf; i < samples; i++, data1++, data2++) {
@@ -929,14 +1006,16 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 				}
 				continue;
 			}
-			audiohook_set_internal_rate(audiohook, audiohook_list->list_internal_samp_rate, 1);
-			/* Feed in frame to manipulation. */
-			if (!audiohook->manipulate_callback(audiohook, chan, middle_frame, direction)) {
-				/* If the manipulation fails then the frame will be returned in its original state.
-				 * Since there are potentially more manipulator callbacks in the list, no action should
-				 * be taken here to exit early. */
-				 middle_frame_manipulated = 1;
-			}
+			audiohook_list_set_hook_rate(audiohook_list, audiohook, &internal_sample_rate);
+			/*
+			 * Feed in frame to manipulation.
+			 *
+			 * XXX FAILURES ARE IGNORED XXX
+			 * If the manipulation fails then the frame will be returned in its original state.
+			 * Since there are potentially more manipulator callbacks in the list, no action should
+			 * be taken here to exit early.
+			 */
+			audiohook->manipulate_callback(audiohook, chan, middle_frame, direction);
 			ast_audiohook_unlock(audiohook);
 		}
 		AST_LIST_TRAVERSE_SAFE_END;
@@ -960,6 +1039,12 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
 	/* Before returning, if an audiohook got removed, reset samplerate compatibility */
 	if (removed) {
 		audiohook_list_set_samplerate_compatibility(audiohook_list);
+	} else {
+		/*
+		 * Set the audiohook_list's rate to the updated rate. Note that if a hook
+		 * was removed then the list's internal rate is reset to the default.
+		 */
+		audiohook_list->list_internal_samp_rate = internal_sample_rate;
 	}
 
 	return end_frame;