[media] Fix video to canvas GPU-CPU race.

VideoFrames marked with VideoFrameMetadata::READ_LOCK_FENCES_ENABLED
must be retained until all reads have been completed. Such frames are
produced when software frames are uploaded to GpuMemoryBuffers. The
VideoResourceUpdater handles this, but PaintCanvasVideoRenderer
implemented only sync tokens.

As a result, WebGL reads of software-decoded frames can tear when the
storage is reused for later frames.

This CL adds a GL_COMMANDS_COMPLETED_CHROMIUM query after each potential
VideoFrame read in PaintCanvasVideoRenderer, and retains a reference to
the VideoFrame until it completes. This ensures that the memory is not
reused until after the reads complete in the GPU process.

Bug: 819914
Change-Id: I3e6e5f83cff86beedbd76a406aa31c4eb84a0929
Reviewed-on: https://chromium-review.googlesource.com/1244835Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594900}

chrome/browser/android/download/video_frame_thumbnail_converter.cc

@@ -38,7 +38,8 @@ class TextureFrameThumbnailConverter : public VideoFrameThumbnailConverter {
     cc::SkiaPaintCanvas canvas(skbitmap);
     renderer.Copy(frame, &canvas,
                   media::Context3D(context_provider_->ContextGL(),
-                                   context_provider_->GrContext()));
+                                   context_provider_->GrContext()),
+                  context_provider_->ContextSupport());
 
     std::move(pixel_callback).Run(true, std::move(skbitmap));
   }

chrome/browser/devtools/devtools_eye_dropper.h

@@ -12,7 +12,6 @@
 #include "components/viz/host/client_frame_sink_video_capturer.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/web_contents_observer.h"
-#include "media/renderers/paint_canvas_video_renderer.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 namespace blink {
@@ -59,7 +58,6 @@ class DevToolsEyeDropper : public content::WebContentsObserver,
   content::RenderWidgetHost::MouseEventCallback mouse_event_callback_;
   content::RenderWidgetHost* host_;
   std::unique_ptr<viz::ClientFrameSinkVideoCapturer> video_capturer_;
-  media::PaintCanvasVideoRenderer video_renderer_;
   base::WeakPtrFactory<DevToolsEyeDropper> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DevToolsEyeDropper);

content/browser/devtools/devtools_video_consumer.cc

@@ -57,7 +57,7 @@ SkBitmap DevToolsVideoConsumer::GetSkBitmapFromFrame(
   skbitmap.allocN32Pixels(frame->visible_rect().width(),
                           frame->visible_rect().height());
   cc::SkiaPaintCanvas canvas(skbitmap);
-  renderer.Copy(frame, &canvas, media::Context3D());
+  renderer.Copy(frame, &canvas, media::Context3D(), nullptr);
   return skbitmap;
 }
 

content/renderer/media/stream/webmediaplayer_ms.cc

@@ -829,6 +829,7 @@ void WebMediaPlayerMS::Paint(cc::PaintCanvas* canvas,
       compositor_->GetCurrentFrameWithoutUpdatingStatistics();
 
   media::Context3D context_3d;
+  gpu::ContextSupport* context_support = nullptr;
   if (frame && frame->HasTextures()) {
     auto* provider =
         RenderThreadImpl::current()->SharedMainThreadContextProvider().get();
@@ -836,11 +837,11 @@ void WebMediaPlayerMS::Paint(cc::PaintCanvas* canvas,
     if (!provider)
       return;
     context_3d = media::Context3D(provider->ContextGL(), provider->GrContext());
-    DCHECK(context_3d.gl);
+    context_support = provider->ContextSupport();
   }
   const gfx::RectF dest_rect(rect.x, rect.y, rect.width, rect.height);
   video_renderer_.Paint(frame, canvas, dest_rect, flags, video_rotation_,
-                        context_3d);
+                        context_3d, context_support);
 }
 
 bool WebMediaPlayerMS::DidGetOpaqueResponseFromServiceWorker() const {
@@ -1016,8 +1017,8 @@ bool WebMediaPlayerMS::CopyVideoTextureToPlatformTexture(
   DCHECK(context_3d.gl);
 
   return video_renderer_.CopyVideoFrameTexturesToGLTexture(
-      context_3d, gl, video_frame.get(), target, texture, internal_format,
-      format, type, level, premultiply_alpha, flip_y);
+      context_3d, provider->ContextSupport(), gl, video_frame.get(), target,
+      texture, internal_format, format, type, level, premultiply_alpha, flip_y);
 }
 
 bool WebMediaPlayerMS::CopyVideoYUVDataToPlatformTexture(

content/renderer/media/stream/webmediaplayer_ms_compositor.cc

@@ -68,7 +68,8 @@ scoped_refptr<media::VideoFrame> CopyFrame(
     DCHECK(provider->ContextGL());
     video_renderer->Copy(
         frame.get(), &paint_canvas,
-        media::Context3D(provider->ContextGL(), provider->GrContext()));
+        media::Context3D(provider->ContextGL(), provider->GrContext()),
+        provider->ContextSupport());
 
     SkPixmap pixmap;
     const bool result = bitmap.peekPixels(&pixmap);

content/renderer/media_recorder/video_track_recorder.cc

@@ -313,7 +313,8 @@ void VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread(
     DCHECK(context_provider->ContextGL());
     video_renderer_->Copy(video_frame.get(), canvas_.get(),
                           media::Context3D(context_provider->ContextGL(),
-                                           context_provider->GrContext()));
+                                           context_provider->GrContext()),
+                          context_provider->ContextSupport());
 
     SkPixmap pixmap;
     if (!bitmap_.peekPixels(&pixmap)) {

media/blink/webmediaplayer_impl.cc

@@ -1152,7 +1152,8 @@ void WebMediaPlayerImpl::Paint(cc::PaintCanvas* canvas,
   }
   video_renderer_.Paint(
       video_frame, canvas, gfx::RectF(gfx_rect), flags,
-      pipeline_metadata_.video_decoder_config.video_rotation(), context_3d);
+      pipeline_metadata_.video_decoder_config.video_rotation(), context_3d,
+      context_provider_->ContextSupport());
 }
 
 bool WebMediaPlayerImpl::DidGetOpaqueResponseFromServiceWorker() const {
@@ -1243,8 +1244,9 @@ bool WebMediaPlayerImpl::CopyVideoTextureToPlatformTexture(
                            context_provider_->GrContext());
   }
   return video_renderer_.CopyVideoFrameTexturesToGLTexture(
-      context_3d, gl, video_frame.get(), target, texture, internal_format,
-      format, type, level, premultiply_alpha, flip_y);
+      context_3d, context_provider_->ContextSupport(), gl, video_frame.get(),
+      target, texture, internal_format, format, type, level, premultiply_alpha,
+      flip_y);
 }
 
 // static

media/renderers/paint_canvas_video_renderer.cc

@@ -14,6 +14,7 @@
 #include "cc/paint/paint_image.h"
 #include "cc/paint/paint_image_builder.h"
 #include "gpu/GLES2/gl2extchromium.h"
+#include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/common/capabilities.h"
 #include "gpu/command_buffer/common/mailbox_holder.h"
@@ -275,6 +276,35 @@ void VideoFrameCopyTextureOrSubTexture(gpu::gles2::GLES2Interface* gl,
   }
 }
 
+void OnQueryDone(scoped_refptr<VideoFrame> video_frame,
+                 gpu::gles2::GLES2Interface* gl,
+                 unsigned query_id) {
+  gl->DeleteQueriesEXT(1, &query_id);
+  // |video_frame| is dropped here.
+}
+
+void SynchronizeVideoFrameRead(scoped_refptr<VideoFrame> video_frame,
+                               gpu::gles2::GLES2Interface* gl,
+                               gpu::ContextSupport* context_support) {
+  DCHECK(gl);
+  DCHECK(context_support);
+
+  SyncTokenClientImpl client(gl);
+  video_frame->UpdateReleaseSyncToken(&client);
+
+  if (video_frame->metadata()->IsTrue(
+          VideoFrameMetadata::READ_LOCK_FENCES_ENABLED)) {
+    // |video_frame| must be kept alive during read operations.
+    unsigned query_id = 0;
+    gl->GenQueriesEXT(1, &query_id);
+    DCHECK(query_id);
+    gl->BeginQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM, query_id);
+    gl->EndQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM);
+    context_support->SignalQuery(
+        query_id, base::BindOnce(&OnQueryDone, video_frame, gl, query_id));
+  }
+}
+
 }  // anonymous namespace
 
 // Generates an RGB image from a VideoFrame. Convert YUV to RGB plain on GPU.
@@ -416,7 +446,8 @@ void PaintCanvasVideoRenderer::Paint(
     const gfx::RectF& dest_rect,
     cc::PaintFlags& flags,
     VideoRotation video_rotation,
-    const Context3D& context_3d) {
+    const Context3D& context_3d,
+    gpu::ContextSupport* context_support) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (flags.getAlpha() == 0) {
     return;
@@ -509,22 +540,23 @@ void PaintCanvasVideoRenderer::Paint(
   canvas->flush();
 
   if (video_frame->HasTextures()) {
-    DCHECK(gl);
-    SyncTokenClientImpl client(gl);
-    video_frame->UpdateReleaseSyncToken(&client);
+    // Synchronize |video_frame| with the read operations in UpdateLastImage(),
+    // which are triggered by canvas->flush().
+    SynchronizeVideoFrameRead(video_frame, gl, context_support);
   }
 }
 
 void PaintCanvasVideoRenderer::Copy(
     const scoped_refptr<VideoFrame>& video_frame,
     cc::PaintCanvas* canvas,
-    const Context3D& context_3d) {
+    const Context3D& context_3d,
+    gpu::ContextSupport* context_support) {
   cc::PaintFlags flags;
   flags.setBlendMode(SkBlendMode::kSrc);
   flags.setFilterQuality(kLow_SkFilterQuality);
   Paint(video_frame, canvas,
         gfx::RectF(gfx::SizeF(video_frame->visible_rect().size())), flags,
-        media::VIDEO_ROTATION_0, context_3d);
+        media::VIDEO_ROTATION_0, context_3d, context_support);
 }
 
 namespace {
@@ -901,14 +933,14 @@ void PaintCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
                                     target, texture, internal_format, format,
                                     type, level, premultiply_alpha, flip_y);
   gl->DeleteTextures(1, &source_texture);
-  gl->Flush();
-
-  SyncTokenClientImpl client(gl);
-  video_frame->UpdateReleaseSyncToken(&client);
+  gl->ShallowFlushCHROMIUM();
+  // The caller must call SynchronizeVideoFrameRead() after this operation, but
+  // we can't do that because we don't have the ContextSupport.
 }
 
 bool PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     const Context3D& context_3d,
+    gpu::ContextSupport* context_support,
     gpu::gles2::GLES2Interface* destination_gl,
     const scoped_refptr<VideoFrame>& video_frame,
     unsigned int target,
@@ -936,7 +968,11 @@ bool PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     if (!backend_texture.getGLTextureInfo(&texture_info))
       return false;
 
+    // Synchronize |video_frame| with the read operations in UpdateLastImage(),
+    // which are triggered by getBackendTexture().
     gpu::gles2::GLES2Interface* canvas_gl = context_3d.gl;
+    SynchronizeVideoFrameRead(video_frame, canvas_gl, context_support);
+
     gpu::MailboxHolder mailbox_holder;
     mailbox_holder.texture_target = texture_info.fTarget;
     canvas_gl->ProduceTextureDirectCHROMIUM(texture_info.fID,
@@ -964,13 +1000,30 @@ bool PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
     gpu::SyncToken dest_sync_token;
     destination_gl->GenUnverifiedSyncTokenCHROMIUM(dest_sync_token.GetData());
     canvas_gl->WaitSyncTokenCHROMIUM(dest_sync_token.GetConstData());
-
-    SyncTokenClientImpl client(canvas_gl);
-    video_frame->UpdateReleaseSyncToken(&client);
   } else {
+    gpu::gles2::GLES2Interface* canvas_gl = context_3d.gl;
+
+    // Create a mailbox for the target texture.
+    gpu::MailboxHolder mailbox_holder;
+    mailbox_holder.texture_target = target;
+    destination_gl->ProduceTextureDirectCHROMIUM(texture,
+                                                 mailbox_holder.mailbox.name);
+    destination_gl->GenUnverifiedSyncTokenCHROMIUM(
+        mailbox_holder.sync_token.GetData());
+
+    // Make the target texture available to |canvas_gl|.
+    canvas_gl->WaitSyncTokenCHROMIUM(mailbox_holder.sync_token.GetConstData());
+    uint32_t intermediate_texture =
+        canvas_gl->CreateAndConsumeTextureCHROMIUM(mailbox_holder.mailbox.name);
+
+    // Copy the texture contents and discard the intermediate texture.
     CopyVideoFrameSingleTextureToGLTexture(
-        destination_gl, video_frame.get(), target, texture, internal_format,
-        format, type, level, premultiply_alpha, flip_y);
+        canvas_gl, video_frame.get(), target, intermediate_texture,
+        internal_format, format, type, level, premultiply_alpha, flip_y);
+    canvas_gl->DeleteTextures(1, &intermediate_texture);
+    canvas_gl->ShallowFlushCHROMIUM();
+
+    SynchronizeVideoFrameRead(video_frame, canvas_gl, context_support);
   }
 
   return true;
@@ -1208,8 +1261,9 @@ bool PaintCanvasVideoRenderer::UpdateLastImage(
       return false;
     last_id_ = video_frame->unique_id();
   }
+
+  DCHECK(last_image_);
   last_image_deleting_timer_.Reset();
-  DCHECK(!!last_image_);
   return true;
 }
 

media/renderers/paint_canvas_video_renderer.h

@@ -29,6 +29,7 @@ class RectF;
 
 namespace gpu {
 struct Capabilities;
+class ContextSupport;
 }
 
 namespace media {
@@ -39,25 +40,29 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
   PaintCanvasVideoRenderer();
   ~PaintCanvasVideoRenderer();
 
-  // Paints |video_frame| on |canvas|, scaling and rotating the result to fit
-  // dimensions specified by |dest_rect|.
+  // Paints |video_frame| translated and scaled to |dest_rect| on |canvas|.
+  //
   // If the format of |video_frame| is PIXEL_FORMAT_NATIVE_TEXTURE, |context_3d|
-  // must be provided.
+  // and |context_support| must be provided.
   //
-  // Black will be painted on |canvas| if |video_frame| is null.
+  // If |video_frame| is nullptr or an unsupported format, |dest_rect| will be
+  // painted black.
   void Paint(const scoped_refptr<VideoFrame>& video_frame,
              cc::PaintCanvas* canvas,
              const gfx::RectF& dest_rect,
              cc::PaintFlags& flags,
              VideoRotation video_rotation,
-             const Context3D& context_3d);
+             const Context3D& context_3d,
+             gpu::ContextSupport* context_support);
 
-  // Copy |video_frame| on |canvas|.
+  // Paints |video_frame| scaled to its visible size on |canvas|.
+  //
   // If the format of |video_frame| is PIXEL_FORMAT_NATIVE_TEXTURE, |context_3d|
-  // must be provided.
+  // and |context_support| must be provided.
   void Copy(const scoped_refptr<VideoFrame>& video_frame,
             cc::PaintCanvas* canvas,
-            const Context3D& context_3d);
+            const Context3D& context_3d,
+            gpu::ContextSupport* context_support);
 
   // Convert the contents of |video_frame| to raw RGB pixels. |rgb_pixels|
   // should point into a buffer large enough to hold as many 32 bit RGBA pixels
@@ -82,15 +87,12 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
       bool premultiply_alpha,
       bool flip_y);
 
-  // Copy the contents of texture of |video_frame| to texture |texture| in
-  // context |destination_gl|.
-  // |level|, |internal_format|, |type| specify target texture |texture|.
+  // Copy the contents of |video_frame| to |texture| of |destination_gl|.
+  //
   // The format of |video_frame| must be VideoFrame::NATIVE_TEXTURE.
