Cast:Updating logging stats

media/cast/logging/logging_defines.h

Index: media/cast/logging/logging_defines.h
diff --git a/media/cast/logging/logging_defines.h b/media/cast/logging/logging_defines.h
index f23aeb1ebbb12fa792a04477ab6922d02b117f84..737170df38c50e549795b461f93db1259aca610a 100644
--- a/media/cast/logging/logging_defines.h
+++ b/media/cast/logging/logging_defines.h
@@ -109,15 +109,39 @@ struct GenericEvent {
   std::vector<base::TimeTicks> timestamp;
 };
 
+// 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.

[imcheng, 2014/01/22 21:03:37]

For metrics that aren't always set, could you be more specific about when they
are set? Maybe a comment just above the fields saying "Applicable only for
kVideoRenderDelay" or something like that. That way it's easier to find out when
they're set without digging through too much code. Same with FrameEvent above.

[mikhal1, 2014/01/23 19:53:11]

Done.

 struct FrameLogStats {
   FrameLogStats();
   ~FrameLogStats();
+  base::TimeTicks first_event_time;
+  base::TimeTicks last_event_time;
+  int event_counter;
+  int size_sum;

[imcheng, 2014/01/22 21:03:37]

size_t?

[mikhal1, 2014/01/23 19:53:11]

Done.

+  base::TimeDelta min_delay;
+  base::TimeDelta max_delay;
+  base::TimeDelta sum_delay;
+};
+
+struct PacketLogStats {
+  PacketLogStats();
+  ~PacketLogStats();
+  base::TimeTicks first_event_time;
+  base::TimeTicks last_event_time;
+  int event_counter;
+  size_t size_sum;

[hguihot1, 2014/01/22 21:13:05]

|size_sum| here, but FrameLogStats uses |sum_delay|. Naming should be
consistent.

[mikhal1, 2014/01/23 19:53:11]

Done.

+};
 
-  double framerate_fps;
-  double bitrate_kbps;
-  int max_delay_ms;
-  int min_delay_ms;
-  int avg_delay_ms;
+struct GenericLogStats {
+  GenericLogStats();
+  ~GenericLogStats();
+  base::TimeTicks first_event_time;
+  base::TimeTicks last_event_time;
+  int event_counter;
+  int sum;

[hguihot1, 2014/01/22 21:13:05]

Potential overflow? Should we use 64-bit for all sums?

[mikhal1, 2014/01/23 19:53:11]

Depends on how often you'll reset it. Let's stick with this, and increment to 64
if we see there's a problem.
On 2014/01/22 21:13:05, hguihot1 wrote:

+  int min;
+  int max;

[hguihot1, 2014/01/22 21:13:05]

Having sum of squares as well would allow the caller to compute variance /
standard variation.

[mikhal1, 2014/01/23 19:53:11]

On the granularity of every time you poll it. If you're fine with that. Then I'm
adding it.
On 2014/01/22 21:13:05, hguihot1 wrote:

 };
 
 struct ReceiverRtcpEvent {
@@ -139,8 +163,9 @@ typedef std::multimap<uint32, ReceiverRtcpEvent> AudioRtcpRawMap;
 typedef std::multimap<uint32, ReceiverRtcpEvent> VideoRtcpRawMap;
 
 typedef std::map<CastLoggingEvent, linked_ptr<FrameLogStats > > FrameStatsMap;

[imcheng, 2014/01/22 21:03:37]

Question: Why do stats maps use linked_ptrs but the ones above don't? Would
prefer to keep them consistent if possible.

[mikhal1, 2014/01/23 19:53:11]

Done.

-typedef std::map<CastLoggingEvent, double> PacketStatsMap;
-typedef std::map<CastLoggingEvent, double> GenericStatsMap;
+typedef std::map<CastLoggingEvent, linked_ptr<PacketLogStats > > PacketStatsMap;
+typedef std::map<CastLoggingEvent, linked_ptr<GenericLogStats > >
+    GenericStatsMap;
 
 }  // namespace cast
 }  // namespace media