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

cc/resources/video_resource_updater.cc

Index: cc/resources/video_resource_updater.cc
diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc
index 6c4f189aaff301863d7dc7c250e86589fa7d269c..7de5bc2b45e846805fd05e752b920d276a33dd9e 100644
--- a/cc/resources/video_resource_updater.cc
+++ b/cc/resources/video_resource_updater.cc
@@ -173,6 +173,49 @@ VideoResourceUpdater::~VideoResourceUpdater() {
 }
 
 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) {
+  if (unique_id >= 0 && plane_index >= 0) {

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

i dont think we should bake in assumptions about the namespace of unique_id in
this file, VideoFrames could come with negative ids. But branching on
plane_index only would be okay. That matches your comment below also. Then maybe
use 0 for the unique_id and -1 for the plane_index? (Or just add another bool
for "allow_return_with_references" or something, up to you)

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

Done. Went with checking only the plane index for != -1, and added comments to
that effect.

+    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) {

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

I kinda feel like it was a bit more clear when this was a single loop. The first
check was if Matches, which could be guarded by do we want to allow. Then the
2nd check inside the loop was for if there's no refs. Now the !has_refs is very
hidden. I'd prefer to combine these two walks (its more efficient too), or
please provide comments on each loop explaining why we have two and what they
each hope to provide. Also we lost the bingo comment about "its safe to use this
even if there are references" which I think had value. Why'd it go away?

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

The original loop was subtle, in that it returned reusable resources immediately
but delayed returning a recyclable resource until the whole list was checked.

I felt like that makes having two loops cleaner, however it was hard to keep the
existing behaviour (which is to recycle the oldest available resource). For that
reason I've gone back to the single loop.

+    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 ==

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

Can you make this an else if () { instead of else { if () {?

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

Done.

+            (resource_provider_->GetTextureHint(it->resource_id()) ==
+             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,
@@ -298,15 +341,23 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
   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.
@@ -328,40 +379,12 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       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;
-        }
-
-        // 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;
-        }
-      }
-    }
+    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);
 
-    // 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;
 
@@ -380,31 +403,57 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
 
   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_EQ(software_compositor, plane_resource.mailbox().IsZero());
 
     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);
-
-      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());
+      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());
+      } 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.software_release_callback = base::Bind(
+          &RecycleResource, AsWeakPtr(), plane_resource.resource_id());
+      external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE;
+    } else {
+      TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(),
+                             resource_provider_->GetResourceTextureTarget(
+                                 plane_resource.resource_id()));
+      mailbox.set_color_space(video_frame->ColorSpace());
+      external_resources.mailboxes.push_back(mailbox);
+      external_resources.release_callbacks.push_back(base::Bind(
+          &RecycleResource, AsWeakPtr(), plane_resource.resource_id()));
+      external_resources.type = VideoFrameExternalResources::RGBA_RESOURCE;
+    }
     return external_resources;
   }
 
@@ -425,12 +474,12 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       // 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;
@@ -553,7 +602,7 @@ void VideoResourceUpdater::ReturnTexture(
 
 // 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) {
@@ -565,27 +614,14 @@ void VideoResourceUpdater::CopyPlaneTexture(
   // 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,
+                                video_frame->ColorSpace(), false, is_immutable,
+                                -1,

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

can you use temp vars to give these litarals names (similar to is_immutable),
and you can put the comment on them there then.

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

Done.

+                                -1 /* do not recycle referenced textures */);
   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();
@@ -621,6 +657,8 @@ void VideoResourceUpdater::CopyPlaneTexture(
 
   external_resources->release_callbacks.push_back(
       base::Bind(&RecycleResource, AsWeakPtr(), resource->resource_id()));
+
+  return true;
 }
 
 VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes(
@@ -651,7 +689,10 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes(
 
     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,
@@ -679,7 +720,6 @@ void VideoResourceUpdater::RecycleResource(
     // 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) {