Ignore frame type when calculating generic frame dependencies. When spatial scalability is used, both vpx and aom set key frame flag for all spatial layers of the first frame, while rtp code expect it to be set only on the frame without spatial dependencies. That creates confusion for the frame dependency calculator. Simplest solution seems to ignore that confusing signal and instead rely encoder wrappers update frame buffer usages when key frame is generated. Bug: webrtc:11999 Change-Id: Ica24f1d8d42d32dd24664beabf32ac24872cd15a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/194002 Reviewed-by: Philip Eliasson <philipel@webrtc.org> Reviewed-by: Erik Språng <sprang@webrtc.org> Commit-Queue: Danil Chapovalov <danilchap@webrtc.org> Cr-Commit-Position: refs/heads/master@{#32667}
diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index f8d6776..5c9c6c0 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc
@@ -235,12 +235,11 @@ RTPVideoHeader::GenericDescriptorInfo RtpPayloadParams::GenericDescriptorFromFrameInfo( const GenericFrameInfo& frame_info, - int64_t frame_id, - VideoFrameType frame_type) { + int64_t frame_id) { RTPVideoHeader::GenericDescriptorInfo generic; generic.frame_id = frame_id; generic.dependencies = dependencies_calculator_.FromBuffersUsage( - frame_type, frame_id, frame_info.encoder_buffers); + frame_id, frame_info.encoder_buffers); generic.chain_diffs = chains_calculator_.From(frame_id, frame_info.part_of_chain); generic.spatial_index = frame_info.spatial_id; @@ -261,9 +260,8 @@ chains_calculator_.Reset( codec_specific_info->generic_frame_info->part_of_chain); } - rtp_video_header->generic = - GenericDescriptorFromFrameInfo(*codec_specific_info->generic_frame_info, - frame_id, rtp_video_header->frame_type); + rtp_video_header->generic = GenericDescriptorFromFrameInfo( + *codec_specific_info->generic_frame_info, frame_id); return; }
diff --git a/call/rtp_payload_params.h b/call/rtp_payload_params.h index 2e0faeb..ebfdd46 100644 --- a/call/rtp_payload_params.h +++ b/call/rtp_payload_params.h
@@ -50,8 +50,7 @@ bool first_frame_in_picture); RTPVideoHeader::GenericDescriptorInfo GenericDescriptorFromFrameInfo( const GenericFrameInfo& frame_info, - int64_t frame_id, - VideoFrameType frame_type); + int64_t frame_id); void SetGeneric(const CodecSpecificInfo* codec_specific_info, int64_t frame_id, bool is_keyframe,
diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 472cb5d..e7a9def 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn
@@ -60,7 +60,6 @@ deps = [ "../../api:array_view", - "../../api/video:video_frame_type", "../../common_video/generic_frame_descriptor", "../../rtc_base:checks", "../../rtc_base:logging",
diff --git a/modules/video_coding/frame_dependencies_calculator.cc b/modules/video_coding/frame_dependencies_calculator.cc index 6de5081..7ca59f7 100644 --- a/modules/video_coding/frame_dependencies_calculator.cc +++ b/modules/video_coding/frame_dependencies_calculator.cc
@@ -17,14 +17,12 @@ #include "absl/algorithm/container.h" #include "absl/container/inlined_vector.h" #include "api/array_view.h" -#include "api/video/video_frame_type.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" namespace webrtc { absl::InlinedVector<int64_t, 5> FrameDependenciesCalculator::FromBuffersUsage( - VideoFrameType frame_type, int64_t frame_id, rtc::ArrayView<const CodecBufferUsage> buffers_usage) { absl::InlinedVector<int64_t, 5> dependencies; @@ -37,29 +35,28 @@ } std::set<int64_t> direct_depenendencies; std::set<int64_t> indirect_depenendencies; - if (frame_type == VideoFrameType::kVideoFrameDelta) { - for (const CodecBufferUsage& buffer_usage : buffers_usage) { - if (!buffer_usage.referenced) { - continue; - } - const BufferUsage& buffer = buffers_[buffer_usage.id]; - if (buffer.frame_id == absl::nullopt) { - RTC_LOG(LS_ERROR) << "Odd configuration: frame " << frame_id - << " references buffer #" << buffer_usage.id - << " that was never updated."; - continue; - } - direct_depenendencies.insert(*buffer.frame_id); - indirect_depenendencies.insert(buffer.dependencies.begin(), - buffer.dependencies.end()); + + for (const CodecBufferUsage& buffer_usage : buffers_usage) { + if (!buffer_usage.referenced) { + continue; } - // Reduce references: if frame #3 depends on frame #2 and #1, and frame #2 - // depends on frame #1, then frame #3 needs to depend just on frame #2. - // Though this set diff removes only 1 level of indirection, it seems - // enough for all currently used structures. - absl::c_set_difference(direct_depenendencies, indirect_depenendencies, - std::back_inserter(dependencies)); + const BufferUsage& buffer = buffers_[buffer_usage.id]; + if (buffer.frame_id == absl::nullopt) { + RTC_LOG(LS_ERROR) << "Odd configuration: frame " << frame_id + << " references buffer #" << buffer_usage.id + << " that was never updated."; + continue; + } + direct_depenendencies.insert(*buffer.frame_id); + indirect_depenendencies.insert(buffer.dependencies.begin(), + buffer.dependencies.end()); } + // Reduce references: if frame #3 depends on frame #2 and #1, and frame #2 + // depends on frame #1, then frame #3 needs to depend just on frame #2. + // Though this set diff removes only 1 level of indirection, it seems + // enough for all currently used structures. + absl::c_set_difference(direct_depenendencies, indirect_depenendencies, + std::back_inserter(dependencies)); // Update buffers. for (const CodecBufferUsage& buffer_usage : buffers_usage) {
diff --git a/modules/video_coding/frame_dependencies_calculator.h b/modules/video_coding/frame_dependencies_calculator.h index b70eddf..2c4a850 100644 --- a/modules/video_coding/frame_dependencies_calculator.h +++ b/modules/video_coding/frame_dependencies_calculator.h
@@ -18,7 +18,6 @@ #include "absl/container/inlined_vector.h" #include "absl/types/optional.h" #include "api/array_view.h" -#include "api/video/video_frame_type.h" #include "common_video/generic_frame_descriptor/generic_frame_info.h" namespace webrtc { @@ -33,7 +32,6 @@ // Calculates frame dependencies based on previous encoder buffer usage. absl::InlinedVector<int64_t, 5> FromBuffersUsage( - VideoFrameType frame_type, int64_t frame_id, rtc::ArrayView<const CodecBufferUsage> buffers_usage);
diff --git a/modules/video_coding/frame_dependencies_calculator_unittest.cc b/modules/video_coding/frame_dependencies_calculator_unittest.cc index 81f774b..a096504 100644 --- a/modules/video_coding/frame_dependencies_calculator_unittest.cc +++ b/modules/video_coding/frame_dependencies_calculator_unittest.cc
@@ -10,7 +10,6 @@ #include "modules/video_coding/frame_dependencies_calculator.h" -#include "api/video/video_frame_type.h" #include "common_video/generic_frame_descriptor/generic_frame_info.h" #include "test/gmock.h" #include "test/gtest.h" @@ -22,9 +21,6 @@ using ::testing::IsEmpty; using ::testing::UnorderedElementsAre; -constexpr VideoFrameType kVideoFrameKey = VideoFrameType::kVideoFrameKey; -constexpr VideoFrameType kVideoFrameDelta = VideoFrameType::kVideoFrameDelta; - constexpr CodecBufferUsage ReferenceAndUpdate(int id) { return CodecBufferUsage(id, /*referenced=*/true, /*updated=*/true); } @@ -39,15 +35,11 @@ CodecBufferUsage pattern[] = {ReferenceAndUpdate(0)}; FrameDependenciesCalculator calculator; - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameKey, /*frame_id=*/1, pattern), - IsEmpty()); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/3, pattern), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/6, pattern), - ElementsAre(3)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/1, pattern), IsEmpty()); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/3, pattern), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/6, pattern), + ElementsAre(3)); } TEST(FrameDependenciesCalculatorTest, TwoTemporalLayers) { @@ -61,30 +53,21 @@ CodecBufferUsage pattern3[] = {Reference(0), Reference(1)}; FrameDependenciesCalculator calculator; - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameKey, /*frame_id=*/1, pattern0), - IsEmpty()); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/2, pattern1), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/3, pattern2), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/4, pattern3), - UnorderedElementsAre(2, 3)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/5, pattern0), - ElementsAre(3)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/6, pattern1), - ElementsAre(5)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/7, pattern2), - ElementsAre(5)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/8, pattern3), - UnorderedElementsAre(6, 7)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/1, pattern0), IsEmpty()); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/2, pattern1), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/3, pattern2), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/4, pattern3), + UnorderedElementsAre(2, 3)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/5, pattern0), + ElementsAre(3)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/6, pattern1), + ElementsAre(5)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/7, pattern2), + ElementsAre(5)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/8, pattern3), + UnorderedElementsAre(6, 7)); } TEST(FrameDependenciesCalculatorTest, ThreeTemporalLayers4FramePattern) { @@ -99,26 +82,19 @@ CodecBufferUsage pattern3[] = {Reference(0), Reference(1), Reference(2)}; FrameDependenciesCalculator calculator; - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameKey, /*frame_id=*/1, pattern0), - IsEmpty()); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/2, pattern1), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/3, pattern2), - ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/1, pattern0), IsEmpty()); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/2, pattern1), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/3, pattern2), + ElementsAre(1)); // Note that frame#4 references buffer#0 that is updated by frame#1, // yet there is no direct dependency from frame#4 to frame#1. - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/4, pattern3), - UnorderedElementsAre(2, 3)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/5, pattern0), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/6, pattern1), - ElementsAre(5)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/4, pattern3), + UnorderedElementsAre(2, 3)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/5, pattern0), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/6, pattern1), + ElementsAre(5)); } TEST(FrameDependenciesCalculatorTest, SimulcastWith2Layers) { @@ -129,24 +105,16 @@ CodecBufferUsage pattern1[] = {ReferenceAndUpdate(1)}; FrameDependenciesCalculator calculator; - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameKey, /*frame_id=*/1, pattern0), - IsEmpty()); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameKey, /*frame_id=*/2, pattern1), - IsEmpty()); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/3, pattern0), - ElementsAre(1)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/4, pattern1), - ElementsAre(2)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/5, pattern0), - ElementsAre(3)); - EXPECT_THAT( - calculator.FromBuffersUsage(kVideoFrameDelta, /*frame_id=*/6, pattern1), - ElementsAre(4)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/1, pattern0), IsEmpty()); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/2, pattern1), IsEmpty()); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/3, pattern0), + ElementsAre(1)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/4, pattern1), + ElementsAre(2)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/5, pattern0), + ElementsAre(3)); + EXPECT_THAT(calculator.FromBuffersUsage(/*frame_id=*/6, pattern1), + ElementsAre(4)); } } // namespace
diff --git a/modules/video_coding/svc/scalability_structure_test_helpers.cc b/modules/video_coding/svc/scalability_structure_test_helpers.cc index 2b0393f..aeb4d88 100644 --- a/modules/video_coding/svc/scalability_structure_test_helpers.cc +++ b/modules/video_coding/svc/scalability_structure_test_helpers.cc
@@ -17,7 +17,6 @@ #include "api/array_view.h" #include "api/transport/rtp/dependency_descriptor.h" #include "api/video/video_bitrate_allocation.h" -#include "api/video/video_frame_type.h" #include "modules/video_coding/chain_diff_calculator.h" #include "modules/video_coding/frame_dependencies_calculator.h" #include "modules/video_coding/svc/scalable_video_controller.h" @@ -56,8 +55,6 @@ frame_info.chain_diffs = chain_diff_calculator_.From(frame_id, frame_info.part_of_chain); for (int64_t base_frame_id : frame_deps_calculator_.FromBuffersUsage( - is_keyframe ? VideoFrameType::kVideoFrameKey - : VideoFrameType::kVideoFrameDelta, frame_id, frame_info.encoder_buffers)) { frame_info.frame_diffs.push_back(frame_id - base_frame_id); }