Cast: Provide more meaningful stats.

media/cast/logging/stats_event_subscriber.h

Index: media/cast/logging/stats_event_subscriber.h
diff --git a/media/cast/logging/stats_event_subscriber.h b/media/cast/logging/stats_event_subscriber.h
index 85419823a6c6e55e30d790f5e6c024522d00f40e..10041dc701c08faa91db1e9c5c556e78d49d7201 100644
--- a/media/cast/logging/stats_event_subscriber.h
+++ b/media/cast/logging/stats_event_subscriber.h
@@ -5,18 +5,30 @@
 #ifndef MEDIA_CAST_LOGGING_STATS_EVENT_SUBSCRIBER_H_
 #define MEDIA_CAST_LOGGING_STATS_EVENT_SUBSCRIBER_H_
 
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/tick_clock.h"
 #include "media/cast/logging/logging_defines.h"
 #include "media/cast/logging/raw_event_subscriber.h"
+#include "media/cast/logging/receiver_time_offset_estimator.h"
+
+namespace base {
+class DictionaryValue;
+}
 
 namespace media {
 namespace cast {
 
+class StatsEventSubscriberTest;
+
 // A RawEventSubscriber implementation that subscribes to events,
 // and aggregates them into stats.
 class StatsEventSubscriber : public RawEventSubscriber {
  public:
-  StatsEventSubscriber(EventMediaType media_type);
+  StatsEventSubscriber(EventMediaType event_media_type,
+                       base::TickClock* clock,
+                       ReceiverTimeOffsetEstimator* offset_estimator);
 
   virtual ~StatsEventSubscriber();
 
@@ -26,23 +38,132 @@ class StatsEventSubscriber : public RawEventSubscriber {
   virtual void OnReceiveGenericEvent(const GenericEvent& generic_event)
       OVERRIDE;
 
-  // Assigns |frame_stats_map| with frame stats.
-  void GetFrameStats(FrameStatsMap* frame_stats_map) const;
-
-  // Assigns |packet_stats_map| with packet stats.
-  void GetPacketStats(PacketStatsMap* packet_stats_map) const;
+  // Returns stats as a DictionaryValue. The dictionary contains one entry -
+  // "audio" or "video" pointing to an inner dictionary.
+  // The inner dictionary consists of string - double entries, where the string
+  // describes the name of the stat, and the double describes
+  // the value of the stat. See CastStat and StatsMap below.
+  scoped_ptr<base::DictionaryValue> GetStats() const;
 
-  // Assigns |generic_stats_map| with generic stats data.
-  void GetGenericStats(GenericStatsMap* generic_stats_map) const;
-
-  // Resets all stats maps in this object.
+  // Resets stats in this object.
   void Reset();
 
  private:
+  friend class StatsEventSubscriberTest;
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, EmptyStats);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, Capture);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, Encode);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, Decode);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, PlayoutDelay);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, E2ELatency);
+  FRIEND_TEST_ALL_PREFIXES(StatsEventSubscriberTest, Packets);
+
+  // Generic statistics given the raw data. More specific data (e.g. frame rate
+  // and bit rate) can be computed given the basic metrics.
+  // Some of the metrics will only be set when applicable, e.g. delay and size.
+  struct FrameLogStats {
+    FrameLogStats();
+    ~FrameLogStats();
+    int event_counter;
+    size_t sum_size;
+    base::TimeDelta sum_delay;
+  };
+
+  struct PacketLogStats {
+    PacketLogStats();
+    ~PacketLogStats();
+    int event_counter;
+    size_t sum_size;
+  };
+
+  enum CastStat {
+    // Capture frame rate.
+    CAPTURE_FPS,
+    // Encode frame rate.
+    ENCODE_FPS,
+    // Decode frame rate.
+    DECODE_FPS,
+    // Average encode duration in milliseconds.
+    // TODO(imcheng): This stat is not populated yet because we do not have
+    // the time when encode started. Record it in kVideoFrameEncoded event.
+    AVG_ENCODE_TIME_MS,

[miu, 2014/04/18 00:02:03]

Why not microseconds for all of these?  Or, let's be sure there isn't any
upstream or downstream code that will static_cast the values to an integer data
type blindly (e.g., for the protobufs).

[imcheng, 2014/04/18 18:20:27]

I made sure of the latter.

+    // Average playout delay in milliseconds, with target delay already
+    // accounted for. Ideally, every frame should have a playout delay of 0.
+    AVG_PLAYOUT_DELAY_MS,
+    // Duration from when a packet is transmitted to when it is received.
+    // This measures latency from sender to receiver.
+    AVG_NETWORK_LATENCY_MS,
+    // Duration from when a frame is captured to when it should be played out.
+    AVG_E2E_LATENCY_MS,
+    // Encode bitrate in kbps.
+    ENCODE_KBPS,
+    // Packet transmission bitrate in kbps.
+    TRANSMISSION_KBPS,
+    // Packet retransmission bitrate in kbps.
+    RETRANSMISSION_KBPS,
+    // Percentage of packet loss.
+    PACKET_LOSS_PERCENTAGE,

[miu, 2014/04/18 00:02:03]

Suggest you use fraction instead of percentage here (i.e., don't multiply values
by 100).  It's likely packet loss will be measured using the basis point scale
in a normal WiFi environment (I hope, for the sake of our users!).

[imcheng, 2014/04/18 18:20:27]

Done.

+    // Duration in milliseconds since last receiver response.
+    MS_SINCE_LAST_RECEIVER_RESPONSE
+  };
+
+  typedef std::map<CastStat, double> StatsMap;
+  typedef std::map<RtpTimestamp, base::TimeTicks> FrameEventTimeMap;
+  typedef std::map<std::pair<RtpTimestamp, uint16>, base::TimeTicks>
+      PacketEventTimeMap;
+  typedef std::map<CastLoggingEvent, FrameLogStats> FrameStatsMap;
+  typedef std::map<CastLoggingEvent, PacketLogStats> PacketStatsMap;
+
+  static const char* CastStatToString(CastStat stat);
+
+  // // Assigns |stats_map| with stats data. Used for testing.

[miu, 2014/04/18 00:02:03]

nit: Duplicate //

[imcheng, 2014/04/18 18:20:27]

Done.

+  void GetStatsInternal(StatsMap* stats_map) const;
+
+  bool GetReceiverOffset(base::TimeDelta* offset);
+  void RecordFrameCapturedTime(const FrameEvent& frame_event);
+  void RecordE2ELatency(const FrameEvent& frame_event);
+  void RecordPacketSentTime(const PacketEvent& packet_event);
+  void ErasePacketSentTime(const PacketEvent& packet_event);
+  void RecordNetworkLatency(const PacketEvent& packet_event);
+
+  void PopulateFpsStat(base::TimeTicks now,
+                       CastLoggingEvent event,
+                       CastStat stat,
+                       StatsMap* stats_map) const;
+  void PopulatePlayoutDelayStat(StatsMap* stats_map) const;
+  void PopulateFrameBitrateStat(base::TimeTicks now, StatsMap* stats_map) const;
+  void PopulatePacketBitrateStat(base::TimeTicks now,
+                                 CastLoggingEvent event,
+                                 CastStat stat,
+                                 StatsMap* stats_map) const;
+  void PopulatePacketLossPercentageStat(StatsMap* stats_map) const;
+
   EventMediaType event_media_type_;

[miu, 2014/04/18 00:02:03]

const, please.  :)

[imcheng, 2014/04/18 18:20:27]

Done.

   FrameStatsMap frame_stats_;
   PacketStatsMap packet_stats_;
-  GenericStatsMap generic_stats_;
+
+  int64 total_network_latency_ms_;

[miu, 2014/04/18 00:02:03]

This member and total_e2e_latency_ms_ should be base::TimeDelta.  The code in
the .cc file is throwing away the sub-milliseconds component of time (which is
significant), and then accumulating the errors over several
RecordPacketSendTime() and RecordE2ELatency() calls.

[imcheng, 2014/04/18 18:20:27]

Good catch. Thanks.

+  int network_latency_datapoints_;
+  int64 total_e2e_latency_ms_;
+  int e2e_latency_datapoints_;
+
+  base::TimeTicks last_response_received_time_;
+
+  // Fixed size map to record when recent frames were captured.
+  FrameEventTimeMap frame_captured_times_;
+
+  // Fixed size map to record when recent packets were sent.
+  PacketEventTimeMap packet_sent_times_;
+
+  // Sender time assigned on creation and |Reset()|.
+  base::TimeTicks start_time_;
+
+  // Not owned by this class.
+  base::TickClock* clock_;

[miu, 2014/04/18 00:02:03]

Since clock_ and offset_estimator_ are unchanging pointers, injected via the
ctor, could you make them const and move them up to just after event_media_type_
above?

[imcheng, 2014/04/18 18:20:27]

Done.

+
+  // Not owned by this class.
+  ReceiverTimeOffsetEstimator* offset_estimator_;
+
   base::ThreadChecker thread_checker_;
   DISALLOW_COPY_AND_ASSIGN(StatsEventSubscriber);
 };