[asterisk-dev] [Code Review] Fix MOH on simultaneous confbridge user entry to a new conference.

[jrose]

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

Ship it!


This looks good.  I have a few minor mostly cosmetic nitpicks.


/branches/10/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2232/#comment14303>

    Remove XXX since this comment just seems to describe what is being done and isn't a reminder of future work that needs to be done or clarifications that need to be made or anything like that.



/branches/10/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2232/#comment14305>

    Maybe I'm just naive, but setting a variable to !(some integer) strikes me as a little odd.  Why not simply set it to ast_bridge_suspend and then use
    if (!in_bridge) {
    
    Actually since what we are looking for is a failure, we could also just do it this way:
    
      change variable name to res
    
      ...
    
      res = ast_bridge_suspend(...
    
      ...
    
      if (res) {
        goto bailout_label
      }
    
      ast_bridge_unsuspend(...)
    
    bailout_label:
    
      ao2_unlock(...)



/branches/10/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2232/#comment14304>

    same issue with XXX comment



/branches/10/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2232/#comment14306>

    same business with odd variable name and such since it isn't actually being used to describe any particular bridge and is really just success/failure variable.


- jrose


On Dec. 7, 2012, 9:37 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2232/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2012, 9:37 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When two users enter a new conference simultaneously, one of the callers hears MOH.
> 
> * Created a confbridge internal MOH API to eliminate the inlined MOH handling code.
> Note that the conference mixing bridge needs to be locked when actually starting/stopping MOH because there is a small window between the conference join unsuspend MOH and actually joining the mixing bridge.  Doing the lock this way may be a violation of the bridging API.  I don't see an alternative though.  See the comment in the conf_moh_start()/conf_moh_stop() functions.
> 
> * Created the concept of suspended MOH so it can be interrupted while conference join announcements and DTMF features can operate.
> 
> * Suspend any MOH until the user actually joins the bridge of the conference.  This way any pre-join file playback does not need to worry about MOH.
> 
> * Made post-join actions only play deferred entry announcement files.  Changing the user/conference state during that time is not protected or controlled by the state machine.
> 
> 
> This addresses bug ASTERISK-20606.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20606
> 
> 
> Diffs
> -----
> 
>   /branches/10/apps/app_confbridge.c 377376 
>   /branches/10/apps/confbridge/conf_state.c 377376 
>   /branches/10/apps/confbridge/conf_state_multi_marked.c 377376 
>   /branches/10/apps/confbridge/include/confbridge.h 377376 
> 
> Diff: https://reviewboard.asterisk.org/r/2232/diff
> 
> 
> Testing
> -------
> 
> Simultaneous entry to a new conference does not have one of the users still hearing MOH.
> Simultaneous entry of the following user types:
> unmarked - unmarked -> MOH is no longer played to the first user who "entered"
> waitmarked - marked -> MOH is no longer played to the waitmarked user who "entered" first.
> unmarked - waitmarked -> MOH is played to both users since the conference has not started.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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