Fix race when drawing video frames to 2d canvas.

Without this change there are possible races in GPU resource access. Most notably PaintCanvasVideoRenderer::Paint flushes the destination canvas after having drawn a video frame into it because the source image buffer's content is ephemeral and is not protected by any locking or synchronization mechanisms. Prior to this change, the call to PaintCanvas::flush in PaintCanvasVideoRenderer::Paint was not effective because RecordPaintCanvas::flush was a no-op. With this change, the flush request gets recorded so that calling code can take care of flushing. BUG=1254747 Change-Id: I2a99fb75f0a29d49079bba92c8f121a6c3005bcf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3231460 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Juanmi Huertas <juanmihd@chromium.org> Commit-Queue: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/main@{#940884}
chromium · Nov 11, 2021 · dae4250 · dae4250
1 parent 08a4fd7
commit dae4250
Show file tree

Hide file tree

Showing 18 changed files with 195 additions and 38 deletions.
diff --git a/cc/paint/paint_canvas.h b/cc/paint/paint_canvas.h
@@ -209,6 +209,8 @@ class CC_PAINT_EXPORT PaintCanvas {
   virtual SkMatrix getTotalMatrix() const = 0;
   virtual SkM44 getLocalToDevice() const = 0;
 
+  virtual bool NeedsFlush() const = 0;
+
   // Used for printing
   enum class AnnotationType {
     URL,

diff --git a/cc/paint/record_paint_canvas.cc b/cc/paint/record_paint_canvas.cc
@@ -30,6 +30,19 @@ SkImageInfo RecordPaintCanvas::imageInfo() const {
   return GetCanvas()->imageInfo();
 }
 
+template <typename T, typename... Args>
+size_t RecordPaintCanvas::push(Args&&... args) {
+#if DCHECK_IS_ON()
+  // The following check fails if client code does not check and handle
+  // NeedsFlush() before issuing draw calls.
+  // Note: restore ops are tolerated when flushes are requested since they are
+  // often necessary in order to bring the canvas to a flushable state
+  DCHECK(disable_flush_check_scope_ || !needs_flush_ ||
+         (std::is_same<T, RestoreOp>::value));
+#endif
+  return list_->push<T>(std::forward<Args>(args)...);
+}
+
 void* RecordPaintCanvas::accessTopLayerPixels(SkImageInfo* info,
                                               size_t* rowBytes,
                                               SkIPoint* origin) {
@@ -38,11 +51,22 @@ void* RecordPaintCanvas::accessTopLayerPixels(SkImageInfo* info,
 }
 
 void RecordPaintCanvas::flush() {
-  // This is a noop when recording.
+  // RecordPaintCanvas is unable to flush its own recording into the graphics
+  // pipeline. So instead we make note of the flush request so that it can be
+  // handled by code that owns the recording.
+  //
+  // Note: The value of needs_flush_ never gets reset. That is because
+  // flushing a recording implies closing this RecordPaintCanvas and starting a
+  // new one.
+  needs_flush_ = true;
+}
+
+bool RecordPaintCanvas::NeedsFlush() const {
+  return needs_flush_;
 }
 
 int RecordPaintCanvas::save() {
-  list_->push<SaveOp>();
+  push<SaveOp>();
   return GetCanvas()->save();
 }
 
@@ -59,21 +83,21 @@ int RecordPaintCanvas::saveLayer(const SkRect* bounds,
     // TODO(enne): it appears that image filters affect matrices and color
     // matrices affect transparent flags on SkCanvas layers, but it's not clear
     // whether those are actually needed and we could just skip ToSkPaint here.
-    list_->push<SaveLayerOp>(bounds, flags);
+    push<SaveLayerOp>(bounds, flags);
     SkPaint paint = flags->ToSkPaint();
     return GetCanvas()->saveLayer(bounds, &paint);
   }
-  list_->push<SaveLayerOp>(bounds, flags);
+  push<SaveLayerOp>(bounds, flags);
   return GetCanvas()->saveLayer(bounds, nullptr);
 }
 
 int RecordPaintCanvas::saveLayerAlpha(const SkRect* bounds, uint8_t alpha) {
-  list_->push<SaveLayerAlphaOp>(bounds, alpha);
+  push<SaveLayerAlphaOp>(bounds, alpha);
   return GetCanvas()->saveLayerAlpha(bounds, alpha);
 }
 
 void RecordPaintCanvas::restore() {
-  list_->push<RestoreOp>();
+  push<RestoreOp>();
   GetCanvas()->restore();
 }
 
@@ -95,46 +119,46 @@ void RecordPaintCanvas::restoreToCount(int save_count) {
 }
 
 void RecordPaintCanvas::translate(SkScalar dx, SkScalar dy) {
-  list_->push<TranslateOp>(dx, dy);
+  push<TranslateOp>(dx, dy);
   GetCanvas()->translate(dx, dy);
 }
 
 void RecordPaintCanvas::scale(SkScalar sx, SkScalar sy) {
-  list_->push<ScaleOp>(sx, sy);
+  push<ScaleOp>(sx, sy);
   GetCanvas()->scale(sx, sy);
 }
 
 void RecordPaintCanvas::rotate(SkScalar degrees) {
-  list_->push<RotateOp>(degrees);
+  push<RotateOp>(degrees);
   GetCanvas()->rotate(degrees);
 }
 
 void RecordPaintCanvas::concat(const SkMatrix& matrix) {
   SkM44 m = SkM44(matrix);
-  list_->push<ConcatOp>(m);
+  push<ConcatOp>(m);
   GetCanvas()->concat(m);
 }
 
 void RecordPaintCanvas::concat(const SkM44& matrix) {
-  list_->push<ConcatOp>(matrix);
+  push<ConcatOp>(matrix);
   GetCanvas()->concat(matrix);
 }
 
 void RecordPaintCanvas::setMatrix(const SkMatrix& matrix) {
   SkM44 m = SkM44(matrix);
-  list_->push<SetMatrixOp>(m);
+  push<SetMatrixOp>(m);
   GetCanvas()->setMatrix(m);
 }
 
 void RecordPaintCanvas::setMatrix(const SkM44& matrix) {
-  list_->push<SetMatrixOp>(matrix);
+  push<SetMatrixOp>(matrix);
   GetCanvas()->setMatrix(matrix);
 }
 
 void RecordPaintCanvas::clipRect(const SkRect& rect,
                                  SkClipOp op,
                                  bool antialias) {
-  list_->push<ClipRectOp>(rect, op, antialias);
+  push<ClipRectOp>(rect, op, antialias);
   GetCanvas()->clipRect(rect, op, antialias);
 }
 
@@ -146,7 +170,7 @@ void RecordPaintCanvas::clipRRect(const SkRRect& rrect,
     clipRect(rrect.getBounds(), op, antialias);
     return;
   }
-  list_->push<ClipRRectOp>(rrect, op, antialias);
+  push<ClipRRectOp>(rrect, op, antialias);
   GetCanvas()->clipRRect(rrect, op, antialias);
 }
 
@@ -175,7 +199,7 @@ void RecordPaintCanvas::clipPath(const SkPath& path,
     }
   }
 
