Share SkImage across PDF paint rectangles

Shares a single SkImage across all paint rectangles requested in a call
to chrome_pdf::PdfViewPluginBase::DoPaint(). The existing implementation
creates a new SkImage for each requested paint rectangle, potentially
triggering many copies of the backing SkBitmap.

Fixed: 1284255
Change-Id: Ife0afa25011a1444e9421041f2879bd6aaebeadc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3621980
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998606}

pdf/pdf_view_plugin_base.cc

@@ -1389,13 +1389,6 @@ 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) {
@@ -1411,6 +1404,7 @@ void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
 
   engine()->PrePaint();
 
+  std::vector<gfx::Rect> ready_rects;
   for (const gfx::Rect& paint_rect : paint_rects) {
     // Intersect with plugin area since there could be pending invalidates from
     // when the plugin area was larger.
@@ -1427,10 +1421,9 @@ 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.emplace_back(ready_rect, painted_image);
+        ready_rects.push_back(ready_rect);
       }
       for (gfx::Rect& pending_rect : pdf_pending) {
         pending_rect.Offset(available_area_.OffsetFromOrigin());
@@ -1445,8 +1438,8 @@ 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.emplace_back(region, image_data_.asImage());
       image_data_.erase(background_color_, gfx::RectToSkIRect(region));
+      ready_rects.push_back(region);
     }
 
     // Ensure the background parts are filled.
@@ -1456,13 +1449,19 @@ void PdfViewPluginBase::DoPaint(const std::vector<gfx::Rect>& paint_rects,
       if (!intersection.IsEmpty()) {
         image_data_.erase(background_part.color,
                           gfx::RectToSkIRect(intersection));
-        ready.emplace_back(intersection, image_data_.asImage());
+        ready_rects.push_back(intersection);
       }
     }
   }
 
   engine()->PostPaint();
 
+  // TODO(crbug.com/1263614): Write pixels directly to the `SkSurface` in
+  // `PaintManager`, rather than using an intermediate `SkBitmap` and `SkImage`.
+  sk_sp<SkImage> painted_image = image_data_.asImage();
+  for (const gfx::Rect& ready_rect : ready_rects)
+    ready.emplace_back(ready_rect, painted_image);
+
   InvalidateAfterPaintDone();
 }
 

pdf/pdf_view_web_plugin_unittest.cc

@@ -21,9 +21,9 @@
 #include "cc/paint/paint_canvas.h"
 #include "cc/test/pixel_comparator.h"
 #include "cc/test/pixel_test_utils.h"
+#include "pdf/paint_ready_rect.h"
 #include "pdf/test/test_helpers.h"
 #include "pdf/test/test_pdfium_engine.h"
-#include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/input/web_coalesced_input_event.h"
@@ -63,10 +63,12 @@ namespace {
 using ::testing::Eq;
 using ::testing::InSequence;
 using ::testing::Invoke;
+using ::testing::IsEmpty;
 using ::testing::MockFunction;
 using ::testing::NiceMock;
 using ::testing::Pointwise;
 using ::testing::Return;
+using ::testing::SizeIs;
 
 // `kCanvasSize` needs to be big enough to hold plugin's snapshots during
 // testing.
@@ -571,6 +573,41 @@ TEST_F(PdfViewWebPluginTest, PaintSnapshotsWithVariousRectPositions) {
   }
 }
 
+TEST_F(PdfViewWebPluginTest, OnPaintWithMultiplePaintRects) {
+  SetDocumentDimensions({100, 200});
+  UpdatePluginGeometryWithoutWaiting(/*device_scale=*/1.0f,
+                                     gfx::Rect(0, 0, 40, 40));
+
+  EXPECT_CALL(*engine_ptr_, Paint)
+      .WillRepeatedly(
+          [](const gfx::Rect& rect, SkBitmap& /*image_data*/,
+             std::vector<gfx::Rect>& ready,
+             std::vector<gfx::Rect>& /*pending*/) { ready.push_back(rect); });
+  std::vector<PaintReadyRect> ready;
+  std::vector<gfx::Rect> pending;
+  plugin_->OnPaint(
+      /*paint_rects=*/{gfx::Rect(5, 5, 10, 10), gfx::Rect(20, 20, 10, 10)},
+      ready, pending);
+
+  // Expect three paints: an initial background-clearing paint, and one for each
+  // requested paint rectangle.
+  ASSERT_THAT(ready, SizeIs(3));
+  EXPECT_THAT(pending, IsEmpty());
+
+  EXPECT_EQ(gfx::Rect(0, 0, 90, 90), ready[0].rect());
+  EXPECT_TRUE(ready[0].flush_now());
+
+  EXPECT_EQ(gfx::Rect(5, 5, 10, 10), ready[1].rect());
+  EXPECT_FALSE(ready[1].flush_now());
+
+  EXPECT_EQ(gfx::Rect(20, 20, 10, 10), ready[2].rect());
+  EXPECT_FALSE(ready[2].flush_now());
+
+  // All the requested paints should share the same `SkImage`.
+  EXPECT_NE(&ready[0].image(), &ready[1].image());
+  EXPECT_EQ(&ready[1].image(), &ready[2].image());
+}
+
 TEST_F(PdfViewWebPluginTest, UpdateLayerTransformWithIdentity) {
   plugin_->UpdateLayerTransform(1.0f, gfx::Vector2dF());
   TestPaintSnapshots(/*device_scale=*/4.0f,

pdf/test/test_pdfium_engine.h

@@ -42,6 +42,14 @@ class TestPDFiumEngine : public PDFiumEngine {
   MOCK_METHOD(void, ScrolledToXPosition, (int), (override));
   MOCK_METHOD(void, ScrolledToYPosition, (int), (override));
 
+  MOCK_METHOD(void,
+              Paint,
+              (const gfx::Rect&,
+               SkBitmap&,
+               std::vector<gfx::Rect>&,
+               std::vector<gfx::Rect>&),
+              (override));
+
   MOCK_METHOD(bool,
               HandleInputEvent,
               (const blink::WebInputEvent&),
@@ -72,6 +80,8 @@ class TestPDFiumEngine : public PDFiumEngine {
   // Returns an empty bookmark list.
   base::Value::List GetBookmarks() override;
 
+  MOCK_METHOD(gfx::Rect, GetPageScreenRect, (int), (const override));
+
   MOCK_METHOD(void, SetGrayscale, (bool), (override));
 
   uint32_t GetLoadedByteSize() override;