[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 <landrey@google.com>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38506}
diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink.cc b/test/pc/e2e/analyzer/video/analyzing_video_sink.cc
index 9534c38..23af426 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 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<size_t>(frame.width()) &&
- required_resolution.height() == static_cast<size_t>(frame.height())) {
- return frame;
- }
-
- RTC_CHECK_LE(std::abs(static_cast<double>(required_resolution.width()) /
- required_resolution.height() -
- static_cast<double>(frame.width()) / frame.height()),
- 2 * std::numeric_limits<double>::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<I420Buffer> 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 @@
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 @@
}
}
+VideoFrame AnalyzingVideoSink::ScaleVideoFrame(
+ const VideoFrame& frame,
+ const VideoResolution& required_resolution) {
+ if (required_resolution.width() == static_cast<size_t>(frame.width()) &&
+ required_resolution.height() == static_cast<size_t>(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<double>(required_resolution.width()) /
+ required_resolution.height() -
+ static_cast<double>(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<I420Buffer> 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 @@
<< " 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 0508a6b..4654d10 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 @@
std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> 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 8cc7aee..16fc05d 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,
- FallbackOnConfigResolutionIfNoSucscriptionProvided) {
+ FallbackOnConfigResolutionIfNoSubscriptionProvided) {
VideoSubscription subscription;
VideoConfig video_config("alice_video", /*width=*/320, /*height=*/240,
/*fps=*/30);
@@ -226,6 +226,51 @@
}
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<test::FrameGeneratorInterface> 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<uint64_t>(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<I420Buffer> actual_frame = frame_reader.ReadFrame();
+ rtc::scoped_refptr<I420BufferInterface> 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;
subscription_before.SubscribeToPeer(
@@ -298,6 +343,116 @@
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<test::FrameGeneratorInterface> 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<uint64_t>(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<I420Buffer> actual_frame = frame_reader.ReadFrame();
+ rtc::scoped_refptr<I420BufferInterface> 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<test::FrameGeneratorInterface> 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<uint64_t>(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<I420Buffer> actual_frame = frame_reader.ReadFrame();
+ rtc::scoped_refptr<I420BufferInterface> 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(
