cc: Changes to use GL API for GpuMemoryBuffers

cc/resources/resource_provider.cc

Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index 35457f70bb16657d25d438ef4d3bd52f8a73ce46..db9b7909e5987821dbf6469f552481a956c8d2e8 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -85,6 +85,7 @@ ResourceProvider::Resource::Resource()
       size(),
       format(0),
       filter(0),
+      image_id(0),
       type(static_cast<ResourceType>(0)) {}
 
 ResourceProvider::Resource::~Resource() {}
@@ -109,6 +110,7 @@ ResourceProvider::Resource::Resource(
       size(size),
       format(format),
       filter(filter),
+      image_id(0),
       type(GLTexture) {}
 
 ResourceProvider::Resource::Resource(
@@ -131,6 +133,7 @@ ResourceProvider::Resource::Resource(
       size(size),
       format(format),
       filter(filter),
+      image_id(0),
       type(Bitmap) {}
 
 ResourceProvider::Child::Child() {}
@@ -308,6 +311,12 @@ void ResourceProvider::DeleteResourceInternal(ResourceMap::iterator it,
   if (style == ForShutdown && resource->exported)
     lost_resource = true;
 
+  if (resource->image_id) {
+    WebGraphicsContext3D* context3d = output_surface_->context3d();
+    DCHECK(context3d);
+    GLC(context3d, context3d->destroyImageCHROMIUM(resource->image_id));
+  }
+
   if (resource->gl_id && !resource->external) {
     WebGraphicsContext3D* context3d = output_surface_->context3d();
     DCHECK(context3d);
@@ -631,7 +640,8 @@ ResourceProvider::ResourceProvider(OutputSurface* output_surface)
       use_texture_usage_hint_(false),
       use_shallow_flush_(false),
       max_texture_size_(0),
-      best_texture_format_(0) {}
+      best_texture_format_(0) {
+}
 
 bool ResourceProvider::Initialize(int highp_threshold_min) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -859,6 +869,7 @@ bool ResourceProvider::TransferResource(WebGraphicsContext3D* context,
   DCHECK(!source->lock_for_read_count);
   DCHECK(!source->external || (source->external && !source->mailbox.IsEmpty()));
   DCHECK(source->allocated);
+  DCHECK(!source->image_id);

[reveman, 2013/05/20 23:16:01]

hm, I think we might need to support this. any specific reason why you're
preventing it?

[kaanb, 2013/05/21 00:52:29]

I'm not sure if this would be relevant for WebView, if so I can open a follow-up
bug to track it. sievers@ what do you think? 

[no sievers, 2013/05/21 19:15:46]

I'd just remove the DCHECK().

WebView would not use delegated rendering, nor have a browser compositor.

Also, I'm not sure that this does not just work. We might not transfer the
GLImage ref to the other client texture through the mailbox (although Antoine's
texture sharing work might address that). But not sure it matters since it won't
be needed after bindTexImage2D().

[piman, 2013/05/21 19:50:39]

It probably doesn't work right now, but should work as soon as
https://codereview.chromium.org/14188053/ lands. I think it would be good to
just remove the DCHECK. In the future, other Image implementation may work
out-of-process, and it'd be good to leverage this if it's available.

[kaanb, 2013/05/21 20:25:41]

Done.

   if (source->exported)
     return false;
   resource->id = id;
@@ -924,6 +935,7 @@ void ResourceProvider::ReleasePixelBuffer(ResourceId id) {
   Resource* resource = &it->second;
   DCHECK(!resource->external);
   DCHECK(!resource->exported);
+  DCHECK(!resource->image_id);

[reveman, 2013/05/20 23:16:01]

technically, there's nothing that stops us from using both pixel buffers and
image, right? I guess it's OK to prevent this for now but please add a DCHECK to
AcquirePixelBuffer too.

[kaanb, 2013/05/21 00:52:29]

Done.

 
   if (resource->gl_id) {
     if (!resource->gl_pixel_buffer_id)
@@ -956,6 +968,7 @@ uint8_t* ResourceProvider::MapPixelBuffer(ResourceId id) {
   Resource* resource = &it->second;
   DCHECK(!resource->external);
   DCHECK(!resource->exported);
+  DCHECK(!resource->image_id);
 
   if (resource->gl_id) {
     WebGraphicsContext3D* context3d = output_surface_->context3d();
@@ -984,6 +997,7 @@ void ResourceProvider::UnmapPixelBuffer(ResourceId id) {
   Resource* resource = &it->second;
   DCHECK(!resource->external);
   DCHECK(!resource->exported);
+  DCHECK(!resource->image_id);
 
   if (resource->gl_id) {
     WebGraphicsContext3D* context3d = output_surface_->context3d();
@@ -992,8 +1006,7 @@ void ResourceProvider::UnmapPixelBuffer(ResourceId id) {
     context3d->bindBuffer(
         GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM,
         resource->gl_pixel_buffer_id);
-    context3d->unmapBufferCHROMIUM(
-        GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM);
+    context3d->unmapBufferCHROMIUM(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM);
     context3d->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0);
   }
 }
@@ -1008,6 +1021,7 @@ void ResourceProvider::SetPixelsFromBuffer(ResourceId id) {
   DCHECK(!resource->external);
   DCHECK(!resource->exported);
   DCHECK(ReadLockFenceHasPassed(resource));
+  DCHECK(!resource->image_id);
   LazyAllocate(resource);
 
   if (resource->gl_id) {
@@ -1073,6 +1087,7 @@ void ResourceProvider::BeginSetPixels(ResourceId id) {
   DCHECK(!resource->pending_set_pixels);
   DCHECK(resource->gl_id || resource->allocated);
   DCHECK(ReadLockFenceHasPassed(resource));
+  DCHECK(!resource->image_id);
 
   bool allocate = !resource->allocated;
   resource->allocated = true;
@@ -1242,4 +1257,114 @@ void ResourceProvider::EnableReadLockFences(ResourceProvider::ResourceId id,
   resource->enable_read_lock_fences = enable;
 }
 
+void ResourceProvider::AcquireImage(ResourceId id) {
+  // TODO(kaanb): Should we delay glGenTextures to this method as well?
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+
+  DCHECK(resource->gl_id);
+  DCHECK(!resource->pixels);
+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK_EQ(0u, resource->image_id);

[reveman, 2013/05/20 23:16:01]

AcquirePixelBuffer can be called twice without an error. I would prefer if
AcquireImage is consistent with that.

[kaanb, 2013/05/21 00:52:29]

I think we'd be leaking the first buffer if we call AcquirePixelBuffer twice,
right? So rather than inheriting that practice we should start with a clean
plate and do the things the right way? 

[reveman, 2013/05/21 02:20:19]

It's perfectly fine to call glBufferData multiple times in AcquirePixelBuffer.
It just redefines the storage.

The current pooling and reuse of resources in the tile manager will require us
to either be allowed to call AcquireImage multiple times or add some code that
track what resources we have acquired images for. Unless we intend to release
and acquire a new image every time we reuse a buffer. I'm assuming not.

[kaanb, 2013/05/21 17:53:48]

I looked at the code and for pixel buffers glBufferData frees the previous
buffer if the bufferData is called successively with the same buffer_id. So I
release the previous image within ResourceProvider to be consistent. createImage
is like createBuffer + bufferData as it generates a new id and allocates the
buffer each time it is called so it is cleaner to free the previous buffer
within ResourceProvider.

+
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  resource->image_id = context3d->createImageCHROMIUM(
+      resource->size.width(), resource->size.height(), GL_RGBA8_OES);
+}
+
+void ResourceProvider::ReleaseImage(ResourceId id) {
+  // TODO(kaanb): CalledOnValidThread() will not be called if we're not in debug
+  // mode is it intentional?
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+
+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK(!resource->pixels);
+  DCHECK(resource->image_id);

[reveman, 2013/05/20 23:16:01]

Same thing here. It's Ok to call ReleasePixelBuffer without first calling
AcquirePixelBuffer.

[kaanb, 2013/05/21 00:52:29]

Again, "double free" is almost always an anti-pattern that causes a plethora of
issues. Given that this is a new code path double free of an image would
probably indicate a bug in some other part of the pipeline. So I'd rather keep
it here.

[reveman, 2013/05/21 02:20:19]

ReleasePixelBuffer handles this properly. Maybe it makes sense to change it.
Either way, I prefer if we're consistent unless there's a good reason why we
shouldn't be.

[kaanb, 2013/05/21 17:53:48]

ReleasePixelBuffer simply returns if pixel_buffer_id is 0, so I decided to
return if the image_id is 0.

+
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  context3d->destroyImageCHROMIUM(resource->image_id);
+  resource->image_id = 0;
+}
+
+uint8_t* ResourceProvider::MapImage(ResourceId id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+

[reveman, 2013/05/20 23:16:01]

It's probably best to have a DCHECK(ReadLockFenceHasPassed(resource)) here.

[kaanb, 2013/05/21 00:52:29]

Done, should we add the same DCHECK for BindImage as well?

[reveman, 2013/05/21 02:20:19]

I don't think so. BindImage doesn't modify the image.

[kaanb, 2013/05/21 17:53:48]

Acknowledged.

+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK(!resource->pixels);
+  DCHECK(resource->image_id);
+
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  return static_cast<uint8_t*>(
+      context3d->mapImageCHROMIUM(resource->image_id, GL_READ_WRITE));
+}
+
+void ResourceProvider::UnmapImage(ResourceId id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+
+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK(!resource->pixels);
+  DCHECK(resource->image_id);
+
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  context3d->unmapImageCHROMIUM(resource->image_id);
+}
+
+void ResourceProvider::BindImage(ResourceId id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+
+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK(!resource->pixels);
+  DCHECK(resource->image_id);
+  DCHECK(resource->gl_id);
+
+  // TODO(kaanb): Do we need ReadLockFenceHasPassed and/or LockForWrite()
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  context3d->bindTexture(GL_TEXTURE_2D, resource->gl_id);
+  context3d->bindTexImage2DCHROMIUM(GL_TEXTURE_2D, resource->image_id);
+}
+
+int ResourceProvider::GetImageStride(ResourceId id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ResourceMap::iterator it = resources_.find(id);
+  CHECK(it != resources_.end());
+  Resource* resource = &it->second;
+
+  DCHECK(!resource->external);
+  DCHECK(!resource->exported);
+  DCHECK(!resource->pixels);
+  DCHECK(resource->image_id);
+
+  WebGraphicsContext3D* context3d = output_surface_->context3d();
+  DCHECK(context3d);
+  int stride = 0;
+  context3d->getImageParameterivCHROMIUM(
+      resource->image_id, GL_IMAGE_ROWBYTES_CHROMIUM, &stride);
+  return stride;
+}
+
+
 }  // namespace cc