Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(249)

Unified Diff: pc/peerconnection_integrationtest.cc

Issue 3008373002: Only return stats for the most recent unsignaled audio stream. (Closed)
Patch Set: Increase test duration and weaken failure criteria. Created 3 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « media/engine/webrtcvoiceengine.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: pc/peerconnection_integrationtest.cc
diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc
index fa99d389557a1ead5a5056ba58b1639cf0cf7831..641b2fa7a557c8695afcd4993cd744a42fa2554c 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<MediaContentDescription*>(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<const webrtc::RTCStatsReport> report =
+ callee()->NewGetStats();
+ ASSERT_NE(nullptr, report);
+ auto track_stats = report->GetStatsOfType<webrtc::RTCMediaStreamTrackStats>();
+ 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<webrtc::RTCMediaStreamTrackStats>();
+ 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<webrtc::RTCInboundRTPStreamStats>();
+ // 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;
« no previous file with comments | « media/engine/webrtcvoiceengine.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698