Some thoughts on RTCP packet JSON re-design.

[agafox]

There are a few issues in original RTCP packet JSON we found while reviewing following pull request to SIP3: https://github.com/sip3io/sip3-salto-ce/pull/77

I think it will be better to represent an RTCP packet JSON not as an object but as an array of RTCP reports JSON objects and here are the reasons why:

1. Current RTCP packet JSON will always have a type of the last report from RTCP packet. For instanse, if original RTCP packet contains SR and SDES you will be able to understand that it actually has SR by analizing sender_info only: https://github.com/sipcapture/heplify/blob/master/protos/rtcp.go#L222
2. In case if we received SR and RR in one and the same RTCP packet there will be a mess in report blocks (they will simply mix an information from both reports): https://github.com/sipcapture/heplify/blob/master/protos/rtcp.go#L286
3. In case if we received RTCP packet with 2 SRs apart from the mess in report blocks there will be a mess in sender_information(which may cause wrong calculations of R-Factor for the first SR): https://github.com/sipcapture/heplify/blob/master/protos/rtcp.go#L236

[lmangani]

Hello @agafox nice to see you here and thanks for the valuable observations!

The first thing I'd like to point out is how this is not an heplify exclusive, and the many other existing clients implementing the HEP RTCP type 5 is not easy to patch necessarily for existing integrations or passive ones - the format was designed to be schema copy of the original structure more or less since it is to be integrated by 3rd party clients who are busy doing the real job of switching RTP/RTCP and not dedicated to reporting, such as RTP:Engine, RTPProxy, Asterisk, Freeswitch, etc. The current choices are explicitly picked not to load up the reporting agent with any memory or computing role such as assembling details about previous reports, keeping track of them, etc. With the above in place, the RTCP Type 5 is not very malleable and should be considered as-is.

The good news is HEP is open for new types all day and all night. What we could do, assuming this does not create an overly lengthy reports in size is to create a HEP Type 6 RTCP which could be based on Type 5 + the array and any optimizations support future PRs and HEP definitions might bring.

Open for discussion.

[agafox]

That what I was thinking too. The easiest way is to introduce new type for RTCP packet with a better structure. If you want we can work together on the new packet design.

[negbie]

@agafox without putting too much effort into this I could use https://pkg.go.dev/github.com/pion/rtcp parser. You will get an array of parsed RTCP packets with following structure:

    expected := []Packet{
        &ReceiverReport{
            SSRC: 0x902f9e2e,
            Reports: []ReceptionReport{{
                SSRC:               0xbc5e9a40,
                FractionLost:       0,
                TotalLost:          0,
                LastSequenceNumber: 0x46e1,
                Jitter:             273,
                LastSenderReport:   0x9f36432,
                Delay:              150137,
            }},
            ProfileExtensions: []byte{},
        },
        &SourceDescription{
            Chunks: []SourceDescriptionChunk{
                {
                    Source: 0x902f9e2e,
                    Items: []SourceDescriptionItem{
                        {
                            Type: SDESCNAME,
                            Text: "{9c00eb92-1afb-9d49-a47d-91f64eee69f5}",
                        },
                    },
                },
            },
        },
        &Goodbye{
            Sources: []uint32{0x902f9e2e},
        },
        &PictureLossIndication{
            SenderSSRC: 0x902f9e2e,
            MediaSSRC:  0x902f9e2e,
        },
        &RapidResynchronizationRequest{
            SenderSSRC: 0x902f9e2e,
            MediaSSRC:  0x902f9e2e,
        },
    }

Code on my side would be tiny and wouldn't require more than this:

func ParsePionRTCP(data []byte) ([]byte, []byte, string) {
    packet, err := rtcp.Unmarshal(data)
    if err != nil {
        return nil, nil, err.Error()
    }
    bytes, err := json.Marshal(packet)
    if err != nil {
        return nil, nil, err.Error()
    }
    return uintsToBytes(packet[0].DestinationSSRC()), bytes, ""
}

