From 00029fe97e7cb96ba4133e09589a1808ff2004b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 21 Mar 2023 12:40:24 +0100 Subject: [PATCH] Reduce flakiness of PeerConnectionSimulcastWithMediaFlowTests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this CL we would EXPECT_TRUE_WAIT until HasOutboundRtpExpectedResolutions() confirmed that we achieved the maximum expected resolution on all simulcast layers. This was meant to catch bugs in case the wrong layers were configured with the wrong layer resolutions. The problem is that if CPU or BW adaptation kicks in, all layers get downscaled by some factor and the test may not always recover in time, e.g. if running on slow slow bots. This CL relaxes the expectation only to fail if the resolution exceeds what we expect, not if they are smaller. This is not as air tight but it should still catch most bugs of interest and reduce flakiness. This was reported in comment https://crbug.com/webrtc/15018#c14 but note that this CL does not attempt to fix the other ASAN issue. Bug: webrtc:15018 Change-Id: I3305bdade5d1626b09aa5c67217bdedb22cdd876 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298563 Commit-Queue: Henrik Boström Reviewed-by: Evan Shrubsole Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#39615} --- pc/peer_connection_simulcast_unittest.cc | 90 +++++++----------------- 1 file changed, 26 insertions(+), 64 deletions(-) diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 1f5f17a517..9d0c9e2f0e 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -992,10 +992,9 @@ class PeerConnectionSimulcastWithMediaFlowTests return true; } - bool HasOutboundRtpExpectedResolutions( + bool OutboundRtpResolutionsAreLessThanOrEqualToExpectations( rtc::scoped_refptr pc_wrapper, - std::vector resolutions, - bool log_during_ramp_up) { + std::vector resolutions) { rtc::scoped_refptr report = GetStats(pc_wrapper); std::vector outbound_rtps = report->GetStatsOfType(); @@ -1009,22 +1008,18 @@ class PeerConnectionSimulcastWithMediaFlowTests if (!outbound_rtp || !outbound_rtp->frame_width.is_defined() || !outbound_rtp->frame_height.is_defined()) { // RTP not found by rid or has not encoded a frame yet. + RTC_LOG(LS_ERROR) << "rid=" << resolution.rid << " does not have " + << "resolution metrics"; return false; } - // The actual resolution must never exceed what is configured, but it may - // be less due to adaptation or ramp up. - EXPECT_THAT(*outbound_rtp->frame_width, Le(resolution.width)); - EXPECT_THAT(*outbound_rtp->frame_height, Le(resolution.height)); - if (*outbound_rtp->frame_width != resolution.width || - *outbound_rtp->frame_height != resolution.height) { - if (log_during_ramp_up) { - // Useful logging for debugging. - RTC_LOG(LS_ERROR) - << "rid=" << resolution.rid << " is " - << *outbound_rtp->frame_width << "x" - << *outbound_rtp->frame_height << " (want " << resolution.width - << "x" << resolution.height << ")"; - } + if (*outbound_rtp->frame_width > resolution.width || + *outbound_rtp->frame_height > resolution.height) { + RTC_LOG(LS_ERROR) << "rid=" << resolution.rid << " is " + << *outbound_rtp->frame_width << "x" + << *outbound_rtp->frame_height + << ", this is greater than the " + << "expected " << resolution.width << "x" + << resolution.height; return false; } } @@ -1108,12 +1103,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // Wait until media is flowing. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u), kDefaultTimeout.ms()); - // Significant ramp up time is needed until maximum resolution is achieved so - // we disable `log_during_ramp_up` to avoid log spam. - EXPECT_TRUE_WAIT( - HasOutboundRtpExpectedResolutions(local_pc_wrapper, {{"", 1280, 720}}, - /*log_during_ramp_up=*/false), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1147,13 +1138,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // Ramp up time is needed before all three layers are sending. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u), kLongTimeoutForRampingUp.ms()); - // Sometimes additional ramp up is needed to get the expected resolutions. If - // that has not happened yet we log (`log_during_ramp_up=true`). - EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions( - local_pc_wrapper, - {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}}, - /*log_during_ramp_up=*/true), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1298,13 +1284,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // Ramp up time is needed before all three layers are sending. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u), kLongTimeoutForRampingUp.ms()); - // Sometimes additional ramp up is needed to get the expected resolutions. If - // that has not happened yet we log (`log_during_ramp_up=true`). - EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions( - local_pc_wrapper, - {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}}, - /*log_during_ramp_up=*/true), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1351,12 +1332,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // We expect to see bytes flowing almost immediately on the lowest layer. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u), kDefaultTimeout.ms()); - // Significant ramp up time is needed until maximum resolution is achieved so - // we disable `log_during_ramp_up` to avoid log spam. - EXPECT_TRUE_WAIT( - HasOutboundRtpExpectedResolutions(local_pc_wrapper, {{"f", 1280, 720}}, - /*log_during_ramp_up=*/false), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"f", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1409,13 +1386,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // We expect to see bytes flowing almost immediately on the lowest layer. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u), kDefaultTimeout.ms()); - // Significant ramp up time is needed until maximum resolution is achieved so - // we disable `log_during_ramp_up` to avoid log spam. - // Because only a single encoding is used, the RID is not used. - EXPECT_TRUE_WAIT( - HasOutboundRtpExpectedResolutions(local_pc_wrapper, {{"", 1280, 720}}, - /*log_during_ramp_up=*/false), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1480,13 +1452,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // Ramp up time is needed before all three layers are sending. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u), kLongTimeoutForRampingUp.ms()); - // Sometimes additional ramp up is needed to get the expected resolutions. If - // that has not happened yet we log (`log_during_ramp_up=true`). - EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions( - local_pc_wrapper, - {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}}, - /*log_during_ramp_up=*/true), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = @@ -1563,13 +1530,8 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, // giving this test extra long timeout. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u), (2 * kLongTimeoutForRampingUp).ms()); - // Sometimes additional ramp up is needed to get the expected resolutions. If - // that has not happened yet we log (`log_during_ramp_up=true`). - EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions( - local_pc_wrapper, - {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}}, - /*log_during_ramp_up=*/true), - kLongTimeoutForRampingUp.ms()); + EXPECT_TRUE(OutboundRtpResolutionsAreLessThanOrEqualToExpectations( + local_pc_wrapper, {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}})); // Verify codec and scalability mode. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps =