Project

General

Profile

Bug #1661

voice not audible in TCH/F-TCH/F calls using osmo-bts-octphy and ortp-0.25

Added by laforge over 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
High
Assignee:
Category:
-
Target version:
-
Start date:
03/17/2016
Due date:
% Done:

100%

Spec Reference:

Description

With the latest ORTP Package (0.25), Voice is not audible during the call. We had
to use old version such as 0.23 or 0.24 for voice to work. (observed in case
of TCHF-TCHF as well)


Related issues

Related to libosmo-abis - Bug #1785: RTP network errorClosed2016-07-28

History

#1 Updated by laforge over 2 years ago

  • Assignee set to msuraev

#2 Updated by msuraev over 2 years ago

  • Status changed from New to In Progress

On which GNU/Linux distribution this was observed?

#3 Updated by laforge over 2 years ago

  • Priority changed from Normal to High

#4 Updated by msuraev over 2 years ago

  • Status changed from In Progress to Feedback
  • Assignee changed from msuraev to laforge

Should be fixed by 8c119f7a0510b75e7fa1b96a37f2a6650e13824f in libosmo-abis.

#5 Updated by laforge over 2 years ago

  • Status changed from Feedback to Closed

let's close the issue and re-open only in case users report it as still broken.

#6 Updated by laforge over 2 years ago

  • Related to Bug #20: phone calls between AMR-capable handset and EFR-only handset added

