Add a test case to make sure no output before getting any input

media/gpu/video_encode_accelerator_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <inttypes.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 811 matching lines @@
       }
     }
   }
 
   // Divide the difference by the size of frame.
   difference /= VideoFrame::AllocationSize(kInputFormat, visible_size);
   EXPECT_TRUE(difference <= kDecodeSimilarityThreshold)
       << "difference = " << difference << "  > decode similarity threshold";
 }
 
-class VEAClient : public VideoEncodeAccelerator::Client {
+class VEAClientBase : public VideoEncodeAccelerator::Client {
+ public:
+  VEAClientBase();
+  ~VEAClientBase() override;
+
+  // VideoDecodeAccelerator::Client implementation.
+  virtual void RequireBitstreamBuffers(unsigned int input_count,

[Owen Lin, 2016/11/04 03:38:50]

There is no need to list these pure virtual functions.

[henryhsu, 2016/11/04 04:15:17]

Done.

+                                       const gfx::Size& input_coded_size,
+                                       size_t output_buffer_size) = 0;
+  virtual void BitstreamBufferReady(int32_t bitstream_buffer_id,
+                                    size_t payload_size,
+                                    bool key_frame,
+                                    base::TimeDelta timestamp) = 0;
+  virtual void NotifyError(VideoEncodeAccelerator::Error error) = 0;
+
+ protected:
+  bool has_encoder() { return encoder_.get(); }
+
+  std::unique_ptr<VideoEncodeAccelerator> CreateFakeVEA();

[Owen Lin, 2016/11/04 03:38:50]

Why these functions need to be a member of VEAClientBase? Can we make these
function global stastic, or put them in a anonymous namespace so that bose
VEAClient and VEAClientWithNoInput can use it.  

[henryhsu, 2016/11/04 04:15:17]

Hmm...If so, then we don't have VEAClientBase class.

+  std::unique_ptr<VideoEncodeAccelerator> CreateV4L2VEA();
+  std::unique_ptr<VideoEncodeAccelerator> CreateVaapiVEA();
+  std::unique_ptr<VideoEncodeAccelerator> CreateVTVEA();
+  std::unique_ptr<VideoEncodeAccelerator> CreateMFVEA();
+
+  std::unique_ptr<VideoEncodeAccelerator> encoder_;
+};
+
+VEAClientBase::VEAClientBase() {

[Owen Lin, 2016/11/04 03:38:51]

Please run gl cl format to check style issue.

[henryhsu, 2016/11/04 04:15:17]

Done.

+}
+
+VEAClientBase::~VEAClientBase() {
+  LOG_ASSERT(!has_encoder());
+}
+
+std::unique_ptr<VideoEncodeAccelerator> VEAClientBase::CreateFakeVEA() {
+  std::unique_ptr<VideoEncodeAccelerator> encoder;
+  if (g_fake_encoder) {
+    encoder.reset(new FakeVideoEncodeAccelerator(
+        scoped_refptr<base::SingleThreadTaskRunner>(
+            base::ThreadTaskRunnerHandle::Get())));
+  }
+  return encoder;
+}
+
+std::unique_ptr<VideoEncodeAccelerator> VEAClientBase::CreateV4L2VEA() {
+  std::unique_ptr<VideoEncodeAccelerator> encoder;
+#if defined(OS_CHROMEOS) && defined(USE_V4L2_CODEC)
+  scoped_refptr<V4L2Device> device = V4L2Device::Create();
+  if (device)
+    encoder.reset(new V4L2VideoEncodeAccelerator(device));
+#endif
+  return encoder;
+}
+
+std::unique_ptr<VideoEncodeAccelerator> VEAClientBase::CreateVaapiVEA() {
+  std::unique_ptr<VideoEncodeAccelerator> encoder;
+#if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
+  encoder.reset(new VaapiVideoEncodeAccelerator());
+#endif
+  return encoder;
+}
+
+std::unique_ptr<VideoEncodeAccelerator> VEAClientBase::CreateVTVEA() {
+  std::unique_ptr<VideoEncodeAccelerator> encoder;
+#if defined(OS_MACOSX)
+  encoder.reset(new VTVideoEncodeAccelerator());
+#endif
+  return encoder;
+}
+
+std::unique_ptr<VideoEncodeAccelerator> VEAClientBase::CreateMFVEA() {
+  std::unique_ptr<VideoEncodeAccelerator> encoder;
+#if defined(OS_WIN)
+  MediaFoundationVideoEncodeAccelerator::PreSandboxInitialization();
+  encoder.reset(new MediaFoundationVideoEncodeAccelerator());
+#endif
+  return encoder;
+}
+
+class VEAClient : public VEAClientBase {
  public:
   VEAClient(TestStream* test_stream,
             ClientStateNotification<ClientState>* note,
             bool save_to_file,
             unsigned int keyframe_period,
             bool force_bitrate,
             bool test_perf,
             bool mid_stream_bitrate_switch,
             bool mid_stream_framerate_switch,
             bool verify_output,
             bool verify_output_timestamp);
   ~VEAClient() override;
   void CreateEncoder();
   void DestroyEncoder();
 
   // VideoDecodeAccelerator::Client implementation.
   void RequireBitstreamBuffers(unsigned int input_count,
                                const gfx::Size& input_coded_size,
                                size_t output_buffer_size) override;
   void BitstreamBufferReady(int32_t bitstream_buffer_id,
                             size_t payload_size,
                             bool key_frame,
                             base::TimeDelta timestamp) override;
   void NotifyError(VideoEncodeAccelerator::Error error) override;
 
  private:
-  bool has_encoder() { return encoder_.get(); }
-
   // Return the number of encoded frames per second.
   double frames_per_second();
 
-  std::unique_ptr<VideoEncodeAccelerator> CreateFakeVEA();
-  std::unique_ptr<VideoEncodeAccelerator> CreateV4L2VEA();
-  std::unique_ptr<VideoEncodeAccelerator> CreateVaapiVEA();
-  std::unique_ptr<VideoEncodeAccelerator> CreateVTVEA();
-  std::unique_ptr<VideoEncodeAccelerator> CreateMFVEA();
-
   void SetState(ClientState new_state);
 
   // Set current stream parameters to given |bitrate| at |framerate|.
   void SetStreamParameters(unsigned int bitrate, unsigned int framerate);
 
   // Called when encoder is done with a VideoFrame.
   void InputNoLongerNeededCallback(int32_t input_id);
 
   // Feed the encoder with one input frame.
   void FeedEncoderWithOneInput();
@@ skipping 45 matching lines @@
   // Called when the quality validator has decoded all the frames.
   void DecodeCompleted();
 
   // Called when the quality validator fails to decode a frame.
   void DecodeFailed();
 
   // Verify that the output timestamp matches input timestamp.
   void VerifyOutputTimestamp(base::TimeDelta timestamp);
 
   ClientState state_;
-  std::unique_ptr<VideoEncodeAccelerator> encoder_;
 
   TestStream* test_stream_;
 
   // Used to notify another thread about the state. VEAClient does not own this.
   ClientStateNotification<ClientState>* note_;
 
   // Ids assigned to VideoFrames.
   std::set<int32_t> inputs_at_client_;
   int32_t next_input_id_;
 
@@ skipping 162 matching lines @@
     EXPECT_EQ(0, base::WriteFile(out_filename, NULL, 0));
   }
 
   // Initialize the parameters of the test streams.
   UpdateTestStreamData(mid_stream_bitrate_switch, mid_stream_framerate_switch);
 
   thread_checker_.DetachFromThread();
 }
 
 VEAClient::~VEAClient() {
-  LOG_ASSERT(!has_encoder());
-}
-
-std::unique_ptr<VideoEncodeAccelerator> VEAClient::CreateFakeVEA() {
-  std::unique_ptr<VideoEncodeAccelerator> encoder;
-  if (g_fake_encoder) {
-    encoder.reset(new FakeVideoEncodeAccelerator(
-        scoped_refptr<base::SingleThreadTaskRunner>(
-            base::ThreadTaskRunnerHandle::Get())));
-  }
-  return encoder;
-}
-
-std::unique_ptr<VideoEncodeAccelerator> VEAClient::CreateV4L2VEA() {
-  std::unique_ptr<VideoEncodeAccelerator> encoder;
-#if defined(OS_CHROMEOS) && defined(USE_V4L2_CODEC)
-  scoped_refptr<V4L2Device> device = V4L2Device::Create();
-  if (device)
-    encoder.reset(new V4L2VideoEncodeAccelerator(device));
-#endif
-  return encoder;
-}
-
-std::unique_ptr<VideoEncodeAccelerator> VEAClient::CreateVaapiVEA() {
-  std::unique_ptr<VideoEncodeAccelerator> encoder;
-#if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
-  encoder.reset(new VaapiVideoEncodeAccelerator());
-#endif
-  return encoder;
-}
-
-std::unique_ptr<VideoEncodeAccelerator> VEAClient::CreateVTVEA() {
-  std::unique_ptr<VideoEncodeAccelerator> encoder;
-#if defined(OS_MACOSX)
-  encoder.reset(new VTVideoEncodeAccelerator());
-#endif
-  return encoder;
-}
-
-std::unique_ptr<VideoEncodeAccelerator> VEAClient::CreateMFVEA() {
-  std::unique_ptr<VideoEncodeAccelerator> encoder;
-#if defined(OS_WIN)
-  MediaFoundationVideoEncodeAccelerator::PreSandboxInitialization();
-  encoder.reset(new MediaFoundationVideoEncodeAccelerator());
-#endif
-  return encoder;
 }
 
 void VEAClient::CreateEncoder() {
   DCHECK(thread_checker_.CalledOnValidThread());
   LOG_ASSERT(!has_encoder());
 
   std::unique_ptr<VideoEncodeAccelerator> encoders[] = {
       CreateFakeVEA(), CreateV4L2VEA(), CreateVaapiVEA(), CreateVTVEA(),
       CreateMFVEA()};
 
@@ skipping 495 matching lines @@
   IvfFrameHeader header = {};
 
   header.frame_size = static_cast<uint32_t>(frame_size);
   header.timestamp = frame_index;
   header.ByteSwap();
   EXPECT_TRUE(base::AppendToFile(
       base::FilePath::FromUTF8Unsafe(test_stream_->out_filename),
       reinterpret_cast<char*>(&header), sizeof(header)));
 }
 
