[asterisk-dev] [Code Review] Allow ast_safe_sleep to defer frames similarly to the way autoservice does it.

[Kevin Fleming]

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

Ship it!


This looks good, and your testing proves it works. Make it so!


/branches/1.4/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/674/#comment4351>

    Minor point: make the parameter 'const'.



/branches/1.4/main/channel.c
<https://reviewboard.asterisk.org/r/674/#comment4352>

    splotch!


- Kevin


On 2010-05-21 10:50:52, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/674/
> -----------------------------------------------------------
> 
> (Updated 2010-05-21 10:50:52)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Kevin Fleming.
> 
> 
> Summary
> -------
> 
> Background:
> A Digium customer discovered a somewhat odd bug. The setup is that parties A
> and B are bridged, and party A places party B on hold. While party B is 
> listening to hold music, he mashes a bunch of DTMF. Party A takes party
> B off hold while this is happening, but party B continues to hear hold
> music. I could reproduce this about 1 in 5 times.
> 
> The issue:
> When DTMF features are enabled and a user presses keys, the channel that
> the DTMF is streamed to is placed in an ast_safe_sleep for 100 ms, the
> duration of the emulated tone. If an AST_CONTROL_UNHOLD frame is read
> from the channel during the sleep, the frame is dropped. Thus the
> unhold indication is never made to the channel that was originally placed
> on hold.
> 
> The fix:
> Originally, I discussed with Kevin possible ways of fixing the specific
> problem reported. However, we determined that the same type of problem
> could happen in other situations where ast_safe_sleep() is used. Using
> autoservice as a model, I modified ast_safe_sleep_conditional() to
> defer specific frame types so they can be re-queued once the sleep has
> finished. I made a common function for determining if a frame should
> be deferred so that there are not two identical switch blocks to
> maintain.
> 
> I have included Kevin on this review since he and I discussed the 
> original fix together. I also have included Russell since this is
> touching a pretty core part of Asterisk and I figure he would want
> to have a say in this.
> 
> The version attached here is for the 1.4 branch since it is the oldest
> branch affected by this issue. The 1.6.2 branch and trunk are also
> affected.
> 
> 
> This addresses bug ABE-2115.
>     https://issues.asterisk.org/view.php?id=ABE-2115
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/channel.h 264541 
>   /branches/1.4/main/autoservice.c 264452 
>   /branches/1.4/main/channel.c 264541 
> 
> Diff: https://reviewboard.asterisk.org/r/674/diff
> 
> 
> Testing
> -------
> 
> I ran the situation from the Digium customer probably about 30 times and was
> unable to cause the error to occur. In addition, I also ran the test under
> valgrind's memcheck tool and it reported no errors.
> 
> 
> Thanks,
> 
> Mark
> 
>