Avoid planar YUV resources when one component EGL images are not supported

cc/resources/video_resource_updater.cc

 // Copyright 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 "cc/resources/video_resource_updater.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 155 matching lines @@
     : context_provider_(context_provider),
       resource_provider_(resource_provider) {
 }
 
 VideoResourceUpdater::~VideoResourceUpdater() {
   for (const PlaneResource& plane_resource : all_resources_)
     resource_provider_->DeleteResource(plane_resource.resource_id());
 }
 
 VideoResourceUpdater::ResourceList::iterator
+VideoResourceUpdater::RecycleOrAllocateResource(
+    const gfx::Size& resource_size,
+    ResourceFormat resource_format,
+    const gfx::ColorSpace& color_space,
+    bool software_resource,
+    bool immutable_hint,
+    int unique_id,
+    int plane_index) {
+  for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) {
+    if (it->Matches(unique_id, plane_index)) {
+      DCHECK(it->resource_size() == resource_size);
+      DCHECK(it->resource_format() == resource_format);
+      DCHECK(it->mailbox().IsZero() == software_resource);
+      return it;
+    }
+  }
+  for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) {
+    if (it->resource_size() == resource_size &&
+        it->resource_format() == resource_format &&
+        it->mailbox().IsZero() == software_resource && !it->has_refs()) {
+      if (software_resource) {
+        // This extra check is needed because resources backed by SharedMemory
+        // are not ref-counted, unlike mailboxes. Full discussion in
+        // codereview.chromium.org/145273021.
+        if (!resource_provider_->InUseByConsumer(it->resource_id()))
+          return it;
+      } else {
+        if (immutable_hint ==
+            (resource_provider_->GetTextureHint(it->resource_id()) ==

[danakj, 2016/08/26 18:24:23]

doesn't this need to match the immutable hint? In the callsite where the hint is
false, it expects the resource to not be immutable currently right?

[Tobias Sargeant, 2016/08/30 11:31:06]

This should only return resources if they have similar immutability to what
would otherwise be allocated. It seems like it would be ok to return a mutable
texture if an immutable texture is asked for, although I presume that
immutability is a performance optimization.

+             ResourceProvider::TEXTURE_HINT_IMMUTABLE))
+          return it;
+      }
+    }
+  }
+
+  // Otherwise allocate a new resource.
+  return AllocateResource(resource_size, resource_format, color_space,
+                          !software_resource, immutable_hint);
+}
+
+VideoResourceUpdater::ResourceList::iterator
 VideoResourceUpdater::AllocateResource(const gfx::Size& plane_size,
                                        ResourceFormat format,
                                        const gfx::ColorSpace& color_space,
                                        bool has_mailbox,
                                        bool immutable_hint) {
   // TODO(danakj): Abstract out hw/sw resource create/delete from
   // ResourceProvider and stop using ResourceProvider in this class.
   const ResourceId resource_id = resource_provider_->CreateResource(
       plane_size, immutable_hint ? ResourceProvider::TEXTURE_HINT_IMMUTABLE
                                  : ResourceProvider::TEXTURE_HINT_DEFAULT,
@@ skipping 105 matching lines @@
   if (!media::IsYuvPlanar(input_frame_format)) {
     NOTREACHED() << media::VideoPixelFormatToString(input_frame_format);
     return VideoFrameExternalResources();
   }
 
   const bool software_compositor = context_provider_ == NULL;
 
   ResourceFormat output_resource_format =
       resource_provider_->YuvResourceFormat(bits_per_channel);
 
+  // If GPU compositing is enabled, but the output resource format
+  // returned by the resource provider is RGBA_8888, then a GPU driver
+  // bug workaround requires that YUV frames must be converted to RGB
+  // before texture upload.
+  bool texture_needs_rgb_conversion =
+      !software_compositor &&
+      output_resource_format == ResourceFormat::RGBA_8888;
   size_t output_plane_count = media::VideoFrame::NumPlanes(input_frame_format);
 
   // TODO(skaslev): If we're in software compositing mode, we do the YUV -> RGB
   // conversion here. That involves an extra copy of each frame to a bitmap.
   // Obviously, this is suboptimal and should be addressed once ubercompositor
   // starts shaping up.
-  if (software_compositor) {
+  if (software_compositor || texture_needs_rgb_conversion) {
     output_resource_format = kRGBResourceFormat;
     output_plane_count = 1;
+    bits_per_channel = 8;
   }
 
   // Drop recycled resources that are the wrong format.
   for (auto it = all_resources_.begin(); it != all_resources_.end();) {
     if (!it->has_refs() && it->resource_format() != output_resource_format)
       DeleteResource(it++);
     else
       ++it;
   }
 
   const int max_resource_size = resource_provider_->max_texture_size();
   std::vector<ResourceList::iterator> plane_resources;
   for (size_t i = 0; i < output_plane_count; ++i) {
     gfx::Size output_plane_resource_size =
         SoftwarePlaneDimension(video_frame.get(), software_compositor, i);
     if (output_plane_resource_size.IsEmpty() ||
         output_plane_resource_size.width() > max_resource_size ||
         output_plane_resource_size.height() > max_resource_size) {
       break;
     }
 
-    // Try recycle a previously-allocated resource.
-    ResourceList::iterator resource_it = all_resources_.end();
-    for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) {
-      if (it->resource_size() == output_plane_resource_size &&
-          it->resource_format() == output_resource_format) {
-        if (it->Matches(video_frame->unique_id(), i)) {
-          // Bingo, we found a resource that already contains the data we are
-          // planning to put in it. It's safe to reuse it even if
-          // resource_provider_ holds some references to it, because those
-          // references are read-only.
-          resource_it = it;
-          break;
-        }
+    const bool is_immutable = true;
+    ResourceList::iterator resource_it = RecycleOrAllocateResource(
+        output_plane_resource_size, output_resource_format,
+        video_frame->ColorSpace(), software_compositor, is_immutable,
+        video_frame->unique_id(), i);
 
-        // This extra check is needed because resources backed by SharedMemory
-        // are not ref-counted, unlike mailboxes. Full discussion in
-        // codereview.chromium.org/145273021.
-        const bool in_use =
-            software_compositor &&
-            resource_provider_->InUseByConsumer(it->resource_id());
-        if (!it->has_refs() && !in_use) {
-          // We found a resource with the correct size that we can overwrite.
-          resource_it = it;
-        }
-      }
-    }
-
-    // Check if we need to allocate a new resource.
-    if (resource_it == all_resources_.end()) {
-      const bool is_immutable = true;
-      resource_it = AllocateResource(
-          output_plane_resource_size, output_resource_format,
-          video_frame->ColorSpace(), !software_compositor, is_immutable);
-    }
     if (resource_it == all_resources_.end())
       break;
 
     resource_it->add_ref();
     plane_resources.push_back(resource_it);
   }
 
   if (plane_resources.size() != output_plane_count) {
     // Allocation failed, nothing will be returned so restore reference counts.
     for (ResourceList::iterator resource_it : plane_resources)
       resource_it->remove_ref();
     return VideoFrameExternalResources();
   }
 
   VideoFrameExternalResources external_resources;
 
   external_resources.bits_per_channel = bits_per_channel;
 
-  if (software_compositor) {
+  if (software_compositor || texture_needs_rgb_conversion) {
     DCHECK_EQ(plane_resources.size(), 1u);
     PlaneResource& plane_resource = *plane_resources[0];
     DCHECK_EQ(plane_resource.resource_format(), kRGBResourceFormat);
-    DCHECK(plane_resource.mailbox().IsZero());
+    DCHECK(software_compositor == plane_resource.mailbox().IsZero());

[danakj, 2016/08/26 18:24:23]

DCHECK_EQ

[Tobias Sargeant, 2016/08/30 11:31:06]

Done.

 
     if (!plane_resource.Matches(video_frame->unique_id(), 0)) {
       // We need to transfer data from |video_frame| to the plane resource.
-      if (!video_renderer_)
-        video_renderer_.reset(new media::SkCanvasVideoRenderer);
+      if (software_compositor) {
+        if (!video_renderer_)
+          video_renderer_.reset(new media::SkCanvasVideoRenderer);
 
-      ResourceProvider::ScopedWriteLockSoftware lock(
-          resource_provider_, plane_resource.resource_id());
-      SkCanvas canvas(lock.sk_bitmap());
-      // This is software path, so canvas and video_frame are always backed
-      // by software.
-      video_renderer_->Copy(video_frame, &canvas, media::Context3D());
+        ResourceProvider::ScopedWriteLockSoftware lock(
+            resource_provider_, plane_resource.resource_id());
+        SkCanvas canvas(lock.sk_bitmap());
+        // This is software path, so canvas and video_frame are always backed
+        // by software.
+        video_renderer_->Copy(video_frame, &canvas, media::Context3D());
+      } else {
+        size_t bytes_per_row = ResourceUtil::CheckedWidthInBytes<size_t>(
+            video_frame->coded_size().width(), ResourceFormat::RGBA_8888);
+        size_t needed_size = bytes_per_row * video_frame->coded_size().height();
+        if (upload_pixels_.size() < needed_size)
+          upload_pixels_.resize(needed_size);
+
+        media::SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
+            video_frame.get(), &upload_pixels_[0], bytes_per_row);
+
+        resource_provider_->CopyToResource(plane_resource.resource_id(),
+                                           &upload_pixels_[0],
+                                           plane_resource.resource_size());
+      }
       plane_resource.SetUniqueId(video_frame->unique_id(), 0);
     }
 
-    external_resources.software_resources.push_back(
-        plane_resource.resource_id());
-    external_resources.software_release_callback =
-        base::Bind(&RecycleResource, AsWeakPtr(), plane_resource.resource_id());
-    external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE;
+    if (software_compositor) {
+      external_resources.software_resources.push_back(
+          plane_resource.resource_id());
+      external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE;
+    } else {
+      TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(),

[danakj, 2016/08/26 18:24:23]

why no SyncToken?

[Tobias Sargeant, 2016/08/30 11:31:05]

This is identical to the mailbox construction on l567. Is there a reason why
they should be different?

[danakj, 2016/09/02 22:58:15]

Oh, I guess this class is relying on it sharing the context provider with the
compositor, okay. Would you mind saying this in a comment (at both places
maybe?), because I forgot even.

[Tobias Sargeant, 2016/09/12 14:25:25]

I've added the comment. Given our discussion, it looks like I could also remove
the sync token created in CopyPlaneTexture given that the copy happens on the
compositor context. Am I correct?

+                             resource_provider_->GetResourceTextureTarget(
+                                 plane_resource.resource_id()));
+      mailbox.set_color_space(video_frame->ColorSpace());
+      external_resources.mailboxes.push_back(mailbox);
+      external_resources.type = VideoFrameExternalResources::RGBA_RESOURCE;
+    }
+    external_resources.release_callbacks.push_back(base::Bind(
+        &RecycleResource, AsWeakPtr(), plane_resource.resource_id()));
     return external_resources;
   }
 
   for (size_t i = 0; i < plane_resources.size(); ++i) {
     PlaneResource& plane_resource = *plane_resources[i];
     // Update each plane's resource id with its content.
     DCHECK_EQ(plane_resource.resource_format(),
               resource_provider_->YuvResourceFormat(bits_per_channel));
 
     if (!plane_resource.Matches(video_frame->unique_id(), i)) {
       // We need to transfer data from |video_frame| to the plane resource.
       // TODO(reveman): Can use GpuMemoryBuffers here to improve performance.
 
       // The |resource_size_pixels| is the size of the resource we want to
       // upload to.
       gfx::Size resource_size_pixels = plane_resource.resource_size();
       // The |video_stride_bytes| is the width of the video frame we are
       // uploading (including non-frame data to fill in the stride).
       int video_stride_bytes = video_frame->stride(i);
 
-      size_t bytes_per_row = ResourceUtil::UncheckedWidthInBytes<size_t>(
+      size_t bytes_per_row = ResourceUtil::CheckedWidthInBytes<size_t>(
           resource_size_pixels.width(), plane_resource.resource_format());
       // Use 4-byte row alignment (OpenGL default) for upload performance.
       // Assuming that GL_UNPACK_ALIGNMENT has not changed from default.
       size_t upload_image_stride =
-          MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u);
+          MathUtil::CheckedRoundUp<size_t>(bytes_per_row, 4u);
 
       bool needs_conversion = false;
       int shift = 0;
 
       // LUMINANCE_F16 uses half-floats, so we always need a conversion step.
       if (plane_resource.resource_format() == LUMINANCE_F16) {
         needs_conversion = true;
         // Note that the current method of converting integers to half-floats
         // stops working if you have more than 10 bits of data.
         DCHECK_LE(bits_per_channel, 10);
@@ skipping 102 matching lines @@
   // Update the release sync point in |video_frame| with |sync_token|
   // returned by the compositor and emit a WaitSyncTokenCHROMIUM on
   // |video_frame|'s previous sync point using the current GL context.
   SyncTokenClientImpl client(updater->context_provider_->ContextGL(),
                              sync_token);
   video_frame->UpdateReleaseSyncToken(&client);
 }
 
 // Create a copy of a texture-backed source video frame in a new GL_TEXTURE_2D
 // texture.
-void VideoResourceUpdater::CopyPlaneTexture(
+bool VideoResourceUpdater::CopyPlaneTexture(
     media::VideoFrame* video_frame,
     const gpu::MailboxHolder& mailbox_holder,
     VideoFrameExternalResources* external_resources) {
   gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL();
   SyncTokenClientImpl client(gl, mailbox_holder.sync_token);
 
   const gfx::Size output_plane_resource_size = video_frame->coded_size();
   // The copy needs to be a direct transfer of pixel data, so we use an RGBA8
   // target to avoid loss of precision or dropping any alpha component.
   const ResourceFormat copy_target_format = ResourceFormat::RGBA_8888;
 
-  // Search for an existing resource to reuse.
-  VideoResourceUpdater::ResourceList::iterator resource = all_resources_.end();
-
-  for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) {
-    // Reuse resource if attributes match and the resource is a currently
-    // unreferenced texture.
-    if (it->resource_size() == output_plane_resource_size &&
-        it->resource_format() == copy_target_format &&
-        !it->mailbox().IsZero() && !it->has_refs() &&
-        resource_provider_->GetTextureHint(it->resource_id()) !=
-            ResourceProvider::TEXTURE_HINT_IMMUTABLE) {
-      resource = it;
-      break;
-    }
-  }
-
-  // Otherwise allocate a new resource.
+  const bool is_immutable = false;
+  VideoResourceUpdater::ResourceList::iterator resource =
+      RecycleOrAllocateResource(output_plane_resource_size, copy_target_format,

[danakj, 2016/08/26 18:24:23]

Hm, but if it uses a resource with references on it because Matches() is true,
then the whole point is that it does not need to modify the resource which is
why it can return it. This code would get a resource with references on it
still, and write to it.

[Tobias Sargeant, 2016/08/30 11:31:06]

Passing -1 for the unique_id and plane_number prevents ref'ed resources from
ever matching. I will make that more explicit.

+                                video_frame->ColorSpace(), false, is_immutable,
+                                -1, -1);
   if (resource == all_resources_.end()) {
-    const bool is_immutable = false;
-    resource = AllocateResource(output_plane_resource_size, copy_target_format,
-                                video_frame->ColorSpace(), true, is_immutable);
+    return false;
   }
 
   resource->add_ref();
 
   ResourceProvider::ScopedWriteLockGL lock(resource_provider_,
                                            resource->resource_id(), false);
   DCHECK_EQ(
       resource_provider_->GetResourceTextureTarget(resource->resource_id()),
       (GLenum)GL_TEXTURE_2D);
 
@@ skipping 15 matching lines @@
   // Done with the source video frame texture at this point.
   video_frame->UpdateReleaseSyncToken(&client);
 
   TextureMailbox mailbox(resource->mailbox(), sync_token, GL_TEXTURE_2D,
                          video_frame->coded_size(), false, false);
   mailbox.set_color_space(video_frame->ColorSpace());
   external_resources->mailboxes.push_back(mailbox);
 
   external_resources->release_callbacks.push_back(
       base::Bind(&RecycleResource, AsWeakPtr(), resource->resource_id()));
+
+  return true;
 }
 
 VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes(
     scoped_refptr<media::VideoFrame> video_frame) {
   TRACE_EVENT0("cc", "VideoResourceUpdater::CreateForHardwarePlanes");
   DCHECK(video_frame->HasTextures());
   if (!context_provider_)
     return VideoFrameExternalResources();
 
   VideoFrameExternalResources external_resources;
@@ skipping 10 matching lines @@
   }
 
   const size_t num_planes = media::VideoFrame::NumPlanes(video_frame->format());
   for (size_t i = 0; i < num_planes; ++i) {
     const gpu::MailboxHolder& mailbox_holder = video_frame->mailbox_holder(i);
     if (mailbox_holder.mailbox.IsZero())
       break;
 
     if (video_frame->metadata()->IsTrue(
             media::VideoFrameMetadata::COPY_REQUIRED)) {
-      CopyPlaneTexture(video_frame.get(), mailbox_holder, &external_resources);
+      if (!CopyPlaneTexture(video_frame.get(), mailbox_holder,
+                            &external_resources)) {
+        return VideoFrameExternalResources();
+      }
     } else {
       TextureMailbox mailbox(mailbox_holder.mailbox, mailbox_holder.sync_token,
                              mailbox_holder.texture_target,
                              video_frame->coded_size(),
                              video_frame->metadata()->IsTrue(
                                  media::VideoFrameMetadata::ALLOW_OVERLAY),
                              false);
       mailbox.set_color_space(video_frame->ColorSpace());
       external_resources.mailboxes.push_back(mailbox);
       external_resources.release_callbacks.push_back(
           base::Bind(&ReturnTexture, AsWeakPtr(), video_frame));
     }
   }
   return external_resources;
 }
 
 // static
 void VideoResourceUpdater::RecycleResource(
     base::WeakPtr<VideoResourceUpdater> updater,
     ResourceId resource_id,
     const gpu::SyncToken& sync_token,
     bool lost_resource,
     BlockingTaskRunner* main_thread_task_runner) {
   if (!updater.get()) {
     // Resource was already deleted.
     return;
   }
-
   const ResourceList::iterator resource_it = std::find_if(
       updater->all_resources_.begin(), updater->all_resources_.end(),
       [resource_id](const PlaneResource& plane_resource) {
         return plane_resource.resource_id() == resource_id;
       });
   if (resource_it == updater->all_resources_.end())
     return;
 
   ContextProvider* context_provider = updater->context_provider_;
   if (context_provider && sync_token.HasData()) {
     context_provider->ContextGL()->WaitSyncTokenCHROMIUM(
         sync_token.GetConstData());
   }
 
   if (lost_resource) {
     resource_it->clear_refs();
     updater->DeleteResource(resource_it);
     return;
   }
 
   resource_it->remove_ref();
 }
 
 }  // namespace cc