RTCStats and RTCStatsReport added (webrtc/stats)

webrtc/api/rtcstats.h

+/*
+ *  Copyright 2016 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_API_RTCSTATS_H_
+#define WEBRTC_API_RTCSTATS_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "webrtc/base/checks.h"
+
+namespace webrtc {
+
+class RTCStatsMemberInterface;
+
+// Abstract base class for RTCStats-derived dictionaries, see
+// https://w3c.github.io/webrtc-stats/.
+//
+// All derived classes must have the following static variable defined:
+//   static const char* const kType;
+// The address of |kType| is used as a unique class identifier and the value as
+// a string representation of the class type, see
+// https://w3c.github.io/webrtc-stats/#rtcstatstype-str*.
+// Use the |WEBRTC_RTCSTATS_IMPL| macro when implementing subclasses, see macro
+// for details.
+//
+// Derived classes list their dictionary members, RTCStatsMember<T>, as public
+// fields, allowing the following:
+//
+// RTCFooStats foo("fooId", GetCurrentTime());
+// foo.bar = 42;
+// foo.baz = std::vector<std::string>();
+// foo.baz->push_back("hello world");
+// uint32_t x = *foo.bar;
+//
+// Pointers to all the members are available with |Members|, allowing iteration:
+//
+// for (const RTCStatsMemberInterface* member : foo.Members()) {
+//   printf("%s = %s\n", member->name(), member->ValueToString().c_str());
+// }
+class RTCStats {
+ public:
+  RTCStats(const std::string& id, double timestamp)
+      : id_(id), timestamp_(timestamp) {}
+  RTCStats(std::string&& id, double timestamp)
+      : id_(std::move(id)), timestamp_(timestamp) {}
+  virtual ~RTCStats() {}
+
+  virtual std::unique_ptr<RTCStats> copy() const = 0;
+
+  const std::string& id() const { return id_; }
+  // Time relative to the UNIX epoch (Jan 1, 1970, UTC), in seconds.
+  double timestamp() const { return timestamp_; }
+  // Returns the address of the static |kType| variable of the implementing
+  // class. Comparing it to |&T::kType| tests if a stats object is of type |T|.
+  virtual const char* const* type() const = 0;

[nisse-webrtc, 2016/08/23 08:27:28]

Do you really need to return a pointer to a pointer? As far as I see,

  static const char kType[] = "integer";
  const char *type() { return kType; }

should be sufficient to ensure that stat_A->type() == stat_B->type() is true if
and only if stat_A and stat_B are of the same type.

(Might be subtly different if you do

  static const char* kType = "integer";

instead, then compiler might unify with other equal string literals, so don't do
that if you think that is a problem. But with kType[], you ought to get unique
storage for each class, even if for some reason type names are the same.)

[hbos, 2016/08/23 16:21:17]

Done. (You're right, const char[] works but not const char*/const char* const.)

+  // Returns the value of the static |kType| variable of the implementing class.
+  virtual const char* type_name() const = 0;
+  // Returns a vector of pointers to all the RTCStatsMemberInterface members of
+  // this class. This allows for iteration of members.
+  std::vector<const RTCStatsMemberInterface*> Members() const;
+
+  // Creates a human readable string representation of the report, listing all
+  // of its members (names and values).
+  std::string ToString() const;
+
+  // Downcasts the stats object to an |RTCStats| subclass |T|. DCHECKs that the
+  // object is of type |T|.
+  template<typename T>
+  T& cast_to() {
+    RTC_DCHECK_EQ(type(), &T::kType);

[nisse-webrtc, 2016/08/23 08:27:28]

I would expect that you don't do cast and then modify. If you agree, I'd suggest
making this method and its return value const.

Nice that you can do this poor-man's-dynamic_cast without macros.

[hbos, 2016/08/23 16:21:18]

With a const return value on the other |cast_to| method, this one can be
removed.

+    return static_cast<T&>(*this);
+  }
+  template<typename T>
+  T& cast_to() const {
+    RTC_DCHECK_EQ(type(), &T::kType);
+    return static_cast<T&>(*this);

[nisse-webrtc, 2016/08/23 08:27:28]

And this one really ought to have a const return value, const T&, right?

[hbos, 2016/08/23 16:21:17]

Done.

+  }
+
+ protected:
+  // Gets a vector of all members of this |RTCStats| object, including members
+  // derived from parent classes. |additional_capacity| is how many more members
+  // shall be reserved in the vector (so that subclasses can allocate a vector
+  // with room for both parent and child members without it having to resize).
+  virtual std::vector<const RTCStatsMemberInterface*>
+  MembersOfThisObjectAndAncestors(
+      size_t additional_capacity) const;
+
+  std::string const id_;
+  double timestamp_;
+};
+
+// All |RTCStats| classes should use this macro in a public section of the class
+// definition.
+//
+// This macro declares the static |kType| and overrides methods as required by
+// subclasses of |RTCStats|: |copy|, |type|, |type_name| and
+// |MembersOfThisObjectAndAncestors|. The |...| argument is a list of addresses
+// to each member defined in the implementing class (list cannot be empty, must
+// have at least one new member).
+//
+// (Since class names need to be known to implement these methods this cannot be
+// part of the base |RTCStats|. While these methods could be implemented using
+// templates, that would only work for immediate subclasses. Subclasses of
+// subclasses also have to override these methods, resulting in boilerplate
+// code. Using a macro avoids this and works for any |RTCStats| class, including
+// grandchildren.)
+//
+// Sample usage:
+//
+// rtcfoostats.h:
+//   class RTCFooStats : public RTCStats {
+//    public:
+//     RTCFooStats(const std::string& id, double timestamp)
+//         : RTCStats(id, timestamp),
+//           foo("foo"),
+//           bar("bar") {
+//     }
+//
+//     WEBRTC_RTCSTATS_IMPL(RTCStats, RTCFooStats,
+//         &foo,
+//         &bar);
+//
+//     RTCStatsMember<int32_t> foo;
+//     RTCStatsMember<int32_t> bar;
+//   };
+//
+// rtcfoostats.cc:
+//   const char* const RTCFooStats::kType = "foo-stats";
+//
+#define WEBRTC_RTCSTATS_IMPL(parent_class, this_class, ...)                    \

