h264_decoder_unittests: Initial Change.

media/gpu/h264_decoder_unittest.cc

Index: media/gpu/h264_decoder_unittest.cc
diff --git a/media/gpu/h264_decoder_unittest.cc b/media/gpu/h264_decoder_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f03ac7038711a69040cebe64a0b0600d59d4db8b
--- /dev/null
+++ b/media/gpu/h264_decoder_unittest.cc
@@ -0,0 +1,385 @@
+// Copyright 2017 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 <stdint.h>
+#include <string.h>
+
+#include <memory>
+#include <queue>
+#include <string>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
+#include "media/base/test_data_util.h"
+#include "media/gpu/h264_decoder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+namespace {
+
+const char* kBaselineFrame0 = "bear-320x192-baseline-frame-0.h264";

[Pawel Osciak, 2017/03/23 08:34:21]

Perhaps const std::string, as GetTestDataFilePath() also takes an std::string ?

[Owen Lin, 2017/06/21 07:37:29]

Done.

+const char* kBaselineFrame1 = "bear-320x192-baseline-frame-1.h264";
+const char* kBaselineFrame2 = "bear-320x192-baseline-frame-2.h264";
+const char* kBaselineFrame3 = "bear-320x192-baseline-frame-3.h264";
+const char* kHighFrame0 = "bear-320x192-high-frame-0.h264";
+const char* kHighFrame1 = "bear-320x192-high-frame-1.h264";
+const char* kHighFrame2 = "bear-320x192-high-frame-2.h264";
+const char* kHighFrame3 = "bear-320x192-high-frame-3.h264";
+
+// Forward declaration

[Pawel Osciak, 2017/03/23 08:34:20]

Nit: comment perhaps not needed.

[Owen Lin, 2017/06/21 07:37:29]

Done.

