Add vaapi_h264_decoder_test.

content/common/gpu/media/vaapi_h264_decoder_test.cc

Index: content/common/gpu/media/vaapi_h264_decoder_test.cc
diff --git a/content/common/gpu/media/vaapi_h264_decoder_test.cc b/content/common/gpu/media/vaapi_h264_decoder_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..20180e9e771ef481c54d68f18f573771c15eb30e
--- /dev/null
+++ b/content/common/gpu/media/vaapi_h264_decoder_test.cc
@@ -0,0 +1,234 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.

[Pawel Osciak, 2013/10/20 23:53:11]

Why is this test not using gtest (even though you include it in dependencies)?
It should.
How is it supposed to be run?

[chihchung, 2013/10/21 09:33:05]

It is supposed to be run as a standalone program, not a unit test. The gtest
dependency is removed.

Comments added:
// This program is run like this:
// DISPLAY=:0 ./vaapi_h264_decoder_test -v=1 input.264 output.yuv
// The input is read from input.264 and output written to output.yuv

[Pawel Osciak, 2013/11/21 06:31:41]

But ultimately this is to be used for tests, frame comparisons for yuv output
from decoder against reference yuv streams. You will have to write a gtest
anyway. I think the right thing to do here is to do what vdatest does, turn this
into a gtest, have it accept input and output and have short input/output
streams committed to testdata dir and otherwise use external streams for real
tests.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stdio.h>

[Pawel Osciak, 2013/10/20 23:53:11]

Headers should be in lexicographical order. But see comments about stdio usage
below.

[chihchung, 2013/10/21 09:33:05]

Yes, I did that originally, but then presubmit check told me C system files
should be before C++ system files :)

[Pawel Osciak, 2013/11/21 06:31:41]

Perhaps if you used <cstdio> instead of <stdio.h>, as you should've.

[chihchung, 2013/11/21 12:17:31]

It's not needed anymore, so removed.

+#include <iostream>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/synchronization/lock.h"
+#include "content/common/gpu/media/vaapi_h264_decoder.h"
+#include "media/base/video_decoder_config.h"
+
+namespace content {
+
+class VaapiH264DecoderLoop {

[Pawel Osciak, 2013/10/20 23:53:11]

Please comment on what the class does.

[chihchung, 2013/10/21 09:33:05]

Done.

+ public:
+  VaapiH264DecoderLoop();
+  ~VaapiH264DecoderLoop();
+
+  // Initialize the decoder. Return true if successful.
+  bool Initialize(base::FilePath input_path, base::FilePath output_path);
+
+  // Run the decode loop. The decoded YUV data is written to the file specified
+  // by output_path in Initialize().  Return true if all decoding is successful.
+  bool Run();
+
+ private:
+  scoped_ptr<VaapiH264Decoder> decoder_;

[Pawel Osciak, 2013/10/20 23:53:11]

Methods should precede member variables.

[chihchung, 2013/10/21 09:33:05]

Done.

+  scoped_ptr<VaapiWrapper> wrapper_;
+  std::string data_;  // data read from input_path
+  base::Lock available_surfaces_lock_;  // protects available_surfaces_
+  std::vector<VASurfaceID> available_surfaces_;
+
+  // Members need to be freed manually.

[Pawel Osciak, 2013/10/20 23:53:11]

Which members?

[chihchung, 2013/10/21 09:33:05]

Done.

+  Display *x_display_;
+  FILE* output_file_;  // output data is written to this file

[Pawel Osciak, 2013/10/20 23:53:11]

Please use file_util::AppendToFile() instead of stdio.h methods. See example in
content/common/gpu/media/video_encode_accelerator_unittest.cc.

[chihchung, 2013/10/21 09:33:05]

Is there some advantage using that over fwrite?

(Originally I used AppendToFile(), but then I found I need to open an empty file
before appending to it, so I use the current form.)

[Pawel Osciak, 2013/11/21 06:31:41]

Yes, that was one of the reasons I pointed you to the example. Please use
AppendToFile, even if just to be consistent and more readable.
But stuff like not having to have code to manage/fopen/free FILE* pointer is
good enough reason.

[chihchung, 2013/11/21 12:17:31]

Good idea. Done.

+  int picture_count_;  // number of pictures already outputted
+
+  void ReportToUMA(VaapiH264Decoder::VAVDAH264DecoderFailure error) {}
+
+  // Callback from the decoder when a picture is decoded.
+  void OutputPicture(int32 input_id,
+                     const scoped_refptr<VASurface>& va_surface);
+
+  // Free one surface and put it on available_surfaces_ list. This may be called

[Pawel Osciak, 2013/10/20 23:53:11]

Does this really free? It looks like it just recycles it.

[chihchung, 2013/10/21 09:33:05]

Yes, this just recycles it. Fixed.

+  // from another thread.

[Pawel Osciak, 2013/10/20 23:53:11]

Which thread? What is the threading model of this class?

[chihchung, 2013/10/21 09:33:05]

I misunderstood the threading model of VaapiH264Decoder, now this program is all
single-thread and all lock stuff are removed.

+  void RecycleSurface(VASurfaceID va_surface_id);

[Pawel Osciak, 2013/10/20 23:53:11]

You could use VASurface class and pass to it RecycleSurface() as the destruction
callback.

[chihchung, 2013/10/21 09:33:05]

I think I did that in RefillSurfaces(), or I should do it some other way?

[Pawel Osciak, 2013/11/21 06:31:41]

Use VASurface class in c/c/gpu/media/va_surface.h, which does exactly this
already, in ReleaseCB callback.

[chihchung, 2013/11/21 12:17:31]

Sorry I still don't understand. This is the code in RefillSurfaces():

    VASurface::ReleaseCB release_cb = base::Bind(
        &VaapiH264DecoderLoop::RecycleSurface, base::Unretained(this));
    scoped_refptr<VASurface> surface(
        new VASurface(available_surfaces_[i], release_cb));

It is using VASurface and ReleaseCB. Could you elaborate a bit more if this is
not expected?

+
+  // Give all surfaces in available_surfaces_ to the decoder.
+  void RefillSurfaces();
+
+  // Free the current set of surfaces and allocate a new set of
+  // surfaces. Returns true when successful.
+  bool AllocateNewSurfaces();
+};
+
+
+VaapiH264DecoderLoop::VaapiH264DecoderLoop() : x_display_(NULL),

[Pawel Osciak, 2013/10/20 23:53:11]

New line and 4-space indent on multiline initializer lists.

[chihchung, 2013/10/21 09:33:05]

Done.

+                                               output_file_(NULL),
+                                               picture_count_(0) {
+}
+
+VaapiH264DecoderLoop::~VaapiH264DecoderLoop() {
+  // We need to destruct decoder and wrapper first because:
+  // (1) The decoder may have references to some surfaces which will be
+  // recycled, and RecycleSurface() uses available_surfaces_lock_.
+  // (2) The wrapper have references to x_display_.
+  decoder_.reset();
+  wrapper_.reset();
+
+  if (x_display_) {
+    XCloseDisplay(x_display_);
+  }
+  if (output_file_) {
+    fclose(output_file_);
+  }
+}
+
+bool VaapiH264DecoderLoop::Initialize(base::FilePath input_path,
+                                      base::FilePath output_path) {
+  x_display_ = XOpenDisplay(NULL);
+  if (!x_display_) {
+    LOG(ERROR) << "Can't open X display";
+    return false;
+  }
+
+  media::VideoCodecProfile profile = media::H264PROFILE_HIGH;

[Pawel Osciak, 2013/10/20 23:53:11]

Shouldn't profile be a parameter of the test, depending on the file?

[chihchung, 2013/10/21 09:33:05]

Yes, I just picked a value which looks like a larger subset of H.264 to avoid
the trouble parsing the stream, and it seems to work fine for base/main profile
streams also.

I don't really understand this parameter in VA API. Maybe VA API can use this
parameter to reserved less resources if given profile needs less, or fails
immediately if some profile is not supported.

[Pawel Osciak, 2013/11/21 06:31:41]

Right, but did you test the other way around? In Chrome we select the profile,
so for base we use base, for main we use main. If you use high for everything it
may work well, but you may not catch a problem with using base for base for
example.
 
Most probably. Digging in libva-intel-driver would probably give you an answer.

[chihchung, 2013/11/21 12:17:31]

Grepping VAProfileH264 in libva-intel-driver, there is no difference at all in
handling Baseline/Main/High...

+  base::Closure report_error_cb = base::Bind(&VaapiH264DecoderLoop::ReportToUMA,
+                                          base::Unretained(this),

[Pawel Osciak, 2013/10/20 23:53:11]

Indent.

[chihchung, 2013/10/21 09:33:05]

Done.

+                                          VaapiH264Decoder::VAAPI_ERROR);
+  wrapper_ = VaapiWrapper::Create(profile, x_display_, report_error_cb);
+  if (!wrapper_.get()) {
+    LOG(ERROR) << "Can't create vaapi wrapper";
+    return false;
+  }
+
+  decoder_.reset(new VaapiH264Decoder(
+      wrapper_.get(),
+      base::Bind(&VaapiH264DecoderLoop::OutputPicture, base::Unretained(this)),
+      base::Bind(&VaapiH264DecoderLoop::ReportToUMA, base::Unretained(this))));
+
+  if (!base::ReadFileToString(input_path, &data_)) {
+    LOG(ERROR)<< "failed to read input data from " << input_path.value() ;
+    return false;
+  }
+
+  decoder_->SetStream((uint8*)(data_.c_str()), data_.size(), 0 /* input_id */);

[Pawel Osciak, 2013/10/20 23:53:11]

No c-style casts, here and everywhere.

[chihchung, 2013/10/21 09:33:05]

Done.

+
+  output_file_ = fopen(output_path.value().c_str(), "w");
+  if (!output_file_) {
+    return false;
+  }
+
+  return true;
+}
+
+bool VaapiH264DecoderLoop::Run() {
+  while (1) {
+    switch (decoder_->Decode()) {
+      case VaapiH264Decoder::kDecodeError:
+        LOG(ERROR) << "Decode Error";
+        return false;
+      case VaapiH264Decoder::kAllocateNewSurfaces:
+        VLOG(1) << "Allocate new surfaces";
+        if (!AllocateNewSurfaces()) {
+          LOG(ERROR) << "Failed to allocate new surfaces";
+          return false;
+        }
+        break;
+      case VaapiH264Decoder::kRanOutOfStreamData:

[Pawel Osciak, 2013/10/20 23:53:11]

Brace on this line.

[chihchung, 2013/10/21 09:33:05]

Done.

+        {
+          bool rc = decoder_->Flush();
+          VLOG(1) << "Flush returns " << rc;
+          return rc;
+        }
+      case VaapiH264Decoder::kRanOutOfSurfaces:
+        VLOG(1) << "Ran out of surfaces";
+        RefillSurfaces();
+        break;
+    }
+  }
+}
+
+void VaapiH264DecoderLoop::OutputPicture(
+    int32 input_id,
+    const scoped_refptr<VASurface>& va_surface) {
+  VLOG(1) << "OutputPicture: picture " << picture_count_++;
+  void *buffer;

[Pawel Osciak, 2013/10/20 23:53:11]

star goes next to void. Here and everywhere else.

[chihchung, 2013/10/21 09:33:05]

Done.

+  size_t size;
+  if (wrapper_->GetI420FromSurface(va_surface->id(), &buffer, &size)) {
+    if (fwrite((const char *)buffer, size, 1, output_file_) != 1) {
+      LOG(ERROR) << "fwrite failed";
+    }
+    free(buffer);
+  } else {
+    LOG(ERROR) << "Cannot convert surface to I420.";
+  }
+}
+
+void VaapiH264DecoderLoop::RecycleSurface(VASurfaceID va_surface_id) {

[Pawel Osciak, 2013/10/20 23:53:11]

As mentioned above, you can use VASurface and pass this as the destruction
callback.

[chihchung, 2013/10/21 09:33:05]

I did, or I should do it some other way?

+  base::AutoLock auto_lock(available_surfaces_lock_);
+  available_surfaces_.push_back(va_surface_id);
+}
+
+void VaapiH264DecoderLoop::RefillSurfaces() {
+  base::AutoLock auto_lock(available_surfaces_lock_);
+  for (size_t i = 0; i < available_surfaces_.size(); i++) {
+    VASurface::ReleaseCB release_cb = base::Bind(
+        &VaapiH264DecoderLoop::RecycleSurface, base::Unretained(this));
+    scoped_refptr<VASurface> surface(
+        new VASurface(available_surfaces_[i], release_cb));
+    decoder_->ReuseSurface(surface);
+  }
+  available_surfaces_.clear();
+}
+
+bool VaapiH264DecoderLoop::AllocateNewSurfaces() {
+  {
+    // TODO: make sure all surfaces are returned

[Pawel Osciak, 2013/10/20 23:53:11]

Todos have to be in this format:

TODO(name):

[chihchung, 2013/10/21 09:33:05]

Done.

+    base::AutoLock auto_lock(available_surfaces_lock_);
+    available_surfaces_.clear();
+  }
+
+  wrapper_->DestroySurfaces();
+
+  gfx::Size size = decoder_->GetPicSize();
+  size_t num_surfaces = decoder_->GetRequiredNumOfPictures();
+  return wrapper_->CreateSurfaces(size, num_surfaces, &available_surfaces_);
+}
+
+}  // namespace content
+
+int main(int argc, char** argv) {

[Pawel Osciak, 2013/10/20 23:53:11]

Please use gtest.

[chihchung, 2013/10/21 09:33:05]

As explained above, this is not a unit test (although I'm thinking about
building unit tests using this program), so I think we don't need gtest.

+  CommandLine::Init(argc, argv);
+
+  // Needed to enable DVLOG through --vmodule.
+  logging::LoggingSettings settings;
+  settings.logging_dest = logging::LOG_TO_SYSTEM_DEBUG_LOG;
+  settings.dcheck_state =
+      logging::ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS;
+  CHECK(logging::InitLogging(settings));
+
+  // Process command line.
+  CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+  CHECK(cmd_line);
+  const CommandLine::StringVector& args = cmd_line->GetArgs();
+  if (args.size() != 2) {
+    std::cerr << "Usage: vaapi_h264_decoder_test input_file output_file\n";
+    return EXIT_FAILURE;
+  }
+
+  // We are not in a sandbox, but we still need to do the initialization.
+  content::VaapiWrapper::PreSandboxInitialization();
+
+  base::FilePath input_path(args[0]);
+  base::FilePath output_path(args[1]);
+
+  VLOG(1) << "Input File: " << input_path.value();
+  VLOG(1) << "Output File: " << output_path.value();
+
+  content::VaapiH264DecoderLoop loop;
+  if (!loop.Initialize(input_path, output_path)) {
+    std::cerr << "initialize decoder loop failed";
+    return EXIT_FAILURE;
+  }
+  if (!loop.Run()) {
+    std::cerr << "run decoder loop failed";
+    return EXIT_FAILURE;
+  }
+  return 0;
+}