From 727d2afc4330efebc904e0e4f366e885d7b08787 Mon Sep 17 00:00:00 2001 From: Sergey Silkin Date: Thu, 11 Mar 2021 17:05:44 +0000 Subject: [PATCH] Revert "Parse encoded frame QP if not provided by encoder" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 8639673f0c098efc294a7593fa3bd98e28ab7508. Reason for revert: linux_tsan fails https://ci.chromium.org/ui/p/webrtc/builders/ci/Linux%20Tsan%20v2/25329/overview Original change's description: > Parse encoded frame QP if not provided by encoder > > Bug: webrtc:12542 > Change-Id: Ic70b46e226f158db7a478a9f20e1f940804febba > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/210966 > Commit-Queue: Sergey Silkin > Reviewed-by: Åsa Persson > Cr-Commit-Position: refs/heads/master@{#33434} TBR=asapersson@webrtc.org,ssilkin@webrtc.org Change-Id: Ie251d8f70f8e87fd86b63730aefd2ef3f941e4bb No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:12542 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/211355 Reviewed-by: Sergey Silkin Commit-Queue: Sergey Silkin Cr-Commit-Position: refs/heads/master@{#33441} --- modules/video_coding/BUILD.gn | 3 - modules/video_coding/utility/qp_parser.cc | 55 --------- modules/video_coding/utility/qp_parser.h | 37 ------ .../utility/qp_parser_unittest.cc | 111 ------------------ video/video_stream_encoder.cc | 21 +--- video/video_stream_encoder.h | 6 - video/video_stream_encoder_unittest.cc | 80 +------------ 7 files changed, 10 insertions(+), 303 deletions(-) delete mode 100644 modules/video_coding/utility/qp_parser.cc delete mode 100644 modules/video_coding/utility/qp_parser.h delete mode 100644 modules/video_coding/utility/qp_parser_unittest.cc diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 8bb90705b5..2a504363df 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -323,8 +323,6 @@ rtc_library("video_coding_utility") { "utility/ivf_file_reader.h", "utility/ivf_file_writer.cc", "utility/ivf_file_writer.h", - "utility/qp_parser.cc", - "utility/qp_parser.h", "utility/quality_scaler.cc", "utility/quality_scaler.h", "utility/simulcast_rate_allocator.cc", @@ -971,7 +969,6 @@ if (rtc_include_tests) { "utility/framerate_controller_unittest.cc", "utility/ivf_file_reader_unittest.cc", "utility/ivf_file_writer_unittest.cc", - "utility/qp_parser_unittest.cc", "utility/quality_scaler_unittest.cc", "utility/simulcast_rate_allocator_unittest.cc", "video_codec_initializer_unittest.cc", diff --git a/modules/video_coding/utility/qp_parser.cc b/modules/video_coding/utility/qp_parser.cc deleted file mode 100644 index 71958d0994..0000000000 --- a/modules/video_coding/utility/qp_parser.cc +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/video_coding/utility/qp_parser.h" - -#include - -#include "modules/video_coding/utility/vp8_header_parser.h" -#include "modules/video_coding/utility/vp9_uncompressed_header_parser.h" - -namespace webrtc { - -absl::optional QpParser::Parse(VideoCodecType codec_type, - size_t spatial_idx, - const uint8_t* frame_data, - size_t frame_size) { - if (frame_data == nullptr || frame_size == 0) { - return absl::nullopt; - } - - if (codec_type == kVideoCodecVP8) { - int qp = -1; - if (vp8::GetQp(frame_data, frame_size, &qp)) { - return qp; - } - } else if (codec_type == kVideoCodecVP9) { - int qp = -1; - if (vp9::GetQp(frame_data, frame_size, &qp)) { - return qp; - } - } else if (codec_type == kVideoCodecH264) { - H264BitstreamParser& parser = FetchOrCreateH264Parser(spatial_idx); - parser.ParseBitstream( - rtc::ArrayView(frame_data, frame_size)); - return parser.GetLastSliceQp(); - } - - return absl::nullopt; -} - -H264BitstreamParser& QpParser::FetchOrCreateH264Parser(size_t spatial_idx) { - if (h264_parsers_.find(spatial_idx) == h264_parsers_.end()) { - h264_parsers_.emplace(std::make_pair(spatial_idx, H264BitstreamParser())); - } - return h264_parsers_.at(spatial_idx); -} - -} // namespace webrtc diff --git a/modules/video_coding/utility/qp_parser.h b/modules/video_coding/utility/qp_parser.h deleted file mode 100644 index 5175e372b7..0000000000 --- a/modules/video_coding/utility/qp_parser.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MODULES_VIDEO_CODING_UTILITY_QP_PARSER_H_ -#define MODULES_VIDEO_CODING_UTILITY_QP_PARSER_H_ - -#include - -#include "absl/types/optional.h" -#include "api/video/video_codec_type.h" -#include "common_video/h264/h264_bitstream_parser.h" - -namespace webrtc { - -class QpParser { - public: - absl::optional Parse(VideoCodecType codec_type, - size_t spatial_idx, - const uint8_t* frame_data, - size_t frame_size); - - private: - H264BitstreamParser& FetchOrCreateH264Parser(size_t spatial_idx); - - std::map h264_parsers_; -}; - -} // namespace webrtc - -#endif // MODULES_VIDEO_CODING_UTILITY_QP_PARSER_H_ diff --git a/modules/video_coding/utility/qp_parser_unittest.cc b/modules/video_coding/utility/qp_parser_unittest.cc deleted file mode 100644 index 1eff2e8e44..0000000000 --- a/modules/video_coding/utility/qp_parser_unittest.cc +++ /dev/null @@ -1,111 +0,0 @@ -/* - * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/video_coding/utility/qp_parser.h" - -#include - -#include "test/gtest.h" - -namespace webrtc { - -namespace { -// ffmpeg -s 16x16 -f rawvideo -pix_fmt rgb24 -r 30 -i /dev/zero -c:v libvpx -// -qmin 20 -qmax 20 -crf 20 -frames:v 1 -y out.ivf -const uint8_t kCodedFrameVp8Qp25[] = { - 0x10, 0x02, 0x00, 0x9d, 0x01, 0x2a, 0x10, 0x00, 0x10, 0x00, - 0x02, 0x47, 0x08, 0x85, 0x85, 0x88, 0x85, 0x84, 0x88, 0x0c, - 0x82, 0x00, 0x0c, 0x0d, 0x60, 0x00, 0xfe, 0xfc, 0x5c, 0xd0}; - -// ffmpeg -s 16x16 -f rawvideo -pix_fmt rgb24 -r 30 -i /dev/zero -c:v libvpx-vp9 -// -qmin 24 -qmax 24 -crf 24 -frames:v 1 -y out.ivf -const uint8_t kCodedFrameVp9Qp96[] = { - 0xa2, 0x49, 0x83, 0x42, 0xe0, 0x00, 0xf0, 0x00, 0xf6, 0x00, - 0x38, 0x24, 0x1c, 0x18, 0xc0, 0x00, 0x00, 0x30, 0x70, 0x00, - 0x00, 0x4a, 0xa7, 0xff, 0xfc, 0xb9, 0x01, 0xbf, 0xff, 0xff, - 0x97, 0x20, 0xdb, 0xff, 0xff, 0xcb, 0x90, 0x5d, 0x40}; - -// ffmpeg -s 16x16 -f rawvideo -pix_fmt yuv420p -r 30 -i /dev/zero -c:v libx264 -// -qmin 38 -qmax 38 -crf 38 -profile:v baseline -frames:v 2 -y out.264 -const uint8_t kCodedFrameH264SpsPpsIdrQp38[] = { - 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0xc0, 0x0a, 0xd9, 0x1e, 0x84, - 0x00, 0x00, 0x03, 0x00, 0x04, 0x00, 0x00, 0x03, 0x00, 0xf0, 0x3c, - 0x48, 0x99, 0x20, 0x00, 0x00, 0x00, 0x01, 0x68, 0xcb, 0x80, 0xc4, - 0xb2, 0x00, 0x00, 0x01, 0x65, 0x88, 0x84, 0xf1, 0x18, 0xa0, 0x00, - 0x20, 0x5b, 0x1c, 0x00, 0x04, 0x07, 0xe3, 0x80, 0x00, 0x80, 0xfe}; - -const uint8_t kCodedFrameH264SpsPpsIdrQp49[] = { - 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0xc0, 0x0a, 0xd9, 0x1e, 0x84, - 0x00, 0x00, 0x03, 0x00, 0x04, 0x00, 0x00, 0x03, 0x00, 0xf0, 0x3c, - 0x48, 0x99, 0x20, 0x00, 0x00, 0x00, 0x01, 0x68, 0xcb, 0x80, 0x5d, - 0x2c, 0x80, 0x00, 0x00, 0x01, 0x65, 0x88, 0x84, 0xf1, 0x18, 0xa0, - 0x00, 0x5e, 0x38, 0x00, 0x08, 0x03, 0xc7, 0x00, 0x01, 0x00, 0x7c}; - -const uint8_t kCodedFrameH264InterSliceQpDelta0[] = {0x00, 0x00, 0x00, 0x01, - 0x41, 0x9a, 0x39, 0xea}; - -} // namespace - -TEST(QpParserTest, ParseQpVp8) { - QpParser parser; - absl::optional qp = parser.Parse( - kVideoCodecVP8, 0, kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25)); - EXPECT_EQ(qp, 25u); -} - -TEST(QpParserTest, ParseQpVp9) { - QpParser parser; - absl::optional qp = parser.Parse( - kVideoCodecVP9, 0, kCodedFrameVp9Qp96, sizeof(kCodedFrameVp9Qp96)); - EXPECT_EQ(qp, 96u); -} - -TEST(QpParserTest, ParseQpH264) { - QpParser parser; - absl::optional qp = parser.Parse( - VideoCodecType::kVideoCodecH264, 0, kCodedFrameH264SpsPpsIdrQp38, - sizeof(kCodedFrameH264SpsPpsIdrQp38)); - EXPECT_EQ(qp, 38u); - - qp = parser.Parse(kVideoCodecH264, 1, kCodedFrameH264SpsPpsIdrQp49, - sizeof(kCodedFrameH264SpsPpsIdrQp49)); - EXPECT_EQ(qp, 49u); - - qp = parser.Parse(kVideoCodecH264, 0, kCodedFrameH264InterSliceQpDelta0, - sizeof(kCodedFrameH264InterSliceQpDelta0)); - EXPECT_EQ(qp, 38u); - - qp = parser.Parse(kVideoCodecH264, 1, kCodedFrameH264InterSliceQpDelta0, - sizeof(kCodedFrameH264InterSliceQpDelta0)); - EXPECT_EQ(qp, 49u); -} - -TEST(QpParserTest, ParseQpUnsupportedCodecType) { - QpParser parser; - absl::optional qp = parser.Parse( - kVideoCodecGeneric, 0, kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25)); - EXPECT_FALSE(qp.has_value()); -} - -TEST(QpParserTest, ParseQpNullData) { - QpParser parser; - absl::optional qp = - parser.Parse(kVideoCodecGeneric, 0, nullptr, 100); - EXPECT_FALSE(qp.has_value()); -} - -TEST(QpParserTest, ParseQpEmptyData) { - QpParser parser; - absl::optional qp = - parser.Parse(kVideoCodecGeneric, 0, kCodedFrameVp8Qp25, 0); - EXPECT_FALSE(qp.has_value()); -} - -} // namespace webrtc diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index db61e7a4e5..cfef876f04 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -658,8 +658,6 @@ VideoStreamEncoder::VideoStreamEncoder( /*source=*/nullptr), default_limits_allowed_( !field_trial::IsEnabled("WebRTC-DefaultBitrateLimitsKillSwitch")), - qp_parsing_allowed_( - !field_trial::IsEnabled("WebRTC-QpParsingKillSwitch")), encoder_queue_(task_queue_factory->CreateTaskQueue( "EncoderQueue", TaskQueueFactory::Priority::NORMAL)) { @@ -1874,18 +1872,6 @@ EncodedImageCallback::Result VideoStreamEncoder::OnEncodedImage( frame_encode_metadata_writer_.UpdateBitstream(codec_specific_info, &image_copy); - VideoCodecType codec_type = codec_specific_info - ? codec_specific_info->codecType - : VideoCodecType::kVideoCodecGeneric; - - if (image_copy.qp_ < 0 && qp_parsing_allowed_) { - // Parse encoded frame QP if that was not provided by encoder. - image_copy.qp_ = qp_parser_ - .Parse(codec_type, spatial_idx, image_copy.data(), - image_copy.size()) - .value_or(-1); - } - // Piggyback ALR experiment group id and simulcast id into the content type. const uint8_t experiment_id = experiment_groups_[videocontenttypehelpers::IsScreenshare( @@ -1908,9 +1894,12 @@ EncodedImageCallback::Result VideoStreamEncoder::OnEncodedImage( // Post a task because |send_codec_| requires |encoder_queue_| lock. unsigned int image_width = image_copy._encodedWidth; unsigned int image_height = image_copy._encodedHeight; - encoder_queue_.PostTask([this, codec_type, image_width, image_height] { + VideoCodecType codec = codec_specific_info + ? codec_specific_info->codecType + : VideoCodecType::kVideoCodecGeneric; + encoder_queue_.PostTask([this, codec, image_width, image_height] { RTC_DCHECK_RUN_ON(&encoder_queue_); - if (codec_type == VideoCodecType::kVideoCodecVP9 && + if (codec == VideoCodecType::kVideoCodecVP9 && send_codec_.VP9()->automaticResizeOn) { unsigned int expected_width = send_codec_.width; unsigned int expected_height = send_codec_.height; diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index e97dc21fc4..b1c3bd8718 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -34,7 +34,6 @@ #include "call/adaptation/video_source_restrictions.h" #include "call/adaptation/video_stream_input_state_provider.h" #include "modules/video_coding/utility/frame_dropper.h" -#include "modules/video_coding/utility/qp_parser.h" #include "rtc_base/experiments/rate_control_settings.h" #include "rtc_base/numerics/exp_filter.h" #include "rtc_base/race_checker.h" @@ -444,11 +443,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // Default bitrate limits in EncoderInfoSettings allowed. const bool default_limits_allowed_; - // QP parser is used to extract QP value from encoded frame when that is not - // provided by encoder. - QpParser qp_parser_; - const bool qp_parsing_allowed_; - // Public methods are proxied to the task queues. The queues must be destroyed // first to make sure no tasks run that use other members. rtc::TaskQueue encoder_queue_; diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 98e63a7da2..72ff98407c 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -97,11 +97,6 @@ uint8_t optimal_sps[] = {0, 0, 0, 1, H264::NaluType::kSps, 0x05, 0x03, 0xC7, 0xE0, 0x1B, 0x41, 0x10, 0x8D, 0x00}; -const uint8_t kCodedFrameVp8Qp25[] = { - 0x10, 0x02, 0x00, 0x9d, 0x01, 0x2a, 0x10, 0x00, 0x10, 0x00, - 0x02, 0x47, 0x08, 0x85, 0x85, 0x88, 0x85, 0x84, 0x88, 0x0c, - 0x82, 0x00, 0x0c, 0x0d, 0x60, 0x00, 0xfe, 0xfc, 0x5c, 0xd0}; - class TestBuffer : public webrtc::I420Buffer { public: TestBuffer(rtc::Event* event, int width, int height) @@ -965,10 +960,9 @@ class VideoStreamEncoderTest : public ::testing::Test { FakeEncoder::Encode(input_image, &frame_type); } - void InjectEncodedImage(const EncodedImage& image, - const CodecSpecificInfo* codec_specific_info) { + void InjectEncodedImage(const EncodedImage& image) { MutexLock lock(&local_mutex_); - encoded_image_callback_->OnEncodedImage(image, codec_specific_info); + encoded_image_callback_->OnEncodedImage(image, nullptr); } void SetEncodedImageData( @@ -1254,11 +1248,6 @@ class VideoStreamEncoderTest : public ::testing::Test { return last_capture_time_ms_; } - const EncodedImage& GetLastEncodedImage() { - MutexLock lock(&mutex_); - return last_encoded_image_; - } - std::vector GetLastEncodedImageData() { MutexLock lock(&mutex_); return std::move(last_encoded_image_data_); @@ -1290,7 +1279,6 @@ class VideoStreamEncoderTest : public ::testing::Test { const CodecSpecificInfo* codec_specific_info) override { MutexLock lock(&mutex_); EXPECT_TRUE(expect_frames_); - last_encoded_image_ = EncodedImage(encoded_image); last_encoded_image_data_ = std::vector( encoded_image.data(), encoded_image.data() + encoded_image.size()); uint32_t timestamp = encoded_image.Timestamp(); @@ -1349,7 +1337,6 @@ class VideoStreamEncoderTest : public ::testing::Test { mutable Mutex mutex_; TestEncoder* test_encoder_; rtc::Event encoded_frame_event_; - EncodedImage last_encoded_image_; std::vector last_encoded_image_data_; uint32_t last_timestamp_ = 0; int64_t last_capture_time_ms_ = 0; @@ -7148,12 +7135,14 @@ TEST_F(VideoStreamEncoderTest, AdjustsTimestampInternalSource) { int64_t timestamp = 1; EncodedImage image; + image.SetEncodedData( + EncodedImageBuffer::Create(kTargetBitrateBps / kDefaultFramerate / 8)); image.capture_time_ms_ = ++timestamp; image.SetTimestamp(static_cast(timestamp * 90)); const int64_t kEncodeFinishDelayMs = 10; image.timing_.encode_start_ms = timestamp; image.timing_.encode_finish_ms = timestamp + kEncodeFinishDelayMs; - fake_encoder_.InjectEncodedImage(image, /*codec_specific_info=*/nullptr); + fake_encoder_.InjectEncodedImage(image); // Wait for frame without incrementing clock. EXPECT_TRUE(sink_.WaitForFrame(kDefaultTimeoutMs)); // Frame is captured kEncodeFinishDelayMs before it's encoded, so restored @@ -7933,63 +7922,4 @@ TEST_F(VideoStreamEncoderTest, EncoderResolutionsExposedInSimulcast) { video_stream_encoder_->Stop(); } -TEST_F(VideoStreamEncoderTest, QpPresent_QpKept) { - // Enable encoder source to force encoder reconfig. - encoder_factory_.SetHasInternalSource(true); - ResetEncoder("VP8", 1, 1, 1, false); - - // Set QP on encoded frame and pass the frame to encode complete callback. - // Since QP is present QP parsing won't be triggered and the original value - // should be kept. - EncodedImage encoded_image; - encoded_image.qp_ = 123; - encoded_image.SetEncodedData(EncodedImageBuffer::Create( - kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25))); - CodecSpecificInfo codec_info; - codec_info.codecType = kVideoCodecVP8; - fake_encoder_.InjectEncodedImage(encoded_image, &codec_info); - EXPECT_TRUE(sink_.WaitForFrame(kDefaultTimeoutMs)); - EXPECT_EQ(sink_.GetLastEncodedImage().qp_, 123); - video_stream_encoder_->Stop(); -} - -TEST_F(VideoStreamEncoderTest, QpAbsent_QpParsed) { - // Enable encoder source to force encoder reconfig. - encoder_factory_.SetHasInternalSource(true); - ResetEncoder("VP8", 1, 1, 1, false); - - // Pass an encoded frame without QP to encode complete callback. QP should be - // parsed and set. - EncodedImage encoded_image; - encoded_image.qp_ = -1; - encoded_image.SetEncodedData(EncodedImageBuffer::Create( - kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25))); - CodecSpecificInfo codec_info; - codec_info.codecType = kVideoCodecVP8; - fake_encoder_.InjectEncodedImage(encoded_image, &codec_info); - EXPECT_TRUE(sink_.WaitForFrame(kDefaultTimeoutMs)); - EXPECT_EQ(sink_.GetLastEncodedImage().qp_, 25); - video_stream_encoder_->Stop(); -} - -TEST_F(VideoStreamEncoderTest, QpAbsentParsingDisabled_QpAbsent) { - webrtc::test::ScopedFieldTrials field_trials( - "WebRTC-QpParsingKillSwitch/Enabled/"); - - // Enable encoder source to force encoder reconfig. - encoder_factory_.SetHasInternalSource(true); - ResetEncoder("VP8", 1, 1, 1, false); - - EncodedImage encoded_image; - encoded_image.qp_ = -1; - encoded_image.SetEncodedData(EncodedImageBuffer::Create( - kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25))); - CodecSpecificInfo codec_info; - codec_info.codecType = kVideoCodecVP8; - fake_encoder_.InjectEncodedImage(encoded_image, &codec_info); - EXPECT_TRUE(sink_.WaitForFrame(kDefaultTimeoutMs)); - EXPECT_EQ(sink_.GetLastEncodedImage().qp_, -1); - video_stream_encoder_->Stop(); -} - } // namespace webrtc