[Asterisk-code-review] rtp: Only accept the first payload for a format in SDP. (asterisk[master])

[Joshua Colp]

Joshua Colp has uploaded a new change for review.

  https://gerrit.asterisk.org/3896

Change subject: rtp: Only accept the first payload for a format in SDP.
......................................................................

rtp: Only accept the first payload for a format in SDP.

When receiving an SDP offer with multiple payloads for
the same format we would generate an answer with the first
payload, but during the payload crossover operation
(to set the payloads for receiving) we would remove all
payloads but the last. This would result in incoming
traffic being matched against the wrong format and outgoing
traffic being sent using the wrong payload.

This change makes it so that one a format has a payload
number put into the mapping all subsequent ones are ignored.
This ensures there is only ever one payload in the mapping
and that it is the payload placed into the answer SDP.

ASTERISK-26365 #close

Change-Id: I1e8150860a3518cab36d00b1fab50f9352b64e60
---
M main/rtp_engine.c
1 file changed, 34 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/96/3896/1

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 0671374..c90c1d5 100644
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -753,18 +753,18 @@
 
 /*!
  * \internal
- * \brief Remove other matching payload mappings.
+ * \brief Determine if a payload type is already present in mappings
  * \since 14.0.0
  *
- * \param codecs Codecs that need tx mappings removed.
- * \param instance RTP instance to notify of any payloads removed.
+ * \param codecs Codecs to be checked for mappings.
  * \param to_match Payload type object to compare against.
  *
  * \note It is assumed that codecs is write locked before calling.
  *
- * \return Nothing
+ * \retval 0 not found
+ * \retval 1 found
  */
-static void payload_mapping_tx_remove_other_mappings(struct ast_rtp_codecs *codecs, struct ast_rtp_instance *instance, struct ast_rtp_payload_type *to_match)
+static int payload_mapping_tx_is_present(const struct ast_rtp_codecs *codecs, const struct ast_rtp_payload_type *to_match)
 {
 	int idx;
 	struct ast_rtp_payload_type *current;
@@ -772,28 +772,24 @@
 	for (idx = 0; idx < AST_VECTOR_SIZE(&codecs->payload_mapping_tx); ++idx) {
 		current = AST_VECTOR_GET(&codecs->payload_mapping_tx, idx);
 
-		if (!current || current == to_match) {
+		if (!current) {
 			continue;
-		}
-		if (current->asterisk_format && to_match->asterisk_format) {
+		} else if (current->asterisk_format && to_match->asterisk_format) {
 			if (ast_format_cmp(current->format, to_match->format) == AST_FORMAT_CMP_NOT_EQUAL) {
 				continue;
+			} else if (current->payload == to_match->payload) {
+				return 0;
 			}
 		} else if (!current->asterisk_format && !to_match->asterisk_format) {
 			if (current->rtp_code != to_match->rtp_code) {
 				continue;
 			}
-		} else {
-			continue;
 		}
 
-		/* Remove other mapping */
-		AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, idx, NULL);
-		ao2_ref(current, -1);
-		if (instance && instance->engine && instance->engine->payload_set) {
-			instance->engine->payload_set(instance, idx, 0, NULL, 0);
-		}
+		return 1;
 	}
+
+	return 0;
 }
 
 /*!
@@ -833,8 +829,6 @@
 		if (instance && instance->engine && instance->engine->payload_set) {
 			instance->engine->payload_set(instance, idx, type->asterisk_format, type->format, type->rtp_code);
 		}
-
-		payload_mapping_tx_remove_other_mappings(dest, instance, type);
 	}
 }
 
@@ -921,17 +915,19 @@
 
 	ast_rwlock_wrlock(&codecs->codecs_lock);
 
-	if (payload < AST_VECTOR_SIZE(&codecs->payload_mapping_tx)) {
-		ao2_t_cleanup(AST_VECTOR_GET(&codecs->payload_mapping_tx, payload),
-			"cleaning up replaced tx payload type");
-	}
-	AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, payload, new_type);
+	if (!payload_mapping_tx_is_present(codecs, new_type)) {
+		if (payload < AST_VECTOR_SIZE(&codecs->payload_mapping_tx)) {
+			ao2_t_cleanup(AST_VECTOR_GET(&codecs->payload_mapping_tx, payload),
+				"cleaning up replaced tx payload type");
+		}
+		AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, payload, new_type);
 
-	if (instance && instance->engine && instance->engine->payload_set) {
-		instance->engine->payload_set(instance, payload, new_type->asterisk_format, new_type->format, new_type->rtp_code);
+		if (instance && instance->engine && instance->engine->payload_set) {
+			instance->engine->payload_set(instance, payload, new_type->asterisk_format, new_type->format, new_type->rtp_code);
+		}
+	} else {
+		ao2_ref(new_type, -1);
 	}
-
-	payload_mapping_tx_remove_other_mappings(codecs, instance, new_type);
 
 	ast_rwlock_unlock(&codecs->codecs_lock);
 }
@@ -995,17 +991,20 @@
 			new_type->format = ast_format_parse_sdp_fmtp(new_type->format, "");
 		}
 
-		if (pt < AST_VECTOR_SIZE(&codecs->payload_mapping_tx)) {
-			ao2_t_cleanup(AST_VECTOR_GET(&codecs->payload_mapping_tx, pt),
-				"cleaning up replaced tx payload type");
-		}
-		AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, pt, new_type);
+		if (!payload_mapping_tx_is_present(codecs, new_type)) {
+			if (pt < AST_VECTOR_SIZE(&codecs->payload_mapping_tx)) {
+				ao2_t_cleanup(AST_VECTOR_GET(&codecs->payload_mapping_tx, pt),
+					"cleaning up replaced tx payload type");
+			}
+			AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, pt, new_type);
 
-		if (instance && instance->engine && instance->engine->payload_set) {
-			instance->engine->payload_set(instance, pt, new_type->asterisk_format, new_type->format, new_type->rtp_code);
+			if (instance && instance->engine && instance->engine->payload_set) {
+				instance->engine->payload_set(instance, pt, new_type->asterisk_format, new_type->format, new_type->rtp_code);
+			}
+		} else {
+			ao2_ref(new_type, -1);
 		}
 
-		payload_mapping_tx_remove_other_mappings(codecs, instance, new_type);
 		break;
 	}
 
@@ -1092,7 +1091,6 @@
 		ao2_cleanup(AST_VECTOR_GET(&codecs->payload_mapping_tx, payload));
 	}
 	AST_VECTOR_REPLACE(&codecs->payload_mapping_tx, payload, type);
-	payload_mapping_tx_remove_other_mappings(codecs, NULL, type);
 	ast_rwlock_unlock(&codecs->codecs_lock);
 
 	return 0;

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e8150860a3518cab36d00b1fab50f9352b64e60
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>