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@6677 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
tommi@webrtc.org 2014-07-14 20:15:26 +00:00
parent b5348c64bb
commit 03505bcb7a
5 changed files with 49 additions and 79 deletions

View File

@ -373,7 +373,7 @@ bool PeerConnection::DoInitialize(
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 @@ bool PeerConnection::AddStream(MediaStreamInterface* local_stream,
if (!mediastream_signaling_->AddLocalStream(local_stream)) {
return false;
}
stats_.AddStream(local_stream);
stats_->AddStream(local_stream);
observer_->OnRenegotiationNeeded();
return true;
}
@ -451,9 +451,9 @@ bool PeerConnection::GetStats(StatsObserver* observer,
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 @@ void PeerConnection::SetLocalDescription(
}
// 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 @@ void PeerConnection::SetRemoteDescription(
}
// 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 @@ const SessionDescriptionInterface* PeerConnection::remote_description() const {
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::OnMessage(talk_base::Message* msg) {
}
void PeerConnection::OnAddRemoteStream(MediaStreamInterface* stream) {
stats_.AddStream(stream);
stats_->AddStream(stream);
observer_->OnAddStream(stream);
}
@ -754,7 +754,7 @@ void PeerConnection::OnAddLocalAudioTrack(MediaStreamInterface* stream,
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 @@ void PeerConnection::OnRemoveLocalAudioTrack(MediaStreamInterface* stream,
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,

View File

@ -196,7 +196,7 @@ class PeerConnection : public PeerConnectionInterface,
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

View File

@ -528,8 +528,12 @@ void ExtractStatsFromList(const std::vector<T>& data,
} // 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 @@ void StatsCollector::UpdateReportFromAudioTrack(AudioTrackInterface* track,
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;

View File

@ -49,13 +49,10 @@ class StatsCollector {
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 @@ class StatsCollector {
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 @@ class StatsCollector {
// 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_;

View File

@ -514,7 +514,6 @@ class StatsCollectorTest : public testing::Test {
// 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 @@ class StatsCollectorTest : public testing::Test {
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 @@ class StatsCollectorTest : public testing::Test {
// 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 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) {
const int64 kBytesSent = 12345678901234LL;
const std::string kBytesSentString("12345678901234");
stats.set_session(&session_);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
@ -700,7 +697,7 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) {
// 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 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) {
const int64 kBytesSent = 12345678901234LL;
const std::string kBytesSentString("12345678901234");
stats.set_session(&session_);
AddOutgoingVideoTrackStats();
stats.AddStream(stream_);
@ -743,9 +739,8 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) {
// 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 @@ TEST_F(StatsCollectorTest, SessionObjectExists) {
// 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 @@ TEST_F(StatsCollectorTest, OnlyOneSessionObjectExists) {
// 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 @@ TEST_F(StatsCollectorTest, TrackObjectExistsWithoutUpdateStats) {
// 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 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) {
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 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) {
// 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 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) {
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 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) {
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
StatsReports reports;
stats.GetStats(NULL, &reports);
std::string transport_id = ExtractStatsValue(
StatsReport::kStatsReportTypeSsrc,
@ -910,7 +895,7 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) {
// 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 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) {
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 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) {
// 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 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) {
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 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) {
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
StatsReports reports;
stats.GetStats(NULL, &reports);
const StatsReport* remote_report = FindNthReportByType(reports,
@ -983,17 +963,13 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) {
// 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 @@ TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) {
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 @@ TEST_F(StatsCollectorTest, ChainlessCertificateReportsCreated) {
// 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 @@ TEST_F(StatsCollectorTest, NoTransport) {
// 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 @@ TEST_F(StatsCollectorTest, UnsupportedDigestIgnored) {
// 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 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) {
Return(true)));
stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelDebug);
StatsReports reports; // returned values.
stats.GetStats(NULL, &reports);
std::string result = ExtractBweStatsValue(
reports, "googReceivedPacketGroupPropagationDeltaSumDebug");
@ -1229,7 +1203,7 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) {
// 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 @@ TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) {
// 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 @@ TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) {
// 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 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) {
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 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) {
// 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 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) {
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 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) {
// 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");