From a6fbb35ac1849c5cd823ec90121d92fc5b776b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 30 Sep 2024 14:41:50 +0200 Subject: [PATCH] Fix LibvpxVp9Encoder simulcast bug. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of [1], a single VP9 encoder instance can produce simulcast 4:2:1. When it does, the EncodedImage has its simulcast index set (0, 1, 2). The bug is that if you then go back to a single encoder instance, either because you're doing singlecast or because you're doing simulcast with scaling factors that are not power of two (not 4:2:1), then the simulcast index which was previously set to 2 is not reset due to the old code path never calling SetSimulcastIndex. Example repro: 1. Send VP9 simulcast {180p, 360p, 720p}, i.e. 4:2.1. 2. Reconfigure to {180p, 360p, 540p}, i.e. no longer 4:2:1. What should happen: all three layers are sent. What actually happened: 180p is not sent and the 540p layer flips flops between 180p and 540p because the EncodedImage says simulcast index is 2 for both encodings[0] and encodings[2]. The fix is a one-line change: `SetSimulcastIndex(std::nullopt)` in the case that we don't have a `simulcast_to_svc_converter_` that sets it (0, 1, 2) for us. [1] https://webrtc-review.googlesource.com/c/src/+/360280 Bug: chromium:370299916 Change-Id: I52bd4428bd12528f0e98869ec61626c06f589b43 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/363941 Reviewed-by: Sergey Silkin Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43109} --- .../codecs/vp9/libvpx_vp9_encoder.cc | 4 +- ...er_connection_encodings_integrationtest.cc | 70 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc b/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc index c2a17c59d4..2da0918062 100644 --- a/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc +++ b/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc @@ -1761,7 +1761,9 @@ void LibvpxVp9Encoder::DeliverBufferedFrame(bool end_of_picture) { codec_specific_.end_of_picture = end_of_picture; - if (simulcast_to_svc_converter_) { + if (!simulcast_to_svc_converter_) { + encoded_image_.SetSimulcastIndex(std::nullopt); + } else { simulcast_to_svc_converter_->ConvertFrame(encoded_image_, codec_specific_); } diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index b8b7e18e58..8058379666 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -2417,6 +2417,76 @@ TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, EXPECT_THAT(*outbound_rtps[2]->scalability_mode, StrEq("L1T3")); } +// Simulcast starting in 720p 4:2:1 then changing to {180p, 360p, 540p} using +// the `scale_resolution_down_by` API. +TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, + SimulcastScaleDownByNoLongerPowerOfTwo) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + if (SkipTestDueToAv1Missing(local_pc_wrapper)) { + return; + } + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector layers = + CreateLayers({"q", "h", "f"}, /*active=*/true); + rtc::scoped_refptr transceiver = + AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, + layers); + std::vector codecs = + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, codec_name_); + transceiver->SetCodecPreferences(codecs); + rtc::scoped_refptr sender = transceiver->sender(); + + // Configure {180p, 360p, 720p}. + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(3)); + parameters.encodings[0].scalability_mode = "L1T1"; + parameters.encodings[0].scale_resolution_down_by = 4.0; + parameters.encodings[1].scalability_mode = "L1T1"; + parameters.encodings[1].scale_resolution_down_by = 2.0; + parameters.encodings[2].scalability_mode = "L1T1"; + parameters.encodings[2].scale_resolution_down_by = 1.0; + sender->SetParameters(parameters); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Wait for media to flow on all layers. + // Needed repro step of https://crbug.com/webrtc/369654168: When the same + // LibvpxVp9Encoder instance was used to first produce simulcast and later for + // a single encoding, the previously used simulcast index (= 2) would still be + // set when producing 180p since non-simulcast config does not reset this, + // resulting in the 180p encoding freezing and the 540p encoding having double + // frame rate and toggling between 180p and 540p in resolution. + ASSERT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 3u), + kLongTimeoutForRampingUp.ms()); + + // Configure {180p, 360p, 540p}. + parameters = sender->GetParameters(); + parameters.encodings[0].scale_resolution_down_by = 4.0; + parameters.encodings[1].scale_resolution_down_by = 2.0; + parameters.encodings[2].scale_resolution_down_by = 1.333333; + sender->SetParameters(parameters); + + // Wait for the new resolutions to be produced. + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper, "q") == + Resolution({.width = 320, .height = 180}), + kLongTimeoutForRampingUp.ms()); + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper, "h") == + Resolution({.width = 640, .height = 360}), + kLongTimeoutForRampingUp.ms()); + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper, "f") == + Resolution({.width = 960, .height = 540}), + kLongTimeoutForRampingUp.ms()); + + // Ensure 180p frames continue to be encoded post reconfiguration. + int frames_encoded = EncodedFrames(local_pc_wrapper, "q"); + ASSERT_TRUE_WAIT(EncodedFrames(local_pc_wrapper, "q") > frames_encoded, + kLongTimeoutForRampingUp.ms()); +} + // The code path that disables layers based on resolution size should NOT run // when `requested_resolution` is specified. (It shouldn't run in any case but // that is an existing legacy code and non-compliance problem that we don't have