From a0e2609a08d0771d4dde6d5a94b06b71f0cdf53b Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Fri, 12 Jul 2019 11:20:47 +0200 Subject: [PATCH] Partially revert of ColorSpace information copying around decoders This partially reverts these 2 CLs: 1) Reland "Copy video frames metadata between encoded and plain frames in one place" https://webrtc.googlesource.com/src/+/2ebf5239782bf6b46d4aa812f34fa9f9e5a02be9 2) Don't copy video frame metadata in each encoder/decoder https://webrtc.googlesource.com/src/+/ab62b2ee51e622be6d0aade15e87e927fa60e6f2 The problem with them were that ColorSpace was made to always be copied from the EncodedImage in the GenericDecoder, which overwrote ColorSpace information from the decoder. If decoder applied color space transition or bitstream color space information was different from the WebRTC signaled one, the incorrect color space data were passed to the renderer. This CL removes introduced change regarding color space data: GenericDecoder doesn't copy or store it and software decoders are restored to copy it. Relevant tests are also removed. Bug: chromium:982486 Change-Id: I989e01476ff7f7df376c05578ab8f540b95a1dd2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145323 Reviewed-by: Philip Eliasson Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#28556} --- .../codecs/vp8/libvpx_vp8_decoder.cc | 11 +++--- .../codecs/vp8/libvpx_vp8_decoder.h | 5 ++- modules/video_coding/codecs/vp9/vp9_impl.cc | 22 +++++++----- modules/video_coding/codecs/vp9/vp9_impl.h | 5 ++- modules/video_coding/generic_decoder.cc | 8 ----- modules/video_coding/generic_decoder.h | 2 +- .../video_coding/generic_decoder_unittest.cc | 34 ------------------- video/video_receive_stream_unittest.cc | 16 --------- 8 files changed, 30 insertions(+), 73 deletions(-) diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc index 6f6d026cf2..6983c5575f 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc @@ -267,7 +267,7 @@ int LibvpxVp8Decoder::Decode(const EncodedImage& input_image, vpx_codec_err_t vpx_ret = vpx_codec_control(decoder_, VPXD_GET_LAST_QUANTIZER, &qp); RTC_DCHECK_EQ(vpx_ret, VPX_CODEC_OK); - ret = ReturnFrame(img, input_image.Timestamp(), qp); + ret = ReturnFrame(img, input_image.Timestamp(), qp, input_image.ColorSpace()); if (ret != 0) { // Reset to avoid requesting key frames too often. if (ret < 0 && propagation_cnt_ > 0) @@ -283,9 +283,11 @@ int LibvpxVp8Decoder::Decode(const EncodedImage& input_image, return WEBRTC_VIDEO_CODEC_OK; } -int LibvpxVp8Decoder::ReturnFrame(const vpx_image_t* img, - uint32_t timestamp, - int qp) { +int LibvpxVp8Decoder::ReturnFrame( + const vpx_image_t* img, + uint32_t timestamp, + int qp, + const webrtc::ColorSpace* explicit_color_space) { if (img == NULL) { // Decoder OK and NULL image => No show frame return WEBRTC_VIDEO_CODEC_NO_OUTPUT; @@ -320,6 +322,7 @@ int LibvpxVp8Decoder::ReturnFrame(const vpx_image_t* img, VideoFrame decoded_image = VideoFrame::Builder() .set_video_frame_buffer(buffer) .set_timestamp_rtp(timestamp) + .set_color_space(explicit_color_space) .build(); decode_complete_callback_->Decoded(decoded_image, absl::nullopt, qp); diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h index 47d54dc33b..abe87c33cc 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h @@ -48,7 +48,10 @@ class LibvpxVp8Decoder : public VideoDecoder { private: class QpSmoother; - int ReturnFrame(const vpx_image_t* img, uint32_t timeStamp, int qp); + int ReturnFrame(const vpx_image_t* img, + uint32_t timeStamp, + int qp, + const webrtc::ColorSpace* explicit_color_space); const bool use_postproc_arm_; I420BufferPool buffer_pool_; diff --git a/modules/video_coding/codecs/vp9/vp9_impl.cc b/modules/video_coding/codecs/vp9/vp9_impl.cc index 852341caac..9f46ade344 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.cc +++ b/modules/video_coding/codecs/vp9/vp9_impl.cc @@ -1664,16 +1664,19 @@ int VP9DecoderImpl::Decode(const EncodedImage& input_image, vpx_codec_err_t vpx_ret = vpx_codec_control(decoder_, VPXD_GET_LAST_QUANTIZER, &qp); RTC_DCHECK_EQ(vpx_ret, VPX_CODEC_OK); - int ret = ReturnFrame(img, input_image.Timestamp(), qp); + int ret = + ReturnFrame(img, input_image.Timestamp(), qp, input_image.ColorSpace()); if (ret != 0) { return ret; } return WEBRTC_VIDEO_CODEC_OK; } -int VP9DecoderImpl::ReturnFrame(const vpx_image_t* img, - uint32_t timestamp, - int qp) { +int VP9DecoderImpl::ReturnFrame( + const vpx_image_t* img, + uint32_t timestamp, + int qp, + const webrtc::ColorSpace* explicit_color_space) { if (img == nullptr) { // Decoder OK and nullptr image => No show frame. return WEBRTC_VIDEO_CODEC_NO_OUTPUT; @@ -1717,10 +1720,13 @@ int VP9DecoderImpl::ReturnFrame(const vpx_image_t* img, auto builder = VideoFrame::Builder() .set_video_frame_buffer(img_wrapped_buffer) - .set_timestamp_rtp(timestamp) - .set_color_space(ExtractVP9ColorSpace(img->cs, img->range, - img->bit_depth)); - + .set_timestamp_rtp(timestamp); + if (explicit_color_space) { + builder.set_color_space(*explicit_color_space); + } else { + builder.set_color_space( + ExtractVP9ColorSpace(img->cs, img->range, img->bit_depth)); + } VideoFrame decoded_image = builder.build(); decode_complete_callback_->Decoded(decoded_image, absl::nullopt, qp); diff --git a/modules/video_coding/codecs/vp9/vp9_impl.h b/modules/video_coding/codecs/vp9/vp9_impl.h index e128d1849e..f0dac27761 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.h +++ b/modules/video_coding/codecs/vp9/vp9_impl.h @@ -196,7 +196,10 @@ class VP9DecoderImpl : public VP9Decoder { const char* ImplementationName() const override; private: - int ReturnFrame(const vpx_image_t* img, uint32_t timestamp, int qp); + int ReturnFrame(const vpx_image_t* img, + uint32_t timestamp, + int qp, + const webrtc::ColorSpace* explicit_color_space); // Memory pool used to share buffers between libvpx and webrtc. Vp9FrameBufferPool frame_buffer_pool_; diff --git a/modules/video_coding/generic_decoder.cc b/modules/video_coding/generic_decoder.cc index 75c5ac168b..986c36c69f 100644 --- a/modules/video_coding/generic_decoder.cc +++ b/modules/video_coding/generic_decoder.cc @@ -82,9 +82,6 @@ void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage, } decodedImage.set_ntp_time_ms(frameInfo->ntp_time_ms); - if (frameInfo->color_space) { - decodedImage.set_color_space(frameInfo->color_space); - } decodedImage.set_packet_infos(frameInfo->packet_infos); decodedImage.set_rotation(frameInfo->rotation); @@ -207,11 +204,6 @@ int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, int64_t nowMs) { _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; - } _frameInfos[_nextFrameInfoIdx].packet_infos = frame.PacketInfos(); // Set correctly only for key frames. Thus, use latest key frame diff --git a/modules/video_coding/generic_decoder.h b/modules/video_coding/generic_decoder.h index 50d7dba5bc..43ca23c036 100644 --- a/modules/video_coding/generic_decoder.h +++ b/modules/video_coding/generic_decoder.h @@ -35,8 +35,8 @@ struct VCMFrameInformation { VideoContentType content_type; EncodedImage::Timing timing; int64_t ntp_time_ms; - absl::optional color_space; RtpPacketInfos packet_infos; + // ColorSpace is not storred here, as it might be modified by decoders. }; class VCMDecodedFrameCallback : public DecodedImageCallback { diff --git a/modules/video_coding/generic_decoder_unittest.cc b/modules/video_coding/generic_decoder_unittest.cc index 93d55a67d4..66167eb610 100644 --- a/modules/video_coding/generic_decoder_unittest.cc +++ b/modules/video_coding/generic_decoder_unittest.cc @@ -89,40 +89,6 @@ class GenericDecoderTest : public ::testing::Test { 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 decoded_frame = user_callback_.WaitForFrame(10); - ASSERT_TRUE(decoded_frame.has_value()); - absl::optional 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 decoded_frame = user_callback_.WaitForFrame(200); - ASSERT_TRUE(decoded_frame.has_value()); - absl::optional decoded_color_space = - decoded_frame->color_space(); - ASSERT_TRUE(decoded_color_space.has_value()); - EXPECT_EQ(*decoded_color_space, color_space); -} - TEST_F(GenericDecoderTest, PassesPacketInfos) { RtpPacketInfos packet_infos = CreatePacketInfos(3); VCMEncodedFrame encoded_frame; diff --git a/video/video_receive_stream_unittest.cc b/video/video_receive_stream_unittest.cc index fc1bd783db..6d88f67e92 100644 --- a/video/video_receive_stream_unittest.cc +++ b/video/video_receive_stream_unittest.cc @@ -299,22 +299,6 @@ TEST_F(VideoReceiveStreamTestWithFakeDecoder, PassesRotation) { EXPECT_EQ(kRotation, fake_renderer_.rotation()); } -TEST_F(VideoReceiveStreamTestWithFakeDecoder, PassesColorSpace) { - auto test_frame = absl::make_unique(); - test_frame->SetPayloadType(99); - test_frame->id.picture_id = 0; - webrtc::ColorSpace color_space = - CreateTestColorSpace(/*with_hdr_metadata=*/true); - test_frame->SetColorSpace(color_space); - - video_receive_stream_->Start(); - video_receive_stream_->OnCompleteFrame(std::move(test_frame)); - EXPECT_TRUE(fake_renderer_.WaitForRenderedFrame(kDefaultTimeOutMs)); - - ASSERT_TRUE(fake_renderer_.color_space().has_value()); - EXPECT_EQ(color_space, *fake_renderer_.color_space()); -} - TEST_F(VideoReceiveStreamTestWithFakeDecoder, PassesPacketInfos) { auto test_frame = absl::make_unique(); test_frame->SetPayloadType(99);