[asterisk-dev] [Code Review] Revert contantssrc change in favor of a more focused fix

[Terry Wilson]

> On 2010-03-05 10:39:18, Kevin Fleming wrote:
> > /trunk/include/asterisk/rtp_engine.h, lines 97-98
> > <https://reviewboard.asterisk.org/r/540/diff/3/?file=8480#file8480line97>
> >
> >     This is out of sync with trunk; the AST_RTP_PROPERTY_MAX value must be the last one in the enum.

As mentioned above, already fixed. :-)


> On 2010-03-05 10:39:18, Kevin Fleming wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 1961-1962
> > <https://reviewboard.asterisk.org/r/540/diff/3/?file=8481#file8481line1961>
> >
> >     Can you document what the purpose of this 'data' element of the SRCUPDATE frame is? I don't think I've seen that usage in the tree before.

Will do. Basically to avoid coming up with yet another control frame that basically would be AST_CONTROL_SRCUPDATE_BUT_THIS_TIME_CHANGE_THE_SSRC_TOO, we use the ast_queue_control_data()-like creation of the control frame so that when the control frame is read from chan_sip, we can differentiate between a SRCUPDATE which requires an ssrc change and one that just requires the marker bit to be set.


> On 2010-03-05 10:39:18, Kevin Fleming wrote:
> > /trunk/res/res_rtp_asterisk.c, lines 665-669
> > <https://reviewboard.asterisk.org/r/540/diff/3/?file=8481#file8481line665>
> >
> >     How does this differ from what ast_rtp_new_source() does?

The main difference is the use of AST_RTP_PROPERTY_UPDATE_SSRC instead of AST_RTP_PROPERTY_CONSTANT_SSRC. The other part of the change is to add some debugging info. The real change of behavior comes from the rest of the code change, and the property name just makes it clear that the default behavior is to not change.


- Terry


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


On 2010-03-05 11:47:25, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/540/
> -----------------------------------------------------------
> 
> (Updated 2010-03-05 11:47:25)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> This change basically reverts the change reviewed in https://reviewboard.asterisk.org/r/374/ and instead limits the updating of the RTP synchronization source to only those times when we detect that the other side of the conversation has changed the ssrc.
> 
> The problem is that SRCUPDATE control frames are sent many times where we don't want a new ssrc, including whenever Asterisk has to send DTMF in a normal bridge. This is also not the first time that this mistake has been made. The initial implementation of the ast_rtp_new_source function also changed the ssrc--and then it was removed because of this same issue. Then, we put it back in again to fix a different issue. Instead, I think we should make a very limited change and just update the ssrc when we see it change.
> 
> This issue affects mantis issues 5413 (srtp doesn't like the ssrc changing), 15642 (sonus dtmf), 16292 (Exchange UM dtmf), and probably 16438 and 16714.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 244504 
>   /trunk/include/asterisk/rtp_engine.h 244504 
>   /trunk/res/res_rtp_asterisk.c 244504 
> 
> Diff: https://reviewboard.asterisk.org/r/540/diff
> 
> 
> Testing
> -------
> 
> So far...it compiles. I'm trying to devise a simple way to replicate the original issue which was a reinvite sent to asterisk which results in an ssrc change in the rtp sent to asterisk.
> 
> 
> Thanks,
> 
> Terry
> 
>