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

[Matt Jordan]

Matt Jordan has posted comments on this change.

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


Patch Set 1: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/1674/1/res/res_format_attr_opus.c
File res/res_format_attr_opus.c:

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);
        : 	}
Nitpick: we don't need the 'added' check here, as this is the first attribute.


Line 268: 	attr_res->dtx = (attr1->dtx | attr2->dtx);
I'm not sure about this change. The value stored in the dtx unsigned integers are read from user provided input. While the RFC mandates that this is always a '1' or '0', there's no guarantee that someone can't provide a larger value.

Say one side offers a sane value, say '1', while the other responds with something invalid, say '912837'. Instead of responding with a valid value to the original offerer, we'll respond with something clearly invalid.

The original code here was superior in that regard - we'd only ever respond with a 1 or 0, regardless if something malfunctioned and sent us something junky.

Also, this changes the semantics. As the comment noted, the original author of the module thought that we should only use DTX if both sides wanted to. Is there a reason, in practice, that you think we should prefer using DTX if only one side wants usedtx?


Line 274: 	attr_res->cbr = (attr1->cbr | attr2->cbr);
        : 	attr_res->spropstereo = (attr1->spropstereo | attr2->spropstereo);
Here as well, I'd prefer if we tolerant to non-zero/one values and only respond with a 0 or 1.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iae85ba3e5960bfd5d51cf65bcffad00dd4875a73
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes