Attention is currently required from: pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32083 )
Change subject: mgcp_codec: refactor payload type converstion
......................................................................
Patch Set 2:
(1 comment)
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32083/comment/0ee60367_4f89d1da
PS2, Line 516: codec_dst = mgcp_codec_find_convertible(conn_dst, codec_src);
> IIUC you also need to do a 2 stage check here. […]
The function mgcp_codec_find_convertible always prefers codecs that are the same. Only when there are no codecs that are the same, then a convertible codec is picked.
(it is assumed that that a "same" codec is always convertible)
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I085260a2ca8cfecdb58656b7a046c536189e238d
Gerrit-Change-Number: 32083
Gerrit-PatchSet: 2
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: Mon, 27 Mar 2023 16:39:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, laforge.
falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/32078 )
Change subject: osmo_rtp2trau() for FR & EFR: correctly handle the no-data case
......................................................................
Patch Set 1:
(2 comments)
File src/trau/trau_rtp_conv.c:
https://gerrit.osmocom.org/c/libosmo-abis/+/32078/comment/1e7ed5a4_a883df1a
PS1, Line 282: } else if (data_len == 0) {
: /* C1 .. C5: idle speech */
: tf->c_bits[0] = 0;
: tf->c_bits[1] = 1;
: tf->c_bits[2] = 1;
: tf->c_bits[3] = 1;
: tf->c_bits[4] = 0;
Please read the rules of TS 48.060, particularly sections 6.5.2 and 6.5.3. Here is a bullet-point summary of these rules:
* In the case of DL, for both FR and EFR, there is no choice: the TRAU must send an idle speech frame, which is distinguished from a regular speech frame by a unique C1..C5 pattern.
* In the case of UL for EFR, the CCU must send a regular EFR speech frame (not idle speech frame) with BFI=1 - once again, no choice given.
* In the case of UL for FR, the CCU has the option of sending either a regular speech frame with BFI=1 or an idle speech frame.
Thus there is only one case where we as implementors get a choice: the case of FR UL. My patch implements the option of sending BFI=1, consistently with EFR when such behaviour is required. So let's decide on this issue first, before arguing over code structure: should we send a speech frame with BFI=1 or an idle speech frame in this case?
Also @laforge wrote:
> Basically above where the "FIXME: Generate SID frames?" comment is (which is no longer a FIXME then.
This FIXME is fixed, and that comment removed, in the second patch of the series.
https://gerrit.osmocom.org/c/libosmo-abis/+/32078/comment/b866177b_87816889
PS1, Line 441: if (data_len == 0 && tf->dir == OSMO_TRAU_DIR_DL) {
> (wondering as above: why only for DL?)
In the present case (EFR codec) the answer to the "why" question is straightforward: because TS 48.060 section 6.5.3 says so.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/32078
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: I7f21e2210bba9ef87f1af515a001a0cfc1c0a1d8
Gerrit-Change-Number: 32078
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Mon, 27 Mar 2023 16:35:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32083 )
Change subject: mgcp_codec: refactor payload type converstion
......................................................................
Patch Set 2:
(1 comment)
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32083/comment/abf95e12_4062732b
PS2, Line 516: codec_dst = mgcp_codec_find_convertible(conn_dst, codec_src);
IIUC you also need to do a 2 stage check here. Otherwise you may return a codec which needs mangling before returning a codec which only needs forwarding.
So you first check if you find "codec_same", and otherwise look for convertible.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I085260a2ca8cfecdb58656b7a046c536189e238d
Gerrit-Change-Number: 32083
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 27 Mar 2023 15:58:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: neels, dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32084 )
Change subject: mgcp_codec: cosmetic: remove line break in api-doc
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Its just that we agreed not to use them, also its the only function in this part of the code that ha […]
I don't recall that agreement and I see no much point for it. I'm not against merging this either, but I wouldn't bother fixing this kind of stuff imho.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32084
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I025bbf36287ef014e294cc18a6435d7e2f9c1bff
Gerrit-Change-Number: 32084
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 27 Mar 2023 15:54:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: dexter.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/32083
to look at the new patch set (#2).
Change subject: mgcp_codec: refactor payload type converstion
......................................................................
mgcp_codec: refactor payload type converstion
in mgcp_codec.c we have a function mgcp_codec_pt_translate that is used
to find the equivalent payload type number on an opposite connection.
This is quite specific and the resulting payload type number may still
belong to a codec that might require some form of conversion.
Lets refactor this so that the function just finds a convertible codec
for a given connection. This also opens up other usecases. The payload
type conversion like we did it before can then be done with a few lines
in mgcp_network.c
Related: OS#5461
Change-Id: I085260a2ca8cfecdb58656b7a046c536189e238d
---
M include/osmocom/mgcp/mgcp_codec.h
M src/libosmo-mgcp/mgcp_codec.c
M src/libosmo-mgcp/mgcp_network.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
5 files changed, 113 insertions(+), 65 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/83/32083/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I085260a2ca8cfecdb58656b7a046c536189e238d
Gerrit-Change-Number: 32083
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: neels, pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32084 )
Change subject: mgcp_codec: cosmetic: remove line break in api-doc
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I sometimes add them, I don't really see anything wrong with this tbh.
Its just that we agreed not to use them, also its the only function in this part of the code that has apidoc formatted that way.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32084
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I025bbf36287ef014e294cc18a6435d7e2f9c1bff
Gerrit-Change-Number: 32084
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 27 Mar 2023 15:51:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin, fixeria.
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/32050 )
Change subject: gmm: Initial implementation of GPRS Detach
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
File src/gmm/gmm.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/32050/comment/a3a4ed8d_87da1294
PS1, Line 376: /* TODO:
> Yes, I still need to find out how to fill up those. […]
Ack
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/32050
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: If6cbb1d425b3a9f713348f1dea4747e2b6be0a44
Gerrit-Change-Number: 32050
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 27 Mar 2023 15:41:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: osmith <osmith(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment