Attention is currently required from: falconia, pespin.

View Change

10 comments:

• Patchset:

  • Patch Set #2:

    Idea: It may make sense at some point to get rid of such VTY comamnd and somehow be able to specify […]

    yes, there has been discussion.

    The autodetection came up because there is no widely accepted way to indicate varying HR formats in SDP.
    The proper way would be to indicate the HR format in fmtp from each MGCP client, but for that we have to invent a new fmtp, and also the BSC has to then be aware of the HR formats in use. That would actually be correct, too. That is also how dexter started out until i started interfering, see https://gerrit.osmocom.org/c/osmo-mgw/+/31214

    So this here might all be one huge show of extra effort introduced by me for dubious reasons.

    But because HR formats are so trivially detectable, I thought why not keep that out of our 3GPP components and just convert it implicitly in the MGW. It would save us from many changes and added complexity in our MGCP clients, always having to indicate HR formats in fmtp as well...

    I do think this point is still valid, I am not 100% sure whether my idea of autodetection is in the end going to bite us for some reason... Maybe it would be better to invent a fmtp instead.

    So far, I still would like to try the autodetection, because it seems the far simpler way, avoiding TS101318 from cascading into our 3GPP components.

• File include/osmocom/mgcp/mgcp_rtp_end.h:

  • Patch Set #2, Line 40: bool hr_ts101318_detected;

    I would prefer better layer separation for these:

    • Here should be an indicator whether and to which fmt to convert, i see an enum here. Who or what detects this or even if any detection happened at all should not be relevant here.
    • the new field should reflect the target format, so we do not need to go back to SDP details.

    My humble suggestion, replace both the current rfc5993_convert flag and the hr_tsnnnn_detected flag with sth like

    ~~~
    enum hr_format {
        HR_FMT_UNSET,
        HR_FMT_RFC5993,
        HR_FMT_TS101318,
        HR_FMT_TWTS002,
    };

    struct mgcp_end {
        ...
        enum hr_format convert_hr_to;
        ...
    };

    ...

    static int rfc5993_hr_convert(struct msgb *msg, struct mgcp_conn_rtp *dst)
    {
        switch (hr_conversion) {
        case HR_FMT_UNSET:
            /* never convert */
            break;
        case HR_FMT_RFC5993:
            /* always emit RFC5993 */
            if (rtp_plen == GSM_HR_BYTES_RTP_TS101318) {
                /* TS 101318 encoding => RFC 5993 encoding */
                ...
            }
            break;
        case HR_FMT_TS101318:
            /* always emit TS 101.318 */
            if (rtp_plen != GSM_HR_BYTES_RTP_TS101318) {
                /* RFC5993 => TS 101318 encoding */
                ...
            }
            break;
        ...
    ~~~

    (while at it I would also put the actual conversion in separate functions, one per switch case.)

• File src/libosmo-mgcp/mgcp_network.c:

  • Patch Set #2, Line 646: exist 3 different RTP payload formats

    There exist maybe lots of formats for HR, but there exist TWO widely deployed formats, plus we have another Osmocom-specific extension, right? TWTS is taking too much mindshare here.

    There should be an easy way to TL;DR for a busy hacker:
    This API doc should efficiently and shortly describe what the function does. When that is clear and clean, there can be another *separate* paragraph about the larger context, and often that larger context should rather be described in the manual instead, so users can find out without having to read the src.

  • Patch Set #2, Line 691: * consider the full RTP chain of a single call:

    (my idea so far, can be in a future patch, is to not send any HR payload to a peer until we have received its first packet; this should be configurable via maybe two new vty options sort of like "[no] hr-auto-detect" to switch auto-detection, and another one maybe "[no] hr-drop-undetected" for dropping HR packets until we have detected something...)

    (As in my above comment: this function here should only and simply convert according to that enum value, and let's describe detection complexity and decision quirks elsewhere.)

  • Patch Set #2, Line 698: will always be RFC 5993

    Ideally, according to 3GPP yes, but in practice, do we always convert to RFC5993?

    But again i have the problem, the feature *is* almost a layer violation: we are in the MGW -- ideally, the BSC,MSC,etc should make these decisions, and the MGW should just adhere to instructions given. So this autodetection of HR formats should be as flat and simple as possible, and this doc should try to avoid tying too much into AoIP specs, really.

  • Patch Set #2, Line 715: static int rfc5993_hr_convert(struct msgb *msg, struct mgcp_conn_rtp *dst)

    (kind of weird to swap the msgb argument position for no reason)

  • Patch Set #2, Line 1577: Th

    s/This internal function examines/Examine/

  • Patch Set #2, Line 1658: Autodetect

    s/between // ?

  • Patch Set #2, Line 1661: int rc = examine_rtp_rx_gsmhr(msg, conn_src);

    Is this called always on GSM-HR-08 or only once, until we successfully detected the HR format? ... It is a minor tweak but it would be needed only once at the start of the HR stream.

  • Patch Set #2, Line 1666: mgcp_conn_watchdog_kick(conn_src->conn);

    It probably makes sense to call this even if output is wrong, but not a hard opinion.

    (i'm not sure, but if yes then this kick should go below "out_free:" ? -- i see this change in a separate patch anyway, so let's resolve the comment for the sake of *this* patch)

To view, visit change 39869. To unsubscribe, or for help writing mail filters, visit settings.