versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.18k stars 1.12k forks source link

Fix regression (crash) in transport-cc feedback generation #1339

Closed ibc closed 7 months ago

ibc commented 7 months ago

Details

ibc commented 7 months ago

This is a draft PR because I need help. See TODO above.

ibc commented 7 months ago

I think I know how to solve it properly. Just a question:

Does somebody remember what is feedbackPacketCount in FeedbackRtpTransportPacket? It's obviously part of the packet fields but I don't remember its meaning. Specifically if an endpoint sends 2 sequential CC feedback packets, what should be the value of that field in those packets?

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);
ibc commented 7 months ago

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?

ibc commented 7 months ago

Concerns resolved in theory. PR ready for review.

ibc commented 7 months ago

I've done this change to test:

uint16_t FeedbackRtpTransportPacket::maxMissingPackets{ (1 << 13) - 1 };
// uint16_t FeedbackRtpTransportPacket::maxPacketStatusCount{ (1 << 16) - 1 };

and it crashes:

(ABORT) RTC::RTCP::FeedbackRtpTransport::AddPacket() | failed assertion `!IsFull()': packet is full

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib              0x7ff80d79b1e2 __pthread_kill + 10
1   libsystem_pthread.dylib             0x7ff80d7d2ee6 pthread_kill + 263
2   libsystem_c.dylib                   0x7ff80d6f9b45 abort + 123
3   mediasoup-worker                       0x1095e6ea0 RTC::RTCP::FeedbackRtpTransportPacket::AddPacket(unsigned short, unsigned long long, unsigned long) (.cold.2) + 64
4   mediasoup-worker                       0x1092b4c47 RTC::RTCP::FeedbackRtpTransportPacket::AddPacket(unsigned short, unsigned long long, unsigned long) + 231
5   mediasoup-worker                       0x109274e22 RTC::TransportCongestionControlServer::FillAndSendTransportCcFeedback() + 210

Why? Because when this->transportCcFeedbackPacket->IsFull() we call SendTransportCcFeedback() which is supposed to send the feedback and reset this->transportCcFeedbackPacket so in the next iteration of the loop there is a fresh feedback packet which obviously is not full, however there are cases in which SendTransportCcFeedback() does NOT send the feedback and does NOT reset it:

if (!this->transportCcFeedbackPacket->IsSerializable())
{
    return;
}

So here another regression in that PR or perhaps

ibc commented 7 months ago

Fixing it as follows:

CleanShot-2024-02-20-at-17 37 57@2x