Reland "Make SimulcastIndex() and SpatialIndex() distinct (remove fallback)."
This is a reland of commit 8ad4924936dea2bd97990b0a951df93f7526f0ff
See diff between latest Patch Set and PS1. Fixes include:
- VideoStreamEncoder's call to bitrate_adjuster_->OnEncodedFrame()
is updated to take stream index (spatial or simulcast index) instead
of only looking at SpatialIndex().
- Migrate test-only helpers to use Spatial/SimulcastIndex correctly.
The fixes are to migrate
some test-only helpers that we had forgot to fix that are used by
external tests.
Original change's description:
> Make SimulcastIndex() and SpatialIndex() distinct (remove fallback).
>
> 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 <ilnik@webrtc.org>
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Commit-Queue: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Philip Eliasson <philipel@webrtc.org>
> Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39343}
Bug: webrtc:14884
Change-Id: Ib966924efca1a040dae881599f0789a7f2ab24a2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/294284
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39358}
diff --git a/api/video/encoded_image.h b/api/video/encoded_image.h
index 5fdd2c6..9046167 100644
--- a/api/video/encoded_image.h
+++ b/api/video/encoded_image.h
@@ -91,15 +91,7 @@
// Every simulcast layer (= encoding) has its own encoder and RTP stream.
// There can be no dependencies between different simulcast layers.
- absl::optional<int> 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<int> SimulcastIndex() const { return simulcast_index_; }
void SetSimulcastIndex(absl::optional<int> simulcast_index) {
RTC_DCHECK_GE(simulcast_index.value_or(0), 0);
RTC_DCHECK_LT(simulcast_index.value_or(0), kMaxSimulcastStreams);
@@ -109,15 +101,7 @@
// 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<int> 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<int> SpatialIndex() const { return spatial_index_; }
void SetSpatialIndex(absl::optional<int> 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 b692abd..18e6d91 100644
--- a/call/rtp_payload_params.cc
+++ b/call/rtp_payload_params.cc
@@ -202,15 +202,8 @@
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 52714a5..98739a5 100644
--- a/call/rtp_video_sender.cc
+++ b/call/rtp_video_sender.cc
@@ -571,18 +571,7 @@
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/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc
index 5914458..a5c9dd7 100644
--- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc
+++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc
@@ -347,10 +347,15 @@
used_encoder.last_frame_id = frame_id;
used_encoder.switched_on_at = now;
used_encoder.switched_from_at = now;
+ // We could either have simulcast layers or spatial layers.
+ // TODO(https://crbug.com/webrtc/14891): If we want to support a mix of
+ // simulcast and SVC we'll also need to consider the case where we have both
+ // simulcast and spatial indices.
+ size_t stream_index = encoded_image.SpatialIndex().value_or(
+ encoded_image.SimulcastIndex().value_or(0));
frame_in_flight.OnFrameEncoded(
now, encoded_image._frameType, DataSize::Bytes(encoded_image.size()),
- stats.target_encode_bitrate, encoded_image.SpatialIndex().value_or(0),
- stats.qp, used_encoder);
+ stats.target_encode_bitrate, stream_index, stats.qp, used_encoder);
if (options_.report_infra_metrics) {
analyzer_stats_.on_frame_encoded_processing_time_ms.AddSample(
diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.cc
index df34dad..fb87fd5 100644
--- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.cc
+++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.cc
@@ -88,14 +88,14 @@
VideoFrameType frame_type,
DataSize encoded_image_size,
uint32_t target_encode_bitrate,
- int spatial_layer,
+ int stream_index,
int qp,
StreamCodecInfo used_encoder) {
encoded_time_ = time;
frame_type_ = frame_type;
encoded_image_size_ = encoded_image_size;
target_encode_bitrate_ += target_encode_bitrate;
- spatial_layers_qp_[spatial_layer].AddSample(SamplesStatsCounter::StatsSample{
+ stream_layers_qp_[stream_index].AddSample(SamplesStatsCounter::StatsSample{
.value = static_cast<double>(qp), .time = time});
// Update used encoder info. If simulcast/SVC is used, this method can
// be called multiple times, in such case we should preserve the value
@@ -186,7 +186,7 @@
stats.encoded_frame_type = frame_type_;
stats.encoded_image_size = encoded_image_size_;
stats.used_encoder = used_encoder_;
- stats.spatial_layers_qp = spatial_layers_qp_;
+ stats.spatial_layers_qp = stream_layers_qp_;
absl::optional<ReceiverFrameStats> receiver_stats =
MaybeGetValue<ReceiverFrameStats>(receiver_stats_, peer);
diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.h b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.h
index 52a526d0..3c5bc95 100644
--- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.h
+++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.h
@@ -88,7 +88,7 @@
VideoFrameType frame_type,
DataSize encoded_image_size,
uint32_t target_encode_bitrate,
- int spatial_layer,
+ int stream_index,
int qp,
StreamCodecInfo used_encoder);
@@ -155,9 +155,9 @@
VideoFrameType frame_type_ = VideoFrameType::kEmptyFrame;
DataSize encoded_image_size_ = DataSize::Bytes(0);
uint32_t target_encode_bitrate_ = 0;
- // Sender side qp values per spatial layer. In case when spatial layer is not
- // set for `webrtc::EncodedImage`, 0 is used as default.
- std::map<int, SamplesStatsCounter> spatial_layers_qp_;
+ // Sender side qp values per spatial or simulcast layer. If neither the
+ // spatial or simulcast index is set in `webrtc::EncodedImage`, 0 is used.
+ std::map<int, SamplesStatsCounter> stream_layers_qp_;
// Can be not set if frame was dropped by encoder.
absl::optional<StreamCodecInfo> used_encoder_ = absl::nullopt;
// Map from the receiver peer's index to frame stats for that peer.
diff --git a/test/pc/e2e/analyzer/video/quality_analyzing_video_encoder.cc b/test/pc/e2e/analyzer/video/quality_analyzing_video_encoder.cc
index e814ba8..7a2b316 100644
--- a/test/pc/e2e/analyzer/video/quality_analyzing_video_encoder.cc
+++ b/test/pc/e2e/analyzer/video/quality_analyzing_video_encoder.cc
@@ -279,8 +279,14 @@
discard = ShouldDiscard(frame_id, encoded_image);
if (!discard) {
- target_encode_bitrate = bitrate_allocation_.GetSpatialLayerSum(
- encoded_image.SpatialIndex().value_or(0));
+ // We could either have simulcast layers or spatial layers.
+ // TODO(https://crbug.com/webrtc/14891): If we want to support a mix of
+ // simulcast and SVC we'll also need to consider the case where we have
+ // both simulcast and spatial indices.
+ size_t stream_index = encoded_image.SpatialIndex().value_or(
+ encoded_image.SimulcastIndex().value_or(0));
+ target_encode_bitrate =
+ bitrate_allocation_.GetSpatialLayerSum(stream_index);
}
codec_name =
std::string(CodecTypeToPayloadString(codec_settings_.codecType)) + "_" +
@@ -326,7 +332,12 @@
if (!emulated_sfu_config)
return false;
- int cur_spatial_index = encoded_image.SpatialIndex().value_or(0);
+ // We could either have simulcast layers or spatial layers.
+ // TODO(https://crbug.com/webrtc/14891): If we want to support a mix of
+ // simulcast and SVC we'll also need to consider the case where we have both
+ // simulcast and spatial indices.
+ int cur_stream_index = encoded_image.SpatialIndex().value_or(
+ encoded_image.SimulcastIndex().value_or(0));
int cur_temporal_index = encoded_image.TemporalIndex().value_or(0);
if (emulated_sfu_config->target_temporal_index &&
@@ -338,12 +349,12 @@
case SimulcastMode::kSimulcast:
// In simulcast mode only encoded images with required spatial index are
// interested, so all others have to be discarded.
- return cur_spatial_index != *emulated_sfu_config->target_layer_index;
+ return cur_stream_index != *emulated_sfu_config->target_layer_index;
case SimulcastMode::kSVC:
// In SVC mode encoded images with spatial indexes that are equal or
// less than required one are interesting, so all above have to be
// discarded.
- return cur_spatial_index > *emulated_sfu_config->target_layer_index;
+ return cur_stream_index > *emulated_sfu_config->target_layer_index;
case SimulcastMode::kKSVC:
// In KSVC mode for key frame encoded images with spatial indexes that
// are equal or less than required one are interesting, so all above
@@ -353,8 +364,8 @@
// all of temporal layer 0 for now.
if (encoded_image._frameType == VideoFrameType::kVideoFrameKey ||
cur_temporal_index == 0)
- return cur_spatial_index > *emulated_sfu_config->target_layer_index;
- return cur_spatial_index != *emulated_sfu_config->target_layer_index;
+ return cur_stream_index > *emulated_sfu_config->target_layer_index;
+ return cur_stream_index != *emulated_sfu_config->target_layer_index;
case SimulcastMode::kNormal:
RTC_DCHECK_NOTREACHED() << "Analyzing encoder is in kNormal mode, but "
"target_layer_index is set";
diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc
index a64ab22..12e88ac 100644
--- a/video/send_statistics_proxy.cc
+++ b/video/send_statistics_proxy.cc
@@ -937,18 +937,7 @@
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_quality_test.cc b/video/video_quality_test.cc
index a903c21..583bb8c 100644
--- a/video/video_quality_test.cc
+++ b/video/video_quality_test.cc
@@ -241,12 +241,7 @@
Result OnEncodedImage(const EncodedImage& encoded_image,
const CodecSpecificInfo* codec_specific_info) override {
if (codec_specific_info) {
- int simulcast_index;
- if (codec_specific_info->codecType == kVideoCodecVP9) {
- simulcast_index = 0;
- } else {
- simulcast_index = encoded_image.SpatialIndex().value_or(0);
- }
+ int simulcast_index = encoded_image.SimulcastIndex().value_or(0);
RTC_DCHECK_GE(simulcast_index, 0);
if (analyzer_) {
analyzer_->PostEncodeOnFrame(simulcast_index,
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 65aa076..b0666a4 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -2434,8 +2434,13 @@
stream_resource_manager_.OnEncodeCompleted(encoded_image, time_sent_us,
encode_duration_us, frame_size);
if (bitrate_adjuster_) {
- bitrate_adjuster_->OnEncodedFrame(
- frame_size, encoded_image.SpatialIndex().value_or(0), temporal_index);
+ // We could either have simulcast layers or spatial layers.
+ // TODO(https://crbug.com/webrtc/14891): If we want to support a mix of
+ // simulcast and SVC we'll also need to consider the case where we have both
+ // simulcast and spatial indices.
+ int stream_index = encoded_image.SpatialIndex().value_or(
+ encoded_image.SimulcastIndex().value_or(0));
+ bitrate_adjuster_->OnEncodedFrame(frame_size, stream_index, temporal_index);
}
}
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 908c96f..c451b6f 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -9473,10 +9473,10 @@
EXPECT_CALL(factory.GetMockFakeEncoder(), EncodeHook)
.WillRepeatedly(Invoke([](EncodedImage& encoded_image,
rtc::scoped_refptr<EncodedImageBuffer> 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;
