Fixed the stats problem when new track is using the same ssrc as the previous track.
Before this patch, when switching from voice mode to stereo mode, the stats won't be updated because StatsCollector binded the ssrc report with the old track, so the report can't be updated by the new track.
This patch fixes the porblem by changing the ssrc report track id to use the new track id.
TEST=libjingle_peerconnection_unittest --gtest_filter="*StatsCollectorTest*"
R=hta@chromium.org
Review URL: https://webrtc-codereview.appspot.com/17859004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@6632 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc
index 74fc3a7..4c3f7de 100644
--- a/talk/app/webrtc/statscollector.cc
+++ b/talk/app/webrtc/statscollector.cc
@@ -270,17 +270,20 @@
return false;
}
+void AddTrackReport(StatsMap* reports, const std::string& track_id) {
+ // Adds an empty track report.
+ StatsReport report;
+ report.type = StatsReport::kStatsReportTypeTrack;
+ report.id = StatsId(StatsReport::kStatsReportTypeTrack, track_id);
+ report.AddValue(StatsReport::kStatsValueNameTrackId, track_id);
+ (*reports)[report.id] = report;
+}
+
template <class TrackVector>
void CreateTrackReports(const TrackVector& tracks, StatsMap* reports) {
for (size_t j = 0; j < tracks.size(); ++j) {
webrtc::MediaStreamTrackInterface* track = tracks[j];
- // Adds an empty track report.
- StatsReport report;
- report.type = StatsReport::kStatsReportTypeTrack;
- report.id = StatsId(StatsReport::kStatsReportTypeTrack, track->id());
- report.AddValue(StatsReport::kStatsValueNameTrackId,
- track->id());
- (*reports)[report.id] = report;
+ AddTrackReport(reports, track->id());
}
}
@@ -543,13 +546,19 @@
void StatsCollector::AddLocalAudioTrack(AudioTrackInterface* audio_track,
uint32 ssrc) {
ASSERT(audio_track != NULL);
-#ifdef _DEBUG
for (LocalAudioTrackVector::iterator it = local_audio_tracks_.begin();
it != local_audio_tracks_.end(); ++it) {
ASSERT(it->first != audio_track || it->second != ssrc);
}
-#endif
+
local_audio_tracks_.push_back(std::make_pair(audio_track, ssrc));
+
+ // Create the kStatsReportTypeTrack report for the new track if there is no
+ // report yet.
+ StatsMap::iterator it = reports_.find(
+ StatsId(StatsReport::kStatsReportTypeTrack, audio_track->id()));
+ if (it == reports_.end())
+ AddTrackReport(&reports_, audio_track->id());
}
void StatsCollector::RemoveLocalAudioTrack(AudioTrackInterface* audio_track,
@@ -617,7 +626,8 @@
// Calls to UpdateStats() that occur less than kMinGatherStatsPeriod number of
// ms apart will be ignored.
const double kMinGatherStatsPeriod = 50;
- if (stats_gathering_started_ + kMinGatherStatsPeriod > time_now) {
+ if (stats_gathering_started_ != 0 &&
+ stats_gathering_started_ + kMinGatherStatsPeriod > time_now) {
return;
}
stats_gathering_started_ = time_now;
@@ -637,13 +647,17 @@
StatsMap::iterator it = reports_.find(StatsId(
StatsReport::kStatsReportTypeSsrc, ssrc_id, direction));
+ // Use the ID of the track that is currently mapped to the SSRC, if any.
std::string track_id;
- if (it == reports_.end()) {
- if (!GetTrackIdBySsrc(ssrc, &track_id, direction))
+ if (!GetTrackIdBySsrc(ssrc, &track_id, direction)) {
+ if (it == reports_.end()) {
+ // The ssrc is not used by any track or existing report, return NULL
+ // in such case to indicate no report is prepared for the ssrc.
return NULL;
- } else {
- // Keeps the old track id since we want to report the stats for inactive
- // tracks.
+ }
+
+ // The ssrc is not used by any existing track. Keeps the old track id
+ // since we want to report the stats for inactive ssrc.
ExtractValueFromReport(it->second,
StatsReport::kStatsValueNameTrackId,
&track_id);
@@ -653,10 +667,12 @@
ssrc_id, direction);
// Clear out stats from previous GatherStats calls if any.
- if (report->timestamp != stats_gathering_started_) {
- report->values.clear();
- report->timestamp = stats_gathering_started_;
- }
+ // This is required since the report will be returned for the new values.
+ // Having the old values in the report will lead to multiple values with
+ // the same name.
+ // TODO(xians): Consider changing StatsReport to use map instead of vector.
+ report->values.clear();
+ report->timestamp = stats_gathering_started_;
report->AddValue(StatsReport::kStatsValueNameSsrc, ssrc_id);
report->AddValue(StatsReport::kStatsValueNameTrackId, track_id);
@@ -674,13 +690,17 @@
StatsMap::iterator it = reports_.find(StatsId(
StatsReport::kStatsReportTypeRemoteSsrc, ssrc_id, direction));
+ // Use the ID of the track that is currently mapped to the SSRC, if any.
std::string track_id;
- if (it == reports_.end()) {
- if (!GetTrackIdBySsrc(ssrc, &track_id, direction))
+ if (!GetTrackIdBySsrc(ssrc, &track_id, direction)) {
+ if (it == reports_.end()) {
+ // The ssrc is not used by any track or existing report, return NULL
+ // in such case to indicate no report is prepared for the ssrc.
return NULL;
- } else {
- // Keeps the old track id since we want to report the stats for inactive
- // tracks.
+ }
+
+ // The ssrc is not used by any existing track. Keeps the old track id
+ // since we want to report the stats for inactive ssrc.
ExtractValueFromReport(it->second,
StatsReport::kStatsValueNameTrackId,
&track_id);
@@ -1060,4 +1080,8 @@
return true;
}
+void StatsCollector::ClearUpdateStatsCache() {
+ stats_gathering_started_ = 0;
+}
+
} // namespace webrtc
diff --git a/talk/app/webrtc/statscollector.h b/talk/app/webrtc/statscollector.h
index 2516904..b3ed1ed 100644
--- a/talk/app/webrtc/statscollector.h
+++ b/talk/app/webrtc/statscollector.h
@@ -89,6 +89,11 @@
bool GetTransportIdFromProxy(const std::string& proxy,
std::string* transport_id);
+ // Method used by the unittest to force a update of stats since UpdateStats()
+ // that occur less than kMinGatherStatsPeriod number of ms apart will be
+ // ignored.
+ void ClearUpdateStatsCache();
+
private:
bool CopySelectedReports(const std::string& selector, StatsReports* reports);
diff --git a/talk/app/webrtc/statscollector_unittest.cc b/talk/app/webrtc/statscollector_unittest.cc
index 2cd716e..af45038 100644
--- a/talk/app/webrtc/statscollector_unittest.cc
+++ b/talk/app/webrtc/statscollector_unittest.cc
@@ -465,8 +465,6 @@
stream_->AddTrack(track_);
EXPECT_CALL(session_, GetLocalTrackIdBySsrc(kSsrcOfTrack, _))
.WillRepeatedly(DoAll(SetArgPointee<1>(kLocalTrackId), Return(true)));
- EXPECT_CALL(session_, GetRemoteTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(Return(false));
}
// Adds a incoming video track with a given SSRC into the stats.
@@ -474,11 +472,8 @@
stream_ = webrtc::MediaStream::Create("streamlabel");
track_= webrtc::VideoTrack::Create(kRemoteTrackId, NULL);
stream_->AddTrack(track_);
- EXPECT_CALL(session_, GetLocalTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(Return(false));
EXPECT_CALL(session_, GetRemoteTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(DoAll(SetArgPointee<1>(kRemoteTrackId),
- Return(true)));
+ .WillRepeatedly(DoAll(SetArgPointee<1>(kRemoteTrackId), Return(true)));
}
// Adds a outgoing audio track with a given SSRC into the stats.
@@ -490,10 +485,7 @@
kLocalTrackId);
stream_->AddTrack(audio_track_);
EXPECT_CALL(session_, GetLocalTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(DoAll(SetArgPointee<1>(kLocalTrackId),
- Return(true)));
- EXPECT_CALL(session_, GetRemoteTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(Return(false));
+ .WillOnce(DoAll(SetArgPointee<1>(kLocalTrackId), Return(true)));
}
// Adds a incoming audio track with a given SSRC into the stats.
@@ -504,10 +496,84 @@
audio_track_ = new talk_base::RefCountedObject<FakeAudioTrack>(
kRemoteTrackId);
stream_->AddTrack(audio_track_);
- EXPECT_CALL(session_, GetLocalTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(Return(false));
EXPECT_CALL(session_, GetRemoteTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(DoAll(SetArgPointee<1>(kRemoteTrackId), Return(true)));
+ .WillOnce(DoAll(SetArgPointee<1>(kRemoteTrackId), Return(true)));
+ }
+
+ void SetupAndVerifyAudioTrackStats(
+ FakeAudioTrack* audio_track,
+ webrtc::MediaStream* stream,
+ webrtc::StatsCollector* stats,
+ cricket::VoiceChannel* voice_channel,
+ const std::string& vc_name,
+ MockVoiceMediaChannel* media_channel,
+ cricket::VoiceSenderInfo* voice_sender_info,
+ cricket::VoiceReceiverInfo* voice_receiver_info,
+ cricket::VoiceMediaInfo* stats_read,
+ StatsReports* reports) {
+ // A track can't have both sender report and recv report at the same time
+ // for now, this might change in the future though.
+ ASSERT((voice_sender_info == NULL) ^ (voice_receiver_info == NULL));
+ stats->set_session(&session_);
+
+ // Instruct the session to return stats containing the transport channel.
+ InitSessionStats(vc_name);
+ EXPECT_CALL(session_, GetStats(_))
+ .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_),
+ Return(true)));
+
+ // Constructs an ssrc stats update.
+ if (voice_sender_info)
+ stats_read->senders.push_back(*voice_sender_info);
+ if (voice_receiver_info)
+ stats_read->receivers.push_back(*voice_receiver_info);
+
+ EXPECT_CALL(session_, voice_channel()).WillRepeatedly(
+ Return(voice_channel));
+ EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
+ EXPECT_CALL(*media_channel, GetStats(_))
+ .WillOnce(DoAll(SetArgPointee<0>(*stats_read), Return(true)));
+
+ stats->UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
+ stats->ClearUpdateStatsCache();
+ stats->GetStats(NULL, reports);
+
+ // Verify the existence of the track report.
+ const StatsReport* report = FindNthReportByType(
+ *reports, StatsReport::kStatsReportTypeSsrc, 1);
+ EXPECT_FALSE(report == NULL);
+ std::string track_id = ExtractSsrcStatsValue(
+ *reports, StatsReport::kStatsValueNameTrackId);
+ EXPECT_EQ(audio_track->id(), track_id);
+ std::string ssrc_id = ExtractSsrcStatsValue(
+ *reports, StatsReport::kStatsValueNameSsrc);
+ EXPECT_EQ(talk_base::ToString<uint32>(kSsrcOfTrack), ssrc_id);
+
+ // Verifies the values in the track report.
+ if (voice_sender_info) {
+ UpdateVoiceSenderInfoFromAudioTrack(audio_track, voice_sender_info);
+ VerifyVoiceSenderInfoReport(report, *voice_sender_info);
+ }
+ if (voice_receiver_info) {
+ VerifyVoiceReceiverInfoReport(report, *voice_receiver_info);
+ }
+
+ // Verify we get the same result by passing a track to GetStats().
+ StatsReports track_reports; // returned values.
+ stats->GetStats(audio_track, &track_reports);
+ const StatsReport* track_report = FindNthReportByType(
+ track_reports, StatsReport::kStatsReportTypeSsrc, 1);
+ EXPECT_TRUE(track_report);
+ track_id = ExtractSsrcStatsValue(track_reports,
+ StatsReport::kStatsValueNameTrackId);
+ EXPECT_EQ(audio_track->id(), track_id);
+ ssrc_id = ExtractSsrcStatsValue(track_reports,
+ StatsReport::kStatsValueNameSsrc);
+ EXPECT_EQ(talk_base::ToString<uint32>(kSsrcOfTrack), ssrc_id);
+ if (voice_sender_info)
+ VerifyVoiceSenderInfoReport(track_report, *voice_sender_info);
+ if (voice_receiver_info)
+ VerifyVoiceReceiverInfoReport(track_report, *voice_receiver_info);
}
void TestCertificateReports(const talk_base::FakeSSLCertificate& local_cert,
@@ -1171,61 +1237,16 @@
media_engine_, media_channel, &session_, kVcName, false);
AddOutgoingAudioTrackStats();
stats.AddStream(stream_);
- stats.AddLocalAudioTrack(audio_track_.get(), kSsrcOfTrack);
-
- stats.set_session(&session_);
-
- // Instruct the session to return stats containing the transport channel.
- InitSessionStats(kVcName);
- EXPECT_CALL(session_, GetStats(_))
- .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_),
- Return(true)));
+ stats.AddLocalAudioTrack(audio_track_, kSsrcOfTrack);
cricket::VoiceSenderInfo voice_sender_info;
InitVoiceSenderInfo(&voice_sender_info);
- // Constructs an ssrc stats update.
cricket::VoiceMediaInfo stats_read;
- stats_read.senders.push_back(voice_sender_info);
-
- EXPECT_CALL(session_, voice_channel()).WillRepeatedly(Return(&voice_channel));
- EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
- EXPECT_CALL(*media_channel, GetStats(_))
- .WillRepeatedly(DoAll(SetArgPointee<0>(stats_read),
- Return(true)));
-
StatsReports reports; // returned values.
- stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
- stats.GetStats(NULL, &reports);
-
- // Verfy the existence of the track report.
- const StatsReport* report = FindNthReportByType(
- reports, StatsReport::kStatsReportTypeSsrc, 1);
- EXPECT_FALSE(report == NULL);
- std::string track_id = ExtractSsrcStatsValue(
- reports, StatsReport::kStatsValueNameTrackId);
- EXPECT_EQ(kLocalTrackId, track_id);
- std::string ssrc_id = ExtractSsrcStatsValue(
- reports, StatsReport::kStatsValueNameSsrc);
- EXPECT_EQ(talk_base::ToString<uint32>(kSsrcOfTrack), ssrc_id);
-
- // Verifies the values in the track report.
- UpdateVoiceSenderInfoFromAudioTrack(audio_track_.get(), &voice_sender_info);
- VerifyVoiceSenderInfoReport(report, voice_sender_info);
-
- // Verify we get the same result by passing a track to GetStats().
- StatsReports track_reports; // returned values.
- stats.GetStats(audio_track_.get(), &track_reports);
- const StatsReport* track_report = FindNthReportByType(
- track_reports, StatsReport::kStatsReportTypeSsrc, 1);
- EXPECT_TRUE(track_report);
- track_id = ExtractSsrcStatsValue(track_reports,
- StatsReport::kStatsValueNameTrackId);
- EXPECT_EQ(kLocalTrackId, track_id);
- ssrc_id = ExtractSsrcStatsValue(track_reports,
- StatsReport::kStatsValueNameSsrc);
- EXPECT_EQ(talk_base::ToString<uint32>(kSsrcOfTrack), ssrc_id);
- VerifyVoiceSenderInfoReport(track_report, voice_sender_info);
+ SetupAndVerifyAudioTrackStats(
+ audio_track_.get(), stream_.get(), &stats, &voice_channel, kVcName,
+ media_channel, &voice_sender_info, NULL, &stats_read, &reports);
// Verify that there is no remote report for the local audio track because
// we did not set it up.
@@ -1246,42 +1267,15 @@
AddIncomingAudioTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- // Instruct the session to return stats containing the transport channel.
- InitSessionStats(kVcName);
- EXPECT_CALL(session_, GetStats(_))
- .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_),
- Return(true)));
-
cricket::VoiceReceiverInfo voice_receiver_info;
InitVoiceReceiverInfo(&voice_receiver_info);
voice_receiver_info.codec_name = "fake_codec";
- // Constructs an ssrc stats update.
cricket::VoiceMediaInfo stats_read;
- stats_read.receivers.push_back(voice_receiver_info);
-
- EXPECT_CALL(session_, voice_channel()).WillRepeatedly(Return(&voice_channel));
- EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
- EXPECT_CALL(*media_channel, GetStats(_))
- .WillRepeatedly(DoAll(SetArgPointee<0>(stats_read),
- Return(true)));
-
StatsReports reports; // returned values.
- stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
- stats.GetStats(NULL, &reports);
-
- // Verify the track id is |kRemoteTrackId|.
- const std::string track_id = ExtractSsrcStatsValue(
- reports, StatsReport::kStatsValueNameTrackId);
- EXPECT_EQ(kRemoteTrackId, track_id);
-
- // Verify the report for this remote track.
- const StatsReport* report = FindNthReportByType(
- reports, StatsReport::kStatsReportTypeSsrc, 1);
- EXPECT_FALSE(report == NULL);
- VerifyVoiceReceiverInfoReport(report, voice_receiver_info);
+ SetupAndVerifyAudioTrackStats(
+ audio_track_.get(), stream_.get(), &stats, &voice_channel, kVcName,
+ media_channel, NULL, &voice_receiver_info, &stats_read, &reports);
}
// This test verifies that a local stats object won't update its statistics
@@ -1361,7 +1355,7 @@
talk_base::scoped_refptr<FakeAudioTrack> remote_track(
new talk_base::RefCountedObject<FakeAudioTrack>(kRemoteTrackId));
EXPECT_CALL(session_, GetRemoteTrackIdBySsrc(kSsrcOfTrack, _))
- .WillRepeatedly(DoAll(SetArgPointee<1>(kRemoteTrackId), Return(true)));
+ .WillOnce(DoAll(SetArgPointee<1>(kRemoteTrackId), Return(true)));
remote_stream->AddTrack(remote_track);
stats.AddStream(remote_stream);
@@ -1418,4 +1412,52 @@
VerifyVoiceReceiverInfoReport(track_report, voice_receiver_info);
}
+// This test verifies that when two outgoing audio tracks are using the same
+// ssrc at different times, they populate stats reports correctly.
+// TODO(xians): Figure out if it is possible to encapsulate the setup and
+// avoid duplication of code in test cases.
+TEST_F(StatsCollectorTest, TwoLocalTracksWithSameSsrc) {
+ webrtc::StatsCollector stats; // Implementation under test.
+ MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
+ // The content_name known by the voice channel.
+ const std::string kVcName("vcname");
+ cricket::VoiceChannel voice_channel(talk_base::Thread::Current(),
+ media_engine_, media_channel, &session_, kVcName, false);
+
+ // Create a local stream with a local audio track and adds it to the stats.
+ AddOutgoingAudioTrackStats();
+ stats.AddStream(stream_);
+ stats.AddLocalAudioTrack(audio_track_, kSsrcOfTrack);
+
+ cricket::VoiceSenderInfo voice_sender_info;
+ voice_sender_info.add_ssrc(kSsrcOfTrack);
+
+ cricket::VoiceMediaInfo stats_read;
+ StatsReports reports; // returned values.
+ SetupAndVerifyAudioTrackStats(
+ audio_track_.get(), stream_.get(), &stats, &voice_channel, kVcName,
+ media_channel, &voice_sender_info, NULL, &stats_read, &reports);
+
+ // Remove the previous audio track from the stream.
+ stream_->RemoveTrack(audio_track_.get());
+ stats.RemoveLocalAudioTrack(audio_track_.get(), kSsrcOfTrack);
+
+ // Create a new audio track and adds it to the stream and stats.
+ static const std::string kNewTrackId = "new_track_id";
+ talk_base::scoped_refptr<FakeAudioTrack> new_audio_track(
+ new talk_base::RefCountedObject<FakeAudioTrack>(kNewTrackId));
+ EXPECT_CALL(session_, GetLocalTrackIdBySsrc(kSsrcOfTrack, _))
+ .WillOnce(DoAll(SetArgPointee<1>(kNewTrackId), Return(true)));
+ stream_->AddTrack(new_audio_track);
+
+ stats.AddLocalAudioTrack(new_audio_track, kSsrcOfTrack);
+ stats.ClearUpdateStatsCache();
+ cricket::VoiceSenderInfo new_voice_sender_info;
+ InitVoiceSenderInfo(&new_voice_sender_info);
+ cricket::VoiceMediaInfo new_stats_read;
+ SetupAndVerifyAudioTrackStats(
+ new_audio_track.get(), stream_.get(), &stats, &voice_channel, kVcName,
+ media_channel, &new_voice_sender_info, NULL, &new_stats_read, &reports);
+}
+
} // namespace
