[media] Reland: 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.

TBR=pfeldman@chromium.org

Bug: 819914
Change-Id: Id181a476b78e2799bdbff149c9655427ae552743
Reviewed-on: https://chromium-review.googlesource.com/1252962
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595676}

chrome/browser/android/download/download_media_parser.cc

@@ -205,8 +205,15 @@ void DownloadMediaParser::OnVideoFrameDecoded(
 
 void DownloadMediaParser::RenderVideoFrame(
     scoped_refptr<media::VideoFrame> video_frame) {
+  media::Context3D context;
+  gpu::ContextSupport* context_support = nullptr;
   auto context_provider =
       gpu_factories_ ? gpu_factories_->GetMediaContextProvider() : nullptr;
+  if (context_provider) {
+    context = media::Context3D(context_provider->ContextGL(),
+                               context_provider->GrContext());
+    context_support = context_provider->ContextSupport();
+  }
 
   media::PaintCanvasVideoRenderer renderer;
   SkBitmap bitmap;
@@ -215,11 +222,7 @@ void DownloadMediaParser::RenderVideoFrame(
 
   // Draw the video frame to |bitmap|.
   cc::SkiaPaintCanvas canvas(bitmap);
-  media::Context3D context =
-      context_provider ? media::Context3D(context_provider->ContextGL(),
-                                          context_provider->GrContext())
-                       : media::Context3D();
-  renderer.Copy(video_frame, &canvas, context);
+  renderer.Copy(video_frame, &canvas, context, context_support);
 
   NotifyComplete(std::move(bitmap));
 }

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

@@ -1131,10 +1131,12 @@ void WebMediaPlayerImpl::Paint(cc::PaintCanvas* canvas,
 
   gfx::Rect gfx_rect(rect);
   Context3D context_3d;
+  gpu::ContextSupport* context_support = nullptr;
   if (video_frame.get() && video_frame->HasTextures()) {
     if (context_provider_) {
       context_3d = Context3D(context_provider_->ContextGL(),
                              context_provider_->GrContext());
+      context_support = context_provider_->ContextSupport();
     }
     if (!context_3d.gl)
       return;  // Unable to get/create a shared main thread context.
@@ -1152,7 +1154,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_support);
 }
 
 bool WebMediaPlayerImpl::DidGetOpaqueResponseFromServiceWorker() const {
@@ -1238,13 +1241,15 @@ bool WebMediaPlayerImpl::CopyVideoTextureToPlatformTexture(
   }
 
   Context3D context_3d;
+  gpu::ContextSupport* context_support = nullptr;
   if (context_provider_) {
     context_3d = Context3D(context_provider_->ContextGL(),
                            context_provider_->GrContext());
+    context_support = context_provider_->ContextSupport();
   }
   return video_renderer_.CopyVideoFrameTexturesToGLTexture(
-      context_3d, gl, video_frame.get(), target, texture, internal_format,
-      format, type, level, premultiply_alpha, flip_y);
+      context_3d, context_support, 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,
@@ -922,7 +954,9 @@ bool PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture(
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(video_frame);
   DCHECK(video_frame->HasTextures());
-  if (video_frame->NumTextures() > 1) {
+  if (video_frame->NumTextures() > 1 ||
+      video_frame->metadata()->IsTrue(
+          VideoFrameMetadata::READ_LOCK_FENCES_ENABLED)) {
     if (!context_3d.gr_context)
       return false;
     if (!UpdateLastImage(video_frame, context_3d))
@@ -936,7 +970,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,9 +1002,6 @@ 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 {
     CopyVideoFrameSingleTextureToGLTexture(
         destination_gl, video_frame.get(), target, texture, internal_format,
@@ -1208,8 +1243,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>(