Add webrtc transport-cc feedback mechanism #1115
Conversation
1、improve the accuracy of gcc bwe when uplink network exists a big propagate jitter(eg: 100ms) 2、this optimization logic is ported from google webrtc(see: RemoteEstimatorProxy)
|
Please leave review rather than individual comments, too many notifications otherwise 🙏 |
I thought I was doing that but definitely I did it wrong. Sorry. |
kingqn0321
force-pushed
the
v3_update_webrtc_transport_cc_feedback
branch
from
c8076b2
to
b810ab0
Compare
kingqn0321
force-pushed
the
v3_update_webrtc_transport_cc_feedback
branch
from
b810ab0
to
01ca264
Compare
|
Thanks @kingqn0321. Please let's review this on next days. We have busy days at work and many things to work on. |
|
It's maybe similar with this pull request. #1088 |
|
We are super busy at work. We don't forget about this, we'll check in next days hopefully. |
| static_assert(std::is_unsigned<T>::value, | ||
| "Type must be an unsigned integer."); |
There was a problem hiding this comment.
Logs in mediasoup (even in libwebrtc imported code) must adhere to the project syntax (must start with lowcase letter and must not include dot at the end). This change must be done in many files in this PR:
| static_assert(std::is_unsigned<T>::value, | |
| "Type must be an unsigned integer."); | |
| static_assert(std::is_unsigned<T>::value, "type must be an unsigned integer"); |
| void OnPacketArrival(uint16_t sequence_number, uint64_t arrival_time); | ||
| void SendPeriodicFeedbacks(); | ||
| int64_t BuildFeedbackPacket( | ||
| int64_t base_sequence_number, | ||
| std::map<int64_t, uint64_t>::const_iterator begin_iterator, // |begin_iterator| is inclusive. | ||
| std::map<int64_t, uint64_t>::const_iterator end_iterator // |end_iterator| is exclusive. | ||
| ); |
There was a problem hiding this comment.
In mediasoup we use camelCase for variables and function arguments.
sequence_number=>sequenceNumber- etc
| absl::optional<int64_t> periodicWindowStartSeq; | ||
| // Map unwrapped seq -> time. | ||
| std::map<int64_t, uint64_t> packetArrivalTimes; | ||
| // Use buffer policy similar to webrtc |
There was a problem hiding this comment.
Dot at the end of comments.
| // Use buffer policy similar to webrtc | |
| // Use buffer policy similar to webrtc. |
| void TransportCongestionControlServer::OnPacketArrival(uint16_t sequence_number, uint64_t arrival_time) | ||
| { |
There was a problem hiding this comment.
All methods and functions in mediasoup CPP files must start with MS_TRACE().
| this->packetArrivalTimes[seq] = arrival_time; | ||
| // Limit the range of sequence numbers to send feedback for. | ||
| auto first_arrival_time_to_keep = this->packetArrivalTimes.lower_bound( | ||
| this->packetArrivalTimes.rbegin()->first - kMaxNumberOfPackets); |
There was a problem hiding this comment.
| this->packetArrivalTimes[seq] = arrival_time; | |
| // Limit the range of sequence numbers to send feedback for. | |
| auto first_arrival_time_to_keep = this->packetArrivalTimes.lower_bound( | |
| this->packetArrivalTimes.rbegin()->first - kMaxNumberOfPackets); | |
| this->packetArrivalTimes[seq] = arrival_time; | |
| // Limit the range of sequence numbers to send feedback for. | |
| auto first_arrival_time_to_keep = this->packetArrivalTimes.lower_bound( | |
| this->packetArrivalTimes.rbegin()->first - kMaxNumberOfPackets); |
| std::map<int64_t, uint64_t>::const_iterator begin_iterator, | ||
| std::map<int64_t, uint64_t>::const_iterator end_iterator) | ||
| { | ||
| // Set base sequence numer and reference time(arrival time of first received packet in the feedback). |
There was a problem hiding this comment.
| // Set base sequence numer and reference time(arrival time of first received packet in the feedback). | |
| // Set base sequence number and reference time(arrival time of first received packet in | |
| // the feedback). |
| ref_timestamp_ms, | ||
| this->maxRtcpPacketLen); | ||
|
|
||
| // RTC_DCHECK(begin_iterator != end_iterator); |
| { | ||
| // If we can't even add the first seq to the feedback packet, we won't be | ||
| // able to build it at all. | ||
| // RTC_CHECK(begin_iterator != it); |
| "add fail! result:%" PRIu32 ", cur-seq:%" PRId64 ", next-seq:%" PRId64 | ||
| ", base-seq:%" PRId64 "", |
There was a problem hiding this comment.
We don't write this kind of logs anywhere.
| "add fail! result:%" PRIu32 ", cur-seq:%" PRId64 ", next-seq:%" PRId64 | |
| ", base-seq:%" PRId64 "", | |
| "failed [result:%" PRIu32 ", curSeq:%" PRId64 ", nextSeq:%" PRId64", baseSeq:%" PRId64 "]", |
| int64_t next_sequence_number = base_sequence_number; | ||
|
|
||
| for (auto it = begin_iterator; it != end_iterator; ++it) |
There was a problem hiding this comment.
camelCase. Here and everywhere (we just allow them within libwebrtc files).
|
Indeed this and #1088 add the same functionality, right? |
Details
1、improve the accuracy of gcc bwe when uplink network exists a big propagate jitter(eg: 100ms)
2、this optimization logic is ported from google webrtc(see: RemoteEstimatorProxy)
Rationale
1、mediasoup transport-cc feedback performance badly when uplink(webrtc-client--->mediasoup) network exists a big propagate jitter，will result in a very low GCC estimate for the webrtc-client，due to not considering disorder.
2、this optimization logic is ported from google webrtc(see: RemoteEstimatorProxy) which use a packetArrivalTimes(Map unwrapped seq -> time) buffer to send missing(arrived late due to disorder) recv delta feedback after a reordering.
3、Network simulation using Linux tc. eg: netem delay 50ms 50ms