Cast:Updating logging stats

media/cast/logging/logging_defines.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_CAST_LOGGING_LOGGING_DEFINES_H_
 #define MEDIA_CAST_LOGGING_LOGGING_DEFINES_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 91 matching lines @@
   BasePacketMap packet_map;
 };
 
 struct GenericEvent {
   GenericEvent();
   ~GenericEvent();
   std::vector<int> value;
   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;
+};
 
-  double framerate_fps;
-  double bitrate_kbps;
-  int max_delay_ms;
-  int min_delay_ms;
-  int avg_delay_ms;
+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.

+};
+
+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 {
   ReceiverRtcpEvent();
   ~ReceiverRtcpEvent();
 
   CastLoggingEvent type;
   base::TimeTicks timestamp;
   base::TimeDelta delay_delta;  // Render/playout delay.
   uint16 packet_id;
 };
 
 // Store all log types in a map based on the event.
 typedef std::map<uint32, FrameEvent> FrameRawMap;
 typedef std::map<uint32, PacketEvent> PacketRawMap;
 typedef std::map<CastLoggingEvent, GenericEvent> GenericRawMap;
 
 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
 
 #endif  // MEDIA_CAST_LOGGING_LOGGING_DEFINES_H_