From 5b76fd79dfbfa78f1b034c0698771298cd15f175 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Mon, 19 Jan 2015 16:49:33 +0000 Subject: [PATCH] Update StatsCollector's interface in preparation of more changes. This CL is the first of three and this one contains interface additions (not deletion for backwards compatibility) as well as a few necessary updates to internal code. The next CL will be in Chromium to consume the new new methods and remove dependency on the old ones. The third CL will then contain the bulk of the updates and improvements and be compatible with this interface. BUG=2822 R=perkj@webrtc.org Review URL: https://webrtc-codereview.appspot.com/36829004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@8095 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../app/webrtc/java/jni/peerconnection_jni.cc | 14 ++--- talk/app/webrtc/objc/RTCStatsReport.mm | 15 +++-- talk/app/webrtc/statscollector.cc | 44 +++++++------- talk/app/webrtc/statscollector_unittest.cc | 17 +++--- talk/app/webrtc/statstypes.cc | 57 +++++++++++++------ talk/app/webrtc/statstypes.h | 38 +++++++++++-- .../webrtc/test/mockpeerconnectionobservers.h | 6 +- 7 files changed, 118 insertions(+), 73 deletions(-) diff --git a/talk/app/webrtc/java/jni/peerconnection_jni.cc b/talk/app/webrtc/java/jni/peerconnection_jni.cc index 08ff72d7fd..7f4aa4cc41 100644 --- a/talk/app/webrtc/java/jni/peerconnection_jni.cc +++ b/talk/app/webrtc/java/jni/peerconnection_jni.cc @@ -1002,14 +1002,14 @@ class StatsObserverWrapper : public StatsObserver { int i = 0; for (const auto* report : reports) { ScopedLocalRefFrame local_ref_frame(jni); - jstring j_id = JavaStringFromStdString(jni, report->id); - jstring j_type = JavaStringFromStdString(jni, report->type); - jobjectArray j_values = ValuesToJava(jni, report->values); + jstring j_id = JavaStringFromStdString(jni, report->id().ToString()); + jstring j_type = JavaStringFromStdString(jni, report->TypeToString()); + jobjectArray j_values = ValuesToJava(jni, report->values()); jobject j_report = jni->NewObject(*j_stats_report_class_, j_stats_report_ctor_, j_id, j_type, - report->timestamp, + report->timestamp(), j_values); jni->SetObjectArrayElement(reports_array, i++, j_report); } @@ -1021,11 +1021,11 @@ class StatsObserverWrapper : public StatsObserver { values.size(), *j_value_class_, NULL); for (int i = 0; i < values.size(); ++i) { ScopedLocalRefFrame local_ref_frame(jni); - const StatsReport::Value& value = values[i]; + const auto& value = values[i]; // Should we use the '.name' enum value here instead of converting the // name to a string? - jstring j_name = JavaStringFromStdString(jni, value.display_name()); - jstring j_value = JavaStringFromStdString(jni, value.value); + jstring j_name = JavaStringFromStdString(jni, value->display_name()); + jstring j_value = JavaStringFromStdString(jni, value->value); jobject j_element_value = jni->NewObject(*j_value_class_, j_value_ctor_, j_name, j_value); jni->SetObjectArrayElement(j_values, i, j_element_value); diff --git a/talk/app/webrtc/objc/RTCStatsReport.mm b/talk/app/webrtc/objc/RTCStatsReport.mm index 98cf6548e3..bbf18539b9 100644 --- a/talk/app/webrtc/objc/RTCStatsReport.mm +++ b/talk/app/webrtc/objc/RTCStatsReport.mm @@ -50,15 +50,14 @@ - (instancetype)initWithStatsReport:(const webrtc::StatsReport&)statsReport { if (self = [super init]) { - _reportId = @(statsReport.id.c_str()); - _type = @(statsReport.type.c_str()); - _timestamp = statsReport.timestamp; + _reportId = @(statsReport.id().ToString().c_str()); + _type = @(statsReport.TypeToString()); + _timestamp = statsReport.timestamp(); NSMutableArray* values = - [NSMutableArray arrayWithCapacity:statsReport.values.size()]; - webrtc::StatsReport::Values::const_iterator it = statsReport.values.begin(); - for (; it != statsReport.values.end(); ++it) { - RTCPair* pair = [[RTCPair alloc] initWithKey:@(it->display_name()) - value:@(it->value.c_str())]; + [NSMutableArray arrayWithCapacity:statsReport.values().size()]; + for (const auto& v : statsReport.values()) { + RTCPair* pair = [[RTCPair alloc] initWithKey:@(v->display_name()) + value:@(v->value.c_str())]; [values addObject:pair]; } _values = values; diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc index 05695c04eb..e2d444e5e1 100644 --- a/talk/app/webrtc/statscollector.cc +++ b/talk/app/webrtc/statscollector.cc @@ -102,10 +102,10 @@ bool ExtractValueFromReport( const StatsReport& report, StatsReport::StatsValueName name, std::string* value) { - StatsReport::Values::const_iterator it = report.values.begin(); - for (; it != report.values.end(); ++it) { - if (it->name == name) { - *value = it->value; + StatsReport::Values::const_iterator it = report.values().begin(); + for (; it != report.values().end(); ++it) { + if ((*it)->name == name) { + *value = (*it)->value; return true; } } @@ -284,13 +284,12 @@ void ExtractStats(const cricket::BandwidthEstimationInfo& info, double stats_gathering_started, PeerConnectionInterface::StatsOutputLevel level, StatsReport* report) { - ASSERT(report->id == StatsReport::kStatsReportVideoBweId); report->type = StatsReport::kStatsReportTypeBwe; // Clear out stats from previous GatherStats calls if any. - if (report->timestamp != stats_gathering_started) { - report->values.clear(); - report->timestamp = stats_gathering_started; + if (report->timestamp() != stats_gathering_started) { + report->ResetValues(); + report->set_timestamp(stats_gathering_started); } report->AddValue(StatsReport::kStatsValueNameAvailableSendBandwidth, @@ -324,13 +323,13 @@ void ExtractStats(const cricket::BandwidthEstimationInfo& info, void ExtractRemoteStats(const cricket::MediaSenderInfo& info, StatsReport* report) { - report->timestamp = info.remote_stats[0].timestamp; + report->set_timestamp(info.remote_stats[0].timestamp); // TODO(hta): Extract some stats here. } void ExtractRemoteStats(const cricket::MediaReceiverInfo& info, StatsReport* report) { - report->timestamp = info.remote_stats[0].timestamp; + report->set_timestamp(info.remote_stats[0].timestamp); // TODO(hta): Extract some stats here. } @@ -554,8 +553,8 @@ StatsReport* StatsCollector::PrepareLocalReport( // 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->ResetValues(); + report->set_timestamp(stats_gathering_started_); report->AddValue(StatsReport::kStatsValueNameSsrc, ssrc_id); report->AddValue(StatsReport::kStatsValueNameTrackId, track_id); @@ -595,8 +594,8 @@ StatsReport* StatsCollector::PrepareRemoteReport( // Clear out stats from previous GatherStats calls if any. // The timestamp will be added later. Zero it for debugging. - report->values.clear(); - report->timestamp = 0; + report->ResetValues(); + report->set_timestamp(0); report->AddValue(StatsReport::kStatsValueNameSsrc, ssrc_id); report->AddValue(StatsReport::kStatsValueNameTrackId, track_id); @@ -637,14 +636,14 @@ std::string StatsCollector::AddOneCertificateReport( StatsReport* report = reports_.ReplaceOrAddNew( StatsId(StatsReport::kStatsReportTypeCertificate, fingerprint)); report->type = StatsReport::kStatsReportTypeCertificate; - report->timestamp = stats_gathering_started_; + report->set_timestamp(stats_gathering_started_); report->AddValue(StatsReport::kStatsValueNameFingerprint, fingerprint); report->AddValue(StatsReport::kStatsValueNameFingerprintAlgorithm, digest_algorithm); report->AddValue(StatsReport::kStatsValueNameDer, der_base64); if (!issuer_id.empty()) report->AddValue(StatsReport::kStatsValueNameIssuerId, issuer_id); - return report->id; + return report->id().ToString(); } std::string StatsCollector::AddCertificateReports( @@ -687,7 +686,7 @@ std::string StatsCollector::AddCandidateReport( report->AddValue(StatsReport::kStatsValueNameCandidateNetworkType, AdapterTypeToStatsType(candidate.network_type())); } - report->timestamp = stats_gathering_started_; + report->set_timestamp(stats_gathering_started_); report->AddValue(StatsReport::kStatsValueNameCandidateIPAddress, candidate.address().ipaddr().ToString()); report->AddValue(StatsReport::kStatsValueNameCandidatePortNumber, @@ -709,8 +708,8 @@ void StatsCollector::ExtractSessionInfo() { StatsReport* report = reports_.ReplaceOrAddNew( StatsId(StatsReport::kStatsReportTypeSession, session_->id())); report->type = StatsReport::kStatsReportTypeSession; - report->timestamp = stats_gathering_started_; - report->values.clear(); + report->set_timestamp(stats_gathering_started_); + report->ResetValues(); report->AddBoolean(StatsReport::kStatsValueNameInitiator, session_->initiator()); @@ -755,7 +754,7 @@ void StatsCollector::ExtractSessionInfo() { << "-" << channel_iter->component; StatsReport* channel_report = reports_.ReplaceOrAddNew(ostc.str()); channel_report->type = StatsReport::kStatsReportTypeComponent; - channel_report->timestamp = stats_gathering_started_; + channel_report->set_timestamp(stats_gathering_started_); channel_report->AddValue(StatsReport::kStatsValueNameComponent, channel_iter->component); if (!local_cert_report_id.empty()) { @@ -776,10 +775,10 @@ void StatsCollector::ExtractSessionInfo() { << channel_iter->component << "-" << i; StatsReport* report = reports_.ReplaceOrAddNew(ost.str()); report->type = StatsReport::kStatsReportTypeCandidatePair; - report->timestamp = stats_gathering_started_; + report->set_timestamp(stats_gathering_started_); // Link from connection to its containing channel. report->AddValue(StatsReport::kStatsValueNameChannelId, - channel_report->id); + channel_report->id().ToString()); const cricket::ConnectionInfo& info = channel_iter->connection_infos[i]; @@ -919,7 +918,6 @@ StatsReport* StatsCollector::GetOrCreateReport(const std::string& type, if (report == NULL) { std::string statsid = StatsId(type, id, direction); report = reports_.FindOrAddNew(statsid); - ASSERT(report->id == statsid); report->type = type; } diff --git a/talk/app/webrtc/statscollector_unittest.cc b/talk/app/webrtc/statscollector_unittest.cc index cba4003906..e89b518f44 100644 --- a/talk/app/webrtc/statscollector_unittest.cc +++ b/talk/app/webrtc/statscollector_unittest.cc @@ -155,10 +155,9 @@ class FakeAudioTrack bool GetValue(const StatsReport* report, StatsReport::StatsValueName name, std::string* value) { - StatsReport::Values::const_iterator it = report->values.begin(); - for (; it != report->values.end(); ++it) { - if (it->name == name) { - *value = it->value; + for (const auto& v : report->values()) { + if (v->name == name) { + *value = v->value; return true; } } @@ -199,12 +198,12 @@ const StatsReport* FindNthReportByType( const StatsReport* FindReportById(const StatsReports& reports, const std::string& id) { - for (size_t i = 0; i < reports.size(); ++i) { - if (reports[i]->id == id) { - return reports[i]; + for (const auto* r : reports) { + if (r->id().ToString() == id) { + return r; } } - return NULL; + return nullptr; } std::string ExtractSsrcStatsValue(StatsReports reports, @@ -1029,7 +1028,7 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { const StatsReport* remote_report = FindNthReportByType(reports, StatsReport::kStatsReportTypeRemoteSsrc, 1); EXPECT_FALSE(remote_report == NULL); - EXPECT_NE(0, remote_report->timestamp); + EXPECT_NE(0, remote_report->timestamp()); } // This test verifies that the empty track report exists in the returned stats diff --git a/talk/app/webrtc/statstypes.cc b/talk/app/webrtc/statstypes.cc index b15dba5d2d..dc943f0740 100644 --- a/talk/app/webrtc/statstypes.cc +++ b/talk/app/webrtc/statstypes.cc @@ -27,6 +27,8 @@ #include "talk/app/webrtc/statstypes.h" +using rtc::scoped_ptr; + namespace webrtc { const char StatsReport::kStatsReportTypeSession[] = "googLibjingleSession"; @@ -46,37 +48,50 @@ const char StatsReport::kStatsReportTypeDataChannel[] = "datachannel"; const char StatsReport::kStatsReportVideoBweId[] = "bweforvideo"; StatsReport::StatsReport(const StatsReport& src) - : id(src.id), + : id_(src.id_), type(src.type), - timestamp(src.timestamp), - values(src.values) { + timestamp_(src.timestamp_), + values_(src.values_) { } StatsReport::StatsReport(const std::string& id) - : id(id), timestamp(0) { + : id_(id), timestamp_(0) { +} + +StatsReport::StatsReport(scoped_ptr id) + : id_(id->ToString()), timestamp_(0) { +} + +// static +scoped_ptr StatsReport::NewTypedId( + StatsReport::StatsType type, const std::string& id) { + std::string internal_id(type); + internal_id += '_'; + internal_id += id; + return scoped_ptr(new Id(internal_id)).Pass(); } StatsReport& StatsReport::operator=(const StatsReport& src) { - ASSERT(id == src.id); + ASSERT(id_ == src.id_); type = src.type; - timestamp = src.timestamp; - values = src.values; + timestamp_ = src.timestamp_; + values_ = src.values_; return *this; } // Operators provided for STL container/algorithm support. bool StatsReport::operator<(const StatsReport& other) const { - return id < other.id; + return id_ < other.id_; } bool StatsReport::operator==(const StatsReport& other) const { - return id == other.id; + return id_ == other.id_; } // Special support for being able to use std::find on a container // without requiring a new StatsReport instance. bool StatsReport::operator==(const std::string& other_id) const { - return id == other_id; + return id_ == other_id; } // The copy ctor can't be declared as explicit due to problems with STL. @@ -318,7 +333,7 @@ const char* StatsReport::Value::display_name() const { void StatsReport::AddValue(StatsReport::StatsValueName name, const std::string& value) { - values.push_back(Value(name, value)); + values_.push_back(ValuePtr(new Value(name, value))); } void StatsReport::AddValue(StatsReport::StatsValueName name, int64 value) { @@ -360,15 +375,21 @@ void StatsReport::AddBoolean(StatsReport::StatsValueName name, bool value) { void StatsReport::ReplaceValue(StatsReport::StatsValueName name, const std::string& value) { - for (Values::iterator it = values.begin(); it != values.end(); ++it) { - if ((*it).name == name) { - it->value = value; + Values::iterator it = std::find_if(values_.begin(), values_.end(), + [&name](const ValuePtr& v)->bool { return v->name == name; }); + // Values are const once added since they may be used outside of the stats + // collection. So we remove it from values_ when replacing and add a new one. + if (it != values_.end()) { + if ((*it)->value == value) return; - } + values_.erase(it); } - // It is not reachable here, add an ASSERT to make sure the overwriting is - // always a success. - ASSERT(false); + + AddValue(name, value); +} + +void StatsReport::ResetValues() { + values_.clear(); } StatsSet::StatsSet() { diff --git a/talk/app/webrtc/statstypes.h b/talk/app/webrtc/statstypes.h index 15a12d892f..d65d30d291 100644 --- a/talk/app/webrtc/statstypes.h +++ b/talk/app/webrtc/statstypes.h @@ -38,6 +38,8 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/common.h" +#include "webrtc/base/linked_ptr.h" +#include "webrtc/base/scoped_ptr.h" #include "webrtc/base/stringencode.h" namespace webrtc { @@ -46,7 +48,7 @@ class StatsReport { public: // TODO(tommi): Remove this ctor after removing reliance upon it in Chromium // (mock_peer_connection_impl.cc). - StatsReport() : timestamp(0) {} + StatsReport() : timestamp_(0) {} // TODO(tommi): Make protected and disallow copy completely once not needed. StatsReport(const StatsReport& src); @@ -69,7 +71,7 @@ class StatsReport { // This is used as a key for this report in ordered containers, // so it must never be changed. // TODO(tommi): Make this member variable const. - std::string id; // See below for contents. + std::string id_; // See below for contents. std::string type; // See below for contents. // StatsValue names. @@ -179,6 +181,15 @@ class StatsReport { kStatsValueNameWritable, }; + class Id { + public: + Id(const std::string& id) : id_(id) {} + Id(const Id& id) : id_(id.id_) {} + const std::string& ToString() const { return id_; } + private: + const std::string id_; + }; + struct Value { // The copy ctor can't be declared as explicit due to problems with STL. Value(const Value& other); @@ -198,6 +209,15 @@ class StatsReport { std::string value; }; + typedef rtc::linked_ptr ValuePtr; + typedef std::vector Values; + typedef const char* StatsType; + + // Ownership of |id| is passed to |this|. + explicit StatsReport(rtc::scoped_ptr id); + + static rtc::scoped_ptr NewTypedId(StatsType type, const std::string& id); + void AddValue(StatsValueName name, const std::string& value); void AddValue(StatsValueName name, int64 value); template @@ -206,9 +226,17 @@ class StatsReport { void ReplaceValue(StatsValueName name, const std::string& value); - double timestamp; // Time since 1970-01-01T00:00:00Z in milliseconds. - typedef std::vector Values; - Values values; + void ResetValues(); + + const Id id() const { return Id(id_); } + double timestamp() const { return timestamp_; } + void set_timestamp(double t) { timestamp_ = t; } + const Values& values() const { return values_; } + + const char* TypeToString() const { return type.c_str(); } + + double timestamp_; // Time since 1970-01-01T00:00:00Z in milliseconds. + Values values_; // TODO(tommi): These should all be enum values. diff --git a/talk/app/webrtc/test/mockpeerconnectionobservers.h b/talk/app/webrtc/test/mockpeerconnectionobservers.h index 56ca397f9d..5dbf92b57e 100644 --- a/talk/app/webrtc/test/mockpeerconnectionobservers.h +++ b/talk/app/webrtc/test/mockpeerconnectionobservers.h @@ -174,9 +174,9 @@ class MockStatsObserver : public webrtc::StatsObserver { bool GetIntValue(const StatsReport* report, StatsReport::StatsValueName name, int* value) { - for (const auto& v : report->values) { - if (v.name == name) { - *value = rtc::FromString(v.value); + for (const auto& v : report->values()) { + if (v->name == name) { + *value = rtc::FromString(v->value); return true; } }