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

[Alexander Traud]

Alexander Traud has posted comments on this change.

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


Patch Set 2:

(6 comments)

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 
The struct definition opus_attr is neither declared in include/asterisk nor public, yes. However, the data itself is accessible via ast_format_get_attribute_data(.) everywhere. If there is a consumer somewhere, he might expect the data to include an unsigned int at that position. Because I am not aware of any other changes in format-attribute modules or transcoding modules in Asterisk 14, I would like to stay away to break binary compatibility with those modules designed for Asterisk 13.7. If you do not agree, I could change this just for master. Please, say so!


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 
Please, see the recent change ASTERISK-25537. Although the numbers are equal, they are not the same, because in parse, the RFC defaults are assigned. However, the default_*_attr structure is the Asterisk internal default. For example, FEC differs between the internal default and the RFC default. Consequently, the behaviour is intended.


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 attrib
The parameter "stereo" is not prefix save. Or stated differently, strstr(.) could find "sprop-stereo" instead. Therefore, we have to double-check a space and semicolon, as was done in the previous version. However, if "stereo" is the very first parameter, this case was missed. The strings are not checked for equality but to start from the same memory address. Consequently, at least these three checks are required. If you like to see a comment in source code, explaining this, please, say so!


Line 166: 	if (!attr) {
        : 		attr = &default_opus_attr;
        : 	}
> Since you are only adding non-default attributes then if no attributes are 
Please, see the recent change ASTERISK-25160. Cached formats do not have attribute data assigned. Therefore, if no attribute exists, we have to apply the Asterisk internal defaults. Consequently, we cannot return here, because the internal defaults could create a fmtp – currently it actually does for FEC. If you like to see a comment in source code, explaining this, please, say so!


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 at
When somebody adds or moves parameters in future, he has to double-check and change that again. I prefer conformity here. Furthermore, an optimising compiler should be able to remove this automatically. I am not doing the job of the compiler when source-code readability is on danger. Even without an optimising compiler, this is about one comparison, per channel/connection.


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
Please, see the recent change ASTERISK-25573. The H.264 module is using the same pattern.

Previously, the return value was not checked at all. If you think you found a bug, please, create an issue report covering H.264 and Opus. Then, someone can look at this and outline/create a better approach. Furthermore, please, describe in detail which issue you see in which case. Currently, I am not able to envision a case when the fmtp-append fails, but the subsequent pamater-append does not fail as well.


-- 
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