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
