[Stats] Fix outbound-rtp.active bug in SVC.
The code for determining outbound-rtp.active assumed, as the spec says,
that we have one RtpEncodingParameters per RTP stream. Unfortunately
SVC is currently implemented as one RtpEncodingParameters per SVC
layer. This causes a discrepency where we do correctly only have one
outbound-rtp stats object, but the lookup to check whether or not we are
"active" needs to look at more than a single encoding.
The bug is that if SVC layers are {inactive, active, active} then
stats reports outbound-rtp.active: false. With this fix, active: true is
reported if ANY of the SVC layers are active.
For singlecast or simulcast this CL has no change in behavior. In these
cases we have the same number of outbound-rtp and encodings and a simple
ssrc lookup does work.
The fix is exercised by unit tests and has also manually been confirmed:
- Singlecast tested by https://jsfiddle.net/henbos/nvd6p4j1/.
- Simulcast tested by https://crbug.com/webrtc/14628#c11.
- SVC tested by Google Meet and chrome://webrtc-internals/.
Bug: webrtc:14628
Change-Id: Ib89945caf29c8f4b85dd8a1120dcf8279296e4a3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/282222
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38569}
This commit is contained in:
parent
0e2cf6cc01
commit
262d9cca08
@ -416,6 +416,28 @@ MergeInfoAboutOutboundRtpSubstreams(
|
||||
return rtp_substreams;
|
||||
}
|
||||
|
||||
bool IsActiveFromEncodings(
|
||||
absl::optional<uint32_t> ssrc,
|
||||
const std::vector<webrtc::RtpEncodingParameters>& encodings) {
|
||||
if (ssrc.has_value()) {
|
||||
// Report the `active` value of a specific ssrc, or false if an encoding
|
||||
// with this ssrc does not exist.
|
||||
auto encoding_it = std::find_if(
|
||||
encodings.begin(), encodings.end(),
|
||||
[ssrc = ssrc.value()](const webrtc::RtpEncodingParameters& encoding) {
|
||||
return encoding.ssrc.has_value() && encoding.ssrc.value() == ssrc;
|
||||
});
|
||||
return encoding_it != encodings.end() ? encoding_it->active : false;
|
||||
}
|
||||
// If `ssrc` is not specified then any encoding being active counts as active.
|
||||
for (const auto& encoding : encodings) {
|
||||
if (encoding.active) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
// This constant is really an on/off, lower-level configurable NACK history
|
||||
@ -2627,20 +2649,16 @@ WebRtcVideoChannel::WebRtcVideoSendStream::GetPerLayerVideoSenderInfos(
|
||||
common_info.aggregated_huge_frames_sent = stats.huge_frames_sent;
|
||||
common_info.power_efficient_encoder = stats.power_efficient_encoder;
|
||||
|
||||
// If we don't have any substreams, get the remaining metrics from `stats`.
|
||||
// Otherwise, these values are obtained from `sub_stream` below.
|
||||
// The normal case is that substreams are present, handled below. But if
|
||||
// substreams are missing (can happen before negotiated/connected where we
|
||||
// have no stats yet) a single outbound-rtp is created representing any and
|
||||
// all layers.
|
||||
if (stats.substreams.empty()) {
|
||||
for (uint32_t ssrc : parameters_.config.rtp.ssrcs) {
|
||||
common_info.add_ssrc(ssrc);
|
||||
auto encoding_it = std::find_if(
|
||||
rtp_parameters_.encodings.begin(), rtp_parameters_.encodings.end(),
|
||||
[&ssrc](const webrtc::RtpEncodingParameters& parameters) {
|
||||
return parameters.ssrc && parameters.ssrc == ssrc;
|
||||
});
|
||||
if (encoding_it != rtp_parameters_.encodings.end()) {
|
||||
common_info.active = encoding_it->active;
|
||||
}
|
||||
}
|
||||
common_info.active =
|
||||
IsActiveFromEncodings(absl::nullopt, rtp_parameters_.encodings);
|
||||
common_info.framerate_sent = stats.encode_frame_rate;
|
||||
common_info.frames_encoded = stats.frames_encoded;
|
||||
common_info.total_encode_time_ms = stats.total_encode_time_ms;
|
||||
@ -2651,21 +2669,24 @@ WebRtcVideoChannel::WebRtcVideoSendStream::GetPerLayerVideoSenderInfos(
|
||||
return infos;
|
||||
}
|
||||
}
|
||||
// Merge `stats.substreams`, which may contain additional SSRCs for RTX or
|
||||
// Flexfec, with media SSRCs. This results in a set of substreams that match
|
||||
// with the outbound-rtp stats objects.
|
||||
auto outbound_rtp_substreams =
|
||||
MergeInfoAboutOutboundRtpSubstreams(stats.substreams);
|
||||
// If SVC is used, one stream is configured but multiple encodings exist. This
|
||||
// is not spec-compliant, but it is how we've implemented SVC so this affects
|
||||
// how the RTP stream's "active" value is determined.
|
||||
bool is_svc = (parameters_.encoder_config.number_of_streams == 1 &&
|
||||
rtp_parameters_.encodings.size() > 1);
|
||||
for (const auto& pair : outbound_rtp_substreams) {
|
||||
auto info = common_info;
|
||||
info.add_ssrc(pair.first);
|
||||
info.rid = parameters_.config.rtp.GetRidForSsrc(pair.first);
|
||||
// Search the associated encoding by SSRC.
|
||||
auto encoding_it = std::find_if(
|
||||
rtp_parameters_.encodings.begin(), rtp_parameters_.encodings.end(),
|
||||
[&pair](const webrtc::RtpEncodingParameters& parameters) {
|
||||
return parameters.ssrc && pair.first == *parameters.ssrc;
|
||||
});
|
||||
if (encoding_it != rtp_parameters_.encodings.end()) {
|
||||
info.active = encoding_it->active;
|
||||
}
|
||||
uint32_t ssrc = pair.first;
|
||||
info.add_ssrc(ssrc);
|
||||
info.rid = parameters_.config.rtp.GetRidForSsrc(ssrc);
|
||||
info.active = IsActiveFromEncodings(
|
||||
!is_svc ? absl::optional<uint32_t>(ssrc) : absl::nullopt,
|
||||
rtp_parameters_.encodings);
|
||||
auto stream_stats = pair.second;
|
||||
RTC_DCHECK_EQ(stream_stats.type,
|
||||
webrtc::VideoSendStream::StreamStats::StreamType::kMedia);
|
||||
|
||||
@ -5824,6 +5824,91 @@ TEST_F(WebRtcVideoChannelTest, GetPerLayerStatsReportForSubStreams) {
|
||||
EXPECT_EQ(sender.rid, absl::nullopt);
|
||||
}
|
||||
|
||||
TEST_F(WebRtcVideoChannelTest,
|
||||
OutboundRtpIsActiveComesFromMatchingEncodingInSimulcast) {
|
||||
constexpr uint32_t kSsrc1 = 123u;
|
||||
constexpr uint32_t kSsrc2 = 456u;
|
||||
|
||||
// Create simulcast stream from both SSRCs.
|
||||
// `kSsrc1` is the "main" ssrc used for getting parameters.
|
||||
FakeVideoSendStream* stream =
|
||||
AddSendStream(cricket::CreateSimStreamParams("cname", {kSsrc1, kSsrc2}));
|
||||
|
||||
webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(kSsrc1);
|
||||
ASSERT_EQ(2u, parameters.encodings.size());
|
||||
parameters.encodings[0].active = false;
|
||||
parameters.encodings[1].active = true;
|
||||
channel_->SetRtpSendParameters(kSsrc1, parameters);
|
||||
|
||||
// Fill in dummy stats.
|
||||
auto stats = GetInitialisedStats();
|
||||
stats.substreams[kSsrc1];
|
||||
stats.substreams[kSsrc2];
|
||||
stream->SetStats(stats);
|
||||
|
||||
// GetStats() and ensure `active` matches `encodings` for each SSRC.
|
||||
cricket::VideoMediaInfo video_media_info;
|
||||
ASSERT_TRUE(channel_->GetStats(&video_media_info));
|
||||
ASSERT_EQ(video_media_info.senders.size(), 2u);
|
||||
ASSERT_TRUE(video_media_info.senders[0].active.has_value());
|
||||
EXPECT_FALSE(video_media_info.senders[0].active.value());
|
||||
ASSERT_TRUE(video_media_info.senders[1].active.has_value());
|
||||
EXPECT_TRUE(video_media_info.senders[1].active.value());
|
||||
}
|
||||
|
||||
TEST_F(WebRtcVideoChannelTest, OutboundRtpIsActiveComesFromAnyEncodingInSvc) {
|
||||
cricket::VideoSendParameters send_parameters;
|
||||
send_parameters.codecs.push_back(GetEngineCodec("VP9"));
|
||||
ASSERT_TRUE(channel_->SetSendParameters(send_parameters));
|
||||
|
||||
constexpr uint32_t kSsrc1 = 123u;
|
||||
constexpr uint32_t kSsrc2 = 456u;
|
||||
constexpr uint32_t kSsrc3 = 789u;
|
||||
|
||||
// Configuring SVC is done the same way that simulcast is configured, the only
|
||||
// difference is that the VP9 codec is used. This triggers special hacks that
|
||||
// we depend on because we don't have a proper SVC API yet.
|
||||
FakeVideoSendStream* stream = AddSendStream(
|
||||
cricket::CreateSimStreamParams("cname", {kSsrc1, kSsrc2, kSsrc3}));
|
||||
// Expect that we got SVC.
|
||||
EXPECT_EQ(stream->GetEncoderConfig().number_of_streams, 1u);
|
||||
webrtc::VideoCodecVP9 vp9_settings;
|
||||
ASSERT_TRUE(stream->GetVp9Settings(&vp9_settings));
|
||||
EXPECT_EQ(vp9_settings.numberOfSpatialLayers, 3u);
|
||||
|
||||
webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(kSsrc1);
|
||||
ASSERT_EQ(3u, parameters.encodings.size());
|
||||
parameters.encodings[0].active = false;
|
||||
parameters.encodings[1].active = true;
|
||||
parameters.encodings[2].active = false;
|
||||
channel_->SetRtpSendParameters(kSsrc1, parameters);
|
||||
|
||||
// Fill in dummy stats.
|
||||
auto stats = GetInitialisedStats();
|
||||
stats.substreams[kSsrc1];
|
||||
stream->SetStats(stats);
|
||||
|
||||
// GetStats() and ensure `active` is true if ANY encoding is active.
|
||||
cricket::VideoMediaInfo video_media_info;
|
||||
ASSERT_TRUE(channel_->GetStats(&video_media_info));
|
||||
ASSERT_EQ(video_media_info.senders.size(), 1u);
|
||||
// Middle layer is active.
|
||||
ASSERT_TRUE(video_media_info.senders[0].active.has_value());
|
||||
EXPECT_TRUE(video_media_info.senders[0].active.value());
|
||||
|
||||
parameters = channel_->GetRtpSendParameters(kSsrc1);
|
||||
ASSERT_EQ(3u, parameters.encodings.size());
|
||||
parameters.encodings[0].active = false;
|
||||
parameters.encodings[1].active = false;
|
||||
parameters.encodings[2].active = false;
|
||||
channel_->SetRtpSendParameters(kSsrc1, parameters);
|
||||
ASSERT_TRUE(channel_->GetStats(&video_media_info));
|
||||
ASSERT_EQ(video_media_info.senders.size(), 1u);
|
||||
// No layer is active.
|
||||
ASSERT_TRUE(video_media_info.senders[0].active.has_value());
|
||||
EXPECT_FALSE(video_media_info.senders[0].active.value());
|
||||
}
|
||||
|
||||
TEST_F(WebRtcVideoChannelTest, MediaSubstreamMissingProducesEmpyStats) {
|
||||
FakeVideoSendStream* stream = AddSendStream();
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user