[Asterisk-code-review] res rtp asterisk.c: Fix rtp source address learning for brok... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/7237 )

Change subject: res_rtp_asterisk.c: Fix rtp source address learning for broken clients
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@117
PS1, Line 117: #define DEFAULT_LEARNING_MIN_DURATION 30
Should express this value in terms of DEFAULT_LEARNING_MIN_SEQUENTIAL since they are related.  Also should have another macro to calculate the value.

/*!
 * \brief Calculate the min learning duration in ms.
 *
 * \details
 * The min supported packet size represents 10 ms and we need to account
 * for some jitter and fast clocks while learning.  Some messed up devices
 * have very bad jitter for a small packet sample size.  Jitter can also
 * be introduced by the network itself.
 *
 * So we'll allow packets to come in every 9ms on average for fast clocking
 * with the last one coming in 5ms early for jitter.
 */
#define CALC_LEARNING_MIN_DURATION(count) (((count) - 1) * 9 - 5)

#define DEFAULT_LEARNING_MIN_DURATION CALC_LEARNING_MIN_DURATION(DEFAULT_LEARNING_MIN_SEQUETIAL)


https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@2786
PS1, Line 2786: {	
red blob


https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@2795
PS1, Line 2795: 	
red blob


https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@2796
PS1, Line 2796: 	/*
              : 	 * During the learning mode the minimum amount of media we'll accept is
              : 	 * 10ms. So check whether we have received the four packets in less than
              : 	 * a reasonable amount of time. */
/*
 * Protect against packet floods by checking that we
 * received the packet sequence in at least the minimum
 * allowed time.
 */


https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@6555
PS1, Line 6555: 		if ((sscanf(s, "%d", &learning_min_sequential) <= 0) || learning_min_sequential <= 0) {
Setting learning_min_sequential to 1 is not good.  It would set the learning_min_duration to an invalid value.  The minimum needs to be at least 2.


https://gerrit.asterisk.org/#/c/7237/1/res/res_rtp_asterisk.c@6560
PS1, Line 6560: 		learning_min_duration = (learning_min_sequential - 1) * 10;
learning_min_duration = CALC_LEARNING_MIN_DURATION(learning_min_sequential);



-- 
To view, visit https://gerrit.asterisk.org/7237
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: If778fe07678a6fd2041eaca7cd78267d0ef4fc6c
Gerrit-Change-Number: 7237
Gerrit-PatchSet: 1
Gerrit-Owner: Pirmin Walthert <infos at nappsoft.ch>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 17 Nov 2017 02:28:13 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171116/efd7b70e/attachment-0001.html>