[asterisk-dev] [Code Review]: ast_indicate(chan, -1) don't stop playing tones

[rmudgett]

> On Nov. 3, 2011, 4:32 p.m., schmidts wrote:
> > branches/1.8/main/channel.c, lines 4392-4394
> > <https://reviewboard.asterisk.org/r/1559/diff/1/?file=21572#file21572line4392>
> >
> >     i am not sure if its a good idea to only look at the _condition and stop playtones.
> >     AFAIK chan->tech->indicate looks mostly on the visible_indication which is here set to 0 if _condition is < 0.
> >     
> >     this could means chan->tech->indicate is doing everything fine with indication 0 but you maybe do stop_playtones twice and i dont know if this could lead to other problems.
> >     
> >     if this is a reproduceable problem you should try to see what happens in the used chan->tech->indicate function. maybe you can see some difference there between condition and visible_indication.
> >     
> >
> 
> may213 wrote:
>     Thank for review.
>     
>     Neither channel driver look to visible_indication:
>     
>     may at vl:~/asterisk/x/original/trunk> find channels/ -name '*.[ch]' | xargs grep visible_indication
>     may at vl:~/asterisk/x/original/trunk>
>     
>     
>     ast_playtones_stop just call ast_deactivate_generator that remove generator only if exists so
>     there will not problem even if it will called twice:
>     
>     ----
>     void ast_deactivate_generator(struct ast_channel *chan)
>     {
>             ast_channel_lock(chan);
>             if (chan->generatordata) {
>             ....
>             }
>             ast_channel_unlock(chan);
>     ----
>     
>     br
>

The visible_indication is used by masquerade so it can maintain visible tones like ringback on the new channel.  Channel drivers don't care about it since they are concerned with the current indication being processed.


- rmudgett


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


On Nov. 2, 2011, 2:47 a.m., may213 wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1559/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2011, 2:47 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ast_indicate_data function must call ast_stop_playtones when condition arg is negative, but don't call ast_stop_playtones if indicate function of channel driver return zero result.
> 
> If channel->tech->indicate return zero then processing go to indicate_cleanup before processing negative condition arg, it's bug
> because we must stop tone generator if condition arg is negative independent of returned from tech->indicate value.
> 
> The patch call ast_stop_playtones before processing indicate return value.
> 
> 
> This addresses bug 18803.
>     https://issues.asterisk.org/jira/browse/18803
> 
> 
> Diffs
> -----
> 
>   branches/1.8/main/channel.c 338895 
> 
> Diff: https://reviewboard.asterisk.org/r/1559/diff
> 
> 
> Testing
> -------
> 
> app_queue work correctly with 'r' option on dahdi channel with patch.
> 
> 
> Thanks,
> 
> may213
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111107/6cd277b0/attachment.htm>