[Asterisk-code-review] translate: Provide translation modules the result of SDP neg... (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: translate: Provide translation modules the result of SDP negotiation.
......................................................................


translate: Provide translation modules the result of SDP negotiation.

Previously, a trancoding module did not have access to the joint but cached
format. Therefore, the module did not have access to the attributes negotiated
via SDP (line fmtp). Now, a translation module receives the joint format.

ASTERISK-25545 #close

Change-Id: Id6878a989b50573298dab115d3371ea369e1a718
---
M include/asterisk/translate.h
M main/translate.c
2 files changed, 34 insertions(+), 5 deletions(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved



diff --git a/include/asterisk/translate.h b/include/asterisk/translate.h
index 87e9a2c..b8cd219 100644
--- a/include/asterisk/translate.h
+++ b/include/asterisk/translate.h
@@ -223,6 +223,14 @@
 	struct ast_trans_pvt *next; /*!< next in translator chain */
 	struct timeval nextin;
 	struct timeval nextout;
+	/*! If a translation path using a format with attributes requires the output
+	 * to be a specific set of attributes, this variable will be set describing
+	 * those attributes to the translator. Otherwise, the translator must choose
+	 * a set of format attributes for the destination that preserves the quality
+	 * of the audio in the best way possible. For example with the Opus Codec,
+	 * explicit_dst contains an attribute which describes whether both parties
+	 * want to do forward-error correction (FEC). */
+	struct ast_format *explicit_dst;
 };
 
 /*! \brief generic frameout function */
diff --git a/main/translate.c b/main/translate.c
index 334d3b5..61a827b 100644
--- a/main/translate.c
+++ b/main/translate.c
@@ -298,6 +298,10 @@
 		t->destroy(pvt);
 	}
 	ao2_cleanup(pvt->f.subclass.format);
+	if (pvt->explicit_dst) {
+		ao2_ref(pvt->explicit_dst, -1);
+		pvt->explicit_dst = NULL;
+	}
 	ast_free(pvt);
 	ast_module_unref(t->module);
 }
@@ -306,7 +310,7 @@
  * \brief Allocate the descriptor, required outbuf space,
  * and possibly desc.
  */
-static struct ast_trans_pvt *newpvt(struct ast_translator *t)
+static struct ast_trans_pvt *newpvt(struct ast_translator *t, struct ast_format *explicit_dst)
 {
 	struct ast_trans_pvt *pvt;
 	int len;
@@ -332,6 +336,12 @@
 	if (t->buf_size) {/* finally buffer and header */
 		pvt->outbuf.c = ofs + AST_FRIENDLY_OFFSET;
 	}
+	/*
+	 * If the format has an attribute module, explicit_dst includes the (joined)
+	 * result of the SDP negotiation. For example with the Opus Codec, the format
+	 * knows whether both parties want to do forward-error correction (FEC).
+	 */
+	pvt->explicit_dst = ao2_bump(explicit_dst);
 
 	ast_module_ref(t->module);
 
@@ -349,9 +359,16 @@
 	pvt->f.src = pvt->t->name;
 	pvt->f.data.ptr = pvt->outbuf.c;
 
-	/* if the translator has not provided a format find one in the cache or create one */
+	/*
+	 * If the translator has not provided a format
+	 * A) use the joined one,
+	 * B) use the cached one, or
+	 * C) create one.
+	 */
 	if (!pvt->f.subclass.format) {
-		if (!ast_strlen_zero(pvt->t->format)) {
+		pvt->f.subclass.format = ao2_bump(pvt->explicit_dst);
+
+		if (!pvt->f.subclass.format && !ast_strlen_zero(pvt->t->format)) {
 			pvt->f.subclass.format = ast_format_cache_get(pvt->t->format);
 		}
 
@@ -477,6 +494,7 @@
 
 	while (src_index != dst_index) {
 		struct ast_trans_pvt *cur;
+		struct ast_format *explicit_dst = NULL;
 		struct ast_translator *t = matrix_get(src_index, dst_index)->step;
 		if (!t) {
 			ast_log(LOG_WARNING, "No translator path from %s to %s\n",
@@ -484,7 +502,10 @@
 			AST_RWLIST_UNLOCK(&translators);
 			return NULL;
 		}
-		if (!(cur = newpvt(t))) {
+		if ((t->dst_codec.sample_rate == ast_format_get_sample_rate(dst)) && (t->dst_codec.type == ast_format_get_type(dst)) && (!strcmp(t->dst_codec.name, ast_format_get_name(dst)))) {
+			explicit_dst = dst;
+		}
+		if (!(cur = newpvt(t, explicit_dst))) {
 			ast_log(LOG_WARNING, "Failed to build translator step from %s to %s\n",
 				ast_format_get_name(src), ast_format_get_name(dst));
 			if (head) {
@@ -638,7 +659,7 @@
 		return;
 	}
 
-	pvt = newpvt(t);
+	pvt = newpvt(t, NULL);
 	if (!pvt) {
 		ast_log(LOG_WARNING, "Translator '%s' appears to be broken and will probably fail.\n", t->name);
 		t->comp_cost = 999999;

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id6878a989b50573298dab115d3371ea369e1a718
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>