Add a GpuMemoryBuffer pool that creates hardware backed VideoFrames.

media/video/gpu_memory_buffer_video_frame_pool.cc

+// Copyright (c) 2015 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 "media/video/gpu_memory_buffer_video_frame_pool.h"
+
+#include <GLES2/gl2.h>
+#include <GLES2/gl2ext.h>
+
+#include <deque>
+
+#include "base/bind.h"
+#include "base/containers/stack_container.h"
+#include "base/location.h"
+#include "base/memory/linked_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/trace_event/trace_event.h"
+#include "gpu/command_buffer/client/gles2_interface.h"
+#include "media/renderers/gpu_video_accelerator_factories.h"
+
+namespace media {
+
+// Implementation of a pool of GpuMemoryBuffers used to back VideoFrames.
+class GpuMemoryBufferVideoFramePool::PoolImpl
+    : public base::RefCountedThreadSafe<
+          GpuMemoryBufferVideoFramePool::PoolImpl> {
+ public:
+  // 'task_runner' is associated to the thread where the context of
+  // GLES2Interface returned by 'gpu_factories' lives.
+  // 'gpu_factories' is an interface to GPU related operation and can be
+  // null.
+  PoolImpl(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+           const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories)
+      : task_runner_(task_runner), gpu_factories_(gpu_factories) {}
+
+  // Takes a software VideoFrame and returns a VideoFrame backed by native
+  // textures if possible.
+  // The data contained in video_frame is copied into the returned frame.
+  const scoped_refptr<VideoFrame> CreateHardwareFrame(
+      scoped_refptr<VideoFrame> video_frame);
+
+ private:
+  friend class base::RefCountedThreadSafe<
+      GpuMemoryBufferVideoFramePool::PoolImpl>;
+  ~PoolImpl();
+
+  // Resource to represent a plane.
+  struct PlaneResource {
+    gfx::Size size;
+    scoped_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer;
+    unsigned texture_id = 0u;

[DaleCurtis, 2015/05/12 17:23:20]

Use the type size, uint32_t or whatever.

[Daniele Castagna, 2015/05/12 21:21:30]

In cc unsigned is used for resouces/textures/images ids. Do you prefer a sized
type?

[DaleCurtis, 2015/05/13 02:52:14]

We don't use 'unsigned' in media/ code, but if this matches the cc code that's
okay with me.

[Daniele Castagna, 2015/05/13 04:29:45]

It's your call. If you prefer a signed type I'll change it. If you don't say
anything I'm going to leave it like this.

[DaleCurtis, 2015/05/14 00:58:57]

I didn't mean use a signed type, I just meant give it an explicit size, like
size_t or uint32_t, etc.  If there are equivalent values for this code somewhere
else using unsigned though, it's better to be consistent here (also in case this
gets serialized via IPC).

[Daniele Castagna, 2015/05/14 17:30:25]

Sorry, I meant sized not signed. Leaving it unsigned since that's what we have
in cc.

+    unsigned image_id = 0u;
+    gpu::Mailbox mailbox;
+  };
+
+  // All the resources needed to compose a frame.
+  struct FrameResources {
+    VideoFrame::Format format;
+    gfx::Size size;
+    PlaneResource plane_resources[VideoFrame::kMaxPlanes];
+  };
+
+  // Return true if 'resources' can be used to represent a frame for
+  // specific 'format' and 'size'.
+  static bool IsFrameResourcesCompatible(const FrameResources* resources,
+                                         const gfx::Size& size,
+                                         VideoFrame::Format format) {
+    return size == resources->size && format == resources->format;
+  }
+
+  // Get the resources needed for a frame out of the pool, or create them if
+  // necessary.
+  // This also drops the LRU resources that can't be reuse for this frame.
+  scoped_ptr<FrameResources> GetOrCreateFrameResources(const gfx::Size& size,
+                                                       VideoFrame::Format);

[reveman, 2015/05/12 03:20:25]

nit: missing param name

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+
+  // Callback called when a VideoFrame generated with GetFrameResources is no
+  // longer referenced.
+  // This could be called by any thread.
+  void MailboxHoldersReleased(scoped_ptr<FrameResources> frame_resources,
+                              uint32 sync_point);
+
+  // Return frame resources to the pool. This has to be called on the thread
+  // where 'task_runner' is current.
+  void ReturnFrameResources(scoped_ptr<FrameResources> frame_resources);
+
+  // Delete resources. This has to be called on the thread where 'task_runner'
+  // is current.
+  static void DeleteFrameResources(
+      scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_,

[reveman, 2015/05/12 03:20:25]

nit: remove '_' suffix. and again, dunno about media/ but we'd typically use a
raw pointer here in chromium

[DaleCurtis, 2015/05/12 17:23:20]

const&

[Daniele Castagna, 2015/05/12 21:21:29]

Done.

[Daniele Castagna, 2015/05/12 21:21:30]

_ removed.

This is used as a callback, the scoped_refptr here make it so the callback keeps
the gpu_factories alive.

+      scoped_ptr<FrameResources> frame_resources);
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories_;
+
+  // Pool of resources, the pool behaves like a LIFO.
+  std::deque<linked_ptr<FrameResources>> resources_pool_;

[DaleCurtis, 2015/05/12 17:23:20]

Can you avoid using linked_ptr here? It makes things harder to reason about. If
not it's fine, but I'm always sad to see it :)

[Daniele Castagna, 2015/05/12 21:21:29]

I was using a ScopedVector but they suggested me to use linked_ptr so that I can
switch to a deque and make it clearer how we're using the pool.

I'll follow up with another iteration where the pool keeps the ownership if
FrameResources all the time. That should make it easier to avoid the linked_ptr.

+
+  unsigned texture_target_ = GL_TEXTURE_2D;
+  DISALLOW_COPY_AND_ASSIGN(PoolImpl);
+};
+
+namespace {
+
+// Copy a buffer info a GpuMemoryBuffer.
+// bytes_per_row is expected to be less or equal than the strides of the two
+// buffers.
+void CopyPlaneToGpuMemoryBuffer(int rows,
+                                int bytes_per_row,
+                                const uint8* source,
+                                const int source_stride,

[DaleCurtis, 2015/05/12 17:23:20]

Remove const for primitive types that aren't pointers.

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+                                gfx::GpuMemoryBuffer* buffer) {
+  TRACE_EVENT2("media", "CopyPlaneToGpuMemoryBuffer", "bytes_per_row",
+               bytes_per_row, "rows", rows);
+
+  DCHECK(buffer);
+  DCHECK(source);
+  void* data = nullptr;
+  CHECK(buffer->Map(&data));

[DaleCurtis, 2015/05/12 17:23:20]

Seems we are more likely to OOM GPU buffers, should the frame be gracefully
returned instead here?

[Daniele Castagna, 2015/05/12 21:21:29]

AFAIU Map doesn't fail with the current GpuMemoryBuffer implementations we have
right now.
The memory buffer allocations can fail, we can return the frame there if that
happens.

reveman: what do you think about this?

+  uint8* mapped_buffer = static_cast<uint8*>(data);
+  int dest_stride = 0;
+  buffer->GetStride(&dest_stride);
+  DCHECK_NE(dest_stride, 0);
+  DCHECK_LE(bytes_per_row, static_cast<int>(dest_stride));

[reveman, 2015/05/12 03:20:25]

what it dest_stride is negative?

[Daniele Castagna, 2015/05/12 21:21:29]

Changed the DCHECK.

+  DCHECK_LE(bytes_per_row, source_stride);

[reveman, 2015/05/12 03:20:25]

can source_stride be negative?

[Daniele Castagna, 2015/05/12 21:21:30]

It can't, software videoframes always have positive strides.

+  for (int row = 0; row < rows; ++row) {
+    memcpy(mapped_buffer + dest_stride * row, source + source_stride * row,
+           bytes_per_row);
+  }
+  buffer->Unmap();
+}
+
+}  // unnamed namespace
+
+// Creates a VideoFrame backed by native textures starting from a software

[DaleCurtis, 2015/05/12 17:23:20]

Comments should go with the function definition not the implementation.

[Daniele Castagna, 2015/05/12 21:21:30]

I'm OK moving it, but
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com...
seems to disagree with your statement.

[DaleCurtis, 2015/05/13 02:52:14]

That seems to agree with me? Can you highlight the text that disagrees, I'm not
seeing it :)

[Daniele Castagna, 2015/05/13 04:29:45]

"Declaration comments describe use of the function; comments at the definition
of a function describe operation."

Here we are defining a method, so I thought I'd be a little bit more verbose
about the internals of the method.

You said: "Comments should go with the function definition not the
implementation.". Assuming you meant declaration instead of definition, I think
this comment makes sense on top of the implementation since declaration comment
should describe only the use of the function.

[DaleCurtis, 2015/05/14 00:58:57]

Hmm, I see; this is atypical for media/ style and I'd prefer if a meaty comment
of this type was interleaved into the code below for better readability than a
text blob here at the top.

[Daniele Castagna, 2015/05/14 17:30:25]

OK, better to be consistent with the rest of media.

+// VideoFrame.
+// The data contained in video_frame is copied into the returned VideoFrame.
+// This method first tries to reuse resources and drops the incompatible ones.
+// If resources are not available, they will be created.
+// The returned VideoFrame will call
+// GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased once the
+// VideoFrame is not referenced anymore and the resources will be put back
+// in the pool.
+const scoped_refptr<VideoFrame>
+GpuMemoryBufferVideoFramePool::PoolImpl::CreateHardwareFrame(
+    scoped_refptr<VideoFrame> video_frame) {
+  gpu::gles2::GLES2Interface* gles2 =
+      gpu_factories_ ? gpu_factories_->GetGLES2Interface() : nullptr;

[reveman, 2015/05/12 03:20:25]

Do both of these need to be allowed to be null? Maybe improve readability a bit
with:

if (!gpu_factories_)
  return video_frame;

gpu::gles2::GLES2Interface* gles2 = gpu_factories_->GetGLES2Interface();
if (!gles2)
  return video_frame;
  

[Daniele Castagna, 2015/05/12 21:21:30]

gpu_factories_ can be null, for example when #disable-accelerated-video-decode
is true.
gles2 can be null when a context can't be created (look at
RendererGpuVideoAcceleratorFactories::GetContext3d)

Changed it as you suggested.

+  if (!gles2)
+    return video_frame;
+
+  VideoFrame::Format format = video_frame->format();
+  size_t planes = VideoFrame::NumPlanes(format);
+  DCHECK(video_frame->visible_rect().origin().IsOrigin());
+  gfx::Size size = video_frame->visible_rect().size();
+  gpu::MailboxHolder mailbox_holders[VideoFrame::kMaxPlanes];
+  scoped_ptr<FrameResources> frame_resources =
+      GetOrCreateFrameResources(size, format);
+
+  for (size_t i = 0; i < planes; ++i) {
+    PlaneResource& plane_resource = frame_resources->plane_resources[i];
+    CopyPlaneToGpuMemoryBuffer(VideoFrame::Rows(i, format, size.height()),
+                               VideoFrame::RowBytes(i, format, size.width()),
+                               video_frame->data(i), video_frame->stride(i),
+                               plane_resource.gpu_memory_buffer.get());
+
+    // Bind the texture and create or rebind the image.
+    gles2->BindTexture(texture_target_, plane_resource.texture_id);
+    if (plane_resource.gpu_memory_buffer && !plane_resource.image_id) {
+      const size_t width = VideoFrame::Columns(i, format, size.width());
+      const size_t height = VideoFrame::Rows(i, format, size.height());
+      const gfx::Size size(width, height);

[reveman, 2015/05/12 03:20:25]

creating this seems useless and just confusing as it shadows |size| above.

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+      plane_resource.image_id = gles2->CreateImageCHROMIUM(
+          plane_resource.gpu_memory_buffer->AsClientBuffer(), size.width(),
+          size.height(), GL_R8_EXT);

[reveman, 2015/05/12 03:20:25]

Do we need to check that GL_R8_EXT is supported?

[Daniele Castagna, 2015/05/12 21:21:30]

Yes we do. I can follow up with another cl since this is not a real issue until
we enable the pool.

+    } else {
+      gles2->ReleaseTexImage2DCHROMIUM(texture_target_,
+                                       plane_resource.image_id);
+    }
+    gles2->BindTexImage2DCHROMIUM(texture_target_, plane_resource.image_id);
+    mailbox_holders[i] =
+        gpu::MailboxHolder(plane_resource.mailbox, texture_target_, 0);
+  }
+
+  unsigned sync_point = gles2->InsertSyncPointCHROMIUM();
+  for (size_t i = 0; i < planes; ++i) {
+    mailbox_holders[i].sync_point = sync_point;
+  }
+
+  return VideoFrame::WrapYUV420NativeTextures(
+      mailbox_holders[VideoFrame::kYPlane],
+      mailbox_holders[VideoFrame::kUPlane],
+      mailbox_holders[VideoFrame::kVPlane],
+      base::Bind(&PoolImpl::MailboxHoldersReleased, this,
+                 base::Passed(&frame_resources)),

[reveman, 2015/05/12 03:20:25]

Not sure passing frame resources containing GMBs and context3d resources in a
callback is a good idea. I understand that our lost context logic in media/ is
not the best but I don't think we should rely on that here. You need to be able
to delete these resources in a lost context situation and that's not possible
when the ownership is passed to a callback. Even if we ignore the lost context
situation, proper shutdown requires use to delete these resources before the
GMB-manager and context3d. What if the media/ thread is torn down before the
compositor thread but the compositor thread has a reference to this callback?

I think it would be better to always keep ownership of these resources here in
the pool and remove the ref-counting.

[Daniele Castagna, 2015/05/12 21:21:30]

I'll follow up with another iteration on this CL keeping FrameResource
ownerships inside the pool all the time.

We'll still need the ref-counting since we need to wait for all the VideoFrame
sent to the compositor to come back before we can destroy the pool.

+      size, video_frame->visible_rect(), video_frame->natural_size(),
+      video_frame->timestamp(), video_frame->allow_overlay());
+}
+
+// Destroy all the resources posting one task per FrameResources
+// to the 'task_runner_'.
+GpuMemoryBufferVideoFramePool::PoolImpl::~PoolImpl() {
+  // Delete all the resources on the media thread.
+  auto resources_it = resources_pool_.begin();

[reveman, 2015/05/12 03:20:25]

?

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+  while (!resources_pool_.empty()) {
+    auto frame_resources_linked_ptr = resources_pool_.front();

[reveman, 2015/05/12 03:20:25]

scoped_ptr<FrameResources> frame_resources =
make_scoped_ptr(resources_pool_.front().release())?

or see comment below

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+    resources_pool_.pop_front();
+    scoped_ptr<FrameResources> frame_resources(
+        frame_resources_linked_ptr.release());
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&PoolImpl::DeleteFrameResources, gpu_factories_,
+                              base::Passed(&frame_resources)));

[reveman, 2015/05/12 03:20:25]

pass frame_resource as linked_ptr and get rid of the scoped_ptr mess..

[Daniele Castagna, 2015/05/12 21:21:30]

Done as above. I preferred to use scoped_ptr as soon as possible.

+  }
+}
+
+// Tries to find the resources in the pool or create them.
+// Incompatible resources will be dropped.
+scoped_ptr<GpuMemoryBufferVideoFramePool::PoolImpl::FrameResources>
+GpuMemoryBufferVideoFramePool::PoolImpl::GetOrCreateFrameResources(
+    const gfx::Size& size,
+    VideoFrame::Format format) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  while (!resources_pool_.empty()) {
+    auto frame_resources_linked_ptr = resources_pool_.front();

[reveman, 2015/05/12 03:20:25]

make consistent with above code

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+    resources_pool_.pop_front();
+    scoped_ptr<FrameResources> frame_resources(
+        frame_resources_linked_ptr.release());
+    if (IsFrameResourcesCompatible(frame_resources.get(), size, format)) {
+      return frame_resources.Pass();
+    } else {
+      DeleteFrameResources(gpu_factories_, frame_resources.Pass());
+    }
+  }
+
+  // Create the resources.
+  gpu::gles2::GLES2Interface* gles2 = gpu_factories_->GetGLES2Interface();
+  DCHECK(gles2);
+  gles2->ActiveTexture(GL_TEXTURE0);
+  size_t planes = VideoFrame::NumPlanes(format);
+  auto frame_resources = make_scoped_ptr(new FrameResources());
+  frame_resources->format = format;
+  frame_resources->size = size;
+  for (size_t i = 0; i < planes; ++i) {
+    PlaneResource& plane_resource = frame_resources->plane_resources[i];
+    const size_t width = VideoFrame::Columns(i, format, size.width());
+    const size_t height = VideoFrame::Rows(i, format, size.height());
+    const gfx::Size plane_size(width, height);
+    plane_resource.gpu_memory_buffer = gpu_factories_->AllocateGpuMemoryBuffer(
+        plane_size, gfx::GpuMemoryBuffer::R_8, gfx::GpuMemoryBuffer::MAP);
+
+    gles2->GenTextures(1, &plane_resource.texture_id);
+    gles2->BindTexture(texture_target_, plane_resource.texture_id);
+    gles2->TexParameteri(texture_target_, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+    gles2->TexParameteri(texture_target_, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+    gles2->TexParameteri(texture_target_, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+    gles2->TexParameteri(texture_target_, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+    gles2->GenMailboxCHROMIUM(plane_resource.mailbox.name);
+    gles2->ProduceTextureCHROMIUM(texture_target_, plane_resource.mailbox.name);
+  }
+  return frame_resources.Pass();
+}
+
+// static
+void GpuMemoryBufferVideoFramePool::PoolImpl::DeleteFrameResources(
+    scoped_refptr<GpuVideoAcceleratorFactories> gpu_factories,
+    scoped_ptr<FrameResources> frame_resources) {
+  DCHECK(frame_resources);

[reveman, 2015/05/12 03:20:24]

nit: why check frame_resources here? it seems a bit overkill to have a private
method where first thing we do is check that one of the params is non-null. if
there's a chance that this can be null, then I'd move that check closer to the
source (e.g. when you construct the callback) as it's not going be be nearly as
useful here.

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+  gpu::gles2::GLES2Interface* gles2 = gpu_factories->GetGLES2Interface();
+  DCHECK(gles2);
+
+  for (size_t i = 0; i < arraysize(frame_resources->plane_resources); ++i) {

[reveman, 2015/05/12 03:20:25]

for (PlaneResource& plane_resource : frame_resources->plane_resources) ?

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+    PlaneResource& plane_resource = frame_resources->plane_resources[i];
+    if (plane_resource.image_id) {
+      gles2->DestroyImageCHROMIUM(plane_resource.image_id);
+      plane_resource.image_id = 0;
+    }
+    if (plane_resource.texture_id) {
+      gles2->DeleteTextures(1, &plane_resource.texture_id);
+      plane_resource.texture_id = 0;
+    }
+    plane_resource.gpu_memory_buffer.reset();
+  }
+}
+
+// Called when a VideoFrame is no longer references.
+void GpuMemoryBufferVideoFramePool::PoolImpl::MailboxHoldersReleased(
+    scoped_ptr<FrameResources> frame_resources,
+    uint32 sync_point) {
+  // Return the resource on the media thread.
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&PoolImpl::ReturnFrameResources, this,
+                                    base::Passed(&frame_resources)));
+}
+
+// Put back the resoruces in the pool.
+void GpuMemoryBufferVideoFramePool::PoolImpl::ReturnFrameResources(
+    scoped_ptr<FrameResources> frame_resources) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  resources_pool_.push_back(
+      linked_ptr<FrameResources>(frame_resources.release()));

[reveman, 2015/05/12 03:20:24]

make_linked_ptr?

[Daniele Castagna, 2015/05/12 21:21:30]

Done.

+}
+
+GpuMemoryBufferVideoFramePool::GpuMemoryBufferVideoFramePool(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories)
+    : pool_impl_(new PoolImpl(task_runner, gpu_factories)) {
+}
+
+GpuMemoryBufferVideoFramePool::~GpuMemoryBufferVideoFramePool() {
+}
+
+const scoped_refptr<VideoFrame>
+GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame(
+    scoped_refptr<VideoFrame> video_frame) {
+  switch (video_frame->format()) {
+    // Supported cases.
+    case VideoFrame::YV12:
+    case VideoFrame::I420:
+      return pool_impl_->CreateHardwareFrame(video_frame);
+    // Unsupported cases.
+    case media::VideoFrame::YV12A:
+    case media::VideoFrame::YV16:
+    case media::VideoFrame::YV12J:
+    case media::VideoFrame::YV12HD:
+    case media::VideoFrame::YV24:
+#if defined(VIDEO_HOLE)
+    case media::VideoFrame::HOLE:
+#endif  // defined(VIDEO_HOLE)
+    case media::VideoFrame::ARGB:
+      return video_frame;
+
+    // Unacceptable inputs.
+    case media::VideoFrame::NATIVE_TEXTURE:

[DaleCurtis, 2015/05/12 17:23:20]

I don't think this should have a NOTREACHED(), it should gracefully return the
frame no?

[Daniele Castagna, 2015/05/12 21:21:29]

These formats are not supported by VideoResourceUpdater::VerifyFrame and it
would fail there.

[DaleCurtis, 2015/05/14 00:58:57]

What I mean is this method is definitely going to be called with these type of
frames since VRI is always calling it, you need to not spam the logs when this
happens :)

+    case media::VideoFrame::UNKNOWN:
+    case media::VideoFrame::NV12:
+      NOTREACHED();
+  }
+  NOTREACHED();
+  return scoped_refptr<VideoFrame>();
+}
+
+}  // namespace media