Fix regression (crash) in transport-cc feedback generation #1339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fixes #1336 ### Details - Regression introduced in PR #1088, which has many issues. - Basically there are cases in which `this->transportCcFeedbackPacket` is reset so it's a complete new packet and hence its `baseSet` is `false`. - However in `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` in which we just call `this->transportCcFeedbackPacket.SetBase()` once at the top, but then we enter a loop in which the packet maybe full so it's sent (or other things) and hence we **reset** the packet while in the loop. And then at the top of the loop `this->transportCcFeedbackPacket.AddPacket()` is called so the assertion fails because its `SetBase()` was not called on that fresh packet. - Also add a missing `break` in a `case` block in that loop. - Also set proper packet count in all cases in which we reset `this->transportCcFeedbackPacket`. ### TODO This is not done yet. The issue is mainly identified but I don't know yet how to properly fix it without doing other wrong things. Basically I'm not sure if the whole logic is ok after PR #1088: - As said before we only set the base of the packet once at the beginning of the loop, and such a `SetBase()` is called with `this->transportCcFeedbackWideSeqNumStart` and a computed `timestamp`. - However, within the loop below, the packet may become full and hence `SendTransportCcFeedback()` is called, which updates `++this->transportCcFeedbackPacketCount` and `this->transportCcFeedbackWideSeqNumStart`, and **also** resets the packet (so missing base again). - So in order to fix the crash, we must call `SetBase()` again, but which which values? Because the values given initially at the top of `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` were the value of `this->transportCcFeedbackWideSeqNumStart` at that time and a timestamp. But that `this->transportCcFeedbackWideSeqNumStart` has now changed, so then what? - Basically every time `this->transportCcFeedbackPacket.reset()` is called (and there are various cases in the whole `TransportCongestionControlServer.cpp` file) **we must** call ``SetBase()` on the fresh packet **with** proper values. And which are those proper values if we are within that loop?
|
This is a draft PR because I need help. See TODO above. |
|
I think I know how to solve it properly. Just a question: Does somebody remember what is And why are we setting it when initiating transport server control? /* Instance methods. */
TransportCongestionControlServer::TransportCongestionControlServer(
RTC::TransportCongestionControlServer::Listener* listener,
RTC::BweType bweType,
size_t maxRtcpPacketLen)
: listener(listener), bweType(bweType), maxRtcpPacketLen(maxRtcpPacketLen)
{
MS_TRACE();
switch (this->bweType)
{
case RTC::BweType::TRANSPORT_CC:
{
// Create a feedback packet.
this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u));
// Set initial packet count.
this->transportCcFeedbackPacket->SetFeedbackPacketCount(this->transportCcFeedbackPacketCount); |
|
Ah, is the number of CC feedback packet sent, right? so it's just a counter that must be incremented every time a feedback packet is sent. Right? |
|
Concerns resolved in theory. PR ready for review. |
ibc
commented
Feb 20, 2024
ibc
commented
Feb 20, 2024
jmillan
reviewed
Feb 20, 2024
ibc
changed the title
|
I've done this change to test: uint16_t FeedbackRtpTransportPacket::maxMissingPackets{ (1 << 13) - 1 };
// uint16_t FeedbackRtpTransportPacket::maxPacketStatusCount{ (1 << 16) - 1 };and it crashes: Why? Because when if (!this->transportCcFeedbackPacket->IsSerializable())
{
return;
}So here another regression in that PR or perhaps |
|
Fixing it as follows: |
jmillan
approved these changes
Feb 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
this->transportCcFeedbackPacketis reset so it's a complete new packet and hence itsbaseSetisfalse.TransportCongestionControlServer::FillAndSendTransportCcFeedback()in which we just callthis->transportCcFeedbackPacket.SetBase()once at the top, but then we enter a loop in which the packet maybe full so it's sent (or other things) and hence we reset the packet while in the loop. And then at the top of the loopthis->transportCcFeedbackPacket.AddPacket()is called so the assertion fails because itsSetBase()was not called on that fresh packet.SetBase()conditionally in every iteration of the loop by checking if current feedback has the base set or not.breakin acaseblock in that loop.this->transportCcFeedbackPacket.