Skip to content

Fix garbled colors when scaling indexed-color images

Tabby Kitten requested to merge nyanpasu/gwenview:fix-indexed-zoom into master

Fixes bug where indexed-color or monochrome-palette images (eg. from pngquant) would render with garbled colors or B&W noise when zoomed.

Test images: gwenview-indexed.zip

In this PR, I converted images from indexed colors to (A)RGB32 even when not zooming. This prevents RasterImageItem::applyDisplayTransform() -> updateDisplayTransform() from printing error "Gwenview cannot apply color profile on QImage::... images" depending on zoom level.

  • image.scaled() can return various formats. Unscaled images return QImage::Format_Mono=1 or QImage::Format_Indexed8=3 (unsupported), while scaled images without/with alpha return QImage::Format_RGB32=4 (ok) or Format_ARGB32_Premultiplied=6 (unsupported). (Qt doesn't appear to output QImage::Format_ARGB32=5 from indexed images?)
  • Somehow zooming into a no-alpha image (like solid-g-fs8.png) with QImage::Format_RGB32=4 "creates" transparency, returning Format_ARGB32_Premultiplied=6 instead.
    • When we scale an opaque image and it produces premultiplied alpha, we convert to QImage::Format_RGB32=4 without alpha. I hope this doesn't break any invariants.
  • Sidenote: if I remove the call to image.convertTo(...) and open an unsupported image, "cannot apply color profile" is only printed on first failure, because mApplyDisplayTransform is set to false and not attempted again.

I verified that wide-gamut images (Webkit-logo-P3.png) are still clipped to SDR gamut. I verified (using a screenshot and eyedropper, and switching between RGB/indexed images) that wide-gamut indexed images (gradient-indexed.png) are now properly clipped rather than being desaturated, but I don't think this type of image is common (I think true-color replaced indexed color before colorspace tagging became mainstream?).

Unfortunately I do not have a wide-gamut display to verify that Gwenview shows indexed images as sRGB on wide-gamut displays, but my changes follow existing codepaths that do this (RasterImageItem::updateDisplayTransform()profile = Cms::Profile::getSRgbProfile()).

  • Should I convert opaque indexed images to QImage::Format_RGB888? This saves 25% of memory, but is inconsistent with how Qt/Gwenview loads opaque PNG/GIF files to QImage::Format_RGB32 (with a dummy FF alpha).
  • Is testing for image.colorTable() the best way to detect indexed color and convert to (A)RGB32 (because image.convertTo(originalImageFormat) will break colors or RasterImageItem::updateDisplayTransform() does not support the format)?
    • I could explicitly test for every format supported by RasterImageItem::updateDisplayTransform(), but the list in RasterImageItem::paint() could go out of sync with the switch in RasterImageItem::updateDisplayTransform().
  • Hot take: Should we run LittleCMS transforms directly on premultiplied alpha (so we don't spend time converting formats)? Brightness-invariant transforms will be unaffected, but this introduces errors to gamma or nonlinear operations. (Are they still smaller than how Gwenview blends translucent images onto the background in gamma space rather than linear light? What even is correct alpha blending...)

The existing code fails to handle images which are already in premultiplied alpha before scaling.

  • PNGs are never premultiplied-alpha (so this does not happen), but I found that some (animated?) GIFs are loaded as Format_ARGB32_Premultiplied=6, which trips the "cannot apply color profile" error. (Oddly Qt treats GIFs as non-indexed, possibly because each frame of an animation has its own index and can overlay on previous frames?)
  • GIMP also outputs TIFF files with premultiplied alpha (Webkit-logo-P3.tiff is QImage::Format_RGBA64_Premultiplied=27, and without this MR prints "cannot apply color profile" and looks different from Webkit-logo-P3.png).
    • GIMP can export 10/12 bit HEIF/AVIFs, but even without an alpha channel, Qt loads them as QImage::Format_RGBX64=25 rather than QImage::Format_BGR30 (even though the latter was added earlier?)

I fixed this bug by renaming QImage::Format originalImageFormat to target/outputImageFormat (removing bool isIndexedColor from my first commit), and find-and-replace Format_ARGB32_Premultiplied with Format_ARGB32, and Format_RGBA64_Premultiplied with RGBA64.

  • What other formats should we convert? I've converted all outputs supported by updateDisplayTransform(), but there are other formats, premultiplied and not (like <8-bit, >8-bit, or floating-point) we can convert into a supported one.
    • Hmm should I convert all formats except ones Little CMS supports, or just the ones I've found when loading real image files?
  • I'm unsure about the code formatting around switch (outputImageFormat). clang-format wouldn't allow one-liners, the default is there to avoid warnings and the semicolon is ugly but break adds another line, and if/else would be less lines of code (but writing out else if (outputImageFormat == ... produces longer lines).

Bug introduced by commit 33d6203c.
BUG: 459627

Merge request reports

Loading