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

(3 comments)

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

Line 84: 	if (original) {
       : 		*attr = *original;
       : 	}
> Since the defaults are no longer zero any more there should be an else clau
Excellent finding! Really missed that. Thanks for pointing out and explaining what to do instead.


Line 109: 		attr->maxplayrate = 48000;
> Rather than using the magic number here, how about:
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 current code is intended as assigning/reusing values from default_*_attr would even be wrong. Same for the other RFC values in parse. Same for the RFC values in generate.


Line 124: 	if (attributes == strstr(attributes, "stereo=1")) {
> Rather than potentially scanning the whole attributes string for something 
Yes, strncmp avoids an unnecessary scan and makes this code even more readable! Kevin, now I do not have to add a comment here anymore, or?


-- 
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: 3
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: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes