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

(5 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 */
> The struct definition opus_attr is neither declared in include/asterisk nor
I think it is fine then. Always better to err on the side of caution.


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;
        : 	}
> The parameter "stereo" is not prefix save. Or stated differently, strstr(.)
Ah okay yeah that makes sense. I'd say a comment can't hurt just to add a bit of clarification.


Line 166: 	if (!attr) {
        : 		attr = &default_opus_attr;
        : 	}
> Please, see the recent change ASTERISK-25160. Cached formats do not have at
I think a comment would be good here as well.


Line 170: 	if (48000 != attr->maxplayrate) {
        : 		if (added) {
        : 			ast_str_append(str, 0, ";");
        : 		}
> When somebody adds or moves parameters in future, he has to double-check an
Fair points. I am fine with leaving it in.


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);
        : 	}
> Please, see the recent change ASTERISK-25573. The H.264 module is using the
You do have a point that if the fmtp append fails then any subsequent ones *should* as well. I'm just not sure if they are guaranteed to fail.

I'd say just drop this finding for now and if a situation does every come up where it occurs it can be dealt with then.


-- 
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: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes