interceptor icon indicating copy to clipboard operation
interceptor copied to clipboard

A question about arrival_group_accumulator.go

Open bug45 opened this issue 1 year ago • 4 comments

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

bug45 avatar Aug 26 '24 04:08 bug45

@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?

thatsnotright avatar Aug 30 '24 15:08 thatsnotright

@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.

bug45 avatar Sep 03 '24 07:09 bug45

@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).

kcaffrey avatar Sep 03 '24 13:09 kcaffrey

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.

mengelbart avatar Sep 03 '24 18:09 mengelbart

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

sterlingdeng avatar Nov 24 '24 22:11 sterlingdeng