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

[jrose]

> On Dec. 11, 2012, 10:25 a.m., jrose wrote:
> > /branches/10/apps/app_confbridge.c, line 868
> > <https://reviewboard.asterisk.org/r/2232/diff/2/?file=32328#file32328line868>
> >
> >     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(...)
> 
> rmudgett wrote:
>     The in_bridge name would be incorrect if I just did
>     in_bridge = ast_bridge_suspend()
>     The name should then be not_in_bridge.
>     Then the code would be
>     if (!not_in_bridge)
>     
>     I try to avoid the use of goto if there is a better way. :)  Also what you are suggesting here would not start/stop MOH if the bridge channel was not in a bridge yet.

I'm pretty sure what I suggested would do the exact same thing as what you are doing now.  If ast_bridge_suspend returns success then it does ast_bridge_suspend followed by the ao2_unlock.  If it failed, it just does the ao2_unlock.

The use of ... indicated stuff that happened between that I simply omitted.

Also that's why I suggested you change the variable name.  I don't really 'in_bridge' is descriptive of the variable since it's a pass/fail thing.


- jrose


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


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/a79ce8a8/attachment-0001.htm>