[Asterisk-code-review] res format attr opus: Update to latest RFC 7587. (asterisk[master])
Matt Jordan
asteriskteam at digium.com
Fri Dec 4 11:34:04 CST 2015
Matt Jordan has submitted this change and it was merged.
Change subject: res_format_attr_opus: Update to latest RFC 7587.
......................................................................
res_format_attr_opus: Update to latest RFC 7587.
Beside that, the format-attribute module sends only non-default values in the
line fmtp, now. This avoids unnecessary overhead in SDP messages. Furthermore,
previously the parameter stereo was not parsed when being the first parameter.
ASTERISK-25583 #close
Change-Id: Iae85ba3e5960bfd5d51cf65bcffad00dd4875a73
---
M res/res_format_attr_opus.c
1 file changed, 151 insertions(+), 59 deletions(-)
Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Richard Mudgett: Looks good to me, but someone else must approve
Anonymous Coward #1000019: Verified
Matt Jordan: Looks good to me, approved
diff --git a/res/res_format_attr_opus.c b/res/res_format_attr_opus.c
index 3c9c3ef..4c09bef 100644
--- a/res/res_format_attr_opus.c
+++ b/res/res_format_attr_opus.c
@@ -33,28 +33,36 @@
#include "asterisk/module.h"
#include "asterisk/format.h"
+#include "asterisk/logger.h" /* for ast_log, LOG_WARNING */
+#include "asterisk/strings.h" /* for ast_str_append */
+#include "asterisk/utils.h" /* for MIN, ast_malloc, ast_free */
/*!
* \brief Opus attribute structure.
*
- * \note http://tools.ietf.org/html/draft-ietf-payload-rtp-opus-00.
+ * \note http://tools.ietf.org/html/rfc7587#section-6
*/
struct opus_attr {
- unsigned int maxbitrate; /* Default 64-128 kb/s for FB stereo music */
- unsigned int maxplayrate /* Default 48000 */;
- unsigned int minptime; /* Default 3, but it's 10 in format.c */
- unsigned int stereo; /* Default 0 */
- unsigned int cbr; /* Default 0 */
- unsigned int fec; /* Default 0 */
- unsigned int dtx; /* Default 0 */
- unsigned int spropmaxcapturerate; /* Default 48000 */
- unsigned int spropstereo; /* Default 0 */
+ unsigned int maxbitrate;
+ unsigned int maxplayrate;
+ unsigned int unused; /* was minptime, kept for binary compatibility */
+ unsigned int stereo;
+ unsigned int cbr;
+ unsigned int fec;
+ unsigned int dtx;
+ unsigned int spropmaxcapturerate;
+ unsigned int spropstereo;
};
static struct opus_attr default_opus_attr = {
- .fec = 0,
- .dtx = 0,
- .stereo = 0,
+ .maxplayrate = 48000,
+ .spropmaxcapturerate = 48000,
+ .maxbitrate = 510000,
+ .stereo = 0,
+ .spropstereo = 0,
+ .cbr = 0,
+ .fec = 1,
+ .dtx = 0,
};
static void opus_destroy(struct ast_format *format)
@@ -67,7 +75,7 @@
static int opus_clone(const struct ast_format *src, struct ast_format *dst)
{
struct opus_attr *original = ast_format_get_attribute_data(src);
- struct opus_attr *attr = ast_calloc(1, sizeof(*attr));
+ struct opus_attr *attr = ast_malloc(sizeof(*attr));
if (!attr) {
return -1;
@@ -75,6 +83,8 @@
if (original) {
*attr = *original;
+ } else {
+ *attr = default_opus_attr;
}
ast_format_set_attribute_data(dst, attr);
@@ -97,33 +107,54 @@
if ((kvp = strstr(attributes, "maxplaybackrate")) && sscanf(kvp, "maxplaybackrate=%30u", &val) == 1) {
attr->maxplayrate = val;
+ } else {
+ attr->maxplayrate = 48000;
}
+
if ((kvp = strstr(attributes, "sprop-maxcapturerate")) && sscanf(kvp, "sprop-maxcapturerate=%30u", &val) == 1) {
attr->spropmaxcapturerate = val;
+ } else {
+ attr->spropmaxcapturerate = 48000;
}
- if ((kvp = strstr(attributes, "minptime")) && sscanf(kvp, "minptime=%30u", &val) == 1) {
- attr->minptime = val;
- }
+
if ((kvp = strstr(attributes, "maxaveragebitrate")) && sscanf(kvp, "maxaveragebitrate=%30u", &val) == 1) {
attr->maxbitrate = val;
+ } else {
+ attr->maxbitrate = 510000;
}
- if ((kvp = strstr(attributes, " stereo")) && sscanf(kvp, " stereo=%30u", &val) == 1) {
- attr->stereo = val;
+
+ if (!strncmp(attributes, "stereo=1", 8)) {
+ attr->stereo = 1;
+ } else if (strstr(attributes, " stereo=1")) {
+ attr->stereo = 1;
+ } else if (strstr(attributes, ";stereo=1")) {
+ attr->stereo = 1;
+ } else {
+ attr->stereo = 0;
}
- if ((kvp = strstr(attributes, ";stereo")) && sscanf(kvp, ";stereo=%30u", &val) == 1) {
- attr->stereo = val;
+
+ if (strstr(attributes, "sprop-stereo=1")) {
+ attr->spropstereo = 1;
+ } else {
+ attr->spropstereo = 0;
}
- if ((kvp = strstr(attributes, "sprop-stereo")) && sscanf(kvp, "sprop-stereo=%30u", &val) == 1) {
- attr->spropstereo = val;
+
+ if (strstr(attributes, "cbr=1")) {
+ attr->cbr = 1;
+ } else {
+ attr->cbr = 0;
}
- if ((kvp = strstr(attributes, "cbr")) && sscanf(kvp, "cbr=%30u", &val) == 1) {
- attr->cbr = val;
+
+ if (strstr(attributes, "useinbandfec=1")) {
+ attr->fec = 1;
+ } else {
+ attr->fec = 0;
}
- if ((kvp = strstr(attributes, "useinbandfec")) && sscanf(kvp, "useinbandfec=%30u", &val) == 1) {
- attr->fec = val;
- }
- if ((kvp = strstr(attributes, "usedtx")) && sscanf(kvp, "usedtx=%30u", &val) == 1) {
- attr->dtx = val;
+
+ if (strstr(attributes, "usedtx=1")) {
+ attr->dtx = 1;
+ } else {
+ attr->dtx = 0;
}
return cloned;
@@ -132,34 +163,92 @@
static void opus_generate_sdp_fmtp(const struct ast_format *format, unsigned int payload, struct ast_str **str)
{
struct opus_attr *attr = ast_format_get_attribute_data(format);
+ int added = 0;
if (!attr) {
- return;
+ /*
+ * (Only) cached formats do not have attribute data assigned because
+ * they were created before this attribute module was registered.
+ * Therefore, we assume the default attribute values here.
+ */
+ attr = &default_opus_attr;
}
- /* FIXME should we only generate attributes that were explicitly set? */
- ast_str_append(str, 0,
- "a=fmtp:%u "
- "maxplaybackrate=%u;"
- "sprop-maxcapturerate=%u;"
- "minptime=%u;"
- "maxaveragebitrate=%u;"
- "stereo=%d;"
- "sprop-stereo=%d;"
- "cbr=%d;"
- "useinbandfec=%d;"
- "usedtx=%d\r\n",
- payload,
- attr->maxplayrate ? attr->maxplayrate : 48000, /* maxplaybackrate */
- attr->spropmaxcapturerate ? attr->spropmaxcapturerate : 48000, /* sprop-maxcapturerate */
- attr->minptime > 10 ? attr->minptime : 10, /* minptime */
- attr->maxbitrate ? attr->maxbitrate : 20000, /* maxaveragebitrate */
- attr->stereo ? 1 : 0, /* stereo */
- attr->spropstereo ? 1 : 0, /* sprop-stereo */
- attr->cbr ? 1 : 0, /* cbr */
- attr->fec ? 1 : 0, /* useinbandfec */
- attr->dtx ? 1 : 0 /* usedtx */
- );
+ if (48000 != attr->maxplayrate) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "maxplaybackrate=%u", attr->maxplayrate);
+ }
+
+ if (48000 != attr->spropmaxcapturerate) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "sprop-maxcapturerate=%u", attr->spropmaxcapturerate);
+ }
+
+ if (510000 != attr->maxbitrate) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "maxaveragebitrate=%u", attr->maxbitrate);
+ }
+
+ if (0 != attr->stereo) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "stereo=%u", attr->stereo);
+ }
+
+ if (0 != attr->spropstereo) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "sprop-stereo=%u", attr->spropstereo);
+ }
+
+ if (0 != attr->cbr) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "cbr=%u", attr->cbr);
+ }
+
+ if (0 != attr->fec) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "useinbandfec=%u", attr->fec);
+ }
+
+ if (0 != attr->dtx) {
+ if (added) {
+ ast_str_append(str, 0, ";");
+ } else if (0 < ast_str_append(str, 0, "a=fmtp:%u ", payload)) {
+ added = 1;
+ }
+ ast_str_append(str, 0, "usedtx=%u", attr->dtx);
+ }
+
+ if (added) {
+ ast_str_append(str, 0, "\r\n");
+ }
}
static struct ast_format *opus_getjoint(const struct ast_format *format1, const struct ast_format *format2)
@@ -183,19 +272,22 @@
}
attr_res = ast_format_get_attribute_data(jointformat);
- /* Only do dtx if both sides want it. DTX is a trade off between
- * computational complexity and bandwidth. */
- attr_res->dtx = attr1->dtx && attr2->dtx ? 1 : 0;
+ attr_res->dtx = attr1->dtx || attr2->dtx ? 1 : 0;
/* Only do FEC if both sides want it. If a peer specifically requests not
* to receive with FEC, it may be a waste of bandwidth. */
attr_res->fec = attr1->fec && attr2->fec ? 1 : 0;
+ attr_res->cbr = attr1->cbr || attr2->cbr ? 1 : 0;
+ attr_res->spropstereo = attr1->spropstereo || attr2->spropstereo ? 1 : 0;
+
/* Only do stereo if both sides want it. If a peer specifically requests not
* to receive stereo signals, it may be a waste of bandwidth. */
attr_res->stereo = attr1->stereo && attr2->stereo ? 1 : 0;
- /* FIXME: do we need to join other attributes as well, e.g., minptime, cbr, etc.? */
+ attr_res->maxbitrate = MIN(attr1->maxbitrate, attr2->maxbitrate);
+ attr_res->spropmaxcapturerate = MIN(attr1->spropmaxcapturerate, attr2->spropmaxcapturerate);
+ attr_res->maxplayrate = MIN(attr1->maxplayrate, attr2->maxplayrate);
return jointformat;
}
@@ -223,7 +315,7 @@
} else if (!strcasecmp(name, "max_playrate")) {
attr->maxplayrate = val;
} else if (!strcasecmp(name, "minptime")) {
- attr->minptime = val;
+ attr->unused = val;
} else if (!strcasecmp(name, "stereo")) {
attr->stereo = val;
} else if (!strcasecmp(name, "cbr")) {
--
To view, visit https://gerrit.asterisk.org/1675
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iae85ba3e5960bfd5d51cf65bcffad00dd4875a73
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>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list