admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reland "Make SimulcastIndex() and SpatialIndex() distinct (remove fallback)."

Browse Source 
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}
This commit is contained in:
Henrik Boström 2023-02-21 15:53:43 +01:00 committed by WebRTC LUCI CQ
parent 573f546197
commit c5a4c938bb
11 changed files with 47 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
api/video/encoded_image.h
View File

@ -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<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 @@ 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<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);

9
call/rtp_payload_params.cc
View File

@ -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_;

13
call/rtp_video_sender.cc
View File

@ -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 =

9
test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc
View File

@ -347,10 +347,15 @@ void DefaultVideoQualityAnalyzer::OnFrameEncoded(
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(

6
test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.cc
View File

@ -88,14 +88,14 @@ void FrameInFlight::OnFrameEncoded(webrtc::Timestamp time,
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 @@ FrameStats FrameInFlight::GetStatsForPeer(size_t peer) const {
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);

8
test/pc/e2e/analyzer/video/default_video_quality_analyzer_frame_in_flight.h
View File

@ -88,7 +88,7 @@ class FrameInFlight {
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 @@ class FrameInFlight {
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.

25
test/pc/e2e/analyzer/video/quality_analyzing_video_encoder.cc
View File

@ -279,8 +279,14 @@ EncodedImageCallback::Result QualityAnalyzingVideoEncoder::OnEncodedImage(
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 @@ bool QualityAnalyzingVideoEncoder::ShouldDiscard(
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 @@ bool QualityAnalyzingVideoEncoder::ShouldDiscard(
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 @@ bool QualityAnalyzingVideoEncoder::ShouldDiscard(
// 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";

13
video/send_statistics_proxy.cc
View File

@ -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.

7
video/video_quality_test.cc
View File

@ -241,12 +241,7 @@ class QualityTestVideoEncoder : public VideoEncoder,
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,

9
video/video_stream_encoder.cc
View File

@ -2434,8 +2434,13 @@ void VideoStreamEncoder::RunPostEncode(const EncodedImage& encoded_image,
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);
}
}

4
video/video_stream_encoder_unittest.cc
View File

@ -9473,10 +9473,10 @@ TEST(VideoStreamEncoderFrameCadenceTest, UpdatesQualityConvergence) {
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;