+// This client is only used to test crbug.com/641600 to make sure the encoder
+// does not return an encoded frame before getting any input.
+class VEANoInputClient : public VEAClientBase {
+ public:
+  VEANoInputClient(ClientStateNotification<ClientState>* note);

[Owen Lin, 2016/11/04 03:38:50]

explicit

[henryhsu, 2016/11/04 04:15:17]

Done.

+  ~VEANoInputClient() = default;
+  void CreateEncoder();
+  void DestroyEncoder();
+
+  // VideoDecodeAccelerator::Client implementation.
+  void RequireBitstreamBuffers(unsigned int input_count,
+                               const gfx::Size& input_coded_size,
+                               size_t output_buffer_size) override;
+  void BitstreamBufferReady(int32_t bitstream_buffer_id,
+                            size_t payload_size,
+                            bool key_frame,
+                            base::TimeDelta timestamp) override;
+  void NotifyError(VideoEncodeAccelerator::Error error) override;
+
+ private:
+  void SetState(ClientState new_state);
+
+  // Provide the encoder with a new output buffer.
+  void FeedEncoderWithOutput(base::SharedMemory* shm, size_t output_size);
+
+  ClientState state_;
+
+  // Used to notify another thread about the state. VEAClient does not own this.
+  ClientStateNotification<ClientState>* note_;
+
+  // All methods of this class should be run on the same thread.
+  base::ThreadChecker thread_checker_;
+
+  // Ids for output BitstreamBuffers.
+  ScopedVector<base::SharedMemory> output_shms_;
+  int32_t next_output_buffer_id_;
+
+  // The timer used to monitor the encoder doesn't return an output buffer in
+  // a period of time.
+  std::unique_ptr<base::Timer> timer_;
+};
+
+VEANoInputClient::VEANoInputClient(ClientStateNotification<ClientState>* note)
+    : state_(CS_CREATED),
+      note_(note),
+      next_output_buffer_id_(0) {
+}
+
+void VEANoInputClient::CreateEncoder() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  LOG_ASSERT(!has_encoder());
+
+  std::unique_ptr<VideoEncodeAccelerator> encoders[] = {
+      CreateFakeVEA(), CreateV4L2VEA(), CreateVaapiVEA(), CreateVTVEA(),
+      CreateMFVEA()};
+
+  // The test case is no input. Use default visible size, bitrate, and fps.
+  gfx::Size visible_size(320, 200);

