Attention is currently required from: pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/29861 )
Change subject: Rename and move func checking if amr mode is explicitly configured ...................................................................... Patch Set 1: Code-Review-1 (1 comment) File include/osmocom/mgcp/mgcp_codec.h: https://gerrit.osmocom.org/c/osmo-mgw/+/29861/comment/4250393c_ba999f4d PS1, Line 20: bool mgcp_codec_amr_mode_is_indicated(const struct mgcp_rtp_codec *codec); problems: - term 'mode' in the spec refers to the AMR modes 4k75 .. 12k2. So the naming 'mode is indicated' would mean that a 'mode-set' fmtp is present, not 'octet-align'. Not sure what term is non-ambiguous for OA vs BE. Maybe 'amr_align_is_indicated'? - per spec, the lack of an 'octet-align' fmtp implies octet-align=0. So it is wrong to even look whether a parameter is present or not, we should (TM) only work with the resulting alignment value. Of course that's a problem when osmo-bsc and osmo-msc always fail to send the octet-align=1 fmtp in their MGCP even though we basically always deal with AMR OA. Is it worth changing this non-compliant code instead of replacing it with the correct behvior? If we have to do some legacy compatibility, the non-compliant behavior deserves loud code comments? - why is this function changing from static to listed in a header file? I oppose that because the function's usefulness is obscure and it should never have been written. -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/29861 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I8dce3038ebccf5e1e37e2908070a67d66693a96f Gerrit-Change-Number: 29861 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Attention: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 24 Oct 2022 20:23:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
