Improve behavior of ast_answer() to not lose incoming frames

ast_answer(), when supplied a delay before returning to the caller, use ast_safe_sleep() to implement the delay. Unfortunately during this time any incoming frames are discarded, which is problematic for T.38 re-INVITES and other sorts of channel operations.

When a delay is not passed to ast_answer(), it still delays for up to 500 milliseconds, waiting for media to arrive. Again, though, it discards any control frames, or non-voice media frames.

This patch rectifies this situation, by storing all incoming frames during the delay period on a list, and then requeuing them onto the channel before returning to the caller.

http://reviewboard.digium.com/r/196/



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

include/asterisk/channel.h

@@ -1008,12 +1008,61 @@ void ast_channel_setwhentohangup_tv(struct ast_channel *chan, struct timeval off
  * This function answers a channel and handles all necessary call
  * setup functions.
  *
- * \note The channel passed does not need to be locked.
+ * \note The channel passed does not need to be locked, but is locked
+ * by the function when needed.
+ *
+ * \note This function will wait up to 500 milliseconds for media to
+ * arrive on the channel before returning to the caller, so that the
+ * caller can properly assume the channel is 'ready' for media flow.
  *
  * \retval 0 on success
  * \retval non-zero on failure
  */
 int ast_answer(struct ast_channel *chan);
+
+/*!
+ * \brief Answer a channel
+ *
+ * \param chan channel to answer
+ * \param cdr_answer flag to control whether any associated CDR should be marked as 'answered'
+ *
+ * This function answers a channel and handles all necessary call
+ * setup functions.
+ *
+ * \note The channel passed does not need to be locked, but is locked
+ * by the function when needed.
+ *
+ * \note Unlike ast_answer(), this function will not wait for media
+ * flow to begin. The caller should be careful before sending media
+ * to the channel before incoming media arrives, as the outgoing
+ * media may be lost.
+ *
+ * \retval 0 on success
+ * \retval non-zero on failure
+ */
+int ast_raw_answer(struct ast_channel *chan, int cdr_answer);
+
+/*!
+ * \brief Answer a channel, with a selectable delay before returning
+ *
+ * \param chan channel to answer
+ * \param delay maximum amount of time to wait for incoming media
+ * \param cdr_answer flag to control whether any associated CDR should be marked as 'answered'
+ *
+ * This function answers a channel and handles all necessary call
+ * setup functions.
+ *
+ * \note The channel passed does not need to be locked, but is locked
+ * by the function when needed.
+ *
+ * \note This function will wait up to 'delay' milliseconds for media to
+ * arrive on the channel before returning to the caller, so that the
+ * caller can properly assume the channel is 'ready' for media flow. If
+ * 'delay' is less than 500, the function will wait up to 500 milliseconds.
+ *
+ * \retval 0 on success
+ * \retval non-zero on failure
+ */
 int __ast_answer(struct ast_channel *chan, unsigned int delay, int cdr_answer);
 
 /*! \brief Make a call

main/channel.c

@@ -1700,8 +1700,7 @@ int ast_hangup(struct ast_channel *chan)
 	return res;
 }
 
-#define ANSWER_WAIT_MS 500
-int __ast_answer(struct ast_channel *chan, unsigned int delay,  int cdr_answer)
+int ast_raw_answer(struct ast_channel *chan, int cdr_answer)
 {
 	int res = 0;
 
@@ -1733,43 +1732,6 @@ int __ast_answer(struct ast_channel *chan, unsigned int delay,  int cdr_answer)
 			ast_cdr_answer(chan->cdr);
 		}
 		ast_channel_unlock(chan);
-		if (delay) {
-			ast_safe_sleep(chan, delay);
-		} else {
-			struct ast_frame *f;
-			int ms = ANSWER_WAIT_MS;
-			while (1) {
-				/* 500 ms was the original delay here, so now
-				 * we cap our waiting at 500 ms
-				 */
-				ms = ast_waitfor(chan, ms);
-				if (ms < 0) {
-					ast_log(LOG_WARNING, "Error condition occurred when polling channel %s for a voice frame: %s\n", chan->name, strerror(errno));
-					res = -1;
-					break;
-				}
-				if (ms == 0) {
-					ast_debug(2, "Didn't receive a voice frame from %s within %d ms of answering. Continuing anyway\n", chan->name, ANSWER_WAIT_MS);
-					res = 0;
-					break;
-				}
-				f = ast_read(chan);
-				if (!f || (f->frametype == AST_FRAME_CONTROL && f->subclass == AST_CONTROL_HANGUP)) {
-					if (f) {
-						ast_frfree(f);
-					}
-					res = -1;
-					ast_debug(2, "Hangup of channel %s detected in answer routine\n", chan->name);
-					break;
-				}
-				if (f->frametype == AST_FRAME_VOICE) {
-					ast_frfree(f);
-					res = 0;
-					break;
-				}
-				ast_frfree(f);
-			}
-		}
 		break;
 	case AST_STATE_UP:
 		/* Calling ast_cdr_answer when it it has previously been called
@@ -1789,6 +1751,107 @@ int __ast_answer(struct ast_channel *chan, unsigned int delay,  int cdr_answer)
 	return res;
 }
 
+int __ast_answer(struct ast_channel *chan, unsigned int delay, int cdr_answer)
+{
+	int res = 0;
+	enum ast_channel_state old_state;
+
+	old_state = chan->_state;
+	if ((res = ast_raw_answer(chan, cdr_answer))) {
+		return res;
+	}
+
+	switch (old_state) {
+	case AST_STATE_RINGING:
+	case AST_STATE_RING:
+		/* wait for media to start flowing, but don't wait any longer
+		 * than 'delay' or 500 milliseconds, whichever is longer
+		 */
+		do {
+			AST_LIST_HEAD_NOLOCK(, ast_frame) frames;
+			struct ast_frame *cur, *new;
+			int ms = MAX(delay, 500);
+			unsigned int done = 0;
+
+			AST_LIST_HEAD_INIT_NOLOCK(&frames);
+
+			for (;;) {
+				ms = ast_waitfor(chan, ms);
+				if (ms < 0) {
+					ast_log(LOG_WARNING, "Error condition occurred when polling channel %s for a voice frame: %s\n", chan->name, strerror(errno));
+					res = -1;
+					break;
+				}
+				if (ms == 0) {
+					ast_debug(2, "Didn't receive a media frame from %s within %d ms of answering. Continuing anyway\n", chan->name, MAX(delay, 500));
+					break;
+				}
+				cur = ast_read(chan);
+				if (!cur || ((cur->frametype == AST_FRAME_CONTROL) &&
+					     (cur->subclass == AST_CONTROL_HANGUP))) {
+					if (cur) {
+						ast_frfree(cur);
+					}
+					res = -1;
+					ast_debug(2, "Hangup of channel %s detected in answer routine\n", chan->name);
+					break;
+				}
+
+				if ((new = ast_frisolate(cur)) != cur) {
+					ast_frfree(cur);
+				}
+
+				AST_LIST_INSERT_TAIL(&frames, new, frame_list);
+
+				/* if a specific delay period was requested, continue
+				 * until that delay has passed. don't stop just because
+				 * incoming media has arrived.
+				 */
+				if (delay) {
+					continue;
+				}
+
+				switch (new->frametype) {
+					/* all of these frametypes qualify as 'media' */
+				case AST_FRAME_VOICE:
+				case AST_FRAME_VIDEO:
+				case AST_FRAME_TEXT:
+				case AST_FRAME_DTMF_BEGIN:
+				case AST_FRAME_DTMF_END:
+				case AST_FRAME_IMAGE:
+				case AST_FRAME_HTML:
+				case AST_FRAME_MODEM:
+					done = 1;
+					break;
+				case AST_FRAME_CONTROL:
+				case AST_FRAME_IAX:
+				case AST_FRAME_NULL:
+				case AST_FRAME_CNG:
+					break;
+				}
+
+				if (done) {
+					break;
+				}
+			}
+
+			if (res == 0) {
+				ast_channel_lock(chan);
+				while ((cur = AST_LIST_REMOVE(&frames, AST_LIST_LAST(&frames), frame_list))) {
+					ast_queue_frame_head(chan, cur);
+					ast_frfree(cur);
+				}
+				ast_channel_unlock(chan);
+			}
+		} while (0);
+		break;
+	default:
+		break;
+	}
+
+	return res;
+}
+
 int ast_answer(struct ast_channel *chan)
 {
 	return __ast_answer(chan, 0, 1);

main/features.c

@@ -2477,8 +2477,11 @@ int ast_bridge_call(struct ast_channel *chan,struct ast_channel *peer,struct ast
 	config->firstpass = 1;
 
 	/* Answer if need be */
-	if (ast_answer(chan))
-		return -1;
+	if (chan->_state != AST_STATE_UP) {
+		if (ast_raw_answer(chan, 1)) {
+			return -1;
+		}
+	}
 
 	ast_copy_string(orig_channame,chan->name,sizeof(orig_channame));
 	ast_copy_string(orig_peername,peer->name,sizeof(orig_peername));