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 <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 24 Oct 2022 20:23:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment