Cellular Network Infrastructure » OsmoMGW

I think where we have to look first is struct mgcp_conn_peer. This is the struct where everything about the connection and the used codec is filled in. Here we also find struct mgcp_codec_param (mentioned above as struct codec_param?).

So lets try to fill it in to model the example above:

mgcp_conn_peer.codecs[0] = CODEC_AMR_8000_1 = 112
mgcp_conn_peer.codecs[1] = CODEC_AMR_8000_1 = 112

mgcp_conn_peer.ptmap[0].codec = CODEC_AMR_8000_1
mgcp_conn_peer.ptmap[0].pt = 111
mgcp_conn_peer.ptmap[1].codec = CODEC_AMR_8000_1
mgcp_conn_peer.ptmap[1].pt = 112

This is already a problem. When we want to map the codec 112 to the payload type number in ptmap we have an ambiguity. However, we could still get away here and take the position into account. So when we map codecs¹, we could start at the position [1] in the ptmap as well. However this would be a bit sketchy though.

struct mgcp_codec_param param is not an arry so we cannot define octet-align=1 and octet-align=2 at the same time at all.

All in all the API was done with the idea in mind that one could use different codecs but not multiple of the same codec with different parameters simultaneously.

There is probably no simple fix. Changing the API accordingly and patching all programs the use the API would probably be best, but we probably cannot do that. We could add a new sub struct that defines everything properly (including a pointer to additional SDP and a flag if the SDP should be appended or replace everything). Then we mark the old struct members as deprecated. That is not very appealing but it is the best I can think of at the moment.

I have the following idea. We could change the struct as follows. This wouldn't even break the ABI.

struct mgcp_conn_peer {
    /*! RTP connection IP-Address (optional, string e.g. "127.0.0.1") */
    char addr[INET6_ADDRSTRLEN];

    /*! RTP connection IP-Port (optional)  */
    uint16_t port;

    /*! RTP endpoint */
    char endpoint[MGCP_ENDPOINT_MAXLEN];

    /*! CALL ID (unique per connection) */
    unsigned int call_id;

    /*! RTP packetization interval (optional) */
    unsigned int ptime;

    /*! RTP codec list (optional) */
    enum mgcp_codecs codecs[MGCP_MAX_CODECS];

    /*! Number of codecs in RTP codec list (optional) */
    unsigned int codecs_len;

    /*! (deprecated, use codecs_pt) Global RTP payload type map. This payload type map is applied on all codecs and
     * must only be used when codecs does not contain the same codec multiple times. (optional, only needed when
     * payload types are used that differ from what IANA/3GPP defines) */
    struct ptmap ptmap[MGCP_MAX_CODECS];

    /*! (deprecated) RTP payload type map length (optional, only needed when payload
     * types are used that differ from what IANA/3GPP defines) */
    unsigned int ptmap_len;

    /*! If nonzero, send 'X-Osmo-IGN:' header. This is useful e.g. for SCCPlite MSCs where the MSC is
     * known to issue incoherent or unknown CallIDs / to issue CRCX commands with a different domain
     * name than the BSC. An OsmoMGW will then ignore these and not fail on mismatches. */
    uint32_t x_osmo_ign;

    /*! send 'X-Osmux: %d' header (or "*" as wildcard). */
    bool x_osmo_osmux_use;
    /*! -1 means send wildcard. */
    int x_osmo_osmux_cid;

    /*! If left MGCP_CONN_NONE, use MGCP_CONN_RECV_ONLY or MGCP_CONN_RECV_SEND, depending on whether an audio RTP
     * address is set. If != MGCP_CONN_NONE, force this conn mode. */
    enum mgcp_connection_mode conn_mode;

    /*! (deprecated, use codecs_param) Global codec params. In case the codec requires additional format parameters
     * (fmtp), those cann be set here, see also mgcp_common.h. The format parameters will be applied on all codecs
     * where applicable. */
    bool param_present;
    struct mgcp_codec_param param;