[Owen Lin, 2016/11/04 03:38:51]

How about making them static const variable.

[henryhsu, 2016/11/04 04:15:17]

Done.

+  for (size_t i = 0; i < arraysize(encoders); ++i) {

[Owen Lin, 2016/11/04 03:38:51]

for (const auto& encoder : encoders) {
}

[henryhsu, 2016/11/04 04:15:17]

Done.

+    if (!encoders[i])
+      continue;
+    encoder_ = std::move(encoders[i]);
+    if (encoder_->Initialize(kInputFormat, visible_size,
+                             VP8PROFILE_ANY,
+                             200000, this)) {
+      encoder_->RequestEncodingParametersChange(200000, 30);
+      SetState(CS_INITIALIZED);
+      return;
+    }
+  }
+  encoder_.reset();
+  LOG(ERROR) << "VideoEncodeAccelerator::Initialize() failed";
+  SetState(CS_ERROR);
+}
+
+void VEANoInputClient::DestroyEncoder() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!has_encoder())
+    return;
+  // Clear the objects that should be destroyed on the same thread as creation.
+  encoder_.reset();
+  timer_.reset();
+}
+
+void VEANoInputClient::RequireBitstreamBuffers(
+    unsigned int input_count,
+    const gfx::Size& input_coded_size,
+    size_t output_size) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  SetState(CS_ENCODING);
+  ASSERT_GT(output_size, 0UL);
+
+  for (unsigned int i = 0; i < kNumOutputBuffers; ++i) {
+    base::SharedMemory* shm = new base::SharedMemory();
+    LOG_ASSERT(shm->CreateAndMapAnonymous(output_size));
+    output_shms_.push_back(shm);
+    FeedEncoderWithOutput(shm, output_size);
+  }
+  timer_.reset(new base::Timer(
+          FROM_HERE, base::TimeDelta::FromMilliseconds(100),

[Owen Lin, 2016/11/04 03:38:51]

Why 100 ms?

[henryhsu, 2016/11/04 04:15:17]

frame rate is usually 30. 100 ms is about 3 frames duration.
I think it is long enough.

[Owen Lin, 2016/11/04 07:24:26]

Acknowledged.

+          base::Bind(&VEANoInputClient::SetState,
+                     base::Unretained(this), CS_FINISHED),
+          false));
+  timer_->Reset();
+}
+
+void VEANoInputClient::BitstreamBufferReady(int32_t bitstream_buffer_id,
+                                            size_t payload_size,
+                                            bool key_frame,
+                                            base::TimeDelta timestamp) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  SetState(CS_ERROR);
+}
+
+void VEANoInputClient::NotifyError(VideoEncodeAccelerator::Error error) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  SetState(CS_ERROR);
+}
+
+void VEANoInputClient::SetState(ClientState new_state) {
+  DVLOG(4) << "Changing state " << state_ << "->" << new_state;
+  note_->Notify(new_state);
+  state_ = new_state;
+}
+
+void VEANoInputClient::FeedEncoderWithOutput(base::SharedMemory* shm,
+                                             size_t output_size) {
+  if (!has_encoder())
+    return;
+
+  base::SharedMemoryHandle dup_handle;
+  LOG_ASSERT(shm->ShareToProcess(base::GetCurrentProcessHandle(), &dup_handle));
+
+  BitstreamBuffer bitstream_buffer(next_output_buffer_id_++, dup_handle,
+                                   output_size);
+  encoder_->UseOutputBitstreamBuffer(bitstream_buffer);
+}
+
 // Test parameters:
 // - Number of concurrent encoders. The value takes effect when there is only
 //   one input stream; otherwise, one encoder per input stream will be
 //   instantiated.
 // - If true, save output to file (provided an output filename was supplied).
 // - Force a keyframe every n frames.
 // - Force bitrate; the actual required value is provided as a property
 //   of the input stream, because it depends on stream type/resolution/etc.
 // - If true, measure performance.
 // - If true, switch bitrate mid-stream.
