Change CHROMIUM_image declarations to support multi planar input.

content/common/gpu/client/command_buffer_proxy_impl.cc

 // Copyright (c) 2012 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/client/command_buffer_proxy_impl.h"
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/shared_memory.h"
 #include "base/stl_util.h"
@@ skipping 329 matching lines @@
   if (last_state_.error != gpu::error::kNoError)
     return;
 
   Send(new GpuCommandBufferMsg_DestroyTransferBuffer(route_id_, id));
 }
 
 gpu::Capabilities CommandBufferProxyImpl::GetCapabilities() {
   return capabilities_;
 }
 
-int32_t CommandBufferProxyImpl::CreateImage(ClientBuffer buffer,
+int32_t CommandBufferProxyImpl::CreateImage(const ClientBuffer* const buffers,
                                             size_t width,
                                             size_t height,
                                             unsigned internalformat) {
   CheckLock();
   if (last_state_.error != gpu::error::kNoError)
     return -1;
 
   int32 new_id = channel_->ReserveImageId();
 
   gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager =
       channel_->gpu_memory_buffer_manager();
+  // TODO(emircan): See http://crbug.com/439520; support passing multiple
+  // buffers when new multi-planar formats are added.
   gfx::GpuMemoryBuffer* gpu_memory_buffer =

[reveman, 2015/03/03 05:30:13]

Please don't stop here. Go as far as you need to fully change this API. That
includes the IPC. The idea is to find out now if the choice of API change makes
sense or if "buffers + num_buffers" or "buffer0, buffer1, buffer2" are better
choices.

[emircan, 2015/03/04 03:03:16]

Sure, I was just trying to keep CL smaller.

I added helper functions to find num_buffers using |internalformat|. However, I
am not passing num_buffers through IPC as the number of args becomes 6, which
isn't declared in the current interface and results in a lot changes. I am
calculating the num_buffers again on the gpu thread.

-      gpu_memory_buffer_manager->GpuMemoryBufferFromClientBuffer(buffer);
+      gpu_memory_buffer_manager->GpuMemoryBufferFromClientBuffer(buffers[0]);
   DCHECK(gpu_memory_buffer);
 
   // This handle is owned by the GPU process and must be passed to it or it
   // will leak. In otherwords, do not early out on error between here and the
   // sending of the CreateImage IPC below.
   bool requires_sync_point = false;
   gfx::GpuMemoryBufferHandle handle =
       channel_->ShareGpuMemoryBufferToGpuProcess(gpu_memory_buffer->GetHandle(),
                                                  &requires_sync_point);
 
@@ skipping 17 matching lines @@
 }
 
 void CommandBufferProxyImpl::DestroyImage(int32 id) {
   CheckLock();
   if (last_state_.error != gpu::error::kNoError)
     return;
 
   Send(new GpuCommandBufferMsg_DestroyImage(route_id_, id));
 }
 
 int32_t CommandBufferProxyImpl::CreateGpuMemoryBufferImage(

[reveman, 2015/03/03 05:30:13]

hm, how do we handle support for multi-plane formats in this function? Do we
need something like gpu::ImageFactory::GetGpuMemoryBufferCountForImageFormat and
a buffer argument to ImageFormatToGpuMemoryBufferFormat?

[emircan, 2015/03/04 03:03:16]

Agreed. In addition to the number of buffers, we need to have a list of formats
if we are going to define them in terms of bits per pixel, such as
GpuMemoryBuffer::X_8, GpuMemoryBuffer::X_2, etc.

  static void ImageFormatToGpuMemoryBufferFormats(
      unsigned internalformat,
      std::vector<gfx::GpuMemoryBuffer::Format>* formats);

I redefined the static function as above, and changed all the references to
accept multi-buffer when possible.

I realize that usage of std::vector may not be efficient as push_back would
reallocate memory. If we define kMaxNumberOfBuffersForAnImage, I can reserve
memory initially. What do you think?

[reveman, 2015/03/04 05:55:59]

How about
static gfx::GpuMemoryBuffer::Format ImageFormatToGpuMemoryBufferFormat(unsigned
internalformat, int buffer)
where the value of |buffer| can be 0-2 in the case of GL_RGB_YUV420... 

[emircan, 2015/03/04 23:31:50]

In case of GL_RGB_YUV420, I thought we want to return a list of
{GpuMemoryBuffer::X_8, GpuMemoryBuffer::X_2, GpuMemoryBuffer::X_2}. As the
planes would have different number of bits per pixels, size and stride functions
would change accordingly. GL_RGB_YUV422 would be {GpuMemoryBuffer::X_8,
GpuMemoryBuffer::X_4, GpuMemoryBuffer::X_4}, and so on. Do we want to create 3
buffers of GpuMemoryBuffer::X_8 instead?

Also, I can't understand the use of |buffer| you are suggesting. Does it
indicate the number of buffers more than 1 with [0-2]? 

[reveman, 2015/03/05 19:35:31]

The idea is that it would be the format of |buffer|. Where |buffer| is the index
used for the IPC. This way you're not forcing the use of a vector unless
necessary.

[emircan, 2015/03/09 21:07:22]

I now understand your suggestion, sorry for the confusion. I changed it to:

  static gfx::GpuMemoryBuffer::Format ImageFormatToGpuMemoryBufferFormat(
      unsigned internalformat,
      int buffer_index);

So later on, how are we planning to differentiate single-buffer and multi-buffer
on |buffer_index|?

#1, we can add a boolean:

  static gfx::GpuMemoryBuffer::Format ImageFormatToGpuMemoryBufferFormat(
      unsigned internalformat,
      unsigned int buffer_index,
      bool is_single_buffer);

ImageFormatToGpuMemoryBufferFormat(GL_RGB, 0, false) returns
gfx::GpuMemoryBuffer::R_8
ImageFormatToGpuMemoryBufferFormat(GL_RGB, 0, true) returns
gfx::GpuMemoryBuffer::RGBA_8888

#2, we can define a new GLEnum
ImageFormatToGpuMemoryBufferFormat(GL_RGB_MP, 0) returns
gfx::GpuMemoryBuffer::R_8
ImageFormatToGpuMemoryBufferFormat(GL_RGB, 0) returns
gfx::GpuMemoryBuffer::RGBA_8888

What would you prefer?

[reveman, 2015/03/10 04:27:20]

Multi buffer formats would need a different internalformat. I don't see a good
reason for adding a multi plane RGB format that is not YUV though.

[emircan, 2015/03/11 18:36:35]

Sure. The current function should be sufficient then.

     size_t width,
     size_t height,
     unsigned internalformat,
     unsigned usage) {
   CheckLock();
   scoped_ptr<gfx::GpuMemoryBuffer> buffer(
       channel_->gpu_memory_buffer_manager()->AllocateGpuMemoryBuffer(
           gfx::Size(width, height),
           gpu::ImageFactory::ImageFormatToGpuMemoryBufferFormat(internalformat),
           gpu::ImageFactory::ImageUsageToGpuMemoryBufferUsage(usage)));
   if (!buffer)
     return -1;
 
-  return CreateImage(buffer->AsClientBuffer(), width, height, internalformat);
+  const ClientBuffer& client_buffer = buffer->AsClientBuffer();
+  return CreateImage(const_cast<const ClientBuffer* const>(&client_buffer),
+                     width, height, internalformat);
 }
 
 int CommandBufferProxyImpl::GetRouteID() const {
   return route_id_;
 }
 
 uint32 CommandBufferProxyImpl::CreateStreamTexture(uint32 texture_id) {
   CheckLock();
   if (last_state_.error != gpu::error::kNoError)
     return 0;
@@ skipping 176 matching lines @@
   }
 }
 
 void CommandBufferProxyImpl::OnUpdateVSyncParameters(base::TimeTicks timebase,
                                                      base::TimeDelta interval) {
   if (!update_vsync_parameters_completion_callback_.is_null())
     update_vsync_parameters_completion_callback_.Run(timebase, interval);
 }
 
 }  // namespace content