If the backend texture is the same we let skia take care of it

We are now using SharedImage to hold the image for the video renderer,
https://chromium-review.googlesource.com/c/chromium/src/+/1616978.
That change, together with the change that made the video be deferred, and
therefore, use the paintRecorder, let us safely remove the workaround.

There was nevertheless the issue of texture lifetime to extend the texture.
If the backend texture is the same in the SkImage and the one contained
in the cache, we will let skia release it. If they are different, cache has
to destroy their own texture, and the one in skia will survive the delete.

Bug: 974656, 991792, 993774, 994516, 996083, 1004780
Change-Id: I3bf8c1037b77434a1cf0385872fc7d994886cd30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789883Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697370}

media/renderers/paint_canvas_video_renderer.cc

@@ -634,25 +634,13 @@ void PaintCanvasVideoRenderer::Paint(scoped_refptr<VideoFrame> video_frame,
                       -SkFloatToScalar(image.height() * 0.5f));
   }
 
-  // This is a workaround for crbug.com/524717. A texture backed image is not
-  // safe to access on another thread or GL context. So if we're drawing into a
-  // recording canvas we read the texture back into CPU memory and record that
-  // sw image into the SkPicture. The long term solution is for Skia to provide
-  // a SkPicture filter that makes a picture safe for multiple CPU raster
-  // threads. (skbug.com/4321).
-  if (canvas->imageInfo().colorType() == kUnknown_SkColorType &&
-      image.IsTextureBacked()) {
-    sk_sp<SkImage> non_texture_image =
-        image.GetSkImage()->makeNonTextureImage();
-    image = cc::PaintImageBuilder::WithProperties(image)
-                .set_image(std::move(non_texture_image), image.content_id())
-                .TakePaintImage();
-  }
-
   SkImageInfo info;
   size_t row_bytes;
   SkIPoint origin;
   void* pixels = nullptr;
