Cast:Updating logging stats

media/cast/logging/logging_stats.cc

Index: media/cast/logging/logging_stats.cc
diff --git a/media/cast/logging/logging_stats.cc b/media/cast/logging/logging_stats.cc
index 2cdabcc5df2e1774b3a29ff7f5f2a5de55e3ad7f..4a24941ddcddcb7cc220917df0b0cd476f31d142 100644
--- a/media/cast/logging/logging_stats.cc
+++ b/media/cast/logging/logging_stats.cc
@@ -12,10 +12,7 @@ namespace cast {
 LoggingStats::LoggingStats()
     : frame_stats_(),
       packet_stats_(),
-      generic_stats_(),
-      start_time_() {
-  memset(counts_, 0, sizeof(counts_));
-  memset(start_time_, 0, sizeof(start_time_));
+      generic_stats_() {
 }
 
 LoggingStats::~LoggingStats() {}
@@ -24,7 +21,6 @@ void LoggingStats::Reset() {
   frame_stats_.clear();
   packet_stats_.clear();
   generic_stats_.clear();
-  memset(counts_, 0, sizeof(counts_));
 }
 
 void LoggingStats::InsertFrameEvent(const base::TimeTicks& time_of_event,
@@ -44,7 +40,7 @@ void LoggingStats::InsertFrameEventWithSize(
   // Update size.
   FrameStatsMap::iterator it = frame_stats_.find(event);
   DCHECK(it != frame_stats_.end());
-  it->second->bitrate_kbps += frame_size;
+  it->second->size_sum += frame_size;
 }
 
 void LoggingStats::InsertFrameEventWithDelay(
@@ -59,12 +55,11 @@ void LoggingStats::InsertFrameEventWithDelay(
   DCHECK(it != frame_stats_.end());
   // Using the average delay as a counter, will divide by the counter when

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

Remove comment

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

Done.

   // triggered.
-  it->second->avg_delay_ms += delay.InMilliseconds();
-  if (delay.InMilliseconds() > it->second->max_delay_ms)
-    it->second->max_delay_ms = delay.InMilliseconds();
-  if ((delay.InMilliseconds() < it->second->min_delay_ms) ||
-      (counts_[event] == 1) )
-    it->second->min_delay_ms = delay.InMilliseconds();
+  it->second->sum_delay += delay;
+  if (delay > it->second->max_delay || it->second->event_counter == 1)
+    it->second->max_delay = delay;
+  if (delay < it->second->min_delay || it->second->event_counter == 1)
+    it->second->min_delay = delay;
 }
 
 void LoggingStats::InsertBaseFrameEvent(const base::TimeTicks& time_of_event,

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

Maybe have this function return a pointer to the FrameLogStats, so you don't
have to do another lookup in InsertFrameEventWithSize /
InsertFrameEventWithDelay?

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

Was deliberating about that one too. I will still need to do the look up in case
of the first element, but can keep it in this function (still will be slightly
ugly).
What do you say? 
On 2014/01/22 21:03:37, imcheng1 wrote:

[imcheng, 2014/01/23 21:17:18]

In the case of inserting, map::insert returns a pair<iterator,bool> where the
iterator is pointing to the inserted element.

On 2014/01/23 19:53:11, mikhal1 wrote:

@@ -75,12 +70,15 @@ void LoggingStats::InsertBaseFrameEvent(const base::TimeTicks& time_of_event,
   FrameStatsMap::iterator it = frame_stats_.find(event);
   if (it == frame_stats_.end()) {
     // New event.
-    start_time_[event] = time_of_event;
     linked_ptr<FrameLogStats> stats(new FrameLogStats());
+    stats->first_event_time = time_of_event;
+    stats->last_event_time = time_of_event;
+    stats->event_counter = 1;
     frame_stats_.insert(std::make_pair(event, stats));
+  } else {
+    it->second->last_event_time = time_of_event;
+    ++it->second->event_counter;

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

nit: Does this work? I am paranoid about operator precedence here.

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

my tests implies this works, but I'm adding a parenthesis to ease your paranoia.
On 2014/01/22 21:03:37, imcheng1 wrote:

[imcheng, 2014/01/23 21:17:18]

Thanks!

On 2014/01/23 19:53:11, mikhal1 wrote:

   }
-
-  ++counts_[event];
 }
 
 void LoggingStats::InsertPacketEvent(const base::TimeTicks& time_of_event,
@@ -94,13 +92,17 @@ void LoggingStats::InsertPacketEvent(const base::TimeTicks& time_of_event,
   PacketStatsMap::iterator it = packet_stats_.find(event);
   if (it == packet_stats_.end()) {
     // New event.
-    start_time_[event] = time_of_event;
-    packet_stats_.insert(std::make_pair(event, size));
+    linked_ptr<PacketLogStats> stats(new PacketLogStats());
+    stats->first_event_time = time_of_event;
+    stats->last_event_time = time_of_event;
+    stats->size_sum = size;
+    stats->event_counter = 1;
+    packet_stats_.insert(std::make_pair(event, stats));
   } else {
-    // Add to existing.
-    it->second += size;
+    // Add to an existing event.
+    it->second->size_sum += size;
+    ++it->second->event_counter;

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

Same as above regarding operator precedence.

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

Done.

   }
-  ++counts_[event];
 }
 
 void LoggingStats::InsertGenericEvent(const base::TimeTicks& time_of_event,
@@ -109,49 +111,36 @@ void LoggingStats::InsertGenericEvent(const base::TimeTicks& time_of_event,
   GenericStatsMap::iterator it = generic_stats_.find(event);
   if (it == generic_stats_.end()) {
     // New event.
-    start_time_[event] = time_of_event;
-    generic_stats_.insert(std::make_pair(event, value));
+    linked_ptr<GenericLogStats> stats(new GenericLogStats());
+    stats->first_event_time = time_of_event;
+    stats->last_event_time = time_of_event;
+    stats->sum = value;
+    stats->min = value;
+    stats->max = value;
+    stats->event_counter = 1;
+    generic_stats_.insert(std::make_pair(event, stats));
   } else {
-    // Add to existing (will be used to compute average).
-    it->second += value;
+    // Add to existing event.
+    it->second->sum += value;
+    ++it->second->event_counter;

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

Same as above regarding operator precedence.

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

Done.

+    it->second->last_event_time = time_of_event;
+    if (it->second->min > value) {
+      it->second->min = value;
+    } else if (it->second->max < value) {
+      it->second->max = value;
+    }
   }
-  ++counts_[event];
 }
 
-const FrameStatsMap* LoggingStats::GetFrameStatsData(
-    const base::TimeTicks& now) {
-  // Compute framerate and bitrate (when available).
-  FrameStatsMap::iterator it;
-  for (it = frame_stats_.begin(); it != frame_stats_.end(); ++it) {
-    base::TimeDelta time_diff = now - start_time_[it->first];
-    it->second->framerate_fps = counts_[it->first] / time_diff.InSecondsF();
-    if (it->second->bitrate_kbps > 0) {
-      it->second->bitrate_kbps = (8 / 1000) * it->second->bitrate_kbps /
-          time_diff.InSecondsF();
-    }
-    if (it->second->avg_delay_ms > 0)
-      it->second->avg_delay_ms /= counts_[it->first];
-  }
+const FrameStatsMap* LoggingStats::GetFrameStatsData() const {
   return &frame_stats_;
 }
 
-const PacketStatsMap* LoggingStats::GetPacketStatsData(
-    const base::TimeTicks& now) {
-  PacketStatsMap::iterator it;
-  for (it = packet_stats_.begin(); it != packet_stats_.end(); ++it) {
-    if (counts_[it->first] == 0) continue;
-    base::TimeDelta time_diff = now - start_time_[it->first];
-    it->second = (8 / 1000) * it->second / time_diff.InSecondsF();
-  }
+const PacketStatsMap* LoggingStats::GetPacketStatsData() const {
   return &packet_stats_;
 }
 
-const GenericStatsMap* LoggingStats::GetGenericStatsData() {
-  // Compute averages.
-  GenericStatsMap::iterator it;
-  for (it = generic_stats_.begin(); it != generic_stats_.end(); ++it) {
-    it->second /= counts_[ it->first];
-  }
+const GenericStatsMap* LoggingStats::GetGenericStatsData() const {
   return &generic_stats_;
 }