[asterisk-dev] [Code Review] 2465: Bridging Native RTP Support

[Mark Michelson]

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


A couple of high-level notes:

1) I'd suggest adding XXX or BUGBUG comments to any places you've touched where ast_bridged_channel() is called. The reason is that currently, this always returns NULL, no matter the current bridging state of a channel. I don't know if there is a long-term goal to make ast_bridged_channel() return a channel if there is only one bridge peer, or if that function will die eventually. But having comments will direct attention to the fact that things might be a bit messed up at the moment. It looked like mixmonitor and chanspy could still work okay, but it appears that directrtpsetup in chan_sip will not work at the moment, even with your addition of checking ast_channel_internal_bridge()

2) I'm surprised that there isn't a ton of code removed from res_rtp_asterisk.c and rtp_engine.c. Is all of the native bridging code from those files still necessary, even with the changes you've made here?


/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16044>

    Though it would be a rare occasion, it may be the channel has an audiohook list, but it's empty. You should allow native RTP bridging if that's the case.



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16064>

    Can you flesh this description out some more? It appears this function is intended to determine what type of native bridging (if any) can be used between c0 and c1. The problem is that this has the capability of stating that native bridging is possible even if c1 doesn't even exist, or if c0 and c1 have differing native bridging capabilities.



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16063>

    Returning 0 in this function is odd since the return type is enum ast_rtp_glue_result



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16045>

    The debug line gets the name of the incorrect channel. It should be c1 instead of c0.



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16048>

    Make this static since there's no need to rebuild the frame hook interface each time this is called.



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16072>

    This function appears to be called for each individual bridge_channel that joins the native bridge. Why do you operate on both channels in the bridge every time this function is called? It seems like everything will happen twice as a result of this behavior.



/team/group/bridge_construction/bridges/bridge_native_rtp.c
<https://reviewboard.asterisk.org/r/2465/#comment16073>

    Should you do some sort of check to make sure that c0 and c1 aren't the same channel? Otherwise, later on, you'll end up calling the instance->local_bridge and glue->update_peer() methods twice on the same channel.
    
    Like with the join code, I'm curious why you operate on both channels in the bridge instead of just the one leaving the bridge.


- Mark Michelson


On April 23, 2013, 4:06 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2465/
> -----------------------------------------------------------
> 
> (Updated April 23, 2013, 4:06 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change implements the following:
> 
> 1. Adds a native RTP bridge technology which does local or remote bridging depending on conditions
> 2. Makes the bridging core aware of native bridges
> 3. Calls into the compatible callback of bridging technologies when present to check compatibility
> 4. Tweaks some logic to cause the bridge to reconfigure when external conditions influence it
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/main/rtp_engine.c 386345 
>   /team/group/bridge_construction/main/bridging.c 386345 
>   /team/group/bridge_construction/channels/chan_sip.c 386345 
>   /team/group/bridge_construction/include/asterisk/rtp_engine.h 386345 
>   /team/group/bridge_construction/bridges/bridge_native_rtp.c PRE-CREATION 
>   /team/group/bridge_construction/apps/app_chanspy.c 386345 
>   /team/group/bridge_construction/apps/app_mixmonitor.c 386345 
> 
> Diff: https://reviewboard.asterisk.org/r/2465/diff/
> 
> 
> Testing
> -------
> 
> 1. Tested that non-RTP channels do not cause the native RTP bridge to be used
> 2. Tested that if conditions allow it that remote bridging (ala reinvite) occurs
> 3. Tested that if remote bridging is not possible that local bridging occurs
> 4. Tested that if conditions are not correct none of the above happens
> 5. Tested that external applications cause the bridge to be reconfigured, and alternate technology used if needed
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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