AndroidVDA by using Android's MediaCodec API.

content/common/gpu/media/android_video_decode_accelerator.cc

Index: content/common/gpu/media/android_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/android_video_decode_accelerator.cc b/content/common/gpu/media/android_video_decode_accelerator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b1079324de409f0f2f725bd589c37a27cfe4c78f
--- /dev/null
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -0,0 +1,366 @@
+// Copyright (c) 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 "content/common/gpu/media/android_video_decode_accelerator.h"
+
+#include <jni.h>
+
+#include "base/android/jni_android.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/bind.h"
+#include "base/debug/trace_event.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "content/common/android/surface_callback.h"
+#include "content/common/gpu/gpu_channel.h"
+#include "content/common/gpu/media/gles2_external_texture_copier.h"
+#include "media/base/android/media_codec_bridge.h"
+#include "media/base/bitstream_buffer.h"
+#include "media/video/picture.h"
+#include "third_party/angle/include/GLES2/gl2.h"
+#include "third_party/angle/include/GLES2/gl2ext.h"
+
+using base::android::MethodID;
+using base::android::ScopedJavaLocalRef;
+
+namespace content {
+
+#define LOG_LINE() VLOG(1) << __FUNCTION__

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

drop before submitting?

[dwkang1, 2013/01/28 14:54:30]

Marked with XXX.

+
+enum { kNumPictureBuffers = 4 };
+
+// TODO(dwkang) : For now, we are using very short timeouts for dequeueing
+// buffers in order to prevent dequeueing for input from blocking it for output,
+// and vice versa. Decoupling input and output procedure and using a longer
+// timeout value may improve performance.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

See my comment on the bridge class; I hope you can drop these.

[dwkang1, 2013/01/28 14:54:30]

Done.

+enum { kDequeueInputBufferTimeOutUs = 10 };
+enum { kDequeueOutputBufferTimeOutUs = 10 };
+
+// static
+const base::TimeDelta AndroidVideoDecodeAccelerator::kDecodePollDelay =
+    base::TimeDelta::FromMilliseconds(10);
+
+AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
+    media::VideoDecodeAccelerator::Client* client,
+    const base::Callback<bool(void)>& make_context_current)
+    : message_loop_(MessageLoop::current()),
+      client_(client),
+      make_context_current_(make_context_current),
+      codec_(UNKNOWN),
+      state_(NO_ERROR),
+      surface_texture_id_(0),

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

-1 to be more obvious?

[dwkang1, 2013/01/28 14:54:30]

Done.

+      picturebuffer_requested_(false),
+      color_format_(0),
+      width_(0),
+      height_(0),

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

should be able to drop once using gfx::Size (which has a no-arg zero-out ctor)

[dwkang1, 2013/01/28 14:54:30]

Removed.

+      current_bitstream_id_(-1) {
+  LOG_LINE();
+}
+
+AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+}
+
+bool AndroidVideoDecodeAccelerator::Initialize(
+    media::VideoCodecProfile profile) {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  if (profile == media::VP8PROFILE_MAIN) {
+    codec_ = VP8;
+  } else if (profile >= media::H264PROFILE_MIN
+      && profile <= media::H264PROFILE_MAX) {
+    codec_ = H264;
+  }else {

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

missing space

[dwkang1, 2013/01/28 14:54:30]

Done.

+    LOG(ERROR) << "Unsupported profile: " << profile;
+    return false;
+  }

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Once you pass profile to ConfigureMediaCodec below, I'd simply replace this
if/else chain with:
if (profile >= media::VP8PROFILE_MAX)
  return false;

[dwkang1, 2013/01/28 14:54:30]

codec_ is used when re-configuring MediaCodec.

+
+  if (media_codec_ == NULL) {

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

How could this *not* be NULL?

[dwkang1, 2013/01/28 14:54:30]

Changed to DCHECK.

+    if (!make_context_current_.Run()) {
+      LOG(ERROR) << "Failed to make this decoder's GL context current.";
+      return false;
+    }
+    glGenTextures(1, &surface_texture_id_);

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Please make sure this file follows the (brand-new) scheme for GL access:
http://crbug.com/169433

[dwkang1, 2013/01/28 14:54:30]

Let me address this issue once we finalize how we handle texture copy. Will be
marked with XXX and will be addressed before submitting.

+    glActiveTexture(GL_TEXTURE0);
+    glBindTexture(GL_TEXTURE_EXTERNAL_OES, surface_texture_id_);
+
+    glTexParameteri(GL_TEXTURE_EXTERNAL_OES, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    glTexParameteri(GL_TEXTURE_EXTERNAL_OES, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameteri(GL_TEXTURE_EXTERNAL_OES,
+                    GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+    glTexParameteri(GL_TEXTURE_EXTERNAL_OES,
+                    GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+
+    surface_texture_ = new SurfaceTextureBridge(surface_texture_id_);
+
+    ConfigureMediaCodec();
+  }
+
+  message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this)));

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Per below, drop.

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+  if (client_)
+    client_->NotifyInitializeDone();
+  return true;
+}
+
+void AndroidVideoDecodeAccelerator::DoDecode() {
+  if (state_ == NO_ERROR) {
+    QueueInput();
+    DequeueOutput();

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Shouldn't you drain the output first, to free up decoder resources and possibly
increase parallelism?

[dwkang1, 2013/01/28 14:54:30]

Makes sense. Changed.

+  }
+
+  message_loop_->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(
+          &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this)),

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Why is Unretained(this) safe here (and also at l.106 above)?
(I believe it is not)

[dwkang1, 2013/01/28 14:54:30]

You are right. Changed to base::AsWeakPtr().

+          kDecodePollDelay);

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Oh my goodness...   I didn't realize MediaCodec required polling...
Can you measure the cost of the poll in the do-nothing case?

Assuming there's no way around polling, I think you want to not queue a task
unless there is pending input or output,.

[dwkang1, 2013/01/28 14:54:30]

with 10ms, cost was about 3% of cpu rate.
Good idea. Thanks!

+}
+
+void AndroidVideoDecodeAccelerator::QueueInput() {
+  if (!pending_bitstream_buffers_.empty()) {

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

if you reverse the test you can early-return and de-indent the rest of the
function, and make it clearer what's going on.

[dwkang1, 2013/01/28 14:54:30]

Done.

+    int input_buf_index =
+        media_codec_->DequeueInputBuffer(kDequeueInputBufferTimeOutUs);
+    if (input_buf_index < 0) {
+      return;
+    }
+    media::BitstreamBuffer& bitstream_buffer =
+        pending_bitstream_buffers_.front();
+    pending_bitstream_buffers_.pop();
+
+    int flags = 0;
+    if (bitstream_buffer.id() == -1) {
+      flags |= media::MediaCodecBridge::BUFFER_FLAG_END_OF_STREAM;
+    }
+    if (bitstream_buffer.size() > 0) {
+      scoped_ptr<base::SharedMemory> shm(
+          new base::SharedMemory(bitstream_buffer.handle(), true));
+      if (!shm->Map(bitstream_buffer.size())) {
+        LOG(ERROR) << "Failed to SharedMemory::Map()";
+        client_->NotifyError(UNREADABLE_INPUT);
+        state_ = ERROR;
+        return;
+      }
+      media_codec_->PutToInputBuffer(
+          input_buf_index,
+          static_cast<const uint8*>(shm->memory()),
+          bitstream_buffer.size());
+    }
+    // Abuse the presentation time argument to propagate the bitstream
+    // buffer ID to the output, so we can report it back to the client in
+    // PictureReady().
+    int64 timestamp = bitstream_buffer.id();
+    media_codec_->QueueInputBuffer(
+        input_buf_index, 0, bitstream_buffer.size(), timestamp, flags);
+
+    if (bitstream_buffer.id() != -1) {
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

This is wrong; you should only NotifyEndOfBitstreamBuffer when the contents of
the buffer are known to have been decoded.
In other words, it's not OK to call PictureReady with a bitstreambufferid of a
bitstreambuffer for which you've already NEOBB'd.

[dwkang1, 2013/01/28 14:54:30]

When I wrote this code, I referred OVDA code. In
OmxVideoDecodeAccelerator::EmptyBufferDoneTask(), which is a callback for input
feeding, it calls NEOBB(). Do you think OVDA's implementation is wrong also?

[Ami GONE FROM CHROMIUM, 2013/01/28 19:49:45]

See my comment on the next patchset.

+    }
+  }
+}
+
+void AndroidVideoDecodeAccelerator::DequeueOutput() {
+  if (picturebuffer_requested_ && picture_map_.empty()) {
+    DLOG(INFO) << "Picture buffers are not ready.";

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

drop

[dwkang1, 2013/01/28 14:54:30]

Done.

+    return;
+  }
+  if (!picture_map_.empty() && free_picture_ids_.empty()) {

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Drop the picture_map_.empty() clause?
SendCurrentSurfaceToBuffer assumes free_pictures_id_ is non-empty.

[dwkang1, 2013/01/28 14:54:30]

Before picturebuffers is requested, free_picture_ids_ will be always empty. So,
we need !picture_map_.empty() clause.

+    // Don't have any picture buffer to send. Need to wait more.
+    return;
+  }
+
+  int32 output_offset = 0;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

this and the other params below don't seem to need the output_ prefix.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Declare vars at first use unless there's a reason not to (need to survive a
scope).  I think here there's no reason not to.

[dwkang1, 2013/01/28 14:54:30]

Done.

[dwkang1, 2013/01/28 14:54:30]

Done.

+  int32 output_size = 0;
+  int32 output_flag = 0;
+  int64 timestamp = 0;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

bitstream_buffer_id

[dwkang1, 2013/01/28 14:54:30]

Done.

+  int32 output_buf_index = 0;
+  do {
+    output_buf_index = media_codec_->DequeueOutputBuffer(
+        kDequeueOutputBufferTimeOutUs, &output_offset, &output_size,
+        &timestamp, &output_flag);
+    switch (output_buf_index) {
+      case media::MediaCodecBridge::INFO_TRY_AGAIN_LATER:
+        return;
+
+      case media::MediaCodecBridge::INFO_OUTPUT_FORMAT_CHANGED:
+        media_codec_->GetOutputFormat(&color_format_, &width_, &height_);
+        DLOG(INFO) << "Output color format: " << color_format_;
+        DLOG(INFO) << "Output size: " << width_ << "x" << height_;

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Drop these two lines

[dwkang1, 2013/01/28 14:54:30]

Done.

+        if (!picturebuffer_requested_) {
+          picturebuffer_requested_ = true;
+          texture_copier_.reset(new Gles2ExternalTextureCopier());
+          texture_copier_->Init(width_, height_);
+          client_->ProvidePictureBuffers(
+              kNumPictureBuffers,
+              gfx::Size(width_, height_),
+              GL_TEXTURE_2D);

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

If you requested a different texture target could you avoid the copy?

(what happened to our conversation about avoiding texture copies by swapping
textures out from under surfaces?)

[dwkang1, 2013/01/28 14:54:30]

Actually, using SurfaceTexure.attachToGLContext() and detachFromGLContext() was
the first thing I tried, but it failed because of the following two reasons.
1. Once we call detachFrameGLContext(), it deletes the texture previous
attached. You can check the implementation of
SurfaceTexture::detachFromContext() in
frameworks/native/libs/gui/SurfaceTexture.cpp If you have access to the Android
code.
2. This may not be a blocker for using the approach, but SurfaceTexture requires
us to apply a transform matrix when we show the texture. And according to the
documentation it may vary frame to frame.

+        }
+        // TODO(dwkang): support the dynamic resolution change.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

set state to ERROR and NotifyError?

[dwkang1, 2013/01/28 14:54:30]

Done.

+        return;
+
+      case media::MediaCodecBridge::INFO_OUTPUT_BUFFERS_CHANGED:
+        media_codec_->GetOutputBuffers();
+        break;
+    }
+  } while (output_buf_index < 0);
+
+  if (output_flag & media::MediaCodecBridge::BUFFER_FLAG_END_OF_STREAM) {
+    if (client_) {
+      client_->NotifyFlushDone();
+    }
+  }
+
+  media_codec_->ReleaseOutputBuffer(output_buf_index, true);
+  current_bitstream_id_ = static_cast<int32>(timestamp);

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

This doesn't need to be a class member; you can just as well pass it to SCSTC()
directly.

[dwkang1, 2013/01/28 14:54:30]

Done.

+  if (current_bitstream_id_ != -1) {
+    SendCurrentSurfaceToClient();
+  }
+}
+
+void AndroidVideoDecodeAccelerator::SendCurrentSurfaceToClient() {
+  LOG_LINE();
+
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  DCHECK_NE(current_bitstream_id_, -1);
+  DCHECK(!free_picture_ids_.empty());
+
+  int32 picture_buffer_id = free_picture_ids_.front();
+  free_picture_ids_.pop();
+
+  if (!make_context_current_.Run()) {
+    LOG(ERROR) << "Failed to make this decoder's GL context current.";

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Shouldn't this set the state to ERROR and also NotifyError to the client?  I
think you want a macro like
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/...

[dwkang1, 2013/01/28 14:54:30]

Done.

+    return;
+  }
+
+  float mtx[16];
+  surface_texture_->UpdateTexImage();
+  surface_texture_->GetTransformMatrix(mtx);
+  CopyCurrentFrameToPictureBuffer(picture_buffer_id, mtx);
+
+  client_->PictureReady(
+      media::Picture(picture_buffer_id, current_bitstream_id_));
+  current_bitstream_id_ = -1;
+}
+
+void AndroidVideoDecodeAccelerator::CopyCurrentFrameToPictureBuffer(

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

single call-site + simple impl == inline into call-site?

[dwkang1, 2013/01/28 14:54:30]

I think it depends on how we will define "simple",  but I don't have strong
objection. Merged with SCSTC().

+    int32 picture_buffer_id, float transfrom_matrix[16]) {
+  PictureMap::const_iterator i = picture_map_.find(picture_buffer_id);
+  if (i == picture_map_.end()) {
+    LOG(ERROR) << "Can't find a PuctureBuffer for " << picture_buffer_id;
+    return;
+  }
+  uint32 picture_buffer_texture_id = i->second.texture_id();
+  texture_copier_->Copy(surface_texture_id_, GL_TEXTURE_EXTERNAL_OES,
+                        transfrom_matrix,
+                        picture_buffer_texture_id, GL_TEXTURE_2D);
+}
+
+void AndroidVideoDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  if (!client_) {
+    return;
+  }
+  pending_bitstream_buffers_.push(bitstream_buffer);
+}
+
+void AndroidVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  DCHECK(picture_map_.empty());
+
+  for (size_t i = 0; i < buffers.size(); ++i) {
+    picture_map_.insert(std::make_pair(buffers[i].id(), buffers[i]));
+    free_picture_ids_.push(buffers[i].id());
+  }

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Add:
if (picture_map_.size() != kNumPictureBuffers) {
  NotifyError(...);
  state = ERROR;
  return;
}
to guard against compromised renderers feeding us garbage, repeated ids, etc.

