Move GCC rewrite from interceptor to this repo #61
Conversation
|
@mengelbart, I like the new, cleaner BWE interface. I want to understand the implementation more deeply, so I might need an additional 1-2 days for review, if that's not a problem. |
|
Yes, please take all the time you need. We don't need to rush and it's much better if there's someone else looking over this, too! Please let me know if anything is unclear. |
| ECNCE // 11 | ||
| ) | ||
|
|
||
| type Acknowledgment struct { |
There was a problem hiding this comment.
Perhaps we could move this to a separate package? This would allow us to:
- Define a generic interface for estimators (currently
OnAcks(arrival time.Time, rtt time.Duration, acks []Acknowledgment) int). - Extend the interface later (e.g. provide more data).
- Support multiple estimators under a unified pattern.
| Arrival: time.Time{}.Add(30 * time.Millisecond), | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Could you clarify how out-of-order packets are being handled? I notice this packet appears in the expected output, which suggests it's being processed normally rather than ignored.
| for len(*e.history) > 0 && (*e.history)[0].arrival.Before(deadline) { | ||
| heap.Pop(e.history) | ||
| } | ||
| earliest := e.latestArrival |
There was a problem hiding this comment.
If we:
- Track the earliest timestamp in the heap (O(1) access)
- Maintain a running sum of values in the heap
Then GetRate() could run in O(1) time since we'd just need to:
return 8*sum / (now - earliest)
| Departure time.Time | ||
| Arrived bool | ||
| Arrival time.Time | ||
| ECN ECN |
|
|
||
| import "time" | ||
|
|
||
| func MeasureRTT(reportSent, reportReceived, latestAckedSent, latestAckedArrival time.Time) time.Duration { |
There was a problem hiding this comment.
This doesn't appear to be directly related to congestion control. Could we move this computation to the caller instead?
gcc/delay_rate_controller.go
Outdated
| "github.com/pion/logging" | ||
| ) | ||
|
|
||
| const maxSamples = 1000 |
|
@aalekseevx Thanks for reviewing and sorry I didn't look at it yet. I will probably have some time to continue working on this soon but I'd like to get pion/interceptor#300 done first because it blocks this PR. |
mengelbart
force-pushed
the
gcc-refactoring
branch
2 times, most recently
from
5b89400 to
b5bd729
Compare
mengelbart
force-pushed
the
gcc-refactoring
branch
from
abc63d2 to
5362b62
Compare
|
Moving this here: pion/bwe#2 |
Description
Moves refactored BWE implementation from interceptors to this repo (as discussed here).