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

[rmudgett]

> 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(...)

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.


> On Dec. 11, 2012, 10:25 a.m., jrose wrote:
> > /branches/10/apps/app_confbridge.c, lines 857-861
> > <https://reviewboard.asterisk.org/r/2232/diff/2/?file=32328#file32328line857>
> >
> >     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.

The point of the XXX's is that this is technically a bridging API violation.  I have given confbridge implementation detail knowledge that the pointer is to an ao2 object.


- rmudgett


-----------------------------------------------------------
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/75c32203/attachment.htm>