From 8ad4924936dea2bd97990b0a951df93f7526f0ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 16 Feb 2023 09:57:05 +0100 Subject: [PATCH] Make SimulcastIndex() and SpatialIndex() distinct (remove fallback). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL removes the fallback logic to return the other index when the one requested has not been set. This means we can remove the codec gates that was previously needed because SpatialIndex() had multiple meanings, resolving the TODOs previously added in https://webrtc-review.googlesource.com/c/src/+/293343. We have already migrated all known external dependencies from SpatialIndex() to SimulcastIndex() where necessary, unblocking this CL. PSA: https://groups.google.com/g/discuss-webrtc/c/SDAVg6xJ3gY Bug: webrtc:14884 Change-Id: I82787505ab10be151e5f64965b270c45465d63a9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/293740 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Erik Språng Commit-Queue: Henrik Boström Reviewed-by: Philip Eliasson Reviewed-by: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#39343} --- api/video/encoded_image.h | 20 ++------------------ call/rtp_payload_params.cc | 9 +-------- call/rtp_video_sender.cc | 13 +------------ video/send_statistics_proxy.cc | 13 +------------ video/video_stream_encoder_unittest.cc | 4 ++-- 5 files changed, 7 insertions(+), 52 deletions(-) diff --git a/api/video/encoded_image.h b/api/video/encoded_image.h index 5fdd2c666c..9046167470 100644 --- a/api/video/encoded_image.h +++ b/api/video/encoded_image.h @@ -91,15 +91,7 @@ class RTC_EXPORT EncodedImage { // Every simulcast layer (= encoding) has its own encoder and RTP stream. // There can be no dependencies between different simulcast layers. - absl::optional SimulcastIndex() const { - // Historically, SpatialIndex() has been used as both simulcast and spatial - // index (one or the other depending on codec). As to not break old code - // which doesn't call SetSimulcastIndex(), SpatialLayer() is used when the - // simulcast index is missing. - // TODO(https://crbug.com/webrtc/14884): When old code has been updated, - // never return `spatial_index_` here. - return simulcast_index_.has_value() ? simulcast_index_ : spatial_index_; - } + absl::optional SimulcastIndex() const { return simulcast_index_; } void SetSimulcastIndex(absl::optional simulcast_index) { RTC_DCHECK_GE(simulcast_index.value_or(0), 0); RTC_DCHECK_LT(simulcast_index.value_or(0), kMaxSimulcastStreams); @@ -109,15 +101,7 @@ class RTC_EXPORT EncodedImage { // Encoded images can have dependencies between spatial and/or temporal // layers, depending on the scalability mode used by the encoder. See diagrams // at https://w3c.github.io/webrtc-svc/#dependencydiagrams*. - absl::optional SpatialIndex() const { - // Historically, SpatialIndex() has been used as both simulcast and spatial - // index (one or the other depending on codec). As to not break old code - // that still uses the SpatialIndex() getter instead of SimulcastIndex() - // we fall back to `simulcast_index_` if `spatial_index_` is not set. - // TODO(https://crbug.com/webrtc/14884): When old code has been updated, - // never return `simulcast_index_` here. - return spatial_index_.has_value() ? spatial_index_ : simulcast_index_; - } + absl::optional SpatialIndex() const { return spatial_index_; } void SetSpatialIndex(absl::optional spatial_index) { RTC_DCHECK_GE(spatial_index.value_or(0), 0); RTC_DCHECK_LT(spatial_index.value_or(0), kMaxSpatialLayers); diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index b692abd976..18e6d9136b 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -202,15 +202,8 @@ RTPVideoHeader RtpPayloadParams::GetRtpVideoHeader( if (codec_specific_info) { PopulateRtpWithCodecSpecifics(*codec_specific_info, image.SpatialIndex(), &rtp_video_header); - // Currently, SimulcastIndex() could return the SpatialIndex() if not set - // correctly so gate on codec type. - // TODO(https://crbug.com/webrtc/14884): Delete this gating logic when - // SimulcastIndex() is guaranteed to be the stream index. - if (codec_specific_info->codecType != kVideoCodecVP9 && - codec_specific_info->codecType != kVideoCodecAV1) { - rtp_video_header.simulcastIdx = image.SimulcastIndex().value_or(0); - } } + rtp_video_header.simulcastIdx = image.SimulcastIndex().value_or(0); rtp_video_header.frame_type = image._frameType; rtp_video_header.rotation = image.rotation_; rtp_video_header.content_type = image.content_type_; diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 52714a55a5..98739a595e 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -571,18 +571,7 @@ EncodedImageCallback::Result RtpVideoSender::OnEncodedImage( return Result(Result::ERROR_SEND_FAILED); shared_frame_id_++; - size_t simulcast_index = 0; - // Currently, SimulcastIndex() could return the SpatialIndex() if not set - // correctly so gate on codec type. - // TODO(https://crbug.com/webrtc/14884): Delete this gating logic when - // SimulcastIndex() is guaranteed to be the stream index. - if (codec_specific_info && - (codec_specific_info->codecType == kVideoCodecVP8 || - codec_specific_info->codecType == kVideoCodecH264 || - codec_specific_info->codecType == kVideoCodecGeneric)) { - // Map spatial index to simulcast. - simulcast_index = encoded_image.SimulcastIndex().value_or(0); - } + size_t simulcast_index = encoded_image.SimulcastIndex().value_or(0); RTC_DCHECK_LT(simulcast_index, rtp_streams_.size()); uint32_t rtp_timestamp = diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index a64ab221db..12e88ac16d 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -937,18 +937,7 @@ void SendStatisticsProxy::OnMinPixelLimitReached() { void SendStatisticsProxy::OnSendEncodedImage( const EncodedImage& encoded_image, const CodecSpecificInfo* codec_info) { - // Simulcast is used for VP8, H264 and Generic. - // Currently, SimulcastIndex() could return the SpatialIndex() if not set - // correctly so gate on codec type. - // TODO(https://crbug.com/webrtc/14884): Delete this gating logic when - // SimulcastIndex() is guaranteed to be the stream index. - int simulcast_idx = - (codec_info && (codec_info->codecType == kVideoCodecVP8 || - codec_info->codecType == kVideoCodecH264 || - codec_info->codecType == kVideoCodecGeneric)) - ? encoded_image.SimulcastIndex().value_or(0) - : 0; - + int simulcast_idx = encoded_image.SimulcastIndex().value_or(0); MutexLock lock(&mutex_); ++stats_.frames_encoded; // The current encode frame rate is based on previously encoded frames. diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index cdd4c75ab7..f2963c6778 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -9428,10 +9428,10 @@ TEST(VideoStreamEncoderFrameCadenceTest, UpdatesQualityConvergence) { EXPECT_CALL(factory.GetMockFakeEncoder(), EncodeHook) .WillRepeatedly(Invoke([](EncodedImage& encoded_image, rtc::scoped_refptr buffer) { - // This sets spatial index 0 content to be at target quality, while + // This sets simulcast index 0 content to be at target quality, while // index 1 content is not. encoded_image.qp_ = kVp8SteadyStateQpThreshold + - (encoded_image.SpatialIndex() == 0 ? 0 : 1); + (encoded_image.SimulcastIndex() == 0 ? 0 : 1); CodecSpecificInfo codec_specific; codec_specific.codecType = kVideoCodecVP8; return codec_specific;