Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A question about arrival_group_accumulator.go #274

Open
bug45 opened this issue Aug 26, 2024 · 5 comments · May be fixed by #291
Open

A question about arrival_group_accumulator.go #274

bug45 opened this issue Aug 26, 2024 · 5 comments · May be fixed by #291

Comments

@bug45
Copy link

bug45 commented Aug 26, 2024

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
Copy link
Contributor

@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
Copy link
Author

bug45 commented Sep 3, 2024

@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
Copy link
Contributor

kcaffrey commented Sep 3, 2024

@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
Copy link
Contributor

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 sterlingdeng linked a pull request Nov 24, 2024 that will close this issue
@sterlingdeng
Copy link
Contributor

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

sterlingdeng pushed a commit to sterlingdeng/interceptor that referenced this issue Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@thatsnotright @kcaffrey @mengelbart @sterlingdeng @bug45 and others