From d0dbda4106c8edbe3c957e49ecf5a539e54e6f0c Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Sat, 23 Dec 2023 03:54:58 +0100 Subject: [PATCH] drop cfg 'sdp audio fmtp-extra' There is considerable code complexity in place for this ancient hack. It dates back to 5ea1bc77a3947f541d576f95e7ecc7249fc65b9b " mgcp: Allow to freely control the a=fmtp line for experiments In case of AMR one can specify the available codecs out-of-band. Allow to configure this line statically in the configuration file. " Looking in mgcp_test.c output, the fmtp-extra tests do not even make sense: they result in fmtp for pt=126 being added, even though there is no payload type 126 listed in the SDP... Related: OS#6313 Change-Id: Icee0cd1f5a751fa760d5a9deca29089e78e7eb93 --- TODO-RELEASE | 1 + include/osmocom/mgcp/mgcp_network.h | 1 - include/osmocom/mgcp/mgcp_trunk.h | 1 - src/libosmo-mgcp/mgcp_conn.c | 2 -- src/libosmo-mgcp/mgcp_protocol.c | 3 -- src/libosmo-mgcp/mgcp_sdp.c | 37 ++-------------------- src/libosmo-mgcp/mgcp_vty.c | 48 ++++++++--------------------- tests/mgcp/mgcp_test.c | 13 ++------ 8 files changed, 19 insertions(+), 87 deletions(-) diff --git a/TODO-RELEASE b/TODO-RELEASE index 964ffe111..ea29b6acb 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -38,3 +38,4 @@ libosmo-mgcp-client remove public API These public API items have not been calle libosmo-mgcp-client deprecate public API New code should no longer use codecs[], instead use ptmap[].codec. There is backwards compat code that moves codecs[] entries, if any, over to ptmap[], so callers may migrate at own leisure. +osmo-mgw remove cfg Remove VTY config item 'sdp audio fmtp-extra' (see OS#6313) diff --git a/include/osmocom/mgcp/mgcp_network.h b/include/osmocom/mgcp/mgcp_network.h index e95907d01..a7bd3330d 100644 --- a/include/osmocom/mgcp/mgcp_network.h +++ b/include/osmocom/mgcp/mgcp_network.h @@ -108,7 +108,6 @@ struct mgcp_rtp_end { int frames_per_packet; uint32_t packet_duration_ms; int maximum_packet_time; /* -1: not set */ - char *fmtp_extra; /* are we transmitting packets (true) or dropping (false) outbound packets */ bool output_enabled; /* FIXME: This parameter can be set + printed, but is nowhere used! */ diff --git a/include/osmocom/mgcp/mgcp_trunk.h b/include/osmocom/mgcp/mgcp_trunk.h index e608161ed..16792397a 100644 --- a/include/osmocom/mgcp/mgcp_trunk.h +++ b/include/osmocom/mgcp/mgcp_trunk.h @@ -27,7 +27,6 @@ struct mgcp_trunk { unsigned int trunk_nr; enum mgcp_trunk_type trunk_type; - char *audio_fmtp_extra; int audio_send_ptime; int audio_send_name; diff --git a/src/libosmo-mgcp/mgcp_conn.c b/src/libosmo-mgcp/mgcp_conn.c index 283a21364..d9bc57371 100644 --- a/src/libosmo-mgcp/mgcp_conn.c +++ b/src/libosmo-mgcp/mgcp_conn.c @@ -110,8 +110,6 @@ static int mgcp_rtp_conn_init(struct mgcp_conn_rtp *conn_rtp, struct mgcp_conn * end->rtcp.fd = -1; memset(&end->addr, 0, sizeof(end->addr)); end->rtcp_port = 0; - talloc_free(end->fmtp_extra); - end->fmtp_extra = NULL; /* Set default values */ end->frames_per_packet = 0; /* unknown */ diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c index 2249a98b1..ac5c68241 100644 --- a/src/libosmo-mgcp/mgcp_protocol.c +++ b/src/libosmo-mgcp/mgcp_protocol.c @@ -1076,9 +1076,6 @@ mgcp_header_done: rc = mgcp_conn_iuup_init(conn); } - conn->end.fmtp_extra = talloc_strdup(trunk->endpoints, - trunk->audio_fmtp_extra); - if (pdata->cfg->force_ptime) { conn->end.packet_duration_ms = pdata->cfg->force_ptime; conn->end.force_output_ptime = 1; diff --git a/src/libosmo-mgcp/mgcp_sdp.c b/src/libosmo-mgcp/mgcp_sdp.c index 183c4ece2..77825f6ce 100644 --- a/src/libosmo-mgcp/mgcp_sdp.c +++ b/src/libosmo-mgcp/mgcp_sdp.c @@ -488,34 +488,10 @@ static int add_audio(struct msgb *sdp, int *payload_types, unsigned int payload_ } /* Add fmtp strings to sdp payload */ -static int add_fmtp(struct msgb *sdp, struct sdp_fmtp_param *fmtp_params, unsigned int fmtp_params_len, - const char *fmtp_extra) +static int add_fmtp(struct msgb *sdp, struct sdp_fmtp_param *fmtp_params, unsigned int fmtp_params_len) { unsigned int i; int rc; - int fmtp_extra_pt = -1; - char *fmtp_extra_pars = ""; - - /* When no fmtp parameters ara available but an fmtp extra string - * is configured, just add the fmtp extra string */ - if (fmtp_params_len == 0 && fmtp_extra) { - return msgb_printf(sdp, "%s\r\n", fmtp_extra); - } - - /* When there is fmtp extra configured we dissect it in order to drop - * in the configured extra parameters at the right place when - * generating the fmtp strings. */ - if (fmtp_extra) { - if (sscanf(fmtp_extra, "a=fmtp:%d ", &fmtp_extra_pt) != 1) - fmtp_extra_pt = -1; - - fmtp_extra_pars = strstr(fmtp_extra, " "); - - if (!fmtp_extra_pars) - fmtp_extra_pars = ""; - else - fmtp_extra_pars++; - } for (i = 0; i < fmtp_params_len; i++) { rc = msgb_printf(sdp, "a=fmtp:%u", fmtp_params[i].payload_type); @@ -532,13 +508,6 @@ static int add_fmtp(struct msgb *sdp, struct sdp_fmtp_param *fmtp_params, unsign return -EINVAL; } - /* Append extra parameters from fmtp extra */ - if (fmtp_params[i].payload_type == fmtp_extra_pt) { - rc = msgb_printf(sdp, " %s", fmtp_extra_pars); - if (rc < 0) - return -EINVAL; - } - rc = msgb_printf(sdp, "\r\n"); if (rc < 0) return -EINVAL; @@ -558,7 +527,6 @@ int mgcp_write_response_sdp(const struct mgcp_endpoint *endp, const char *addr) { const struct mgcp_rtp_codec *codec; - const char *fmtp_extra; const char *audio_name; int payload_type; struct sdp_fmtp_param fmtp_param; @@ -575,7 +543,6 @@ int mgcp_write_response_sdp(const struct mgcp_endpoint *endp, OSMO_ASSERT(addr); codec = conn->end.codec; - fmtp_extra = conn->end.fmtp_extra; audio_name = codec->audio_name; payload_type = codec->payload_type; @@ -617,7 +584,7 @@ int mgcp_write_response_sdp(const struct mgcp_endpoint *endp, fmtp_params[0] = fmtp_param; fmtp_params_len = 1; } - rc = add_fmtp(sdp, fmtp_params, fmtp_params_len, fmtp_extra); + rc = add_fmtp(sdp, fmtp_params, fmtp_params_len); if (rc < 0) goto buffer_too_small; } diff --git a/src/libosmo-mgcp/mgcp_vty.c b/src/libosmo-mgcp/mgcp_vty.c index 784894c18..b82f3df0c 100644 --- a/src/libosmo-mgcp/mgcp_vty.c +++ b/src/libosmo-mgcp/mgcp_vty.c @@ -111,9 +111,6 @@ static int config_write_mgcp(struct vty *vty) VTY_NEWLINE); } else vty_out(vty, " no rtp-patch%s", VTY_NEWLINE); - if (trunk->audio_fmtp_extra) - vty_out(vty, " sdp audio fmtp-extra %s%s", - trunk->audio_fmtp_extra, VTY_NEWLINE); vty_out(vty, " %ssdp audio-payload send-ptime%s", trunk->audio_send_ptime ? "" : "no ", VTY_NEWLINE); vty_out(vty, " %ssdp audio-payload send-name%s", @@ -187,7 +184,7 @@ static void dump_rtp_end(struct vty *vty, struct mgcp_conn_rtp *conn) " Payload Type: %d Rate: %u Channels: %d %s" " Frame Duration: %u Frame Denominator: %u%s" " FPP: %d Packet Duration: %u%s" - " FMTP-Extra: %s Audio-Name: %s Sub-Type: %s%s" + " Audio-Name: %s Sub-Type: %s%s" " Output-Enabled: %d Force-PTIME: %d%s", tx_packets->current, tx_bytes->current, VTY_NEWLINE, rx_packets->current, rx_bytes->current, VTY_NEWLINE, @@ -198,7 +195,7 @@ static void dump_rtp_end(struct vty *vty, struct mgcp_conn_rtp *conn) codec->payload_type, codec->rate, codec->channels, VTY_NEWLINE, codec->frame_duration_num, codec->frame_duration_den, VTY_NEWLINE, end->frames_per_packet, end->packet_duration_ms, - VTY_NEWLINE, end->fmtp_extra, codec->audio_name, + VTY_NEWLINE, codec->audio_name, codec->subtype_name, VTY_NEWLINE, end->output_enabled, end->force_output_ptime, VTY_NEWLINE); if (mgcp_conn_rtp_is_osmux(conn)) { @@ -681,21 +678,15 @@ DEFUN_USRATTR(cfg_mgcp_no_rtp_force_ptime, return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_mgcp_sdp_fmtp_extra, - cfg_mgcp_sdp_fmtp_extra_cmd, - X(MGW_CMD_ATTR_NEWCONN), - "sdp audio fmtp-extra .NAME", - "Add extra fmtp for the SDP file\n" "Audio\n" "Fmtp-extra\n" - "Extra Information\n") -{ - struct mgcp_trunk *trunk = mgcp_trunk_by_num(g_cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID); - OSMO_ASSERT(trunk); - char *txt = argv_concat(argv, argc, 0); - if (!txt) - return CMD_WARNING; +#define SDP_STR "SDP File related options\n" +#define AUDIO_STR "Audio payload options\n" - osmo_talloc_replace_string(g_cfg, &trunk->audio_fmtp_extra, txt); - talloc_free(txt); +DEFUN_DEPRECATED(cfg_mgcp_sdp_fmtp_extra, + cfg_mgcp_sdp_fmtp_extra_cmd, + "sdp audio fmtp-extra .NAME", + SDP_STR AUDIO_STR "Deprecated, without effect since osmo-mgw v1.13\n" "Deprecated, without effect\n") +{ + vty_out(vty, "%% deprecated: the config option 'sdp audio fmtp-extra' has been removed.%s", VTY_NEWLINE); return CMD_SUCCESS; } @@ -715,8 +706,6 @@ DEFUN_DEPRECATED(cfg_mgcp_no_allow_transcoding, return CMD_SUCCESS; } -#define SDP_STR "SDP File related options\n" -#define AUDIO_STR "Audio payload options\n" DEFUN_DEPRECATED(cfg_mgcp_sdp_payload_number, cfg_mgcp_sdp_payload_number_cmd, "sdp audio-payload number <0-255>", @@ -1062,28 +1051,17 @@ static int config_write_trunk(struct vty *vty) VTY_NEWLINE); } else vty_out(vty, " no rtp-patch%s", VTY_NEWLINE); - if (trunk->audio_fmtp_extra) - vty_out(vty, " sdp audio fmtp-extra %s%s", - trunk->audio_fmtp_extra, VTY_NEWLINE); } return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_trunk_sdp_fmtp_extra, +DEFUN_DEPRECATED(cfg_trunk_sdp_fmtp_extra, cfg_trunk_sdp_fmtp_extra_cmd, - X(MGW_CMD_ATTR_NEWCONN), "sdp audio fmtp-extra .NAME", - "Add extra fmtp for the SDP file\n" "Audio\n" "Fmtp-extra\n" - "Extra Information\n") + SDP_STR AUDIO_STR "Deprecated, without effect since osmo-mgw v1.13\n" "Deprecated, without effect\n") { - struct mgcp_trunk *trunk = vty->index; - char *txt = argv_concat(argv, argc, 0); - if (!txt) - return CMD_WARNING; - - osmo_talloc_replace_string(g_cfg, &trunk->audio_fmtp_extra, txt); - talloc_free(txt); + vty_out(vty, "%% deprecated: the config option 'sdp audio fmtp-extra' has been removed.%s", VTY_NEWLINE); return CMD_SUCCESS; } diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c index e37bb57ca..621ee7702 100644 --- a/tests/mgcp/mgcp_test.c +++ b/tests/mgcp/mgcp_test.c @@ -127,7 +127,6 @@ static void test_strline(void) "t=0 0\r\n" \ "m=audio 16006 RTP/AVP 97\r\n" \ "a=rtpmap:97 GSM-EFR/8000\r\n" \ - "a=fmtp:126 0/1/2\r\n" \ "a=ptime:40\r\n" #define MDCX4_ADDR0000 \ @@ -336,7 +335,6 @@ static void test_strline(void) "t=0 0\r\n" \ "m=audio 16006 RTP/AVP 97\r\n" \ "a=rtpmap:97 GSM-EFR/8000\r\n" \ - "a=fmtp:126 0/1/2\r\n" \ "a=ptime:40\r\n" #define CRCX_ZYN \ @@ -586,7 +584,6 @@ struct mgcp_test { const char *req; const char *exp_resp; int ptype; - const char *extra_fmtp; }; static const struct mgcp_test tests[] = { @@ -614,10 +611,9 @@ static const struct mgcp_test tests[] = { {"RQNT1", RQNT, RQNT1_RET}, {"RQNT2", RQNT2, RQNT2_RET}, {"DLCX", DLCX, DLCX_RET, PTYPE_IGNORE}, - {"CRCX", CRCX, CRCX_FMTP_RET, 97,.extra_fmtp = "a=fmtp:126 0/1/2"}, - {"MDCX3", MDCX3, MDCX3_FMTP_RET, PTYPE_NONE,.extra_fmtp = - "a=fmtp:126 0/1/2"}, - {"DLCX", DLCX, DLCX_RET, PTYPE_IGNORE,.extra_fmtp = "a=fmtp:126 0/1/2"}, + {"CRCX", CRCX, CRCX_FMTP_RET, 97}, + {"MDCX3", MDCX3, MDCX3_FMTP_RET, PTYPE_NONE}, + {"DLCX", DLCX, DLCX_RET, PTYPE_IGNORE}, {"CRCX", CRCX_NO_LCO_NO_SDP, CRCX_NO_LCO_NO_SDP_RET, 97}, {"CRCX", CRCX_X_OSMO_IGN, CRCX_X_OSMO_IGN_RET, 97}, {"MDCX_TOO_LONG_CI", MDCX_TOO_LONG_CI, MDCX_TOO_LONG_CI_RET}, @@ -831,9 +827,6 @@ static void test_messages(void) dummy_packets = 0; - osmo_talloc_replace_string(cfg, &trunk->audio_fmtp_extra, - t->extra_fmtp); - inp = create_msg(t->req, last_conn_id); msg = mgcp_handle_message(cfg, inp); msgb_free(inp);