From 80663cd0da6c7af99621d5a47dd2af4e41ab893c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= <hbos@webrtc.org>
Date: Tue, 28 Feb 2023 15:29:34 +0100
Subject: [PATCH] Expect VP9 legacy SVC test to ramp up eventually.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The helper function is updated to decide whether or not to log; in the
VP8 simulcast test we're expected to have ramped up already and want
the logging but in the VP9 test ramp up time is significant and we
don't want to log spam.

The kLongTimeoutForRampingUp time is increased from 20s to 30s because
we noticed that SVC is slower to ramp up than simulcast and we don't
want flaky bots. The value 30s is still 2-3 times longer than what was
needed locally, but we want the bots to have plenty of margins so we
update it "just in case" even if 20s may have been enough.

Bug: webrtc:14884
Change-Id: I4b3cea20b65b2601982edcaaa90af2ef949a23ab
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295507
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39423}
---
 pc/peer_connection_simulcast_unittest.cc | 41 ++++++++++++++----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc
index 2351fe1ab1..af0c8c0f71 100644
--- a/pc/peer_connection_simulcast_unittest.cc
+++ b/pc/peer_connection_simulcast_unittest.cc
@@ -166,7 +166,7 @@ const webrtc::RTCOutboundRTPStreamStats* FindOutboundRtpByRid(
 namespace webrtc {
 
 constexpr TimeDelta kDefaultTimeout = TimeDelta::Seconds(5);
-constexpr TimeDelta kLongTimeoutForRampingUp = TimeDelta::Seconds(20);
+constexpr TimeDelta kLongTimeoutForRampingUp = TimeDelta::Seconds(30);
 
 class PeerConnectionSimulcastTests : public ::testing::Test {
  public:
@@ -977,7 +977,8 @@ class PeerConnectionSimulcastWithMediaFlowTests
 
   bool HasOutboundRtpExpectedResolutions(
       rtc::scoped_refptr<PeerConnectionTestWrapper> pc_wrapper,
-      std::vector<RidAndResolution> resolutions) {
+      std::vector<RidAndResolution> resolutions,
+      bool log_during_ramp_up) {
     rtc::scoped_refptr<const RTCStatsReport> report = GetStats(pc_wrapper);
     std::vector<const RTCOutboundRTPStreamStats*> outbound_rtps =
         report->GetStatsOfType<RTCOutboundRTPStreamStats>();
@@ -995,12 +996,14 @@ class PeerConnectionSimulcastWithMediaFlowTests
       EXPECT_THAT(*outbound_rtp->frame_height, Le(resolution.height));
       if (*outbound_rtp->frame_width != resolution.width ||
           *outbound_rtp->frame_height != resolution.height) {
-        // 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 (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 << ")";
+        }
         return false;
       }
     }
@@ -1083,11 +1086,15 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
   remote_pc_wrapper->WaitForConnection();
 
   // Wait until media is flowing on all three layers.
+  // Ramp up time is needed before all three layers are sending.
   EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u),
                    kLongTimeoutForRampingUp.ms());
+  // No significant additional ramp up time should be needed so we use
+  // `kDefaultTimeout` and `log_during_ramp_up`.
   EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions(
                        local_pc_wrapper,
-                       {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}}),
+                       {{"f", 320, 180}, {"h", 640, 360}, {"q", 1280, 720}},
+                       /*log_during_ramp_up=*/true),
                    kDefaultTimeout.ms());
   // Verify codec and scalability mode.
   rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
@@ -1130,15 +1137,15 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests,
   remote_pc_wrapper->WaitForConnection();
 
   // Wait until media is flowing. We only expect a single RTP stream.
+  // We expect to see bytes flowing almost immediately on the lowest layer.
   EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u),
-                   kLongTimeoutForRampingUp.ms());
-  // It takes a long time for SVC to ramp up. To avoid risk of flakes on slow
-  // machines this is commented out, but we do expect this to pass.
-  // TODO(https://crbug.com/webrtc/14884): See if we can enable this EXPECT line
-  // in a standalone CL in case it gets reverted.
-  // EXPECT_TRUE_WAIT(HasOutboundRtpExpectedResolutions(local_pc_wrapper,
-  //                                                    {{"f", 1280, 720}}),
-  //                  kLongTimeoutForRampingUp.ms());
+                   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());
   // Verify codec and scalability mode.
   rtc::scoped_refptr<const RTCStatsReport> report = GetStats(local_pc_wrapper);
   std::vector<const RTCOutboundRTPStreamStats*> outbound_rtps =