[asterisk-dev] [Code Review] 2688: ARI: Music on Hold for Bridges

[David Lee]

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



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18202>

    The _hash and _compare callbacks are now more complicated. See https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=25919686



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18203>

    The second if (!cap) is redundant. Remove.



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18204>

    Does the 10 second sleep mean that MOH plays in 10 second increments? If so, maybe 1 second would be more appropriate.



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18205>

    The RAII_VAR should be doing this ao2_cleanup for you.



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18206>

    Race condition. If multiple threads try to get the MOH channel at the same time, you could end up with duplicate MOH channels.
    
    Locak app_bridges_moh around both the find and the create.



/trunk/res/res_stasis.c
<https://reviewboard.asterisk.org/r/2688/#comment18207>

    Another race condition. The find and unlink should both happen with the container's lock held.



/trunk/res/stasis_http/resource_bridges.c
<https://reviewboard.asterisk.org/r/2688/#comment18208>

    I prefer a comment stating that find_bridge fills in the response. Otherwise it just looks like you forgot it.



/trunk/res/stasis_http/resource_bridges.c
<https://reviewboard.asterisk.org/r/2688/#comment18209>

    Same comment here.



/trunk/rest-api/api-docs/bridges.json
<https://reviewboard.asterisk.org/r/2688/#comment18210>

    You need to specify both the 404 and 409 error responses.



/trunk/rest-api/api-docs/bridges.json
<https://reviewboard.asterisk.org/r/2688/#comment18211>

    You need to specify both the 404 and 409 error responses.


- David Lee


On July 25, 2013, 5:53 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2688/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 5:53 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee, Matt Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-21974
>     https://issues.asterisk.org/jira/browse/ASTERISK-21974
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds ARI commands for playing music on hold and stopping music on hold that is played by ARI.
> Note that if music on hold is being played by non-ARI mechanisms, the ARI stop music on hold command will have no effects.
> 
> ARI music on hold for bridges works by creating a dedicated announcer channel for playing music on hold and putting it in the bridge. This channel will play music on hold until it is removed from the bridge at which point it will hang up. Issuing a new music on hold command with one running will fetch that channel and issue a new music on hold command which will do nothing if it uses the same class or will start playing the new class if the music on hold command specifies a class other than the one provided. If the moh stop command is issued and an MOH channel exists for that bridge, it will be hung up.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/stasis_app.h 394810 
>   /trunk/res/res_stasis.c 394810 
>   /trunk/res/res_stasis_http_bridges.c 394810 
>   /trunk/res/stasis_http/resource_bridges.h 394810 
>   /trunk/res/stasis_http/resource_bridges.c 394810 
>   /trunk/rest-api/api-docs/bridges.json 394810 
> 
> Diff: https://reviewboard.asterisk.org/r/2688/diff/
> 
> 
> Testing
> -------
> 
> SIP channels with directmedia=no (which at present doesn't appear to do what it should do)
> 
> Both call into stasis and are placed in a native bridge (becomes native RTP bridge)
> 
> MOH start is issued
> 
> Both channels hear MOH Any new channels at this point will also hear MOH
> 
> MOH stop is issued
> 
> MOH channel gets blown away, moh stops for all channels in the bridge obviously.
> 
> MOH start is issued, yadda yadda
> 
> MOH start with a different class is issued, music on hold changes to a new song
> 
> MOH start is issued with default class. MOH changes to default class.
> 
> MOH stop is issued. Channel gets blown away.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130726/0c902c34/attachment-0001.htm>