[dwkang1, 2013/01/28 14:54:30]

Done.

+}
+
+void AndroidVideoDecodeAccelerator::ReusePictureBuffer(
+    int32 picture_buffer_id) {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  free_picture_ids_.push(picture_buffer_id);
+}
+
+void AndroidVideoDecodeAccelerator::Flush() {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  Decode(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0));
+}
+
+void AndroidVideoDecodeAccelerator::ConfigureMediaCodec() {
+  DCHECK(surface_texture_.get());
+  DCHECK(codec_ == H264 || codec_ == VP8);
+
+  std::string mime;
+  if (codec_ == VP8) {
+    mime = "video/x-vnd.on2.vp8";
+  } else if (codec_ == H264) {
+    mime = "video/avc";
+  } else {
+    LOG(ERROR) << "Unsupported codec type " << codec_;
+    NOTREACHED();
+  }
+  media_codec_.reset(new media::MediaCodecBridge(mime));
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  CHECK(env);
+  ScopedJavaLocalRef<jclass> cls(
+      base::android::GetClass(env, "android/view/Surface"));
+  jmethodID constructor = MethodID::Get<MethodID::TYPE_INSTANCE>(
+      env, cls.obj(), "<init>", "(Landroid/graphics/SurfaceTexture;)V");
+  ScopedJavaLocalRef<jobject> j_surface(
+      env, env->NewObject(
+          cls.obj(), constructor,
+          surface_texture_->j_surface_texture().obj()));
+
+  // VDA does not pass the container indicated resolution in the initialization
+  // phase. Here, we set 1080p by default.

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

This is a mistake that is likely to hide bugs, IMO.
Better to set a width/height of 0 and handle the "format change" when dequeuing
outputs.

[dwkang1, 2013/01/28 14:54:30]

Actually, that was my first try, but it lead to Exception from MediaCodec. I
think options for avoiding this ugly hard-coded values is 1. pass the container
indicated resolution to vda. 2. start with 1080p and wait until the real
resolution and start from the beginning. What do you think?

+  media_codec_->ConfigureVideo(
+      mime, 1920, 1080, NULL, 0, NULL, 0, j_surface.obj());
+  content::ReleaseSurface(j_surface.obj());

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

How does this work?  Didn't you just ask MediaCodec to render output to this
surface that you're freeing here?

[dwkang1, 2013/01/28 14:54:30]

If Surface is not registered to the windows manager, like this case, Surface is
just a mean to pass the texture to MediaCodec. other example:
https://cs.corp.google.com/#chrome/src/content/browser/android/surface_textur...

+
+  media_codec_->Start();
+  media_codec_->GetInputBuffers();
+  media_codec_->GetOutputBuffers();
+}
+
+void AndroidVideoDecodeAccelerator::Reset() {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  while(!pending_bitstream_buffers_.empty()) {
+    media::BitstreamBuffer& bitstream_buffer =
+        pending_bitstream_buffers_.front();
+    pending_bitstream_buffers_.pop();
+
+    if (bitstream_buffer.id() != -1) {
+      client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

I don't think the VDA interface requires this (and the other impls don't do it).
So this while loop can be replaced with:
pending_bitstream_buffers_.clear();

[dwkang1, 2013/01/28 14:54:30]

Actually, I referred OVDA code when I wrote this code. In OVDA, on Reset(), it
flushes the ports. According to the openmax spec 1.2 (3.2.2.4 OMX_CommandFlush),
this flushing command will make component return unprocessed buffers to OVDA by
calling EmptyBufferDoneTask() and it will call NEOBB.

+    }
+  }
+  media_codec_->Flush();
+  media_codec_->Stop();
+  media_codec_->Release();

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

Why the Stop/Release/ConfigureMediaCodec dance?

[dwkang1, 2013/01/28 14:54:30]

It's a recommended way to reset MediaCodec. FWIW, I am not a big fan of
MediaCodec.

+  ConfigureMediaCodec();
+  state_ = NO_ERROR;
+
+  if (client_) {
+    client_->NotifyResetDone();

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

For complicated reasons, it's not OK to make client callbacks on the same stack
that the client calls us.  Always PostTask callbacks unless you're guaranteed
not to be in this situation.

[dwkang1, 2013/01/28 14:54:30]

Done.

+  }
+}
+
+void AndroidVideoDecodeAccelerator::Destroy() {
+  LOG_LINE();
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+}

[Ami GONE FROM CHROMIUM, 2013/01/23 01:32:32]

This method needs to 
delete this;
or else you have a leak (look at how this is called from
GpuVideoDecodeAccelerator).

[dwkang1, 2013/01/28 14:54:30]

Done.

+
+}  // namespace content