From 962b3935e44053641764beb0bd095540fe8cbd64 Mon Sep 17 00:00:00 2001 From: Emil Vardar Date: Tue, 6 Aug 2024 14:36:54 +0000 Subject: [PATCH] Each spatial layer can only have 1 QP value. As the code is now, it looks like it accepts that a spatial layer can have more than 1 QP value. These QP values according to the code are summed. However, to my best knowledge this cannot be the case and makes the code hard to read. Therefore, updating it with a check such that it checks that each spatial layer only have 1 QP value and would be easier for a future code reader. Bug: webrtc:357636606 Change-Id: I650cac724811a1ddc7ab8933c1e1ac5fe844b61c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/358101 Reviewed-by: Mirko Bonadei Reviewed-by: Artem Titov Commit-Queue: Emil Vardar (xWF) Cr-Commit-Position: refs/heads/main@{#42736} --- .../video/default_video_quality_analyzer.cc | 3 +- ...ideo_quality_analyzer_frames_comparator.cc | 12 +++-- ...quality_analyzer_frames_comparator_test.cc | 53 ++++++++----------- ...quality_analyzer_internal_shared_objects.h | 9 +++- 4 files changed, 37 insertions(+), 40 deletions(-) diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc index d2f5725cc9..e25680037d 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc @@ -365,7 +365,8 @@ void DefaultVideoQualityAnalyzer::OnFrameEncoded( // TODO(https://crbug.com/webrtc/14891): If we want to support a mix of // simulcast and SVC we'll also need to consider the case where we have both // simulcast and spatial indices. - size_t stream_index = encoded_image.SpatialIndex().value_or( + // In singlecast, spatial/simulcast is expected not to be set. + int stream_index = encoded_image.SpatialIndex().value_or( encoded_image.SimulcastIndex().value_or(0)); frame_in_flight.OnFrameEncoded( now, time_between_encoded_frames, encoded_image._frameType, diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc index eb10aef34c..5cd8bf71f3 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc @@ -501,12 +501,14 @@ void DefaultVideoQualityAnalyzerFramesComparator::ProcessComparison( frame_stats.encoded_image_size.bytes(); stats->target_encode_bitrate.AddSample(StatsSample( frame_stats.target_encode_bitrate, frame_stats.encoded_time, metadata)); - for (const auto& [spatial_layer, qp_values] : + for (const auto& [spatial_layer, qp_value] : frame_stats.spatial_layers_qp) { - for (SamplesStatsCounter::StatsSample qp : qp_values.GetTimedSamples()) { - qp.metadata = metadata; - stats->spatial_layers_qp[spatial_layer].AddSample(std::move(qp)); - } + // Each spatial/simulcast layer can only have one qp value. + RTC_CHECK_EQ(qp_value.GetTimedSamples().size(), 1) + << "Can only be 1 QP value per spatial layer."; + SamplesStatsCounter::StatsSample qp = qp_value.GetTimedSamples()[0]; + qp.metadata = metadata; + stats->spatial_layers_qp[spatial_layer].AddSample(std::move(qp)); } // Stats sliced on encoded frame type. diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc index 2656bf5d44..e4387d5152 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc @@ -503,8 +503,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; comparator.Start(/*max_threads_count=*/1); comparator.EnsureStatsForStream(stream, sender, /*peers_count=*/2, @@ -534,7 +533,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectEmpty(stats.recv_key_frame_size_bytes); ExpectEmpty(stats.recv_delta_frame_size_bytes); @@ -579,8 +578,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; comparator.Start(/*max_threads_count=*/1); comparator.EnsureStatsForStream(stream, sender, /*peers_count=*/2, @@ -610,7 +608,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectEmpty(stats.recv_key_frame_size_bytes); ExpectEmpty(stats.recv_delta_frame_size_bytes); @@ -655,8 +653,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -692,7 +689,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectSizeAndAllElementsAre(stats.recv_key_frame_size_bytes, /*size=*/1, /*value=*/500.0); @@ -738,8 +735,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -782,7 +778,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectSizeAndAllElementsAre(stats.recv_key_frame_size_bytes, /*size=*/1, /*value=*/500.0); @@ -829,8 +825,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -870,7 +865,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectSizeAndAllElementsAre(stats.recv_key_frame_size_bytes, /*size=*/1, /*value=*/500.0); @@ -1038,8 +1033,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; comparator.Start(/*max_threads_count=*/1); comparator.EnsureStatsForStream(stream, sender, /*peers_count=*/2, @@ -1069,7 +1063,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectEmpty(stats.recv_key_frame_size_bytes); ExpectEmpty(stats.recv_delta_frame_size_bytes); @@ -1114,8 +1108,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; comparator.Start(/*max_threads_count=*/1); comparator.EnsureStatsForStream(stream, sender, /*peers_count=*/2, @@ -1145,7 +1138,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectEmpty(stats.recv_key_frame_size_bytes); ExpectEmpty(stats.recv_delta_frame_size_bytes); @@ -1267,8 +1260,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -1309,7 +1301,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectEmpty(stats.recv_key_frame_size_bytes); ExpectEmpty(stats.recv_delta_frame_size_bytes); @@ -1355,8 +1347,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -1396,7 +1387,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectSizeAndAllElementsAre(stats.recv_key_frame_size_bytes, /*size=*/1, /*value=*/500.0); @@ -1447,8 +1438,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); @@ -1492,7 +1482,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, ExpectSizeAndAllElementsAre(stats.target_encode_bitrate, /*size=*/1, /*value=*/2000.0); EXPECT_THAT(stats.spatial_layers_qp, SizeIs(1)); - ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/2, + ExpectSizeAndAllElementsAre(stats.spatial_layers_qp[0], /*size=*/1, /*value=*/5.0); ExpectSizeAndAllElementsAre(stats.recv_key_frame_size_bytes, /*size=*/1, /*value=*/500.0); @@ -1540,8 +1530,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, AllStatsHaveMetadataSet) { frame_stats.target_encode_bitrate = 2000; frame_stats.spatial_layers_qp = { {0, StatsCounter( - /*samples=*/{{5, Timestamp::Seconds(1)}, - {5, Timestamp::Seconds(2)}})}}; + /*samples=*/{{5, Timestamp::Seconds(1)}})}}; // Frame pre decoded frame_stats.pre_decoded_frame_type = VideoFrameType::kVideoFrameKey; frame_stats.pre_decoded_image_size = DataSize::Bytes(500); diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_internal_shared_objects.h b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_internal_shared_objects.h index 88c0335b5a..1c35d6f9b3 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_internal_shared_objects.h +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_internal_shared_objects.h @@ -69,8 +69,13 @@ struct FrameStats { VideoFrameType pre_decoded_frame_type = VideoFrameType::kEmptyFrame; DataSize pre_decoded_image_size = DataSize::Bytes(0); uint32_t target_encode_bitrate = 0; - // Sender side qp values per spatial layer. In case when spatial layer is not - // set for `webrtc::EncodedImage`, 0 is used as default. + // Sender side qp value per spatial layer. In case when spatial layer is not + // set for `webrtc::EncodedImage`, 0 is used as default. Each spatial layer + // can only have 1 QP value. Reason for using `SamplesStatsCounter` is because + // `SamplesStatsCounter::StatsSample` not having a default ctor and + // `Timestamp` is of interest. + // TODO(webrtc:357636606): Replace `SamplesStatsCounter` with optional + // and `Timestamp`. std::map spatial_layers_qp; absl::optional decoded_frame_width = absl::nullopt;