-  // |context_3d| has a GrContext that may be used during the copy.
-  // CorrectLastImageDimensions() ensures that the source texture will be
-  // cropped to |visible_rect|. Returns true on success.
   bool CopyVideoFrameTexturesToGLTexture(
       const Context3D& context_3d,
+      gpu::ContextSupport* context_support,
       gpu::gles2::GLES2Interface* destination_gl,
       const scoped_refptr<VideoFrame>& video_frame,
       unsigned int target,
@@ -168,8 +170,6 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
   // never be painted again, so we can release the resource.
   void ResetCache();
 
-  void CorrectLastImageDimensions(const SkIRect& visible_rect);
-
   // Used for unit test.
   SkISize LastImageDimensionsForTesting();
 
@@ -179,6 +179,8 @@ class MEDIA_EXPORT PaintCanvasVideoRenderer {
   bool UpdateLastImage(const scoped_refptr<VideoFrame>& video_frame,
                        const Context3D& context_3d);
 
+  void CorrectLastImageDimensions(const SkIRect& visible_rect);
+
   // Last image used to draw to the canvas.
   cc::PaintImage last_image_;
 

media/renderers/paint_canvas_video_renderer_unittest.cc

@@ -225,7 +225,7 @@ void PaintCanvasVideoRendererTest::PaintWithoutFrame(cc::PaintCanvas* canvas) {
   cc::PaintFlags flags;
   flags.setFilterQuality(kLow_SkFilterQuality);
   renderer_.Paint(nullptr, canvas, kNaturalRect, flags, VIDEO_ROTATION_0,
-                  Context3D());
+                  Context3D(), nullptr);
 }
 
 void PaintCanvasVideoRendererTest::Paint(
@@ -260,13 +260,13 @@ void PaintCanvasVideoRendererTest::PaintRotated(
   flags.setBlendMode(mode);
   flags.setFilterQuality(kLow_SkFilterQuality);
   renderer_.Paint(video_frame, canvas, dest_rect, flags, video_rotation,
-                  Context3D());
+                  Context3D(), nullptr);
 }
 
 void PaintCanvasVideoRendererTest::Copy(
     const scoped_refptr<VideoFrame>& video_frame,
     cc::PaintCanvas* canvas) {
-  renderer_.Copy(video_frame, canvas, Context3D());
+  renderer_.Copy(video_frame, canvas, Context3D(), nullptr);
 }
 
 TEST_F(PaintCanvasVideoRendererTest, NoFrame) {
@@ -564,7 +564,7 @@ TEST_F(PaintCanvasVideoRendererTest, Y16) {
   flags.setFilterQuality(kNone_SkFilterQuality);
   renderer_.Paint(video_frame, &canvas,
                   gfx::RectF(bitmap.width(), bitmap.height()), flags,
-                  VIDEO_ROTATION_0, Context3D());
+                  VIDEO_ROTATION_0, Context3D(), nullptr);
   for (int j = 0; j < bitmap.height(); j++) {
     for (int i = 0; i < bitmap.width(); i++) {
       const int value = i + j * bitmap.width();
@@ -657,43 +657,32 @@ TEST_F(PaintCanvasVideoRendererTest, ContextLost) {
   cc::PaintFlags flags;
   flags.setFilterQuality(kLow_SkFilterQuality);
   renderer_.Paint(video_frame, &canvas, kNaturalRect, flags, VIDEO_ROTATION_90,
-                  context_3d);
+                  context_3d, nullptr);
 }
 
 void EmptyCallback(const gpu::SyncToken& sync_token) {}
 
 TEST_F(PaintCanvasVideoRendererTest, CorrectFrameSizeToVisibleRect) {
-  int fWidth{16}, fHeight{16};
+  constexpr int fWidth{16}, fHeight{16};
   SkImageInfo imInfo =
       SkImageInfo::MakeN32(fWidth, fHeight, kOpaque_SkAlphaType);
 
-  sk_sp<const GrGLInterface> glInterface(GrGLCreateNullInterface());
-  sk_sp<GrContext> grContext = GrContext::MakeGL(std::move(glInterface));
-
-  sk_sp<SkSurface> surface =
-      SkSurface::MakeRenderTarget(grContext.get(), SkBudgeted::kYes, imInfo);
-  cc::SkiaPaintCanvas canvas(surface->getCanvas());
+  cc::SkiaPaintCanvas canvas(AllocBitmap(kWidth, kHeight));
 
-  TestGLES2Interface gles2;
-  Context3D context_3d(&gles2, grContext.get());
   gfx::Size coded_size(fWidth, fHeight);
   gfx::Size visible_size(fWidth / 2, fHeight / 2);
 
-  gpu::MailboxHolder mailbox_holders[VideoFrame::kMaxPlanes];
-  for (size_t i = 0; i < VideoFrame::kMaxPlanes; i++) {
-    mailbox_holders[i] = gpu::MailboxHolder(
-        gpu::Mailbox::Generate(), gpu::SyncToken(), GL_TEXTURE_RECTANGLE_ARB);
-  }
+  uint8_t memory[fWidth * fHeight * 2] = {0};
 
-  auto video_frame = VideoFrame::WrapNativeTextures(
-      PIXEL_FORMAT_I420, mailbox_holders, base::Bind(EmptyCallback), coded_size,
-      gfx::Rect(visible_size), visible_size,
+  auto video_frame = media::VideoFrame::WrapExternalData(
+      media::PIXEL_FORMAT_Y16, coded_size, gfx::Rect(visible_size),
+      visible_size, &memory[0], fWidth * fHeight * 2,
       base::TimeDelta::FromMilliseconds(4));
 
   gfx::RectF visible_rect(visible_size.width(), visible_size.height());
   cc::PaintFlags flags;
   renderer_.Paint(video_frame, &canvas, visible_rect, flags, VIDEO_ROTATION_0,
-                  context_3d);
+                  Context3D(), nullptr);
 
   EXPECT_EQ(fWidth / 2, renderer_.LastImageDimensionsForTesting().width());
   EXPECT_EQ(fWidth / 2, renderer_.LastImageDimensionsForTesting().height());

media/renderers/video_resource_updater.cc

@@ -866,7 +866,7 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
 
         // This is software path, so canvas and video_frame are always backed
         // by software.
-        video_renderer_->Copy(video_frame, &canvas, Context3D());
+        video_renderer_->Copy(video_frame, &canvas, Context3D(), nullptr);
       } else {
         HardwarePlaneResource* hardware_resource = plane_resource->AsHardware();
         size_t bytes_per_row = viz::ResourceSizes::CheckedWidthInBytes<size_t>(