Reland "Validate frame consistency when writing DependencyDescriptor"
This reverts commit 81aa059b85949001dabbedaaf99574dc6390882f.
Reason for revert: downstream tests fixed
Original change's description:
> Revert "Validate frame consistency when writing DependencyDescriptor"
>
> This reverts commit 200fd82771ae29d23b2be40194be674b3437f0ab.
>
> Reason for revert: breaks downstream
>
> Original change's description:
> > Validate frame consistency when writing DependencyDescriptor
> >
> > To write DependencyDescriptor frame properties should be consistent with
> > the FrameDependencyStructure.
> > Historically that was ensured by webrtc codec wrappers, but with with frame transform api interface there are now more ways to inject video frame for packetizing.
> > Thus DependencyDescriptorWriter should be more protective to avoid crashes.
> >
> > Bug: chromium:379282549
> > Change-Id: I98f226ff09c32154e18888c8e811e7981567ad45
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371301
> > Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
> > Reviewed-by: Åsa Persson <asapersson@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#43551}
>
> Bug: chromium:379282549
> Change-Id: I7711756f774648cbb85c51b61424bb950c1d3775
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371420
> Commit-Queue: Jeremy Leconte <jleconte@webrtc.org>
> Owners-Override: Jeremy Leconte <jleconte@webrtc.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#43556}
Bug: chromium:379282549
Change-Id: I71ef363d710b7f28b298d11543e1c8ad6c884f15
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371304
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Jeremy Leconte <jleconte@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43563}
diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc
index acc9e3c..ffe62eb 100644
--- a/call/rtp_payload_params.cc
+++ b/call/rtp_payload_params.cc
@@ -483,6 +483,10 @@
temporal_index, DecodeTargetIndication::kNotPresent);
std::fill(it, generic.decode_target_indications.end(),
DecodeTargetIndication::kSwitch);
+ generic.chain_diffs = {
+ (is_keyframe || last_frame_id_[0][0] < 0)
+ ? 0
+ : static_cast<int>(frame_id - last_frame_id_[0][0])};
if (is_keyframe) {
RTC_DCHECK_EQ(temporal_index, 0);
diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn
index 3bbddc7..1f4a0ca 100644
--- a/modules/rtp_rtcp/BUILD.gn
+++ b/modules/rtp_rtcp/BUILD.gn
@@ -136,6 +136,7 @@
"../../rtc_base:event_tracer",
"../../rtc_base:logging",
"../../rtc_base:macromagic",
+ "../../rtc_base:safe_compare",
"../../rtc_base:safe_conversions",
"../../rtc_base:stringutils",
"../../rtc_base/network:ecn_marking",
diff --git a/modules/rtp_rtcp/source/rtp_dependency_descriptor_extension_unittest.cc b/modules/rtp_rtcp/source/rtp_dependency_descriptor_extension_unittest.cc
index 148e4f9..e666d4a 100644
--- a/modules/rtp_rtcp/source/rtp_dependency_descriptor_extension_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_dependency_descriptor_extension_unittest.cc
@@ -132,5 +132,48 @@
descriptor));
}
+TEST(RtpDependencyDescriptorExtensionTest,
+ FailsToWriteWhenNumberOfChainsMismatch) {
+ uint8_t buffer[256];
+ FrameDependencyStructure structure;
+ structure.num_decode_targets = 2;
+ structure.num_chains = 2;
+ structure.templates = {
+ FrameDependencyTemplate().T(0).Dtis("SR").ChainDiffs({2, 2})};
+ DependencyDescriptor descriptor;
+ descriptor.frame_dependencies = structure.templates[0];
+
+ // Structure has 2 chains, but frame provide 1 chain diff,
+ descriptor.frame_dependencies.chain_diffs = {2};
+
+ EXPECT_EQ(
+ RtpDependencyDescriptorExtension::ValueSize(structure, 0b11, descriptor),
+ 0u);
+ EXPECT_FALSE(RtpDependencyDescriptorExtension::Write(buffer, structure, 0b11,
+ descriptor));
+}
+
+TEST(RtpDependencyDescriptorExtensionTest,
+ FailsToWriteWhenNumberOfDecodeTargetsMismatch) {
+ uint8_t buffer[256];
+ FrameDependencyStructure structure;
+ structure.num_decode_targets = 2;
+ structure.num_chains = 2;
+ structure.templates = {
+ FrameDependencyTemplate().T(0).Dtis("SR").ChainDiffs({2, 2})};
+ DependencyDescriptor descriptor;
+ descriptor.frame_dependencies = structure.templates[0];
+
+ // Structure has 2 decode targets, but frame provide 1 indication,
+ descriptor.frame_dependencies.decode_target_indications = {
+ DecodeTargetIndication::kSwitch};
+
+ EXPECT_EQ(
+ RtpDependencyDescriptorExtension::ValueSize(structure, 0b11, descriptor),
+ 0u);
+ EXPECT_FALSE(RtpDependencyDescriptorExtension::Write(buffer, structure, 0b11,
+ descriptor));
+}
+
} // namespace
} // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtp_dependency_descriptor_writer.cc b/modules/rtp_rtcp/source/rtp_dependency_descriptor_writer.cc
index 31df783..59a327a 100644
--- a/modules/rtp_rtcp/source/rtp_dependency_descriptor_writer.cc
+++ b/modules/rtp_rtcp/source/rtp_dependency_descriptor_writer.cc
@@ -20,6 +20,7 @@
#include "api/transport/rtp/dependency_descriptor.h"
#include "rtc_base/bit_buffer.h"
#include "rtc_base/checks.h"
+#include "rtc_base/numerics/safe_compare.h"
namespace webrtc {
namespace {
@@ -62,6 +63,17 @@
structure_(structure),
active_chains_(active_chains),
bit_writer_(data.data(), data.size()) {
+ if (rtc::SafeNe(descriptor.frame_dependencies.chain_diffs.size(),
+ structure_.num_chains)) {
+ build_failed_ = true;
+ return;
+ }
+ if (rtc::SafeNe(
+ descriptor.frame_dependencies.decode_target_indications.size(),
+ structure_.num_decode_targets)) {
+ build_failed_ = true;
+ return;
+ }
FindBestTemplate();
}
diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc
index b2f407e..fed4adb 100644
--- a/video/rtp_video_stream_receiver2_unittest.cc
+++ b/video/rtp_video_stream_receiver2_unittest.cc
@@ -1308,7 +1308,7 @@
deltaframe1_descriptor.frame_number = 0xfffe;
DependencyDescriptor deltaframe2_descriptor;
- deltaframe1_descriptor.frame_dependencies = stream_structure.templates[1];
+ deltaframe2_descriptor.frame_dependencies = stream_structure.templates[1];
deltaframe2_descriptor.frame_number = 0x0002;
// Parser should unwrap frame ids correctly even if packets were reordered by
