diff --git a/media/engine/webrtcvoiceengine.cc b/media/engine/webrtcvoiceengine.cc index 00c7fd61fc..881e8ec831 100644 --- a/media/engine/webrtcvoiceengine.cc +++ b/media/engine/webrtcvoiceengine.cc @@ -2265,6 +2265,23 @@ bool WebRtcVoiceMediaChannel::GetStats(VoiceMediaInfo* info) { // Get SSRC and stats for each receiver. RTC_DCHECK_EQ(info->receivers.size(), 0U); for (const auto& stream : recv_streams_) { + uint32_t ssrc = stream.first; + // When SSRCs are unsignaled, there's only one audio MediaStreamTrack, but + // multiple RTP streams can be received over time (if the SSRC changes for + // whatever reason). We only want the RTCMediaStreamTrackStats to represent + // the stats for the most recent stream (the one whose audio is actually + // routed to the MediaStreamTrack), so here we ignore any unsignaled SSRCs + // except for the most recent one (last in the vector). This is somewhat of + // a hack, and means you don't get *any* stats for these inactive streams, + // but it's slightly better than the previous behavior, which was "highest + // SSRC wins". + // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=8158 + if (!unsignaled_recv_ssrcs_.empty()) { + auto end_it = --unsignaled_recv_ssrcs_.end(); + if (std::find(unsignaled_recv_ssrcs_.begin(), end_it, ssrc) != end_it) { + continue; + } + } webrtc::AudioReceiveStream::Stats stats = stream.second->GetStats(); VoiceReceiverInfo rinfo; rinfo.add_ssrc(stats.remote_ssrc); diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index fa99d38955..641b2fa7a5 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -1988,6 +1988,97 @@ TEST_F(PeerConnectionIntegrationTest, EXPECT_TRUE(media_stats[audio_index]->audio_level.is_defined()); } +// Helper for test below. +void ModifySsrcs(cricket::SessionDescription* desc) { + for (ContentInfo& content : desc->contents()) { + MediaContentDescription* media_desc = + static_cast(content.description); + for (cricket::StreamParams& stream : media_desc->mutable_streams()) { + for (uint32_t& ssrc : stream.ssrcs) { + ssrc = rtc::CreateRandomId(); + } + } + } +} + +// Test that the "RTCMediaSteamTrackStats" object is updated correctly when +// SSRCs are unsignaled, and the SSRC of the received (audio) stream changes. +// This should result in two "RTCInboundRTPStreamStats", but only one +// "RTCMediaStreamTrackStats", whose counters go up continuously rather than +// being reset to 0 once the SSRC change occurs. +// +// Regression test for this bug: +// https://bugs.chromium.org/p/webrtc/issues/detail?id=8158 +// +// The bug causes the track stats to only represent one of the two streams: +// whichever one has the higher SSRC. So with this bug, there was a 50% chance +// that the track stat counters would reset to 0 when the new stream is +// received, and a 50% chance that they'll stop updating (while +// "concealed_samples" continues increasing, due to silence being generated for +// the inactive stream). +TEST_F(PeerConnectionIntegrationTest, + TrackStatsUpdatedCorrectlyWhenUnsignaledSsrcChanges) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddAudioOnlyMediaStream(); + // Remove SSRCs and MSIDs from the received offer SDP, simulating an endpoint + // that doesn't signal SSRCs (from the callee's perspective). + callee()->SetReceivedSdpMunger(RemoveSsrcsAndMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + // Wait for 50 audio frames (500ms of audio) to be received by the callee. + ExpectNewFramesReceivedWithWait(0, 0, 25, 0, kMaxWaitForFramesMs); + + // Some audio frames were received, so we should have nonzero "samples + // received" for the track. + rtc::scoped_refptr report = + callee()->NewGetStats(); + ASSERT_NE(nullptr, report); + auto track_stats = report->GetStatsOfType(); + ASSERT_EQ(1U, track_stats.size()); + ASSERT_TRUE(track_stats[0]->total_samples_received.is_defined()); + ASSERT_GT(*track_stats[0]->total_samples_received, 0U); + // uint64_t prev_samples_received = *track_stats[0]->total_samples_received; + + // Create a new offer and munge it to cause the caller to use a new SSRC. + caller()->SetGeneratedSdpMunger(ModifySsrcs); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + // Wait for 25 more audio frames (250ms of audio) to be received, from the new + // SSRC. + ExpectNewFramesReceivedWithWait(0, 0, 25, 0, kMaxWaitForFramesMs); + + report = callee()->NewGetStats(); + ASSERT_NE(nullptr, report); + track_stats = report->GetStatsOfType(); + ASSERT_EQ(1U, track_stats.size()); + ASSERT_TRUE(track_stats[0]->total_samples_received.is_defined()); + // The "total samples received" stat should only be greater than it was + // before. + // TODO(deadbeef): Uncomment this assertion once the bug is completely fixed. + // Right now, the new SSRC will cause the counters to reset to 0. + // EXPECT_GT(*track_stats[0]->total_samples_received, prev_samples_received); + + // Additionally, the percentage of concealed samples (samples generated to + // conceal packet loss) should be less than 25%%. If it's greater, that's a + // good sign that we're seeing stats from the old stream that's no longer + // receiving packets, and is generating concealed samples of silence. + constexpr double kAcceptableConcealedSamplesPercentage = 0.25; + ASSERT_TRUE(track_stats[0]->concealed_samples.is_defined()); + EXPECT_LT(*track_stats[0]->concealed_samples, + *track_stats[0]->total_samples_received * + kAcceptableConcealedSamplesPercentage); + + // Also ensure that we have two "RTCInboundRTPStreamStats" as expected, as a + // sanity check that the SSRC really changed. + // TODO(deadbeef): This isn't working right now, because we're not returning + // *any* stats for the inactive stream. Uncomment when the bug is completely + // fixed. + // auto inbound_stream_stats = + // report->GetStatsOfType(); + // ASSERT_EQ(2U, inbound_stream_stats.size()); +} + // Test that DTLS 1.0 is used if both sides only support DTLS 1.0. TEST_F(PeerConnectionIntegrationTest, EndToEndCallWithDtls10) { PeerConnectionFactory::Options dtls_10_options;