From d23d04163d77c0a1779722263601cbbb651de296 Mon Sep 17 00:00:00 2001 From: thebongy Date: Thu, 16 Jan 2025 19:41:42 +0530 Subject: [PATCH] Fix to allow small negative jumps due to out of order packets in packet buffer This resolves an issue where when packets appear out of order at the beginning of a stream, packet_buffer.cc might drop the entire packet buffer because it detects a "large negative jump" even though the difference in sequence numbers is very minor and is caused by network congestion / packet re-ordering. Currently, when the issue occurs, this can cause video corruption/artifacts. More details and reproduction is available on the attached webrtc bug report 390329776. Bug: webrtc:390329776 Change-Id: Idb56eb2e066d596d8afd7ec904359baf0cb3feef Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374540 Reviewed-by: Sergey Silkin Reviewed-by: Danil Chapovalov Commit-Queue: Sergey Silkin Cr-Commit-Position: refs/heads/main@{#43753} --- AUTHORS | 1 + modules/video_coding/packet_buffer.cc | 3 ++- modules/video_coding/packet_buffer_unittest.cc | 13 +++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 46587ac6f3..b4e0e330fc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -114,6 +114,7 @@ Ralph Giles Raman Budny Ramprakash Jelari Riku Voipio +Rishit Bansal Robert Bares Robert Mader Robert Mader diff --git a/modules/video_coding/packet_buffer.cc b/modules/video_coding/packet_buffer.cc index df53e81685..718c4a86b9 100644 --- a/modules/video_coding/packet_buffer.cc +++ b/modules/video_coding/packet_buffer.cc @@ -82,7 +82,8 @@ PacketBuffer::InsertResult PacketBuffer::InsertPacket( return result; } - if (ForwardDiff(first_seq_num_, seq_num) >= max_size_) { + if (ForwardDiff(first_seq_num_, seq_num) >= max_size_ && + ForwardDiff(seq_num, first_seq_num_) >= max_size_ / 2) { // Large negative jump in rtp sequence number: clear the buffer and treat // latest packet as the new first packet. Clear(); diff --git a/modules/video_coding/packet_buffer_unittest.cc b/modules/video_coding/packet_buffer_unittest.cc index 6c05d09277..2f4d808d1b 100644 --- a/modules/video_coding/packet_buffer_unittest.cc +++ b/modules/video_coding/packet_buffer_unittest.cc @@ -356,6 +356,19 @@ TEST_F(PacketBufferTest, FramesReordered) { StartSeqNumsAre(seq_num + 2)); } +TEST_F(PacketBufferTest, FramesReorderedReconstruction) { + Insert(100, kKeyFrame, kFirst, kNotLast, {}, 2); + + Insert(98, kKeyFrame, kFirst, kNotLast, {}, 1); + EXPECT_THAT(Insert(99, kDeltaFrame, kNotFirst, kLast, {}, 1), + StartSeqNumsAre(98)); + + // Ideally frame with timestamp 2, seq No 100 should be + // reconstructed here from the first Insert() call in the test + EXPECT_THAT(Insert(101, kDeltaFrame, kNotFirst, kLast, {}, 2), + StartSeqNumsAre(100)); +} + TEST_F(PacketBufferTest, InsertPacketAfterSequenceNumberWrapAround) { int64_t kFirstSeqNum = 0; uint32_t kTimestampDelta = 100;