Don't truncate frame diffs in RtpGenericFrameDescriptor RtpGenericFrameDescriptor::AddFrameDependencyDiff() took a uint16_t, so frame diffs computed from 64-bit frame ids were silently truncated. A diff that truncated into the valid range, e.g. 2^16 + 5, was accepted and written as a dependency on the wrong frame. Take the diff as int64_t and reject values that the descriptor can't represent. RTPSenderVideo ignores the result, so it now leaves such a dependency out rather than misrepresenting it. Leaving out the whole descriptor instead would make the stream inconsistent, while the missing dependency only matters to receivers that lost the referenced frame. RtpDescriptorAuthentication calls the same method, so the authenticated data still matches the descriptor that is sent. Bug: webrtc:566342228 Change-Id: I344a641a1b0956984ed69fc44d439d8adba021df Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/505901 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#48744}
diff --git a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.cc b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.cc index 44124f3..013d159 100644 --- a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.cc +++ b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.cc
@@ -81,15 +81,13 @@ return std::span(frame_deps_id_diffs_, num_frame_deps_); } -bool RtpGenericFrameDescriptor::AddFrameDependencyDiff(uint16_t fdiff) { +bool RtpGenericFrameDescriptor::AddFrameDependencyDiff(int64_t fdiff) { RTC_DCHECK(FirstPacketInSubFrame()); if (num_frame_deps_ == kMaxNumFrameDependencies) return false; - if (fdiff == 0) + if (fdiff <= 0 || fdiff > kMaxFrameDependencyDiff) return false; - RTC_DCHECK_LT(fdiff, 1 << 14); - RTC_DCHECK_GT(fdiff, 0); - frame_deps_id_diffs_[num_frame_deps_] = fdiff; + frame_deps_id_diffs_[num_frame_deps_] = static_cast<uint16_t>(fdiff); num_frame_deps_++; return true; }
diff --git a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h index 4075bfe..0c8a847 100644 --- a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h +++ b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h
@@ -25,6 +25,8 @@ static constexpr int kMaxNumFrameDependencies = 8; static constexpr int kMaxTemporalLayers = 8; static constexpr int kMaxSpatialLayers = 8; + // Largest frame dependency diff that can be represented on the wire. + static constexpr int kMaxFrameDependencyDiff = (1 << 14) - 1; RtpGenericFrameDescriptor(); RtpGenericFrameDescriptor(const RtpGenericFrameDescriptor&); @@ -55,8 +57,9 @@ std::span<const uint16_t> FrameDependenciesDiffs() const; void ClearFrameDependencies() { num_frame_deps_ = 0; } - // Returns false on failure, i.e. number of dependencies is too large. - bool AddFrameDependencyDiff(uint16_t fdiff); + // Returns false on failure, i.e. number of dependencies is too large or + // `fdiff` is outside of the range [1, kMaxFrameDependencyDiff]. + bool AddFrameDependencyDiff(int64_t fdiff); private: bool beginning_of_subframe_ = false;
diff --git a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension_unittest.cc b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension_unittest.cc index 4e3ac9b..325135f 100644 --- a/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension_unittest.cc
@@ -263,5 +263,22 @@ EXPECT_TRUE(RtpGenericFrameDescriptorExtension00::Write(buffer, descriptor)); EXPECT_THAT(buffer, ElementsAreArray(kRaw)); } + +TEST(RtpGenericFrameDescriptorExtensionTest, + AddFrameDependencyDiffRejectsUnrepresentableDiffs) { + constexpr int kMaxDiff = RtpGenericFrameDescriptor::kMaxFrameDependencyDiff; + RtpGenericFrameDescriptor descriptor; + descriptor.SetFirstPacketInSubFrame(true); + + EXPECT_FALSE(descriptor.AddFrameDependencyDiff(0)); + EXPECT_FALSE(descriptor.AddFrameDependencyDiff(-1)); + EXPECT_FALSE(descriptor.AddFrameDependencyDiff(kMaxDiff + 1)); + // Would be 1 if truncated to 16 bits. + EXPECT_FALSE(descriptor.AddFrameDependencyDiff((int64_t{1} << 16) + 1)); + EXPECT_TRUE(descriptor.AddFrameDependencyDiff(1)); + EXPECT_TRUE(descriptor.AddFrameDependencyDiff(kMaxDiff)); + + EXPECT_THAT(descriptor.FrameDependenciesDiffs(), ElementsAre(1, kMaxDiff)); +} } // namespace } // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 2db50be..42ef5eb 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -475,6 +475,9 @@ generic_descriptor.SetFrameId( static_cast<uint16_t>(video_header.generic->frame_id)); for (int64_t dep : video_header.generic->dependencies) { + // A dependency that the descriptor can't represent is left out. That + // only matters to receivers that lost the referenced frame, whereas + // leaving out the descriptor would make the stream inconsistent. generic_descriptor.AddFrameDependencyDiff( video_header.generic->frame_id - dep); }
diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index ed2aafc..e5794b9 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc
@@ -1001,6 +1001,33 @@ EXPECT_EQ(descriptor_wire.SpatialLayersBitmask(), 0b0000'0100); } +// A dependency that the generic frame descriptor can't represent is left out +// rather than sent with a wrong frame diff. +TEST_F(RtpSenderVideoTest, + LeavesOutDependencyThatGenericFrameDescriptorCannotRepresent) { + const int64_t kFrameId = 100000; + uint8_t kFrame[100]; + rtp_module_->RegisterRtpHeaderExtension( + RtpGenericFrameDescriptorExtension00::Uri(), kGenericDescriptorId); + + // Send delta frame that references the previous frame and a frame with a + // frame diff that is 5 when truncated to 16 bits. + RTPVideoHeader hdr; + RTPVideoHeader::GenericDescriptorInfo& generic = hdr.generic.emplace(); + generic.frame_id = kFrameId + (1 << 16) + 5; + generic.dependencies = {kFrameId + (1 << 16) + 4, kFrameId}; + hdr.frame_type = VideoFrameType::kVideoFrameDelta; + EXPECT_TRUE(rtp_sender_video_->SendVideoFrame( + kPayloadType, kType, kTimestampInfo, fake_clock_.CurrentTime(), kFrame, + sizeof(kFrame), hdr, kDefaultExpectedRetransmissionTime, {})); + ASSERT_EQ(transport_.packets_sent(), 1); + RtpGenericFrameDescriptor descriptor_wire; + ASSERT_TRUE(transport_.last_sent_packet() + .GetExtension<RtpGenericFrameDescriptorExtension00>( + &descriptor_wire)); + EXPECT_THAT(descriptor_wire.FrameDependenciesDiffs(), ElementsAre(1)); +} + void RtpSenderVideoTest:: UsesMinimalVp8DescriptorWhenGenericFrameDescriptorExtensionIsUsed( int /* version */) {