    /* Each codec has a unique default payload type number assigned (IANA/3GPP), in some cases, especially when
     * the same codec is used multiple times but with different parameters, a new payload type number must be
     * assigned. This can be done by listing the new payload type numbers in codecs_pt[] at the same index as in
     * codecs[].
    bool codecs_pt_present;
    unsigned int codecs_pt[MGCP_MAX_CODECS];

    /* In case the codec requires additional format parameters (fmtp), those can be set here for each codec
    *  individually, see also mgcp_common.h. The index in codecs_param[] must be the same index as in codecs[]. */
    bool codecs_param_present;
    struct mgcp_codec_param codecs_param[MGCP_MAX_CODECS];
};

So lets try:

/* First we define two times AMR */
mgcp_conn_peer.codecs[0] = CODEC_AMR_8000_1;
mgcp_conn_peer.codecs[1] = CODEC_AMR_8000_1;

/* Then we assign a new individual payload type number */
mgcp_conn_peer.ptmap_len = 0;
mgcp_conn_peer.codecs_pt_present = true;
codecs_pt[0] = 111;
codecs_pt[1] = 112;

/* Then we set the codec parameters for each codec indvidually */
mgcp_conn_peer.param_present = false;
mgcp_conn_peer.codecs_pt_present = true;
codecs_param[0].amr_octet_aligned_present = true;
codecs_param[0].amr_octet_aligned = true;
codecs_param[1].amr_octet_aligned_present = true;
codecs_param[1].amr_octet_aligned = false;

This could be a solution, what do you think?

Quick summary ahead: in this response, I discuss:

• what it would take to remove SDP parsing from osmo-mgcp-client: either introduce external dep sofia-sip, or adopt the sdp_msg.h API from osmo-msc.git.
• if keeping SDP parsing in osmo-mgcp-client: how to add fmtp, and some other restructuring around codecs.

If we keep SDP parsing in osmo-mgcp-client:

conceptually, we need to simply pass a list of tuples { codec name, payload type nr, fmtp }.

Indeed struct ptmap looks like it is a good match: it already is a list of { codec name, payload type nr }.

I still see these problems / room for thought:

• there is no need to have a list 'enum mgcp_codecs codecs[MGCP_MAX_CODECS];' -- it is a duplication of the ptmap member 'enum mgcp_codecs codecs[MGCP_MAX_CODECS];'

• fmtp is a pretty generic parameter -- depending on the codec, there may be various codec tweaks defined in fmtp. So far we only have one codec where fmtp is important to us: AMR. There we have the 'octet-aligned' and the 'mode-set' parameters that are most important. We could model these two as explicit C types, but osmo-msc (codecs branch) is already managing fmtp in string form -- it receives them from external MNCC in form of SDP. So both for flexibility and to reduce the amount of conversion, it makes most sense to me to simply introduce a generic fmtp in string form. So, instead of adding 'bool octet_aligned_present' and 'bool octet_aligned' flags etc., we can simply add a char[]. (In sdp_msg.h I picked a char⁶⁴ for fmtp)

• besides the tuple { codec, payload type number, fmtp }, technically there is also the 'rate' parameter -- this is generally 8000 for us, except IUFP (for 3G) that typically has 16000 set. We drag this parameter along and ignore it. But while at it, we might as well have a proper representation of it. So far we have the rate hardcoded. How to add the rate? We could add an explicit 'int rate' in the ptmap struct. We could also represent the codec as an SDP string like "AMR/8000" or "VND.3GPP.IUFP/16000", i.e. no longer use enum mgcp_codecs, but adopt the SDP string format.

Removing SDP parsing from osmo-mgcp-client:

When we already have fmtp as string, and we are also adding the rate possibly by replacing enum mgcp_codecs with an SDP string, then how far are we from simply passing the entire SDP string as-is to osmo_mgcp_client API?

Essentially this mgcp_client API is a conversion between SDP string <-> C structs. mgcp_client is not using any of the items in the SDP. On the one end there are our MGCP clients osmo-bsc, osmo-msc, osmo-hnbgw, and on the other end there is the MGCP wire. (Not talking about osmo-mgw side, only the MGCP client side.)

