Attention is currently required from: laforge, pespin.
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/35434?usp=email )
Change subject: add fmtp.[hc]
......................................................................
Patch Set 5:
(6 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/e32e3e03_dd185761
PS4, Line 9: Upcoming patches will add handling of arbitrary ftmp strings to
fmtp strings
lol! thx
https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/e6755321_69675d81
PS4, Line 13: Add generic API for handling ftmp strings. The primary intended user is
fmtp
Done
Patchset:
PS4:
minor nitpick: the subject states "ftmp.h"
while in reality it's more than a header.
ah yes, it already secretly morphed
from just-a-header to a fmtp.c.
Now it wants to morph into a libosmo-sdp.so.
File src/libosmo-mgcp-client/fmtp.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/bbd958dd_9970414c
PS4, Line 29: for (; fmtp && *fmtp && *fmtp != ';'; fmtp++);
fmtp being nonull should be checked only once at
start.
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/73bc9c47_285cf0a4
PS4, Line 41: * if (osmo_sdp_fmtp_get_val(res, sizeof(res), fmtp_vals,
"mode-set"))
IIUC res contains "0,2,4,7" here, maybe
worth saying that.
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/be198855_4b453326
PS4, Line 89: int osmo_sdp_fmtp_get_int(const char *fmtp, const char *option_name, int
default_value)
did you check in SDP spec if int range is enough?
the fmtp contents are codec specific and not defined in SDP, apart from their
general structure (which characters to use for separation). For our known fmtp use cases,
we will use numbers as high as 7.
If a fmtp needs a number above two billion, then the caller should use
osmo_sdp_fmtp_get_val() to retrieve the value as string. This convenience function is
useful only for very very small integers, i.e. below ... TWO BILLION =) scnr
But it took me longer to type this response than it would take to increase the range, so
i'm also increasing the range now.
--
To view, visit
https://gerrit.osmocom.org/c/osmo-mgw/+/35434?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I3eaea353dbd98c19212199b564342d0ac16cbc07
Gerrit-Change-Number: 35434
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 05 Jan 2024 02:12:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment