[asterisk-dev] [Code Review] Format change when queue still has frames of the old format

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/363/#review1092
-----------------------------------------------------------


This is an interesting approach.  However, I see some problems with it.

This patch queues up a control frame to a channel read queue, and when the frame gets read, restores the old write format.  However, the important thing here is that the frame queue on a channel is a _read_ queue.  It's not a queue of frames to _write_ to the channel.  So, the frames on the readq, and the format they are in, are really completely unrelated to the format used for writing to the channel.

Also, I'm trying to wrap my head around the connection between the issue you're addressing and the bug that has been reported.  The bug report (and your patch) address file stream handling.  However, file playback doesn't use the channel frame queue at all.  The frames are written directly to the channel.  Can you come up with a (psuedo) code path that shows how the patch you are proposing modifies what happens with file playback to a DAHDI channel?  Also, the bug report says that incoming DTMF is the trigger of the problem.  How do the incoming DTMF events fit into this picture?

- Russell


On 2009-09-15 10:46:10, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/363/
> -----------------------------------------------------------
> 
> (Updated 2009-09-15 10:46:10)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Implements a new control frame, such that the format can change back to the oldwriteformat only when frames in-queue have already been processed.
> 
> 
> This addresses bug 15129.
>     https://issues.asterisk.org/view.php?id=15129
> 
> 
> Diffs
> -----
> 
>   /branches/1.6.1/include/asterisk/frame.h 218361 
>   /branches/1.6.1/main/channel.c 218361 
>   /branches/1.6.1/main/file.c 218361 
> 
> Diff: https://reviewboard.asterisk.org/r/363/diff
> 
> 
> Testing
> -------
> 
> Due to the test requiring hardware that I do not have access to, testing will need to be performed by the reporter.  Given that the reporter states that this problem always occurs, this should be a fairly easy test.
> 
> 
> Thanks,
> 
> Tilghman
> 
>