From b3c5bdb85a56e053ef6401b6e51ad8ba335f8372 Mon Sep 17 00:00:00 2001 From: Zen Xu Date: Tue, 21 Feb 2023 13:55:08 -0800 Subject: [PATCH] Allow video frame gaps in packet buffer for H.264 With LTR and SVC etc., H.264 should be able to skip lost frames, and continue to play from the new frames. With DependencyDescriptor, it is allowed to reference the previous frames, even there is a gap in the middle. However, we found there is a special logic for H.264 in packet_buffer.cc, which requires no gap for H.264. We should allow gaps if the packet has GenericDescriptorInfo (either GenericDescriptor or DependencyDescriptor header extension). Bug: webrtc:14887 Change-Id: Id66726bab33229bd883f257136ff2e8523fb44c0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/294062 Commit-Queue: Philip Eliasson Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/main@{#39370} --- modules/video_coding/packet_buffer.cc | 15 ++++++---- .../video_coding/packet_buffer_unittest.cc | 30 +++++++++++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/modules/video_coding/packet_buffer.cc b/modules/video_coding/packet_buffer.cc index 3dcfc48213..9a599cd4bd 100644 --- a/modules/video_coding/packet_buffer.cc +++ b/modules/video_coding/packet_buffer.cc @@ -256,7 +256,9 @@ std::vector> PacketBuffer::FindFrames( int64_t frame_timestamp = buffer_[start_index]->timestamp; // Identify H.264 keyframes by means of SPS, PPS, and IDR. - bool is_h264 = buffer_[start_index]->codec() == kVideoCodecH264; + bool is_generic = buffer_[start_index]->video_header.generic.has_value(); + bool is_h264_descriptor = + (buffer_[start_index]->codec() == kVideoCodecH264) && !is_generic; bool has_h264_sps = false; bool has_h264_pps = false; bool has_h264_idr = false; @@ -267,7 +269,7 @@ std::vector> PacketBuffer::FindFrames( while (true) { ++tested_packets; - if (!is_h264) { + if (!is_h264_descriptor) { if (buffer_[start_index] == nullptr || buffer_[start_index]->is_first_packet_in_frame()) { full_frame_found = buffer_[start_index] != nullptr; @@ -275,7 +277,7 @@ std::vector> PacketBuffer::FindFrames( } } - if (is_h264) { + if (is_h264_descriptor) { const auto* h264_header = absl::get_if( &buffer_[start_index]->video_header.video_type_header); if (!h264_header || h264_header->nalus_length >= kMaxNalusPerPacket) @@ -317,7 +319,8 @@ std::vector> PacketBuffer::FindFrames( // the timestamp of that packet is the same as this one. This may cause // the PacketBuffer to hand out incomplete frames. // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=7106 - if (is_h264 && (buffer_[start_index] == nullptr || + if (is_h264_descriptor && + (buffer_[start_index] == nullptr || buffer_[start_index]->timestamp != frame_timestamp)) { break; } @@ -325,7 +328,7 @@ std::vector> PacketBuffer::FindFrames( --start_seq_num; } - if (is_h264) { + if (is_h264_descriptor) { // Warn if this is an unsafe frame. if (has_h264_idr && (!has_h264_sps || !has_h264_pps)) { RTC_LOG(LS_WARNING) @@ -363,7 +366,7 @@ std::vector> PacketBuffer::FindFrames( } } - if (is_h264 || full_frame_found) { + if (is_h264_descriptor || full_frame_found) { const uint16_t end_seq_num = seq_num + 1; // Use uint16_t type to handle sequence number wrap around case. uint16_t num_packets = end_seq_num - start_seq_num; diff --git a/modules/video_coding/packet_buffer_unittest.cc b/modules/video_coding/packet_buffer_unittest.cc index 6969b023db..901f44b188 100644 --- a/modules/video_coding/packet_buffer_unittest.cc +++ b/modules/video_coding/packet_buffer_unittest.cc @@ -405,8 +405,9 @@ class PacketBufferH264Test : public PacketBufferTest { IsLast last, // is last packet of frame uint32_t timestamp, // rtp timestamp rtc::ArrayView data = {}, - uint32_t width = 0, // width of frame (SPS/IDR) - uint32_t height = 0) { // height of frame (SPS/IDR) + uint32_t width = 0, // width of frame (SPS/IDR) + uint32_t height = 0, // height of frame (SPS/IDR) + bool generic = false) { // has generic descriptor auto packet = std::make_unique(); packet->video_header.codec = kVideoCodecH264; auto& h264_header = @@ -428,6 +429,9 @@ class PacketBufferH264Test : public PacketBufferTest { packet->video_header.height = height; packet->video_header.is_first_packet_in_frame = first == kFirst; packet->video_header.is_last_packet_in_frame = last == kLast; + if (generic) { + packet->video_header.generic.emplace(); + } packet->video_payload.SetData(data.data(), data.size()); return PacketBufferInsertResult( @@ -823,6 +827,28 @@ TEST_F(PacketBufferH264SpsPpsIdrIsKeyframeTest, SpsPpsIdrIsKeyframe) { ElementsAre(KeyFrame())); } +class PacketBufferH264FrameGap : public PacketBufferH264Test { + protected: + PacketBufferH264FrameGap() : PacketBufferH264Test(true) {} +}; + +TEST_F(PacketBufferH264FrameGap, AllowFrameGapForH264WithGeneric) { + auto generic = true; + InsertH264(1, kKeyFrame, kFirst, kLast, 1001, {}, 0, 0, generic); + EXPECT_THAT(InsertH264(3, kDeltaFrame, kFirst, kLast, 1003, {}, 0, 0, generic) + .packets, + SizeIs(1)); +} + +TEST_F(PacketBufferH264FrameGap, DisallowFrameGapForH264NoGeneric) { + auto generic = false; + InsertH264(1, kKeyFrame, kFirst, kLast, 1001, {}, 0, 0, generic); + + EXPECT_THAT(InsertH264(3, kDeltaFrame, kFirst, kLast, 1003, {}, 0, 0, generic) + .packets, + IsEmpty()); +} + } // namespace } // namespace video_coding } // namespace webrtc