Attention is currently required from: pespin.

Patch set 1:Code-Review -1

View Change

1 comment:

• File include/osmocom/mgcp/mgcp_codec.h:

  • Patch Set #1, 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 change 29861. To unsubscribe, or for help writing mail filters, visit settings.