From dc7fe40f497179721e53af1b3ece37c741bb757e Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 13 Dec 2019 12:23:58 +0100 Subject: [PATCH] Delete RtpPayloadParams::SetDependenciesVp8Deprecated as unused Bug: webrtc:10242 Change-Id: Iddad086d8ce3652bd9f0fb12788d5c73b5ebda76 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161945 Reviewed-by: Niels Moller Reviewed-by: Philip Eliasson Reviewed-by: Elad Alon Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#30159} --- call/rtp_payload_params.cc | 71 +--------------- call/rtp_payload_params.h | 23 ----- call/rtp_payload_params_unittest.cc | 84 ++++++++++++------- modules/video_coding/BUILD.gn | 1 + .../codecs/vp8/default_temporal_layers.cc | 1 - .../codecs/vp8/screenshare_layers.cc | 1 - .../include/video_codec_interface.h | 7 +- 7 files changed, 61 insertions(+), 127 deletions(-) diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index c71af6b097..0b46466739 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -379,71 +379,6 @@ void RtpPayloadParams::Vp8ToGeneric(const CodecSpecificInfoVP8& vp8_info, generic.spatial_index = spatial_index; generic.temporal_index = temporal_index; - if (vp8_info.useExplicitDependencies) { - SetDependenciesVp8New(vp8_info, shared_frame_id, is_keyframe, - vp8_header.layerSync, &generic); - } else { - SetDependenciesVp8Deprecated(vp8_info, shared_frame_id, is_keyframe, - spatial_index, temporal_index, - vp8_header.layerSync, &generic); - } -} - -void RtpPayloadParams::SetDependenciesVp8Deprecated( - const CodecSpecificInfoVP8& vp8_info, - int64_t shared_frame_id, - bool is_keyframe, - int spatial_index, - int temporal_index, - bool layer_sync, - RTPVideoHeader::GenericDescriptorInfo* generic) { - RTC_DCHECK(!vp8_info.useExplicitDependencies); - RTC_DCHECK(!new_version_used_.has_value() || !new_version_used_.value()); - new_version_used_ = false; - - if (is_keyframe) { - RTC_DCHECK_EQ(temporal_index, 0); - last_shared_frame_id_[spatial_index].fill(-1); - last_shared_frame_id_[spatial_index][temporal_index] = shared_frame_id; - return; - } - - if (layer_sync) { - int64_t tl0_frame_id = last_shared_frame_id_[spatial_index][0]; - - for (int i = 1; i < RtpGenericFrameDescriptor::kMaxTemporalLayers; ++i) { - if (last_shared_frame_id_[spatial_index][i] < tl0_frame_id) { - last_shared_frame_id_[spatial_index][i] = -1; - } - } - - RTC_DCHECK_GE(tl0_frame_id, 0); - RTC_DCHECK_LT(tl0_frame_id, shared_frame_id); - generic->dependencies.push_back(tl0_frame_id); - } else { - for (int i = 0; i <= temporal_index; ++i) { - int64_t frame_id = last_shared_frame_id_[spatial_index][i]; - - if (frame_id != -1) { - RTC_DCHECK_LT(frame_id, shared_frame_id); - generic->dependencies.push_back(frame_id); - } - } - } - - last_shared_frame_id_[spatial_index][temporal_index] = shared_frame_id; -} - -void RtpPayloadParams::SetDependenciesVp8New( - const CodecSpecificInfoVP8& vp8_info, - int64_t shared_frame_id, - bool is_keyframe, - bool layer_sync, - RTPVideoHeader::GenericDescriptorInfo* generic) { - RTC_DCHECK(vp8_info.useExplicitDependencies); - RTC_DCHECK(!new_version_used_.has_value() || new_version_used_.value()); - new_version_used_ = true; - if (is_keyframe) { RTC_DCHECK_EQ(vp8_info.referencedBuffersCount, 0u); buffer_id_to_frame_id_.fill(shared_frame_id); @@ -467,10 +402,10 @@ void RtpPayloadParams::SetDependenciesVp8New( RTC_DCHECK_LT(dependency_frame_id, shared_frame_id); const bool is_new_dependency = - std::find(generic->dependencies.begin(), generic->dependencies.end(), - dependency_frame_id) == generic->dependencies.end(); + std::find(generic.dependencies.begin(), generic.dependencies.end(), + dependency_frame_id) == generic.dependencies.end(); if (is_new_dependency) { - generic->dependencies.push_back(dependency_frame_id); + generic.dependencies.push_back(dependency_frame_id); } } diff --git a/call/rtp_payload_params.h b/call/rtp_payload_params.h index b012398518..ae9a2d2368 100644 --- a/call/rtp_payload_params.h +++ b/call/rtp_payload_params.h @@ -62,23 +62,6 @@ class RtpPayloadParams final { bool is_keyframe, RTPVideoHeader* rtp_video_header); - // TODO(bugs.webrtc.org/10242): Delete SetDependenciesVp8Deprecated() and move - // the logic in SetDependenciesVp8New() into Vp8ToGeneric() once all hardware - // wrappers have been updated. - void SetDependenciesVp8Deprecated( - const CodecSpecificInfoVP8& vp8_info, - int64_t shared_frame_id, - bool is_keyframe, - int spatial_index, - int temporal_index, - bool layer_sync, - RTPVideoHeader::GenericDescriptorInfo* generic); - void SetDependenciesVp8New(const CodecSpecificInfoVP8& vp8_info, - int64_t shared_frame_id, - bool is_keyframe, - bool layer_sync, - RTPVideoHeader::GenericDescriptorInfo* generic); - // TODO(bugs.webrtc.org/10242): Remove once all encoder-wrappers are updated. // Holds the last shared frame id for a given (spatial, temporal) layer. std::array, @@ -93,12 +76,6 @@ class RtpPayloadParams final { // Maps buffer IDs to the frame-ID stored in them. std::array buffer_id_to_frame_id_; - // Until we remove SetDependenciesVp8Deprecated(), we should make sure - // that, for a given object, we either always use - // SetDependenciesVp8Deprecated(), or always use SetDependenciesVp8New(). - // TODO(bugs.webrtc.org/10242): Remove. - absl::optional new_version_used_; - const uint32_t ssrc_; RtpPayloadState state_; diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc index d3bdf5615e..2438f769ff 100644 --- a/call/rtp_payload_params_unittest.cc +++ b/call/rtp_payload_params_unittest.cc @@ -14,6 +14,7 @@ #include #include +#include #include "absl/container/inlined_vector.h" #include "absl/types/optional.h" @@ -29,11 +30,13 @@ #include "test/gmock.h" #include "test/gtest.h" -using ::testing::ElementsAre; -using ::testing::IsEmpty; - namespace webrtc { namespace { + +using ::testing::ElementsAre; +using ::testing::IsEmpty; +using ::testing::UnorderedElementsAreArray; + const uint32_t kSsrc1 = 12345; const uint32_t kSsrc2 = 23456; const int16_t kPictureId = 123; @@ -378,20 +381,32 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { void ConvertAndCheck(int temporal_index, int64_t shared_frame_id, - VideoFrameType frame_type, + const std::vector& references, + const std::vector& updates, LayerSync layer_sync, const std::set& expected_deps, uint16_t width = 0, uint16_t height = 0) { EncodedImage encoded_image; - encoded_image._frameType = frame_type; + encoded_image._frameType = references.empty() + ? VideoFrameType::kVideoFrameKey + : VideoFrameType::kVideoFrameDelta; encoded_image._encodedWidth = width; encoded_image._encodedHeight = height; CodecSpecificInfo codec_info; codec_info.codecType = kVideoCodecVP8; - codec_info.codecSpecific.VP8.temporalIdx = temporal_index; - codec_info.codecSpecific.VP8.layerSync = layer_sync == kSync; + auto& vp8 = codec_info.codecSpecific.VP8; + vp8.temporalIdx = temporal_index; + vp8.layerSync = layer_sync == kSync; + vp8.referencedBuffersCount = 0; + for (int reference : references) { + vp8.referencedBuffers[vp8.referencedBuffersCount++] = reference; + } + vp8.updatedBuffersCount = 0; + for (int update : updates) { + vp8.updatedBuffers[vp8.updatedBuffersCount++] = update; + } RTPVideoHeader header = params_.GetRtpVideoHeader(encoded_image, &codec_info, shared_frame_id); @@ -402,9 +417,8 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { EXPECT_EQ(header.generic->frame_id, shared_frame_id); EXPECT_EQ(header.generic->temporal_index, temporal_index); - std::set actual_deps(header.generic->dependencies.begin(), - header.generic->dependencies.end()); - EXPECT_EQ(expected_deps, actual_deps); + EXPECT_THAT(header.generic->dependencies, + UnorderedElementsAreArray(expected_deps)); EXPECT_EQ(header.width, width); EXPECT_EQ(header.height, height); @@ -417,13 +431,16 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { }; TEST_F(RtpPayloadParamsVp8ToGenericTest, Keyframe) { - ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); - ConvertAndCheck(0, 1, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(0, 2, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); + ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, + 360); + ConvertAndCheck(0, 1, /*references=*/{0}, /*updates=*/{0}, kNoSync, {0}); + ConvertAndCheck(0, 2, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, + 360); } TEST_F(RtpPayloadParamsVp8ToGenericTest, TooHighTemporalIndex) { - ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); + ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, + 360); EncodedImage encoded_image; encoded_image._frameType = VideoFrameType::kVideoFrameDelta; @@ -438,30 +455,35 @@ TEST_F(RtpPayloadParamsVp8ToGenericTest, TooHighTemporalIndex) { EXPECT_FALSE(header.generic); } -TEST_F(RtpPayloadParamsVp8ToGenericTest, LayerSync) { - // 02120212 pattern - ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); - ConvertAndCheck(2, 1, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(1, 2, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(2, 3, VideoFrameType::kVideoFrameDelta, kNoSync, {0, 1, 2}); +TEST_F(RtpPayloadParamsVp8ToGenericTest, Pattern02120212) { + ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, + 360); + ConvertAndCheck(2, 1, /*references=*/{0}, /*updates=*/{2}, kNoSync, {0}); + ConvertAndCheck(1, 2, /*references=*/{0}, /*updates=*/{1}, kNoSync, {0}); + ConvertAndCheck(2, 3, /*references=*/{0, 1, 2}, /*updates=*/{2}, kNoSync, + {0, 1, 2}); - ConvertAndCheck(0, 4, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(2, 5, VideoFrameType::kVideoFrameDelta, kNoSync, {2, 3, 4}); - ConvertAndCheck(1, 6, VideoFrameType::kVideoFrameDelta, kSync, - {4}); // layer sync - ConvertAndCheck(2, 7, VideoFrameType::kVideoFrameDelta, kNoSync, {4, 5, 6}); + ConvertAndCheck(0, 4, /*references=*/{0}, /*updates=*/{0}, kNoSync, {0}); + ConvertAndCheck(2, 5, /*references=*/{0, 1, 2}, /*updates=*/{2}, kNoSync, + {2, 3, 4}); + ConvertAndCheck(1, 6, /*references=*/{0}, /*updates=*/{1}, kSync, {4}); + ConvertAndCheck(2, 7, /*references=*/{0, 1, 2}, /*updates=*/{2}, kNoSync, + {4, 5, 6}); } TEST_F(RtpPayloadParamsVp8ToGenericTest, FrameIdGaps) { // 0101 pattern - ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); - ConvertAndCheck(1, 1, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); + ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, + 360); + ConvertAndCheck(1, 1, /*references=*/{0}, /*updates=*/{1}, kNoSync, {0}); - ConvertAndCheck(0, 5, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(1, 10, VideoFrameType::kVideoFrameDelta, kNoSync, {1, 5}); + ConvertAndCheck(0, 5, /*references=*/{0}, /*updates=*/{0}, kNoSync, {0}); + ConvertAndCheck(1, 10, /*references=*/{0, 1}, /*updates=*/{1}, kNoSync, + {1, 5}); - ConvertAndCheck(0, 15, VideoFrameType::kVideoFrameDelta, kNoSync, {5}); - ConvertAndCheck(1, 20, VideoFrameType::kVideoFrameDelta, kNoSync, {10, 15}); + ConvertAndCheck(0, 15, /*references=*/{0}, /*updates=*/{0}, kNoSync, {5}); + ConvertAndCheck(1, 20, /*references=*/{0, 1}, /*updates=*/{1}, kNoSync, + {10, 15}); } class RtpPayloadParamsH264ToGenericTest : public ::testing::Test { diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 571618172d..c6b109f524 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -182,6 +182,7 @@ rtc_library("video_codec_interface") { "../../api/video_codecs:video_codecs_api", "../../common_video", "../../common_video/generic_frame_descriptor", + "../../rtc_base:deprecation", "../../rtc_base/system:rtc_export", "//third_party/abseil-cpp/absl/types:optional", ] diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.cc b/modules/video_coding/codecs/vp8/default_temporal_layers.cc index 426ee76779..94d96e399b 100644 --- a/modules/video_coding/codecs/vp8/default_temporal_layers.cc +++ b/modules/video_coding/codecs/vp8/default_temporal_layers.cc @@ -520,7 +520,6 @@ void DefaultTemporalLayers::OnEncodeDone(size_t stream_index, } } - vp8_info.useExplicitDependencies = true; RTC_DCHECK_EQ(vp8_info.referencedBuffersCount, 0u); RTC_DCHECK_EQ(vp8_info.updatedBuffersCount, 0u); diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.cc b/modules/video_coding/codecs/vp8/screenshare_layers.cc index b5b963e2a9..72e9d51fe9 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers.cc +++ b/modules/video_coding/codecs/vp8/screenshare_layers.cc @@ -351,7 +351,6 @@ void ScreenshareLayers::OnEncodeDone(size_t stream_index, layers_[active_layer_].state = TemporalLayer::State::kNormal; } - vp8_info.useExplicitDependencies = true; RTC_DCHECK_EQ(vp8_info.referencedBuffersCount, 0u); RTC_DCHECK_EQ(vp8_info.updatedBuffersCount, 0u); diff --git a/modules/video_coding/include/video_codec_interface.h b/modules/video_coding/include/video_codec_interface.h index 54839e1e1d..93f45c9508 100644 --- a/modules/video_coding/include/video_codec_interface.h +++ b/modules/video_coding/include/video_codec_interface.h @@ -22,6 +22,7 @@ #include "modules/video_coding/codecs/h264/include/h264_globals.h" #include "modules/video_coding/codecs/vp9/include/vp9_globals.h" #include "modules/video_coding/include/video_error_codes.h" +#include "rtc_base/deprecation.h" #include "rtc_base/system/rtc_export.h" namespace webrtc { @@ -41,9 +42,9 @@ struct CodecSpecificInfoVP8 { // codec buffers, but the exact mapping (i.e. whether 0 refers to Last, // to Golden or to Arf) is not pre-determined. // More references may be specified than are strictly necessary, but not less. - // TODO(bugs.webrtc.org/10242): Remove |useExplicitDependencies| once all - // encoder-wrappers are updated. - bool useExplicitDependencies; + // TODO(bugs.webrtc.org/10242): Remove |useExplicitDependencies| when not set + // by downstream projects. + RTC_DEPRECATED bool useExplicitDependencies; static constexpr size_t kBuffersCount = 3; size_t referencedBuffers[kBuffersCount]; size_t referencedBuffersCount;