Don't implement CPDF_DIB::GetBuffer() (try 2)
The first try [1] to just return an empty span in CPDF_DIB::GetBuffer()
did not work. This CL goes further and deletes CFX_DIBBase::GetBuffer()
altogether. Instead, only implement a non-virtual
CFX_DIBBitmap::GetBuffer(). To make this work, get rid of all
CFX_DIBBase::GetBuffer() callers:
- Switch to calling CFX_DIBBitmap::GetBuffer() when it easy to do so.
- Make CFX_DIBBase::RealizeIfNeeded() virtual, and change the default
implementation to just call Realize().
- Make RealizeIfNeeded() available for Skia-builds and use it in
CreateSkiaImageFromDib().
- Change CreateSkiaImageFromTransformedDib() to only use scanlines.
Fix other issues to make sure everything works:
- Avoid calling RealizeIfNeeded() on `CPDF_DIB::m_pCachedBitmap`, as
that gives the wrong answer, just like CPDF_DIB::GetBuffer().
- Make sure CreateSkiaImageFromDib() does not fail and leak memory.
- Give up on avoiding the CachedImage::SkipToScanline() call.
- Add a test case for a related bug.
[1] https://pdfium-review.googlesource.com/108850
Bug: chromium:1478366,pdfium:2050,pdfium:2051
Change-Id: Ia20d4d257d441dd971ee0a912733f48f29dafad1
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/111434
Reviewed-by: Nigi <nigi@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/page/cpdf_dib.cpp b/core/fpdfapi/page/cpdf_dib.cpp
index c478d37..657cf16 100644
--- a/core/fpdfapi/page/cpdf_dib.cpp
+++ b/core/fpdfapi/page/cpdf_dib.cpp
@@ -1132,11 +1132,6 @@
return true;
}
-pdfium::span<const uint8_t> CPDF_DIB::GetBuffer() const {
- return m_pCachedBitmap ? m_pCachedBitmap->GetBuffer()
- : pdfium::span<const uint8_t>();
-}
-
pdfium::span<const uint8_t> CPDF_DIB::GetScanline(int line) const {
if (m_bpc == 0)
return pdfium::span<const uint8_t>();
diff --git a/core/fpdfapi/page/cpdf_dib.h b/core/fpdfapi/page/cpdf_dib.h
index 5206bf5..1ef656a 100644
--- a/core/fpdfapi/page/cpdf_dib.h
+++ b/core/fpdfapi/page/cpdf_dib.h
@@ -45,7 +45,6 @@
CONSTRUCT_VIA_MAKE_RETAIN;
// CFX_DIBBase:
- pdfium::span<const uint8_t> GetBuffer() const override;
pdfium::span<const uint8_t> GetScanline(int line) const override;
bool SkipToScanline(int line, PauseIndicatorIface* pPause) const override;
size_t GetEstimatedImageMemoryBurden() const override;
diff --git a/core/fpdfapi/page/cpdf_pageimagecache.cpp b/core/fpdfapi/page/cpdf_pageimagecache.cpp
index b36f089..30d6aa1 100644
--- a/core/fpdfapi/page/cpdf_pageimagecache.cpp
+++ b/core/fpdfapi/page/cpdf_pageimagecache.cpp
@@ -28,7 +28,6 @@
#if defined(_SKIA_SUPPORT_)
#include "core/fxcrt/data_vector.h"
#include "core/fxge/cfx_defaultrenderdevice.h"
-#include "third_party/base/notreached.h"
#include "third_party/skia/include/core/SkImage.h" // nogncheck
#include "third_party/skia/include/core/SkRefCnt.h" // nogncheck
#endif
@@ -63,15 +62,6 @@
}
}
- pdfium::span<const uint8_t> GetBuffer() const override {
- // TODO(crbug.com/pdfium/2051): Still needed for
- // CGdiDeviceDriver::GDI_StretchDIBits(). `CachedImage` is only used when
- // Skia is the default renderer, which should call `RealizeSkImage()`
- // instead. Consider removing this or making it `NOTREACHED_NORETURN()`
- // by reimplementing its caller when Skia is the default renderer.
- return image_->GetBuffer();
- }
-
pdfium::span<const uint8_t> GetScanline(int line) const override {
// TODO(crbug.com/pdfium/2050): Still needed for `Realize()` call in
// `CPDF_ImageRenderer`.
@@ -79,10 +69,6 @@
}
bool SkipToScanline(int line, PauseIndicatorIface* pause) const override {
- // TODO(crbug.com/pdfium/2051): `CachedImage` is only used by Skia, which
- // should call `RealizeSkImage()` instead. Consider removing this, or at
- // least making it `NOTREACHED_NORETURN()`.
- NOTREACHED();
return image_->SkipToScanline(line, pause);
}
@@ -91,6 +77,12 @@
return image_->GetEstimatedImageMemoryBurden();
}
+#if BUILDFLAG(IS_WIN) || defined(_SKIA_SUPPORT_)
+ RetainPtr<const CFX_DIBitmap> RealizeIfNeeded() const override {
+ return image_->RealizeIfNeeded();
+ }
+#endif
+
sk_sp<SkImage> RealizeSkImage() const override {
if (!cached_skia_image_) {
cached_skia_image_ = image_->RealizeSkImage();
@@ -111,15 +103,7 @@
bool realize_hint) {
#if defined(_SKIA_SUPPORT_)
if (CFX_DefaultRenderDevice::SkiaIsDefaultRenderer()) {
- // TODO(crbug.com/pdfium/2050): Ignore `realize_hint`, as `RealizeSkImage()`
- // doesn't benefit from it. The current behavior masks a bug in `CPDF_DIB`
- // in which `GetBuffer()` and `GetScanline()` don't give the same answer.
- if (realize_hint) {
- image = image->Realize();
- if (!image) {
- return nullptr;
- }
- }
+ // Ignore `realize_hint`, as `RealizeSkImage()` doesn't benefit from it.
return pdfium::MakeRetain<CachedImage>(std::move(image));
}
#endif // defined(_SKIA_SUPPORT_)
diff --git a/core/fxge/dib/cfx_dibbase.cpp b/core/fxge/dib/cfx_dibbase.cpp
index 2a80f89..bf823dc 100644
--- a/core/fxge/dib/cfx_dibbase.cpp
+++ b/core/fxge/dib/cfx_dibbase.cpp
@@ -617,10 +617,6 @@
CFX_DIBBase::~CFX_DIBBase() = default;
-pdfium::span<const uint8_t> CFX_DIBBase::GetBuffer() const {
- return pdfium::span<const uint8_t>();
-}
-
bool CFX_DIBBase::SkipToScanline(int line, PauseIndicatorIface* pPause) const {
return false;
}
@@ -629,12 +625,9 @@
return GetRequiredPaletteSize() * sizeof(uint32_t);
}
-#if BUILDFLAG(IS_WIN)
-RetainPtr<const CFX_DIBBase> CFX_DIBBase::RealizeIfNeeded() const {
- if (GetBuffer().empty()) {
- return Realize();
- }
- return pdfium::WrapRetain(this);
+#if BUILDFLAG(IS_WIN) || defined(_SKIA_SUPPORT_)
+RetainPtr<const CFX_DIBitmap> CFX_DIBBase::RealizeIfNeeded() const {
+ return Realize();
}
#endif
diff --git a/core/fxge/dib/cfx_dibbase.h b/core/fxge/dib/cfx_dibbase.h
index 6327888..98febad 100644
--- a/core/fxge/dib/cfx_dibbase.h
+++ b/core/fxge/dib/cfx_dibbase.h
@@ -41,10 +41,13 @@
static constexpr uint32_t kPaletteSize = 256;
- virtual pdfium::span<const uint8_t> GetBuffer() const;
virtual pdfium::span<const uint8_t> GetScanline(int line) const = 0;
virtual bool SkipToScanline(int line, PauseIndicatorIface* pPause) const;
virtual size_t GetEstimatedImageMemoryBurden() const;
+#if BUILDFLAG(IS_WIN) || defined(_SKIA_SUPPORT_)
+ // Calls Realize() if needed. Otherwise, return `this`.
+ virtual RetainPtr<const CFX_DIBitmap> RealizeIfNeeded() const;
+#endif
int GetWidth() const { return m_Width; }
int GetHeight() const { return m_Height; }
@@ -65,11 +68,6 @@
// Copies into internally-owned palette.
void SetPalette(pdfium::span<const uint32_t> src_palette);
-#if BUILDFLAG(IS_WIN)
- // Calls Realize() if needed. Otherwise, return `this`.
- RetainPtr<const CFX_DIBBase> RealizeIfNeeded() const;
-#endif
-
RetainPtr<CFX_DIBitmap> Realize() const;
RetainPtr<CFX_DIBitmap> ClipTo(const FX_RECT& rect) const;
RetainPtr<CFX_DIBitmap> ConvertTo(FXDIB_Format format) const;
diff --git a/core/fxge/dib/cfx_dibitmap.cpp b/core/fxge/dib/cfx_dibitmap.cpp
index 6df1eee..cf349c0 100644
--- a/core/fxge/dib/cfx_dibitmap.cpp
+++ b/core/fxge/dib/cfx_dibitmap.cpp
@@ -110,6 +110,15 @@
return result;
}
+#if BUILDFLAG(IS_WIN) || defined(_SKIA_SUPPORT_)
+RetainPtr<const CFX_DIBitmap> CFX_DIBitmap::RealizeIfNeeded() const {
+ if (GetBuffer().empty()) {
+ return Realize();
+ }
+ return pdfium::WrapRetain(this);
+}
+#endif
+
void CFX_DIBitmap::TakeOver(RetainPtr<CFX_DIBitmap>&& pSrcBitmap) {
m_pBuffer = std::move(pSrcBitmap->m_pBuffer);
m_palette = std::move(pSrcBitmap->m_palette);
diff --git a/core/fxge/dib/cfx_dibitmap.h b/core/fxge/dib/cfx_dibitmap.h
index 34f5887..e59644d 100644
--- a/core/fxge/dib/cfx_dibitmap.h
+++ b/core/fxge/dib/cfx_dibitmap.h
@@ -34,10 +34,13 @@
bool Copy(const RetainPtr<CFX_DIBBase>& pSrc);
// CFX_DIBBase
- pdfium::span<const uint8_t> GetBuffer() const override;
pdfium::span<const uint8_t> GetScanline(int line) const override;
size_t GetEstimatedImageMemoryBurden() const override;
+#if BUILDFLAG(IS_WIN) || defined(_SKIA_SUPPORT_)
+ RetainPtr<const CFX_DIBitmap> RealizeIfNeeded() const override;
+#endif
+ pdfium::span<const uint8_t> GetBuffer() const;
pdfium::span<uint8_t> GetWritableBuffer() {
pdfium::span<const uint8_t> src = GetBuffer();
return {const_cast<uint8_t*>(src.data()), src.size()};
diff --git a/core/fxge/skia/cfx_dibbase_skia.cpp b/core/fxge/skia/cfx_dibbase_skia.cpp
index 417aa93..8606e33 100644
--- a/core/fxge/skia/cfx_dibbase_skia.cpp
+++ b/core/fxge/skia/cfx_dibbase_skia.cpp
@@ -33,41 +33,35 @@
namespace {
-// Releases `CFX_DIBBase` "leaked" by `CreateSkiaImageFromDib()`.
+// Releases `CFX_DIBitmap` "leaked" by `CreateSkiaImageFromDib()`.
void ReleaseRetainedHeldBySkImage(const void* /*pixels*/,
SkImages::ReleaseContext context) {
- RetainPtr<const CFX_DIBBase> retained;
- retained.Unleak(reinterpret_cast<const CFX_DIBBase*>(context));
+ RetainPtr<const CFX_DIBitmap> realized_bitmap;
+ realized_bitmap.Unleak(reinterpret_cast<const CFX_DIBitmap*>(context));
}
-// Creates an `SkImage` from a `CFX_DIBBase`, sharing the underlying pixels if
-// possible.
-//
-// Note that an `SkImage` must be immutable, so if sharing pixels, they must not
-// be modified during the lifetime of the `SkImage`.
+// Creates an `SkImage` from a `CFX_DIBBase`.
sk_sp<SkImage> CreateSkiaImageFromDib(const CFX_DIBBase* source,
SkColorType color_type,
SkAlphaType alpha_type) {
// Make sure the DIB is backed by a buffer.
- RetainPtr<const CFX_DIBBase> retained;
- if (source->GetBuffer().empty()) {
- retained = source->Realize();
- if (!retained) {
- return nullptr;
- }
- DCHECK(!retained->GetBuffer().empty());
- } else {
- retained.Reset(source);
+ RetainPtr<const CFX_DIBitmap> realized_bitmap = source->RealizeIfNeeded();
+ if (!realized_bitmap) {
+ return nullptr;
}
+ CHECK(!realized_bitmap->GetBuffer().empty());
- // Convert unowned pointer to a retained pointer, then "leak" to `SkImage`.
- source = retained.Leak();
- SkImageInfo info = SkImageInfo::Make(source->GetWidth(), source->GetHeight(),
+ // Transfer ownership of `realized_bitmap` to `bitmap`, which will be freed by
+ // ReleaseRetainedHeldBySkImage().
+ const CFX_DIBitmap* bitmap = realized_bitmap.Leak();
+ SkImageInfo info = SkImageInfo::Make(bitmap->GetWidth(), bitmap->GetHeight(),
color_type, alpha_type);
- return SkImages::RasterFromPixmap(
- SkPixmap(info, source->GetBuffer().data(), source->GetPitch()),
+ auto result = SkImages::RasterFromPixmap(
+ SkPixmap(info, bitmap->GetBuffer().data(), bitmap->GetPitch()),
/*rasterReleaseProc=*/ReleaseRetainedHeldBySkImage,
- /*releaseContext=*/const_cast<CFX_DIBBase*>(source));
+ /*releaseContext=*/const_cast<CFX_DIBitmap*>(bitmap));
+ CHECK(result); // Otherwise, `bitmap` leaks.
+ return result;
}
// Releases allocated memory "leaked" by `CreateSkiaImageFromTransformedDib()`.
@@ -125,22 +119,6 @@
DCHECK_GE(scanline.size(), min_row_bytes);
}
-void ValidateBufferSize(pdfium::span<const uint8_t> buffer,
- const CFX_DIBBase& source) {
-#if DCHECK_IS_ON()
- if (source.GetHeight() == 0) {
- return;
- }
-
- FX_SAFE_SIZE_T buffer_size = source.GetHeight() - 1;
- buffer_size *= source.GetPitch();
- buffer_size += fxge::CalculatePitch8OrDie(source.GetBPP(), /*components=*/1,
- source.GetWidth());
-
- DCHECK_GE(buffer.size(), buffer_size.ValueOrDie());
-#endif // DCHECK_IS_ON()
-}
-
// Creates an `SkImage` from a `CFX_DIBBase`, transforming the source pixels
// using `pixel_transform`.
//
@@ -167,40 +145,21 @@
return nullptr;
}
- // Transform source pixels to output pixels.
- pdfium::span<const uint8_t> source_buffer = source.GetBuffer();
+ // Transform source pixels to output pixels. Iterate by individual scanline.
Result* output_cursor = reinterpret_cast<Result*>(output.get());
- if (source_buffer.empty()) {
- // No buffer; iterate by individual scanline.
- const size_t min_row_bytes =
- fxge::CalculatePitch8OrDie(source.GetBPP(), /*components=*/1, width);
- DCHECK_LE(min_row_bytes, source.GetPitch());
+ const size_t min_row_bytes =
+ fxge::CalculatePitch8OrDie(source.GetBPP(), /*components=*/1, width);
+ DCHECK_LE(min_row_bytes, source.GetPitch());
- int line = 0;
- for (int row = 0; row < height; ++row) {
- pdfium::span<const uint8_t> scanline = source.GetScanline(line++);
- ValidateScanlineSize(scanline, min_row_bytes);
+ int line = 0;
+ for (int row = 0; row < height; ++row) {
+ pdfium::span<const uint8_t> scanline = source.GetScanline(line++);
+ ValidateScanlineSize(scanline, min_row_bytes);
- for (int column = 0; column < width; ++column) {
- *output_cursor++ =
- Traits::Invoke(std::forward<PixelTransform>(pixel_transform),
- scanline.data(), column);
- }
- }
- } else {
- // Iterate over the entire buffer.
- ValidateBufferSize(source_buffer, source);
- const size_t row_bytes = source.GetPitch();
-
- const uint8_t* next_scanline = source_buffer.data();
- for (int row = 0; row < height; ++row) {
- const uint8_t* scanline = next_scanline;
- next_scanline += row_bytes;
-
- for (int column = 0; column < width; ++column) {
- *output_cursor++ = Traits::Invoke(
- std::forward<PixelTransform>(pixel_transform), scanline, column);
- }
+ for (int column = 0; column < width; ++column) {
+ *output_cursor++ =
+ Traits::Invoke(std::forward<PixelTransform>(pixel_transform),
+ scanline.data(), column);
}
}
diff --git a/core/fxge/win32/cgdi_device_driver.cpp b/core/fxge/win32/cgdi_device_driver.cpp
index e4c2b15..e6e2c21 100644
--- a/core/fxge/win32/cgdi_device_driver.cpp
+++ b/core/fxge/win32/cgdi_device_driver.cpp
@@ -394,7 +394,7 @@
int left,
int top) {
if (m_DeviceType == DeviceType::kPrinter) {
- RetainPtr<const CFX_DIBBase> flipped_source =
+ RetainPtr<const CFX_DIBitmap> flipped_source =
source->FlipImage(/*bXFlip=*/false, /*bYFlip=*/true);
if (!flipped_source) {
return false;
@@ -414,7 +414,7 @@
return true;
}
- RetainPtr<const CFX_DIBBase> realized_source = source->RealizeIfNeeded();
+ RetainPtr<const CFX_DIBitmap> realized_source = source->RealizeIfNeeded();
if (!realized_source) {
return false;
}
@@ -445,7 +445,7 @@
SetStretchBltMode(m_hDC, COLORONCOLOR);
}
- RetainPtr<const CFX_DIBBase> realized_source;
+ RetainPtr<const CFX_DIBitmap> realized_source;
if (m_DeviceType == DeviceType::kPrinter &&
((int64_t)source->GetWidth() * source->GetHeight() >
(int64_t)abs(dest_width) * abs(dest_height))) {
@@ -477,7 +477,7 @@
return false;
}
- RetainPtr<const CFX_DIBBase> realized_source = source->RealizeIfNeeded();
+ RetainPtr<const CFX_DIBitmap> realized_source = source->RealizeIfNeeded();
if (!realized_source) {
return false;
}
diff --git a/core/fxge/win32/cgdi_plus_ext.cpp b/core/fxge/win32/cgdi_plus_ext.cpp
index f6a9181..0cd0a37 100644
--- a/core/fxge/win32/cgdi_plus_ext.cpp
+++ b/core/fxge/win32/cgdi_plus_ext.cpp
@@ -223,7 +223,7 @@
return;
}
- RetainPtr<const CFX_DIBBase> realized_source = source->RealizeIfNeeded();
+ RetainPtr<const CFX_DIBitmap> realized_source = source->RealizeIfNeeded();
if (!realized_source) {
return;
}
diff --git a/testing/resources/pixel/bug_1478366.in b/testing/resources/pixel/bug_1478366.in
new file mode 100644
index 0000000..5749b52
--- /dev/null
+++ b/testing/resources/pixel/bug_1478366.in
@@ -0,0 +1,56 @@
+{{header}}
+{{object 1 0}} <<
+ /Type /Catalog
+ /Pages 2 0 R
+>>
+endobj
+{{object 2 0}} <<
+ /Type /Pages
+ /Count 1
+ /Kids [3 0 R]
+>>
+endobj
+{{object 3 0}} <<
+ /Type /Page
+ /Parent 2 0 R
+ /Contents [4 0 R]
+ /MediaBox [0 0 200 200]
+ /Resources <<
+ /XObject <<
+ /X1 5 0 R
+ >>
+ >>
+>>
+endobj
+{{object 4 0}} <<
+ {{streamlen}}
+>>
+stream
+q
+200 0 0 200 0 0 cm
+/X1 Do
+Q
+endstream
+endobj
+{{object 5 0}} <<
+ /Type /XObject
+ /Subtype /Image
+ % The image needs to have a bitmap size that's at least `kHugeImageSize`.
+ /Width 4000
+ /Height 4000
+ /BitsPerComponent 1
+ /ColorSpace /DeviceGray
+ /Filter [/ASCIIHexDecode /JBIG2Decode]
+ /Mask [255 255]
+ {{streamlen}}
+>>
+stream
+000000003000010000001300000dea00000353000017110000171151000000000001260001000000
+3500000dea000003530000000000000000020003fffdff02fefefeff7f86530fb6c922cfff7fff7f
+ff7fff7fff7fff7fff7fff7fffac
+endstream
+endobj
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
diff --git a/testing/resources/pixel/bug_1478366_expected.pdf.0.png b/testing/resources/pixel/bug_1478366_expected.pdf.0.png
new file mode 100644
index 0000000..23e6296
--- /dev/null
+++ b/testing/resources/pixel/bug_1478366_expected.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/bug_1478366_expected_skia.pdf.0.png b/testing/resources/pixel/bug_1478366_expected_skia.pdf.0.png
new file mode 100644
index 0000000..baf8b4b
--- /dev/null
+++ b/testing/resources/pixel/bug_1478366_expected_skia.pdf.0.png
Binary files differ
