Make StatsCollector depend on always having a valid session pointer.
This is required since the session pointer is currently used on multiple threads but there's no synchronization code to guard it.
I'm removing the set_session() method and session() getter since they would cause problems if used without synchronization.
This is a reland of an already reviewed cl that got reverted by mistake.
TBR=xians@google.com
Review URL: https://webrtc-codereview.appspot.com/13959004
git-svn-id: http://webrtc.googlecode.com/svn/trunk/talk@6677 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/app/webrtc/peerconnection.cc b/app/webrtc/peerconnection.cc
index 0839977..ec20593 100644
--- a/app/webrtc/peerconnection.cc
+++ b/app/webrtc/peerconnection.cc
@@ -373,7 +373,7 @@
mediastream_signaling_.get()));
stream_handler_container_.reset(new MediaStreamHandlerContainer(
session_.get(), session_.get()));
- stats_.set_session(session_.get());
+ stats_.reset(new StatsCollector(session_.get()));
// Initialize the WebRtcSession. It creates transport channels etc.
if (!session_->Initialize(factory_->options(), constraints,
@@ -410,7 +410,7 @@
if (!mediastream_signaling_->AddLocalStream(local_stream)) {
return false;
}
- stats_.AddStream(local_stream);
+ stats_->AddStream(local_stream);
observer_->OnRenegotiationNeeded();
return true;
}
@@ -451,9 +451,9 @@
return false;
}
- stats_.UpdateStats(level);
+ stats_->UpdateStats(level);
talk_base::scoped_ptr<GetStatsMsg> msg(new GetStatsMsg(observer));
- if (!stats_.GetStats(track, &(msg->reports))) {
+ if (!stats_->GetStats(track, &(msg->reports))) {
return false;
}
signaling_thread()->Post(this, MSG_GETSTATS, msg.release());
@@ -534,7 +534,7 @@
}
// Update stats here so that we have the most recent stats for tracks and
// streams that might be removed by updating the session description.
- stats_.UpdateStats(kStatsOutputLevelStandard);
+ stats_->UpdateStats(kStatsOutputLevelStandard);
std::string error;
if (!session_->SetLocalDescription(desc, &error)) {
PostSetSessionDescriptionFailure(observer, error);
@@ -557,7 +557,7 @@
}
// Update stats here so that we have the most recent stats for tracks and
// streams that might be removed by updating the session description.
- stats_.UpdateStats(kStatsOutputLevelStandard);
+ stats_->UpdateStats(kStatsOutputLevelStandard);
std::string error;
if (!session_->SetRemoteDescription(desc, &error)) {
PostSetSessionDescriptionFailure(observer, error);
@@ -649,7 +649,7 @@
void PeerConnection::Close() {
// Update stats here so that we have the most recent stats for tracks and
// streams before the channels are closed.
- stats_.UpdateStats(kStatsOutputLevelStandard);
+ stats_->UpdateStats(kStatsOutputLevelStandard);
session_->Terminate();
}
@@ -713,7 +713,7 @@
}
void PeerConnection::OnAddRemoteStream(MediaStreamInterface* stream) {
- stats_.AddStream(stream);
+ stats_->AddStream(stream);
observer_->OnAddStream(stream);
}
@@ -754,7 +754,7 @@
AudioTrackInterface* audio_track,
uint32 ssrc) {
stream_handler_container_->AddLocalAudioTrack(stream, audio_track, ssrc);
- stats_.AddLocalAudioTrack(audio_track, ssrc);
+ stats_->AddLocalAudioTrack(audio_track, ssrc);
}
void PeerConnection::OnAddLocalVideoTrack(MediaStreamInterface* stream,
VideoTrackInterface* video_track,
@@ -766,7 +766,7 @@
AudioTrackInterface* audio_track,
uint32 ssrc) {
stream_handler_container_->RemoveLocalTrack(stream, audio_track);
- stats_.RemoveLocalAudioTrack(audio_track, ssrc);
+ stats_->RemoveLocalAudioTrack(audio_track, ssrc);
}
void PeerConnection::OnRemoveLocalVideoTrack(MediaStreamInterface* stream,
diff --git a/app/webrtc/peerconnection.h b/app/webrtc/peerconnection.h
index 4a428ef..ebb5dba 100644
--- a/app/webrtc/peerconnection.h
+++ b/app/webrtc/peerconnection.h
@@ -196,7 +196,7 @@
talk_base::scoped_ptr<WebRtcSession> session_;
talk_base::scoped_ptr<MediaStreamSignaling> mediastream_signaling_;
talk_base::scoped_ptr<MediaStreamHandlerContainer> stream_handler_container_;
- StatsCollector stats_;
+ talk_base::scoped_ptr<StatsCollector> stats_;
};
} // namespace webrtc
diff --git a/app/webrtc/statscollector.cc b/app/webrtc/statscollector.cc
index 4c3f7de..b80da23 100644
--- a/app/webrtc/statscollector.cc
+++ b/app/webrtc/statscollector.cc
@@ -528,8 +528,12 @@
} // namespace
-StatsCollector::StatsCollector()
- : session_(NULL), stats_gathering_started_(0) {
+StatsCollector::StatsCollector(WebRtcSession* session)
+ : session_(session), stats_gathering_started_(0) {
+ ASSERT(session_);
+}
+
+StatsCollector::~StatsCollector() {
}
// Adds a MediaStream with tracks that can be used as a |selector| in a call
@@ -1063,14 +1067,14 @@
bool StatsCollector::GetTrackIdBySsrc(uint32 ssrc, std::string* track_id,
TrackDirection direction) {
if (direction == kSending) {
- if (!session()->GetLocalTrackIdBySsrc(ssrc, track_id)) {
+ if (!session_->GetLocalTrackIdBySsrc(ssrc, track_id)) {
LOG(LS_WARNING) << "The SSRC " << ssrc
<< " is not associated with a sending track";
return false;
}
} else {
ASSERT(direction == kReceiving);
- if (!session()->GetRemoteTrackIdBySsrc(ssrc, track_id)) {
+ if (!session_->GetRemoteTrackIdBySsrc(ssrc, track_id)) {
LOG(LS_WARNING) << "The SSRC " << ssrc
<< " is not associated with a receiving track";
return false;
diff --git a/app/webrtc/statscollector.h b/app/webrtc/statscollector.h
index b3ed1ed..f51cee8 100644
--- a/app/webrtc/statscollector.h
+++ b/app/webrtc/statscollector.h
@@ -49,13 +49,10 @@
kReceiving,
};
- StatsCollector();
-
- // Register the session Stats should operate on.
- // Set to NULL if the session has ended.
- void set_session(WebRtcSession* session) {
- session_ = session;
- }
+ // The caller is responsible for ensuring that the session outlives the
+ // StatsCollector instance.
+ explicit StatsCollector(WebRtcSession* session);
+ virtual ~StatsCollector();
// Adds a MediaStream with tracks that can be used as a |selector| in a call
// to GetStats.
@@ -110,7 +107,6 @@
void ExtractVideoInfo(PeerConnectionInterface::StatsOutputLevel level);
double GetTimeNow();
void BuildSsrcToTransportId();
- WebRtcSession* session() { return session_; }
webrtc::StatsReport* GetOrCreateReport(const std::string& type,
const std::string& id,
TrackDirection direction);
@@ -131,7 +127,7 @@
// A map from the report id to the report.
std::map<std::string, StatsReport> reports_;
// Raw pointer to the session the statistics are gathered from.
- WebRtcSession* session_;
+ WebRtcSession* const session_;
double stats_gathering_started_;
cricket::ProxyTransportMap proxy_to_transport_;
diff --git a/app/webrtc/statscollector_unittest.cc b/app/webrtc/statscollector_unittest.cc
index af45038..76251e2 100644
--- a/app/webrtc/statscollector_unittest.cc
+++ b/app/webrtc/statscollector_unittest.cc
@@ -514,7 +514,6 @@
// 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);
@@ -580,9 +579,8 @@
const std::vector<std::string>& local_ders,
const talk_base::FakeSSLCertificate& remote_cert,
const std::vector<std::string>& remote_ders) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
StatsReports reports; // returned values.
- stats.set_session(&session_);
// Fake stats to process.
cricket::TransportChannelStats channel_stats;
@@ -667,7 +665,7 @@
// This test verifies that 64-bit counters are passed successfully.
TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
@@ -678,7 +676,6 @@
const int64 kBytesSent = 12345678901234LL;
const std::string kBytesSentString("12345678901234");
- stats.set_session(&session_);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
@@ -700,7 +697,7 @@
// Test that BWE information is reported via stats.
TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
@@ -712,7 +709,6 @@
const int64 kBytesSent = 12345678901234LL;
const std::string kBytesSentString("12345678901234");
- stats.set_session(&session_);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
@@ -743,9 +739,8 @@
// This test verifies that an object of type "googSession" always
// exists in the returned stats.
TEST_F(StatsCollectorTest, SessionObjectExists) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
StatsReports reports; // returned values.
- stats.set_session(&session_);
EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull());
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
@@ -758,9 +753,8 @@
// This test verifies that only one object of type "googSession" exists
// in the returned stats.
TEST_F(StatsCollectorTest, OnlyOneSessionObjectExists) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
StatsReports reports; // returned values.
- stats.set_session(&session_);
EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull());
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
@@ -777,18 +771,15 @@
// This test verifies that the empty track report exists in the returned stats
// without calling StatsCollector::UpdateStats.
TEST_F(StatsCollectorTest, TrackObjectExistsWithoutUpdateStats) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- StatsReports reports;
-
// Verfies the existence of the track report.
+ StatsReports reports;
stats.GetStats(NULL, &reports);
EXPECT_EQ((size_t)1, reports.size());
EXPECT_EQ(std::string(StatsReport::kStatsReportTypeTrack),
@@ -804,17 +795,13 @@
// This test verifies that the empty track report exists in the returned stats
// when StatsCollector::UpdateStats is called with ssrc stats.
TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- StatsReports reports;
-
// Constructs an ssrc stats update.
cricket::VideoSenderInfo video_sender_info;
cricket::VideoMediaInfo stats_read;
@@ -832,6 +819,7 @@
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
+ StatsReports reports;
stats.GetStats(NULL, &reports);
// |reports| should contain at least one session report, one track report,
// and one ssrc report.
@@ -861,7 +849,7 @@
// This test verifies that an SSRC object has the identifier of a Transport
// stats object, and that this transport stats object exists in stats.
TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
// The content_name known by the video channel.
const std::string kVcName("vcname");
@@ -870,10 +858,6 @@
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- StatsReports reports;
-
// Constructs an ssrc stats update.
cricket::VideoSenderInfo video_sender_info;
cricket::VideoMediaInfo stats_read;
@@ -896,6 +880,7 @@
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
+ StatsReports reports;
stats.GetStats(NULL, &reports);
std::string transport_id = ExtractStatsValue(
StatsReport::kStatsReportTypeSsrc,
@@ -910,7 +895,7 @@
// This test verifies that a remote stats object will not be created for
// an outgoing SSRC where remote stats are not returned.
TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
// The content_name known by the video channel.
const std::string kVcName("vcname");
@@ -919,8 +904,6 @@
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull());
EXPECT_CALL(session_, voice_channel()).WillRepeatedly(ReturnNull());
@@ -935,7 +918,7 @@
// This test verifies that a remote stats object will be created for
// an outgoing SSRC where stats are returned.
TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
// The content_name known by the video channel.
const std::string kVcName("vcname");
@@ -944,10 +927,6 @@
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- StatsReports reports;
-
// Instruct the session to return stats containing the transport channel.
InitSessionStats(kVcName);
EXPECT_CALL(session_, GetStats(_))
@@ -972,6 +951,7 @@
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
+ StatsReports reports;
stats.GetStats(NULL, &reports);
const StatsReport* remote_report = FindNthReportByType(reports,
@@ -983,17 +963,13 @@
// This test verifies that the empty track report exists in the returned stats
// when StatsCollector::UpdateStats is called with ssrc stats.
TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
AddIncomingVideoTrackStats();
stats.AddStream(stream_);
- stats.set_session(&session_);
-
- StatsReports reports;
-
// Constructs an ssrc stats update.
cricket::VideoReceiverInfo video_receiver_info;
cricket::VideoMediaInfo stats_read;
@@ -1011,6 +987,7 @@
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
+ StatsReports reports;
stats.GetStats(NULL, &reports);
// |reports| should contain at least one session report, one track report,
// and one ssrc report.
@@ -1069,9 +1046,8 @@
// This test verifies that the stats are generated correctly when no
// transport is present.
TEST_F(StatsCollectorTest, NoTransport) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
StatsReports reports; // returned values.
- stats.set_session(&session_);
// Fake stats to process.
cricket::TransportChannelStats channel_stats;
@@ -1116,9 +1092,8 @@
// This test verifies that the stats are generated correctly when the transport
// does not have any certificates.
TEST_F(StatsCollectorTest, NoCertificates) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
StatsReports reports; // returned values.
- stats.set_session(&session_);
// Fake stats to process.
cricket::TransportChannelStats channel_stats;
@@ -1185,13 +1160,11 @@
// Verifies the correct optons are passed to the VideoMediaChannel when using
// verbose output level.
TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVideoMediaChannel* media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(talk_base::Thread::Current(),
media_engine_, media_channel, &session_, "", false, NULL);
- stats.set_session(&session_);
- StatsReports reports; // returned values.
cricket::VideoMediaInfo stats_read;
cricket::BandwidthEstimationInfo bwe;
bwe.total_received_propagation_delta_ms = 10;
@@ -1214,6 +1187,7 @@
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelDebug);
+ StatsReports reports; // returned values.
stats.GetStats(NULL, &reports);
std::string result = ExtractBweStatsValue(
reports, "googReceivedPacketGroupPropagationDeltaSumDebug");
@@ -1229,7 +1203,7 @@
// This test verifies that a local stats object can get statistics via
// AudioTrackInterface::GetStats() method.
TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
// The content_name known by the voice channel.
const std::string kVcName("vcname");
@@ -1258,7 +1232,7 @@
// This test verifies that audio receive streams populate stats reports
// correctly.
TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
// The content_name known by the voice channel.
const std::string kVcName("vcname");
@@ -1281,7 +1255,7 @@
// This test verifies that a local stats object won't update its statistics
// after a RemoveLocalAudioTrack() call.
TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
// The content_name known by the voice channel.
const std::string kVcName("vcname");
@@ -1291,8 +1265,6 @@
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(_))
@@ -1337,7 +1309,7 @@
// This test verifies that when ongoing and incoming audio tracks are using
// the same ssrc, they populate stats reports correctly.
TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) {
- webrtc::StatsCollector stats; // Implementation under test.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
// The content_name known by the voice channel.
const std::string kVcName("vcname");
@@ -1359,8 +1331,6 @@
remote_stream->AddTrack(remote_track);
stats.AddStream(remote_stream);
- stats.set_session(&session_);
-
// Instruct the session to return stats containing the transport channel.
InitSessionStats(kVcName);
EXPECT_CALL(session_, GetStats(_))
@@ -1417,7 +1387,7 @@
// 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.
+ webrtc::StatsCollector stats(&session_); // Implementation under test.
MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel();
// The content_name known by the voice channel.
const std::string kVcName("vcname");