+  // This if is a special handling of video for SkiaPaintcanvas backend, where
+  // the video does not need any transform and it is enough to draw the frame
+  // directly into the skia canvas
   if (!need_transform && video_frame->IsMappable() &&
       flags.getAlpha() == SK_AlphaOPAQUE &&
       flags.getBlendMode() == SkBlendMode::kSrc &&
@@ -1469,7 +1457,8 @@ PaintCanvasVideoRenderer::Cache::~Cache() {
   DCHECK(!source_mailbox.IsZero());
   DCHECK(source_texture);
   auto* gl = context_provider->ContextGL();
-  gl->DeleteTextures(1, &source_texture);
+  if (!texture_ownership_in_skia)
+    gl->DeleteTextures(1, &source_texture);
   if (!wraps_video_frame_texture) {
     gpu::SyncToken sync_token;
     gl->GenUnverifiedSyncTokenCHROMIUM(sync_token.GetData());
@@ -1478,6 +1467,26 @@ PaintCanvasVideoRenderer::Cache::~Cache() {
   }
 }
 
+bool PaintCanvasVideoRenderer::Cache::Recycle() {
+  if (!texture_ownership_in_skia)
+    return true;
+
+  auto sk_image = paint_image.GetSkImage();
+  paint_image = cc::PaintImage();
+  if (!sk_image->unique())
+    return false;
+
+  // Flush any pending GPU work using this texture.
+  sk_image->flush(context_provider->GrContext());
+
+  // We need a new texture ID because skia will destroy the previous one with
+  // the SkImage.
+  texture_ownership_in_skia = false;
+  source_texture = SynchronizeAndImportMailbox(
+      context_provider->ContextGL(), gpu::SyncToken(), source_mailbox);
+  return true;
+}
+
 bool PaintCanvasVideoRenderer::UpdateLastImage(
     scoped_refptr<VideoFrame> video_frame,
     viz::ContextProvider* context_provider,
@@ -1515,7 +1524,8 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
                           video_frame->ColorSpace(), context_provider);
       } else {
         if (cache_ && cache_->context_provider == context_provider &&
-            cache_->coded_size == video_frame->coded_size()) {
+            cache_->coded_size == video_frame->coded_size() &&
+            cache_->Recycle()) {
           // We can reuse the shared image from the previous cache.
           cache_->frame_id = video_frame->unique_id();
         } else {
@@ -1527,6 +1537,8 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
           cache_->source_texture = SynchronizeAndImportMailbox(
               gl, sii->GenUnverifiedSyncToken(), cache_->source_mailbox);
         }
+
+        DCHECK(!cache_->texture_ownership_in_skia);
         ScopedSharedImageAccess dest_access(
             gl, cache_->source_texture, cache_->source_mailbox,
             GL_SHARED_IMAGE_ACCESS_MODE_READWRITE_CHROMIUM);
@@ -1561,9 +1573,32 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
       cache_->context_provider = context_provider;
       cache_->coded_size = video_frame->coded_size();
       cache_->visible_rect = video_frame->visible_rect();
-      paint_image_builder.set_image(
-          source_image->makeSubset(gfx::RectToSkIRect(cache_->visible_rect)),
-          cc::PaintImage::GetNextContentId());
+      sk_sp<SkImage> source_subset =
+          source_image->makeSubset(gfx::RectToSkIRect(cache_->visible_rect));
+      if (source_subset) {
+        // We use the flushPendingGrContextIO = true so we can flush any pending
+        // GPU work on the GrContext to ensure that skia exectues the work for
+        // generating the subset and it can be safely destroyed.
+        GrBackendTexture image_backend =
+            source_image->getBackendTexture(/*flushPendingGrContextIO*/ true);
+        GrBackendTexture subset_backend =
+            source_subset->getBackendTexture(/*flushPendingGrContextIO*/ true);
+#if DCHECK_IS_ON()
+        GrGLTextureInfo backend_info;
+        if (image_backend.getGLTextureInfo(&backend_info))
+          DCHECK_EQ(backend_info.fID, cache_->source_texture);
+#endif
+        if (subset_backend.isValid() &&
+            subset_backend.isSameTexture(image_backend)) {
+          cache_->texture_ownership_in_skia = true;
+          source_subset = SkImage::MakeFromAdoptedTexture(
+              cache_->context_provider->GrContext(), image_backend,
+              kTopLeft_GrSurfaceOrigin, kRGBA_8888_SkColorType,
+              kPremul_SkAlphaType, source_image->imageInfo().refColorSpace());
+        }
+      }
+      paint_image_builder.set_image(source_subset,
+                                    cc::PaintImage::GetNextContentId());
     } else {
       cache_.emplace(video_frame->unique_id());
       paint_image_builder.set_paint_image_generator(

media/renderers/paint_canvas_video_renderer.h

@@ -226,6 +226,14 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
     // Whether |source_mailbox| directly points to a texture of the VideoFrame
     // (if true), or to an allocated shared image (if false).
     bool wraps_video_frame_texture = false;
+
+    // Whether the texture pointed by |paint_image| is owned by skia or not.
+    bool texture_ownership_in_skia = false;
+
+    // Used to allow recycling of the previous shared image. This requires that
+    // no external users have access to this resource via SkImage. Returns true
+    // if the existing resource can be recycled.
+    bool Recycle();
   };
 
   // Update the cache holding the most-recently-painted frame. Returns false

media/renderers/paint_canvas_video_renderer_unittest.cc

@@ -1174,6 +1174,14 @@ class PaintCanvasVideoRendererWithGLTest : public PaintCanvasVideoRendererTest {
                                       gfx::Rect(2, 2, 12, 4),
                                       std::move(closure));
   }
+  // Creates a cropped I420 VideoFrame. |closure| is run once the shared images
+  // backing the VideoFrame have been destroyed.
+  scoped_refptr<VideoFrame> CreateTestI420FrameNotSubset(
+      base::OnceClosure closure) {
+    return CreateSharedImageI420Frame(media_context_, gfx::Size(16, 8),
+                                      gfx::Rect(0, 0, 16, 8),
+                                      std::move(closure));
+  }
 
   // Checks that the contents of a texture/canvas match the expectations for the
   // cropped I420 frame above. |get_color| is a callback that returns the actual
@@ -1192,6 +1200,23 @@ class PaintCanvasVideoRendererWithGLTest : public PaintCanvasVideoRendererTest {
     EXPECT_EQ(SK_ColorWHITE, get_color.Run(11, 3));
   }
 
+  // Checks that the contents of a texture/canvas match the expectations for the
+  // cropped I420 frame above. |get_color| is a callback that returns the actual
+  // color at a given pixel location.
+  static void CheckI420FramePixelsNotSubset(GetColorCallback get_color) {
+    // Avoid checking around the "seams" where subsamples may be interpolated.
+    EXPECT_EQ(SK_ColorBLACK, get_color.Run(2, 2));
+    EXPECT_EQ(SK_ColorRED, get_color.Run(5, 2));
+    EXPECT_EQ(SK_ColorRED, get_color.Run(6, 2));
+    EXPECT_EQ(SK_ColorGREEN, get_color.Run(9, 2));
+    EXPECT_EQ(SK_ColorGREEN, get_color.Run(10, 2));
+    EXPECT_EQ(SK_ColorYELLOW, get_color.Run(13, 2));
+    EXPECT_EQ(SK_ColorBLUE, get_color.Run(2, 5));
+    EXPECT_EQ(SK_ColorMAGENTA, get_color.Run(5, 5));
+    EXPECT_EQ(SK_ColorCYAN, get_color.Run(9, 5));
+    EXPECT_EQ(SK_ColorWHITE, get_color.Run(13, 5));
+  }
+
   // Creates a cropped NV12 VideoFrame, or nullptr if the needed extension is
   // not available. |closure| is run once the shared images backing the
   // VideoFrame have been destroyed.
@@ -1344,6 +1369,19 @@ TEST_F(PaintCanvasVideoRendererWithGLTest, PaintI420) {
   run_loop.Run();
 }
 
+// Checks that we correctly paint a I420 shared image VideoFrame, including
+// correct cropping.
+TEST_F(PaintCanvasVideoRendererWithGLTest, PaintI420NotSubset) {
+  base::RunLoop run_loop;
+  scoped_refptr<VideoFrame> frame =
+      CreateTestI420FrameNotSubset(run_loop.QuitClosure());
+
+  PaintVideoFrameAndCheckPixels(frame, &CheckI420FramePixelsNotSubset);
+
+  frame.reset();
+  run_loop.Run();
+}
+
 // Checks that we correctly copy a NV12 shared image VideoFrame when using
 // CopyVideoFrameYUVDataToGLTexture, including correct cropping.
 TEST_F(PaintCanvasVideoRendererWithGLTest,

third_party/blink/web_tests/fast/canvas/canvas-drawImage-video-reset.html

+<html>
+<head>
+  <title>Ensure correct behavior of drawImage video elements.</title>
+  <style type="text/css">
+  video {
+    display: none;
+  }
+  </style>
+</head>
+<body>
+  <canvas id="canvas"></canvas>
+  <video id="video">
+    <source src="resources/canvas_video.webm" type='video/webm' />
+    <source src="resources/canvas_video.mp4"  type='video/mp4' />
+    <source src="resources/canvas_video.ogv"  type='video/ogg' />
+  </video>
+  <script>
+  var length = 150;
+  var canvas = document.getElementById("canvas");
+  canvas.setAttribute("width", length);
+  canvas.setAttribute("height", length);
+  var ctx = canvas.getContext("2d");
+
+  var video = document.getElementById("video");
+  video.addEventListener("playing", drawImageToCanvas, true);
+  video.play();
+
+  function drawImageToCanvas() {
+    video.removeEventListener("playing", drawImageToCanvas, true);
+    ctx.fillStyle = "blue";
+    ctx.fillRect(0, 0, length, length);
+    ctx.drawImage(video, 0, 0);
+    ctx.globalAlpha = 0.5;
+    ctx.drawImage(video, 0, 60);
+    video.srcObject = null;
+    if (window.testRunner)
+      testRunner.notifyDone();
+  }
+  </script>
+</body>
+</html>