[Asterisk-code-review] rtp engine.c: Fix performance issue with several channel dri... (asterisk[master])
Matt Jordan
asteriskteam at digium.com
Sat Aug 8 08:07:13 CDT 2015
Matt Jordan has submitted this change and it was merged.
Change subject: rtp_engine.c: Fix performance issue with several channel drivers that use RTP.
......................................................................
rtp_engine.c: Fix performance issue with several channel drivers that use RTP.
ast_rtp_codecs_get_payload() gets called once or twice for every received
RTP frame so it would be nice to not allocate an ao2 object to then have
it destroyed shortly thereafter. The ao2 object gets allocated only if
the payload type is not set by the channel driver as a negotiated value.
The issue affects chan_skinny, chan_unistim, chan_rtp, and chan_ooh323.
* Made static_RTP_PT[] an array of ao2 objects that
ast_rtp_codecs_get_payload() can return instead of an array of structs
that must be copied into a created ao2 object.
ASTERISK-25296 #close
Reported by: Richard Mudgett
Change-Id: Icb6de5cd90bfae07d44403a1352963db9109dac0
---
M main/rtp_engine.c
1 file changed, 57 insertions(+), 51 deletions(-)
Approvals:
Anonymous Coward #1000019: Verified
Matt Jordan: Looks good to me, approved
Joshua Colp: Looks good to me, but someone else must approve
diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 587149a..1375897 100644
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -229,7 +229,7 @@
* See http://www.iana.org/assignments/rtp-parameters for a list of
* assigned values
*/
-static struct ast_rtp_payload_type static_RTP_PT[AST_RTP_MAX_PT];
+static struct ast_rtp_payload_type *static_RTP_PT[AST_RTP_MAX_PT];
static ast_rwlock_t static_RTP_PT_lock;
/*! \brief \ref stasis topic for RTP related messages */
@@ -651,23 +651,23 @@
return;
}
- new_type = ast_rtp_engine_alloc_payload_type();
+ ast_rwlock_rdlock(&static_RTP_PT_lock);
+ new_type = ao2_bump(static_RTP_PT[payload]);
+ ast_rwlock_unlock(&static_RTP_PT_lock);
if (!new_type) {
+ ast_debug(1, "Don't have a default tx payload type %d format for m type on %p\n",
+ payload, codecs);
return;
}
- ast_rwlock_rdlock(&static_RTP_PT_lock);
+ ast_debug(1, "Setting tx payload type %d based on m type on %p\n",
+ payload, codecs);
+
ast_rwlock_wrlock(&codecs->codecs_lock);
+
if (payload < AST_VECTOR_SIZE(&codecs->payloads)) {
ao2_t_cleanup(AST_VECTOR_GET(&codecs->payloads, payload), "cleaning up replaced payload type");
}
-
- new_type->asterisk_format = static_RTP_PT[payload].asterisk_format;
- new_type->rtp_code = static_RTP_PT[payload].rtp_code;
- new_type->payload = payload;
- new_type->format = ao2_bump(static_RTP_PT[payload].format);
-
- ast_debug(1, "Setting payload %d (%p) based on m type on %p\n", payload, new_type, codecs);
AST_VECTOR_REPLACE(&codecs->payloads, payload, new_type);
if (instance && instance->engine && instance->engine->payload_set) {
@@ -675,7 +675,6 @@
}
ast_rwlock_unlock(&codecs->codecs_lock);
- ast_rwlock_unlock(&static_RTP_PT_lock);
}
int ast_rtp_codecs_payloads_set_rtpmap_type_rate(struct ast_rtp_codecs *codecs, struct ast_rtp_instance *instance, int pt,
@@ -791,15 +790,8 @@
ast_rwlock_unlock(&codecs->codecs_lock);
if (!type) {
- type = ast_rtp_engine_alloc_payload_type();
- if (!type) {
- return NULL;
- }
ast_rwlock_rdlock(&static_RTP_PT_lock);
- type->asterisk_format = static_RTP_PT[payload].asterisk_format;
- type->rtp_code = static_RTP_PT[payload].rtp_code;
- type->payload = payload;
- type->format = ao2_bump(static_RTP_PT[payload].format);
+ type = ao2_bump(static_RTP_PT[payload]);
ast_rwlock_unlock(&static_RTP_PT_lock);
}
@@ -810,17 +802,24 @@
{
struct ast_rtp_payload_type *type;
- if (payload < 0 || payload >= AST_RTP_MAX_PT) {
+ if (payload < 0 || payload >= AST_RTP_MAX_PT || !format) {
return -1;
}
+ type = ast_rtp_engine_alloc_payload_type();
+ if (!type) {
+ return -1;
+ }
+ ao2_ref(format, +1);
+ type->format = format;
+ type->asterisk_format = 1;
+ type->payload = payload;
+
ast_rwlock_wrlock(&codecs->codecs_lock);
if (payload < AST_VECTOR_SIZE(&codecs->payloads)) {
- type = AST_VECTOR_GET(&codecs->payloads, payload);
- if (type && type->asterisk_format) {
- ao2_replace(type->format, format);
- }
+ ao2_cleanup(AST_VECTOR_GET(&codecs->payloads, payload));
}
+ AST_VECTOR_REPLACE(&codecs->payloads, payload, type);
ast_rwlock_unlock(&codecs->codecs_lock);
return 0;
@@ -923,12 +922,15 @@
if (payload < 0) {
ast_rwlock_rdlock(&static_RTP_PT_lock);
for (i = 0; i < AST_RTP_MAX_PT; i++) {
- if (static_RTP_PT[i].asterisk_format && asterisk_format && format &&
- (ast_format_cmp(format, static_RTP_PT[i].format) != AST_FORMAT_CMP_NOT_EQUAL)) {
+ if (!static_RTP_PT[i]) {
+ continue;
+ }
+ if (static_RTP_PT[i]->asterisk_format && asterisk_format && format &&
+ (ast_format_cmp(format, static_RTP_PT[i]->format) != AST_FORMAT_CMP_NOT_EQUAL)) {
payload = i;
break;
- } else if (!static_RTP_PT[i].asterisk_format && !asterisk_format &&
- (static_RTP_PT[i].rtp_code == code)) {
+ } else if (!static_RTP_PT[i]->asterisk_format && !asterisk_format &&
+ (static_RTP_PT[i]->rtp_code == code)) {
payload = i;
break;
}
@@ -1695,16 +1697,6 @@
/*! \internal
* \brief Small helper routine that cleans up entry i in
- * \c static_RTP_PT.
- */
-static void rtp_engine_static_RTP_PT_cleanup(int i)
-{
- ao2_cleanup(static_RTP_PT[i].format);
- memset(&static_RTP_PT[i], 0, sizeof(struct ast_rtp_payload_type));
-}
-
-/*! \internal
- * \brief Small helper routine that cleans up entry i in
* \c ast_rtp_mime_types.
*/
static void rtp_engine_mime_type_cleanup(int i)
@@ -1744,6 +1736,7 @@
static void add_static_payload(int map, struct ast_format *format, int rtp_code)
{
int x;
+ struct ast_rtp_payload_type *type;
ast_assert(map < ARRAY_LEN(static_RTP_PT));
@@ -1751,24 +1744,36 @@
if (map < 0) {
/* find next available dynamic payload slot */
for (x = AST_RTP_PT_FIRST_DYNAMIC; x < AST_RTP_MAX_PT; ++x) {
- if (!static_RTP_PT[x].asterisk_format && !static_RTP_PT[x].rtp_code) {
+ if (!static_RTP_PT[x]) {
map = x;
break;
}
}
if (map < 0) {
- ast_log(LOG_WARNING, "No Dynamic RTP mapping available for format %s\n",
- ast_format_get_name(format));
+ if (format) {
+ ast_log(LOG_WARNING, "No Dynamic RTP mapping available for format %s\n",
+ ast_format_get_name(format));
+ } else {
+ ast_log(LOG_WARNING, "No Dynamic RTP mapping available for RTP code %d\n",
+ rtp_code);
+ }
ast_rwlock_unlock(&static_RTP_PT_lock);
return;
}
}
- if (format) {
- static_RTP_PT[map].asterisk_format = 1;
- static_RTP_PT[map].format = ao2_bump(format);
- } else {
- static_RTP_PT[map].rtp_code = rtp_code;
+ type = ast_rtp_engine_alloc_payload_type();
+ if (type) {
+ if (format) {
+ ao2_ref(format, +1);
+ type->format = format;
+ type->asterisk_format = 1;
+ } else {
+ type->rtp_code = rtp_code;
+ }
+ type->payload = map;
+ ao2_cleanup(static_RTP_PT[map]);
+ static_RTP_PT[map] = type;
}
ast_rwlock_unlock(&static_RTP_PT_lock);
}
@@ -1797,8 +1802,10 @@
ast_rwlock_wrlock(&static_RTP_PT_lock);
/* remove everything pertaining to this format id from the lists */
for (x = 0; x < AST_RTP_MAX_PT; x++) {
- if (ast_format_cmp(static_RTP_PT[x].format, format) == AST_FORMAT_CMP_EQUAL) {
- rtp_engine_static_RTP_PT_cleanup(x);
+ if (static_RTP_PT[x]
+ && ast_format_cmp(static_RTP_PT[x]->format, format) == AST_FORMAT_CMP_EQUAL) {
+ ao2_ref(static_RTP_PT[x], -1);
+ static_RTP_PT[x] = NULL;
}
}
ast_rwlock_unlock(&static_RTP_PT_lock);
@@ -2087,9 +2094,8 @@
ast_rwlock_wrlock(&static_RTP_PT_lock);
for (x = 0; x < AST_RTP_MAX_PT; x++) {
- if (static_RTP_PT[x].format) {
- rtp_engine_static_RTP_PT_cleanup(x);
- }
+ ao2_cleanup(static_RTP_PT[x]);
+ static_RTP_PT[x] = NULL;
}
ast_rwlock_unlock(&static_RTP_PT_lock);
--
To view, visit https://gerrit.asterisk.org/1017
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Icb6de5cd90bfae07d44403a1352963db9109dac0
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
More information about the asterisk-code-review
mailing list