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

This reverts commit d5ce90c8.

Reason for revert: crbug.com/1005587

Original change's description:
> 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/+/1789883
> Reviewed-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}

TBR=dalecurtis@chromium.org,sandersd@chromium.org,fserb@chromium.org,khushalsagar@chromium.org,juanmihd@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 974656, 991792, 993774, 994516, 996083, 1004780, 1005587
Change-Id: Iccd3daa14e8ec0287850ec64eda37bad6e296169
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1812459Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697876}

media/renderers/paint_canvas_video_renderer.cc

@@ -634,13 +634,25 @@ 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 &&
@@ -1457,8 +1469,7 @@ PaintCanvasVideoRenderer::Cache::~Cache() {
   DCHECK(!source_mailbox.IsZero());
   DCHECK(source_texture);
   auto* gl = context_provider->ContextGL();
-  if (!texture_ownership_in_skia)
-    gl->DeleteTextures(1, &source_texture);
+  gl->DeleteTextures(1, &source_texture);
   if (!wraps_video_frame_texture) {
     gpu::SyncToken sync_token;
     gl->GenUnverifiedSyncTokenCHROMIUM(sync_token.GetData());
@@ -1467,26 +1478,6 @@ 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,
@@ -1524,8 +1515,7 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
                           video_frame->ColorSpace(), context_provider);
       } else {
         if (cache_ && cache_->context_provider == context_provider &&
-            cache_->coded_size == video_frame->coded_size() &&
-            cache_->Recycle()) {
+            cache_->coded_size == video_frame->coded_size()) {
           // We can reuse the shared image from the previous cache.
           cache_->frame_id = video_frame->unique_id();
         } else {
@@ -1537,8 +1527,6 @@ 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);
@@ -1573,32 +1561,9 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
       cache_->context_provider = context_provider;
       cache_->coded_size = video_frame->coded_size();
       cache_->visible_rect = video_frame->visible_rect();
-      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());
+      paint_image_builder.set_image(
+          source_image->makeSubset(gfx::RectToSkIRect(cache_->visible_rect)),
+          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,14 +226,6 @@ 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,14 +1174,6 @@ 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
@@ -1200,23 +1192,6 @@ 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.
@@ -1369,19 +1344,6 @@ 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>