From 12bca469969610e764f0e42e74abc50e83d2b9ae Mon Sep 17 00:00:00 2001 From: Eirik Aavitsland Date: Tue, 13 Aug 2024 13:06:53 +0200 Subject: [PATCH] Remove inline downscaling in png reader In Qt 5.0 timeframe, support for downscaled reading was added to the png handler, as a help for devices with very limited memory. Instead of decoding the whole image into memory and then scaling it down (as happens for most image formats if scaled reading is requested), a separate reading function was added that would perform line-by-line decoding and downscaling. Although this provides a transient memory saving during decoding, it has a number of drawbacks: - The scaling routine is simplistic and can create artifacts for images with transparency, ref. the linked bug report - The scaled result from the png handler is in general different from the result of using the common QImage smoothscaling function - The line-by-line scaling function is generally slower than the common smoothscaling function, since the latter is optimized using SSE/NEON extensions - It adds a redundant implementation of image scaling to the code base Hence we drop this functionalty going forward. This effectively reverts 44a9c08eff0647f398863de55e2b62dbd6541e27. [ChangeLog][QtGui][Image I/O] The PNG handler no longer implements progressive downscaling during decoding when scaled image reading is requested. Fixes: QTBUG-121724 Change-Id: I7b524b12c174c64201b98945ee2fa062ed127f87 Reviewed-by: Eskil Abrahamsen Blomfeldt (cherry picked from commit a2a315eaa28edf9c649e13c951fdb1154e3ddf48) Reviewed-by: Qt Cherry-pick Bot --- src/gui/image/qpnghandler.cpp | 207 ++++-------------- .../image/qimagereader/tst_qimagereader.cpp | 1 - .../image/qimagewriter/tst_qimagewriter.cpp | 3 +- 3 files changed, 43 insertions(+), 168 deletions(-) diff --git a/src/gui/image/qpnghandler.cpp b/src/gui/image/qpnghandler.cpp index 615a36fa36a..6500b162e5d 100644 --- a/src/gui/image/qpnghandler.cpp +++ b/src/gui/image/qpnghandler.cpp @@ -76,7 +76,8 @@ public: }; QPngHandlerPrivate(QPngHandler *qq) - : gamma(0.0), fileGamma(0.0), quality(50), compression(50), colorSpaceState(Undefined), png_ptr(nullptr), info_ptr(nullptr), end_info(nullptr), state(Ready), q(qq) + : gamma(0.0), fileGamma(0.0), quality(50), compression(50), colorSpaceState(Undefined), + png_ptr(nullptr), info_ptr(nullptr), end_info(nullptr), row_pointers(nullptr), state(Ready), q(qq) { } float gamma; @@ -84,7 +85,6 @@ public: int quality; // quality is used for backward compatibility, maps to compression int compression; QString description; - QSize scaledSize; QStringList readTexts; QColorSpace colorSpace; ColorSpaceState colorSpaceState; @@ -92,6 +92,7 @@ public: png_struct *png_ptr; png_info *info_ptr; png_info *end_info; + png_byte **row_pointers; bool readPngHeader(); bool readPngImage(QImage *image); @@ -99,30 +100,6 @@ public: QImage::Format readImageFormat(); - struct AllocatedMemoryPointers { - AllocatedMemoryPointers() - : row_pointers(nullptr), accRow(nullptr), inRow(nullptr), outRow(nullptr) - { } - void deallocate() - { - delete [] row_pointers; - row_pointers = nullptr; - delete [] accRow; - accRow = nullptr; - delete [] inRow; - inRow = nullptr; - delete [] outRow; - outRow = nullptr; - } - - png_byte **row_pointers; - quint32 *accRow; - png_byte *inRow; - uchar *outRow; - }; - - AllocatedMemoryPointers amp; - State state; QPngHandler *q; @@ -206,7 +183,7 @@ void qpiw_flush_fn(png_structp /* png_ptr */) } static -bool setup_qt(QImage& image, png_structp png_ptr, png_infop info_ptr, QSize scaledSize, bool *doScaledRead) +bool setup_qt(QImage& image, png_structp png_ptr, png_infop info_ptr) { png_uint_32 width = 0; png_uint_32 height = 0; @@ -217,8 +194,7 @@ bool setup_qt(QImage& image, png_structp png_ptr, png_infop info_ptr, QSize scal int num_trans; png_colorp palette = nullptr; int num_palette; - int interlace_method = PNG_INTERLACE_LAST; - png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, &interlace_method, nullptr, nullptr); + png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, nullptr, nullptr, nullptr); QSize size(width, height); png_set_interlace_handling(png_ptr); @@ -360,15 +336,7 @@ bool setup_qt(QImage& image, png_structp png_ptr, png_infop info_ptr, QSize scal // We want 4 bytes, but it isn't an alpha channel format = QImage::Format_RGB32; } - QSize outSize(width,height); - if (!scaledSize.isEmpty() && quint32(scaledSize.width()) <= width && - quint32(scaledSize.height()) <= height && scaledSize != outSize && interlace_method == PNG_INTERLACE_NONE) { - // Do inline downscaling - outSize = scaledSize; - if (doScaledRead) - *doScaledRead = true; - } - if (!QImageIOHandler::allocateImage(outSize, format, &image)) + if (!QImageIOHandler::allocateImage(size, format, &image)) return false; if (QSysInfo::ByteOrder == QSysInfo::BigEndian) @@ -384,85 +352,6 @@ bool setup_qt(QImage& image, png_structp png_ptr, png_infop info_ptr, QSize scal return true; } -static void read_image_scaled(QImage *outImage, png_structp png_ptr, png_infop info_ptr, - QPngHandlerPrivate::AllocatedMemoryPointers &, QSize scaledSize) -{ - - png_uint_32 width = 0; - png_uint_32 height = 0; - png_int_32 offset_x = 0; - png_int_32 offset_y = 0; - - int bit_depth = 0; - int color_type = 0; - int unit_type = PNG_OFFSET_PIXEL; - png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, nullptr, nullptr, nullptr); - png_get_oFFs(png_ptr, info_ptr, &offset_x, &offset_y, &unit_type); - uchar *data = outImage->bits(); - qsizetype bpl = outImage->bytesPerLine(); - - if (scaledSize.isEmpty() || !width || !height) - return; - - const quint32 iysz = height; - const quint32 ixsz = width; - const quint32 oysz = scaledSize.height(); - const quint32 oxsz = scaledSize.width(); - const quint32 ibw = 4*width; - amp.accRow = new quint32[ibw]; - memset(amp.accRow, 0, ibw*sizeof(quint32)); - amp.inRow = new png_byte[ibw]; - memset(amp.inRow, 0, ibw*sizeof(png_byte)); - amp.outRow = new uchar[ibw]; - memset(amp.outRow, 0, ibw*sizeof(uchar)); - qint32 rval = 0; - for (quint32 oy=0; oy 0; rval-=oysz) { - png_read_row(png_ptr, amp.inRow, nullptr); - quint32 fact = qMin(oysz, quint32(rval)); - for (quint32 i=0; i < ibw; i++) - amp.accRow[i] += fact*amp.inRow[i]; - } - rval *= -1; - - // We have a full output row, store it - for (quint32 i=0; i < ibw; i++) - amp.outRow[i] = uchar(amp.accRow[i]/iysz); - - quint32 a[4] = {0, 0, 0, 0}; - qint32 cval = oxsz; - quint32 ix = 0; - for (quint32 ox=0; ox 0; cval-=oxsz) { - ix += 4; - if (ix >= ibw) - break; // Safety belt, should not happen - quint32 fact = qMin(oxsz, quint32(cval)); - for (quint32 i=0; i < 4; i++) - a[i] += fact * amp.outRow[ix+i]; - } - cval *= -1; - for (quint32 i=0; i < 4; i++) - data[(4*ox)+i] = uchar(a[i]/ixsz); - } - data += bpl; - } - amp.deallocate(); - - outImage->setDotsPerMeterX((png_get_x_pixels_per_meter(png_ptr,info_ptr)*oxsz)/ixsz); - outImage->setDotsPerMeterY((png_get_y_pixels_per_meter(png_ptr,info_ptr)*oysz)/iysz); - - if (unit_type == PNG_OFFSET_PIXEL) - outImage->setOffset(QPoint(offset_x*oxsz/ixsz, offset_y*oysz/iysz)); - -} - extern "C" { static void qt_png_warning(png_structp /*png_ptr*/, png_const_charp message) { @@ -616,10 +505,11 @@ bool QPngHandlerPrivate::readPngImage(QImage *outImage) return false; } + row_pointers = nullptr; if (setjmp(png_jmpbuf(png_ptr))) { png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); png_ptr = nullptr; - amp.deallocate(); + delete[] row_pointers; state = Error; return false; } @@ -632,55 +522,49 @@ bool QPngHandlerPrivate::readPngImage(QImage *outImage) colorSpaceState = GammaChrm; } - bool doScaledRead = false; - if (!setup_qt(*outImage, png_ptr, info_ptr, scaledSize, &doScaledRead)) { + if (!setup_qt(*outImage, png_ptr, info_ptr)) { png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); png_ptr = nullptr; - amp.deallocate(); + delete[] row_pointers; state = Error; return false; } - if (doScaledRead) { - read_image_scaled(outImage, png_ptr, info_ptr, amp, scaledSize); - } else { - png_uint_32 width = 0; - png_uint_32 height = 0; - png_int_32 offset_x = 0; - png_int_32 offset_y = 0; + png_uint_32 width = 0; + png_uint_32 height = 0; + png_int_32 offset_x = 0; + png_int_32 offset_y = 0; - int bit_depth = 0; - int color_type = 0; - int unit_type = PNG_OFFSET_PIXEL; - png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, nullptr, nullptr, nullptr); - png_get_oFFs(png_ptr, info_ptr, &offset_x, &offset_y, &unit_type); - uchar *data = outImage->bits(); - qsizetype bpl = outImage->bytesPerLine(); - amp.row_pointers = new png_bytep[height]; + int bit_depth = 0; + int color_type = 0; + int unit_type = PNG_OFFSET_PIXEL; + png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type, nullptr, nullptr, nullptr); + png_get_oFFs(png_ptr, info_ptr, &offset_x, &offset_y, &unit_type); + uchar *data = outImage->bits(); + qsizetype bpl = outImage->bytesPerLine(); + row_pointers = new png_bytep[height]; - for (uint y = 0; y < height; y++) - amp.row_pointers[y] = data + y * bpl; + for (uint y = 0; y < height; y++) + row_pointers[y] = data + y * bpl; - png_read_image(png_ptr, amp.row_pointers); - amp.deallocate(); + png_read_image(png_ptr, row_pointers); - outImage->setDotsPerMeterX(png_get_x_pixels_per_meter(png_ptr,info_ptr)); - outImage->setDotsPerMeterY(png_get_y_pixels_per_meter(png_ptr,info_ptr)); + outImage->setDotsPerMeterX(png_get_x_pixels_per_meter(png_ptr,info_ptr)); + outImage->setDotsPerMeterY(png_get_y_pixels_per_meter(png_ptr,info_ptr)); - if (unit_type == PNG_OFFSET_PIXEL) - outImage->setOffset(QPoint(offset_x, offset_y)); + if (unit_type == PNG_OFFSET_PIXEL) + outImage->setOffset(QPoint(offset_x, offset_y)); - // sanity check palette entries - if (color_type == PNG_COLOR_TYPE_PALETTE && outImage->format() == QImage::Format_Indexed8) { - int color_table_size = outImage->colorCount(); - for (int y=0; y<(int)height; ++y) { - uchar *p = FAST_SCAN_LINE(data, bpl, y); - uchar *end = p + width; - while (p < end) { - if (*p >= color_table_size) - *p = 0; - ++p; - } + // sanity check palette entries + if (color_type == PNG_COLOR_TYPE_PALETTE && outImage->format() == QImage::Format_Indexed8) { + int color_table_size = outImage->colorCount(); + for (int y=0; y<(int)height; ++y) { + uchar *p = FAST_SCAN_LINE(data, bpl, y); + uchar *end = p + width; + while (p < end) { + if (*p >= color_table_size) + *p = 0; + ++p; } } } @@ -694,12 +578,10 @@ bool QPngHandlerPrivate::readPngImage(QImage *outImage) png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); png_ptr = nullptr; - amp.deallocate(); + delete[] row_pointers; + row_pointers = nullptr; state = Ready; - if (scaledSize.isValid() && outImage->size() != scaledSize) - *outImage = outImage->scaled(scaledSize, Qt::IgnoreAspectRatio, Qt::SmoothTransformation); - if (colorSpaceState > Undefined && colorSpace.isValid()) outImage->setColorSpace(colorSpace); @@ -1199,8 +1081,7 @@ bool QPngHandler::supportsOption(ImageOption option) const || option == ImageFormat || option == Quality || option == CompressionRatio - || option == Size - || option == ScaledSize; + || option == Size; } QVariant QPngHandler::option(ImageOption option) const @@ -1221,8 +1102,6 @@ QVariant QPngHandler::option(ImageOption option) const else if (option == Size) return QSize(png_get_image_width(d->png_ptr, d->info_ptr), png_get_image_height(d->png_ptr, d->info_ptr)); - else if (option == ScaledSize) - return d->scaledSize; else if (option == ImageFormat) return d->readImageFormat(); return QVariant(); @@ -1238,8 +1117,6 @@ void QPngHandler::setOption(ImageOption option, const QVariant &value) d->compression = value.toInt(); else if (option == Description) d->description = value.toString(); - else if (option == ScaledSize) - d->scaledSize = value.toSize(); } QT_END_NAMESPACE diff --git a/tests/auto/gui/image/qimagereader/tst_qimagereader.cpp b/tests/auto/gui/image/qimagereader/tst_qimagereader.cpp index 96af8b4e9b5..de9fea78ea6 100644 --- a/tests/auto/gui/image/qimagereader/tst_qimagereader.cpp +++ b/tests/auto/gui/image/qimagereader/tst_qimagereader.cpp @@ -1702,7 +1702,6 @@ void tst_QImageReader::supportsOption_data() QImageIOHandler::Quality, QImageIOHandler::CompressionRatio, QImageIOHandler::Size, - QImageIOHandler::ScaledSize, QImageIOHandler::ImageFormat, }; } diff --git a/tests/auto/gui/image/qimagewriter/tst_qimagewriter.cpp b/tests/auto/gui/image/qimagewriter/tst_qimagewriter.cpp index 1059cc48abf..973fa4cfb13 100644 --- a/tests/auto/gui/image/qimagewriter/tst_qimagewriter.cpp +++ b/tests/auto/gui/image/qimagewriter/tst_qimagewriter.cpp @@ -389,8 +389,7 @@ void tst_QImageWriter::supportsOption_data() << QImageIOHandler::Description << QImageIOHandler::Quality << QImageIOHandler::CompressionRatio - << QImageIOHandler::Size - << QImageIOHandler::ScaledSize); + << QImageIOHandler::Size); } void tst_QImageWriter::supportsOption()