neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/28783 ) Change subject: sdp_msg: add sdp_audio_codecs_cmp(), add compare flags ...................................................................... sdp_msg: add sdp_audio_codecs_cmp(), add compare flags A problem with SDP fmtp handling is visible in this patch: when cmp_fmtp is true, we compare fmtp strings 1:1, which is not how things should be done. The intention is to fix fmtp handling in a later patch. At least there now is a flag to bypass fmtp comparison altogether. Related: SYS#5066 Change-Id: I18d33e189674229501afec950aa1c732386455a2 --- M include/osmocom/msc/sdp_msg.h M src/libmsc/sdp_msg.c 2 files changed, 73 insertions(+), 17 deletions(-) Approvals: Jenkins Builder: Verified dexter: Looks good to me, but someone else must approve neels: Looks good to me, approved diff --git a/include/osmocom/msc/sdp_msg.h b/include/osmocom/msc/sdp_msg.h index 44a8a30..537713c 100644 --- a/include/osmocom/msc/sdp_msg.h +++ b/include/osmocom/msc/sdp_msg.h @@ -36,7 +36,10 @@ const char *sdp_msg_line_end(const char *src); -int sdp_audio_codec_cmp(const struct sdp_audio_codec *a, const struct sdp_audio_codec *b); +int sdp_audio_codec_cmp(const struct sdp_audio_codec *a, const struct sdp_audio_codec *b, + bool cmp_fmtp, bool cmp_payload_type); +int sdp_audio_codecs_cmp(const struct sdp_audio_codecs *a, const struct sdp_audio_codecs *b, + bool cmp_fmtp, bool cmp_payload_type); struct sdp_audio_codec *sdp_audio_codec_add(struct sdp_audio_codecs *ac, unsigned int payload_type, const char *subtype_name, unsigned int rate, const char *fmtp); diff --git a/src/libmsc/sdp_msg.c b/src/libmsc/sdp_msg.c index 08a2186..a2a2d3d 100644 --- a/src/libmsc/sdp_msg.c +++ b/src/libmsc/sdp_msg.c @@ -31,30 +31,83 @@ #include <osmocom/msc/sdp_msg.h> /* Compare name, rate and fmtp, returning typical cmp result: 0 on match, and -1 / 1 on mismatch. - * Do *not* compare the payload_type number. + * If cmp_fmtp is false, do *not* compare the fmtp string; if true, compare fmtp 1:1 as strings. + * If cmp_payload_type is false, do *not* compare the payload_type number. * The fmtp is only string-compared -- e.g. if AMR parameters appear in a different order, it amounts to a mismatch even * though all parameters are the same. */ -int sdp_audio_codec_cmp(const struct sdp_audio_codec *a, const struct sdp_audio_codec *b) +int sdp_audio_codec_cmp(const struct sdp_audio_codec *a, const struct sdp_audio_codec *b, + bool cmp_fmtp, bool cmp_payload_type) { - int rc; + int cmp; if (a == b) return 0; if (!a) return -1; if (!b) return 1; - rc = strncmp(a->subtype_name, b->subtype_name, sizeof(a->subtype_name)); - if (rc) - return rc; + cmp = strncmp(a->subtype_name, b->subtype_name, sizeof(a->subtype_name)); + if (cmp) + return cmp; + cmp = OSMO_CMP(a->rate, b->rate); + if (cmp) + return cmp; + if (cmp_fmtp) { + cmp = strncmp(a->fmtp, b->fmtp, sizeof(a->fmtp)); + if (cmp) + return cmp; + } + if (cmp_payload_type) { + cmp = OSMO_CMP(a->payload_type, b->payload_type); + if (cmp) + return cmp; + } + return 0; +} - if (a->rate < b->rate) +/* Compare two lists of audio codecs, returning typical cmp result: 0 on match, and -1 / 1 on mismatch. + * The ordering in the two lists may differ, except that the first codec in 'a' must also be the first codec in 'b'. + * This is because the first codec typically expresses the preferred codec to use. + * If cmp_fmtp is false, do *not* compare the fmtp strings; if true, compare fmtp 1:1 as strings. + * If cmp_payload_type is false, do *not* compare the payload_type numbers. + * The fmtp is only string-compared -- e.g. if AMR parameters appear in a different order, it amounts to a mismatch even + * though all parameters are the same. */ +int sdp_audio_codecs_cmp(const struct sdp_audio_codecs *a, const struct sdp_audio_codecs *b, + bool cmp_fmtp, bool cmp_payload_type) +{ + const struct sdp_audio_codec *codec_a; + const struct sdp_audio_codec *codec_b; + int cmp; + if (a == b) + return 0; + if (!a) return -1; - if (a->rate > b->rate) + if (!b) return 1; - rc = strncmp(a->fmtp, b->fmtp, sizeof(a->fmtp)); - if (rc) - return rc; + cmp = OSMO_CMP(a->count, b->count); + if (cmp) + return cmp; + + if (!a->count) + return 0; + + /* The first codec is the "chosen" codec and should match. The others may appear in different order. */ + cmp = sdp_audio_codec_cmp(&a->codec[0], &b->codec[0], cmp_fmtp, cmp_payload_type); + if (cmp) + return cmp; + + /* See if each codec in a is also present in b */ + foreach_sdp_audio_codec(codec_a, a) { + bool match_found = false; + foreach_sdp_audio_codec(codec_b, b) { + if (!sdp_audio_codec_cmp(codec_a, codec_b, cmp_fmtp, cmp_payload_type)) { + match_found = true; + break; + } + } + if (!match_found) + return -1; + } return 0; } @@ -130,13 +183,13 @@ return codec; } -/* Return a given sdp_msg's codec entry that matches the subtype_name, rate and fmtp of the given codec, or NULL if no - * match is found. Comparison is made by sdp_audio_codec_cmp(). */ +/* Return a given sdp_msg's codec entry that matches the subtype_name and rate of the given codec, or NULL if no + * match is found. Comparison is made by sdp_audio_codec_cmp(cmp_payload_type=false). */ struct sdp_audio_codec *sdp_audio_codec_by_descr(struct sdp_audio_codecs *ac, const struct sdp_audio_codec *codec) { struct sdp_audio_codec *i; foreach_sdp_audio_codec(i, ac) { - if (!sdp_audio_codec_cmp(i, codec)) + if (!sdp_audio_codec_cmp(i, codec, false, false)) return i; } return NULL; @@ -452,8 +505,8 @@ } /* Leave only those codecs in 'ac_dest' that are also present in 'ac_other'. - * The matching is made by sdp_audio_codec_cmp(), i.e. payload_type numbers are not compared and fmtp parameters are - * compared 1:1 as plain strings. + * The matching is made by sdp_audio_codec_cmp(cmp_payload_type=false), i.e. payload_type numbers are not compared and + * fmtp parameters are compared 1:1 as plain strings. * If translate_payload_type_numbers has an effect if ac_dest and ac_other have mismatching payload_type numbers for the * same SDP codec descriptions. If translate_payload_type_numbers is true, take the payload_type numbers from ac_other. * If false, keep payload_type numbers in ac_dest unchanged. */ -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/28783 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I18d33e189674229501afec950aa1c732386455a2 Gerrit-Change-Number: 28783 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier@sysmocom.de> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> Gerrit-Reviewer: pespin <pespin@sysmocom.de> Gerrit-MessageType: merged