From 3ea160881600eca4277538d51ca8d7df7f148fc6 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Sun, 30 Oct 2022 00:49:31 +0200 Subject: [PATCH] [PCLF] Improve error handling and test coverage for AnalyzingVideoSink Bug: b/240540204 Change-Id: If60ade3dce760e8e730cbde2b199d407461b16ce Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281080 Reviewed-by: Andrey Logvin Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#38506} --- .../analyzer/video/analyzing_video_sink.cc | 73 ++++---- .../e2e/analyzer/video/analyzing_video_sink.h | 12 +- .../video/analyzing_video_sink_test.cc | 157 +++++++++++++++++- 3 files changed, 208 insertions(+), 34 deletions(-) diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink.cc b/test/pc/e2e/analyzer/video/analyzing_video_sink.cc index 9534c38a11..23af426c72 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink.cc +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink.cc @@ -30,7 +30,6 @@ namespace webrtc { namespace webrtc_pc_e2e { -namespace { using VideoSubscription = ::webrtc::webrtc_pc_e2e:: PeerConnectionE2EQualityTestFixture::VideoSubscription; @@ -39,35 +38,6 @@ using VideoResolution = ::webrtc::webrtc_pc_e2e:: using VideoConfig = ::webrtc::webrtc_pc_e2e::PeerConnectionE2EQualityTestFixture::VideoConfig; -// Scales video frame to `required_resolution` if necessary. Crashes if video -// frame and `required_resolution` have different aspect ratio. -VideoFrame ScaleVideoFrame(const VideoFrame& frame, - VideoResolution required_resolution) { - if (required_resolution.width() == static_cast(frame.width()) && - required_resolution.height() == static_cast(frame.height())) { - return frame; - } - - RTC_CHECK_LE(std::abs(static_cast(required_resolution.width()) / - required_resolution.height() - - static_cast(frame.width()) / frame.height()), - 2 * std::numeric_limits::epsilon()) - << "Received frame has different aspect ratio compared to requested " - << "video resolution: required resolution=" - << required_resolution.ToString() - << "; actual resolution=" << frame.width() << "x" << frame.height(); - - rtc::scoped_refptr scaled_buffer(I420Buffer::Create( - required_resolution.width(), required_resolution.height())); - scaled_buffer->ScaleFrom(*frame.video_frame_buffer()->ToI420()); - - VideoFrame scaled_frame = frame; - scaled_frame.set_video_frame_buffer(scaled_buffer); - return scaled_frame; -} - -} // namespace - AnalyzingVideoSink::AnalyzingVideoSink(absl::string_view peer_name, Clock* clock, VideoQualityAnalyzerInterface& analyzer, @@ -93,6 +63,13 @@ void AnalyzingVideoSink::UpdateSubscription( subscription_.GetResolutionForPeer(it->second.sender_peer_name); if (!new_requested_resolution.has_value() || (*new_requested_resolution != it->second.resolution)) { + RTC_LOG(LS_INFO) << peer_name_ << ": Subscribed resolution for stream " + << it->first << " from " << it->second.sender_peer_name + << " was updated from " + << it->second.resolution.ToString() << " to " + << new_requested_resolution->ToString() + << ". Repopulating all video sinks and recreating " + << "requested video writers"; writers_to_close.insert(it->second.video_frame_writer); it = stream_sinks_.erase(it); } else { @@ -127,6 +104,36 @@ void AnalyzingVideoSink::OnFrame(const VideoFrame& frame) { } } +VideoFrame AnalyzingVideoSink::ScaleVideoFrame( + const VideoFrame& frame, + const VideoResolution& required_resolution) { + if (required_resolution.width() == static_cast(frame.width()) && + required_resolution.height() == static_cast(frame.height())) { + return frame; + } + + // We allow some difference in the aspect ration because when decoder + // downscales video stream it may round up some dimensions to make them even, + // ex: 960x540 -> 480x270 -> 240x136 instead of 240x135. + RTC_CHECK_LE(std::abs(static_cast(required_resolution.width()) / + required_resolution.height() - + static_cast(frame.width()) / frame.height()), + 0.1) + << peer_name_ + << ": Received frame has too different aspect ratio compared to " + << "requested video resolution: required resolution=" + << required_resolution.ToString() + << "; actual resolution=" << frame.width() << "x" << frame.height(); + + rtc::scoped_refptr scaled_buffer(I420Buffer::Create( + required_resolution.width(), required_resolution.height())); + scaled_buffer->ScaleFrom(*frame.video_frame_buffer()->ToI420()); + + VideoFrame scaled_frame = frame; + scaled_frame.set_video_frame_buffer(scaled_buffer); + return scaled_frame; +} + void AnalyzingVideoSink::AnalyzeFrame(const VideoFrame& frame) { VideoFrame frame_copy = frame; frame_copy.set_video_frame_buffer( @@ -159,6 +166,12 @@ AnalyzingVideoSink::SinksDescriptor* AnalyzingVideoSink::PopulateSinks( << " for which they were not subscribed"; resolution = config.GetResolution(); } + if (!resolution->IsRegular()) { + RTC_LOG(LS_ERROR) << peer_name_ << " received stream " << stream_label + << " from " << sender_peer_name + << " for which resolution wasn't resolved"; + resolution = config.GetResolution(); + } RTC_CHECK(resolution.has_value()); diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink.h b/test/pc/e2e/analyzer/video/analyzing_video_sink.h index 0508a6b3ee..4654d10a26 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink.h +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink.h @@ -66,9 +66,15 @@ class AnalyzingVideoSink : public rtc::VideoSinkInterface { std::vector>> sinks; }; - // Creates full copy of the frame to free any frame owned internal buffers and - // passes created copy to analyzer. Uses `I420Buffer` to represent frame - // content. + // Scales video frame to `required_resolution` if necessary. Crashes if video + // frame and `required_resolution` have different aspect ratio. + VideoFrame ScaleVideoFrame( + const VideoFrame& frame, + const PeerConnectionE2EQualityTestFixture::VideoResolution& + required_resolution); + // Creates full copy of the frame to free any frame owned internal buffers + // and passes created copy to analyzer. Uses `I420Buffer` to represent + // frame content. void AnalyzeFrame(const VideoFrame& frame); // Populates sink for specified stream and caches them in `stream_sinks_`. SinksDescriptor* PopulateSinks(absl::string_view stream_label); diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc b/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc index 8cc7aee1b5..16fc05d718 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc @@ -183,7 +183,7 @@ TEST_F(AnalyzingVideoSinkTest, VideoFramesAreDumpedCorrectly) { } TEST_F(AnalyzingVideoSinkTest, - FallbackOnConfigResolutionIfNoSucscriptionProvided) { + FallbackOnConfigResolutionIfNoSubscriptionProvided) { VideoSubscription subscription; VideoConfig video_config("alice_video", /*width=*/320, /*height=*/240, /*fps=*/30); @@ -225,6 +225,51 @@ TEST_F(AnalyzingVideoSinkTest, ExpectOutputFilesCount(1); } +TEST_F(AnalyzingVideoSinkTest, + FallbackOnConfigResolutionIfNoSubscriptionIsNotResolved) { + VideoSubscription subscription; + subscription.SubscribeToAllPeers( + VideoResolution(VideoResolution::Spec::kMaxFromSender)); + VideoConfig video_config("alice_video", /*width=*/320, /*height=*/240, + /*fps=*/30); + video_config.output_dump_options = VideoDumpOptions(test_directory_); + + ExampleVideoQualityAnalyzer analyzer; + std::unique_ptr frame_generator = + CreateFrameGenerator(/*width=*/320, /*height=*/240); + VideoFrame frame = CreateFrame(*frame_generator); + frame.set_id(analyzer.OnFrameCaptured("alice", "alice_video", frame)); + + { + // `helper` and `sink` has to be destroyed so all frames will be written + // to the disk. + AnalyzingVideoSinksHelper helper; + helper.AddConfig("alice", video_config); + AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, + subscription); + sink.OnFrame(frame); + } + + EXPECT_THAT(analyzer.frames_rendered(), Eq(static_cast(1))); + + test::Y4mFrameReaderImpl frame_reader( + test::JoinFilename(test_directory_, "alice_video_bob_320x240_30.y4m"), + /*width=*/320, + /*height=*/240); + ASSERT_TRUE(frame_reader.Init()); + EXPECT_THAT(frame_reader.NumberOfFrames(), Eq(1)); + rtc::scoped_refptr actual_frame = frame_reader.ReadFrame(); + rtc::scoped_refptr expected_frame = + frame.video_frame_buffer()->ToI420(); + double psnr = I420PSNR(*expected_frame, *actual_frame); + double ssim = I420SSIM(*expected_frame, *actual_frame); + // Frames should be equal. + EXPECT_DOUBLE_EQ(ssim, 1.00); + EXPECT_DOUBLE_EQ(psnr, 48); + + ExpectOutputFilesCount(1); +} + TEST_F(AnalyzingVideoSinkTest, VideoFramesAreDumpedCorrectlyWhenSubscriptionChanged) { VideoSubscription subscription_before; @@ -298,6 +343,116 @@ TEST_F(AnalyzingVideoSinkTest, ExpectOutputFilesCount(2); } +TEST_F(AnalyzingVideoSinkTest, + VideoFramesAreDumpedCorrectlyWhenSubscriptionChangedOnTheSameOne) { + VideoSubscription subscription_before; + subscription_before.SubscribeToPeer( + "alice", VideoResolution(/*width=*/640, /*height=*/360, /*fps=*/30)); + VideoSubscription subscription_after; + subscription_after.SubscribeToPeer( + "alice", VideoResolution(/*width=*/640, /*height=*/360, /*fps=*/30)); + VideoConfig video_config("alice_video", /*width=*/640, /*height=*/360, + /*fps=*/30); + video_config.output_dump_options = VideoDumpOptions(test_directory_); + + ExampleVideoQualityAnalyzer analyzer; + std::unique_ptr frame_generator = + CreateFrameGenerator(/*width=*/640, /*height=*/360); + VideoFrame frame_before = CreateFrame(*frame_generator); + frame_before.set_id( + analyzer.OnFrameCaptured("alice", "alice_video", frame_before)); + VideoFrame frame_after = CreateFrame(*frame_generator); + frame_after.set_id( + analyzer.OnFrameCaptured("alice", "alice_video", frame_after)); + + { + // `helper` and `sink` has to be destroyed so all frames will be written + // to the disk. + AnalyzingVideoSinksHelper helper; + helper.AddConfig("alice", video_config); + AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, + subscription_before); + sink.OnFrame(frame_before); + + sink.UpdateSubscription(subscription_after); + sink.OnFrame(frame_after); + } + + EXPECT_THAT(analyzer.frames_rendered(), Eq(static_cast(2))); + + { + test::Y4mFrameReaderImpl frame_reader( + test::JoinFilename(test_directory_, "alice_video_bob_640x360_30.y4m"), + /*width=*/640, + /*height=*/360); + ASSERT_TRUE(frame_reader.Init()); + EXPECT_THAT(frame_reader.NumberOfFrames(), Eq(2)); + // Read the first frame. + rtc::scoped_refptr actual_frame = frame_reader.ReadFrame(); + rtc::scoped_refptr expected_frame = + frame_before.video_frame_buffer()->ToI420(); + // Frames should be equal. + EXPECT_DOUBLE_EQ(I420SSIM(*expected_frame, *actual_frame), 1.00); + EXPECT_DOUBLE_EQ(I420PSNR(*expected_frame, *actual_frame), 48); + // Read the second frame. + actual_frame = frame_reader.ReadFrame(); + expected_frame = frame_after.video_frame_buffer()->ToI420(); + // Frames should be equal. + EXPECT_DOUBLE_EQ(I420SSIM(*expected_frame, *actual_frame), 1.00); + EXPECT_DOUBLE_EQ(I420PSNR(*expected_frame, *actual_frame), 48); + } + + ExpectOutputFilesCount(1); +} + +TEST_F(AnalyzingVideoSinkTest, SmallDiviationsInAspectRationAreAllowed) { + VideoSubscription subscription; + subscription.SubscribeToPeer( + "alice", VideoResolution(/*width=*/480, /*height=*/270, /*fps=*/30)); + VideoConfig video_config("alice_video", /*width=*/480, /*height=*/270, + /*fps=*/30); + video_config.output_dump_options = VideoDumpOptions(test_directory_); + + ExampleVideoQualityAnalyzer analyzer; + // Generator produces downscaled frames with a bit different aspect ration. + std::unique_ptr frame_generator = + CreateFrameGenerator(/*width=*/240, /*height=*/136); + VideoFrame frame = CreateFrame(*frame_generator); + frame.set_id(analyzer.OnFrameCaptured("alice", "alice_video", frame)); + + { + // `helper` and `sink` has to be destroyed so all frames will be written + // to the disk. + AnalyzingVideoSinksHelper helper; + helper.AddConfig("alice", video_config); + AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, + subscription); + sink.OnFrame(frame); + } + + EXPECT_THAT(analyzer.frames_rendered(), Eq(static_cast(1))); + + { + test::Y4mFrameReaderImpl frame_reader( + test::JoinFilename(test_directory_, "alice_video_bob_480x270_30.y4m"), + /*width=*/480, + /*height=*/270); + ASSERT_TRUE(frame_reader.Init()); + EXPECT_THAT(frame_reader.NumberOfFrames(), Eq(1)); + // Read the first frame. + rtc::scoped_refptr actual_frame = frame_reader.ReadFrame(); + rtc::scoped_refptr expected_frame = + frame.video_frame_buffer()->ToI420(); + // Actual frame is upscaled version of the expected. But because rendered + // resolution is equal to the actual frame size we need to upscale expected + // during comparison and then they have to be the same. + EXPECT_DOUBLE_EQ(I420SSIM(*actual_frame, *expected_frame), 1); + EXPECT_DOUBLE_EQ(I420PSNR(*actual_frame, *expected_frame), 48); + } + + ExpectOutputFilesCount(1); +} + TEST_F(AnalyzingVideoSinkTest, VideoFramesIdsAreDumpedWhenRequested) { VideoSubscription subscription; subscription.SubscribeToPeer(