[asterisk-dev] [Code Review] Improve behavior of ast_answer() to not lose incoming frames

[Russell Bryant]

> On 2009-03-16 16:09:08, Russell Bryant wrote:
> > /trunk/main/channel.c, line 1779
> > <http://reviewboard.digium.com/r/196/diff/1/?file=3068#file3068line1779>
> >
> >     This should actually be INSERT_HEAD().  Otherwise, the frames do not end up in the proper order when the get put back on the channel.
> 
>  wrote:
>     I believe they are in the proper order. They are placed into the 'frames' list in such a way that that list represents them in the actual order received. Then later, they are pulled off that list from the tail end first, and queued onto the channel's queue using ast_queue_frame_head(). This should result in them appearing the channel's queue in the order received.
>     
>     When my multiframe patch makes its way in, there will be an ast_queue_frame_list() API call that makes this simpler.
> 
>  wrote:
>     You're absolutely correct.  I totally read the second part of this wrong.  I'm so used to seeing AST_LIST_REMOVE_HEAD() for removing each item from a list instead of how you have it written.
>     
>     It will be nice when you have ast_queue_frame_list() for this.  If that weren't coming, I'd recommend using INSERT_HEAD() and REMOVE_HEAD() instead of INSERT_TAIL() and REMOVE(TAIL()).  REMOVE(TAIL()) is O(n^2) while REMOVE_HEAD() is O(n).  Of course, we're talking about only a few frames most likely so it's unlikely to make much of a difference, performance wise ...
> 
>  wrote:
>     I don't believe that's correct.  REMOVE(TAIL()) is O(n), while REMOVE_HEAD() is O(1).

We're both right.  ;-)

If you look at the cost of a single removal, it is what you said.  If you look at the cost of removing every item from the list using each method, it is what I said.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/196/#review547
-----------------------------------------------------------


On 2009-03-17 06:42:02, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/196/
> -----------------------------------------------------------
> 
> (Updated 2009-03-17 06:42:02)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/channel.h 182520 
>   /trunk/main/channel.c 182520 
>   /trunk/main/features.c 182520 
> 
> Diff: http://reviewboard.digium.com/r/196/diff
> 
> 
> Testing
> -------
> 
> Compile testing only.
> 
> 
> Thanks,
> 
> Kevin
> 
>