[Asterisk-code-review] (WIP) rtp: Dynamically choose dynamic RTP payload numbers pe... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: (WIP) rtp: Dynamically choose dynamic RTP payload numbers per instance.
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/3679/1/main/rtp_engine.c
File main/rtp_engine.c:

Line 2718
Rather than always picking the first dynamic payload type available, would we want a historic asterisk dynamic assignment preference table to make interacting with older versions more compatible?  The table should be different from static_RTP_PT[] so ast_rtp_codecs_payloads_set_m_type() can allow us to detect if an rtpmap attribute is missing.


Line 1223: static int rtp_codecs_find_empty_dynamic_rx(struct ast_rtp_codecs *codecs)
Since the static_RTP_PT[] now only contains truly static assignments, this routine should be checking if the codec rx mapping position and the static mapping position are unused in the dynamic range.  Besides the unnecessary locking, the routine does this but could be coded better.


Line 1228:         ast_rwlock_rdlock(&static_RTP_PT_lock);
There is only one caller of this function and this rd lock is already held.  Rwlocks are not reentrant.


PS1, Line 1306: 	if (payload >= 0) {
              : 		return payload;
              : 	}
This should be made to return only if (!asterisk_format && payload < 0).  We want the rx payload mapping to contain static assignments.  Otherwise we have to fall back to fetching the format from the static_RTP_PT[] which involves acquiring a different lock.


Line 1321: 	if ((payload > -1) && (payload < AST_RTP_PT_FIRST_DYNAMIC
Payload currently can only be -1 at this point because of your change above returning any staticly assigned type found.  This if test is thus always false.


PS1, Line 2272: 	/*
              : 	 * ARRAY_LEN's result is cast to an int so 'map' is not autocast to a size_t,
              : 	 * which if negative would cause an assertion.
              : 	 */
              : 	ast_assert(map < (int)ARRAY_LEN(static_RTP_PT));
              : 
              : 	/*
              : 	 * An RTP code is required, if not then one will be dynamically allocated
              : 	 * per instance.
              : 	 */
              : 	if (map < 0) {
              : 		return;
              : 	}
A negative map value is now invalid because we no longer find a dynamic position.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7cd8ff6f61562445ad236aa3e606aa3de03739b
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes