Use SkImage in chrome_pdf::PaintReadyRect

Replaces SkBitmap in PaintReadyRect with SkImage, allowing earlier decisions on when to pay the copy cost of an SkBitmap::asImage() call. In particular, while there's still room for improvement, this change greatly reduces copies when PDFiumEngine::Paint() returns a large number of ready rectangles, as the backing memory is no longer duplicated for each rectangle. Fixed: 1284255 Change-Id: Ifd34bb652d314022c085834bfab0aaf2c25c946d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3617149 Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: K. Moon <kmoon@chromium.org> Cr-Commit-Position: refs/heads/main@{#997991}
qweri0p · Apr 29, 2022 · 950a852 · 950a852
1 parent 9ad9961
commit 950a852
Show file tree

Hide file tree

Showing 9 changed files with 42 additions and 45 deletions.
diff --git a/pdf/paint_manager.cc b/pdf/paint_manager.cc
@@ -20,7 +20,6 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "pdf/paint_ready_rect.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkRect.h"
@@ -287,11 +286,10 @@ void PaintManager::DoPaint() {
   }
 
   for (const auto& ready_rect : ready_now) {
-    // TODO(crbug.com/1284255): Avoid inefficient `SkBitmap::asImage()`.
     SkRect skia_rect = gfx::RectToSkRect(ready_rect.rect());
     surface_->getCanvas()->drawImageRect(
-        ready_rect.image().asImage(), skia_rect, skia_rect, SkSamplingOptions(),
-        nullptr, SkCanvas::kStrict_SrcRectConstraint);
+        &ready_rect.image(), skia_rect, skia_rect, SkSamplingOptions(), nullptr,
+        SkCanvas::kStrict_SrcRectConstraint);
   }
 
   Flush();

diff --git a/pdf/paint_manager_unittest.cc b/pdf/paint_manager_unittest.cc
@@ -142,15 +142,12 @@ class PaintManagerTest : public testing::Test {
         1, 1, plugin_size.width() - 1, plugin_size.height() - 2));
     initial_surface->getCanvas()->clear(SK_ColorGREEN);
 
-    SkBitmap initial_bitmap;
-    ASSERT_TRUE(
-        initial_surface->makeImageSnapshot()->asLegacyBitmap(&initial_bitmap));
-
     paint_manager_.Invalidate();
-    ASSERT_TRUE(
-        WaitForFlush(/*expected_paint_rects=*/{gfx::Rect(plugin_size)},
-                     /*fake_ready=*/{{gfx::Rect(plugin_size), initial_bitmap}},
-                     /*fake_pending=*/{}));
+    ASSERT_TRUE(WaitForFlush(
+        /*expected_paint_rects=*/{gfx::Rect(plugin_size)},
+        /*fake_ready=*/
+        {{gfx::Rect(plugin_size), initial_surface->makeImageSnapshot()}},
+        /*fake_pending=*/{}));
 
     // Scroll by `scroll_amount`, painting `expected_paint_rect` magenta.
     paint_manager_.ScrollRect(gfx::Rect(plugin_size), scroll_amount);

diff --git a/pdf/paint_ready_rect.cc b/pdf/paint_ready_rect.cc
@@ -4,15 +4,21 @@
 
 #include "pdf/paint_ready_rect.h"
 
-#include "third_party/skia/include/core/SkBitmap.h"
+#include <utility>
+
+#include "base/check.h"
+#include "third_party/skia/include/core/SkImage.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace chrome_pdf {
 
 PaintReadyRect::PaintReadyRect(const gfx::Rect& rect,
-                               const SkBitmap& image,
+                               sk_sp<SkImage> image,
                                bool flush_now)
-    : rect_(rect), image_(image), flush_now_(flush_now) {}
+    : rect_(rect), image_(std::move(image)), flush_now_(flush_now) {
+  DCHECK(image_);
+}
 
 PaintReadyRect::PaintReadyRect(const PaintReadyRect& other) = default;
 

diff --git a/pdf/paint_ready_rect.h b/pdf/paint_ready_rect.h
@@ -5,9 +5,11 @@
 #ifndef PDF_PAINT_READY_RECT_H_
 #define PDF_PAINT_READY_RECT_H_
 
-#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
 #include "ui/gfx/geometry/rect.h"
 
+class SkImage;
+
 namespace chrome_pdf {
 
 // Stores information about a rectangle that has finished painting. The
@@ -16,7 +18,7 @@ namespace chrome_pdf {
 class PaintReadyRect {
  public:
   PaintReadyRect(const gfx::Rect& rect,
-                 const SkBitmap& image,
+                 sk_sp<SkImage> image,
                  bool flush_now = false);
 
   PaintReadyRect(const PaintReadyRect& other);
@@ -26,15 +28,15 @@ class PaintReadyRect {
   const gfx::Rect& rect() const { return rect_; }
   void set_rect(const gfx::Rect& rect) { rect_ = rect; }
 
-  const SkBitmap& image() const { return image_; }
+  const SkImage& image() const { return *image_; }
 
   // Whether to flush to screen immediately; otherwise, when the rest of the
   // plugin viewport is ready.
   bool flush_now() const { return flush_now_; }
 
  private:
   gfx::Rect rect_;
-  SkBitmap image_;
+  sk_sp<SkImage> image_;
   bool flush_now_;
 };
 

diff --git a/pdf/pdf_view_plugin_base.cc b/pdf/pdf_view_plugin_base.cc
@@ -59,7 +59,9 @@
 #include "third_party/blink/public/web/web_print_preset_options.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/point_conversions.h"
@@ -856,10 +858,6 @@ void PdfViewPluginBase::UpdateGeometryOnPluginRectChanged(
   OnGeometryChanged(zoom_, old_device_scale);
 }
 
-SkBitmap PdfViewPluginBase::GetPluginImageData() const {
-  return image_data_;
-}
-
 void PdfViewPluginBase::RecalculateAreas(double old_zoom,
                                          float old_device_scale) {
   if (zoom_ != old_zoom || device_scale_ != old_device_scale)
@@ -1391,6 +1389,13 @@ void PdfViewPluginBase::SaveToFile(const std::string& token) {
   SaveAs();
 }
 
+// TODO(crbug.com/1263614): Minimize inefficient `SkBitmap::asImage()` calls
+// (which copy the underlying pixel memory) by only creating the `SkImage` after
+// making all changes to `image_data_` first.
+//
+// We probably can reduce this further by writing pixels directly into the
+// `SkSurface` in `PaintManager`, rather than using an intermediate `SkBitmap`
+// and `SkImage`.
 void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
                                 std::vector<PaintReadyRect>& ready,
                                 std::vector<gfx::Rect>& pending) {
@@ -1422,9 +1427,10 @@ void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
       std::vector<gfx::Rect> pdf_ready;
       std::vector<gfx::Rect> pdf_pending;
       engine()->Paint(pdf_rect, image_data_, pdf_ready, pdf_pending);
+      sk_sp<SkImage> painted_image = image_data_.asImage();
       for (gfx::Rect& ready_rect : pdf_ready) {
         ready_rect.Offset(available_area_.OffsetFromOrigin());
-        ready.push_back(PaintReadyRect(ready_rect, GetPluginImageData()));
+        ready.emplace_back(ready_rect, painted_image);
       }
       for (gfx::Rect& pending_rect : pdf_pending) {
         pending_rect.Offset(available_area_.OffsetFromOrigin());
@@ -1439,7 +1445,7 @@ void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
     if (rect.y() < first_page_ypos) {
       gfx::Rect region = gfx::IntersectRects(
           rect, gfx::Rect(gfx::Size(plugin_rect_.width(), first_page_ypos)));
-      ready.push_back(PaintReadyRect(region, GetPluginImageData()));
+      ready.emplace_back(region, image_data_.asImage());
       image_data_.erase(background_color_, gfx::RectToSkIRect(region));
     }
 
@@ -1450,7 +1456,7 @@ void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
       if (!intersection.IsEmpty()) {
         image_data_.erase(background_part.color,
                           gfx::RectToSkIRect(intersection));
-        ready.push_back(PaintReadyRect(intersection, GetPluginImageData()));
+        ready.emplace_back(intersection, image_data_.asImage());
       }
     }
   }
@@ -1468,9 +1474,8 @@ void PdfViewPluginBase::PrepareForFirstPaint(
   // Fill the image data buffer with the background color.
   first_paint_ = false;
   image_data_.eraseColor(background_color_);
-  gfx::Rect rect(gfx::SkISizeToSize(image_data_.dimensions()));
-  ready.push_back(
-      PaintReadyRect(rect, GetPluginImageData(), /*flush_now=*/true));
+  ready.emplace_back(gfx::SkIRectToRect(image_data_.bounds()),
+                     image_data_.asImage(), /*flush_now=*/true);
 }
 
 void PdfViewPluginBase::ClearDeferredInvalidates() {

diff --git a/pdf/pdf_view_plugin_base.h b/pdf/pdf_view_plugin_base.h
@@ -259,9 +259,6 @@ class PdfViewPluginBase : public PDFEngine::Client,
   // engine, and updates the accessibility information.
   void OnGeometryChanged(double old_zoom, float old_device_scale);
 
-  // Returns the plugin-specific image data buffer.
-  virtual SkBitmap GetPluginImageData() const;
-
   // Updates the geometry of the plugin and its image data if the plugin rect
   // or the device scale has changed. `new_plugin_rect` must be in device
   // pixels (with the device scale applied).

diff --git a/pdf/pdf_view_web_plugin_unittest.cc b/pdf/pdf_view_web_plugin_unittest.cc
@@ -431,10 +431,8 @@ class PdfViewWebPluginTest : public PdfViewWebPluginWithoutInitializeTest {
     canvas_.DrawColor(kDefaultColor);
 
     // Paint the plugin with `kPaintColor`.
-    plugin_->UpdateSnapshot(
-        CreateSkiaImageForTesting(plugin_->GetPluginRectForTesting().size(),
-                                  kPaintColor)
-            .asImage());
+    plugin_->UpdateSnapshot(CreateSkiaImageForTesting(
+        plugin_->GetPluginRectForTesting().size(), kPaintColor));
     plugin_->Paint(canvas_.sk_canvas(), paint_rect);
 
     // Expect the clipped area on canvas to be filled with `kPaintColor`.

diff --git a/pdf/test/test_helpers.cc b/pdf/test/test_helpers.cc
@@ -53,13 +53,8 @@ sk_sp<SkSurface> CreateSkiaSurfaceForTesting(const gfx::Size& size,
   return surface;
 }
 
-SkBitmap CreateSkiaImageForTesting(const gfx::Size& size, SkColor color) {
-  sk_sp<SkImage> snapshot =
-      CreateSkiaSurfaceForTesting(size, color)->makeImageSnapshot();
-
-  SkBitmap bitmap;
-  snapshot->asLegacyBitmap(&bitmap);
-  return bitmap;
+sk_sp<SkImage> CreateSkiaImageForTesting(const gfx::Size& size, SkColor color) {
+  return CreateSkiaSurfaceForTesting(size, color)->makeImageSnapshot();
 }
 
 }  // namespace chrome_pdf
diff --git a/pdf/test/test_helpers.h b/pdf/test/test_helpers.h
@@ -10,7 +10,6 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 
-class SkBitmap;
 class SkImage;
 class SkSurface;
 
@@ -35,7 +34,7 @@ sk_sp<SkSurface> CreateSkiaSurfaceForTesting(const gfx::Size& size,
                                              SkColor color);
 
 // Creates a Skia image with dimensions `size` and filled with `color`.
-SkBitmap CreateSkiaImageForTesting(const gfx::Size& size, SkColor color);
+sk_sp<SkImage> CreateSkiaImageForTesting(const gfx::Size& size, SkColor color);
 
 }  // namespace chrome_pdf