Fixed bug in test frame generator, causing incorrect reuse of frame

object, in turn causing performance regression.

Plus a small optimization.

BUG=chromium:460954, 4329
R=pbos@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/42499004

Cr-Commit-Position: refs/heads/master@{#8552}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8552 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/test/frame_generator.cc

@@ -69,7 +69,7 @@ class YuvFileGenerator : public FrameGenerator {
         current_display_count_(0) {
     assert(width > 0);
     assert(height > 0);
-    ReadNextFrame();
+    assert(frame_repeat_count > 0);
   }
 
   virtual ~YuvFileGenerator() {
@@ -78,17 +78,18 @@ class YuvFileGenerator : public FrameGenerator {
   }
 
   virtual I420VideoFrame* NextFrame() OVERRIDE {
-    if (frame_display_count_ > 0) {
+    if (current_display_count_ == 0)
-      if (current_display_count_ < frame_display_count_) {
+      ReadNextFrame();
-        ++current_display_count_;
+    if (++current_display_count_ >= frame_display_count_)
-      } else {
+      current_display_count_ = 0;
-        ReadNextFrame();
-        current_display_count_ = 0;
-      }
-    }
 
-    current_frame_.CopyFrame(last_read_frame_);
+    // If this is the last repeatition of this frame, it's OK to use the
-    return &current_frame_;
+    // original instance, otherwise use a copy.
+    if (current_display_count_ == frame_display_count_)
+      return &last_read_frame_;
+
+    temp_frame_copy_.CopyFrame(last_read_frame_);
+    return &temp_frame_copy_;
   }
 
   void ReadNextFrame() {
@@ -121,8 +122,8 @@ class YuvFileGenerator : public FrameGenerator {
   const rtc::scoped_ptr<uint8_t[]> frame_buffer_;
   const int frame_display_count_;
   int current_display_count_;
-  I420VideoFrame current_frame_;
   I420VideoFrame last_read_frame_;
+  I420VideoFrame temp_frame_copy_;
 };
 }  // namespace
 

webrtc/test/frame_generator.h

@@ -33,7 +33,7 @@ class FrameGenerator {
 
   // Creates a frame generator that repeatedly plays a set of yuv files.
   // The frame_repeat_count determines how many times each frame is shown,
-  // with 0 = show the first frame indefinitely, 1 = show each frame once, etc.
+  // with 1 = show each frame once, etc.
   static FrameGenerator* CreateFromYuvFile(std::vector<std::string> files,
                                            size_t width,
                                            size_t height,

webrtc/test/frame_generator_unittest.cc

@@ -0,0 +1,152 @@
+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include <stdio.h>
+#include <string>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/test/frame_generator.h"
+#include "webrtc/test/testsupport/fileutils.h"
+
+namespace webrtc {
+namespace test {
+
+static const int kFrameWidth = 4;
+static const int kFrameHeight = 4;
+
+class FrameGeneratorTest : public ::testing::Test {
+ public:
+  void SetUp() override {
+    two_frame_filename_ =
+        test::TempFilename(test::OutputPath(), "2_frame_yuv_file");
+    one_frame_filename_ =
+        test::TempFilename(test::OutputPath(), "1_frame_yuv_file");
+
+    FILE* file = fopen(two_frame_filename_.c_str(), "wb");
+    WriteYuvFile(file, 0, 0, 0);
+    WriteYuvFile(file, 127, 127, 127);
+    fclose(file);
+    file = fopen(one_frame_filename_.c_str(), "wb");
+    WriteYuvFile(file, 255, 255, 255);
+    fclose(file);
+  }
+  void TearDown() override {
+    remove(one_frame_filename_.c_str());
+    remove(two_frame_filename_.c_str());
+  }
+
+ protected:
+  void WriteYuvFile(FILE* file, uint8_t y, uint8_t u, uint8_t v) {
+    assert(file);
+    rtc::scoped_ptr<uint8_t[]> plane_buffer(new uint8_t[y_size]);
+    memset(plane_buffer.get(), y, y_size);
+    fwrite(plane_buffer.get(), 1, y_size, file);
+    memset(plane_buffer.get(), u, uv_size);
+    fwrite(plane_buffer.get(), 1, uv_size, file);
+    memset(plane_buffer.get(), v, uv_size);
+    fwrite(plane_buffer.get(), 1, uv_size, file);
+  }
+
+  void CheckFrameAndMutate(I420VideoFrame* frame,
+                           uint8_t y,
+                           uint8_t u,
+                           uint8_t v) {
+    // Check that frame is valid, has the correct color and timestamp are clean.
+    ASSERT_NE(nullptr, frame);
+    uint8_t* buffer;
+    ASSERT_EQ(y_size, frame->allocated_size(PlaneType::kYPlane));
+    buffer = frame->buffer(PlaneType::kYPlane);
+    for (int i = 0; i < y_size; ++i)
+      ASSERT_EQ(y, buffer[i]);
+    ASSERT_EQ(uv_size, frame->allocated_size(PlaneType::kUPlane));
+    buffer = frame->buffer(PlaneType::kUPlane);
+    for (int i = 0; i < uv_size; ++i)
+      ASSERT_EQ(u, buffer[i]);
+    ASSERT_EQ(uv_size, frame->allocated_size(PlaneType::kVPlane));
+    buffer = frame->buffer(PlaneType::kVPlane);
+    for (int i = 0; i < uv_size; ++i)
+      ASSERT_EQ(v, buffer[i]);
+    EXPECT_EQ(0, frame->ntp_time_ms());
+    EXPECT_EQ(0, frame->render_time_ms());
+    EXPECT_EQ(0u, frame->timestamp());
+
+    // Mutate to something arbitrary non-zero.
+    frame->set_ntp_time_ms(11);
+    frame->set_render_time_ms(12);
+    frame->set_timestamp(13);
+  }
+
+  std::string two_frame_filename_;
+  std::string one_frame_filename_;
+  const int y_size = kFrameWidth * kFrameHeight;
+  const int uv_size = ((kFrameHeight + 1) / 2) * ((kFrameWidth + 1) / 2);
+};
+
+TEST_F(FrameGeneratorTest, SingleFrameFile) {
+  rtc::scoped_ptr<FrameGenerator> generator(FrameGenerator::CreateFromYuvFile(
+      std::vector<std::string>(1, one_frame_filename_), kFrameWidth,
+      kFrameHeight, 1));
+  CheckFrameAndMutate(generator->NextFrame(), 255, 255, 255);
+  CheckFrameAndMutate(generator->NextFrame(), 255, 255, 255);
+}
+
+TEST_F(FrameGeneratorTest, TwoFrameFile) {
+  rtc::scoped_ptr<FrameGenerator> generator(FrameGenerator::CreateFromYuvFile(
+      std::vector<std::string>(1, two_frame_filename_), kFrameWidth,
+      kFrameHeight, 1));
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+  CheckFrameAndMutate(generator->NextFrame(), 127, 127, 127);
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+}
+
+TEST_F(FrameGeneratorTest, MultipleFrameFiles) {
+  std::vector<std::string> files;
+  files.push_back(two_frame_filename_);
+  files.push_back(one_frame_filename_);
+
+  rtc::scoped_ptr<FrameGenerator> generator(
+      FrameGenerator::CreateFromYuvFile(files, kFrameWidth, kFrameHeight, 1));
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+  CheckFrameAndMutate(generator->NextFrame(), 127, 127, 127);
+  CheckFrameAndMutate(generator->NextFrame(), 255, 255, 255);
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+}
+
+TEST_F(FrameGeneratorTest, TwoFrameFileWithRepeat) {
+  const int kRepeatCount = 3;
+  rtc::scoped_ptr<FrameGenerator> generator(FrameGenerator::CreateFromYuvFile(
+      std::vector<std::string>(1, two_frame_filename_), kFrameWidth,
+      kFrameHeight, kRepeatCount));
+  for (int i = 0; i < kRepeatCount; ++i)
+    CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+  for (int i = 0; i < kRepeatCount; ++i)
+    CheckFrameAndMutate(generator->NextFrame(), 127, 127, 127);
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+}
+
+TEST_F(FrameGeneratorTest, MultipleFrameFilesWithRepeat) {
+  const int kRepeatCount = 3;
+  std::vector<std::string> files;
+  files.push_back(two_frame_filename_);
+  files.push_back(one_frame_filename_);
+  rtc::scoped_ptr<FrameGenerator> generator(FrameGenerator::CreateFromYuvFile(
+      files, kFrameWidth, kFrameHeight, kRepeatCount));
+  for (int i = 0; i < kRepeatCount; ++i)
+    CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+  for (int i = 0; i < kRepeatCount; ++i)
+    CheckFrameAndMutate(generator->NextFrame(), 127, 127, 127);
+  for (int i = 0; i < kRepeatCount; ++i)
+    CheckFrameAndMutate(generator->NextFrame(), 255, 255, 255);
+  CheckFrameAndMutate(generator->NextFrame(), 0, 0, 0);
+}
+
+}  // namespace test
+}  // namespace webrtc

webrtc/test/webrtc_test_common.gyp

@@ -157,6 +157,7 @@
           ],
           'sources': [
             'fake_network_pipe_unittest.cc',
+            'frame_generator_unittest.cc',
             'rtp_file_reader_unittest.cc',
             'rtp_file_writer_unittest.cc',
           ],