Improve SkPictureRecorder ownership
Uses std::unique_ptr to improve the ownership handling for
SkPictureRecorder. This fixes an issue with CFX_SkiaDeviceDriver
creating, and then leaking, the SkPictureRecorder.
Bug: pdfium:1929
Change-Id: Idd4ae80d6bb9b1c5ff08298748f99743c606aaa2
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/100750
Auto-Submit: K. Moon <kmoon@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fxge/cfx_defaultrenderdevice.h b/core/fxge/cfx_defaultrenderdevice.h
index 028612e..0dcd4c2 100644
--- a/core/fxge/cfx_defaultrenderdevice.h
+++ b/core/fxge/cfx_defaultrenderdevice.h
@@ -7,11 +7,14 @@
#ifndef CORE_FXGE_CFX_DEFAULTRENDERDEVICE_H_
#define CORE_FXGE_CFX_DEFAULTRENDERDEVICE_H_
+#include <memory>
+
#include "core/fxcrt/retain_ptr.h"
#include "core/fxge/cfx_renderdevice.h"
#include "core/fxge/dib/fx_dib.h"
class SkPictureRecorder;
+struct SkRect;
class CFX_DefaultRenderDevice final : public CFX_RenderDevice {
public:
@@ -33,7 +36,7 @@
#if defined(_SKIA_SUPPORT_)
bool AttachRecorder(SkPictureRecorder* recorder);
void Clear(uint32_t color);
- SkPictureRecorder* CreateRecorder(int size_x, int size_y);
+ std::unique_ptr<SkPictureRecorder> CreateRecorder(const SkRect& bounds);
void DebugVerifyBitmapIsPreMultiplied() const override;
bool SetBitsWithMask(const RetainPtr<CFX_DIBBase>& pBitmap,
const RetainPtr<CFX_DIBBase>& pMask,
diff --git a/core/fxge/skia/fx_skia_device.cpp b/core/fxge/skia/fx_skia_device.cpp
index 57eb781..772de30 100644
--- a/core/fxge/skia/fx_skia_device.cpp
+++ b/core/fxge/skia/fx_skia_device.cpp
@@ -1740,16 +1740,6 @@
}
#if defined(_SKIA_SUPPORT_)
-CFX_SkiaDeviceDriver::CFX_SkiaDeviceDriver(int size_x, int size_y)
- : m_pBitmap(nullptr),
- m_pBackdropBitmap(nullptr),
- m_pRecorder(new SkPictureRecorder),
- m_pCache(std::make_unique<SkiaState>(this)),
- m_bGroupKnockout(false) {
- m_pRecorder->beginRecording(SkIntToScalar(size_x), SkIntToScalar(size_y));
- m_pCanvas = m_pRecorder->getRecordingCanvas();
-}
-
CFX_SkiaDeviceDriver::CFX_SkiaDeviceDriver(SkPictureRecorder* recorder)
: m_pBitmap(nullptr),
m_pBackdropBitmap(nullptr),
@@ -2880,11 +2870,12 @@
skDriver->Clear(color);
}
-SkPictureRecorder* CFX_DefaultRenderDevice::CreateRecorder(int size_x,
- int size_y) {
- auto driver = std::make_unique<CFX_SkiaDeviceDriver>(size_x, size_y);
- SkPictureRecorder* recorder = driver->GetRecorder();
- SetDeviceDriver(std::move(driver));
+std::unique_ptr<SkPictureRecorder> CFX_DefaultRenderDevice::CreateRecorder(
+ const SkRect& bounds) {
+ auto recorder = std::make_unique<SkPictureRecorder>();
+ recorder->beginRecording(bounds);
+
+ SetDeviceDriver(std::make_unique<CFX_SkiaDeviceDriver>(recorder.get()));
return recorder;
}
#endif // defined(_SKIA_SUPPORT_)
diff --git a/core/fxge/skia/fx_skia_device.h b/core/fxge/skia/fx_skia_device.h
index 449927c..3a8fca6 100644
--- a/core/fxge/skia/fx_skia_device.h
+++ b/core/fxge/skia/fx_skia_device.h
@@ -32,7 +32,6 @@
bool bGroupKnockout);
#if defined(_SKIA_SUPPORT_)
explicit CFX_SkiaDeviceDriver(SkPictureRecorder* recorder);
- CFX_SkiaDeviceDriver(int size_x, int size_y);
#endif
~CFX_SkiaDeviceDriver() override;
@@ -156,7 +155,6 @@
const SkMatrix& matrix);
void Clear(uint32_t color);
void Flush() override;
- SkPictureRecorder* GetRecorder() const { return m_pRecorder; }
void PreMultiply();
static void PreMultiply(const RetainPtr<CFX_DIBitmap>& pDIBitmap);
SkCanvas* SkiaCanvas() { return m_pCanvas; }
diff --git a/fpdfsdk/BUILD.gn b/fpdfsdk/BUILD.gn
index 20eebb9..a1fc16b 100644
--- a/fpdfsdk/BUILD.gn
+++ b/fpdfsdk/BUILD.gn
@@ -97,6 +97,10 @@
]
allow_circular_includes_from += [ "fpdfxfa" ]
}
+
+ if (pdf_use_skia || pdf_use_skia_paths) {
+ deps += [ "//skia" ]
+ }
}
pdfium_unittest_source_set("unittests") {
@@ -158,4 +162,8 @@
"../core/fxge",
]
pdfium_root_dir = "../"
+
+ if (pdf_use_skia || pdf_use_skia_paths) {
+ deps += [ "//skia" ]
+ }
}
diff --git a/fpdfsdk/DEPS b/fpdfsdk/DEPS
index 0e367fb..14d2700 100644
--- a/fpdfsdk/DEPS
+++ b/fpdfsdk/DEPS
@@ -2,6 +2,7 @@
'+core',
'+fxjs',
'+public',
+ '+third_party/skia/include',
'+v8',
'+xfa/fgas/font',
'+xfa/fgas/graphics',
diff --git a/fpdfsdk/fpdf_view.cpp b/fpdfsdk/fpdf_view.cpp
index 211de79..8f93c66 100644
--- a/fpdfsdk/fpdf_view.cpp
+++ b/fpdfsdk/fpdf_view.cpp
@@ -51,6 +51,11 @@
#include "third_party/base/ptr_util.h"
#include "third_party/base/span.h"
+#ifdef _SKIA_SUPPORT_
+#include "third_party/skia/include/core/SkPictureRecorder.h"
+#include "third_party/skia/include/core/SkRect.h"
+#endif // _SKIA_SUPPORT_
+
#ifdef PDF_ENABLE_V8
#include "fxjs/cfx_v8_array_buffer_allocator.h"
#include "third_party/base/no_destructor.h"
@@ -735,14 +740,15 @@
pPage->SetRenderContext(std::move(pOwnedContext));
auto skDevice = std::make_unique<CFX_DefaultRenderDevice>();
- FPDF_RECORDER recorder = skDevice->CreateRecorder(size_x, size_y);
+ std::unique_ptr<SkPictureRecorder> recorder =
+ skDevice->CreateRecorder(SkRect::MakeWH(size_x, size_y));
pContext->m_pDevice = std::move(skDevice);
CPDFSDK_RenderPageWithContext(pContext, pPage, 0, 0, size_x, size_y, 0, 0,
/*color_scheme=*/nullptr,
/*need_to_restore=*/true, /*pause=*/nullptr);
- return recorder;
+ return recorder.release();
}
#endif // defined(_SKIA_SUPPORT_)
diff --git a/fpdfsdk/fpdf_view_embeddertest.cpp b/fpdfsdk/fpdf_view_embeddertest.cpp
index 13f1f89..0f3ccbd 100644
--- a/fpdfsdk/fpdf_view_embeddertest.cpp
+++ b/fpdfsdk/fpdf_view_embeddertest.cpp
@@ -7,6 +7,7 @@
#include <limits>
#include <memory>
#include <string>
+#include <utility>
#include <vector>
#include "build/build_config.h"
@@ -26,6 +27,19 @@
#include "testing/utils/path_service.h"
#include "third_party/base/check.h"
+#ifdef _SKIA_SUPPORT_
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "third_party/skia/include/core/SkColorType.h"
+#include "third_party/skia/include/core/SkImage.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
+#include "third_party/skia/include/core/SkPicture.h"
+#include "third_party/skia/include/core/SkPictureRecorder.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/core/SkSize.h"
+#include "third_party/skia/include/core/SkSurface.h"
+#endif // _SKIA_SUPPORT_
+
using pdfium::ManyRectanglesChecksum;
namespace {
@@ -95,6 +109,48 @@
~MockDownloadHints() = default;
};
+#ifdef _SKIA_SUPPORT_
+ScopedFPDFBitmap SkImageToPdfiumBitmap(const SkImage& image) {
+ ScopedFPDFBitmap bitmap(
+ FPDFBitmap_Create(image.width(), image.height(), /*alpha=*/1));
+ if (!bitmap) {
+ ADD_FAILURE() << "Could not create FPDF_BITMAP";
+ return nullptr;
+ }
+
+ if (!image.readPixels(/*context=*/nullptr,
+ image.imageInfo().makeColorType(kBGRA_8888_SkColorType),
+ FPDFBitmap_GetBuffer(bitmap.get()),
+ FPDFBitmap_GetStride(bitmap.get()),
+ /*srcX=*/0, /*srcY=*/0)) {
+ ADD_FAILURE() << "Could not read pixels from SkImage";
+ return nullptr;
+ }
+
+ return bitmap;
+}
+
+ScopedFPDFBitmap SkPictureToPdfiumBitmap(sk_sp<SkPicture> picture,
+ const SkISize& size) {
+ sk_sp<SkSurface> surface =
+ SkSurface::MakeRasterN32Premul(size.width(), size.height());
+ if (!surface) {
+ ADD_FAILURE() << "Could not create SkSurface";
+ return nullptr;
+ }
+
+ surface->getCanvas()->clear(SK_ColorWHITE);
+ surface->getCanvas()->drawPicture(picture);
+ sk_sp<SkImage> image = surface->makeImageSnapshot();
+ if (!image) {
+ ADD_FAILURE() << "Could not snapshot SkSurface";
+ return nullptr;
+ }
+
+ return SkImageToPdfiumBitmap(*image);
+}
+#endif // _SKIA_SUPPORT_
+
} // namespace
TEST(fpdf, CApiTest) {
@@ -1892,3 +1948,35 @@
ASSERT_EQ(size, FPDF_GetTrailerEnds(document(), ends.data(), size));
EXPECT_EQ(kExpectedEnds, ends);
}
+
+#ifdef _SKIA_SUPPORT_
+TEST_F(FPDFViewEmbedderTest, RenderPageToSkp) {
+ if (!CFX_DefaultRenderDevice::SkiaIsDefaultRenderer())
+ GTEST_SKIP() << "FPDF_RenderPageSkp() only makes sense with Skia";
+
+ ASSERT_TRUE(OpenDocument("rectangles.pdf"));
+
+ FPDF_PAGE page = LoadPage(0);
+ ASSERT_TRUE(page);
+
+ constexpr SkISize kOutputSize = SkISize::Make(200, 300);
+
+ FPDF_RECORDER opaque_recorder =
+ FPDF_RenderPageSkp(page, kOutputSize.width(), kOutputSize.height());
+ UnloadPage(page);
+ ASSERT_TRUE(opaque_recorder);
+
+ SkPictureRecorder* recorder =
+ reinterpret_cast<SkPictureRecorder*>(opaque_recorder);
+ sk_sp<SkPicture> picture = recorder->finishRecordingAsPicture();
+ delete recorder;
+ ASSERT_TRUE(picture);
+
+ ScopedFPDFBitmap bitmap =
+ SkPictureToPdfiumBitmap(std::move(picture), kOutputSize);
+ ASSERT_TRUE(bitmap);
+
+ CompareBitmap(bitmap.get(), kOutputSize.width(), kOutputSize.height(),
+ pdfium::RectanglesChecksum());
+}
+#endif // _SKIA_SUPPORT_
