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..84811f2c270b7ae7e410f881704d8a778dccb3d1 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
@@ -339,11 +342,20 @@ std::vector<std::pair<int, int> > CalculateFrameClusters(FILE* file) {
   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;

[kjellander_webrtc, 2017/02/06 12:30:38]

Can we increment a counter for this so we can log this as a perf metric?

[mandermo, 2017/02/07 14:25:27]

I have now added a separate counter for reference and test and print them in the
end.

+    } 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,7 +384,7 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
   }
 
   int max_repeated_frames = 1;
-  int max_skipped_frames = 1;
+  int max_skipped_frames = 0;
 
   std::vector<std::pair<int, int> > frame_cnt_ref =
       CalculateFrameClusters(stats_file_ref);
@@ -393,9 +405,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;

[kjellander_webrtc, 2017/02/06 12:30:38]

It seems we should start returning an exit code from the application instead of
just printing something and returning void.

Then
https://cs.chromium.org/chromium/src/third_party/webrtc/tools/frame_analyzer/...
could just be updated to return the int and we would get test failures for cases
like this assuming the calling script for frame analyzer is looking at the exit
code.

With that, it would make sense to start writing a few unit tests for
PrintMaxRepeatedAndSkippedFrames that verifies the code for the different cases.
With this CL there's now so much more error handling logic so I think it makes a
lot of sense to have such tests now.

[mandermo, 2017/02/07 14:25:27]

Have added test cases covering different scenarios. The cases that return early
are less interesting and complicated than the different combinations of decode
errors I cover.

+  }
+
   // 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 +427,54 @@ void PrintMaxRepeatedAndSkippedFrames(FILE* output,
     return;
   }
 
+  // The test frames that does not have corresponding barcode in the reference
+  // 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;
+    auto old_it_ref = it_ref;
     ++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) {
+        passed_error = true;
+        continue;
+      }
+      if (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) {
+      skipped_frames = 0;
     }
-    if (skipped_frames > max_skipped_frames) {
-      max_skipped_frames = skipped_frames;
+    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;
+    }
+    no_match_in_ref.push_back(it_test->first);
+    if (it_ref == end_ref) {
+      break;
+    } else {
+      it_ref = old_it_ref;
     }
   }
 
@@ -432,6 +482,16 @@ 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);
+  fprintf(output, "RESULT No_matched: [");
+  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) {