[asterisk-dev] [Code Review] 2459: bridge_construction: apply Jitterbuffers at bridge join via the JITTERBUFFER function.

[jrose]

> On April 18, 2013, 9:35 p.m., Mark Michelson wrote:
> > The behavior of this doesn't seem correct to me. If I've set up jitter buffer parameters in a channel driver config file, but I then use JITTERBUFFER in the dialplan to set up different parameters for the particular call, I'd expect that the jitter buffer set up by JITTERBUFFER would be the one in effect on the bridge. However, instead what happens is that the bridge always uses the configuration as set in the channel driver configuration. This behavior is especially bothersome if someone has enabled jitter buffers in their channel driver config but then used the JITTERBUFFER function to attempt to disable the jitter buffer.
> > 
> > One potential way of fixing the problem would be to call ast_jb_enable_for_channel() during channel creation and remove the call to ast_jb_enable_for_channel() from app_confbridge. This way, the jitter buffer as configured by the channel driver is first applied at channel creation, and then any jitter buffer created by JITTERBUFFER can override the initial jitter buffer configuration. When the channel enters the bridge, the bridge will not need to worry about trying to apply a jitter buffer because it will already be there if configured.
> 
> Mark Michelson wrote:
>     The second paragraph's first sentence should say bridging.c instead of app_confbridge. My mistake.
> 
> jrose wrote:
>     I discussed this behavior with Matt earlier today and his opinion was that as long as it's consistent, it's alright. I already knew of a perfectly feasible way to fix it simply by providing an option to ast_jb_create_framehook which would reject the new jitterbuffer configuration if one was found from the datastore instead of wiping the one pointed to by the datastore and replacing it. In such a scenario, jitterbuffers applied by bridges would always use this option.
>     
>     We don't want to create the jitterbuffer on channel creation as that would get in the way of native bridging.
> 
> Matt Jordan wrote:
>     I'm fine with either way, as long as it's consistent and documented.
>     
>     However, simply preventing any new jitterbuffer configuration doesn't solve Mark's problem either - it only solves it on the initial bridge join. If a channel goes into a bridge and gets a channel configuration provided jitterbuffer, func_jitterbuffer can no longer replace or change the channel provided one. This is the same situation as now.
>     
>     Channel configuration provided jitter buffers are only being supported for legacy reasons: they are not the preferred way of putting jitter buffers on channels. Mixing and matching the two is already odd - I'm more concerned that the behavior is documented and predictable than anything else.
> 
> jrose wrote:
>     > However, simply preventing any new jitterbuffer configuration doesn't solve Mark's problem
>     > either - it only solves it on the initial bridge join. If a channel goes into a bridge and
>     > gets a channel configuration provided jitterbuffer, func_jitterbuffer can no longer replace
>     > or change the channel provided one. This is the same situation as now.
>     
>     That's why preventing the override would be an option to the function. JITTERBUFFER simply
>     wouldn't use the option in the function call and would still swap out the jitterbuffer
>     unconditionally.

*'an option to the function' should be 'an option to the ast_jb_create_framehook function'


- jrose


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


On April 18, 2013, 7:52 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2459/
> -----------------------------------------------------------
> 
> (Updated April 18, 2013, 7:52 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21333
>     https://issues.asterisk.org/jira/browse/ASTERISK-21333
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch adds support for applying jitterbuffers based on channel settings for jitterbuffers when bridging channels with simple bridges and softmixed bridges by use of func_jitterbuffer.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 386018 
>   /team/group/bridge_construction/configs/confbridge.conf.sample 386018 
>   /team/group/bridge_construction/funcs/func_jitterbuffer.c 386018 
>   /team/group/bridge_construction/include/asterisk/abstract_jb.h 386018 
>   /team/group/bridge_construction/main/abstract_jb.c 386018 
>   /team/group/bridge_construction/main/bridging.c 386018 
> 
> Diff: https://reviewboard.asterisk.org/r/2459/diff/
> 
> 
> Testing
> -------
> 
> Tested simple two way calls with SIP with the SIP option jbenable on and off.  While on the jitterbuffer is applied as expected and while off it is not applied. Note that this is different from behavior in trunk. In trunk if a SIP channel calls another SIP channel with jbenabled=yes, there generally will not be a jitterbuffer created unless jbforced is set. This is because trunk checks both channels in a two way bridge for the following condition:
> 
> if (((!c0_wants_jitter && c1_creates_jitter) || (c0_force_jb && c1_creates_jitter)) && c0_jb_enabled) {
>    /* jitter buffer creation flag is raised for c0 */
>    ...
> }
> 
> and since SIP has the wants_jitter flag enabled, the jitter buffer is not applied.
> 
> We are no longer reaching across the bridge to look at both channels when doing this, so the jitter buffer will be applied unconditionally on bridging. This means the jbforced option is no longer respected.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130418/b731230b/attachment.htm>