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

1 files changed, 96 insertions, 33 deletions

diff --git a/main/channel.c b/main/channel.c
index f4ab05f95..3520efc22 100644
--- a/main/channel.c
+++ b/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,15 +1732,50 @@ 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
-				 */
+		break;
+	case AST_STATE_UP:
+		/* Calling ast_cdr_answer when it it has previously been called
+		 * is essentially a no-op, so it is safe.
+		 */
+		if (cdr_answer) {
+			ast_cdr_answer(chan->cdr);
+		}
+		break;
+	default:
+		break;
+	}
+
+	ast_indicate(chan, -1);
+	chan->visible_indication = 0;
+
+	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));
@@ -1749,43 +1783,72 @@ int __ast_answer(struct ast_channel *chan, unsigned int delay,  int cdr_answer)
 					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;
+					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;
 				}
-				f = ast_read(chan);
-				if (!f || (f->frametype == AST_FRAME_CONTROL && f->subclass == AST_CONTROL_HANGUP)) {
-					if (f) {
-						ast_frfree(f);
+				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 (f->frametype == AST_FRAME_VOICE) {
-					ast_frfree(f);
-					res = 0;
+
+				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;
 				}
-				ast_frfree(f);
 			}
-		}
-		break;
-	case AST_STATE_UP:
-		/* Calling ast_cdr_answer when it it has previously been called
-		 * is essentially a no-op, so it is safe.
-		 */
-		if (cdr_answer) {
-			ast_cdr_answer(chan->cdr);
-		}
+
+			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;
 	}
 
-	ast_indicate(chan, -1);
-	chan->visible_indication = 0;
-
 	return res;
 }