func uintsToBytes(vs []uint32) []byte {
    buf := make([]byte, len(vs)*4)
    for i, v := range vs {
        binary.BigEndian.PutUint32(buf[i*4:], v)
    }
    return buf
}

I would introduce a new flag for this.

[agafox]

Thank you, @negbie! Looks perfect IMO. Pion is a future of RTC for sure, so it's good to re-use their parser. Plus packet structure is in accordance with RFC. I like it a lot.

[agafox]

However, as far as I see Pion doesn't have RTCP-XR support at the moment... I assume it's good to have a new flag compatible in terms of functionality. It's up to you guys.

[negbie]

Totally up to you. My first suggestion would cost me about 5min which is ok for me to spend this amount of time for such a small usecase. Adding XR reports on top of this would take more time which I can't just give away for free.

[agafox]

I'm all in with what you offered above. Talking about Pion implementation.

[lmangani]

My 2 cents - if this is to be sent over HEP, we can't just change the type format around and we need to keep in consideration other clients too in a healthy ecosystem. Changing it within the existing type with or without a flag (even if for the best) would only end up breaking the integrations. Create a new HEP type if this is a desirable format?

[negbie]

Yip new HEP Type should be choosed aswell. Will change that tomorrow mby.

[lmangani]

... and what is the point of bastardizing an existing HEP formats when its open for as many types as you wish? its an integer

[adubovikov]

please don't mix or change the current one, just make a new HEP RTCP type and set it inside. I think the change existing type 5 will make a MESS

[adubovikov]

or another way , use an own vendor ID in the chunk's vendor, but before register it in the list.

[negbie]

All good let's just use new HEP RTCP type. Any suggestions?

[adubovikov]

https://github.com/sipcapture/HEP/blob/master/docs/HEP3_Network_Protocol_Specification_REV_30.pdf base on this, 0x40 is free for a next one: "RTCP SHORT" ?

[negbie]

Ok, I will take 0x40. I don't think that it's really short because it produces a way bigger JSON than HEP Type 5. Pion RTCP parser produces with minimal introduced JSON tags something like this:

[{"SRSSRC":2884455299,"NTPTime":39684424097744,"RTPTime":3055083925,"PacketCount":229,"OctetCount":36640,"SenderReports":[{"SSRC":3124621683,"FractionLost":0,"TotalLost":0,"LastSequenceNumber":26124,"Jitter":0,"LastSenderReport":0,"Delay":0}],"ProfileExtensions":null},{"SourceDescriptions":[{"Source":2884455299,"Items":[{"Type":1,"Text":"sip:78903377@10.189.213.70"},{"Type":4,"Text":"78903377"}]}]},{"XRSSRC":2884455299,"VoIPMetricsReport":{"BlockType":7,"BlockLength":8,"SSRC":3124621683,"LossRate":0,"DiscardRate":0,"BurstDensity":0,"GapDensity":0,"BurstDuration":0,"GapDuration":0,"RoundTripDelay":0,"EndSystemDelay":78,"SignalLevel":234,"NoiseLevel":184,"EchoReturnLoss":75,"GapThreshold":16,"RFactor":91,"ExternalRFactor":127,"MeanOpinionScoreListening":43,"MeanOpinionScoreConversation":43,"RXConfig":48,"JitterBufferNominalDelay":0,"JitterBufferMaximumDelay":30,"JitterBufferAbsoluteMaximumDelay":300}}]

[adubovikov]

RTCP PION ?

[negbie]

Sounds good. RTCP PION (JSON)

[adubovikov]

sorry, I was calculated in the DEC.... 0x3a is the correct one

[negbie]

Ok let's take 0x3a.

[adubovikov]

https://github.com/sipcapture/HEP/blob/master/docs/HEP3_Network_Protocol_Specification_REV_31.pdf

Done

[negbie]

Implemented with HEP Type 0x3a or 58 in Decimal. To enable this new RTCP PION HEP type use the flag -m SIPRTCPPION

[adubovikov]

super!