[asterisk-dev] [Code Review] Handle multi-frame lists returned from channel drivers or translators

[Kevin Fleming]

> On 2009-02-25 17:23:09, Russell Bryant wrote:
> > /branches/1.4/main/channel.c, line 2218
> > <http://reviewboard.digium.com/r/175/diff/1/?file=2861#file2861line2218>
> >
> >     We discussed that this condition might not be possible today.  However, I think it's probably good to leave in for the sake of being future proof.  Also, I think the code looks safe for the case that readq_tail is NULL.

Actually, it's broken in at least two ways. If readq_tail is NULL (the normal case), then any additional frames from the read translator would be placed at the tail of the readq, but they belong at the head. Also, this condition is *always* possible, because this code path can be taken whether the frame arrived from the readq (which could have still more frames on it) or from the channel driver. If the frame arrived from the readq, then the block of code immediately following this won't be triggered (as we would have only read a single frame from the readq), but the later block after calling the read translator may still have to insert frames onto the head of the readq in front of the frames remaining there.

I'll be uploading a new patch in a bit, with this fixed along with some frame freeing that is missing right now, and fixing up the ast_write() path.


- Kevin


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


On 2009-02-24 18:14:59, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/175/
> -----------------------------------------------------------
> 
> (Updated 2009-02-24 18:14:59)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> While working on some translator issues, it became apparent that Asterisk has some flaws in handling translators that implement frame-based codecs and that cannot always return translated frames one-for-one for each source frame. That caused me to start looking at various code paths, and found that even the simple multi-frame support in __ast_read() that was added for UDPTL support is not properly implemented.
> 
> This patch does a few things:
> 
> 1) Enhances ast_queue_frame() to be able to accept a list of frames, and places the entire list of frames in the desired location in the channel's readq. The internal-only __ast_queue_frame can also accept an 'after' parameter, so that the frame or list of frames being passed can be inserted into the readq after a specific frame already in the readq.
> 
> 2) Corrects the multi-frame support existing in ast_read() that is used when a channel drivers's read() function returns a list of frames; now the frames are queued (as they were before), but the alertpipe or timingfd on the channel is also properly updated to reflect the number of frames that were queued, so that when ast_read() returns the thread reading from the channel will immediately wake up again to read the queued frames (one by one).
> 
> 3) Adds support in __ast_read() for a translation path (on chan->readtrans) returning multiple frames, queuing the excess frames onto the channel's readq in the proper location.
> 
> 4) Adds support in ast_write() for a translation path (on chan->writetrans) returning multiple frames, feeding each returned frame to the channel driver's write() function in order.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/linkedlists.h 178442 
>   /branches/1.4/main/channel.c 178442 
> 
> Diff: http://reviewboard.digium.com/r/175/diff
> 
> 
> Testing
> -------
> 
> Compile testing only, so far.
> 
> 
> Thanks,
> 
> Kevin
> 
>