Avoid duplicate RTCCodecStats entries.
The code incorrectly assumed that codecs exist on a per-mid/transceiver
basis, but codec payload types are unique on a per-transport basis and
in practise most applications use BUNDLE (single transport for the
entire PC).
This CL makes the codecs per-transport instead of per-transceiver. We
still need to iterate transceivers because codecs are exposed on a
per-transceiver basis and as shown in
https://jsfiddle.net/henbos/7kqxgnr8/ it is possible for FMTP lines to
be different on different m= sections despite BUNDLE.
Manual testing shows that this CL brings down the number of "codec"
stats in Google Meet 50p from 872 objects to 43 objects.
Bug: webrtc:14414
Change-Id: Ic854b31bd595799554b99fff22cbd48264ebd141
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273707
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Philipp Hancke <phancke@microsoft.com>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37989}
diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc
index be6c23e..1f944a0 100644
--- a/pc/rtc_stats_collector.cc
+++ b/pc/rtc_stats_collector.cc
@@ -70,13 +70,18 @@
return "RTCCertificate_" + fingerprint;
}
-std::string RTCCodecStatsIDFromMidDirectionAndPayload(const std::string& mid,
- bool inbound,
- uint32_t payload_type) {
+std::string RTCCodecStatsIDFromTransportAndCodecParameters(
+ const std::string& transport_id,
+ bool inbound,
+ const RtpCodecParameters& codec_params) {
char buf[1024];
rtc::SimpleStringBuilder sb(buf);
- sb << "RTCCodec_" << mid << (inbound ? "_Inbound_" : "_Outbound_")
- << payload_type;
+ sb << "RTCCodec_" << transport_id << (inbound ? "_Recv_" : "_Send_")
+ << codec_params.payload_type;
+ rtc::StringBuilder fmtp;
+ if (WriteFmtpParameters(codec_params.parameters, &fmtp)) {
+ sb << "_" << fmtp.Release();
+ }
return sb.str();
}
@@ -354,7 +359,6 @@
std::unique_ptr<RTCCodecStats> CodecStatsFromRtpCodecParameters(
uint64_t timestamp_us,
- const std::string& mid,
const std::string& transport_id,
bool inbound,
const RtpCodecParameters& codec_params) {
@@ -362,9 +366,10 @@
RTC_DCHECK_LE(codec_params.payload_type, 127);
RTC_DCHECK(codec_params.clock_rate);
uint32_t payload_type = static_cast<uint32_t>(codec_params.payload_type);
- std::unique_ptr<RTCCodecStats> codec_stats(new RTCCodecStats(
- RTCCodecStatsIDFromMidDirectionAndPayload(mid, inbound, payload_type),
- timestamp_us));
+ std::unique_ptr<RTCCodecStats> codec_stats(
+ new RTCCodecStats(RTCCodecStatsIDFromTransportAndCodecParameters(
+ transport_id, inbound, codec_params),
+ timestamp_us));
codec_stats->payload_type = payload_type;
codec_stats->mime_type = codec_params.mime_type();
if (codec_params.clock_rate) {
@@ -421,7 +426,9 @@
}
std::unique_ptr<RTCInboundRTPStreamStats> CreateInboundAudioStreamStats(
+ const cricket::VoiceMediaInfo& voice_media_info,
const cricket::VoiceReceiverInfo& voice_receiver_info,
+ const std::string& transport_id,
const std::string& mid,
int64_t timestamp_us) {
auto inbound_audio = std::make_unique<RTCInboundRTPStreamStats>(
@@ -430,12 +437,18 @@
timestamp_us);
SetInboundRTPStreamStatsFromMediaReceiverInfo(voice_receiver_info,
inbound_audio.get());
+ inbound_audio->transport_id = transport_id;
inbound_audio->mid = mid;
inbound_audio->media_type = "audio";
inbound_audio->kind = "audio";
- if (voice_receiver_info.codec_payload_type) {
- inbound_audio->codec_id = RTCCodecStatsIDFromMidDirectionAndPayload(
- mid, /*inbound=*/true, *voice_receiver_info.codec_payload_type);
+ if (voice_receiver_info.codec_payload_type.has_value()) {
+ auto codec_param_it = voice_media_info.receive_codecs.find(
+ voice_receiver_info.codec_payload_type.value());
+ RTC_DCHECK(codec_param_it != voice_media_info.receive_codecs.end());
+ if (codec_param_it != voice_media_info.receive_codecs.end()) {
+ inbound_audio->codec_id = RTCCodecStatsIDFromTransportAndCodecParameters(
+ transport_id, /*inbound=*/true, codec_param_it->second);
+ }
}
inbound_audio->jitter = static_cast<double>(voice_receiver_info.jitter_ms) /
rtc::kNumMillisecsPerSec;
@@ -479,7 +492,7 @@
CreateRemoteOutboundAudioStreamStats(
const cricket::VoiceReceiverInfo& voice_receiver_info,
const std::string& mid,
- const std::string& inbound_audio_id,
+ const RTCInboundRTPStreamStats& inbound_audio_stats,
const std::string& transport_id) {
if (!voice_receiver_info.last_sender_report_timestamp_ms.has_value()) {
// Cannot create `RTCRemoteOutboundRtpStreamStats` when the RTCP SR arrival
@@ -501,15 +514,14 @@
stats->ssrc = voice_receiver_info.ssrc();
stats->kind = "audio";
stats->transport_id = transport_id;
- stats->codec_id = RTCCodecStatsIDFromMidDirectionAndPayload(
- mid,
- /*inbound=*/true, // Remote-outbound same as local-inbound.
- *voice_receiver_info.codec_payload_type);
+ if (inbound_audio_stats.codec_id.is_defined()) {
+ stats->codec_id = *inbound_audio_stats.codec_id;
+ }
// - RTCSentRtpStreamStats.
stats->packets_sent = voice_receiver_info.sender_reports_packets_sent;
stats->bytes_sent = voice_receiver_info.sender_reports_bytes_sent;
// - RTCRemoteOutboundRtpStreamStats.
- stats->local_id = inbound_audio_id;
+ stats->local_id = inbound_audio_stats.id();
RTC_DCHECK(
voice_receiver_info.last_sender_report_remote_timestamp_ms.has_value());
stats->remote_timestamp = static_cast<double>(
@@ -528,17 +540,25 @@
}
void SetInboundRTPStreamStatsFromVideoReceiverInfo(
+ const std::string& transport_id,
const std::string& mid,
+ const cricket::VideoMediaInfo& video_media_info,
const cricket::VideoReceiverInfo& video_receiver_info,
RTCInboundRTPStreamStats* inbound_video) {
SetInboundRTPStreamStatsFromMediaReceiverInfo(video_receiver_info,
inbound_video);
+ inbound_video->transport_id = transport_id;
inbound_video->mid = mid;
inbound_video->media_type = "video";
inbound_video->kind = "video";
- if (video_receiver_info.codec_payload_type) {
- inbound_video->codec_id = RTCCodecStatsIDFromMidDirectionAndPayload(
- mid, /*inbound=*/true, *video_receiver_info.codec_payload_type);
+ if (video_receiver_info.codec_payload_type.has_value()) {
+ auto codec_param_it = video_media_info.receive_codecs.find(
+ video_receiver_info.codec_payload_type.value());
+ RTC_DCHECK(codec_param_it != video_media_info.receive_codecs.end());
+ if (codec_param_it != video_media_info.receive_codecs.end()) {
+ inbound_video->codec_id = RTCCodecStatsIDFromTransportAndCodecParameters(
+ transport_id, /*inbound=*/true, codec_param_it->second);
+ }
}
inbound_video->jitter = static_cast<double>(video_receiver_info.jitter_ms) /
rtc::kNumMillisecsPerSec;
@@ -622,37 +642,53 @@
}
void SetOutboundRTPStreamStatsFromVoiceSenderInfo(
+ const std::string& transport_id,
const std::string& mid,
+ const cricket::VoiceMediaInfo& voice_media_info,
const cricket::VoiceSenderInfo& voice_sender_info,
RTCOutboundRTPStreamStats* outbound_audio) {
SetOutboundRTPStreamStatsFromMediaSenderInfo(voice_sender_info,
outbound_audio);
+ outbound_audio->transport_id = transport_id;
outbound_audio->mid = mid;
outbound_audio->media_type = "audio";
outbound_audio->kind = "audio";
if (voice_sender_info.target_bitrate > 0) {
outbound_audio->target_bitrate = voice_sender_info.target_bitrate;
}
- if (voice_sender_info.codec_payload_type) {
- outbound_audio->codec_id = RTCCodecStatsIDFromMidDirectionAndPayload(
- mid, /*inbound=*/false, *voice_sender_info.codec_payload_type);
+ if (voice_sender_info.codec_payload_type.has_value()) {
+ auto codec_param_it = voice_media_info.send_codecs.find(
+ voice_sender_info.codec_payload_type.value());
+ RTC_DCHECK(codec_param_it != voice_media_info.send_codecs.end());
+ if (codec_param_it != voice_media_info.send_codecs.end()) {
+ outbound_audio->codec_id = RTCCodecStatsIDFromTransportAndCodecParameters(
+ transport_id, /*inbound=*/false, codec_param_it->second);
+ }
}
// `fir_count`, `pli_count` and `sli_count` are only valid for video and are
// purposefully left undefined for audio.
}
void SetOutboundRTPStreamStatsFromVideoSenderInfo(
+ const std::string& transport_id,
const std::string& mid,
+ const cricket::VideoMediaInfo& video_media_info,
const cricket::VideoSenderInfo& video_sender_info,
RTCOutboundRTPStreamStats* outbound_video) {
SetOutboundRTPStreamStatsFromMediaSenderInfo(video_sender_info,
outbound_video);
+ outbound_video->transport_id = transport_id;
outbound_video->mid = mid;
outbound_video->media_type = "video";
outbound_video->kind = "video";
- if (video_sender_info.codec_payload_type) {
- outbound_video->codec_id = RTCCodecStatsIDFromMidDirectionAndPayload(
- mid, /*inbound=*/false, *video_sender_info.codec_payload_type);
+ if (video_sender_info.codec_payload_type.has_value()) {
+ auto codec_param_it = video_media_info.send_codecs.find(
+ video_sender_info.codec_payload_type.value());
+ RTC_DCHECK(codec_param_it != video_media_info.send_codecs.end());
+ if (codec_param_it != video_media_info.send_codecs.end()) {
+ outbound_video->codec_id = RTCCodecStatsIDFromTransportAndCodecParameters(
+ transport_id, /*inbound=*/false, codec_param_it->second);
+ }
}
outbound_video->fir_count =
static_cast<uint32_t>(video_sender_info.firs_rcvd);
@@ -1543,6 +1579,19 @@
RTC_DCHECK_RUN_ON(network_thread_);
rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
+ // For each transport, payload types does uniquely identify codecs, but the
+ // FMTP line could in theory be different on different m= sections. As such,
+ // the (PT,FMTP) pair on a per-transport basis uniquely identifies an
+ // RTCCodecStats. These maps are used to avoid duplicates.
+ // TODO(https://crbug.com/webrtc/14420): If we stop supporting different FMTP
+ // lines, this can be simplified to only looking at the set of PTs.
+ typedef std::pair<int, cricket::CodecParameterMap>
+ PayloadTypeAndCodecParametersMap;
+ std::map<std::string, std::set<PayloadTypeAndCodecParametersMap>>
+ send_codecs_by_transport;
+ std::map<std::string, std::set<PayloadTypeAndCodecParametersMap>>
+ receive_codecs_by_transport;
+
for (const auto& stats : transceiver_stats_infos) {
if (!stats.mid) {
continue;
@@ -1550,19 +1599,39 @@
std::string transport_id = RTCTransportStatsIDFromTransportChannel(
*stats.transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+ // Codecs (PT,FMTP) seen so far for this transport.
+ std::set<PayloadTypeAndCodecParametersMap>& send_codecs =
+ send_codecs_by_transport[transport_id];
+ std::set<PayloadTypeAndCodecParametersMap>& receive_codecs =
+ receive_codecs_by_transport[transport_id];
+
// Audio
if (stats.track_media_info_map.voice_media_info().has_value()) {
// Inbound
for (const auto& pair :
stats.track_media_info_map.voice_media_info()->receive_codecs) {
+ auto& codec = pair.second;
+ if (receive_codecs
+ .insert(PayloadTypeAndCodecParametersMap(codec.payload_type,
+ codec.parameters))
+ .second != true) {
+ continue; // (PT,FMTP) already seen.
+ }
report->AddStats(CodecStatsFromRtpCodecParameters(
- timestamp_us, *stats.mid, transport_id, true, pair.second));
+ timestamp_us, transport_id, true, codec));
}
// Outbound
for (const auto& pair :
stats.track_media_info_map.voice_media_info()->send_codecs) {
+ auto& codec = pair.second;
+ if (send_codecs
+ .insert(PayloadTypeAndCodecParametersMap(codec.payload_type,
+ codec.parameters))
+ .second != true) {
+ continue; // (PT,FMTP) already seen.
+ }
report->AddStats(CodecStatsFromRtpCodecParameters(
- timestamp_us, *stats.mid, transport_id, false, pair.second));
+ timestamp_us, transport_id, false, codec));
}
}
// Video
@@ -1570,14 +1639,28 @@
// Inbound
for (const auto& pair :
stats.track_media_info_map.video_media_info()->receive_codecs) {
+ auto& codec = pair.second;
+ if (receive_codecs
+ .insert(PayloadTypeAndCodecParametersMap(codec.payload_type,
+ codec.parameters))
+ .second != true) {
+ continue; // (PT,FMTP) already seen.
+ }
report->AddStats(CodecStatsFromRtpCodecParameters(
- timestamp_us, *stats.mid, transport_id, true, pair.second));
+ timestamp_us, transport_id, true, codec));
}
// Outbound
for (const auto& pair :
stats.track_media_info_map.video_media_info()->send_codecs) {
+ auto& codec = pair.second;
+ if (send_codecs
+ .insert(PayloadTypeAndCodecParametersMap(codec.payload_type,
+ codec.parameters))
+ .second != true) {
+ continue; // (PT,FMTP) already seen.
+ }
report->AddStats(CodecStatsFromRtpCodecParameters(
- timestamp_us, *stats.mid, transport_id, false, pair.second));
+ timestamp_us, transport_id, false, codec));
}
}
}
@@ -1924,8 +2007,9 @@
if (!voice_receiver_info.connected())
continue;
// Inbound.
- auto inbound_audio =
- CreateInboundAudioStreamStats(voice_receiver_info, mid, timestamp_us);
+ auto inbound_audio = CreateInboundAudioStreamStats(
+ stats.track_media_info_map.voice_media_info().value(),
+ voice_receiver_info, transport_id, mid, timestamp_us);
// TODO(hta): This lookup should look for the sender, not the track.
rtc::scoped_refptr<AudioTrackInterface> audio_track =
stats.track_media_info_map.GetAudioTrack(voice_receiver_info);
@@ -1937,10 +2021,9 @@
.value());
inbound_audio->track_identifier = audio_track->id();
}
- inbound_audio->transport_id = transport_id;
// Remote-outbound.
auto remote_outbound_audio = CreateRemoteOutboundAudioStreamStats(
- voice_receiver_info, mid, inbound_audio->id(), transport_id);
+ voice_receiver_info, mid, *inbound_audio.get(), transport_id);
// Add stats.
if (remote_outbound_audio) {
// When the remote outbound stats are available, the remote ID for the
@@ -1960,8 +2043,10 @@
RTCOutboundRTPStreamStatsIDFromSSRC(cricket::MEDIA_TYPE_AUDIO,
voice_sender_info.ssrc()),
timestamp_us);
- SetOutboundRTPStreamStatsFromVoiceSenderInfo(mid, voice_sender_info,
- outbound_audio.get());
+ SetOutboundRTPStreamStatsFromVoiceSenderInfo(
+ transport_id, mid,
+ stats.track_media_info_map.voice_media_info().value(),
+ voice_sender_info, outbound_audio.get());
rtc::scoped_refptr<AudioTrackInterface> audio_track =
stats.track_media_info_map.GetAudioTrack(voice_sender_info);
if (audio_track) {
@@ -1975,7 +2060,6 @@
RTCMediaSourceStatsIDFromKindAndAttachment(cricket::MEDIA_TYPE_AUDIO,
attachment_id);
}
- outbound_audio->transport_id = transport_id;
audio_outbound_rtps.insert(
std::make_pair(outbound_audio->id(), outbound_audio.get()));
report->AddStats(std::move(outbound_audio));
@@ -2018,8 +2102,10 @@
RTCInboundRTPStreamStatsIDFromSSRC(cricket::MEDIA_TYPE_VIDEO,
video_receiver_info.ssrc()),
timestamp_us);
- SetInboundRTPStreamStatsFromVideoReceiverInfo(mid, video_receiver_info,
- inbound_video.get());
+ SetInboundRTPStreamStatsFromVideoReceiverInfo(
+ transport_id, mid,
+ stats.track_media_info_map.video_media_info().value(),
+ video_receiver_info, inbound_video.get());
rtc::scoped_refptr<VideoTrackInterface> video_track =
stats.track_media_info_map.GetVideoTrack(video_receiver_info);
if (video_track) {
@@ -2030,7 +2116,6 @@
.value());
inbound_video->track_identifier = video_track->id();
}
- inbound_video->transport_id = transport_id;
report->AddStats(std::move(inbound_video));
}
// Outbound
@@ -2043,8 +2128,10 @@
RTCOutboundRTPStreamStatsIDFromSSRC(cricket::MEDIA_TYPE_VIDEO,
video_sender_info.ssrc()),
timestamp_us);
- SetOutboundRTPStreamStatsFromVideoSenderInfo(mid, video_sender_info,
- outbound_video.get());
+ SetOutboundRTPStreamStatsFromVideoSenderInfo(
+ transport_id, mid,
+ stats.track_media_info_map.video_media_info().value(),
+ video_sender_info, outbound_video.get());
rtc::scoped_refptr<VideoTrackInterface> video_track =
stats.track_media_info_map.GetVideoTrack(video_sender_info);
if (video_track) {
@@ -2058,7 +2145,6 @@
RTCMediaSourceStatsIDFromKindAndAttachment(cricket::MEDIA_TYPE_VIDEO,
attachment_id);
}
- outbound_video->transport_id = transport_id;
video_outbound_rtps.insert(
std::make_pair(outbound_video->id(), outbound_video.get()));
report->AddStats(std::move(outbound_video));