[nisse-webrtc, 2016/08/23 08:27:28]

You have checked that variadic macros, with ..., are standard enough to be used?
(There are ugly workarounds if they aren't).

[hbos, 2016/08/23 16:21:18]

Yes, it is used by other code (e.g. webrtc/base/logging.h) and compiles on all
trybots.

+ public:                                                                       \
+  static const char* const kType;                                              \

[nisse-webrtc, 2016/08/23 08:27:28]

As above, I'd suggest making this

  static const char kType[] = name;

Where name should be an additional argument to the macro. Or you could even let
the preprocessor fill it out automatically using

  static const char kType[] = #this_class;

if you think that stringified name is pretty enough.

[hbos, 2016/08/23 16:21:18]

Unfortunately, the variable can only be declared in the class definition, the
variable must be defined separately in a .cc file and " = ...;" is as such not
part of the macro.

+  std::unique_ptr<webrtc::RTCStats> copy() const override {                    \
+    return std::unique_ptr<webrtc::RTCStats>(new this_class(*this));           \
+  }                                                                            \
+  const char* const* type() const override { return &this_class::kType; }      \
+  const char* type_name() const override { return this_class::kType; }         \
+ protected:                                                                    \
+  std::vector<const webrtc::RTCStatsMemberInterface*>                          \
+  MembersOfThisObjectAndAncestors(                                             \
+      size_t additional_capacity) const override {                             \
+    const webrtc::RTCStatsMemberInterface* members[] = {                       \

[nisse-webrtc, 2016/08/23 08:27:28]

In general when doing large macros, you have to take care to name local
variables to that they are highly unlikely to collides with names used outside
of the macro. Consider the case where one of the macro arguments in __VA_ARGS__
happens to be &members. 

Not sure if the styleguide proposes a particular style here, but to be safe,
you'd need to replace "members" with something more painful, like
"rtcstats_impl_members" and similarly for all other local variables.

[hbos, 2016/08/23 16:21:18]

Done. Nice catch. I couldn't find anything in the style guide about this and
code I found for reference had no special variable names. I renamed them to
"local_var_...".

+      __VA_ARGS__                                                              \
+    };                                                                         \
+    size_t members_count = sizeof(members) / sizeof(members[0]);               \
+    std::vector<const webrtc::RTCStatsMemberInterface*> members_vec =          \
+        parent_class::MembersOfThisObjectAndAncestors(                         \
+            additional_capacity + members_count);                              \

[nisse-webrtc, 2016/08/23 08:27:28]

How do you test this allocation logic? Is there some test case that will raise
an assert in case the vector is nevertheless resized?

[hbos, 2016/08/23 16:21:18]

I added a DCHECK that ensures the capacity is enough to avoid resize.

+    members_vec.insert(members_vec.end(),                                      \
+                       &members[0], &members[members_count]);                  \

[nisse-webrtc, 2016/08/23 08:27:28]

There's an extra copy, first into the members array, then into members_vec.
Maybe not a big deal, but I wonder if there's some way to get around that.

[hbos, 2016/08/23 16:21:18]

Variadic functions come to mind, but I don't think it would improve performance,
only make it harder to read. (I'm guessing va_... stuff works like an array
behind the scenes anyway.)

+    return members_vec;                                                        \
+  }                                                                            \
+ public:
+
+// Interface for |RTCStats| members, which have a name and a value of a type
+// defined in a subclass. Only the types listed in |Type| are supported, these
+// are implemented by |RTCStatsMember<T>|. The value of a member may be
+// undefined, the value can only be read if |is_defined|.
+class RTCStatsMemberInterface {
+ public:
+  // Member value types.
+  enum Type {

[nisse-webrtc, 2016/08/23 08:27:28]

Hmm, this was a bit confusing at first. Here types are identified by enums,
while above they're identified by strings (or pointers thereto), which seems
inconsistent.

But the enums are used for primitive types, while strings are used for
rtc-specific aggregates, right?

[hbos, 2016/08/23 16:21:18]

That is correct.

The RTCStats type is for RTCStats subclasses. If an enum was used (original
patch set had that) then this file/the RTCStats class would have to know of all
subclasses, and adding custom "stats dictionaries" would require modifying this
file, kind of like modifying an interface in the act of implementing it. const
char kType[] was a means to decouple rtcstats.h from implementation files.

We could have done the same thing for RTCStatsMemberInterface, but since the
type of members is a short list that is likely to be fixed, an enum made sense
for readability and simplicity.

+    kInt32,                 // int32_t
+    kUint32,                // uint32_t
+    kInt64,                 // int64_t
+    kUint64,                // uint64_t
+    kDouble,                // double
+    kStaticString,          // const char*
+    kString,                // std::string
+
+    kSequenceInt32,         // std::vector<int32_t>
+    kSequenceUint32,        // std::vector<uint32_t>
+    kSequenceInt64,         // std::vector<int64_t>
+    kSequenceUint64,        // std::vector<uint64_t>
+    kSequenceDouble,        // std::vector<double>
+    kSequenceStaticString,  // std::vector<const char*>
+    kSequenceString,        // std::vector<std::string>
+  };
+
+  virtual ~RTCStatsMemberInterface() {}
+
+  const char* name() const { return name_; }
+  virtual Type type() const = 0;
+  virtual bool is_sequence() const = 0;
+  virtual bool is_string() const = 0;  // true for sequences of strings too.

[nisse-webrtc, 2016/08/23 08:27:28]

That's kind of surprising. Why?

Only use I've seen is 

  if (is_string() && !is_sequence())

so it seems it makes things more complicated, not less.

[hbos, 2016/08/23 16:21:17]

Hmm, yeah, strange decision, I'll make it false instead.

+  bool is_defined() const { return is_defined_; }
+  virtual std::string ValueToString() const = 0;
+
+  template<typename T>
+  T& cast_to() const {

[nisse-webrtc, 2016/08/23 08:27:28]

I think the return type should be const T&.

[hbos, 2016/08/23 16:21:18]

Done.

+    RTC_DCHECK_EQ(type(), T::kType);
+    return static_cast<T&>(*this);
+  }
+
+ protected:
+  RTCStatsMemberInterface(const char* name, bool is_defined)
+      : name_(name), is_defined_(is_defined) {}
+
+  const char* const name_;
+  bool is_defined_;
+};
+
+// Template implementation of |RTCStatsMemberInterface|. Every possible |T| is
+// specialized in rtcstats.cc, using a different |T| results in a linker error
+// (undefined reference to |kType|). The supported types are the ones described
+// by |RTCStatsMemberInterface::Type|.
+template<typename T>
+class RTCStatsMember : public RTCStatsMemberInterface {
+ public:
+  static const Type kType;
+
+  explicit RTCStatsMember(const char* name)
+      : RTCStatsMemberInterface(name, false),
+        value_() {}
+  RTCStatsMember(const char* name, const T& value)
+      : RTCStatsMemberInterface(name, true),
+        value_(value) {}
+  RTCStatsMember(const char* name, T&& value)
+      : RTCStatsMemberInterface(name, true),
+        value_(std::move(value)) {}
+  explicit RTCStatsMember(const RTCStatsMember<T>& other)
+      : RTCStatsMemberInterface(other.name_, other.is_defined_),
+        value_(other.value_) {}
+  explicit RTCStatsMember(RTCStatsMember<T>&& other)
+      : RTCStatsMemberInterface(other.name_, other.is_defined_),
+        value_(std::move(other.value_)) {}
+
+  Type type() const override { return kType; }
+  bool is_sequence() const override;
+  bool is_string() const override;
+  std::string ValueToString() const override;
+
+  // Assignment operators.
+  T& operator=(const T& value) {
+    value_ = value;
+    is_defined_ = true;
+    return value_;
+  }
+  T& operator=(const T&& value) {
+    value_ = std::move(value);
+    is_defined_ = true;
+    return value_;
+  }
+  T& operator=(const RTCStatsMember<T>& other) {
+    RTC_DCHECK(other.is_defined_);
+    value_ = other.is_defined_;
+    is_defined_ = true;
+    return value_;
+  }
+
+  // Value getters.
+  T& operator*() {
+    RTC_DCHECK(is_defined_);
+    return value_;
+  }
+  const T& operator*() const {
+    RTC_DCHECK(is_defined_);
+    return value_;
+  }
+
+  // Value getters, arrow operator.
+  T* operator->() {
+    RTC_DCHECK(is_defined_);
+    return &value_;
+  }
+  const T* operator->() const {
+    RTC_DCHECK(is_defined_);
+    return &value_;
+  }
+
+ private:
+  T value_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_API_RTCSTATS_H_