Make VEA test support videos with different coded size and visible size

content/common/gpu/media/video_encode_accelerator_unittest.cc

Index: content/common/gpu/media/video_encode_accelerator_unittest.cc
diff --git a/content/common/gpu/media/video_encode_accelerator_unittest.cc b/content/common/gpu/media/video_encode_accelerator_unittest.cc
index 9d1e3b69dc7c9edccf56b2fab6021bc0fb20f23b..c2af7684402b21d871de677c305355cafdb61c32 100644
--- a/content/common/gpu/media/video_encode_accelerator_unittest.cc
+++ b/content/common/gpu/media/video_encode_accelerator_unittest.cc
@@ -33,6 +33,8 @@
 #error The VideoEncodeAcceleratorUnittest is not supported on this platform.
 #endif
 
+#define ALIGN_64_BYTES(x) (((x) + 63) & ~63)

[Pawel Osciak, 2014/09/05 07:21:32]

Inline static function please.

[henryhsu, 2014/09/10 11:01:49]

Done.

+
 using media::VideoEncodeAccelerator;
 
 namespace content {
@@ -103,33 +105,156 @@ struct TestStream {
   ~TestStream() {}
 
   gfx::Size size;
+
+  // Input file name and the file must be an I420 (YUV planar) raw stream.
+  std::string in_filename;

[Pawel Osciak, 2014/09/05 07:21:32]

Please say that his is the original unaligned file provided as an argument to
the test.

[henryhsu, 2014/09/10 11:01:49]

Done.

+
+  // The memory mapped of |temp_file|
   base::MemoryMappedFile input_file;

[Pawel Osciak, 2014/09/05 07:21:34]

Please rename this and temp_file to (mapped_)aligned_input_file, or something
similar to make code easier understandable.

[henryhsu, 2014/09/10 11:01:49]

Done.

-  media::VideoCodecProfile requested_profile;
+
+  // A temporary file used to prepare input buffers.

[Pawel Osciak, 2014/09/05 07:21:32]

Please say what it will contain and why.

[henryhsu, 2014/09/10 11:01:49]

Done.

+  base::FilePath temp_file;
+
   std::string out_filename;
+  media::VideoCodecProfile requested_profile;
   unsigned int requested_bitrate;
   unsigned int requested_framerate;
   unsigned int requested_subsequent_bitrate;
   unsigned int requested_subsequent_framerate;
 };
+TestStream *g_test_streams;

[Pawel Osciak, 2014/09/05 07:21:32]

Could this be a property of the Environment impl?

[henryhsu, 2014/09/10 11:01:48]

Done.

+size_t g_test_streams_size;
+
+static bool WriteFile(base::File *file,

[Pawel Osciak, 2014/09/05 07:21:33]

Documentation please. Also star next to type name.

[henryhsu, 2014/09/10 11:01:50]

Done.

+                      const off_t offset,
+                      const char* data,

[Pawel Osciak, 2014/09/05 07:21:34]

Perhaps take uint8* and cast here in the method, not in the caller.

[henryhsu, 2014/09/10 11:01:49]

Done.

+                      size_t size) {
+  size_t written_bytes = 0;
+  while (written_bytes < size) {
+    int bytes = file->Write(
+        offset + written_bytes, data + written_bytes, size - written_bytes);
+    if (bytes <= 0) return false;
+    written_bytes += bytes;
+  }
+  return true;
+}
+
+// ARM performs CPU cache management with CPU cache line granularity. We thus
+// need to ensure our buffers are CPU cache line-aligned (64 byte-aligned).
+// Otherwise newer kernels will refuse to accept them, and on older kernels
+// we'll be treating ourselves to random corruption.
+// Since we are just mmapping and passing chunks of the input file, to ensure
+// alignment, if the starting virtual addresses of YUV planes of the frames
+// in it were not 64 byte-aligned, we'd have to prepare a memory with 64
+// byte-aligned starting address and make sure the addresses of YUV planes of
+// each frame are 64 byte-aligned before sending to the encoder.
+// Now we test resolutions different from coded size and prepare chunks before

[Pawel Osciak, 2014/09/05 07:21:33]

"Now we test..." is not needed, we don't need to document that we changed a
previous behavior, just say what's happening now please.
Also, you are using a conditional "we'd have to prepare a memory", but we are
doing this already. You should say instead:

"Since we are just mmapping and passing chunks of the input file directly to the
VEA as input frames to avoid copying large chunks of raw data on each frame and
thus affecting performance measurements, we have to prepare a temporary file
with all planes aligned to 64-byte boundaries beforehand."

[henryhsu, 2014/09/10 11:01:48]

Done.

+// encoding to avoid performance impact.
+// YUV data are copied to file directly. Use |visible_size| and |coded_size| to
+// write YUV data from |in_filename| to |input_file|. Also calculate the byte
+// size of an input frame and set it to |coded_buffer_size|. |temp_file| is used
+// to prepare input buffers and will be deleted after test finished.
+static void PrepareInputBuffers(const gfx::Size& visible_size,

[Pawel Osciak, 2014/09/05 07:21:33]

The name is misleading.
Please call it CreateAlignedInputStreamFile.

[henryhsu, 2014/09/10 11:01:49]

Done.

+                                const gfx::Size& coded_size,
+                                const std::string in_filename,
+                                base::MemoryMappedFile* input_file,
+                                base::FilePath* temp_file,
+                                size_t* coded_buffer_size) {

[Pawel Osciak, 2014/09/05 07:21:32]

Can we make coded_buffer_size a property of TestStream perhaps?

[henryhsu, 2014/09/10 11:01:48]

Done.

+  size_t input_num_planes = media::VideoFrame::NumPlanes(kInputFormat);

[Pawel Osciak, 2014/09/05 07:21:32]

s/input_num_planes/num_planes/

[henryhsu, 2014/09/10 11:01:50]

Done.

+  std::vector<size_t> padding_sizes(input_num_planes);

[Pawel Osciak, 2014/09/05 07:21:34]

These require explanation.

[henryhsu, 2014/09/10 11:01:48]

I think the below comments are enough to explain these variables.

+  std::vector<size_t> coded_bpl(input_num_planes);
+  std::vector<size_t> visible_bpl(input_num_planes);
+  std::vector<size_t> visible_plane_rows(input_num_planes);
+
+  // YUV plane starting address should be 64 bytes alignment. Calculate padding

[Pawel Osciak, 2014/09/05 07:21:33]

s/be 64 bytes alignment/aligned to 64 byte boundary/

[henryhsu, 2014/09/10 11:01:49]

Done.

+  // size for each plane, and frame allocation size for coded size. Also store
+  // bytes per line information of coded size and visible size.

[Pawel Osciak, 2014/09/05 07:21:33]

Please in general don't comment describing what code does, but rather give a
high-level picture and explain why.
Example:

- less useful: "Calculate coded size, frame allocation size and store bytes per
line".

- more useful is to explain what those things are for and what we are doing:
"Calculate padding in bytes to be added after each plane required to keep
starting addresses of all planes at a 64 byte boundary. This padding will be
added after each plane when copying to the temporary file.

At the same time we also need to take into account coded_size requested by the
VEA; each row of visible_bpl bytes in the original file needs to be copied into
a row of coded_bpl bytes in the aligned file."

[henryhsu, 2014/09/10 11:01:49]

Done.

+  *coded_buffer_size = 0;

[Pawel Osciak, 2014/09/05 07:21:33]

aligned_buffer_size

[henryhsu, 2014/09/10 11:01:49]

Done.

+  for (off_t i = 0; i < input_num_planes; i++) {
+    size_t size =
+        media::VideoFrame::PlaneAllocationSize(kInputFormat, i, coded_size);
+    size_t padding_bytes = ALIGN_64_BYTES(size) - size;
+    *coded_buffer_size += ALIGN_64_BYTES(size);
+
+    coded_bpl[i] =
+        media::VideoFrame::RowBytes(i, kInputFormat, coded_size.width());
+    visible_bpl[i] =
+        media::VideoFrame::RowBytes(i, kInputFormat, visible_size.width());
+    visible_plane_rows[i] =
+        media::VideoFrame::Rows(i, kInputFormat, visible_size.height());
+    size_t padding_rows =
+        media::VideoFrame::Rows(i, kInputFormat, coded_size.height()) -
+        visible_plane_rows[i];
+    padding_sizes[i] = padding_rows * coded_bpl[i] + padding_bytes;

[Pawel Osciak, 2014/09/05 07:21:33]

Your destination buffer is coded_bpl[i] * coded_size.height() bytes.

So I think you should instead calculate

coded_buffer_size += ALIGN(coded_bpl[i] * coded_size.height())

size and padding_bytes should not be necessary I think?

[henryhsu, 2014/09/10 11:01:49]

coded_bpl[i] * coded_size.height() is exactly PlaneAllocationSize. But it
doesn't guarantee the size is 64 byte alignment. We still need padding_bytes to
set offset to correct value after copied data of rows of visible plane.

[Pawel Osciak, 2014/09/11 09:51:46]

That's why I suggested ALIGN(coded_bpl[i] * coded_size.height()) above, not just
coded_bpl[i] * coded_size.height()...

+  }
+
+  // Test case may have many encoders and memory should be prepared once.
+  if (input_file->IsValid())

[Pawel Osciak, 2014/09/05 07:21:33]

This should go at the beginning of the method.

[henryhsu, 2014/09/10 11:01:49]

Because we should set coded_buffer_size for each encoder.
If we put coded_buffer_size into TestStream class, we can move this line to
beginning of the method.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

+    return;
+
+  base::MemoryMappedFile src_file;
+  CHECK(base::CreateTemporaryFile(temp_file));
+  CHECK(src_file.Initialize(base::FilePath(in_filename)));

[Pawel Osciak, 2014/09/05 07:21:33]

This should go one line up, or l195 should go one line down.

[henryhsu, 2014/09/10 11:01:49]

Done.

+
+  size_t visible_buffer_size =
+      media::VideoFrame::AllocationSize(kInputFormat, visible_size);
+  size_t num_frames = src_file.length() / visible_buffer_size;

[Pawel Osciak, 2014/09/05 07:21:34]

The check that was here before should still be here, i.e.

CHECK_EQ(test_stream_.input_file.length() % input_buffer_size_, 0U)
      << "Stream byte size is not a product of calculated frame byte size";

[henryhsu, 2014/09/10 11:01:49]

Done.

+  uint32 flags = base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE |
+                 base::File::FLAG_READ;
+
+  // Create a temporary file with coded_size length.
+  base::File dest_file(*temp_file, flags);
+  dest_file.SetLength(*coded_buffer_size * num_frames);
+
+  off_t src_offset = 0, dest_offset = 0;
+  while (src_offset < static_cast<off_t>(src_file.length())) {

[Pawel Osciak, 2014/09/05 07:21:32]

Iterate over num_frames?

[henryhsu, 2014/09/10 11:01:48]

Done.

+    for (off_t i = 0; i < input_num_planes; i++) {
+#if defined(ARCH_CPU_ARMEL)

[Pawel Osciak, 2014/09/05 07:21:32]

We are now doing alignment for all platforms, so no need for ifdef.

[henryhsu, 2014/09/10 11:01:50]

Done.

+      // Assert that each plane of frame starts at 64-byte boundary.
+      ASSERT_EQ(dest_offset & 63, 0)
+          << "Planes of frame should be mapped at a 64 byte boundary";
+#endif
+      for (off_t j = 0; j < visible_plane_rows[i]; j++) {
+        const char* src =

[Pawel Osciak, 2014/09/05 07:21:33]

Perhaps keep a rolling pointer to the data, instead of calculating data + offset
every time?

[henryhsu, 2014/09/10 11:01:49]

Done.

+            reinterpret_cast<const char*>(src_file.data() + src_offset);
+        CHECK(WriteFile(&dest_file, dest_offset, src, visible_bpl[i]));
+        src_offset += visible_bpl[i];
+        dest_offset += coded_bpl[i];
+      }
+      dest_offset += padding_sizes[i];
+    }
+  }
+  CHECK(input_file->Initialize(dest_file.Pass()));
+#if defined(ARCH_CPU_ARMEL)

[Pawel Osciak, 2014/09/05 07:21:33]

Please remove ifdef.

[henryhsu, 2014/09/10 11:01:49]

Done.

+  // Assert that memory mapped of file starts at 64-byte boundary. So each
+  // plane of frames also start at 64-byte boundary.
+  ASSERT_EQ(reinterpret_cast<off_t>(input_file->data()) & 63, 0)
+      << "File should be mapped at a 64 byte boundary";
+#endif
+}
 
 // Parse |data| into its constituent parts, set the various output fields
-// accordingly, read in video stream, and store them to |test_streams|.
+// accordingly, read in video stream, and store them to |test_streams|. Also
+// store the size of |test_streams| to |test_streams_size|.
 static void ParseAndReadTestStreamData(const base::FilePath::StringType& data,
-                                       ScopedVector<TestStream>* test_streams) {
+                                       TestStream** test_streams,
+                                       size_t* test_streams_size) {
   // Split the string to individual test stream data.
   std::vector<base::FilePath::StringType> test_streams_data;
   base::SplitString(data, ';', &test_streams_data);
   CHECK_GE(test_streams_data.size(), 1U) << data;
 
+  *test_streams_size = test_streams_data.size();
+  *test_streams = new TestStream[*test_streams_size];

[Pawel Osciak, 2014/09/05 07:21:33]

std::vector of TestStream instead? 

[henryhsu, 2014/09/10 11:01:50]

Static or global variables of class type are forbidden.
Maybe let test_stream be a property of the Environment impl.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

   // Parse each test stream data and read the input file.
-  for (size_t index = 0; index < test_streams_data.size(); ++index) {
+  for (size_t index = 0; index < *test_streams_size; ++index) {
     std::vector<base::FilePath::StringType> fields;
     base::SplitString(test_streams_data[index], ':', &fields);
     CHECK_GE(fields.size(), 4U) << data;
     CHECK_LE(fields.size(), 9U) << data;
-    TestStream* test_stream = new TestStream();
+    TestStream* test_stream = &*test_streams[index];
 
-    base::FilePath::StringType filename = fields[0];
+    test_stream->in_filename = fields[0];
     int width, height;
     CHECK(base::StringToInt(fields[1], &width));
     CHECK(base::StringToInt(fields[2], &height));
@@ -152,61 +277,14 @@ static void ParseAndReadTestStreamData(const base::FilePath::StringType& data,
       CHECK(base::StringToUint(fields[6], &test_stream->requested_framerate));
 
     if (fields.size() >= 8 && !fields[7].empty()) {
-      CHECK(base::StringToUint(fields[7],
-                               &test_stream->requested_subsequent_bitrate));
+      CHECK(base::StringToUint(
+          fields[7], &test_stream->requested_subsequent_bitrate));
     }
 
     if (fields.size() >= 9 && !fields[8].empty()) {
-      CHECK(base::StringToUint(fields[8],
-                               &test_stream->requested_subsequent_framerate));
-    }
-
-    CHECK(test_stream->input_file.Initialize(base::FilePath(filename)));
-    test_streams->push_back(test_stream);
-  }
-}
-
-// Set default parameters of |test_streams| and update the parameters according
-// to |mid_stream_bitrate_switch| and |mid_stream_framerate_switch|.
-static void UpdateTestStreamData(bool mid_stream_bitrate_switch,
-                                 bool mid_stream_framerate_switch,
-                                 ScopedVector<TestStream>* test_streams) {
-  for (size_t i = 0; i < test_streams->size(); i++) {
-    TestStream* test_stream = (*test_streams)[i];
-    // Use defaults for bitrate/framerate if they are not provided.
-    if (test_stream->requested_bitrate == 0)
-      test_stream->requested_bitrate = kDefaultBitrate;
-
-    if (test_stream->requested_framerate == 0)
-      test_stream->requested_framerate = kDefaultFramerate;
-
-    // If bitrate/framerate switch is requested, use the subsequent values if
-    // provided, or, if not, calculate them from their initial values using
-    // the default ratios.
-    // Otherwise, if a switch is not requested, keep the initial values.
-    if (mid_stream_bitrate_switch) {
-      if (test_stream->requested_subsequent_bitrate == 0) {
-        test_stream->requested_subsequent_bitrate =
-            test_stream->requested_bitrate * kDefaultSubsequentBitrateRatio;
-      }
-    } else {
-      test_stream->requested_subsequent_bitrate =
-          test_stream->requested_bitrate;
-    }
-    if (test_stream->requested_subsequent_bitrate == 0)
-      test_stream->requested_subsequent_bitrate = 1;
-
-    if (mid_stream_framerate_switch) {
-      if (test_stream->requested_subsequent_framerate == 0) {
-        test_stream->requested_subsequent_framerate =
-            test_stream->requested_framerate * kDefaultSubsequentFramerateRatio;
-      }
-    } else {
-      test_stream->requested_subsequent_framerate =
-          test_stream->requested_framerate;
+      CHECK(base::StringToUint(
+          fields[8], &test_stream->requested_subsequent_framerate));
     }
-    if (test_stream->requested_subsequent_framerate == 0)
-      test_stream->requested_subsequent_framerate = 1;
   }
 }
 
@@ -368,12 +446,14 @@ scoped_ptr<StreamValidator> StreamValidator::Create(
 
 class VEAClient : public VideoEncodeAccelerator::Client {
  public:
-  VEAClient(const TestStream& test_stream,
+  VEAClient(TestStream& test_stream,
             ClientStateNotification<ClientState>* note,
             bool save_to_file,
             unsigned int keyframe_period,
             bool force_bitrate,
-            bool test_perf);
+            bool test_perf,
+            bool mid_stream_bitrate_switch,
+            bool mid_stream_framerate_switch);
   virtual ~VEAClient();
   void CreateEncoder();
   void DestroyEncoder();
@@ -427,10 +507,15 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   // the input stream, ready to be sent to encoder.
   scoped_refptr<media::VideoFrame> PrepareInputFrame(off_t position);
 
+  // Update the parameters according to |mid_stream_bitrate_switch| and
+  // |mid_stream_framerate_switch|.
+  void UpdateTestStreamData(bool mid_stream_bitrate_switch,
+                            bool mid_stream_framerate_switch);
+
   ClientState state_;
   scoped_ptr<VideoEncodeAccelerator> encoder_;
 
-  const TestStream& test_stream_;
+  TestStream& test_stream_;

[Pawel Osciak, 2014/09/05 07:21:33]

I think you can keep const here if you don't UpdateTestStreamData from the
client, as suggested below.

[henryhsu, 2014/09/10 11:01:49]

We put input_buffer_size_ as a property of TestStream and we have to update it.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

   // Used to notify another thread about the state. VEAClient does not own this.
   ClientStateNotification<ClientState>* note_;
 
@@ -505,14 +590,28 @@ class VEAClient : public VideoEncodeAccelerator::Client {
 
   // All methods of this class should be run on the same thread.
   base::ThreadChecker thread_checker_;
+
+  // Requested bitrate in bits per second.
+  unsigned int requested_bitrate_;
+
+  // Requested initial framerate.
+  unsigned int requested_framerate_;
+
+  // Bitrate to switch to in the middle of the stream.
+  unsigned int requested_subsequent_bitrate_;
+
+  // Framerate to switch to in the middle of the stream.
+  unsigned int requested_subsequent_framerate_;
 };
 
-VEAClient::VEAClient(const TestStream& test_stream,
+VEAClient::VEAClient(TestStream& test_stream,
                      ClientStateNotification<ClientState>* note,
                      bool save_to_file,
                      unsigned int keyframe_period,
                      bool force_bitrate,
-                     bool test_perf)
+                     bool test_perf,
+                     bool mid_stream_bitrate_switch,
+                     bool mid_stream_framerate_switch)
     : state_(CS_CREATED),
       test_stream_(test_stream),
       note_(note),
@@ -534,7 +633,11 @@ VEAClient::VEAClient(const TestStream& test_stream,
       current_requested_bitrate_(0),
       current_framerate_(0),
       encoded_stream_size_since_last_check_(0),
-      test_perf_(test_perf) {
+      test_perf_(test_perf),
+      requested_bitrate_(0),
+      requested_framerate_(0),
+      requested_subsequent_bitrate_(0),
+      requested_subsequent_framerate_(0) {
   if (keyframe_period_)
     CHECK_LT(kMaxKeyframeDelay, keyframe_period_);
 
@@ -552,28 +655,8 @@ VEAClient::VEAClient(const TestStream& test_stream,
     EXPECT_EQ(0, base::WriteFile(out_filename, NULL, 0));
   }
 
-  input_buffer_size_ =
-      media::VideoFrame::AllocationSize(kInputFormat, test_stream.size);
-  CHECK_GT(input_buffer_size_, 0UL);
-
-  // Calculate the number of frames in the input stream by dividing its length
-  // in bytes by frame size in bytes.
-  CHECK_EQ(test_stream_.input_file.length() % input_buffer_size_, 0U)
-      << "Stream byte size is not a product of calculated frame byte size";
-  num_frames_in_stream_ = test_stream_.input_file.length() / input_buffer_size_;
-  CHECK_GT(num_frames_in_stream_, 0UL);
-  CHECK_LE(num_frames_in_stream_, kMaxFrameNum);
-
-  // We may need to loop over the stream more than once if more frames than
-  // provided is required for bitrate tests.
-  if (force_bitrate_ && num_frames_in_stream_ < kMinFramesForBitrateTests) {
-    DVLOG(1) << "Stream too short for bitrate test (" << num_frames_in_stream_
-             << " frames), will loop it to reach " << kMinFramesForBitrateTests
-             << " frames";
-    num_frames_to_encode_ = kMinFramesForBitrateTests;
-  } else {
-    num_frames_to_encode_ = num_frames_in_stream_;
-  }
+  // Initialize the parameters of the test streams.
+  UpdateTestStreamData(mid_stream_bitrate_switch, mid_stream_framerate_switch);
 
   thread_checker_.DetachFromThread();
 }
@@ -594,19 +677,18 @@ void VEAClient::CreateEncoder() {
   SetState(CS_ENCODER_SET);
 
   DVLOG(1) << "Profile: " << test_stream_.requested_profile
-           << ", initial bitrate: " << test_stream_.requested_bitrate;
+           << ", initial bitrate: " << requested_bitrate_;
   if (!encoder_->Initialize(kInputFormat,
                             test_stream_.size,
                             test_stream_.requested_profile,
-                            test_stream_.requested_bitrate,
+                            requested_bitrate_,
                             this)) {
     DLOG(ERROR) << "VideoEncodeAccelerator::Initialize() failed";
     SetState(CS_ERROR);
     return;
   }
 
-  SetStreamParameters(test_stream_.requested_bitrate,
-                      test_stream_.requested_framerate);
+  SetStreamParameters(requested_bitrate_, requested_framerate_);
   SetState(CS_INITIALIZED);
 }
 
@@ -617,6 +699,49 @@ void VEAClient::DestroyEncoder() {
   encoder_.reset();
 }
 
+void VEAClient::UpdateTestStreamData(bool mid_stream_bitrate_switch,
+                                     bool mid_stream_framerate_switch) {
+  // Use defaults for bitrate/framerate if they are not provided.
+  if (test_stream_.requested_bitrate == 0)
+    requested_bitrate_ = kDefaultBitrate;
+  else
+    requested_bitrate_ = test_stream_.requested_bitrate;
+
+  if (test_stream_.requested_framerate == 0)
+    requested_framerate_ = kDefaultFramerate;
+  else
+    requested_framerate_ = test_stream_.requested_framerate;
+
+  // If bitrate/framerate switch is requested, use the subsequent values if
+  // provided, or, if not, calculate them from their initial values using
+  // the default ratios.
+  // Otherwise, if a switch is not requested, keep the initial values.
+  if (mid_stream_bitrate_switch) {
+    if (test_stream_.requested_subsequent_bitrate == 0)
+      requested_subsequent_bitrate_ =
+          requested_bitrate_ * kDefaultSubsequentBitrateRatio;
+    else
+      requested_subsequent_bitrate_ = test_stream_.requested_subsequent_bitrate;
+  } else {
+    requested_subsequent_bitrate_ = requested_bitrate_;
+  }
+  if (requested_subsequent_bitrate_ == 0)
+    requested_subsequent_bitrate_ = 1;
+
+  if (mid_stream_framerate_switch) {
+    if (test_stream_.requested_subsequent_framerate == 0)
+      requested_subsequent_framerate_ =
+          requested_framerate_ * kDefaultSubsequentFramerateRatio;
+    else
+      requested_subsequent_framerate_ =
+          test_stream_.requested_subsequent_framerate;
+  } else {
+    requested_subsequent_framerate_ = requested_framerate_;
+  }
+  if (requested_subsequent_framerate_ == 0)
+    requested_subsequent_framerate_ = 1;
+}
+
 double VEAClient::frames_per_second() {
   base::TimeDelta duration = last_frame_ready_time_ - encode_start_time_;
   return num_encoded_frames_ / duration.InSecondsF();
@@ -629,35 +754,34 @@ void VEAClient::RequireBitstreamBuffers(unsigned int input_count,
   ASSERT_EQ(state_, CS_INITIALIZED);
   SetState(CS_ENCODING);
 
-  // TODO(posciak): For now we only support input streams that meet encoder
-  // size requirements exactly (i.e. coded size == visible size), so that we
-  // can simply mmap the stream file and feed the encoder directly with chunks
-  // of that, instead of memcpying from mmapped file into a separate set of
-  // input buffers that would meet the coded size and alignment requirements.
-  // If/when this is changed, the ARM-specific alignment check below should be
-  // redone as well.
-  input_coded_size_ = input_coded_size;
-  ASSERT_EQ(input_coded_size_, test_stream_.size);
-#if defined(ARCH_CPU_ARMEL)
-  // ARM performs CPU cache management with CPU cache line granularity. We thus
-  // need to ensure our buffers are CPU cache line-aligned (64 byte-aligned).
-  // Otherwise newer kernels will refuse to accept them, and on older kernels
-  // we'll be treating ourselves to random corruption.
-  // Since we are just mmapping and passing chunks of the input file, to ensure
-  // alignment, if the starting virtual addresses of the frames in it were not
-  // 64 byte-aligned, we'd have to use a separate set of input buffers and copy
-  // the frames into them before sending to the encoder. It would have been an
-  // overkill here though, because, for now at least, we only test resolutions
-  // that result in proper alignment, and it would have also interfered with
-  // performance testing. So just assert that the frame size is a multiple of
-  // 64 bytes. This ensures all frames start at 64-byte boundary, because
-  // MemoryMappedFile should be mmapp()ed at virtual page start as well.
-  ASSERT_EQ(input_buffer_size_ & 63, 0u)
-      << "Frame size has to be a multiple of 64 bytes";
-  ASSERT_EQ(reinterpret_cast<off_t>(test_stream_.input_file.data()) & 63, 0)
-      << "Mapped file should be mapped at a 64 byte boundary";
-#endif
+  PrepareInputBuffers(test_stream_.size,

[Pawel Osciak, 2014/09/05 07:21:32]

Please just pass the test stream, instead of its members.

[henryhsu, 2014/09/10 11:01:49]

Done.

+                      input_coded_size,
+                      test_stream_.in_filename,
+                      &test_stream_.input_file,
+                      &test_stream_.temp_file,
+                      &input_buffer_size_);
+  CHECK_GT(input_buffer_size_, 0UL);
 
+  // Calculate the number of frames in the input stream by dividing its length
+  // in bytes by frame size in bytes.
+  CHECK_EQ(test_stream_.input_file.length() % input_buffer_size_, 0U)
+      << "Stream byte size is not a product of calculated frame byte size";
+  num_frames_in_stream_ = test_stream_.input_file.length() / input_buffer_size_;

[Pawel Osciak, 2014/09/05 07:21:32]

This is already calculated in PrepareInputBuffers. Let's make it a property of
the TestStream instead and don't calculate it again here.

[henryhsu, 2014/09/10 11:01:49]

We put input_buffer_size and num_frames into TestStream. So we don't need
num_frames_in_stream_ and input_buffer_size_ in VEAClient any more.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

+  CHECK_GT(num_frames_in_stream_, 0UL);
+  CHECK_LE(num_frames_in_stream_, kMaxFrameNum);
+
+  // We may need to loop over the stream more than once if more frames than
+  // provided is required for bitrate tests.
+  if (force_bitrate_ && num_frames_in_stream_ < kMinFramesForBitrateTests) {
+    DVLOG(1) << "Stream too short for bitrate test (" << num_frames_in_stream_
+             << " frames), will loop it to reach " << kMinFramesForBitrateTests
+             << " frames";
+    num_frames_to_encode_ = kMinFramesForBitrateTests;
+  } else {
+    num_frames_to_encode_ = num_frames_in_stream_;
+  }
+
+  input_coded_size_ = input_coded_size;
   num_required_input_buffers_ = input_count;
   ASSERT_GT(num_required_input_buffers_, 0UL);
 
@@ -743,8 +867,14 @@ void VEAClient::InputNoLongerNeededCallback(int32 input_id) {
 scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
   CHECK_LE(position + input_buffer_size_, test_stream_.input_file.length());
 
-  uint8* frame_data =
+  uint8* frame_data_y =
       const_cast<uint8*>(test_stream_.input_file.data() + position);
+  uint8* frame_data_u =

[Pawel Osciak, 2014/09/05 07:21:32]

It would be better to have these as the property of TestStream, instead of
recalculating it here every frame, and also risking that the algorithm changes
in PrepareInputBuffers changes, but not here.

[henryhsu, 2014/09/10 11:01:49]

Done.

+      frame_data_y + ALIGN_64_BYTES(media::VideoFrame::PlaneAllocationSize(
+                         kInputFormat, 0, input_coded_size_));
+  uint8* frame_data_v =
+      frame_data_u + ALIGN_64_BYTES(media::VideoFrame::PlaneAllocationSize(
+                         kInputFormat, 1, input_coded_size_));
 
   CHECK_GT(current_framerate_, 0U);
   scoped_refptr<media::VideoFrame> frame =
@@ -756,9 +886,9 @@ scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
           input_coded_size_.width(),
           input_coded_size_.width() / 2,
           input_coded_size_.width() / 2,
-          frame_data,
-          frame_data + input_coded_size_.GetArea(),
-          frame_data + (input_coded_size_.GetArea() * 5 / 4),
+          frame_data_y,
+          frame_data_u,
+          frame_data_v,
           base::TimeDelta().FromMilliseconds(
               next_input_id_ * base::Time::kMillisecondsPerSecond /
               current_framerate_),
@@ -854,11 +984,10 @@ bool VEAClient::HandleEncodedFrame(bool keyframe) {
 
   if (num_encoded_frames_ == num_frames_to_encode_ / 2) {
     VerifyStreamProperties();
-    if (test_stream_.requested_subsequent_bitrate !=
-        current_requested_bitrate_ ||
-        test_stream_.requested_subsequent_framerate != current_framerate_) {
-      SetStreamParameters(test_stream_.requested_subsequent_bitrate,
-                          test_stream_.requested_subsequent_framerate);
+    if (requested_subsequent_bitrate_ != current_requested_bitrate_ ||

[Pawel Osciak, 2014/09/05 07:21:32]

Please keep those as a property of TestStream.

[henryhsu, 2014/09/10 11:01:50]

Since TestStream is a property of environment object, I prefer TestStream is
used to store values which can be used for all test cases and VEAClient has
updated test parameters for each test case.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

+        requested_subsequent_framerate_ != current_framerate_) {
+      SetStreamParameters(requested_subsequent_bitrate_,
+                          requested_subsequent_framerate_);
     }
   } else if (num_encoded_frames_ == num_frames_to_encode_) {
     VerifyPerf();
@@ -897,6 +1026,16 @@ void VEAClient::VerifyStreamProperties() {
   }
 }
 
+class VideoEncodeAcceleratorTestEnvironment : public ::testing::Environment {

[Pawel Osciak, 2014/09/05 07:21:33]

Documentation please.

[henryhsu, 2014/09/10 11:01:49]

Done.

+  public:
+    virtual void TearDown() {
+      for (size_t i = 0; i < g_test_streams_size; i++)
+        base::DeleteFile(g_test_streams[i].temp_file, false);
+      delete[] g_test_streams;
+      g_test_streams = NULL;
+    }
+};
+
 // Test parameters:
 // - Number of concurrent encoders.
 // - If true, save output to file (provided an output filename was supplied).
@@ -919,12 +1058,6 @@ TEST_P(VideoEncodeAcceleratorTest, TestSimpleEncode) {
   const bool mid_stream_bitrate_switch = GetParam().f;
   const bool mid_stream_framerate_switch = GetParam().g;
 
-  // Initialize the test streams.
-  ScopedVector<TestStream> test_streams;
-  ParseAndReadTestStreamData(*g_test_stream_data, &test_streams);
-  UpdateTestStreamData(
-      mid_stream_bitrate_switch, mid_stream_framerate_switch, &test_streams);
-
   ScopedVector<ClientStateNotification<ClientState> > notes;
   ScopedVector<VEAClient> clients;
   base::Thread encoder_thread("EncoderThread");
@@ -932,19 +1065,21 @@ TEST_P(VideoEncodeAcceleratorTest, TestSimpleEncode) {
 
   // Create all encoders.
   for (size_t i = 0; i < num_concurrent_encoders; i++) {
-    size_t test_stream_index = i % test_streams.size();
+    size_t test_stream_index = i % g_test_streams_size;
     // Disregard save_to_file if we didn't get an output filename.
     bool encoder_save_to_file =
         (save_to_file &&
-         !test_streams[test_stream_index]->out_filename.empty());
+         !g_test_streams[test_stream_index].out_filename.empty());
 
     notes.push_back(new ClientStateNotification<ClientState>());
-    clients.push_back(new VEAClient(*test_streams[test_stream_index],
+    clients.push_back(new VEAClient(g_test_streams[test_stream_index],
                                     notes.back(),
                                     encoder_save_to_file,
                                     keyframe_period,
                                     force_bitrate,
-                                    test_perf));
+                                    test_perf,
+                                    mid_stream_bitrate_switch,

[Pawel Osciak, 2014/09/05 07:21:34]

Please update the streams from here like before, not through VEAClient.

And I think you should be able to pull g_test_streams from Environment class if
you move it into it?

[henryhsu, 2014/09/10 11:01:50]

requested_subsequent_bitrate and requested_subsequent_framerate are set by test
case parameters. I think putting these in VEAClient is better than TestStream
because TestStream will be used for all test cases.

[Pawel Osciak, 2014/09/11 09:51:46]

Acknowledged.

+                                    mid_stream_framerate_switch));
 
     encoder_thread.message_loop()->PostTask(
         FROM_HERE,
@@ -1028,6 +1163,8 @@ INSTANTIATE_TEST_CASE_P(
 }  // namespace content
 
 int main(int argc, char** argv) {
+  testing::AddGlobalTestEnvironment(
+      new content::VideoEncodeAcceleratorTestEnvironment);
   testing::InitGoogleTest(&argc, argv);  // Removes gtest-specific args.
   base::CommandLine::Init(argc, argv);
 
@@ -1058,6 +1195,8 @@ int main(int argc, char** argv) {
       continue;
     LOG(FATAL) << "Unexpected switch: " << it->first << ":" << it->second;
   }
-
+  content::ParseAndReadTestStreamData(*content::g_test_stream_data,

[Pawel Osciak, 2014/09/05 07:21:33]

There is a Environment::SetUp() call, a counterpart to TearDown I believe.
Perhaps use that?

Also, perhaps we could store the globals in
VideoEncodeAcceleratorTestEnvironment instead?

[henryhsu, 2014/09/10 11:01:49]

Done.

+                                      &content::g_test_streams,
+                                      &content::g_test_streams_size);
   return RUN_ALL_TESTS();
 }