@@ skipping 122 matching lines @@
         std::make_tuple(3, false, 0, false, false, false, false, false, false),
         std::make_tuple(3, false, 0, true, false, false, true, false, false),
         std::make_tuple(3, false, 0, true, false, true, false, false, false)));
 
 INSTANTIATE_TEST_CASE_P(
     VerifyTimestamp,
     VideoEncodeAcceleratorTest,
     ::testing::Values(
         std::make_tuple(1, false, 0, false, false, false, false, false, true)));
 
+TEST(VEANoInputTest, CheckOutput) {
+  std::unique_ptr<ClientStateNotification<ClientState>> note;
+  std::unique_ptr<VEANoInputClient> client;
+  base::Thread encoder_thread("EncoderThread");
+  ASSERT_TRUE(encoder_thread.Start());
+
+  // Create encoder.
+  note.reset(new ClientStateNotification<ClientState>());

[Owen Lin, 2016/11/04 03:38:51]

why not merge the line with declaration.

[henryhsu, 2016/11/04 04:15:17]

Done.

+  client.reset(new VEANoInputClient(note.get()));
+
+  encoder_thread.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&VEANoInputClient::CreateEncoder,
+                            base::Unretained(client.get())));
+
+  // All encoders must pass through states in this order.
+  enum ClientState state_transitions[] = {
+      CS_INITIALIZED, CS_ENCODING, CS_FINISHED};
+
+  for (size_t state_no = 0; state_no < arraysize(state_transitions);

[Owen Lin, 2016/11/04 03:38:51]

can we use range based for loop here as well.

[henryhsu, 2016/11/04 04:15:17]

Done.

+       ++state_no) {
+    ASSERT_EQ(note->Wait(), state_transitions[state_no]);
+  }
+
+  encoder_thread.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&VEANoInputClient::DestroyEncoder,
+                            base::Unretained(client.get())));
+
+  // This ensures all tasks have finished.
+  encoder_thread.Stop();
+}
+
 #elif defined(OS_MACOSX) || defined(OS_WIN)
 INSTANTIATE_TEST_CASE_P(
     SimpleEncode,
     VideoEncodeAcceleratorTest,
     ::testing::Values(
         std::make_tuple(1, true, 0, false, false, false, false, false, false),
         std::make_tuple(1, true, 0, false, false, false, false, true, false)));
 
 INSTANTIATE_TEST_CASE_P(
     EncoderPerf,
@@ skipping 110 matching lines @@
 
   media::g_env =
       reinterpret_cast<media::VideoEncodeAcceleratorTestEnvironment*>(
           testing::AddGlobalTestEnvironment(
               new media::VideoEncodeAcceleratorTestEnvironment(
                   std::move(test_stream_data), log_path, run_at_fps,
                   needs_encode_latency, verify_all_output)));
 
   return RUN_ALL_TESTS();
 }