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

[Alexander Traud]

Alexander Traud has posted comments on this change.

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


Patch Set 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 attribu
I like nickpicks, however, this was done on purpose. Therefore, I disagree. When somebody adds or moves parameters, he has to double-check. 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 268: 	attr_res->dtx = (attr1->dtx | attr2->dtx);
> I'm not sure about this change. The value stored in the dtx unsigned intege
Yes, semantics was changed on purpose. Forgot to mention that issue.

VAD/DTX/CN is not about total bandwidth but usage bandwidth. For example, when used in a company Wi-Fi network, the bandwidth is sufficient but shared across multiple users. If too many employees phone at the same time, the network gets overloaded. Another example is UMTS/LTE when it is billed by data volume. DTX is able to reduce the overall data usage. Normally, an Asterisk server has enough total bandwidth and no restriction on bandwidth usage. However, when a VoIP client requests DTX, this is an issue in his network. Therefore, both upstream and downstream should do DTX. Therefore, I changed from AND to OR.


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 res
I am going to change/filter this in parse_sdp_fmtp as well. OK?


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