-  list_->push<ClipPathOp>(path, op, antialias, use_paint_cache);
+  push<ClipPathOp>(path, op, antialias, use_paint_cache);
   GetCanvas()->clipPath(path, op, antialias);
   return;
 }
@@ -201,37 +225,37 @@ bool RecordPaintCanvas::getDeviceClipBounds(SkIRect* bounds) const {
 }
 
 void RecordPaintCanvas::drawColor(SkColor color, SkBlendMode mode) {
-  list_->push<DrawColorOp>(color, mode);
+  push<DrawColorOp>(color, mode);
 }
 
 void RecordPaintCanvas::clear(SkColor color) {
-  list_->push<DrawColorOp>(color, SkBlendMode::kSrc);
+  push<DrawColorOp>(color, SkBlendMode::kSrc);
 }
 
 void RecordPaintCanvas::drawLine(SkScalar x0,
                                  SkScalar y0,
                                  SkScalar x1,
                                  SkScalar y1,
                                  const PaintFlags& flags) {
-  list_->push<DrawLineOp>(x0, y0, x1, y1, flags);
+  push<DrawLineOp>(x0, y0, x1, y1, flags);
 }
 
 void RecordPaintCanvas::drawRect(const SkRect& rect, const PaintFlags& flags) {
-  list_->push<DrawRectOp>(rect, flags);
+  push<DrawRectOp>(rect, flags);
 }
 
 void RecordPaintCanvas::drawIRect(const SkIRect& rect,
                                   const PaintFlags& flags) {
-  list_->push<DrawIRectOp>(rect, flags);
+  push<DrawIRectOp>(rect, flags);
 }
 
 void RecordPaintCanvas::drawOval(const SkRect& oval, const PaintFlags& flags) {
-  list_->push<DrawOvalOp>(oval, flags);
+  push<DrawOvalOp>(oval, flags);
 }
 
 void RecordPaintCanvas::drawRRect(const SkRRect& rrect,
                                   const PaintFlags& flags) {
-  list_->push<DrawRRectOp>(rrect, flags);
+  push<DrawRRectOp>(rrect, flags);
 }
 
 void RecordPaintCanvas::drawDRRect(const SkRRect& outer,
@@ -243,7 +267,7 @@ void RecordPaintCanvas::drawDRRect(const SkRRect& outer,
     drawRRect(outer, flags);
     return;
   }
-  list_->push<DrawDRRectOp>(outer, inner, flags);
+  push<DrawDRRectOp>(outer, inner, flags);
 }
 
 void RecordPaintCanvas::drawRoundRect(const SkRect& rect,
@@ -263,7 +287,7 @@ void RecordPaintCanvas::drawRoundRect(const SkRect& rect,
 void RecordPaintCanvas::drawPath(const SkPath& path,
                                  const PaintFlags& flags,
                                  UsePaintCache use_paint_cache) {
-  list_->push<DrawPathOp>(path, flags, use_paint_cache);
+  push<DrawPathOp>(path, flags, use_paint_cache);
 }
 
 void RecordPaintCanvas::drawImage(const PaintImage& image,
@@ -272,7 +296,7 @@ void RecordPaintCanvas::drawImage(const PaintImage& image,
                                   const SkSamplingOptions& sampling,
                                   const PaintFlags* flags) {
   DCHECK(!image.IsPaintWorklet());
-  list_->push<DrawImageOp>(image, left, top, sampling, flags);
+  push<DrawImageOp>(image, left, top, sampling, flags);
 }
 
 void RecordPaintCanvas::drawImageRect(const PaintImage& image,
@@ -281,34 +305,34 @@ void RecordPaintCanvas::drawImageRect(const PaintImage& image,
                                       const SkSamplingOptions& sampling,
                                       const PaintFlags* flags,
                                       SkCanvas::SrcRectConstraint constraint) {
-  list_->push<DrawImageRectOp>(image, src, dst, sampling, flags, constraint);
+  push<DrawImageRectOp>(image, src, dst, sampling, flags, constraint);
 }
 
 void RecordPaintCanvas::drawSkottie(scoped_refptr<SkottieWrapper> skottie,
                                     const SkRect& dst,
                                     float t,
                                     SkottieFrameDataMap images) {
-  list_->push<DrawSkottieOp>(std::move(skottie), dst, t, std::move(images));
+  push<DrawSkottieOp>(std::move(skottie), dst, t, std::move(images));
 }
 
 void RecordPaintCanvas::drawTextBlob(sk_sp<SkTextBlob> blob,
                                      SkScalar x,
                                      SkScalar y,
                                      const PaintFlags& flags) {
-  list_->push<DrawTextBlobOp>(std::move(blob), x, y, flags);
+  push<DrawTextBlobOp>(std::move(blob), x, y, flags);
 }
 
 void RecordPaintCanvas::drawTextBlob(sk_sp<SkTextBlob> blob,
                                      SkScalar x,
                                      SkScalar y,
                                      NodeId node_id,
                                      const PaintFlags& flags) {
-  list_->push<DrawTextBlobOp>(std::move(blob), x, y, node_id, flags);
+  push<DrawTextBlobOp>(std::move(blob), x, y, node_id, flags);
 }
 
 void RecordPaintCanvas::drawPicture(sk_sp<const PaintRecord> record) {
   // TODO(enne): If this is small, maybe flatten it?
-  list_->push<DrawRecordOp>(record);
+  push<DrawRecordOp>(record);
 }
 
 bool RecordPaintCanvas::isClipEmpty() const {
@@ -327,15 +351,15 @@ SkM44 RecordPaintCanvas::getLocalToDevice() const {
 void RecordPaintCanvas::Annotate(AnnotationType type,
                                  const SkRect& rect,
                                  sk_sp<SkData> data) {
-  list_->push<AnnotateOp>(type, rect, data);
+  push<AnnotateOp>(type, rect, data);
 }
 
 void RecordPaintCanvas::recordCustomData(uint32_t id) {
-  list_->push<CustomDataOp>(id);
+  push<CustomDataOp>(id);
 }
 
 void RecordPaintCanvas::setNodeId(int node_id) {
-  list_->push<SetNodeIdOp>(node_id);
+  push<SetNodeIdOp>(node_id);
 }
 
 const SkNoDrawCanvas* RecordPaintCanvas::GetCanvas() const {

diff --git a/cc/paint/record_paint_canvas.h b/cc/paint/record_paint_canvas.h
@@ -120,6 +120,8 @@ class CC_PAINT_EXPORT RecordPaintCanvas : public PaintCanvas {
   void recordCustomData(uint32_t id) override;
   void setNodeId(int) override;
 
+  bool NeedsFlush() const override;
+
   // Don't shadow non-virtual helper functions.
   using PaintCanvas::clipRect;
   using PaintCanvas::clipRRect;
@@ -128,7 +130,46 @@ class CC_PAINT_EXPORT RecordPaintCanvas : public PaintCanvas {
   using PaintCanvas::drawImage;
   using PaintCanvas::drawPicture;
 
+#if DCHECK_IS_ON()
+  void EnterDisableFlushCheckScope() { ++disable_flush_check_scope_; }
+  void LeaveDisableFlushCheckScope() { DCHECK(disable_flush_check_scope_--); }
+  bool IsInDisableFlushCheckScope() { return disable_flush_check_scope_; }
+#endif
+
+  class DisableFlushCheckScope {
+    // Create an object of this type to temporarily allow draw commands to be
+    // recorded while the recording is marked as needing to be flushed.  This is
+    // meant to be used to allow client code to issue the commands necessary to
+    // reach a state where the recording can be safely flushed before beginning
+    // to enforce a check that forbids recording additional draw commands after
+    // a flush was requested.
+   public:
+    explicit DisableFlushCheckScope(RecordPaintCanvas* canvas) {
+#if DCHECK_IS_ON()
+      // We require that NeedsFlush be false upon entering a top-level scope
+      // to prevent consecutive scopes from evading evading flush checks
+      // indefinitely.
+      DCHECK(!canvas->NeedsFlush() || canvas->IsInDisableFlushCheckScope());
+      canvas->EnterDisableFlushCheckScope();
+      canvas_ = canvas;
+#endif
+    }
+    ~DisableFlushCheckScope() {
+#if DCHECK_IS_ON()
+      canvas_->LeaveDisableFlushCheckScope();
+#endif
+    }
+
+   private:
+#if DCHECK_IS_ON()
+    RecordPaintCanvas* canvas_;
+#endif
+  };
+
  private:
+  template <typename T, typename... Args>
+  size_t push(Args&&... args);
+
   const SkNoDrawCanvas* GetCanvas() const;
   SkNoDrawCanvas* GetCanvas();
 
@@ -146,6 +187,10 @@ class CC_PAINT_EXPORT RecordPaintCanvas : public PaintCanvas {
   // lazy initialize the canvas can still be const.
   mutable absl::optional<SkNoDrawCanvas> canvas_;
   SkRect recording_bounds_;
+  bool needs_flush_ = false;
+#if DCHECK_IS_ON()
+  unsigned disable_flush_check_scope_ = 0;
+#endif
 };
 
 }  // namespace cc

diff --git a/cc/paint/skia_paint_canvas.cc b/cc/paint/skia_paint_canvas.cc
@@ -57,6 +57,12 @@ void SkiaPaintCanvas::flush() {
   canvas_->flush();
 }
 
+bool SkiaPaintCanvas::NeedsFlush() const {
+  // Since flush() is always capable of flushing immediately with
+  // SkiaPaintCanvas, there is never any need for deferred flushing.
+  return false;
+}
+
 int SkiaPaintCanvas::save() {
   return canvas_->save();
 }

diff --git a/cc/paint/skia_paint_canvas.h b/cc/paint/skia_paint_canvas.h
@@ -138,6 +138,8 @@ class CC_PAINT_EXPORT SkiaPaintCanvas final : public PaintCanvas {
   SkMatrix getTotalMatrix() const override;
   SkM44 getLocalToDevice() const override;
 
+  bool NeedsFlush() const override;
+
   void Annotate(AnnotationType type,
                 const SkRect& rect,
                 sk_sp<SkData> data) override;

diff --git a/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc b/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
@@ -1513,6 +1513,8 @@ void BaseRenderingContext2D::DrawImageInternal(
     const FloatRect& dst_rect,
     const SkSamplingOptions& sampling,
     const PaintFlags* flags) {
+  cc::RecordPaintCanvas::DisableFlushCheckScope disable_flush_check_scope(
+      static_cast<cc::RecordPaintCanvas*>(c));
   int initial_save_count = c->getSaveCount();
   PaintFlags image_flags = *flags;