From cf1308fddd9974a1fdf5f9d2fb03e93af69b6d1b Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 18 Nov 2020 18:27:37 +0100 Subject: [PATCH] Ignore frame type when calculating generic frame dependencies. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Erik Språng Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#32667} --- call/rtp_payload_params.cc | 10 +- call/rtp_payload_params.h | 3 +- modules/video_coding/BUILD.gn | 1 - .../frame_dependencies_calculator.cc | 43 +++---- .../frame_dependencies_calculator.h | 2 - .../frame_dependencies_calculator_unittest.cc | 114 +++++++----------- .../svc/scalability_structure_test_helpers.cc | 3 - 7 files changed, 66 insertions(+), 110 deletions(-) diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index f8d6776c12..5c9c6c0c05 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -235,12 +235,11 @@ void RtpPayloadParams::SetCodecSpecific(RTPVideoHeader* rtp_video_header, 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 @@ void RtpPayloadParams::SetGeneric(const CodecSpecificInfo* codec_specific_info, 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 2e0faeb5c9..ebfdd4605a 100644 --- a/call/rtp_payload_params.h +++ b/call/rtp_payload_params.h @@ -50,8 +50,7 @@ class RtpPayloadParams final { 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 472cb5de27..e7a9def577 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -60,7 +60,6 @@ rtc_library("frame_dependencies_calculator") { 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 6de5081b94..7ca59f779a 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 FrameDependenciesCalculator::FromBuffersUsage( - VideoFrameType frame_type, int64_t frame_id, rtc::ArrayView buffers_usage) { absl::InlinedVector dependencies; @@ -37,29 +35,28 @@ absl::InlinedVector FrameDependenciesCalculator::FromBuffersUsage( } std::set direct_depenendencies; std::set 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 b70eddfc53..2c4a8502e1 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 @@ class FrameDependenciesCalculator { // Calculates frame dependencies based on previous encoder buffer usage. absl::InlinedVector FromBuffersUsage( - VideoFrameType frame_type, int64_t frame_id, rtc::ArrayView buffers_usage); diff --git a/modules/video_coding/frame_dependencies_calculator_unittest.cc b/modules/video_coding/frame_dependencies_calculator_unittest.cc index 81f774b227..a09650401a 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::ElementsAre; 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 @@ TEST(FrameDependenciesCalculatorTest, SingleLayer) { 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 @@ TEST(FrameDependenciesCalculatorTest, TwoTemporalLayers) { 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 @@ TEST(FrameDependenciesCalculatorTest, ThreeTemporalLayers4FramePattern) { 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 @@ TEST(FrameDependenciesCalculatorTest, SimulcastWith2Layers) { 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 2b0393f9cf..aeb4d88f1a 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 @@ void ScalabilityStructureWrapper::GenerateFrames( 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); }