Reland "Copy video frames metadata between encoded and plain frames in one place"
Reland with fixes.
Currently some video frames metadata like rotation or ntp timestamps are
copied in every encoder and decoder separately. This CL makes copying to
happen at a single place for send or receive side. This will make it
easier to add new metadata in the future.
Also, added some missing tests.
Original Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/133346
Bug: webrtc:10460
Change-Id: I98629589fa55ca1d74056033cf86faccfdf848cd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/136582
Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27930}
diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn
index baa78f4..a6fa96c 100644
--- a/modules/video_coding/BUILD.gn
+++ b/modules/video_coding/BUILD.gn
@@ -781,6 +781,7 @@
"decoding_state_unittest.cc",
"fec_controller_unittest.cc",
"frame_buffer2_unittest.cc",
+ "generic_decoder_unittest.cc",
"h264_sprop_parameter_sets_unittest.cc",
"h264_sps_pps_tracker_unittest.cc",
"histogram_unittest.cc",
@@ -840,6 +841,7 @@
"../../api:scoped_refptr",
"../../api:simulcast_test_fixture_api",
"../../api:videocodec_test_fixture_api",
+ "../../api/task_queue:default_task_queue_factory",
"../../api/test/video:function_video_factory",
"../../api/video:builtin_video_bitrate_allocator_factory",
"../../api/video:video_bitrate_allocation",
@@ -850,6 +852,7 @@
"../../api/video_codecs:video_codecs_api",
"../../api/video_codecs:vp8_temporal_layers_factory",
"../../common_video",
+ "../../common_video/test:utilities",
"../../media:rtc_media_base",
"../../rtc_base",
"../../rtc_base:checks",
@@ -863,6 +866,7 @@
"../../system_wrappers:event_wrapper",
"../../system_wrappers:field_trial",
"../../system_wrappers:metrics",
+ "../../test:fake_video_codecs",
"../../test:field_trial",
"../../test:fileutils",
"../../test:test_common",
diff --git a/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc b/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
index 3654ed5..4af0ebb 100644
--- a/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
+++ b/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
@@ -19,7 +19,6 @@
#include "api/video_codecs/video_decoder.h"
#include "api/video_codecs/video_encoder.h"
#include "common_video/libyuv/include/webrtc_libyuv.h"
-#include "common_video/test/utilities.h"
#include "media/base/codec.h"
#include "media/base/media_constants.h"
#include "modules/video_coding/codecs/h264/include/h264.h"
@@ -49,17 +48,9 @@
#ifdef WEBRTC_USE_H264
#define MAYBE_EncodeDecode EncodeDecode
#define MAYBE_DecodedQpEqualsEncodedQp DecodedQpEqualsEncodedQp
-#define MAYBE_EncodedColorSpaceEqualsInputColorSpace \
- EncodedColorSpaceEqualsInputColorSpace
-#define MAYBE_DecodedColorSpaceEqualsEncodedColorSpace \
- DecodedColorSpaceEqualsEncodedColorSpace
#else
#define MAYBE_EncodeDecode DISABLED_EncodeDecode
#define MAYBE_DecodedQpEqualsEncodedQp DISABLED_DecodedQpEqualsEncodedQp
-#define MAYBE_EncodedColorSpaceEqualsInputColorSpace \
- DISABLED_EncodedColorSpaceEqualsInputColorSpace
-#define MAYBE_DecodedColorSpaceEqualsEncodedColorSpace \
- DISABLED_DecodedColorSpaceEqualsEncodedColorSpace
#endif
TEST_F(TestH264Impl, MAYBE_EncodeDecode) {
@@ -105,45 +96,4 @@
EXPECT_EQ(encoded_frame.qp_, *decoded_qp);
}
-TEST_F(TestH264Impl, MAYBE_EncodedColorSpaceEqualsInputColorSpace) {
- VideoFrame* input_frame = NextInputFrame();
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Encode(*input_frame, nullptr));
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- EXPECT_FALSE(encoded_frame.ColorSpace());
-
- // Video frame with explicit color space information.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- VideoFrame input_frame_w_color_space =
- VideoFrame::Builder()
- .set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(color_space)
- .build();
-
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
- encoder_->Encode(input_frame_w_color_space, nullptr));
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- ASSERT_TRUE(encoded_frame.ColorSpace());
- EXPECT_EQ(*encoded_frame.ColorSpace(), color_space);
-}
-
-TEST_F(TestH264Impl, MAYBE_DecodedColorSpaceEqualsEncodedColorSpace) {
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
- encoder_->Encode(*NextInputFrame(), nullptr));
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- // Add color space to encoded frame.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- encoded_frame.SetColorSpace(color_space);
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Decode(encoded_frame, false, 0));
- std::unique_ptr<VideoFrame> decoded_frame;
- absl::optional<uint8_t> decoded_qp;
- ASSERT_TRUE(WaitForDecodedFrame(&decoded_frame, &decoded_qp));
- ASSERT_TRUE(decoded_frame);
- ASSERT_TRUE(decoded_frame->color_space());
- EXPECT_EQ(color_space, *decoded_frame->color_space());
-}
-
} // namespace webrtc
diff --git a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
index a2597ef..eb6b89f 100644
--- a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
+++ b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
@@ -227,51 +227,10 @@
EncodeAndWaitForFrame(*input_frame, &encoded_frame, &codec_specific_info);
EXPECT_EQ(kInitialTimestampRtp, encoded_frame.Timestamp());
- EXPECT_EQ(kInitialTimestampMs, encoded_frame.capture_time_ms_);
EXPECT_EQ(kWidth, static_cast<int>(encoded_frame._encodedWidth));
EXPECT_EQ(kHeight, static_cast<int>(encoded_frame._encodedHeight));
}
-// We only test the encoder here, since the decoded frame rotation is set based
-// on the CVO RTP header extension in VCMDecodedFrameCallback::Decoded.
-// TODO(brandtr): Consider passing through the rotation flag through the decoder
-// in the same way as done in the encoder.
-TEST_F(TestVp8Impl, EncodedRotationEqualsInputRotation) {
- VideoFrame* input_frame = NextInputFrame();
- input_frame->set_rotation(kVideoRotation_0);
-
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- EncodeAndWaitForFrame(*input_frame, &encoded_frame, &codec_specific_info);
- EXPECT_EQ(kVideoRotation_0, encoded_frame.rotation_);
-
- input_frame->set_rotation(kVideoRotation_90);
- EncodeAndWaitForFrame(*input_frame, &encoded_frame, &codec_specific_info);
- EXPECT_EQ(kVideoRotation_90, encoded_frame.rotation_);
-}
-
-TEST_F(TestVp8Impl, EncodedColorSpaceEqualsInputColorSpace) {
- // Video frame without explicit color space information.
- VideoFrame* input_frame = NextInputFrame();
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- EncodeAndWaitForFrame(*input_frame, &encoded_frame, &codec_specific_info);
- EXPECT_FALSE(encoded_frame.ColorSpace());
-
- // Video frame with explicit color space information.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- VideoFrame input_frame_w_color_space =
- VideoFrame::Builder()
- .set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(color_space)
- .build();
-
- EncodeAndWaitForFrame(input_frame_w_color_space, &encoded_frame,
- &codec_specific_info);
- ASSERT_TRUE(encoded_frame.ColorSpace());
- EXPECT_EQ(*encoded_frame.ColorSpace(), color_space);
-}
-
TEST_F(TestVp8Impl, DecodedQpEqualsEncodedQp) {
VideoFrame* input_frame = NextInputFrame();
EncodedImage encoded_frame;
@@ -290,24 +249,6 @@
EXPECT_EQ(encoded_frame.qp_, *decoded_qp);
}
-TEST_F(TestVp8Impl, DecodedColorSpaceEqualsEncodedColorSpace) {
- VideoFrame* input_frame = NextInputFrame();
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- EncodeAndWaitForFrame(*input_frame, &encoded_frame, &codec_specific_info);
-
- // Encoded frame with explicit color space information.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- encoded_frame.SetColorSpace(color_space);
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Decode(encoded_frame, false, -1));
- std::unique_ptr<VideoFrame> decoded_frame;
- absl::optional<uint8_t> decoded_qp;
- ASSERT_TRUE(WaitForDecodedFrame(&decoded_frame, &decoded_qp));
- ASSERT_TRUE(decoded_frame);
- ASSERT_TRUE(decoded_frame->color_space());
- EXPECT_EQ(color_space, *decoded_frame->color_space());
-}
-
TEST_F(TestVp8Impl, ChecksSimulcastSettings) {
codec_settings_.numberOfSimulcastStreams = 2;
// Resolutions are not in ascending order, temporal layers do not match.
@@ -402,7 +343,6 @@
// Compute PSNR on all planes (faster than SSIM).
EXPECT_GT(I420PSNR(input_frame, decoded_frame.get()), 36);
EXPECT_EQ(kInitialTimestampRtp, decoded_frame->timestamp());
- EXPECT_EQ(kTestNtpTimeMs, decoded_frame->ntp_time_ms());
}
#if defined(WEBRTC_ANDROID)
diff --git a/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc b/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
index 95ba266..309dac1 100644
--- a/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
+++ b/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
@@ -11,7 +11,6 @@
#include "api/video/color_space.h"
#include "api/video/i420_buffer.h"
#include "common_video/libyuv/include/webrtc_libyuv.h"
-#include "common_video/test/utilities.h"
#include "media/base/vp9_profile.h"
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "modules/video_coding/codecs/test/video_codec_unittest.h"
@@ -146,50 +145,7 @@
color_space.chroma_siting_vertical());
}
-// We only test the encoder here, since the decoded frame rotation is set based
-// on the CVO RTP header extension in VCMDecodedFrameCallback::Decoded.
-// TODO(brandtr): Consider passing through the rotation flag through the decoder
-// in the same way as done in the encoder.
-TEST_F(TestVp9Impl, EncodedRotationEqualsInputRotation) {
- VideoFrame* input_frame = NextInputFrame();
- input_frame->set_rotation(kVideoRotation_0);
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Encode(*input_frame, nullptr));
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- EXPECT_EQ(kVideoRotation_0, encoded_frame.rotation_);
-
- input_frame = NextInputFrame();
- input_frame->set_rotation(kVideoRotation_90);
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Encode(*input_frame, nullptr));
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- EXPECT_EQ(kVideoRotation_90, encoded_frame.rotation_);
-}
-
-TEST_F(TestVp9Impl, EncodedColorSpaceEqualsInputColorSpace) {
- // Video frame without explicit color space information.
- VideoFrame* input_frame = NextInputFrame();
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Encode(*input_frame, nullptr));
- EncodedImage encoded_frame;
- CodecSpecificInfo codec_specific_info;
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- EXPECT_FALSE(encoded_frame.ColorSpace());
-
- // Video frame with explicit color space information.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/true);
- VideoFrame input_frame_w_hdr =
- VideoFrame::Builder()
- .set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(color_space)
- .build();
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
- encoder_->Encode(input_frame_w_hdr, nullptr));
- ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
- ASSERT_TRUE(encoded_frame.ColorSpace());
- EXPECT_EQ(*encoded_frame.ColorSpace(), color_space);
-}
-
-TEST_F(TestVp9Impl, DecodedColorSpaceEqualsEncodedColorSpace) {
+TEST_F(TestVp9Impl, DecodedColorSpaceFromBitstream) {
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
encoder_->Encode(*NextInputFrame(), nullptr));
EncodedImage encoded_frame;
@@ -206,15 +162,6 @@
ASSERT_TRUE(decoded_frame->color_space());
// No HDR metadata present.
EXPECT_FALSE(decoded_frame->color_space()->hdr_metadata());
-
- // Encoded frame with explicit color space information.
- ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/true);
- encoded_frame.SetColorSpace(color_space);
- EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Decode(encoded_frame, false, 0));
- ASSERT_TRUE(WaitForDecodedFrame(&decoded_frame, &decoded_qp));
- ASSERT_TRUE(decoded_frame);
- ASSERT_TRUE(decoded_frame->color_space());
- EXPECT_EQ(color_space, *decoded_frame->color_space());
}
TEST_F(TestVp9Impl, DecodedQpEqualsEncodedQp) {
diff --git a/modules/video_coding/encoded_frame.h b/modules/video_coding/encoded_frame.h
index 94da40f..a6bb55b 100644
--- a/modules/video_coding/encoded_frame.h
+++ b/modules/video_coding/encoded_frame.h
@@ -52,8 +52,10 @@
return static_cast<const webrtc::EncodedImage&>(*this);
}
+ using EncodedImage::ColorSpace;
using EncodedImage::data;
using EncodedImage::set_size;
+ using EncodedImage::SetColorSpace;
using EncodedImage::SetSpatialIndex;
using EncodedImage::SetTimestamp;
using EncodedImage::size;
diff --git a/modules/video_coding/generic_decoder.cc b/modules/video_coding/generic_decoder.cc
index cf986d6..cb6c819 100644
--- a/modules/video_coding/generic_decoder.cc
+++ b/modules/video_coding/generic_decoder.cc
@@ -80,6 +80,12 @@
return;
}
+ decodedImage.set_ntp_time_ms(frameInfo->ntp_time_ms);
+ if (frameInfo->color_space) {
+ decodedImage.set_color_space(frameInfo->color_space);
+ }
+ decodedImage.set_rotation(frameInfo->rotation);
+
const int64_t now_ms = _clock->TimeInMilliseconds();
if (!decode_time_ms) {
decode_time_ms = now_ms - frameInfo->decodeStartTimeMs;
@@ -140,7 +146,6 @@
decodedImage.set_timestamp_us(frameInfo->renderTimeMs *
rtc::kNumMicrosecsPerMillisec);
- decodedImage.set_rotation(frameInfo->rotation);
_receiveCallback->FrameToRender(decodedImage, qp, frameInfo->content_type);
}
@@ -199,6 +204,14 @@
_frameInfos[_nextFrameInfoIdx].renderTimeMs = frame.RenderTimeMs();
_frameInfos[_nextFrameInfoIdx].rotation = frame.rotation();
_frameInfos[_nextFrameInfoIdx].timing = frame.video_timing();
+ _frameInfos[_nextFrameInfoIdx].ntp_time_ms =
+ frame.EncodedImage().ntp_time_ms_;
+ if (frame.ColorSpace()) {
+ _frameInfos[_nextFrameInfoIdx].color_space = *frame.ColorSpace();
+ } else {
+ _frameInfos[_nextFrameInfoIdx].color_space = absl::nullopt;
+ }
+
// Set correctly only for key frames. Thus, use latest key frame
// content type. If the corresponding key frame was lost, decode will fail
// and content type will be ignored.
diff --git a/modules/video_coding/generic_decoder.h b/modules/video_coding/generic_decoder.h
index 36428fb..97336b1 100644
--- a/modules/video_coding/generic_decoder.h
+++ b/modules/video_coding/generic_decoder.h
@@ -34,6 +34,8 @@
VideoRotation rotation;
VideoContentType content_type;
EncodedImage::Timing timing;
+ int64_t ntp_time_ms;
+ absl::optional<ColorSpace> color_space;
};
class VCMDecodedFrameCallback : public DecodedImageCallback {
diff --git a/modules/video_coding/generic_decoder_unittest.cc b/modules/video_coding/generic_decoder_unittest.cc
new file mode 100644
index 0000000..adc945c
--- /dev/null
+++ b/modules/video_coding/generic_decoder_unittest.cc
@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) 2016 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/generic_decoder.h"
+
+#include <vector>
+
+#include "absl/types/optional.h"
+#include "api/task_queue/default_task_queue_factory.h"
+#include "common_video/test/utilities.h"
+#include "modules/video_coding/timing.h"
+#include "rtc_base/critical_section.h"
+#include "rtc_base/event.h"
+#include "system_wrappers/include/clock.h"
+#include "test/fake_decoder.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace video_coding {
+
+class ReceiveCallback : public VCMReceiveCallback {
+ public:
+ int32_t FrameToRender(VideoFrame& videoFrame, // NOLINT
+ absl::optional<uint8_t> qp,
+ VideoContentType content_type) override {
+ {
+ rtc::CritScope cs(&lock_);
+ last_frame_ = videoFrame;
+ }
+ received_frame_event_.Set();
+ return 0;
+ }
+
+ absl::optional<VideoFrame> GetLastFrame() {
+ rtc::CritScope cs(&lock_);
+ return last_frame_;
+ }
+
+ absl::optional<VideoFrame> WaitForFrame(int64_t wait_ms) {
+ if (received_frame_event_.Wait(wait_ms)) {
+ rtc::CritScope cs(&lock_);
+ return last_frame_;
+ } else {
+ return absl::nullopt;
+ }
+ }
+
+ private:
+ rtc::CriticalSection lock_;
+ rtc::Event received_frame_event_;
+ absl::optional<VideoFrame> last_frame_ RTC_GUARDED_BY(lock_);
+};
+
+class GenericDecoderTest : public ::testing::Test {
+ protected:
+ GenericDecoderTest()
+ : clock_(0),
+ timing_(&clock_),
+ task_queue_factory_(CreateDefaultTaskQueueFactory()),
+ decoder_(task_queue_factory_.get()),
+ vcm_callback_(&timing_, &clock_),
+ generic_decoder_(&decoder_, /*isExternal=*/true) {}
+
+ void SetUp() override {
+ generic_decoder_.RegisterDecodeCompleteCallback(&vcm_callback_);
+ vcm_callback_.SetUserReceiveCallback(&user_callback_);
+ VideoCodec settings;
+ settings.codecType = kVideoCodecVP8;
+ settings.width = 10;
+ settings.height = 10;
+ generic_decoder_.InitDecode(&settings, /*numberOfCores=*/4);
+ }
+
+ SimulatedClock clock_;
+ VCMTiming timing_;
+ std::unique_ptr<TaskQueueFactory> task_queue_factory_;
+ webrtc::test::FakeDecoder decoder_;
+ VCMDecodedFrameCallback vcm_callback_;
+ VCMGenericDecoder generic_decoder_;
+ ReceiveCallback user_callback_;
+};
+
+TEST_F(GenericDecoderTest, PassesColorSpace) {
+ webrtc::ColorSpace color_space =
+ CreateTestColorSpace(/*with_hdr_metadata=*/true);
+ VCMEncodedFrame encoded_frame;
+ encoded_frame.SetColorSpace(color_space);
+ generic_decoder_.Decode(encoded_frame, clock_.TimeInMilliseconds());
+ absl::optional<VideoFrame> decoded_frame = user_callback_.WaitForFrame(10);
+ ASSERT_TRUE(decoded_frame.has_value());
+ absl::optional<webrtc::ColorSpace> decoded_color_space =
+ decoded_frame->color_space();
+ ASSERT_TRUE(decoded_color_space.has_value());
+ EXPECT_EQ(*decoded_color_space, color_space);
+}
+
+TEST_F(GenericDecoderTest, PassesColorSpaceForDelayedDecoders) {
+ webrtc::ColorSpace color_space =
+ CreateTestColorSpace(/*with_hdr_metadata=*/true);
+ decoder_.SetDelayedDecoding(100);
+
+ {
+ // Ensure the original frame is destroyed before the decoding is completed.
+ VCMEncodedFrame encoded_frame;
+ encoded_frame.SetColorSpace(color_space);
+ generic_decoder_.Decode(encoded_frame, clock_.TimeInMilliseconds());
+ }
+
+ absl::optional<VideoFrame> decoded_frame = user_callback_.WaitForFrame(200);
+ ASSERT_TRUE(decoded_frame.has_value());
+ absl::optional<webrtc::ColorSpace> decoded_color_space =
+ decoded_frame->color_space();
+ ASSERT_TRUE(decoded_color_space.has_value());
+ EXPECT_EQ(*decoded_color_space, color_space);
+}
+
+} // namespace video_coding
+} // namespace webrtc