Attention is currently required from: dexter, pespin.
Patch set 8:Code-Review -1
7 comments:
Patchset:
this can be much simpler, especially after https://gerrit.osmocom.org/c/osmo-mgw/+/34899 ...
File include/osmocom/mgcp/mgcp_common.h:
Patch Set #8, Line 73: } amr;
let's add a char* fmtp entry to struct ptmap instead.
To go with it, let's add individual parsing functions like
bool oa = (fmtp_get_int(ptmap->fmtp, "octet-align") == 1)
The parsing should only happen when we actually need details on an AMR entry, not on every message parsing and composition.
why:
so what i don't like about this is that it needs to model each and every fmtp that exists in anyone's MGCP in a C struct. Much rather, I'd suggest that we simply keep an actual string of the fmtp, and evaluate that string whenever we would need the conveyed information. Like that we can transport and respond with fmtp that osmo-mgw does not even need to care about. With the C struct implementation, we silently drop all unknown fmtp in a conversation, or we need to error a lot.
File include/osmocom/mgcp_client/mgcp_client_fsm.h:
Patch Set #2, Line 70: struct mgcp_codec_param2 codecs_param[MGCP_MAX_CODECS];
Ok, better sort that kind of problems *before* this patch.
Instead please add a fmtp entry to the struct ptmap, so that all of pt, codec and fmtp are kept in the same place. (further below i explain why i think it should be a string)
why:
i don't like this at all at all, it creates a separate array that needs to be kept in sync with (in this patch) the 'codecs' list as well as the 'ptmap' list. While doing this review, i submitted a patch to combine those two into just 'ptmap', because it is complete madness to keep N separate arrays of individual struct members, when you can just keep one array of structs. It would be cool to continue in that way.
File src/libosmo-mgcp-client/mgcp_client.c:
Patch Set #8, Line 1374: OSMO_ASSERT(!(mgcp_msg->param_present && mgcp_msg->codecs_param_present));
when we add a fmtp string to ptmap, then we don't need a codecs_param list nor a codecs_param_present flag. Instead I would see if in an incoming mgcp_msg, if param_present == true, apply that param only to those ptmap entries where there is no fmtp yet.
It would be cooler if we could just drop param, but we need to be backwards compatible, right?
Patch Set #8, Line 1391: for (i = 0; i < mgcp_msg->codecs_len; i++) {
if you accept my suggestion in https://gerrit.osmocom.org/c/osmo-mgw/+/34899 then here you'd iterate mgcp_msg->ptmap instead.
Patch Set #8, Line 1394: continue;
so this is dropping all fmtp except for AMR codecs, not ideal. Instead we could just simply store the string as it is.
Patch Set #8, Line 1399: MSGB_PRINTF_OR_RET("a=fmtp:%u octet-align=0\r\n", pt);
i don't like this: this is implementing codec-specific fmtp composition in osmo-mgw. Instead we should use the exact string that the user provided.
If we take on fmtp composition, we are opening a can of worms.
For example, the octet-align parameter is specified with a default value. It is usually only present when it should differ from the default, and omitted if it is the default. I'm not saying which one is better -- the point is that we don't want to have to make these decisions in osmo-mgw, at all.
If we keep the fmtp string 1:1, we can just lean back and use whatever clients throw at us, just carry the fmtp along as they are. Only when we need a fmtp detail like for AMR OA vs BE conversion, only then do we need to validate that the fmtp parameters make any sense. Only then will we have to throw an error if the fmtp is garbage. For those cases we don't care whether a default value is present or omitted, we just need to get the bit of message handling relevant information from it.
To view, visit change 34351. To unsubscribe, or for help writing mail filters, visit settings.