Step #2: Add batch mode to VideoProcessor integration tests.

webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h

Index: webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
diff --git a/webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h b/webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
index 68fe68e7108764e391730c86c88ec841569866b1..55bc4efae4c45c15dc638c18982ce404fdc84062 100644
--- a/webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
+++ b/webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
@@ -28,6 +28,7 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/file.h"
+#include "webrtc/base/logging.h"
 #include "webrtc/media/engine/webrtcvideodecoderfactory.h"
 #include "webrtc/media/engine/webrtcvideoencoderfactory.h"
 #include "webrtc/modules/video_coding/codecs/h264/include/h264.h"
@@ -84,6 +85,12 @@ struct CodecParams {
 
   std::string filename;
   bool verbose_logging;
+
+  // In batch mode, the VideoProcessor is fed all the frames for processing
+  // before any metrics are calculated. This is useful for pipelining HW codecs,
+  // for which some calculated metrics otherwise would be incorrect. The
+  // downside with batch mode is that mid-test rate allocation is not supported.
+  bool batch_mode;
 };
 
 // Thresholds for the quality metrics.
@@ -342,7 +349,7 @@ class VideoProcessorIntegrationTest : public testing::Test {
 
   // Reset quantities after each encoder update, update the target
   // per-frame bandwidth.
-  void ResetRateControlMetrics(int num_frames) {
+  void ResetRateControlMetrics(int num_frames_to_hit_target) {
     for (int i = 0; i < num_temporal_layers_; i++) {
       num_frames_per_update_[i] = 0;
       sum_frame_size_mismatch_[i] = 0.0f;
@@ -364,35 +371,40 @@ class VideoProcessorIntegrationTest : public testing::Test {
     sum_encoded_frame_size_total_ = 0.0f;
     encoding_bitrate_total_ = 0.0f;
     perc_encoding_rate_mismatch_ = 0.0f;
-    num_frames_to_hit_target_ = num_frames;
+    num_frames_to_hit_target_ = num_frames_to_hit_target;
     encoding_rate_within_target_ = false;
     sum_key_frame_size_mismatch_ = 0.0;
     num_key_frames_ = 0;
   }
 
   // For every encoded frame, update the rate control metrics.
-  void UpdateRateControlMetrics(int frame_num, FrameType frame_type) {
-    float encoded_size_kbits = processor_->EncodedFrameSize() * 8.0f / 1000.0f;
+  void UpdateRateControlMetrics(int frame_number,
+                                FrameType frame_type,
+                                size_t encoded_frame_size) {
+    RTC_CHECK_GE(frame_number, 0);
+    int layer = LayerIndexForFrame(frame_number);

[sprang_webrtc, 2017/03/08 16:20:21]

Not obvious to me which layer this refers to?

[brandtr, 2017/03/09 15:18:27]

This function returns what TL this frame belongs to. I've updated the naming a
bit to make that more clear.

+    float encoded_size_kbits = encoded_frame_size * 8.0f / 1000.0f;
+
     // Update layer data.
     // Update rate mismatch relative to per-frame bandwidth for delta frames.
     if (frame_type == kVideoFrameDelta) {
       // TODO(marpan): Should we count dropped (zero size) frames in mismatch?
-      sum_frame_size_mismatch_[layer_] +=
-          fabs(encoded_size_kbits - per_frame_bandwidth_[layer_]) /
-          per_frame_bandwidth_[layer_];
+      sum_frame_size_mismatch_[layer] +=
+          fabs(encoded_size_kbits - per_frame_bandwidth_[layer]) /
+          per_frame_bandwidth_[layer];
     } else {
-      float target_size = (frame_num == 1) ? target_size_key_frame_initial_
-                                           : target_size_key_frame_;
+      float target_size = (frame_number == 0) ? target_size_key_frame_initial_
+                                              : target_size_key_frame_;
       sum_key_frame_size_mismatch_ +=
           fabs(encoded_size_kbits - target_size) / target_size;
       num_key_frames_ += 1;
     }
-    sum_encoded_frame_size_[layer_] += encoded_size_kbits;
-    // Encoding bitrate per layer: from the start of the update/run to the
+    sum_encoded_frame_size_[layer] += encoded_size_kbits;
+    // Encoding bit rate per layer: from the start of the update/run to the
     // current frame.
-    encoding_bitrate_[layer_] = sum_encoded_frame_size_[layer_] *
-                                frame_rate_layer_[layer_] /
-                                num_frames_per_update_[layer_];
+    encoding_bitrate_[layer] = sum_encoded_frame_size_[layer] *
+                               frame_rate_layer_[layer] /
+                               num_frames_per_update_[layer];
     // Total encoding rate: from the start of the update/run to current frame.
     sum_encoded_frame_size_total_ += encoded_size_kbits;
     encoding_bitrate_total_ =
@@ -408,9 +420,9 @@ class VideoProcessorIntegrationTest : public testing::Test {
 
   // Verify expected behavior of rate control and print out data.
   void VerifyRateControlMetrics(int update_index,
+                                int num_dropped_frames,
+                                int num_resize_actions,
                                 const RateControlThresholds& rc_expected) {
-    int num_dropped_frames = processor_->NumberDroppedFrames();
-    int num_resize_actions = processor_->NumberSpatialResizes();
     printf(
         "For update #: %d,\n"
         " Target Bitrate: %d,\n"
@@ -503,11 +515,10 @@ class VideoProcessorIntegrationTest : public testing::Test {
         RTC_NOTREACHED();
         break;
     }
-
     return layer;
   }
 
-  // Set the bitrate and frame rate per layer, for up to 3 layers.
+  // Set the bit rate and frame rate per layer, for up to 3 layers.
   void SetLayerRates() {
     RTC_DCHECK_LE(num_temporal_layers_, kMaxNumTemporalLayers);
     for (int i = 0; i < num_temporal_layers_; i++) {
@@ -551,47 +562,88 @@ class VideoProcessorIntegrationTest : public testing::Test {
     processor_->SetRates(bit_rate_, frame_rate_);
 
     // Process each frame, up to |num_frames|.
-    int num_frames = rate_profile.num_frames;
+    int frame_number = 0;
     int update_index = 0;
+    int num_frames = rate_profile.num_frames;
     ResetRateControlMetrics(
         rate_profile.frame_index_rate_update[update_index + 1]);
-    int frame_number = 0;
-    FrameType frame_type = kVideoFrameDelta;
-    while (processor_->ProcessFrame(frame_number) &&
-           frame_number < num_frames) {
-      // Get the layer index for the frame |frame_number|.
-      layer_ = LayerIndexForFrame(frame_number);
-      // Get the frame_type.
-      frame_type = processor_->EncodedFrameType();
-      // Counter for whole sequence run.
-      ++frame_number;
-      // Counters for each rate update.
-      ++num_frames_per_update_[layer_];
-      ++num_frames_total_;
-      UpdateRateControlMetrics(frame_number, frame_type);
-      // If we hit another/next update, verify stats for current state and
-      // update layers and codec with new rates.
-      if (frame_number ==
-          rate_profile.frame_index_rate_update[update_index + 1]) {
-        VerifyRateControlMetrics(update_index, rc_thresholds[update_index]);
-        // Update layer rates and the codec with new rates.
-        ++update_index;
-        bit_rate_ = rate_profile.target_bit_rate[update_index];
-        frame_rate_ = rate_profile.input_frame_rate[update_index];
-        SetLayerRates();
-        ResetRateControlMetrics(
-            rate_profile.frame_index_rate_update[update_index + 1]);
-        processor_->SetRates(bit_rate_, frame_rate_);
+
+    if (process.batch_mode) {
+      // In batch mode, we calculate the metrics for all frames after all frames
+      // have been sent for encoding.
+
+      for (frame_number = 0; frame_number < num_frames; ++frame_number) {

[åsapersson, 2017/03/08 15:26:46]

use frame_number <= num_frames and remove the ProcessFrame call below the loop?

[sprang_webrtc, 2017/03/08 16:20:21]

Then the expect becomes a little more complicated though, but might make sense?
Might even make sense to indicate which frame failed in that case.

EXPECT_TRUE(frame_number < num_frames,
            processor_->ProcessFrame(frame_number)) << frame_number;

In any case, might be good to comment on why we want to do that last call.

[brandtr, 2017/03/09 15:18:27]

asapersson: Done.

sprang: The last call to ProcessFrame is intended and should actually always
succeed, so I added an EXPECT_TRUE to that call as well.

What's confusing/annoying is that |num_frames| is set as "actual_num_frames" - 1
in the calling code, leading to a bunch of "+1"'s in other places, see e.g.,
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...

This should be cleaned up, so I left a TODO for that.

[sprang_webrtc, 2017/03/10 09:09:37]

Acknowledged.

+        EXPECT_TRUE(processor_->ProcessFrame(frame_number));
+      }
+      processor_->ProcessFrame(frame_number);
+
+      // Release encoder and decoder to make sure they have finished processing.
+      EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Release());
+      EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Release());
+
+      for (frame_number = 0; frame_number < num_frames; ++frame_number) {
+        ++num_frames_per_update_[LayerIndexForFrame(frame_number)];
+        ++num_frames_total_;
+        FrameType frame_type = processor_->EncodedFrameType(frame_number);
+        size_t encoded_frame_size = processor_->EncodedFrameSize(frame_number);
+        UpdateRateControlMetrics(frame_number, frame_type, encoded_frame_size);
       }
+    } else {
+      // In online mode, we calculate the metrics for a given frame right after
+      // it has been sent for encoding.
+
+      if (process.hw_codec) {
+        LOG(LS_WARNING) << "HW codecs should mostly be run in batch mode, "
+                           "since they may be pipelining.";
+      }
+
+      while (frame_number < num_frames) {
+        EXPECT_TRUE(processor_->ProcessFrame(frame_number));
+
+        ++num_frames_per_update_[LayerIndexForFrame(frame_number)];
+        ++num_frames_total_;
+        FrameType frame_type = processor_->EncodedFrameType(frame_number);
+        size_t encoded_frame_size = processor_->EncodedFrameSize(frame_number);

[åsapersson, 2017/03/08 15:26:46]

maybe get frame_type and encoded_frame_size in UpdateRateControlMetrics

[brandtr, 2017/03/09 15:18:27]

Done.

+        UpdateRateControlMetrics(frame_number, frame_type, encoded_frame_size);
+
+        ++frame_number;
+
+        // If we hit another/next update, verify stats for current state and
+        // update layers and codec with new rates.
+        if (frame_number ==
+            rate_profile.frame_index_rate_update[update_index + 1]) {
+          int num_dropped_frames = processor_->NumberDroppedFrames();
+          int num_resize_actions = processor_->NumberSpatialResizes();

[åsapersson, 2017/03/08 15:26:46]

ditto

[brandtr, 2017/03/09 15:18:27]

Done.

[åsapersson, 2017/03/10 08:43:27]

Perhaps same here.

[brandtr, 2017/03/10 08:48:40]

Sorry for missing that.. :/

+          VerifyRateControlMetrics(update_index, num_dropped_frames,
+                                   num_resize_actions,
+                                   rc_thresholds[update_index]);
+
+          // Update layer rates and the codec with new rates.
+          ++update_index;
+          bit_rate_ = rate_profile.target_bit_rate[update_index];
+          frame_rate_ = rate_profile.input_frame_rate[update_index];
+          SetLayerRates();
+          ResetRateControlMetrics(
+              rate_profile.frame_index_rate_update[update_index + 1]);
+          processor_->SetRates(bit_rate_, frame_rate_);
+        }
+      }
+      processor_->ProcessFrame(frame_number);
+
+      // Release encoder and decoder to make sure they have finished processing.
+      EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Release());
+      EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Release());
     }
-    VerifyRateControlMetrics(update_index, rc_thresholds[update_index]);
+
+    // Verify rate control metrics for all frames (if in batch mode), or for all
+    // frames since the last rate update (if not in batch mode).
+    int num_dropped_frames = processor_->NumberDroppedFrames();
+    int num_resize_actions = processor_->NumberSpatialResizes();
+    VerifyRateControlMetrics(update_index, num_dropped_frames,
+                             num_resize_actions, rc_thresholds[update_index]);
     EXPECT_EQ(num_frames, frame_number);
     EXPECT_EQ(num_frames + 1, static_cast<int>(stats_.stats_.size()));
 
-    // Release encoder and decoder to make sure they have finished processing:
-    EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->Release());
-    EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, decoder_->Release());

[åsapersson, 2017/03/08 15:26:46]

maybe make this change if needed in upcoming cl

[brandtr, 2017/03/09 15:18:27]

Done.

-
     // Close the analysis files before we use them for SSIM/PSNR calculations.
     analysis_frame_reader_->Close();
     analysis_frame_writer_->Close();
@@ -601,7 +653,7 @@ class VideoProcessorIntegrationTest : public testing::Test {
       source_frame_writer_->Close();
     }
     if (encoded_frame_writer_) {
-      encoded_frame_writer_->Close();
+      EXPECT_TRUE(encoded_frame_writer_->Close());
     }
     if (decoded_frame_writer_) {
       decoded_frame_writer_->Close();
@@ -640,7 +692,8 @@ class VideoProcessorIntegrationTest : public testing::Test {
                              int width,
                              int height,
                              const std::string& filename,
-                             bool verbose_logging) {
+                             bool verbose_logging,
+                             bool batch_mode) {
     process_settings->codec_type = codec_type;
     process_settings->hw_codec = hw_codec;
     process_settings->use_single_core = use_single_core;
@@ -655,6 +708,7 @@ class VideoProcessorIntegrationTest : public testing::Test {
     process_settings->height = height;
     process_settings->filename = filename;
     process_settings->verbose_logging = verbose_logging;
+    process_settings->batch_mode = batch_mode;
   }
 
   static void SetCodecParams(CodecParams* process_settings,
@@ -672,7 +726,8 @@ class VideoProcessorIntegrationTest : public testing::Test {
                    packet_loss_probability, key_frame_interval,
                    num_temporal_layers, error_concealment_on, denoising_on,
                    frame_dropper_on, spatial_resize_on, kCifWidth, kCifHeight,
-                   kFilenameForemanCif, false /* verbose_logging */);
+                   kFilenameForemanCif, false /* verbose_logging */,
+                   false /* batch_mode */);
   }
 
   static void SetQualityThresholds(QualityThresholds* quality_thresholds,
@@ -756,7 +811,6 @@ class VideoProcessorIntegrationTest : public testing::Test {
   bool encoding_rate_within_target_;
   int bit_rate_;
   int frame_rate_;
-  int layer_;
   float target_size_key_frame_initial_;
   float target_size_key_frame_;
   float sum_key_frame_size_mismatch_;