[Asterisk-code-review] res format attr opus: Update to latest RFC 7587. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_format_attr_opus: Update to latest RFC 7587.
......................................................................


Patch Set 2: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/1675/2//COMMIT_MSG
Commit Message:

Line 9: sents
sends or sent?


Line 9: attribite
s/attribite/attribute


https://gerrit.asterisk.org/#/c/1675/2/res/res_format_attr_opus.c
File res/res_format_attr_opus.c:

Line 48: 	unsigned int unused; /* was minptime, kept for binary compatibility */
I could be wrong here, but I think the compatibility issue only applies to external interfaces and structures? If so this field can simply be removed.


Line 104: 	attr = ast_format_get_attribute_data(cloned);
        : 
        : 	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 the attributes are cloned from the defaults structure then the defaults are already applied (so no need to set them again). If it is non-defaulted data that was cloned then this will overwrite that setting with a default value. Wouldn't you want to keep what was already there if a new value was not specified?


Line 124: 	if (attributes == strstr(attributes, "stereo=1")) {
        : 		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;
        : 	}
I don't understand what the first "if" check here is for - comparing attributes to the strstr result? If it is meant to just check if "stereo=1" is in the string then the subsequent checks shouldn't be needed. Meaning would the following be the same thing:

attr->stereo = strstr(attributes, "stereo=1") ? 1 : 0;


Line 166: 	if (!attr) {
        : 		attr = &default_opus_attr;
        : 	}
Since you are only adding non-default attributes then if no attributes are specified then you should just be able to 'return' here.


Line 170: 	if (48000 != attr->maxplayrate) {
        : 		if (added) {
        : 			ast_str_append(str, 0, ";");
        : 		}
'added' is always initially false, so no need for the check on the first attribute.


Line 170: 	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);
        : 	}
I think these sections need to be reworked a bit. If 'added' is 'false' and the payload line fails to append, then the subsequent attribute is potentially appended to the string without the payload.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iae85ba3e5960bfd5d51cf65bcffad00dd4875a73
Gerrit-PatchSet: 2
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: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes