From eafee5e3d68a7356dd415d2b3eee53017140d38b Mon Sep 17 00:00:00 2001 From: k-wasniowski Date: Wed, 22 Jan 2025 18:45:34 +0100 Subject: [PATCH] fix: h26x packet buffer video artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change resolves an issue that arises when there is a gap in the sequence numbers of packets associated with a single frame. Before this change, the H26x packet buffer could potentially assemble a frame using only a subset of the packets in the buffer if a packet was missing in the middle and a packet with a marker bit arrived. To address this, the change introduces a check before assembling a frame. This ensures that all packets belonging to a single frame are correctly collected by iterating backward until the first packet in the frame is identified. Bug: webrtc:384391181 Change-Id: I4d09a3d6d569624ece204264cb32e5076ed090a2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374183 Reviewed-by: Henrik Boström Reviewed-by: Sergey Silkin Reviewed-by: Jianlin Qiu Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43793} --- AUTHORS | 1 + modules/video_coding/h26x_packet_buffer.cc | 6 +++ modules/video_coding/h26x_packet_buffer.h | 2 +- .../h26x_packet_buffer_unittest.cc | 37 ++++++++++++++++++- video/rtp_video_stream_receiver2_unittest.cc | 2 + 5 files changed, 46 insertions(+), 2 deletions(-) diff --git a/AUTHORS b/AUTHORS index 4c7e6900a4..0adf774e2e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -74,6 +74,7 @@ Jie Mao Jiwon Kim Johnny Wong Jose Antonio Olivera Ortega +Kacper Waśniowski Karim Hammache Keiichi Enomoto Kiran Thind diff --git a/modules/video_coding/h26x_packet_buffer.cc b/modules/video_coding/h26x_packet_buffer.cc index 2afe1426b1..8bc5d7e3bf 100644 --- a/modules/video_coding/h26x_packet_buffer.cc +++ b/modules/video_coding/h26x_packet_buffer.cc @@ -207,6 +207,12 @@ H26xPacketBuffer::InsertResult H26xPacketBuffer::FindFrames( auto& prev_packet = GetPacket(seq_num_start - 1); if (prev_packet == nullptr || prev_packet->timestamp != rtp_timestamp) { + const auto& current_packet = GetPacket(seq_num_start); + if (!current_packet->video_header.is_first_packet_in_frame) { + // First packet of the frame is missing. + return result; + } + if (MaybeAssembleFrame(seq_num_start, seq_num, result)) { // Frame was assembled, continue to look for more frames. break; diff --git a/modules/video_coding/h26x_packet_buffer.h b/modules/video_coding/h26x_packet_buffer.h index c3db72bdd6..d7f5286a2f 100644 --- a/modules/video_coding/h26x_packet_buffer.h +++ b/modules/video_coding/h26x_packet_buffer.h @@ -85,7 +85,7 @@ class H26xPacketBuffer { // received without SPS/PPS. void InsertSpsPpsNalus(const std::vector& sps, const std::vector& pps); - // Insert start code and paramter sets for H.264 payload, also update header + // Insert start code and parameter sets for H.264 payload, also update header // if parameter sets are inserted. Return false if required SPS or PPS is not // found. bool FixH264Packet(Packet& packet); diff --git a/modules/video_coding/h26x_packet_buffer_unittest.cc b/modules/video_coding/h26x_packet_buffer_unittest.cc index a3ea3dc1c5..dc5f3b5502 100644 --- a/modules/video_coding/h26x_packet_buffer_unittest.cc +++ b/modules/video_coding/h26x_packet_buffer_unittest.cc @@ -547,6 +547,40 @@ TEST(H26xPacketBufferTest, IdrIsKeyframeFuaRequiresFirstFragmet) { SizeIs(2)); } +TEST(H26xPacketBufferTest, ReorderedRtpPackets) { + H26xPacketBuffer packet_buffer(/*h264_allow_idr_only_keyframes=*/true); + + RTC_UNUSED(packet_buffer.InsertPacket(H264Packet(kH264StapA) + .Sps() + .Pps() + .SeqNum(1) + .Time(0) + .AsFirstPacket() + .AsFirstFragment() + .Build())); + + RTC_UNUSED(packet_buffer.InsertPacket( + H264Packet(kH264FuA).Idr().SeqNum(2).Time(0).Build())); + + RTC_UNUSED(packet_buffer.InsertPacket( + H264Packet(kH264FuA).Idr().SeqNum(3).Time(0).Build())); + + RTC_UNUSED(packet_buffer.InsertPacket( + H264Packet(kH264FuA).Idr().SeqNum(5).Time(0).Build())); + + RTC_UNUSED(packet_buffer.InsertPacket( + H264Packet(kH264FuA).Idr().AsFirstFragment().SeqNum(6).Time(0).Build())); + + RTC_UNUSED(packet_buffer.InsertPacket( + H264Packet(kH264FuA).Idr().SeqNum(7).Time(0).Marker().Build())); + + EXPECT_THAT( + packet_buffer + .InsertPacket(H264Packet(kH264FuA).Idr().SeqNum(4).Time(0).Build()) + .packets, + SizeIs(7)); +} + TEST(H26xPacketBufferTest, SpsPpsIdrIsKeyframeSingleNalus) { H26xPacketBuffer packet_buffer(/*h264_allow_idr_only_keyframes=*/false); @@ -933,6 +967,7 @@ TEST(H26xPacketBufferTest, FrameBoundariesAreSet) { .Idr() .SeqNum(1) .Time(1) + .AsFirstPacket() .Marker() .Build()); @@ -941,7 +976,7 @@ TEST(H26xPacketBufferTest, FrameBoundariesAreSet) { EXPECT_TRUE(key.packets[0]->video_header.is_last_packet_in_frame); RTC_UNUSED(packet_buffer.InsertPacket( - H264Packet(kH264FuA).Slice().SeqNum(2).Time(2).Build())); + H264Packet(kH264FuA).Slice().SeqNum(2).Time(2).AsFirstPacket().Build())); RTC_UNUSED(packet_buffer.InsertPacket( H264Packet(kH264FuA).Slice().SeqNum(3).Time(2).Build())); auto delta = packet_buffer.InsertPacket( diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc index 07c23a4f33..357fd9b895 100644 --- a/video/rtp_video_stream_receiver2_unittest.cc +++ b/video/rtp_video_stream_receiver2_unittest.cc @@ -1726,11 +1726,13 @@ TEST_F(RtpVideoStreamReceiver2TestH265, H265Bitstream) { rtp_packet.SetSequenceNumber(0); rtp_packet.SetPayloadType(kH265PayloadType); RTPVideoHeader video_header = GetDefaultH265VideoHeader(); + video_header.is_first_packet_in_frame = true; mock_on_complete_frame_callback_.AppendExpectedBitstream(vps, sizeof(vps)); rtp_video_stream_receiver_->OnReceivedPayloadData( rtc::CopyOnWriteBuffer(vps, sizeof(vps)), rtp_packet, video_header, 0); rtp_packet.SetSequenceNumber(1); + video_header.is_first_packet_in_frame = false; mock_on_complete_frame_callback_.AppendExpectedBitstream(sps, sizeof(sps)); rtp_video_stream_receiver_->OnReceivedPayloadData( rtc::CopyOnWriteBuffer(sps, sizeof(sps)), rtp_packet, video_header, 0);