pion / interceptor

Pluggable RTP/RTCP processors for building real time communication
https://pion.ly/
MIT License
114 stars 63 forks source link

A question about arrival_group_accumulator.go #274

Open bug45 opened 3 months ago

bug45 commented 3 months ago
func (a *arrivalGroupAccumulator) run(in <-chan []cc.Acknowledgment, agWriter func(arrivalGroup)) {
    init := false
    group := arrivalGroup{}
    for acks := range in {
        for _, next := range acks {
            if !init {
                group.add(next)
                init = true
                continue
            }
            if next.Arrival.Before(group.arrival) {
                // ignore out of order arrivals
                continue
            }
            if next.Departure.After(group.departure) {
                if interDepartureTimePkt(group, next) <= a.interDepartureThreshold {
                    group.add(next)
                    continue
                }

                if interArrivalTimePkt(group, next) <= a.interArrivalThreshold &&
                    interGroupDelayVariationPkt(group, next) < a.interGroupDelayVariationTreshold {
                    group.add(next)
                    continue
                }
                // println("Arrival len", len(group.packets), group.packets[len(group.packets)-1].Departure.Sub(group.packets[0].Departure).Milliseconds())
                agWriter(group)
                group = arrivalGroup{}
                group.add(next)
            }
        }
    }
}

The above code is excerpted from lines 23 to 55 of arrival_group_accumulator.go. After each new pkt is added, the group's departure_time is updated to the new packet's departure_time. In other words, as long as the time difference between the departure_time of the newly added packet and the previous packet is less than the threshold, it will be added to the group. How can we ensure that the time difference between the first packet and the last packet is less than 5 ms (the threshold)?

@thatsnotright

thatsnotright commented 3 months ago

@bug45 I'm not sure I'm understanding your question or specifically tagging me. Are you looking to use the jitter buffer but still need to discard out of order packets?

bug45 commented 3 months ago

@thatsnotright Thx for your reply! I am not sure if this issue is related to the jitter buffer. My question is as follows: In https://datatracker.ietf.org/doc/html/draft-ietf-rmcat-gcc-02, " A sequence of packets which are sent within a burst_time interval constitute a group." But in the code above, it only considers the time difference between adjacent packets, rather than whether the sending times of a series of packets fall within a burst time interval. For example, for three consecutive packets, Packet 1, Packet 2, and Packet 3, if the departure_time difference between Packet 1 and Packet 2 is 3 ms and the departure_time difference between Packet 2 and Packet 3 is also 3 ms, then the departure_time difference between Packet 1 and Packet 3 is 6 ms, which would exceed the burst time. However, Packet 1, Packet 2, and Packet 3 are still considered as an arrival group. This is different from 'A sequence of packets which are sent within a burst_time interval constitute a group.' The code above shows a different behavior.

kcaffrey commented 3 months ago

@bug45 I am not sure why you tagged @thatsnotright but I don't see any commits from that user for the file in question. It looks like @mengelbart was the primary contributor.

However, I do think you might be right that the code has a discrepancy from both the RFC and libwebrtc's implementation regarding the group length threshold. The current implementation of gcc in libwebrtc doesn't precisely follow what the draft RFC states, but it does seem to only group packets together when the delta between the first and last send time is less than 5ms (plus a few other conditions around the relative arrival times).

mengelbart commented 2 months ago

I think you both are right, @bug45 and @kcaffrey. I currently don't have the time to fix it, but I am happy to review a PR.

sterlingdeng commented 1 week ago

I noticed the same issue when I was doing a rewrite of GCC for quic. I opened #291 to fix the issue. PTAL