Better comparison for frame analyzer

webrtc/tools/frame_analyzer/video_quality_analysis.cc

Index: webrtc/tools/frame_analyzer/video_quality_analysis.cc
diff --git a/webrtc/tools/frame_analyzer/video_quality_analysis.cc b/webrtc/tools/frame_analyzer/video_quality_analysis.cc
index dfb7d900657540ca87adc6e38e79b143a721abe6..18841f7e570cc5e670345bf03ddb531635f6a4b8 100644
--- a/webrtc/tools/frame_analyzer/video_quality_analysis.cc
+++ b/webrtc/tools/frame_analyzer/video_quality_analysis.cc
@@ -328,6 +328,9 @@ void PrintMaxRepeatedAndSkippedFrames(const std::string& label,
 }
 
 namespace {
+// The barcode number that means that the barcode could not be decoded.
+const int DECODE_ERROR = -1;
+
 // Clusters the frames in the file. First in the pair is the frame number and
 // second is the number
 // of frames in that cluster. So if first frame in video has number 100 and it
@@ -335,15 +338,32 @@ namespace {
 // each other, then the first entry in the returned vector has first set to 100
 // and second set
 // to 3.

[kjellander_webrtc, 2017/02/08 12:39:34]

Can you explain in the comment what happens if there's are 3 frames in a row
that gets frame number decoded like this:
[1, DECODE_ERROR, 2]

How many clusters are returned? Please write a unit test for that.

Also write unit tests for: 
[1]
[1,1,2]
[1, 1, DECODE_ERROR, 3]
[DECODE_ERROR, DECODE_ERROR]

[mandermo, 2017/02/13 17:04:11]

Added comment. Should I move CalculateFrameClusters() out of the unnamed
namespace and put a declaration in video_quality_analysis.cc to make a test case
for it?

[kjellander_webrtc, 2017/02/14 20:55:58]

Please do, I think that'll be very useful in the future as this is complex stuff
with many edge cases.

[mandermo, 2017/02/15 11:52:15]

Done. Found I missed +1 when there are decode errors between frames with
identical barcodes.

-std::vector<std::pair<int, int> > CalculateFrameClusters(FILE* file) {
+std::vector<std::pair<int, int> > CalculateFrameClusters(
+    FILE* file,
+    int* num_decode_errors) {
+  if (num_decode_errors) {
+    *num_decode_errors = 0;
+  }
   std::vector<std::pair<int, int> > frame_cnt;
   char line[STATS_LINE_LENGTH];
   while (GetNextStatsLine(file, line)) {
-    int decoded_frame_number = ExtractDecodedFrameNumber(line);
-    if (decoded_frame_number == -1) {
-      continue;
+    int decoded_frame_number;
+    if (IsThereBarcodeError(line)) {
+      decoded_frame_number = DECODE_ERROR;
+      if (num_decode_errors) {
+        ++*num_decode_errors;
+      }
+    } else {
+      decoded_frame_number = ExtractDecodedFrameNumber(line);
     }
-    if (frame_cnt.empty() || frame_cnt.back().first != decoded_frame_number) {
+    if (frame_cnt.size() >= 2 && decoded_frame_number != DECODE_ERROR &&
+        frame_cnt.back().first == DECODE_ERROR &&
+        frame_cnt[frame_cnt.size() - 2].first == decoded_frame_number) {
+      // Handle when there is a decoding error inside a cluster of frames.
+      frame_cnt[frame_cnt.size() - 2].second += frame_cnt.back().second;
+      frame_cnt.pop_back();
+    } else if (frame_cnt.empty() ||
+               frame_cnt.back().first != decoded_frame_number) {
       frame_cnt.push_back(std::make_pair(decoded_frame_number, 1));
     } else {
       ++frame_cnt.back().second;
@@ -372,13 +392,16 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
   }
 
   int max_repeated_frames = 1;

[kjellander_webrtc, 2017/02/08 12:39:33]

Does max_repeated_frames = 1 as a starting value make sense or should it also be
0?

[mandermo, 2017/02/13 17:04:11]

You have a point. Fixing it will change the old behavior, let's change it in a
follow up CL.

-  int max_skipped_frames = 1;
+  int max_skipped_frames = 0;
+
+  int decode_errors_ref = 0;
+  int decode_errors_test = 0;
 
   std::vector<std::pair<int, int> > frame_cnt_ref =
-      CalculateFrameClusters(stats_file_ref);
+      CalculateFrameClusters(stats_file_ref, &decode_errors_ref);
 
   std::vector<std::pair<int, int> > frame_cnt_test =
-      CalculateFrameClusters(stats_file_test);
+      CalculateFrameClusters(stats_file_test, &decode_errors_test);
 
   fclose(stats_file_ref);
   fclose(stats_file_test);
@@ -393,9 +416,19 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
     return;
   }
 
+  while (it_test != end_test && it_test->first == DECODE_ERROR) {
+    ++it_test;
+  }
+
+  if (it_test == end_test) {
+    fprintf(stderr, "Test video only has barcode decode errors\n");
+    return;
+  }
+
   // Find the first frame in the reference video that match the first frame in
   // the test video.
-  while (it_ref != end_ref && it_ref->first != it_test->first) {
+  while (it_ref != end_ref &&
+         (it_ref->first == DECODE_ERROR || it_ref->first != it_test->first)) {
     ++it_ref;
   }
   if (it_ref == end_ref) {
@@ -405,26 +438,51 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
     return;
   }
 
+  // The test frames that does not have corresponding barcode in the reference

[kjellander_webrtc, 2017/02/08 12:39:33]

Do we have proof that this ever happens? If not I don't see why we would add
such complex code to look for it.
I don't see when it should happen if we always sample the reference screen fast
enough.

[mandermo, 2017/02/13 17:04:11]

Have removed that code and print an error instead if it happens anyway.

+  // video.
+  std::vector<int> no_match_in_ref;
+
+  int total_skipped = 0;
   for (;;) {
     max_repeated_frames =
         std::max(max_repeated_frames, it_test->second - it_ref->second + 1);
+
+    bool passed_error = false;
+
     ++it_test;
+    while (it_test != end_test && it_test->first == DECODE_ERROR) {
+      ++it_test;
+      passed_error = true;
+    }
     if (it_test == end_test) {
       break;
     }
+
     int skipped_frames = 0;
     ++it_ref;
-    while (it_ref != end_ref && it_ref->first != it_test->first) {
-      skipped_frames += it_ref->second;
-      ++it_ref;
+    for (; it_ref != end_ref; ++it_ref) {
+      if (it_ref->first != DECODE_ERROR && it_ref->first >= it_test->first) {
+        break;
+      }
+      ++skipped_frames;
     }
-    if (it_ref == end_ref) {
-      fprintf(stderr,
-              "The barcode in the test video is not in the reference video.\n");
-      return;
+    if (passed_error) {
+      // If we pass an error in the test video, then we are conservative
+      // and will not calculate skipped frames for that part.
+      skipped_frames = 0;
+    }
+    if (it_ref != end_ref && it_ref->first == it_test->first) {
+      total_skipped += skipped_frames;
+      if (skipped_frames > max_skipped_frames) {
+        max_skipped_frames = skipped_frames;
+      }
+      continue;
     }
-    if (skipped_frames > max_skipped_frames) {
-      max_skipped_frames = skipped_frames;
+    no_match_in_ref.push_back(it_test->first);
+    if (it_ref == end_ref) {
+      break;
+    } else {
+      --it_ref;
     }
   }
 
@@ -432,6 +490,20 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
           max_repeated_frames);
   fprintf(output, "RESULT Max_skipped: %s= %d\n", label.c_str(),
           max_skipped_frames);
+  fprintf(output, "RESULT Total_skipped: %s= %d\n", label.c_str(),
+          total_skipped);

[kjellander_webrtc, 2017/02/08 12:39:33]

Seeing this, I'd like to have total_repeated_frames as well. Could be done in a
follow-up CL though.

[kjellander_webrtc, 2017/02/08 12:39:33]

Name this variable total_skipped_frames to be consistent with the max_*_frames
variables.

[mandermo, 2017/02/13 17:04:11]

I don't think such a value provide any meaningful info.

[mandermo, 2017/02/13 17:04:11]

Done.

[kjellander_webrtc, 2017/02/14 20:55:58]

Why not? If we have periodic freezes, let's say every second, we will still only
see the maximum which may not be that much different from a normal run. If we
would track the total as well, such a scenario would show significant increase.

[mandermo, 2017/02/15 11:52:15]

Lets fix it in a follow up CL where we initialize max_repeated_frames to 0
instead of 1 and remove + 1 from the update of max_repeated_frames.

+  fprintf(output, "RESULT Decode_errors_reference: %s= %d\n", label.c_str(),
+          decode_errors_ref);
+  fprintf(output, "RESULT Decode_errors_test: %s= %d\n", label.c_str(),
+          decode_errors_test);
+  fprintf(output, "RESULT No_matched: [");

[kjellander_webrtc, 2017/02/08 12:39:34]

This is not suitable as a perf number. The perf dashboard is monitoring the
metrics reported and plots them in a graph in order to detect regressions when
the values are diverging.
Since we're printing frame numbers here, there'll always be a variation on the
numbers output here, making it pointless from a perf regression perspective.
It could be useful to output them to the console for future debugging, but not
as a RESULT line.

Plotting the size of no_match_in_ref as a RESULT line would make sense though.

[mandermo, 2017/02/13 17:04:11]

Done.

+  for (std::size_t i = 0; i < no_match_in_ref.size(); ++i) {
+    if (i > 0) {
+      fprintf(output, ", ");
+    }
+    fprintf(output, "%d", no_match_in_ref[i]);
+  }
+  fprintf(output, "]\n");
 }
 
 void PrintAnalysisResults(const std::string& label, ResultsContainer* results) {