From c65f5fd90ff8fb8451aa7687e98288071df4e280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 28 Feb 2023 16:04:21 +0100 Subject: [PATCH] VP9 Simulcast test: Update comments to reflect L1T3. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Looked in to this some more and had a chat with Evan, and L1T3 being reported in getStats() is a real sign that L1T3 is used. This CL updates the comments of the VP9 simulcast test to reflect that this is what we are getting, not SVC, even if layers are being dropped etc. Bug: webrtc:14884 Change-Id: I15eac981625302480ce337879138537c0ad73664 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295540 Commit-Queue: Evan Shrubsole Reviewed-by: Ilya Nikolaevskiy Auto-Submit: Henrik Boström Reviewed-by: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#39421} --- pc/peer_connection_simulcast_unittest.cc | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index dd56117c12..2351fe1ab1 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -1162,8 +1162,7 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, } // TODO(https://crbug.com/webrtc/14884): Support VP9 simulcast and update this -// test to EXPECT three encodings of L1T3, not the VP9 SVC legacy fallback path -// that happens today which is wrong. +// test to EXPECT three RTP streams of L1T3, not the single RTP we get today. TEST_F(PeerConnectionSimulcastWithMediaFlowTests, SendingThreeEncodings_VP9_Simulcast) { rtc::scoped_refptr local_pc_wrapper = CreatePc(); @@ -1194,24 +1193,22 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, remote_pc_wrapper->WaitForConnection(); // We want to EXPECT to get 3 "outbound-rtps", but because VP9 simulcast is - // not supported yet (webrtc:14884), we expect a single RTP stream with SVC. - // Due to bugs, the fallback to SVC is not reported in either getStats() or - // getParameters(). This test expects what we see, not what we want to see. - - // Legacy SVC fallback only has a single RTP stream. + // not supported yet (webrtc:14884), we expect a single RTP stream. We get + // "L1T3" so we do avoid SVC fallback, but the other two layers are dropped + // and some parts of the code still assume SVC which could lead to other bugs + // such as invalid bitrate configurations. EXPECT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u), kLongTimeoutForRampingUp.ms()); - // Legacy SVC fallback uses "L3T3_KEY" but the `scalability_mode` returned by - // the API seems to reflect what we asked for, not what we got. rtc::scoped_refptr report = GetStats(local_pc_wrapper); std::vector outbound_rtps = report->GetStatsOfType(); - // The fact that we only have a single RTP is a sign SVC is used. ASSERT_THAT(outbound_rtps, SizeIs(1u)); EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]), StrCaseEq("video/VP9")); - // In SVC we should see "L3T3_KEY" but we see the "L1T3" that we asked for. + // `scalability_mode` in stats does reflect LibvpxVp9Encoder settings! EXPECT_EQ(*outbound_rtps[0]->scalability_mode, "L1T3"); + // What GetParameters() is returning though is most likely just reflecting + // what was set, not what was configured. parameters = sender->GetParameters(); ASSERT_EQ(parameters.encodings.size(), 3u); EXPECT_THAT(parameters.encodings[0].scalability_mode,