• In osmo-sip-connector we use sofia-sip for SDP parsing (or, soon, possibly not at all).
• In osmo-msc, we have the sdp_msg.h API to do the same. (The reason why osmo-msc's sdp_msg.h exists is because mgcp_client has its SDP parsing/composition very tightly glued to the MGCP wire actions, and it was not easily reusable, plus has problems like fmtp.)

So, if we have a separate parsing/composition of SDP, we can take this burden out of mgcp_client API entirely. A separate SDP parsing+composition would be nicely modular. If we agree on that, then there's two ways to go:

• a) adopt sofia-sip as dependency to parse and compose SDP -- if we adpot sofia-sip in osmo-mgcp-client and osmo-mgw, then osmo-msc should probably also use sofia-sip instead of a reinvented wheel that sdp_msg.h is. We need to fundamentally decide on this; before, i made the slightly questionable decision of "meh i don't like this over-complexity in the parsed C structs representing SDP from sofia-sip, it would be so much easier to implement my own parsing and not have to deal with an external dependency", and i may not have given sofia-sip enough credit back then. Using a proper implementation that is production proven clearly is a massive benefit in its own, certainly worth getting used to some C struct navigation for. I could re-evaluate.

• b) or we decide for osmo-msc's sdp_msg.h, move it to libosmo-mgcp-client as public API and use it everywhere. For that we may add an "owner" SDP element: the call_id so far is part of the owner/creator "o=..." (optional and trivial); x_osmo_ign and x_osmo_osmux are part of the MGCP header, not part of the SDP section. Otherwise all items are already implemented.

Personally, I still feel it is easier to use sdp_msg.h; but I'd also accept if we look into sofia-sip, find it generally ok to use and accept to take it as external dependency where ever we use SDP (osmo-msc, osmo-bsc and osmo-hnbgw, possibly also osmo-mgw server).

I am very certain that it would be a move in the right direction to remove SDP parsing and composition from osmo_mgcp_client API and merely pass SDP as string buffers between the MGCP wire and osmo_mgcp_client users. It should be pretty quick to do the osmo_mgcp_client API change: instead of composing SDP in add_sdp(), we would simply append the string provided by the caller. Instead of parsing the string from the wire, we would pass it on to the caller as-is. The bigger end of this change would be in our clients: they need to call sdp_msg_to_sdp_str() when dispatching MGCP verbs, and sdp_msg_from_sdp_str() when receiving MGCP responses. Still pretty trivial. (If sysmocom agrees, then I could do most of this work fairly quickly, I'm very familiar with all the relevant places in the code.)

Though it would probably still be quicker to patch up current osmo-mgcp-client's SDP parsing and composition, however ugly its quirks are.

Through the grapevine I have heard a strong opinion on the state of libsofia.
Apparently the original project is unmaintained, libsofia is fork-maintained by FreeSwitch,
and that libsofia has problems with code quality.

We can still investigate other SDP parsing libraries.
But for me this reinforces the idea to use the sdp_msg.h/c, which currently lives in osmo-msc.git

Yes libsofia-sip seems not to be the first option on the list, but as far as I understand we would not embed it directly in libosmo-mgcp-client anyway and only pass the generated strings.

Unfortunately I am having some difficulties with this task at the moment. First of all we should decide if we want to accept API/ABI incompatibilities? Also passing SDP strings from the outside would be a change of paradigm.

My original idea was to isolate everything MGCP/SDP related from the API user. I was not aware that there might be entities that pass SDP strings into the system from outside. I need to think a bit more about this.

On Mon, Mar 27, 2023 at 04:32:44PM +0000, dexter wrote:

My original idea was to isolate everything MGCP/SDP related from the API user. I was not aware that there might be entities that pass SDP strings into the system from outside. I need to think a bit more about this.

Maybe I can spend some limited time to come up with a patch to show what I have
in mind...