[asterisk-dev] [Code Review] 3379: ARI: Make bridges/{bridgeId}/play queue sound files if sounds are already playing on the bridge instead of playing them simultaneously as they are called

[Matt Jordan]

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



/branches/12/include/asterisk/stasis_app.h
<https://reviewboard.asterisk.org/r/3379/#comment21051>

    If you're going to break parameters to functions up everywhere else, you may as well do it here as well. In particular here, since this one actually exceeds the column width in the guidelines.



/branches/12/res/res_ari_bridges.c
<https://reviewboard.asterisk.org/r/3379/#comment21052>

    If it isn't already noted, this needs to be in the CHANGES file.
    
    



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3379/#comment21053>

    bridge_id really doesn't need to be a RAII_VAR.
    
    (1) If you don't allocate it, you don't need to do anything
    (2) If ast_bridge_set_after_callback succeeds, then it *will* attempt to delete the memory pointed to by bridge_id when the channel leaves the bridge. As soon as that succeeds, you must no longer attempt to free it.
    (3) If ast_bridge_set_after_callback fails, free it and bail.
    
    There's only one off nominal path that needs the free, and you never free on the nominal path.
    
    This makes the code a bit clearer as to who owns the memory, as setting bridge_id to NULL in order to prevent the free on leaving the scope of this function is a bit of an odd pattern.



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3379/#comment21054>

    This is a global ao2 container. How are we able to insert an item into it without locking it?



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3379/#comment21055>

    What's the point of using a SCOPED_AO2LOCK here?
    
    Why wouldn't you just let ao2_find lock the container?


- Matt Jordan


On March 25, 2014, 5:14 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3379/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:14 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22677
>     https://issues.asterisk.org/jira/browse/ASTERISK-22677
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Previously, if you played an audio file and then played another before the first finished, the second audio file would start playing immediately as it was called overlapping the previous sound. Apparently people don't like that. This patch changes that behavior so that the sound will be queued at the end of any existing controls if they are running.
> 
> 
> Diffs
> -----
> 
>   /branches/12/rest-api/api-docs/bridges.json 411187 
>   /branches/12/res/stasis/control.c 411187 
>   /branches/12/res/stasis/control.h 411187 
>   /branches/12/res/res_stasis_playback.c 411187 
>   /branches/12/res/res_stasis.c 411187 
>   /branches/12/res/res_ari_bridges.c 411187 
>   /branches/12/res/ari/resource_bridges.c 411187 
>   /branches/12/res/ari/resource_bridges.h 411187 
>   /branches/12/include/asterisk/stasis_app.h 411187 
> 
> Diff: https://reviewboard.asterisk.org/r/3379/diff/
> 
> 
> Testing
> -------
> 
> Tested for playback channel wrapper leaks, tested to make sure control objects were being destroyed when they fell out of use.  Tested playing of a single file. Tested playing of multiple files in a row. Tested playing of multiple files in a row and then after a sequence finished, playing additional files so that new channels would have to be created. Tested playing sounds right as other sounds were concluding. I wasn't able to break it (although I wouldn't be surprised if there is a possible condition where you can grab a control as it is finishing up its queue and then attempting to add a sound to a finished queue causing the playback to fail. I don't think this would break things in a profound way, it just might possibly make one sound fail to queue under extremely unlikely conditions).
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140327/87ed9117/attachment-0001.html>