From 1c1b99e30f0ad099042ee412981b076197243a04 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 7 Jan 2020 19:16:33 +0000 Subject: [PATCH] Revert "Delete RtpPayloadParams::SetDependenciesVp8Deprecated as unused" This reverts commit dc7fe40f497179721e53af1b3ece37c741bb757e. Reason for revert: speculative revert for breaking downstream projects Original change's description: > 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} TBR=danilchap@webrtc.org,eladalon@webrtc.org,nisse@webrtc.org,philipel@webrtc.org Change-Id: Ie7f875291610a7b676539a5ccc4bac9a08011f42 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10242 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/165240 Reviewed-by: Qingsi Wang Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#30173} --- call/rtp_payload_params.cc | 71 +++++++++++++++- call/rtp_payload_params.h | 23 ++++++ call/rtp_payload_params_unittest.cc | 82 +++++++------------ 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, 126 insertions(+), 60 deletions(-) diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index 0b46466739..c71af6b097 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -379,6 +379,71 @@ 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); @@ -402,10 +467,10 @@ void RtpPayloadParams::Vp8ToGeneric(const CodecSpecificInfoVP8& vp8_info, 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 ae9a2d2368..b012398518 100644 --- a/call/rtp_payload_params.h +++ b/call/rtp_payload_params.h @@ -62,6 +62,23 @@ 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, @@ -76,6 +93,12 @@ 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 2438f769ff..d3bdf5615e 100644 --- a/call/rtp_payload_params_unittest.cc +++ b/call/rtp_payload_params_unittest.cc @@ -14,7 +14,6 @@ #include #include -#include #include "absl/container/inlined_vector.h" #include "absl/types/optional.h" @@ -30,13 +29,11 @@ #include "test/gmock.h" #include "test/gtest.h" -namespace webrtc { -namespace { - using ::testing::ElementsAre; using ::testing::IsEmpty; -using ::testing::UnorderedElementsAreArray; +namespace webrtc { +namespace { const uint32_t kSsrc1 = 12345; const uint32_t kSsrc2 = 23456; const int16_t kPictureId = 123; @@ -381,32 +378,20 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { void ConvertAndCheck(int temporal_index, int64_t shared_frame_id, - const std::vector& references, - const std::vector& updates, + VideoFrameType frame_type, LayerSync layer_sync, const std::set& expected_deps, uint16_t width = 0, uint16_t height = 0) { EncodedImage encoded_image; - encoded_image._frameType = references.empty() - ? VideoFrameType::kVideoFrameKey - : VideoFrameType::kVideoFrameDelta; + encoded_image._frameType = frame_type; encoded_image._encodedWidth = width; encoded_image._encodedHeight = height; CodecSpecificInfo codec_info; codec_info.codecType = kVideoCodecVP8; - 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; - } + codec_info.codecSpecific.VP8.temporalIdx = temporal_index; + codec_info.codecSpecific.VP8.layerSync = layer_sync == kSync; RTPVideoHeader header = params_.GetRtpVideoHeader(encoded_image, &codec_info, shared_frame_id); @@ -417,8 +402,9 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { EXPECT_EQ(header.generic->frame_id, shared_frame_id); EXPECT_EQ(header.generic->temporal_index, temporal_index); - EXPECT_THAT(header.generic->dependencies, - UnorderedElementsAreArray(expected_deps)); + std::set actual_deps(header.generic->dependencies.begin(), + header.generic->dependencies.end()); + EXPECT_EQ(expected_deps, actual_deps); EXPECT_EQ(header.width, width); EXPECT_EQ(header.height, height); @@ -431,16 +417,13 @@ class RtpPayloadParamsVp8ToGenericTest : public ::testing::Test { }; TEST_F(RtpPayloadParamsVp8ToGenericTest, Keyframe) { - 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); + ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); + ConvertAndCheck(0, 1, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); + ConvertAndCheck(0, 2, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); } TEST_F(RtpPayloadParamsVp8ToGenericTest, TooHighTemporalIndex) { - ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, - 360); + ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); EncodedImage encoded_image; encoded_image._frameType = VideoFrameType::kVideoFrameDelta; @@ -455,35 +438,30 @@ TEST_F(RtpPayloadParamsVp8ToGenericTest, TooHighTemporalIndex) { EXPECT_FALSE(header.generic); } -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}); +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}); - 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}); + 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}); } TEST_F(RtpPayloadParamsVp8ToGenericTest, FrameIdGaps) { // 0101 pattern - ConvertAndCheck(0, 0, /*references=*/{}, /*updates=*/{0}, kNoSync, {}, 480, - 360); - ConvertAndCheck(1, 1, /*references=*/{0}, /*updates=*/{1}, kNoSync, {0}); + ConvertAndCheck(0, 0, VideoFrameType::kVideoFrameKey, kNoSync, {}, 480, 360); + ConvertAndCheck(1, 1, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); - ConvertAndCheck(0, 5, /*references=*/{0}, /*updates=*/{0}, kNoSync, {0}); - ConvertAndCheck(1, 10, /*references=*/{0, 1}, /*updates=*/{1}, kNoSync, - {1, 5}); + ConvertAndCheck(0, 5, VideoFrameType::kVideoFrameDelta, kNoSync, {0}); + ConvertAndCheck(1, 10, VideoFrameType::kVideoFrameDelta, kNoSync, {1, 5}); - ConvertAndCheck(0, 15, /*references=*/{0}, /*updates=*/{0}, kNoSync, {5}); - ConvertAndCheck(1, 20, /*references=*/{0, 1}, /*updates=*/{1}, kNoSync, - {10, 15}); + ConvertAndCheck(0, 15, VideoFrameType::kVideoFrameDelta, kNoSync, {5}); + ConvertAndCheck(1, 20, VideoFrameType::kVideoFrameDelta, kNoSync, {10, 15}); } class RtpPayloadParamsH264ToGenericTest : public ::testing::Test { diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index c6b109f524..571618172d 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -182,7 +182,6 @@ 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 94d96e399b..426ee76779 100644 --- a/modules/video_coding/codecs/vp8/default_temporal_layers.cc +++ b/modules/video_coding/codecs/vp8/default_temporal_layers.cc @@ -520,6 +520,7 @@ 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 72e9d51fe9..b5b963e2a9 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers.cc +++ b/modules/video_coding/codecs/vp8/screenshare_layers.cc @@ -351,6 +351,7 @@ 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 93f45c9508..54839e1e1d 100644 --- a/modules/video_coding/include/video_codec_interface.h +++ b/modules/video_coding/include/video_codec_interface.h @@ -22,7 +22,6 @@ #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 { @@ -42,9 +41,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| when not set - // by downstream projects. - RTC_DEPRECATED bool useExplicitDependencies; + // TODO(bugs.webrtc.org/10242): Remove |useExplicitDependencies| once all + // encoder-wrappers are updated. + bool useExplicitDependencies; static constexpr size_t kBuffersCount = 3; size_t referencedBuffers[kBuffersCount]; size_t referencedBuffersCount;