From 89d4b8836faf4ae81a0407bccc76bd937083a519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 25 Jul 2024 19:52:56 +0200 Subject: [PATCH 1/3] Fix frozen video in simulcast due to wrong dropping of padding only packets Fixes #1429 Thanks to @quanli168 for the report. ### Details - In `SimulcastConsumer` we must drop padding only packets ONLY if the packet belong to the current simulcast stream being sent to the consuming endpoint. - Changes in other `XxxxConsumer` classes are just cosmetic for consistency. --- CHANGELOG.md | 1 + worker/src/RTC/PipeConsumer.cpp | 18 +++++++++--------- worker/src/RTC/SimpleConsumer.cpp | 24 ++++++++++++------------ worker/src/RTC/SimulcastConsumer.cpp | 13 +++++++++++++ worker/src/RTC/SvcConsumer.cpp | 24 ++++++++++++------------ 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d333e2c610..9144b4cfd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Worker: Test, fix buffer overflow ([PR #1419](https://github.com/versatica/mediasoup/pull/1419)). - Bump up Meson from 1.3.0 to 1.5.0 ([PR #1424](https://github.com/versatica/mediasoup/pull/1424)). +- Fix frozen video in simulcast due to wrong dropping of payload only packets ([PR #1431](https://github.com/versatica/mediasoup/pull/1431), thanks to @quanli168). ### 3.14.8 diff --git a/worker/src/RTC/PipeConsumer.cpp b/worker/src/RTC/PipeConsumer.cpp index 35a0c3d1a9..cde4fbfc76 100644 --- a/worker/src/RTC/PipeConsumer.cpp +++ b/worker/src/RTC/PipeConsumer.cpp @@ -250,24 +250,24 @@ namespace RTC auto& syncRequired = this->mapRtpStreamSyncRequired.at(rtpStream); auto& rtpSeqManager = this->mapRtpStreamRtpSeqManager.at(rtpStream); - // Packets with only padding are not forwarded. - if (packet->GetPayloadLength() == 0) + // If we need to sync, support key frames and this is not a key frame, ignore + // the packet. + if (syncRequired && this->keyFrameSupported && !packet->IsKeyFrame()) { - rtpSeqManager.Drop(packet->GetSequenceNumber()); - #ifdef MS_RTC_LOGGER_RTP - packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); + packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::NOT_A_KEYFRAME); #endif return; } - // If we need to sync, support key frames and this is not a key frame, ignore - // the packet. - if (syncRequired && this->keyFrameSupported && !packet->IsKeyFrame()) + // Packets with only padding are not forwarded. + if (packet->GetPayloadLength() == 0) { + rtpSeqManager.Drop(packet->GetSequenceNumber()); + #ifdef MS_RTC_LOGGER_RTP - packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::NOT_A_KEYFRAME); + packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); #endif return; diff --git a/worker/src/RTC/SimpleConsumer.cpp b/worker/src/RTC/SimpleConsumer.cpp index ce7fa9283c..d2b635babe 100644 --- a/worker/src/RTC/SimpleConsumer.cpp +++ b/worker/src/RTC/SimpleConsumer.cpp @@ -314,18 +314,6 @@ namespace RTC return; } - // Packets with only padding are not forwarded. - if (packet->GetPayloadLength() == 0) - { - this->rtpSeqManager.Drop(packet->GetSequenceNumber()); - -#ifdef MS_RTC_LOGGER_RTP - packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); -#endif - - return; - } - auto payloadType = packet->GetPayloadType(); // NOTE: This may happen if this Consumer supports just some codecs of those @@ -372,6 +360,18 @@ namespace RTC return; } + // Packets with only padding are not forwarded. + if (packet->GetPayloadLength() == 0) + { + this->rtpSeqManager.Drop(packet->GetSequenceNumber()); + +#ifdef MS_RTC_LOGGER_RTP + packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); +#endif + + return; + } + // Whether this is the first packet after re-sync. const bool isSyncPacket = this->syncRequired; diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp index 847d4beb64..926a2566c2 100644 --- a/worker/src/RTC/SimulcastConsumer.cpp +++ b/worker/src/RTC/SimulcastConsumer.cpp @@ -810,6 +810,19 @@ namespace RTC return; } + // If the packet belongs to current spatial layer being sent and packet does + // not have payload other than padding, then drop it. + if (spatialLayer == this->currentSpatialLayer && packet->GetPayloadLength() == 0) + { + this->rtpSeqManager.Drop(packet->GetSequenceNumber()); + +#ifdef MS_RTC_LOGGER_RTP + packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); +#endif + + return; + } + // Whether this is the first packet after re-sync. const bool isSyncPacket = this->syncRequired; diff --git a/worker/src/RTC/SvcConsumer.cpp b/worker/src/RTC/SvcConsumer.cpp index bc0ce1417b..a2337b4f81 100644 --- a/worker/src/RTC/SvcConsumer.cpp +++ b/worker/src/RTC/SvcConsumer.cpp @@ -639,18 +639,6 @@ namespace RTC return; } - // Packets with only padding are not forwarded. - if (packet->GetPayloadLength() == 0) - { - this->rtpSeqManager.Drop(packet->GetSequenceNumber()); - -#ifdef MS_RTC_LOGGER_RTP - packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); -#endif - - return; - } - // clang-format off if ( this->encodingContext->GetTargetSpatialLayer() == -1 || @@ -690,6 +678,18 @@ namespace RTC return; } + // Packets with only padding are not forwarded. + if (packet->GetPayloadLength() == 0) + { + this->rtpSeqManager.Drop(packet->GetSequenceNumber()); + +#ifdef MS_RTC_LOGGER_RTP + packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); +#endif + + return; + } + // Whether this is the first packet after re-sync. const bool isSyncPacket = this->syncRequired; From 0e08dfa1d147083dd0fb0da947c172fa511fcff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 25 Jul 2024 19:53:37 +0200 Subject: [PATCH 2/3] Update CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9144b4cfd5..6e5073d3d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ - Worker: Test, fix buffer overflow ([PR #1419](https://github.com/versatica/mediasoup/pull/1419)). - Bump up Meson from 1.3.0 to 1.5.0 ([PR #1424](https://github.com/versatica/mediasoup/pull/1424)). -- Fix frozen video in simulcast due to wrong dropping of payload only packets ([PR #1431](https://github.com/versatica/mediasoup/pull/1431), thanks to @quanli168). +- Fix frozen video in simulcast due to wrong dropping of padding only packets ([PR #1431](https://github.com/versatica/mediasoup/pull/1431), thanks to @quanli168). ### 3.14.8 From 190467690131b632064f9eb212e121e271fb0274 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Fri, 26 Jul 2024 10:48:17 +0200 Subject: [PATCH 3/3] fix --- worker/src/RTC/SimulcastConsumer.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp index 926a2566c2..bc54bf3424 100644 --- a/worker/src/RTC/SimulcastConsumer.cpp +++ b/worker/src/RTC/SimulcastConsumer.cpp @@ -730,18 +730,6 @@ namespace RTC return; } - // Packets with only padding are not forwarded. - if (packet->GetPayloadLength() == 0) - { - this->rtpSeqManager.Drop(packet->GetSequenceNumber()); - -#ifdef MS_RTC_LOGGER_RTP - packet->logger.Dropped(RtcLogger::RtpPacket::DropReason::EMPTY_PAYLOAD); -#endif - - return; - } - if (this->targetTemporalLayer == -1) { #ifdef MS_RTC_LOGGER_RTP