Skip to content

Commit

Permalink
Fix regression (crash) in Transport CC handling
Browse files Browse the repository at this point in the history
- 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?
  • Loading branch information
ibc committed Feb 20, 2024
1 parent b34a3b7 commit 0d1dfa7
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 13 deletions.
1 change: 1 addition & 0 deletions worker/include/RTC/TransportCongestionControlServer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ namespace RTC
// Whether any packet with transport wide sequence number was received.
bool transportWideSeqNumberReceived{ false };
uint16_t transportCcFeedbackWideSeqNumStart{ 0u };
// Map of arrival timestamp (ms) indexed by wide seq number.
std::map<uint16_t, uint64_t, RTC::SeqManager<uint16_t>::SeqLowerThan> mapPacketArrivalTimes;
};
} // namespace RTC
Expand Down
43 changes: 30 additions & 13 deletions worker/src/RTC/TransportCongestionControlServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ namespace RTC
// Create a new feedback packet.
this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(0u, 0u));

// Set current packet count.
this->transportCcFeedbackPacket->SetFeedbackPacketCount(this->transportCcFeedbackPacketCount);

break;
}

Expand Down Expand Up @@ -131,9 +134,10 @@ namespace RTC
break;
}

// We may receive packets with sequence number lower than the one in previous
// tcc feedback, these packets may have been reported as lost previously,
// therefore we need to reset the start sequence num for the next tcc feedback.
// We may receive packets with sequence number lower than the one in
// previous tcc feedback, these packets may have been reported as lost
// previously, therefore we need to reset the start sequence num for the
// next tcc feedback.
if (
!this->transportWideSeqNumberReceived ||
RTC::SeqManager<uint16_t>::IsSeqLowerThan(
Expand Down Expand Up @@ -192,13 +196,17 @@ namespace RTC
return;
}

auto baseTimestamp = it->second;

// Set base sequence num and reference time.
this->transportCcFeedbackPacket->SetBase(this->transportCcFeedbackWideSeqNumStart, it->second);
this->transportCcFeedbackPacket->SetBase(this->transportCcFeedbackWideSeqNumStart, baseTimestamp);

for (; it != this->mapPacketArrivalTimes.end(); ++it)
{
auto result =
this->transportCcFeedbackPacket->AddPacket(it->first, it->second, this->maxRtcpPacketLen);
auto sequenceNumber = it->first;
auto timestamp = it->second;
auto result = this->transportCcFeedbackPacket->AddPacket(
sequenceNumber, timestamp, this->maxRtcpPacketLen);

switch (result)
{
Expand All @@ -223,6 +231,14 @@ namespace RTC
// Create a new feedback packet.
this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(
this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc));

// Set current packet count.
// NOTE: Do not increment it since the previous ongoing feedback
// packet was not sent.
this->transportCcFeedbackPacket->SetFeedbackPacketCount(
this->transportCcFeedbackPacketCount);

break;
}

case RTC::RTCP::FeedbackRtpTransportPacket::AddPacketResult::FATAL:
Expand All @@ -231,7 +247,7 @@ namespace RTC
this->transportCcFeedbackPacket.reset(new RTC::RTCP::FeedbackRtpTransportPacket(
this->transportCcFeedbackSenderSsrc, this->transportCcFeedbackMediaSsrc));

// Use current packet count.
// Set current packet count.
// NOTE: Do not increment it since the previous ongoing feedback
// packet was not sent.
this->transportCcFeedbackPacket->SetFeedbackPacketCount(
Expand All @@ -242,6 +258,8 @@ namespace RTC
}
}

// It may happen that the packet is empty (no deltas) but in that case
// SendTransportCcFeedback() won't send it so we are safe.
SendTransportCcFeedback();
}

Expand All @@ -264,7 +282,7 @@ namespace RTC
}
}

inline void TransportCongestionControlServer::SendTransportCcFeedback()
void TransportCongestionControlServer::SendTransportCcFeedback()
{
MS_TRACE();

Expand Down Expand Up @@ -307,8 +325,7 @@ namespace RTC
this->transportCcFeedbackWideSeqNumStart = latestWideSeqNumber + 1;
}

inline void TransportCongestionControlServer::MayDropOldPacketArrivalTimes(
uint16_t seqNum, uint64_t nowMs)
void TransportCongestionControlServer::MayDropOldPacketArrivalTimes(uint16_t seqNum, uint64_t nowMs)
{
MS_TRACE();

Expand All @@ -330,7 +347,7 @@ namespace RTC
}
}

inline void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs)
void TransportCongestionControlServer::MaySendLimitationRembFeedback(uint64_t nowMs)
{
MS_TRACE();

Expand Down Expand Up @@ -402,7 +419,7 @@ namespace RTC
this->packetLoss = totalPacketLoss / samples;
}

inline void TransportCongestionControlServer::OnRembServerAvailableBitrate(
void TransportCongestionControlServer::OnRembServerAvailableBitrate(
const webrtc::RemoteBitrateEstimator* /*rembServer*/,
const std::vector<uint32_t>& ssrcs,
uint32_t availableBitrate)
Expand Down Expand Up @@ -440,7 +457,7 @@ namespace RTC
this->listener->OnTransportCongestionControlServerSendRtcpPacket(this, &packet);
}

inline void TransportCongestionControlServer::OnTimer(TimerHandle* timer)
void TransportCongestionControlServer::OnTimer(TimerHandle* timer)
{
MS_TRACE();

Expand Down

0 comments on commit 0d1dfa7

Please sign in to comment.