Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/32218
to look at the new patch set (#3).
Change subject: mgcp_codec: fix codec decision
......................................................................
mgcp_codec: fix codec decision
Unfortunately OsmoMGW was never really tested with multiple different
codecs on either side of the connection. While OsmoMSC and OsmoBSC only
assign exactly one codec on each side this has never been a problem,
however it might become a problem when a call againt assigns multiple
codecs on one side. This has been observed in a setup where OsmoMGW had
one leg towards an external call agent. Also due to recent upgrades to
the TTCN3 tests we are now able to simulate different codecs on both
sides to pinpoint issues.
Testing has shown that OsmoMGW has difficulties with multiple codecs.
The reason for this is that the function that makes the codec decision
was not fully finished. So let's finish the codec decision function and
let's also use that decision when patching the payload type of outgoing
RTP packets.
Related: OS#5461
Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
---
M include/osmocom/mgcp/mgcp_codec.h
M include/osmocom/mgcp/mgcp_trunk.h
M src/libosmo-mgcp/mgcp_codec.c
M src/libosmo-mgcp/mgcp_network.c
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_vty.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
8 files changed, 286 insertions(+), 266 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/18/32218/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision
......................................................................
Patch Set 3:
(8 comments)
File include/osmocom/mgcp/mgcp_codec.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/3ce0af72_57723ec1
PS2, Line 16: int mgcp_codec_decide(struct mgcp_conn_rtp *conn, struct mgcp_conn_rtp *conn_opposite);
> my understanding is that this decides which codec is used in one direction between conn A and conn B […]
Done
File src/libosmo-mgcp/mgcp_codec.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/28f2c879_c1d1d047
PS2, Line 440: conn->end.codec = mgcp_codec_find_same(conn, found_same_codec_opposite);
> I wonder why do you need to call mgcp_codec_find_same() twice here, something looks wrong imho.
The problem is that it is not possible to assign codec_conn_dst to conn_src->end.codec since then the pointer in conn_src->end.codec becomes stale when conn_dst is freed. So we just search once more to get the pointer from the list within conn_src
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/563f1f24_4f257dd8
PS2, Line 449: found_same_codec_opposite = codec_find_convertible(conn_opposite, &conn->end.codecs[i]);
> I wonder why do you need to call codec_find_convertible() twice here, something looks wrong imho.
(same as above)
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/257caf90_c3a7843a
PS2, Line 461: }
> IIUC you are finding/selecting/setting the same codec A->B and B->A here, but I wonder if that reall […]
When we trying to find a convertible codec, then it indeed may be the case that A ends up with a different codec than B. But it will always be a compatible codec. What however is true is when you swap conn_src and conn_dst, then the result will be different but I see no problem with this, it would still be two codecs that are compatible.
This is also more a theoretical issue, normally one side (the leg pointing to the MSC) will assign exactly one codec.
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/bf1d569b_dc6ce036
PS2, Line 514: /* Lookup a suitable codec in the destination connection. */
> I see no lookup being done here anymore.
There was even more that I have overlooked.
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/f870f6af_3802cde0
PS2, Line 809: conn_opposite = mgcp_find_dst_conn(conn->conn);
> see, here we have a clear concept of conn_src ("conn" here) and conn_dst (returned by mgcp_find_dst_ […]
Done
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/3ce68c38_04c74e72
PS2, Line 816: if (mgcp_codec_decide(conn, conn_rtp_opposite) != 0)
> in here you are deciding what codec is used to forward/transmit from conn_src to conn_dst.
Done
File src/libosmo-mgcp/mgcp_vty.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/0f544b7f_291209de
PS2, Line 703: "allow-transcoding", "Allow transcoding\n")
> Add vty_out telling the user that this command is deprecated and is a NO-OP, so that they can drop i […]
Oh, looks like this has been forgotten on all other DEFUN_DEPRECATED functions as well.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 11:25:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32221 )
Change subject: Fix octet 2 of NM GPRS Cell
......................................................................
Fix octet 2 of NM GPRS Cell
Octet 2 should contain the address of the GPRS cell in the GPRS NSE
object. Since there's 1 GPRS Cell per BTS and we have only 1 BTS in
osmo-bts, then this address should be 0.
Otherwise, osmo-bts answers sometimes using (0x00, 0xff,0xff) instead of
requested (0x00, 0x00, 0xff), for instance when ACKing an Admin Unlock.
This is kinda still fine since value 0xff has the meaning of "all"
addresses, and that means the only one available.
Still, it's not the proper way to identify the object, so this patch
fixes it.
Change-Id: I2ea05778f5b5ac335c75f3958324664553da7f0d
---
M src/common/bts.c
1 file changed, 20 insertions(+), 1 deletion(-)
Approvals:
laforge: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/common/bts.c b/src/common/bts.c
index 051f41b..29f0c56 100644
--- a/src/common/bts.c
+++ b/src/common/bts.c
@@ -280,7 +280,7 @@
memcpy(&bts->gprs.cell.timer, bts_cell_timer_default,
sizeof(bts->gprs.cell.timer));
gsm_mo_init(&bts->gprs.cell.mo, bts, NM_OC_GPRS_CELL,
- bts->nr, 0xff, 0xff);
+ bts->nr, 0, 0xff);
memcpy(&bts->gprs.cell.rlc_cfg, &rlc_cfg_default,
sizeof(bts->gprs.cell.rlc_cfg));
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32221
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I2ea05778f5b5ac335c75f3958324664553da7f0d
Gerrit-Change-Number: 32221
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32224 )
Change subject: Rearrange declaration of struct gsm_bts_gprs_nsvc
......................................................................
Rearrange declaration of struct gsm_bts_gprs_nsvc
Move it together with the other similar objects like gprs_nse and
gprs_cell.
Move the "mo" field to the start of the struct, similar to the other
types.
Change-Id: I5dc020a6bab8c94ab831b6ca506bc5cb681d07a3
---
M include/osmo-bts/bts.h
1 file changed, 26 insertions(+), 12 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
osmith: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/include/osmo-bts/bts.h b/include/osmo-bts/bts.h
index c5a0709..03a29e4 100644
--- a/include/osmo-bts/bts.h
+++ b/include/osmo-bts/bts.h
@@ -72,18 +72,6 @@
#define bts_internal_flag_set(bts, flag) \
bts->flags |= (typeof(bts->flags)) flag
-struct gsm_bts_gprs_nsvc {
- struct gsm_bts *bts;
- /* data read via VTY config file, to configure the BTS
- * via OML from BSC */
- int id;
- uint16_t nsvci;
- struct osmo_sockaddr local; /* on the BTS */
- struct osmo_sockaddr remote; /* on the SGSN */
-
- struct gsm_abis_mo mo;
-};
-
struct gprs_rlc_cfg {
uint16_t parameter[_NUM_RLC_PAR];
struct {
@@ -136,6 +124,18 @@
struct gsm_abis_mo mo;
};
+/* GPRS NSVC; ip.access specific NM Object */
+struct gsm_bts_gprs_nsvc {
+ struct gsm_abis_mo mo;
+ struct gsm_bts *bts;
+ /* data read via VTY config file, to configure the BTS
+ * via OML from BSC */
+ int id;
+ uint16_t nsvci;
+ struct osmo_sockaddr local; /* on the BTS */
+ struct osmo_sockaddr remote; /* on the SGSN */
+};
+
/* GPRS NSE; ip.access specific NM Object */
struct gsm_gprs_nse {
struct gsm_abis_mo mo;
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32224
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I5dc020a6bab8c94ab831b6ca506bc5cb681d07a3
Gerrit-Change-Number: 32224
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged