Improve behavior of ast_answer() to not lose incoming frames
authorKevin P. Fleming <kpfleming@digium.com>
Tue, 17 Mar 2009 14:38:11 +0000 (14:38 +0000)
committerKevin P. Fleming <kpfleming@digium.com>
Tue, 17 Mar 2009 14:38:11 +0000 (14:38 +0000)
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
main/channel.c
main/features.c

index 0f30ea1..eff55ca 100644 (file)
@@ -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
index f4ab05f..3520efc 100644 (file)
@@ -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;
 }
 
index 4a8a345..91d90fe 100644 (file)
@@ -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));