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.