+class FakeH264Accelerator;
+
+class H264DecoderTest : public ::testing::Test {

[Pawel Osciak, 2017/03/23 08:34:21]

Please add a comment what this test is for.

[Owen Lin, 2017/06/21 07:37:29]

Done.

+ public:
+  // The events we used to verify the correctness. Will check if these
+  // events are triggers in order.
+  struct Event {
+    enum Type {

[Pawel Osciak, 2017/03/23 08:34:20]

enum class?

[Owen Lin, 2017/06/21 07:37:28]

Done.

+      CREATE_H264_PICTURE,
+      SUBMIT_DECODE,
+      OUTPUT_PICTURE,
+      RAN_OUT_OF_SURFACES,
+      NEED_CONTEXT_UPDATE,
+      ALLOCATE_NEW_SURFACES,
+      DECODE_ERROR,
+    };
+
+    Type type;
+
+    Event(Type type) : type(type) {}

[Pawel Osciak, 2017/03/23 08:34:20]

explicit?

[Owen Lin, 2017/06/21 07:37:29]

You're right. Implicit conversion is not allowed.

+  };
+
+  // The specialize Event for OutputPicture as we will also check the POC.
+  struct OutputPictureEvent : public Event {
+    int pic_order_cnt;
+
+    OutputPictureEvent(int pic_order_cnt)
+        : Event(OUTPUT_PICTURE), pic_order_cnt(pic_order_cnt) {}
+  };
+
+  // Function called when an event dispatch to the test.
+  using EventHandler = std::function<void(const Event& event)>;

[Pawel Osciak, 2017/03/23 08:34:20]

std::function is not allowed per Chromium coding style
(https://chromium-cpp.appspot.com/#library-blacklist). Please use
base::Callback.

[Owen Lin, 2017/06/21 07:37:29]

Done.

+
+  H264DecoderTest() {}

[Pawel Osciak, 2017/03/23 08:34:20]

= default; perhaps?

[Owen Lin, 2017/06/21 07:37:29]

Done.

+
+  void SetUp() override;
+
+  // Helper function to feed bitstreams.
+  void FeedData(std::vector<const char*> frames);
+
+  // Called when an event happens.
+  void OnEvent(const Event& event);
+
+  // Expects the specified event will be dispatched to the test. When it
+  // happens, handler will be called. The expected events will be queued.
+  // This function could be called multiple times and we expect those
+  // events will happen in order.
+  void ExpectEvent(Event::Type type, EventHandler handler = {});
+
+  // Expects the decoder output a picture with the specified POC..
+  void ExpectOutputPictureWithPoc(int expected_poc);
+
+  // Gets the number of remaining expected events.
+  size_t GetNumberOfExpectedEvents() const { return expected_events_.size(); }

[DaleCurtis, 2017/03/21 18:07:18]

Inline should be hacker_style().

[Owen Lin, 2017/06/21 07:37:29]

Done.

+
+ protected:

[Pawel Osciak, 2017/03/23 08:34:20]

Do we need protected instead of private?

[Owen Lin, 2017/06/21 07:37:29]

Some tests can directly access these members.

+  std::unique_ptr<H264Decoder> decoder_;
+  std::unique_ptr<FakeH264Accelerator> accelerator_;
+
+ private:
+  struct ExpectedEvent {
+    Event::Type type;
+    EventHandler handler;
+
+    ExpectedEvent(Event::Type type, EventHandler handler)
+        : type(type), handler(handler) {}
+  };
+
+  std::queue<ExpectedEvent> expected_events_;
+};
+
+class FakeH264Accelerator : public H264Decoder::H264Accelerator {
+ public:
+  FakeH264Accelerator(H264DecoderTest* test) : test_(test) {}
+
+  scoped_refptr<H264Picture> CreateH264Picture() override {
+    if (new_picture_quota_ > 0) {
+      --new_picture_quota_;
+      return new H264Picture();

[Pawel Osciak, 2017/03/23 08:34:20]

To mimic the actual behavior of the accelerator, we should perhaps have a simple
subclass of H264Picture that increments picture_quota when all refs to it are
dropped.

[Owen Lin, 2017/06/21 07:37:29]

It sounds like we should add some tests about free H264Picture.

In this test, I would like to test each part of the decoder individually. So, I
would prefer to add other tests to see H264Picture is released as expected.

[Pawel Osciak, 2017/06/23 02:07:34]

Acknowledged.

+    }
+    return nullptr;
+  }
+
+  // Implementations of H264Decoder::H264Accelerator interface.
+
+  bool SubmitFrameMetadata(const H264SPS* sps,
+                           const H264PPS* pps,
+                           const H264DPB& dpb,
+                           const H264Picture::Vector& ref_pic_listp0,
+                           const H264Picture::Vector& ref_pic_listb0,
+                           const H264Picture::Vector& ref_pic_listb1,
+                           const scoped_refptr<H264Picture>& pic) override {
+    return true;
+  }
+
+  bool SubmitSlice(const H264PPS* pps,
+                   const H264SliceHeader* slice_hdr,
+                   const H264Picture::Vector& ref_pic_list0,
+                   const H264Picture::Vector& ref_pic_list1,
+                   const scoped_refptr<H264Picture>& pic,
+                   const uint8_t* data,
+                   size_t size) override {
+    return true;
+  }
+
+  bool SubmitDecode(const scoped_refptr<H264Picture>& pic) override {
+    test_->OnEvent(H264DecoderTest::Event::SUBMIT_DECODE);
+    return true;
+  }
+
+  bool OutputPicture(const scoped_refptr<H264Picture>& pic) override {
+    test_->OnEvent(H264DecoderTest::OutputPictureEvent(pic->pic_order_cnt));
+    return true;
+  }
+
+  void Reset() override {}
+
+  // How many times CreateH264Picture() will returns valid H264Picture.
+  void SetNewPictureQuota(size_t quota) { new_picture_quota_ = quota; }
+
+  size_t GetNewPictureQuota() const { return new_picture_quota_; }
+
+ private:
+  H264DecoderTest* test_;
+  size_t new_picture_quota_ = 0;
+};
+
+std::ostream& operator<<(std::ostream& os,
+                         const H264DecoderTest::Event::Type type) {
+  switch (type) {
+    case H264DecoderTest::Event::CREATE_H264_PICTURE:
+      return os << "CreateH264Picture";
+    case H264DecoderTest::Event::SUBMIT_DECODE:
+      return os << "SubmitDecode";
+    case H264DecoderTest::Event::OUTPUT_PICTURE:
+      return os << "OutputPicture";
+    case H264DecoderTest::Event::RAN_OUT_OF_SURFACES:
+      return os << "RanOutOfSurfaces";
+    case H264DecoderTest::Event::NEED_CONTEXT_UPDATE:
+      return os << "NeedContextUpdate";
+    case H264DecoderTest::Event::ALLOCATE_NEW_SURFACES:
+      return os << "AllocateNewSurfaces";
+    case H264DecoderTest::Event::DECODE_ERROR:
+      return os << "DecodeError";
+  }
+  return os;
+}
+
+std::ostream& operator<<(std::ostream& os, const H264DecoderTest::Event& e) {
+  switch (e.type) {
+    case H264DecoderTest::Event::OUTPUT_PICTURE: {
+      const H264DecoderTest::OutputPictureEvent& event =
+          static_cast<const H264DecoderTest::OutputPictureEvent&>(e);

[Pawel Osciak, 2017/03/23 08:34:20]

Perhaps we could instead have a virtual AsString() method in Event, that would
be overriden in OutputPictureEvent to also output poc and call e.AsString() from
here without having to check the type and cast?

[Owen Lin, 2017/06/21 07:37:29]

Done. But follow most namings in chromium as ToString().

+      return os << e.type << "(poc=" << event.pic_order_cnt << ")";
+    }
+    default:
+      return os << e.type;
+  }
+}
+
+void H264DecoderTest::SetUp() {
+  accelerator_.reset(new FakeH264Accelerator(this));
+  decoder_.reset(new H264Decoder(accelerator_.get()));
+}
+
+void H264DecoderTest::FeedData(std::vector<const char*> frames) {

[Pawel Osciak, 2017/03/23 08:34:20]

Perhaps const vector<std::string>&?

[Owen Lin, 2017/06/21 07:37:29]

Done.

+  std::string bitstream;
+  auto frame_iter = frames.begin();
+  while (true) {
+    switch (decoder_->Decode()) {
+      case AcceleratedVideoDecoder::kAllocateNewSurfaces:
+        ASSERT_NO_FATAL_FAILURE(OnEvent(Event::ALLOCATE_NEW_SURFACES));
+        break;
+      case AcceleratedVideoDecoder::kRanOutOfStreamData: {
+        if (frame_iter == frames.end())
+          return;
+        base::FilePath input_file = GetTestDataFilePath(*frame_iter++);
+        ASSERT_TRUE(base::ReadFileToString(input_file, &bitstream))
+            << "failed to read input data from " << input_file.value();
+        decoder_->SetStream(reinterpret_cast<const uint8_t*>(bitstream.data()),
+                            bitstream.size());
+        break;
+      }
+      case AcceleratedVideoDecoder::kRanOutOfSurfaces:
+        ASSERT_NO_FATAL_FAILURE(OnEvent(Event::RAN_OUT_OF_SURFACES));
+        break;
+      case AcceleratedVideoDecoder::kNeedContextUpdate:
+        ASSERT_NO_FATAL_FAILURE(OnEvent(Event::NEED_CONTEXT_UPDATE));
+        break;
+      case AcceleratedVideoDecoder::kDecodeError:
+        ASSERT_NO_FATAL_FAILURE(OnEvent(Event::DECODE_ERROR));
+        break;
+    }
+  }
+}
+
+void H264DecoderTest::OnEvent(const Event& event) {
+  VLOG(2) << "OnEvent(" << event << ")";
+  ASSERT_FALSE(expected_events_.empty()) << "Unexpected event";
+  ExpectedEvent ee = expected_events_.front();
+  expected_events_.pop();
+  ASSERT_EQ(ee.type, event.type);
+  if (ee.handler)
+    ASSERT_NO_FATAL_FAILURE(ee.handler(event));
+}
+
+void H264DecoderTest::ExpectEvent(Event::Type type, EventHandler handler) {

[Pawel Osciak, 2017/03/23 08:34:20]

Should this take ownership of handler here and when creating the ExpectedEvent?

[Owen Lin, 2017/06/21 07:37:29]

I did some code study I think 
we should pass by reference and do a copy when creating the ExpectedEvent.

+  expected_events_.push({type, handler});
+}
+
+void H264DecoderTest::ExpectOutputPictureWithPoc(int expected_poc) {
+  ExpectEvent(Event::OUTPUT_PICTURE, [expected_poc](const Event& e) {
+    auto event = static_cast<const OutputPictureEvent&>(e);
+    EXPECT_EQ(expected_poc, event.pic_order_cnt);
+  });
+}
+
+// Test Cases
+
+TEST_F(H264DecoderTest, DecodeSingleFrame) {
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES, [&](const Event&) {
+    EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192));

[Pawel Osciak, 2017/03/23 08:34:20]

Perhaps have another specialization of Event for ALLOCATE_NEW_SURFACES that
takes and verifies expected Size, just like the OUTPUT_PICTURE one does with
PoC?

[kcwu, 2017/03/23 08:54:33]

Please swap the arguments.
They are read as EXPECT_*(expect, actual);

[Owen Lin, 2017/06/21 07:37:29]

Done.

[Owen Lin, 2017/06/21 07:37:29]

Done.

+  });
+
+  ExpectEvent(Event::RAN_OUT_OF_SURFACES,
+              [&](const Event&) { accelerator_->SetNewPictureQuota(1); });
+
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectEvent(Event::OUTPUT_PICTURE);
+
+  ASSERT_NO_FATAL_FAILURE(FeedData({kBaselineFrame0}));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+TEST_F(H264DecoderTest, SkipNoneIDRFrames) {
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES, [&](const Event&) {
+    EXPECT_EQ(decoder_->GetPicSize(), gfx::Size(320, 192));

[kcwu, 2017/03/23 08:54:33]

Please swap the arguments.
They are read as EXPECT_*(expect, actual);

[Owen Lin, 2017/06/21 07:37:29]

Done.

+  });
+
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+
+  accelerator_->SetNewPictureQuota(1);
+  ASSERT_NO_FATAL_FAILURE(FeedData({
+      kBaselineFrame1, kBaselineFrame2, kBaselineFrame0,
+  }));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+TEST_F(H264DecoderTest, DecodeProfileBaseline) {
+  accelerator_->SetNewPictureQuota(4);

[Pawel Osciak, 2017/03/23 08:34:21]

Should we perhaps SetNewPictureQuota() in the ALLOCATE_NEW_SURFACES event
instead?

[Owen Lin, 2017/06/21 07:37:29]

I would prefer we focus on one thing we are testing with each test cases.

[Pawel Osciak, 2017/06/23 02:07:34]

Acknowledged.

+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(2);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(4);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(6);
+
+  ASSERT_NO_FATAL_FAILURE(FeedData({
+      kBaselineFrame0, kBaselineFrame1, kBaselineFrame2, kBaselineFrame3,
+  }));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+TEST_F(H264DecoderTest, DecodeProfileHigh) {
+  accelerator_->SetNewPictureQuota(4);
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+
+  // Two pictures will be kept in DPB for reordering. The first picture should
+  // be outputted after feeding the third frame.
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(2);
+  ExpectOutputPictureWithPoc(4);
+  ExpectOutputPictureWithPoc(6);
+
+  ASSERT_NO_FATAL_FAILURE(FeedData({
+      kHighFrame0, kHighFrame1, kHighFrame2, kHighFrame3,
+  }));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+TEST_F(H264DecoderTest, SwitchBaselineToHigh) {
+  accelerator_->SetNewPictureQuota(5);
+
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(2);
+  ExpectOutputPictureWithPoc(4);
+  ExpectOutputPictureWithPoc(6);
+
+  ASSERT_NO_FATAL_FAILURE(FeedData({
+      kBaselineFrame0, kHighFrame0, kHighFrame1, kHighFrame2, kHighFrame3,
+  }));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+TEST_F(H264DecoderTest, SwitchHighToBaseline) {
+  accelerator_->SetNewPictureQuota(5);
+
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::ALLOCATE_NEW_SURFACES);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(0);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(2);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(4);
+  ExpectEvent(Event::SUBMIT_DECODE);
+  ExpectOutputPictureWithPoc(6);
+
+  ASSERT_NO_FATAL_FAILURE(FeedData({
+      kHighFrame0, kBaselineFrame0, kBaselineFrame1, kBaselineFrame2,
+      kBaselineFrame3,
+  }));
+  ASSERT_TRUE(decoder_->Flush());
+
+  EXPECT_EQ(0u, accelerator_->GetNewPictureQuota());
+  EXPECT_EQ(0u, GetNumberOfExpectedEvents());
+}
+
+}  // namespace
+}  // namespace media