From a27cfbffdfa0bf359628d2164db5b9d6321f9c9c Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Sun, 20 Jun 2021 14:59:48 -0700 Subject: [PATCH] Fix echo return loss stats and add to RTCAudioSourceStats. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This solves two problems: * Echo return loss stats weren't being gathered in Chrome, because they need to be taken from the audio processor attached to the track rather than the audio send stream. * The standardized location is in RTCAudioSourceStats, not RTCMediaStreamTrackStats. For now, will populate the stats in both locations. Bug: webrtc:12770 Change-Id: I47eaf7f2b50b914a1be84156aa831e27497d07e3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/223182 Commit-Queue: Taylor Brandstetter Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#34344} --- api/stats/rtcstats_objects.h | 2 + pc/rtc_stats_collector.cc | 47 ++++++++++++--- pc/rtc_stats_collector_unittest.cc | 96 ++++++++++++++++++++++++++++-- pc/rtc_stats_integrationtest.cc | 6 ++ stats/rtcstats_objects.cc | 12 +++- 5 files changed, 148 insertions(+), 15 deletions(-) diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index fe6d65832b..cb8e25a02e 100644 --- a/api/stats/rtcstats_objects.h +++ b/api/stats/rtcstats_objects.h @@ -629,6 +629,8 @@ class RTC_EXPORT RTCAudioSourceStats final : public RTCMediaSourceStats { RTCStatsMember audio_level; RTCStatsMember total_audio_energy; RTCStatsMember total_samples_duration; + RTCStatsMember echo_return_loss; + RTCStatsMember echo_return_loss_enhancement; }; // https://w3c.github.io/webrtc-stats/#dom-rtcvideosourcestats diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 4b38abc073..2049d835a2 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -743,10 +743,22 @@ const std::string& ProduceIceCandidateStats(int64_t timestamp_us, return stats->id(); } +template +void SetAudioProcessingStats(StatsType* stats, + const AudioProcessingStats& apm_stats) { + if (apm_stats.echo_return_loss) { + stats->echo_return_loss = *apm_stats.echo_return_loss; + } + if (apm_stats.echo_return_loss_enhancement) { + stats->echo_return_loss_enhancement = + *apm_stats.echo_return_loss_enhancement; + } +} + std::unique_ptr ProduceMediaStreamTrackStatsFromVoiceSenderInfo( int64_t timestamp_us, - const AudioTrackInterface& audio_track, + AudioTrackInterface& audio_track, const cricket::VoiceSenderInfo& voice_sender_info, int attachment_id) { std::unique_ptr audio_track_stats( @@ -761,13 +773,17 @@ ProduceMediaStreamTrackStatsFromVoiceSenderInfo( attachment_id); audio_track_stats->remote_source = false; audio_track_stats->detached = false; - if (voice_sender_info.apm_statistics.echo_return_loss) { - audio_track_stats->echo_return_loss = - *voice_sender_info.apm_statistics.echo_return_loss; - } - if (voice_sender_info.apm_statistics.echo_return_loss_enhancement) { - audio_track_stats->echo_return_loss_enhancement = - *voice_sender_info.apm_statistics.echo_return_loss_enhancement; + // Audio processor may be attached to either the track or the send + // stream, so look in both places. + SetAudioProcessingStats(audio_track_stats.get(), + voice_sender_info.apm_statistics); + auto audio_processor(audio_track.GetAudioProcessor()); + if (audio_processor.get()) { + // The |has_remote_tracks| argument is obsolete; makes no difference if it's + // set to true or false. + AudioProcessorInterface::AudioProcessorStatistics ap_stats = + audio_processor->GetStats(/*has_remote_tracks=*/false); + SetAudioProcessingStats(audio_track_stats.get(), ap_stats.apm_statistics); } return audio_track_stats; } @@ -1657,6 +1673,8 @@ void RTCStatsCollector::ProduceMediaSourceStats_s( // create separate media source stats objects on a per-attachment basis. std::unique_ptr media_source_stats; if (track->kind() == MediaStreamTrackInterface::kAudioKind) { + AudioTrackInterface* audio_track = + static_cast(track.get()); auto audio_source_stats = std::make_unique( RTCMediaSourceStatsIDFromKindAndAttachment( cricket::MEDIA_TYPE_AUDIO, sender_internal->AttachmentId()), @@ -1677,8 +1695,21 @@ void RTCStatsCollector::ProduceMediaSourceStats_s( voice_sender_info->total_input_energy; audio_source_stats->total_samples_duration = voice_sender_info->total_input_duration; + SetAudioProcessingStats(audio_source_stats.get(), + voice_sender_info->apm_statistics); } } + // Audio processor may be attached to either the track or the send + // stream, so look in both places. + auto audio_processor(audio_track->GetAudioProcessor()); + if (audio_processor.get()) { + // The |has_remote_tracks| argument is obsolete; makes no difference + // if it's set to true or false. + AudioProcessorInterface::AudioProcessorStatistics ap_stats = + audio_processor->GetStats(/*has_remote_tracks=*/false); + SetAudioProcessingStats(audio_source_stats.get(), + ap_stats.apm_statistics); + } media_source_stats = std::move(audio_source_stats); } else { RTC_DCHECK_EQ(MediaStreamTrackInterface::kVideoKind, track->kind()); diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 92f73770d8..34884604e1 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -200,14 +200,34 @@ std::unique_ptr CreateFakeCandidate( return candidate; } +class FakeAudioProcessor : public AudioProcessorInterface { + public: + FakeAudioProcessor() {} + ~FakeAudioProcessor() {} + + private: + AudioProcessorInterface::AudioProcessorStatistics GetStats( + bool has_recv_streams) override { + AudioProcessorStatistics stats; + stats.apm_statistics.echo_return_loss = 2.0; + stats.apm_statistics.echo_return_loss_enhancement = 3.0; + return stats; + } +}; + class FakeAudioTrackForStats : public MediaStreamTrack { public: static rtc::scoped_refptr Create( const std::string& id, - MediaStreamTrackInterface::TrackState state) { + MediaStreamTrackInterface::TrackState state, + bool create_fake_audio_processor) { rtc::scoped_refptr audio_track_stats( new rtc::RefCountedObject(id)); audio_track_stats->set_state(state); + if (create_fake_audio_processor) { + audio_track_stats->processor_ = + rtc::make_ref_counted(); + } return audio_track_stats; } @@ -222,8 +242,11 @@ class FakeAudioTrackForStats : public MediaStreamTrack { void RemoveSink(webrtc::AudioTrackSinkInterface* sink) override {} bool GetSignalLevel(int* level) override { return false; } rtc::scoped_refptr GetAudioProcessor() override { - return nullptr; + return processor_; } + + private: + rtc::scoped_refptr processor_; }; class FakeVideoTrackSourceForStats : public VideoTrackSourceInterface { @@ -308,9 +331,11 @@ class FakeVideoTrackForStats : public MediaStreamTrack { rtc::scoped_refptr CreateFakeTrack( cricket::MediaType media_type, const std::string& track_id, - MediaStreamTrackInterface::TrackState track_state) { + MediaStreamTrackInterface::TrackState track_state, + bool create_fake_audio_processor = false) { if (media_type == cricket::MEDIA_TYPE_AUDIO) { - return FakeAudioTrackForStats::Create(track_id, track_state); + return FakeAudioTrackForStats::Create(track_id, track_state, + create_fake_audio_processor); } else { RTC_DCHECK_EQ(media_type, cricket::MEDIA_TYPE_VIDEO); return FakeVideoTrackForStats::Create(track_id, track_state, nullptr); @@ -2580,6 +2605,9 @@ TEST_F(RTCStatsCollectorTest, RTCAudioSourceStatsCollectedForSenderWithTrack) { voice_media_info.senders[0].audio_level = 32767; // [0,32767] voice_media_info.senders[0].total_input_energy = 2.0; voice_media_info.senders[0].total_input_duration = 3.0; + voice_media_info.senders[0].apm_statistics.echo_return_loss = 42.0; + voice_media_info.senders[0].apm_statistics.echo_return_loss_enhancement = + 52.0; auto* voice_media_channel = pc_->AddVoiceChannel("AudioMid", "TransportName"); voice_media_channel->SetStats(voice_media_info); stats_->SetupLocalTrackAndSender(cricket::MEDIA_TYPE_AUDIO, @@ -2595,6 +2623,8 @@ TEST_F(RTCStatsCollectorTest, RTCAudioSourceStatsCollectedForSenderWithTrack) { expected_audio.audio_level = 1.0; // [0,1] expected_audio.total_audio_energy = 2.0; expected_audio.total_samples_duration = 3.0; + expected_audio.echo_return_loss = 42.0; + expected_audio.echo_return_loss_enhancement = 52.0; ASSERT_TRUE(report->Get(expected_audio.id())); EXPECT_EQ(report->Get(expected_audio.id())->cast_to(), @@ -3056,6 +3086,64 @@ TEST_F(RTCStatsCollectorTest, EXPECT_FALSE(report->Get("RTCVideoSource_42")); } +// Test collecting echo return loss stats from the audio processor attached to +// the track, rather than the voice sender info. +TEST_F(RTCStatsCollectorTest, CollectEchoReturnLossFromTrackAudioProcessor) { + rtc::scoped_refptr local_stream = + MediaStream::Create("LocalStreamId"); + pc_->mutable_local_streams()->AddStream(local_stream); + + // Local audio track + rtc::scoped_refptr local_audio_track = + CreateFakeTrack(cricket::MEDIA_TYPE_AUDIO, "LocalAudioTrackID", + MediaStreamTrackInterface::kEnded, + /*create_fake_audio_processor=*/true); + local_stream->AddTrack( + static_cast(local_audio_track.get())); + + cricket::VoiceSenderInfo voice_sender_info_ssrc1; + voice_sender_info_ssrc1.local_stats.push_back(cricket::SsrcSenderInfo()); + voice_sender_info_ssrc1.local_stats[0].ssrc = 1; + + stats_->CreateMockRtpSendersReceiversAndChannels( + {std::make_pair(local_audio_track.get(), voice_sender_info_ssrc1)}, {}, + {}, {}, {local_stream->id()}, {}); + + rtc::scoped_refptr report = stats_->GetStatsReport(); + + RTCMediaStreamTrackStats expected_local_audio_track_ssrc1( + IdForType(report), report->timestamp_us(), + RTCMediaStreamTrackKind::kAudio); + expected_local_audio_track_ssrc1.track_identifier = local_audio_track->id(); + expected_local_audio_track_ssrc1.media_source_id = + "RTCAudioSource_11"; // Attachment ID = SSRC + 10 + expected_local_audio_track_ssrc1.remote_source = false; + expected_local_audio_track_ssrc1.ended = true; + expected_local_audio_track_ssrc1.detached = false; + expected_local_audio_track_ssrc1.echo_return_loss = 2.0; + expected_local_audio_track_ssrc1.echo_return_loss_enhancement = 3.0; + ASSERT_TRUE(report->Get(expected_local_audio_track_ssrc1.id())) + << "Did not find " << expected_local_audio_track_ssrc1.id() << " in " + << report->ToJson(); + EXPECT_EQ(expected_local_audio_track_ssrc1, + report->Get(expected_local_audio_track_ssrc1.id()) + ->cast_to()); + + RTCAudioSourceStats expected_audio("RTCAudioSource_11", + report->timestamp_us()); + expected_audio.track_identifier = "LocalAudioTrackID"; + expected_audio.kind = "audio"; + expected_audio.audio_level = 0; + expected_audio.total_audio_energy = 0; + expected_audio.total_samples_duration = 0; + expected_audio.echo_return_loss = 2.0; + expected_audio.echo_return_loss_enhancement = 3.0; + + ASSERT_TRUE(report->Get(expected_audio.id())); + EXPECT_EQ(report->Get(expected_audio.id())->cast_to(), + expected_audio); +} + TEST_F(RTCStatsCollectorTest, GetStatsWithSenderSelector) { ExampleStatsGraph graph = SetupExampleStatsGraphForSelectorTests(); // Expected stats graph when filtered by sender: diff --git a/pc/rtc_stats_integrationtest.cc b/pc/rtc_stats_integrationtest.cc index 032cbe9592..90be60003f 100644 --- a/pc/rtc_stats_integrationtest.cc +++ b/pc/rtc_stats_integrationtest.cc @@ -1082,6 +1082,12 @@ class RTCStatsReportVerifier { verifier.TestMemberIsNonNegative(audio_source.audio_level); verifier.TestMemberIsPositive(audio_source.total_audio_energy); verifier.TestMemberIsPositive(audio_source.total_samples_duration); + // TODO(hbos): |echo_return_loss| and |echo_return_loss_enhancement| are + // flaky on msan bot (sometimes defined, sometimes undefined). Should the + // test run until available or is there a way to have it always be + // defined? crbug.com/627816 + verifier.MarkMemberTested(audio_source.echo_return_loss, true); + verifier.MarkMemberTested(audio_source.echo_return_loss_enhancement, true); return verifier.ExpectAllMembersSuccessfullyTested(); } diff --git a/stats/rtcstats_objects.cc b/stats/rtcstats_objects.cc index 0db833c7fa..a2d7aa0b07 100644 --- a/stats/rtcstats_objects.cc +++ b/stats/rtcstats_objects.cc @@ -987,7 +987,9 @@ RTCMediaSourceStats::~RTCMediaSourceStats() {} WEBRTC_RTCSTATS_IMPL(RTCAudioSourceStats, RTCMediaSourceStats, "media-source", &audio_level, &total_audio_energy, - &total_samples_duration) + &total_samples_duration, + &echo_return_loss, + &echo_return_loss_enhancement) // clang-format on RTCAudioSourceStats::RTCAudioSourceStats(const std::string& id, @@ -998,13 +1000,17 @@ RTCAudioSourceStats::RTCAudioSourceStats(std::string&& id, int64_t timestamp_us) : RTCMediaSourceStats(std::move(id), timestamp_us), audio_level("audioLevel"), total_audio_energy("totalAudioEnergy"), - total_samples_duration("totalSamplesDuration") {} + total_samples_duration("totalSamplesDuration"), + echo_return_loss("echoReturnLoss"), + echo_return_loss_enhancement("echoReturnLossEnhancement") {} RTCAudioSourceStats::RTCAudioSourceStats(const RTCAudioSourceStats& other) : RTCMediaSourceStats(other), audio_level(other.audio_level), total_audio_energy(other.total_audio_energy), - total_samples_duration(other.total_samples_duration) {} + total_samples_duration(other.total_samples_duration), + echo_return_loss(other.echo_return_loss), + echo_return_loss_enhancement(other.echo_return_loss_enhancement) {} RTCAudioSourceStats::~RTCAudioSourceStats() {}