From 95340100f95f248defac47db8d63d6ce57b512d8 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Thu, 2 Aug 2018 21:40:18 +0000 Subject: Tidy JBig2_Image.cpp Add checked/unchecked GetLine(y) methods and use them. Introduce BIT_INDEX_TO_ALIGNED_BYTE() to de-mystify some shifting. Move local declarations to spot of use. Remove spurious Fill(), as we initialize to 0s. Initialize members in header where possible. Add unit tests. Change-Id: I41ccb91b57320dbc790fd0f680f6d98571280343 Reviewed-on: https://pdfium-review.googlesource.com/39370 Commit-Queue: Tom Sepez Reviewed-by: Lei Zhang --- core/fxcodec/jbig2/JBig2_Image.cpp | 102 +++++++------- core/fxcodec/jbig2/JBig2_Image.h | 9 +- core/fxcodec/jbig2/JBig2_Image_unittest.cpp | 204 +++++++++++++++++++++++++++- 3 files changed, 251 insertions(+), 64 deletions(-) diff --git a/core/fxcodec/jbig2/JBig2_Image.cpp b/core/fxcodec/jbig2/JBig2_Image.cpp index 6c61c782f8..f66780aa16 100644 --- a/core/fxcodec/jbig2/JBig2_Image.cpp +++ b/core/fxcodec/jbig2/JBig2_Image.cpp @@ -16,6 +16,7 @@ #include "core/fxcrt/fx_memory.h" #include "core/fxcrt/fx_safe_types.h" #include "third_party/base/ptr_util.h" +#include "third_party/base/stl_util.h" #define JBIG2_GETDWORD(buf) \ ((static_cast((buf)[0]) << 24) | \ @@ -29,6 +30,8 @@ (buf)[2] = static_cast((val) >> 8), \ (buf)[3] = static_cast((val) >> 0)) +#define BIT_INDEX_TO_ALIGNED_BYTE(x) (((x) >> 5) << 2) + namespace { const int kMaxImagePixels = INT_MAX - 31; @@ -36,8 +39,7 @@ const int kMaxImageBytes = kMaxImagePixels / 8; } // namespace -CJBig2_Image::CJBig2_Image(int32_t w, int32_t h) - : m_pData(nullptr), m_nWidth(0), m_nHeight(0), m_nStride(0) { +CJBig2_Image::CJBig2_Image(int32_t w, int32_t h) { if (w <= 0 || h <= 0 || w > kMaxImagePixels) return; @@ -52,9 +54,15 @@ CJBig2_Image::CJBig2_Image(int32_t w, int32_t h) FX_Alloc2D(uint8_t, m_nStride, m_nHeight))); } -CJBig2_Image::CJBig2_Image(int32_t w, int32_t h, int32_t stride, uint8_t* pBuf) - : m_pData(nullptr), m_nWidth(0), m_nHeight(0), m_nStride(0) { - if (w < 0 || h < 0 || stride < 0 || stride > kMaxImageBytes) +CJBig2_Image::CJBig2_Image(int32_t w, + int32_t h, + int32_t stride, + uint8_t* pBuf) { + if (w < 0 || h < 0) + return; + + // Stride must be word-aligned. + if (stride < 0 || stride > kMaxImageBytes || stride % 4 != 0) return; int32_t stride_pixels = 8 * stride; @@ -68,8 +76,7 @@ CJBig2_Image::CJBig2_Image(int32_t w, int32_t h, int32_t stride, uint8_t* pBuf) } CJBig2_Image::CJBig2_Image(const CJBig2_Image& other) - : m_pData(nullptr), - m_nWidth(other.m_nWidth), + : m_nWidth(other.m_nWidth), m_nHeight(other.m_nHeight), m_nStride(other.m_nStride) { if (other.m_pData) { @@ -117,6 +124,10 @@ void CJBig2_Image::SetPixel(int32_t x, int32_t y, int v) { data()[m] &= ~n; } +uint8_t* CJBig2_Image::GetLine(int32_t y) const { + return (y >= 0 && y < m_nHeight) ? GetLineUnsafe(y) : nullptr; +} + void CJBig2_Image::CopyLine(int32_t hTo, int32_t hFrom) { if (!m_pData) return; @@ -170,60 +181,40 @@ std::unique_ptr CJBig2_Image::SubImage(int32_t x, int32_t y, int32_t w, int32_t h) { - int32_t m; - int32_t n; - int32_t j; - uint8_t* pLineSrc; - uint8_t* pLineDst; - uint32_t wTmp; - uint8_t* pSrc; - uint8_t* pSrcEnd; - uint8_t* pDst; - uint8_t* pDstEnd; - if (w == 0 || h == 0) - return nullptr; - auto pImage = pdfium::MakeUnique(w, h); - if (!m_pData) { - pImage->Fill(0); + if (!pImage->data() || !m_pData) return pImage; - } - if (!pImage->m_pData) + + if (x < 0 || x >= m_nWidth || y < 0 || y >= m_nHeight) return pImage; - pLineSrc = data() + m_nStride * y; - pLineDst = pImage->data(); - m = (x >> 5) << 2; - n = x & 31; + int32_t m = BIT_INDEX_TO_ALIGNED_BYTE(x); + int32_t n = x & 31; + int32_t bytes_to_copy = std::min(pImage->m_nStride, m_nStride - m); + int32_t lines_to_copy = std::min(pImage->m_nHeight, m_nHeight - y); + + // Fast case when DWORD-aligned. if (n == 0) { - for (j = 0; j < h; j++) { - pSrc = pLineSrc + m; - pSrcEnd = pLineSrc + m_nStride; - pDst = pLineDst; - pDstEnd = pLineDst + pImage->m_nStride; - for (; pDst < pDstEnd; pSrc += 4, pDst += 4) { - *((uint32_t*)pDst) = *((uint32_t*)pSrc); - } - pLineSrc += m_nStride; - pLineDst += pImage->m_nStride; + for (int32_t j = 0; j < lines_to_copy; j++) { + const uint8_t* pLineSrc = GetLineUnsafe(y + j); + uint8_t* pLineDst = pImage->GetLineUnsafe(j); + memcpy(pLineDst, pLineSrc + m, bytes_to_copy); } - } else { - for (j = 0; j < h; j++) { - pSrc = pLineSrc + m; - pSrcEnd = pLineSrc + m_nStride; - pDst = pLineDst; - pDstEnd = pLineDst + pImage->m_nStride; - for (; pDst < pDstEnd; pSrc += 4, pDst += 4) { - if (pSrc + 4 < pSrcEnd) { - wTmp = (JBIG2_GETDWORD(pSrc) << n) | - (JBIG2_GETDWORD(pSrc + 4) >> (32 - n)); - } else { - wTmp = JBIG2_GETDWORD(pSrc) << n; - } - JBIG2_PUTDWORD(pDst, wTmp); - } - pLineSrc += m_nStride; - pLineDst += pImage->m_nStride; + return pImage; + } + + // Normal slow case. + for (int32_t j = 0; j < lines_to_copy; j++) { + const uint8_t* pLineSrc = GetLineUnsafe(y + j); + uint8_t* pLineDst = pImage->GetLineUnsafe(j); + const uint8_t* pSrc = pLineSrc + m; + const uint8_t* pSrcEnd = pLineSrc + m_nStride; + uint8_t* pDstEnd = pLineDst + bytes_to_copy; + for (uint8_t *pDst = pLineDst; pDst < pDstEnd; pSrc += 4, pDst += 4) { + uint32_t wTmp = JBIG2_GETDWORD(pSrc) << n; + if (pSrc + 4 < pSrcEnd) + wTmp |= (JBIG2_GETDWORD(pSrc + 4) >> (32 - n)); + JBIG2_PUTDWORD(pDst, wTmp); } } return pImage; @@ -382,7 +373,6 @@ bool CJBig2_Image::ComposeToOpt2(CJBig2_Image* pDst, } else { uint8_t* sp = nullptr; uint8_t* dp = nullptr; - if (s1 > d1) { uint32_t shift1 = s1 - d1; uint32_t shift2 = 32 - shift1; diff --git a/core/fxcodec/jbig2/JBig2_Image.h b/core/fxcodec/jbig2/JBig2_Image.h index 3cf28b0643..b1b1489506 100644 --- a/core/fxcodec/jbig2/JBig2_Image.h +++ b/core/fxcodec/jbig2/JBig2_Image.h @@ -34,11 +34,14 @@ class CJBig2_Image { int32_t width() const { return m_nWidth; } int32_t height() const { return m_nHeight; } int32_t stride() const { return m_nStride; } + uint8_t* data() const { return m_pData.Get(); } int GetPixel(int32_t x, int32_t y) const; void SetPixel(int32_t x, int32_t y, int bVal); + uint8_t* GetLine(int32_t y) const; + uint8_t* GetLineUnsafe(int32_t y) const { return data() + y * m_nStride; } void CopyLine(int32_t hTo, int32_t hFrom); void Fill(bool v); @@ -74,9 +77,9 @@ class CJBig2_Image { const FX_RECT& rtSrc); MaybeOwned m_pData; - int32_t m_nWidth; // 1-bit pixels - int32_t m_nHeight; // lines - int32_t m_nStride; // bytes + int32_t m_nWidth = 0; // 1-bit pixels + int32_t m_nHeight = 0; // lines + int32_t m_nStride = 0; // bytes, must be multiple of 4. }; #endif // CORE_FXCODEC_JBIG2_JBIG2_IMAGE_H_ diff --git a/core/fxcodec/jbig2/JBig2_Image_unittest.cpp b/core/fxcodec/jbig2/JBig2_Image_unittest.cpp index bebfb6b20d..2b4d897cea 100644 --- a/core/fxcodec/jbig2/JBig2_Image_unittest.cpp +++ b/core/fxcodec/jbig2/JBig2_Image_unittest.cpp @@ -8,26 +8,75 @@ #include "core/fxcodec/jbig2/JBig2_Image.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/base/ptr_util.h" namespace { const int32_t kWidthPixels = 80; const int32_t kWidthBytes = 10; -const int32_t kStrideBytes = kWidthBytes + 1; // For testing stride != width. +const int32_t kStrideBytes = kWidthBytes + 2; // For testing stride != width. const int32_t kHeightLines = 20; const int32_t kLargerHeightLines = 100; const int32_t kTooLargeHeightLines = 40000000; +void CheckImageEq(CJBig2_Image* img1, CJBig2_Image* img2, int line) { + EXPECT_EQ(img1->width(), img2->width()); + EXPECT_EQ(img1->height(), img2->height()); + for (int32_t y = 0; y < img1->height(); ++y) { + for (int32_t x = 0; x < img1->width(); ++x) { + EXPECT_EQ(img1->GetPixel(x, y), img2->GetPixel(x, y)) + << " at " << x << " " << y << " actual line " << line; + } + } +} + } // namespace +TEST(fxcodec, EmptyImage) { + CJBig2_Image empty(0, 0); + EXPECT_EQ(empty.width(), 0); + EXPECT_EQ(empty.height(), 0); + + // Out-of-bounds SetPixel() is silent no-op. + empty.SetPixel(0, 0, true); + empty.SetPixel(1, 1, true); + + // Out-of-bounds GetPixel returns 0. + EXPECT_EQ(empty.GetPixel(0, 0), 0); + EXPECT_EQ(empty.GetPixel(1, 1), 0); + + // Out-of-bounds GetLine() returns null. + EXPECT_EQ(empty.GetLine(0), nullptr); + EXPECT_EQ(empty.GetLine(1), nullptr); +} + TEST(fxcodec, JBig2ImageCreate) { CJBig2_Image img(kWidthPixels, kHeightLines); - img.SetPixel(0, 0, true); - img.SetPixel(kWidthPixels - 1, kHeightLines - 1, false); EXPECT_EQ(kWidthPixels, img.width()); EXPECT_EQ(kHeightLines, img.height()); - EXPECT_TRUE(img.GetPixel(0, 0)); - EXPECT_FALSE(img.GetPixel(kWidthPixels - 1, kHeightLines - 1)); + EXPECT_EQ(0, img.GetPixel(0, 0)); + EXPECT_EQ(0, img.GetLine(0)[0]); + EXPECT_EQ(0, img.GetPixel(kWidthPixels - 1, kHeightLines - 1)); + EXPECT_EQ(0, img.GetLine(kHeightLines - 1)[kWidthBytes - 1]); + + img.SetPixel(0, 0, true); + img.SetPixel(kWidthPixels - 1, kHeightLines - 1, true); + EXPECT_EQ(1, img.GetPixel(0, 0)); + EXPECT_EQ(1, img.GetPixel(kWidthPixels - 1, kHeightLines - 1)); + EXPECT_EQ(0x80, img.GetLine(0)[0]); + EXPECT_EQ(0x01, img.GetLine(kHeightLines - 1)[kWidthBytes - 1]); + + // Out-of-bounds SetPixel() is silent no-op. + img.SetPixel(-1, 1, true); + img.SetPixel(kWidthPixels, kHeightLines, true); + + // Out-of-bounds GetPixel returns 0. + EXPECT_EQ(0, img.GetPixel(-1, -1)); + EXPECT_EQ(0, img.GetPixel(kWidthPixels, kHeightLines)); + + // Out-of-bounds GetLine() returns null. + EXPECT_EQ(nullptr, img.GetLine(-1)); + EXPECT_EQ(nullptr, img.GetLine(kHeightLines)); } TEST(fxcodec, JBig2ImageCreateTooBig) { @@ -56,6 +105,14 @@ TEST(fxcodec, JBig2ImageCreateExternalTooBig) { EXPECT_EQ(nullptr, img.data()); } +TEST(fxcodec, JBig2ImageCreateExternalBadStride) { + uint8_t buf[kHeightLines * kStrideBytes]; + CJBig2_Image img(kWidthPixels, kTooLargeHeightLines, kStrideBytes - 1, buf); + EXPECT_EQ(0, img.width()); + EXPECT_EQ(0, img.height()); + EXPECT_EQ(nullptr, img.data()); +} + TEST(fxcodec, JBig2ImageExpand) { CJBig2_Image img(kWidthPixels, kHeightLines); img.SetPixel(0, 0, true); @@ -103,3 +160,140 @@ TEST(fxcodec, JBig2ImageExpandExternalTooBig) { EXPECT_TRUE(img.GetPixel(0, 0)); EXPECT_FALSE(img.GetPixel(kWidthPixels - 1, kHeightLines - 1)); } + +TEST(fxcodec, JBig2EmptyImage) { + auto empty = pdfium::MakeUnique(0, 0); + + // Empty subimage. + auto sub1 = empty->SubImage(0, 0, 0, 0); + EXPECT_EQ(sub1->width(), 0); + EXPECT_EQ(sub1->height(), 0); + + // Larger dimensions are zero-padded. + auto sub2 = empty->SubImage(0, 0, 1, 1); + EXPECT_EQ(1, sub2->width()); + EXPECT_EQ(1, sub2->height()); + EXPECT_EQ(0, sub2->GetPixel(0, 0)); + + // Bad dimensions give an empty image. + sub2 = empty->SubImage(0, 0, -1, -1); + EXPECT_EQ(sub2->width(), 0); + EXPECT_EQ(sub2->height(), 0); + + // Bad offsets zero pad the image. + auto sub3 = empty->SubImage(-1, -1, 2, 2); + EXPECT_EQ(sub3->width(), 2); + EXPECT_EQ(sub3->height(), 2); + + // Bad dimensions and bad offsets give an empty image. + sub3 = empty->SubImage(-1, -1, -100, -100); + EXPECT_EQ(sub3->width(), 0); + EXPECT_EQ(sub3->height(), 0); +} + +TEST(fxcodec, JBig2SubImage) { + // 1-px wide rectangle in image. + uint8_t pattern[5][8] = { + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x01, 0xff, 0x80, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x01, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x01, 0xff, 0x80, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + }; + + // 1-px wide rectangle in image, offset 2 in x. + uint8_t pattern20[5][8] = { + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x04, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + }; + + // 1-px wide rectangle in image, offset 2 in x and y, padded. + uint8_t pattern22[5][8] = { + {0x00, 0x04, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + }; + + // Image size a nice clean power of two. + auto img32 = pdfium::MakeUnique( + 32, 5, 8, reinterpret_cast(pattern)); + + // Image size not a nice clean value. + auto img37 = pdfium::MakeUnique( + 37, 5, 8, reinterpret_cast(pattern)); + + // Expected results to check against. + auto expected20 = pdfium::MakeUnique( + 30, 5, 8, reinterpret_cast(pattern20)); + + auto expected22 = pdfium::MakeUnique( + 30, 5, 8, reinterpret_cast(pattern22)); + + auto expected_zeros = pdfium::MakeUnique(32, 5); + + // Empty subimage. + auto sub = img32->SubImage(0, 0, 0, 0); + EXPECT_EQ(sub->width(), 0); + EXPECT_EQ(sub->height(), 0); + + // Full sub-image. + sub = img32->SubImage(0, 0, 32, 5); + EXPECT_EQ(sub->width(), 32); + EXPECT_EQ(sub->height(), 5); + CheckImageEq(img32.get(), sub.get(), __LINE__); + + sub = img37->SubImage(0, 0, 32, 5); + EXPECT_EQ(sub->width(), 32); + EXPECT_EQ(sub->height(), 5); + CheckImageEq(img32.get(), sub.get(), __LINE__); + + // Actual bit manipulations. + sub = img32->SubImage(2, 0, 30, 5); + CheckImageEq(expected20.get(), sub.get(), __LINE__); + + sub = img37->SubImage(2, 2, 30, 5); + CheckImageEq(expected22.get(), sub.get(), __LINE__); + + // Aligned Sub-image including cruft in stride beyond width. + sub = img37->SubImage(32, 0, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image waaaaay beyond width. + sub = img37->SubImage(2000, 0, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image waaaaay beyond height. + sub = img37->SubImage(0, 2000, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image with negative x offset. + sub = img37->SubImage(-1, 0, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image with negative y offset. + sub = img37->SubImage(0, -1, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image with negative width. + sub = img37->SubImage(-1, 0, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image with negative height. + sub = img37->SubImage(0, -1, 32, 5); + CheckImageEq(expected_zeros.get(), sub.get(), __LINE__); + + // Sub-image wider than original. + sub = img37->SubImage(0, 0, 128, 5); + EXPECT_EQ(128, sub->width()); + EXPECT_EQ(5, sub->height()); + + // Sub-image higher than original. + sub = img37->SubImage(0, 0, 32, 40); + EXPECT_EQ(32, sub->width()); + EXPECT_EQ(40, sub->height()); +} -- cgit v1.2.3