#7 Updated by laforge over 2 years ago

  • Related to deleted (Bug #20: phone calls between AMR-capable handset and EFR-only handset)

#8 Updated by laforge about 2 years ago

  • Related to Bug #1785: RTP network error added

#9 Updated by msuraev about 2 years ago

  • Status changed from Closed to New

Notice: the error was re-introduced in c77c2a6aa13accbc558888ab788d1148eb9aeb1a.

#10 Updated by neels about 2 years ago

  • Assignee changed from laforge to msuraev

AFAICT c77c2a6aa13accbc558888ab788d1148eb9aeb1a is quite clearly a
fix of erratic use of libortp (at least version 0.25).

The commit being reverted, 8c119f7a0510b75e7fa1b96a37f2a6650e13824f, says:

Set connected mode after setting remote address

ortp: according to oRTP documentation rtp_session_set_connected_mode()
uses the address set by rtp_session_set_remote_addr() - move the
function call accordingly.

Fixes: OS#1661

Looking at the ortp code, that is definitely not the case.
rtp_session_set_connected_mode() simply sets a flag which is used in
rtp_session_set_remote_addr().

void rtp_session_set_connected_mode(RtpSession *session, bool_t yesno){
        session->use_connect=yesno;
}
static int
_rtp_session_set_remote_addr_full ([...]) { [called by rtp_session_set_remote_addr()]
[...]
        if (can_connect(session)){
                if (try_connect(session->rtp.gs.socket,(struct sockaddr*)&session->rtp.gs.rem_addr,session->rtp.gs.rem_addrlen))
                        session->flags|=RTP_SOCKET_CONNECTED;
[...]
}
#define can_connect(s)  ( (s)->use_connect && !(s)->symmetric_rtp)

I wonder how introducing this bug could fix this issue, and how fixing the bug
could again introduce the problem observed in OS#1661. It seems to be some
unintended side effect.

Can you clarify?

#11 Updated by msuraev about 2 years ago

According to API docs - see http://download-mirror.savannah.gnu.org/releases/linphone/ortp/docs/rtpsession_8h.html#a84cdf75e48fea6e42b67da998e67fbda for instance:

"If yesno is TRUE, thus a connect() syscall is done on the socket to the destination address set by rtp_session_set_remote_addr()" which I interpret as "rtp_session_set_remote_addr() have to be called before rtp_session_set_connected_mode()".

The tests so far have showed that this is indeed the right order for later ortp versions. I'm not sure yet how to please both old and new - or if it's worth the efforts at all: we can just fix on latest stable version ortp (0.22.0 in Debian stable for example).

#12 Updated by msuraev about 2 years ago

At the same time examples supplied with ortp do the opposite - see src/tests/rtpsend.c:81
It's really hard to work with the library when library's documentation contradict library's examples.

#13 Updated by laforge about 2 years ago

On Mon, Sep 19, 2016 at 11:53:16AM +0000, msuraev [REDMINE] wrote:

At the same time examples supplied with ortp do the opposite - see src/tests/rtpsend.c:81
It's really hard to work with the library when library's documentation contradict library's examples.

It might be worth raising this at the apropriate mailing list or issue
tracker and ask for guidance.

--
- Harald Welte <> http://laforge.gnumonks.org/ ============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)

#14 Updated by msuraev about 2 years ago

Already did - no response so far. From previous experience it could be weeks before upstream devs react if at all.

#15 Updated by neels about 2 years ago

I have analysed a bit further. The starting point was that I switched to using the
SysmoBTS SDK version 1.5.4, which includes a newer libortp version, and with that
voice was broken (only background noise).

Then Max said:

This might be related to commit c77c2a6aa13accbc558888ab788d1148eb9aeb1a
in osmobts [libosmo-abis]

If you roll-back this commit, does voice re-appear?

Wow, it actually does! But when looking at the ortp code as described in detail
in OS#1661, this makes no sense at all. It should not fix anything.
I tried to understand the ortp code:

rtp_session_set_connected_mode() simply sets a flag (use_connect=1) which is
used in rtp_session_set_remote_addr(). Setting the flag only later amounts to
setting a different flag value (use_connect=0) for the first connection. The
main difference: with use_connect=1, rtp_session_set_remote_addr() establishes
a connect() already and sets the "is connected" flag, and with use_connect=0
it seems only the socket is opened.

But the use_connect flag is also queried during rtp_process_incoming_packet().
This calls rtp_session_update_remote_sock_addr(), to update the address to the
remote counterpart, and will connect only if the "is connected" flag is not set yet.

So I suspect this:

When use_connect 1, rtp_session_set_remote_addr() connects and sets the
connection flag. Then the rtp_session_update_remote_sock_addr() wants to update
to the real remote counterpart, but this has no effect, since the flag that
we're connected is already set, and the code does not reconnect the socket.
We still have a connect() to the initial wrong remote address.

When use_connect 0, rtp_session_set_remote_addr() does not connect(), and
the rtp_session_update_remote_sock_addr() has an effect, since the connection
is established only in rtp_process_incoming_packet(). So by setting the
use_connect only after rtp_session_set_remote_addr(), the first incoming
packet does the connect() when it knows the proper remote address.

This is confirmed by a hack: with use_connect == 1 at the top, voice is broken;
if I then set the internal ortp flags to not-connected, voice works: the code
reconnects with the updated address upon receiving a packet.

--- a/src/trau/osmo_ortp.c
+++ b/src/trau/osmo_ortp.c
@@ -401,6 +401,9 @@ int osmo_rtp_socket_connect(struct osmo_rtp_socket *rs, const char *ip, uint16_t
        if (rc < 0)
                return rc;

+       rs->sess->flags &= ~(1<<8); // RTP_SOCKET_CONNECTED=1<<8
+       rs->sess->flags &= ~(1<<9); // RTCP_SOCKET_CONNECTED=1<<9
+
        if (rs->flags & OSMO_RTP_F_POLL)
                return rc;
        else

What appears ugly at this point:

- each rtp_process_incoming_packet() call unconditionally does
rtp_session_update_remote_sock_addr(), but it appears to have an effect only
the first time it is called.

- rtp_process_incoming_packet() should probably update the address on the first
time only.

- rtp_process_incoming_packet() should detect whether an address change is
needed and disconnect the socket (so that it is reconnected in the code
below).

Hence it would be possible to set the use_connect flag a priori and the other
code would automagically know whether a reconnect is necessary.

But in summary, setting the use_connect flag "too late" actually fixes voice
for newer libortp.

The rtp_process_incoming_packet() was introduced after libortp 0.16.5 which
might explain why setting use_connect = 1 at the top doesn't break voice for
libortp 0.16.5.

#16 Updated by neels about 2 years ago

Todo: check whether litecell15 also works with a recent ortp-0.25 with https://gerrit.osmocom.org/1011 applied.
(The instructions were to use 0.16.5)

#17 Updated by neels about 2 years ago

To illustrate the landscape a bit further ...

  • libortp versions:
    • The SDK for Litecell 1.5 recommends using libortp v0.16.5.
    • Recent libosmo-abis has moved to requiring a minimum of libortp v0.22.0
  • c77c2a6aa13accbc is a patch originally suggested by the Litecell 1.5 SDK.
    It looked like a fix at first to me, so I readily applied it, but now the
    situation turns out to be far more complex.
  • sysmoBTS SDK versions:
    • The sysmoBTS SDK v1.5.3 works fine with c77c2a6aa13accbc, presumably
      because it still includes an older libortp.
    • The sysmoBTS SDK v1.5.4 produces no voice when c77c2a6aa13accbc is applied,
      presumably because it includes a more recent libortp version.

It seems "recent" libortp internals around the set_connected_mode flag have
either introduced a regression, or alternatively a (seemingly bad) design
choice forces us to take more care when setting that flag.

We should clarify these:

  • Why does c77c2a6aa13accbc matter when changing to a recent libortp? Is that
    situation intended by libortp upstream or actually a regression?
  • Why exactly does Litecell 1.5 need libortp 0.16.5?
    • Can we accept Litecell 1.5 to stick to a libortp version that current
      libosmo-abis master clearly rejects as too old?
    • What would it take to pull Litecell 1.5 up to a recent libortp?
  • Furthermore, why does Litecell 1.5 need c77c2a6aa13accbc on top of an old
    libortp 0.16.5? (Asking because for sysmoBTS it seems that an older libortp
    makes c77c2a6aa13accbc not matter much.)

#18 Updated by msuraev about 2 years ago

The latest litecell sdk has oRTP version 0.22.0 which I've tested recently and it works fine. They promised that documentation update will follow but it has not materialized yet. Hence we can focus on testing with oRTP 0.22.0+ ignoring older versions which are not supported by upstream anyway. See for SYS#2569 details.

#19 Updated by laforge about 2 years ago

On Sun, Oct 09, 2016 at 10:27:44PM +0000, neels [REDMINE] wrote:

  • Furthermore, why does Litecell 1.5 need c77c2a6aa13accbc on top of an old
    libortp 0.16.5? (Asking because for sysmoBTS it seems that an older libortp
    makes c77c2a6aa13accbc not matter much.)

As all this is related to RTP audio socket binding/connecting behavior,
it might very well be that the test environment for the two BTS models
is different, too. sysmoBTS is normally tested against OsmoNITB, which
I believe is not the case for LC1.5.

So when making stateents like "commit xxxx breaks LC15", I think it is
very important to describe the test environment for this.

A different BSC implementation than our libbsc might very well be using
different parameters and timing for the RSL IPA CRCX/MDCX messages that
affect RTP sockets.

--
- Harald Welte <> http://laforge.gnumonks.org/ ============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)

#20 Updated by msuraev about 2 years ago

As original bug refers to osmo-bts-octphy, it got to be tested as well.

#21 Updated by dexter about 2 years ago

Checked it with libortp 0.25.0 on OCTBTS3500 with most recent firmware (02.07.00-B1039) and it works fine. However, the recent libortp (0.27.0 and above) there will be problems because of API changes.

#22 Updated by msuraev about 2 years ago

  • Status changed from New to Resolved
  • Assignee changed from msuraev to laforge
  • % Done changed from 0 to 100

As it works for osmo-bts-octphy again I suggest to close this and move further lc15-specific discussion to linked #1785.

#23 Updated by laforge almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)