Project

General

Profile

Feature #4150

dead code / If an MDCX omits SDP, all codec config might be cleared

Added by neels 4 months ago.

Status:
New
Priority:
Low
Assignee:
-
Category:
-
Target version:
-
Start date:
08/13/2019
Due date:
% Done:

0%


Description

CRCX and MDCX may include elaborate SDP including lists of codecs and parameters for each of them (like AMR's 'octet-aligned' and 'mode-set'...).

If osmo-mgw receives an MDCX without any SDP at all, it might completely clear this list of codecs and only keeps the "currently selected" codec.
See handle_codec_info():

/* Collect codec information /
if (have_sdp) {
...
} else if (endp->local_options.codec) {
/
When no SDP is available, we use the codec information from * the local connection options (if present) */
mgcp_codec_reset_all(conn);
rc = mgcp_codec_add(conn, PTYPE_UNDEFINED, endp->local_options.codec, NULL);
if (rc != 0)
goto error;
}

So the codec parameters are flushed, and only the codec type is re-added without any parameters.
That seems wrong.

This is apparently the only reason why PTYPE_UNDEFINED is passed to mgcp_codec_add(), with a longish code path for this case that seems unnecessary.

One approach would be to not change anything if SDP is missing.

Another would be to always require SDP.

I think why this condition exists is because sometimes we send a codec in the MGCP "L: ... a:FOO" part of a CRCX:

CRCX 1 rtpbridge/*@bsc MGCP 1.0
C: 2
L: p:20, a:AMR, nt:IN
M: recvonly

So e.g. the "a:AMR" sets endp.local_options.codec in set_local_cx_options(), and that is meant to populate the codec list with a default entry for AMR.
The OK-reply for this CRCX actually reflects this entry:

200 1 OK
Z: rtpbridge/0@bsc
I: 30DA177E
v=0
o=- 30DA177E 23 IN IP4 192.168.178.54
s=-
c=IN IP4 192.168.178.54
t=0 0
m=audio 50004 RTP/AVP 112
a=rtpmap:112 AMR
a=ptime:20

Bad style / problems with this:

  • the 'a:' option of an 'L:' header lacks ftmp parameters like the AMR octet-aligned,mode-set,... settings.
  • the 'a:' option of an 'L:' header isn't up to spec, it supports only one codec while specs allow more than one.
  • the 'a:' option of an 'L:' header as specified isn't good enough, that's the point of adding SDP.
  • If the 'a:' header should indeed set a codec in osmo-mgw, it should be a special case inside the CRCX parsing calling a proper mgcp_codec_add() with correct payload type argument, the special case should not bleed into SDP parsing, i.e. not into mgcp_codec_add().
  • The local_options from one CRCX could remain set on an endp and affect later MDCX that have neither an MGCP "L:... a:FOO" line nor contain SDP.

This is low priority because all of our MGCP peers send SDP all the time, or at least follow up with SDP in an MDCX later on, overwriting the effects of this code.
Also our MGCP peers never omit SDP from MDCX after having sent SDP.

Still, to me this looks like cruft that we are not actually using.
Decide what to do with it, IMHO lose all of it and only heed SDP.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)