[asterisk-dev] [Code Review]: Add a configurable termination threshold to strictrtp's learning mode.

[Matt Jordan]

> On Jan. 16, 2012, 3:43 p.m., Matt Jordan wrote:
> > /branches/1.8/res/res_rtp_asterisk.c, lines 542-563
> > <https://reviewboard.asterisk.org/r/1663/diff/5/?file=23189#file23189line542>
> >
> >     I think this code will still never be executed.
> >     
> >     If rtp->learning_probation is 0, then we've returned 0 from this method, as that is what sets probation to 0.  In that case, we will exit learning mode.
> >     
> >     If we reenter learning mode (at any time), we re-initialize the learning mode counters via rtp_learning_seq_init - which resets rtp->learning_probation to the number of packets we have to see sequentially.
> >     
> >     This code looks useful, but should probably be executed inside the else {...} block of if (seq == rtp->learning_max_seq + 1) {
> >
> 
> jrose wrote:
>     Actually, yeah, it looks like this code is more about anticipating when to enter learning mode than actually about exiting learning mode.  It doesn't need to go anywhere else since we already have our own ways of handling that.  I'll go ahead and get rid of all this stuff for now, if we need it again in the future, it will probably be as part of a much larger pjmedia integration.
> 
> Matt Jordan wrote:
>     I'm not sure that's the right answer here.  For one, if you simply remove this code you'll need to check all of the code that supported it, as there will be a lot of #define's and other variables that won't be used.
>     
>     The first part (udelta < LEARNING_MAX_DROPOUT) isn't about when to exit learning mode.  It is to see if the delta between the current sequence number and the last known sequence number was less then the amount we'd let drop out.  If so, it accepted the value anyways.  That could be useful - you would be more tolerant to dropping a single RTP packet from the source you're tracking - and its fairly unlikely to cause a problem with two sources providing two RTP streams that are within one sequence count of each other.
>     
>     The second part may - or may not be - redundant with the rollover you currently have implemented.  Without looking at the pjmedia code and getting the full context its a bit hard to tell; however, it looks like it allowed some jumping in the sequence if the delta wasn't too large.  There's a good likelihood that this was added to deal with some real world cases where sequence numbers don't arrive exactly how you expect them.

Update to my comment - we compared this against the pjmedia code some more, and determined that the udelta calculations are being done to determine when pjmedia enters into probation mode.  As we rely on the SIP stack to notify us when to enter into probation mode (in our case, learning mode), those mechanisms aren't as applicable.  As such, going with jrose's suggestion of removing those features seems appropriate, but may be something worth looking into as a later enhancement.

The code supporting this was removed in v7 of the patch.


- Matt


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


On Jan. 17, 2012, 10:24 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1663/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2012, 10:24 a.m.)
> 
> 
> Review request for Asterisk Developers, Kevin Fleming, Matt Jordan, and jcolp.
> 
> 
> Summary
> -------
> 
> The purpose of this patch is to resolve some problems that can occur with peers using a mix of directmedia=yes and no occuring between multiple asterisk servers when strictrtp is enabled in rtp.conf.  When attempting to set the strictrtppeer during a reinvite, res_rtp_asterisk will ignore the requested sockaddr and instead set learning mode. Prior to this patch, learning mode would simply set the sockaddr owning the next related RTP packet received and then exit learning mode.  Now, learning mode will track how many times a particular socket address sent rtp packets without being interrupted by another socket address until it reaches the learning mode hit threshold value (set in rtp.conf) and then resume closed mode.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/CHANGES 350939 
>   /branches/1.8/configs/rtp.conf.sample 350939 
>   /branches/1.8/res/res_rtp_asterisk.c 350939 
> 
> Diff: https://reviewboard.asterisk.org/r/1663/diff
> 
> 
> Testing
> -------
> 
> I tested this against two scenarios with the following configurations... always with strictrtp=yes
> 
> s1
> Phone 1 <-- DirectMedia=yes --> Asterisk 1 <-- DirectMedia=Yes --> Asterisk 2 <-- DirectMedia=No --> Phone 3
> 
> and
> 
> s2
> Phone 1 <-- DirectMedia=yes --> Asterisk 1 <-- DirectMedia=Yes --> Phone 2
> and then doing a blind transfer with:
> Phone 2 <-- DirectMedia=yes --> Asterisk 1 <-- DirectMedia=Yes --> Asterisk 2 <-- DirectMedia=No --> Phone 3
> 
> Prior to this test, in s1 there would be one way audio from phone 3 to phone 1 because RTP coming from phone 1 would be rejected by Asterisk 2.
> 
> In s2, audio would work normally from phone 1 to phone 2, but after the transfer the result would be the same with one way audio from phone 3 to phone 1 due to rejected
> packets from phone 1.
> 
> 
> After this patch is applied, both of these scenarios worked as expected as long as I used a threshold above 3.  Phones 1 and 3 were polycom SP430s while Phone 2 was a Grandstream GXP-2020.
> 
> 
> Thanks,
> 
> jrose
> 
>

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