From 0145db40912babe3f74e00bed01df1a7b4502105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 26 Jul 2023 14:01:59 +0200 Subject: [PATCH] Recreate the stream when switching from standard to legacy API. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReconfigureEncoder() is supposed to recreate the send stream when switching between legacy and standard API paths to ensure that the upper and lower layers agree on the number of streams that exist (legacy = 3 encodings but 1 stream, standard = same as encodings). This successfully happened when going from standard to legacy but due to a bug in the condition this did not happen when going from legacy to standard because `scalability_mode_used` is always false here (even though the standard path does use a scalability mode). As a consequence, SetRtpParameters()'s call to UpdateSendState() resulted in a DCHECK-crash. In release builds we still avoid IOOB because active_modules.size() < rtp_streams.size() but to avoid mistakes like this happening again in the future, the DCHECK is promoted to a CHECK. The fix is to remove the scalability mode condition which didn't make sense anyway - changing scalability mode does not require recreation but recreation is necessary when number of streams change, whether or not scalability mode changed. TESTED = Using Simulcast Playground and switching back and forth between standard and legacy and changing scalability modes and confirming from stats, see https://crbug.com/1467455. Bug: chromium:1467455 Change-Id: Ide29742972ba83f2e0a11f135ab9b39c39d4eb49 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/313280 Reviewed-by: Erik Språng Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#40477} --- call/rtp_video_sender.cc | 2 +- media/engine/webrtc_video_engine.cc | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 7e88b59d83..b7616d1a1f 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -498,7 +498,7 @@ void RtpVideoSender::SetActiveModules(const std::vector& active_modules) { void RtpVideoSender::SetActiveModulesLocked( const std::vector& active_modules) { RTC_DCHECK_RUN_ON(&transport_checker_); - RTC_DCHECK_EQ(rtp_streams_.size(), active_modules.size()); + RTC_CHECK_EQ(rtp_streams_.size(), active_modules.size()); active_ = false; for (size_t i = 0; i < active_modules.size(); ++i) { if (active_modules[i]) { diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index af5d0c7fe9..35a5eb479d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2166,14 +2166,9 @@ void WebRtcVideoSendChannel::WebRtcVideoSendStream::ReconfigureEncoder( // layers specified by `scalability_mode`), the number of streams can change. bool num_streams_changed = parameters_.encoder_config.number_of_streams != encoder_config.number_of_streams; - bool scalability_mode_used = !codec_settings.codec.scalability_modes.empty(); - bool scalability_modes = absl::c_any_of( - rtp_parameters_.encodings, - [](const auto& e) { return e.scalability_mode.has_value(); }); - parameters_.encoder_config = std::move(encoder_config); - if (num_streams_changed && (scalability_mode_used != scalability_modes)) { + if (num_streams_changed) { // The app is switching between legacy and standard modes, recreate instead // of reconfiguring to avoid number of streams not matching in lower layers. RecreateWebRtcStream();