[Skia] Fix rendering issue for converting 1 bpp image source
This CL makes the following changes to fix the process of upsampling
non-grayscale 1 bit-per-pixel images:
- Move the process for handling 1-bpp image from
Fill32BppDestStorageWithPalette() to a new helper function
Fill32BppDestStorageWith1BppSource().
- For a 1 bit-per-pixel image, even if it's not a grayscale image, it
should have only two colors. Remove the process of building a new
palette with the size of 256 since it's not needed.
- As BuildPaletteFrom1BppSource() is no longer needed outside of
core/fxge/dib/cfx_imagestretcher.cpp, move it into anonymous
namespace.
Bug: chromium:1383708, chromium:1400882
Change-Id: I77557ed3b61b745b5ee98def5dc359ba80b8f85f
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/102610
Commit-Queue: Nigi <nigi@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fxge/dib/cfx_imagestretcher.cpp b/core/fxge/dib/cfx_imagestretcher.cpp
index 61af22a..f9b828a 100644
--- a/core/fxge/dib/cfx_imagestretcher.cpp
+++ b/core/fxge/dib/cfx_imagestretcher.cpp
@@ -34,31 +34,16 @@
return format;
}
-} // namespace
+// Builds a new palette with a size of `CFX_DIBBase::kPaletteSize` from the
+// existing palette in `source`. Note: The caller must make sure that the
+// parameters meet the following conditions:
+// source - The format must be `FXDIB_Format::k1bppRgb` and it must
+// have a palette.
+// palette_span - The size must be `CFX_DIBBase::kPaletteSize` to be able
+// to hold the new palette.
-CFX_ImageStretcher::CFX_ImageStretcher(
- ScanlineComposerIface* pDest,
- const RetainPtr<const CFX_DIBBase>& pSource,
- int dest_width,
- int dest_height,
- const FX_RECT& bitmap_rect,
- const FXDIB_ResampleOptions& options)
- : m_pDest(pDest),
- m_pSource(pSource),
- m_ResampleOptions(options),
- m_DestWidth(dest_width),
- m_DestHeight(dest_height),
- m_ClipRect(bitmap_rect),
- m_DestFormat(GetStretchedFormat(*pSource)) {
- DCHECK(m_ClipRect.Valid());
-}
-
-CFX_ImageStretcher::~CFX_ImageStretcher() = default;
-
-// static
-void CFX_ImageStretcher::BuildPaletteFrom1BppSource(
- const RetainPtr<const CFX_DIBBase>& source,
- pdfium::span<FX_ARGB> palette_span) {
+void BuildPaletteFrom1BppSource(const RetainPtr<const CFX_DIBBase>& source,
+ pdfium::span<FX_ARGB> palette_span) {
DCHECK_EQ(FXDIB_Format::k1bppRgb, source->GetFormat());
DCHECK(source->HasPalette());
DCHECK_EQ(CFX_DIBBase::kPaletteSize, palette_span.size());
@@ -84,6 +69,27 @@
}
}
+} // namespace
+
+CFX_ImageStretcher::CFX_ImageStretcher(
+ ScanlineComposerIface* pDest,
+ const RetainPtr<const CFX_DIBBase>& pSource,
+ int dest_width,
+ int dest_height,
+ const FX_RECT& bitmap_rect,
+ const FXDIB_ResampleOptions& options)
+ : m_pDest(pDest),
+ m_pSource(pSource),
+ m_ResampleOptions(options),
+ m_DestWidth(dest_width),
+ m_DestHeight(dest_height),
+ m_ClipRect(bitmap_rect),
+ m_DestFormat(GetStretchedFormat(*pSource)) {
+ DCHECK(m_ClipRect.Valid());
+}
+
+CFX_ImageStretcher::~CFX_ImageStretcher() = default;
+
bool CFX_ImageStretcher::Start() {
if (m_DestWidth == 0 || m_DestHeight == 0)
return false;
diff --git a/core/fxge/dib/cfx_imagestretcher.h b/core/fxge/dib/cfx_imagestretcher.h
index 58c5634..7e51dc7 100644
--- a/core/fxge/dib/cfx_imagestretcher.h
+++ b/core/fxge/dib/cfx_imagestretcher.h
@@ -14,7 +14,6 @@
#include "core/fxcrt/unowned_ptr.h"
#include "core/fxge/dib/fx_dib.h"
#include "core/fxge/dib/scanlinecomposer_iface.h"
-#include "third_party/base/span.h"
class CFX_DIBBase;
class CStretchEngine;
@@ -30,17 +29,6 @@
const FXDIB_ResampleOptions& options);
~CFX_ImageStretcher();
- // Builds a new palette with a size of `CFX_DIBBase::kPaletteSize` from the
- // existing palette in `source`. Note: The caller must make sure that the
- // parameters meet the following conditions:
- // source - The format must be `FXDIB_Format::k1bppRgb` and it must
- // have a palette.
- // palette_span - The size must be `CFX_DIBBase::kPaletteSize` to be able
- // to hold the new palette.
- static void BuildPaletteFrom1BppSource(
- const RetainPtr<const CFX_DIBBase>& source,
- pdfium::span<FX_ARGB> palette_span);
-
bool Start();
bool Continue(PauseIndicatorIface* pPause);
diff --git a/core/fxge/skia/fx_skia_device.cpp b/core/fxge/skia/fx_skia_device.cpp
index e64fc14..449f6b0 100644
--- a/core/fxge/skia/fx_skia_device.cpp
+++ b/core/fxge/skia/fx_skia_device.cpp
@@ -39,7 +39,6 @@
#include "core/fxge/cfx_textrenderoptions.h"
#include "core/fxge/dib/cfx_dibitmap.h"
#include "core/fxge/dib/cfx_imagerenderer.h"
-#include "core/fxge/dib/cfx_imagestretcher.h"
#include "core/fxge/dib/cstretchengine.h"
#include "core/fxge/dib/fx_dib.h"
#include "core/fxge/text_char_pos.h"
@@ -155,11 +154,39 @@
FXARGB_R(color) == FXARGB_B(color);
}
+// Called by Upsample, return a 32 bit-per-pixel buffer filled with 2 colors
+// from a 1 bit-per-pixel source palette.
+DataVector<uint32_t> Fill32BppDestStorageWith1BppSource(
+ const RetainPtr<CFX_DIBBase>& source) {
+ DCHECK_EQ(1, source->GetBPP());
+ int width = source->GetWidth();
+ int height = source->GetHeight();
+ void* buffer = source->GetBuffer().data();
+ DCHECK(buffer);
+
+ uint32_t color0 = source->GetPaletteArgb(0);
+ uint32_t color1 = source->GetPaletteArgb(1);
+ DataVector<uint32_t> dst32_storage(Fx2DSizeOrDie(width, height));
+ pdfium::span<SkPMColor> dst32_pixels(dst32_storage);
+
+ for (int y = 0; y < height; ++y) {
+ const uint8_t* src_row =
+ static_cast<const uint8_t*>(buffer) + y * source->GetPitch();
+ pdfium::span<uint32_t> dst_row = dst32_pixels.subspan(y * width);
+ for (int x = 0; x < width; ++x) {
+ bool use_color1 = src_row[x / 8] & (1 << (7 - x % 8));
+ dst_row[x] = use_color1 ? color1 : color0;
+ }
+ }
+ return dst32_storage;
+}
+
// Called by Upsample(), returns a 32 bit-per-pixel buffer filled with colors
// from `palette`.
DataVector<uint32_t> Fill32BppDestStorageWithPalette(
const RetainPtr<CFX_DIBBase>& source,
pdfium::span<const uint32_t> palette) {
+ DCHECK_EQ(8, source->GetBPP());
int width = source->GetWidth();
int height = source->GetHeight();
void* buffer = source->GetBuffer().data();
@@ -593,22 +620,12 @@
if (pSource->GetFormat() == FXDIB_Format::k1bppRgb &&
pSource->HasPalette()) {
- // When `pSource` has 1 bit per pixel, its palette will contain 2
- // colors, and the R,G,B channels of either color will have equal
- // values. However, there are cases that R,G,B are not all the same and
- // these 2 colors stand for the boundaries of a range of colors to be
- // used for bitmap rendering. We need to handle the color palette
- // differently depending on these conditions.
uint32_t palette_color0 = pSource->GetPaletteArgb(0);
uint32_t palette_color1 = pSource->GetPaletteArgb(1);
- bool use_two_colors = IsRGBColorGrayScale(palette_color0) &&
- IsRGBColorGrayScale(palette_color1);
- if (!use_two_colors) {
- // If more than 2 colors are provided, calculate the range of colors
- // and store them in `new_palette`
- FX_ARGB new_palette[CFX_DIBBase::kPaletteSize];
- CFX_ImageStretcher::BuildPaletteFrom1BppSource(pSource, new_palette);
- dst32_storage = Fill32BppDestStorageWithPalette(pSource, new_palette);
+ bool use_gray_colors = IsRGBColorGrayScale(palette_color0) &&
+ IsRGBColorGrayScale(palette_color1);
+ if (!use_gray_colors) {
+ dst32_storage = Fill32BppDestStorageWith1BppSource(pSource);
buffer = dst32_storage.data();
rowBytes = width * sizeof(uint32_t);
colorType = Get32BitSkColorType(bRgbByteOrder);
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS
index 9768236..f022c38 100644
--- a/testing/SUPPRESSIONS
+++ b/testing/SUPPRESSIONS
@@ -491,9 +491,6 @@
# TODO(chromium:1356149): Remove after associated bug is fixed
bug_1356149.in mac * * agg
-# TODO(chromium:1383708): Remove after associated bug is fixed
-bug_1383708.in * * * skia
-
# TODO(pdfium:1457): Remove after associated bug is fixed
bug_1457.in * * * *
diff --git a/testing/resources/pixel/bug_1383708_expected_skia.pdf.0.png b/testing/resources/pixel/bug_1383708_expected_skia.pdf.0.png
new file mode 100644
index 0000000..36119e0
--- /dev/null
+++ b/testing/resources/pixel/bug_1383708_expected_skia.pdf.0.png
Binary files differ
