Project

General

Profile

Bug #3114

libosmo-mgcp doesn't properly deal with SDP containing multiple codecs

Added by laforge 7 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
libosmo-mgcp
Target version:
-
Start date:
03/26/2018
Due date:
% Done:

100%


Description

libosmo-mgcp has a SDP parser that can deal with a rtpmap of up to 8 different codecs, but then only passes two of them up in the parsed data structure, see mgcp_rtp_end.codec and mgcp_rtp_end.alt_codec

This needs to change to make sure that the full list of codecs is parsed + passed into the actual MGW codebase, so the MGW can make an educated choice between the codecs.


Related issues

Related to osmo-sip-connector - Bug #1683: osmo-sip-connector: Implement codec selectionNew2016-03-31

History

#1 Updated by laforge 7 months ago

  • Related to Bug #1683: osmo-sip-connector: Implement codec selection added

#2 Updated by laforge 6 months ago

  • Assignee changed from sysmocom to dexter

#3 Updated by dexter 5 months ago

As a peperation I have cleaned up the existing codec selection. There is a codec and an alt_codec struct memeber. The codec is the primary field which is actually used in the code. However, alt_codec is not used at all. I have removed this alt_codec field now.

https://gerrit.osmocom.org/#/c/osmo-mgw/+/9234 sdp: remove unused alt_codec field from struct mgcp_rtp_end

#4 Updated by dexter 5 months ago

  • % Done changed from 0 to 70

I have worked myself through the specs and through the actual implementation of our SDP parser, which I find is not exactly easy to understand since it does some educated guesses to extrapolate the full codec information. But I think It now works almost as it should. It also still needs some final cleanup and fixing of the unittest (which I believe are violating the spec here and there)

I see we use payload type 255 here and there, but it is from the reserved range. The dynamic range is limited from 96 to 127.

see also pmaier/sdp

#5 Updated by dexter 5 months ago

I have done some more cleanups to untangle the codec handling. I think it should work like this:

1) Have SDP and LCO, take SDP and ignore LCO
2) Have LCO only, use LCO
3) Have no LCO, nor SDP, pick a sane default (see #2658)

The codec decision should only be made once after all codec information (LCO or SDP) has been collected. (I do not really understand why there are so many ways to obtain codec info)

Also I think it is very harmful to do initalize some magic dummy codecs on conn allocation. This makes things less understandable. The only place where codec settings should be initalized/altered should be CRCX and MDCX. Unfortunately removing the initalization from mgcp_conn.c messes up the unit-tests, but I this can be fixed of course.

#6 Updated by dexter 5 months ago

  • Status changed from New to In Progress

I think now the worst quirks with the internal codec handling of osmo-mgw are now cleaned up. I also have fixed the mgcp_client now so that it can send ptime, payload-type, codec name and rate. When I populate the new struct members with some static data in osmo-msc then it looks fine on the trace. The MGW now properly accepts what I send and returns the codec data I have sent. I still have to fix the fsm based client as well. Once that is done we can continue with the codec decision.

I would try the codec decision as follows:
- Accept all codecs from the first leg, whatever it is
- Do an intersection with the codecs from the second leg and accept only that intersection.

I wonder if this can work at all. The second leg will indeed send with a codec that is understood by the first leg. But how does the first leg know which codec it has to use so that the second leg can decode?

#7 Updated by laforge 5 months ago

I would try the codec decision as follows:
- Accept all codecs from the first leg, whatever it is
- Do an intersection with the codecs from the second leg and accept only that intersection.

That's the only valid approach until we do transcoding.

I wonder if this can work at all. The second leg will indeed send with a codec that is understood by the first leg. But how does the first leg know which codec it has to use so that the second leg can decode?

The MGW is not involved in making any decisions whatsoever. It simply has to execute
what it's being told. The decision is made in OsmoMSC and/or osmo-sip-connector.

#8 Updated by dexter 4 months ago

The MGW is not involved in making any decisions whatsoever. It simply has to execute
what it's being told. The decision is made in OsmoMSC and/or osmo-sip-connector.

So that would mean we must be sure that both parties choose the same set of codecs. At least for the non-transcoding case this looks like a must. But if we do transcoding? Then the MGW would choose from the set of codecs the other connection supports and do the transcoding?

#9 Updated by laforge 4 months ago

On Mon, Jun 11, 2018 at 08:00:09PM +0000, dexter [REDMINE] wrote:

The MGW is not involved in making any decisions whatsoever. It simply has to execute
what it's being told. The decision is made in OsmoMSC and/or osmo-sip-connector.

So that would mean we must be sure that both parties choose the same set of codecs. At least for the non-transcoding case this looks like a must.

yes, the MGW should simply refuse a CRCX/MDCX if there's no intersection with the other connection.

But if we do transcoding? Then the MGW would choose from the set of codecs the other connection supports and do the transcoding?

correct. And which to chose becomes primarily a matter of policy. Some use
cases would want to optimize for speech quality, while others may want to
optimize on cpu consumption. But we're far from that at the moment.

#10 Updated by dexter 4 months ago

  • % Done changed from 70 to 80

I have submitted my changes to review now, there is still work to do on the codec selection side. We now can handle multiple codecs in SDP, the parsed information is checked kept properly in structs. I also modularized the code and moved everything related to codec handling into a separate c file.

Also we do not support multiple codecs in LCO yet, which I think is an issue for the side pointing towards the PBX, what if the PBX supports multiple codecs and wants to negotiate them with the CRCX? Probably we should create a task for it?

However, I tested the patch and osmo-msc/osmo-bsc work fine with it. Also the TTCN3 tests pass nicely.

See also:
https://gerrit.osmocom.org/#/c/osmo-mgw/+/9648/

#11 Updated by dexter 4 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 80 to 100

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)