From 2a8ea9205474486cc3b9eb437db27c760dc56894 Mon Sep 17 00:00:00 2001 From: Timothy Nikkel Date: Mon, 9 Sep 2024 09:39:12 +0000 Subject: [PATCH] Bug 1846055. Fix color management of grayscale avif files. r=gfx-reviewers,lsalzman Color management is usually handled in the SurfacePipe, but SurfacePipe is RGB(A) only, and qcms can only operator on grayscale images in grayscale format, not RGB(A). So we must handle the qcms operation in the decoder. This is the same as how the png decoder handles this same situation. One additional wrinkle is that gfx::ConvertYCbCrToRGB32 only outputs RGB even for grayscale input data so we have to create an intermediate grayscale copy of the data for qcms to work on. Recording command lines here that were used to create the test in case it needs to be modified or extended in the future. Convert png to grayscale or grayscale + alpha ffmpeg -i file.png -pix_fmt gray file-gray.png ffmpeg -i file.png -pix_fmt ya8 file-gray.png Extract icc profile from png exiftool -icc_profile -b -w icc file.png Add icc profile to png exiftool "-icc_profile<=profile.icc" file.png Encode avif using provided icc profile in file avifenc --icc profile.icc input.png output.avif Differential Revision: https://phabricator.services.mozilla.com/D220101 --- image/decoders/nsAVIFDecoder.cpp | 43 +++++++++++++++--- image/decoders/nsAVIFDecoder.h | 1 + .../reftest/avif/grayscale-alpha-icc.avif | Bin 0 -> 746 bytes .../test/reftest/avif/grayscale-alpha-icc.png | Bin 0 -> 392 bytes image/test/reftest/avif/grayscale-icc.avif | Bin 0 -> 604 bytes image/test/reftest/avif/grayscale-icc.png | Bin 0 -> 317 bytes image/test/reftest/avif/reftest.list | 4 ++ 7 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 image/test/reftest/avif/grayscale-alpha-icc.avif create mode 100644 image/test/reftest/avif/grayscale-alpha-icc.png create mode 100644 image/test/reftest/avif/grayscale-icc.avif create mode 100644 image/test/reftest/avif/grayscale-icc.png diff --git a/image/decoders/nsAVIFDecoder.cpp b/image/decoders/nsAVIFDecoder.cpp index 01edff6d14af..d7b87831c647 100644 --- a/image/decoders/nsAVIFDecoder.cpp +++ b/image/decoders/nsAVIFDecoder.cpp @@ -1848,6 +1848,7 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( uint32_t profileSpace = qcms_profile_get_color_space(mInProfile); if (profileSpace != icSigGrayData) { + mUsePipeTransform = true; // If the transform happens with SurfacePipe, it will be in RGBA if we // have an alpha channel, because the swizzle and premultiplication // happens after color management. Otherwise it will be in BGRA because @@ -1860,6 +1861,10 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( outType = inType; } } else { + // We can't use SurfacePipe to do the color management (it can't handle + // grayscale data), we have to do it ourselves on the grayscale data + // before passing the now RGB data to SurfacePipe. + mUsePipeTransform = false; if (mHasAlpha) { inType = QCMS_DATA_GRAYA_8; outType = gfxPlatform::GetCMSOSRGBAType(); @@ -1914,8 +1919,9 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( if (mTransform) { // Color management needs to be done on non-premult data, so // ConvertYCbCrToRGB32 needs to produce non-premult data, then color - // management can happen and then later in the surface pipe we will - // convert to premult if needed. + // management can happen (either here for grayscale data, or in surface + // pipe otherwise) and then later in the surface pipe we will convert to + // premult if needed. if (hasPremultiply) { premultOp = libyuv::ARGBUnattenuate; } @@ -1944,8 +1950,6 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( MOZ_LOG(sAVIFLog, LogLevel::Debug, ("[this=%p] calling SurfacePipeFactory::CreateSurfacePipe", this)); - Maybe pipe = Nothing(); - SurfacePipeFlags pipeFlags = SurfacePipeFlags(); if (decodedData->mAlpha && mTransform) { // we know data is non-premult in this case, see above, so if we @@ -1955,6 +1959,9 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( } } + Maybe pipe = Nothing(); + auto* transform = mUsePipeTransform ? mTransform : nullptr; + if (mIsAnimated) { SurfaceFormat outFormat = decodedData->mAlpha ? SurfaceFormat::OS_RGBA : SurfaceFormat::OS_RGBX; @@ -1966,10 +1973,10 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( } pipe = SurfacePipeFactory::CreateSurfacePipe( this, Size(), OutputSize(), FullFrame(), format, outFormat, animParams, - mTransform, pipeFlags); + transform, pipeFlags); } else { pipe = SurfacePipeFactory::CreateReorientSurfacePipe( - this, Size(), OutputSize(), format, mTransform, GetOrientation(), + this, Size(), OutputSize(), format, transform, GetOrientation(), pipeFlags); } @@ -1982,8 +1989,29 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( MOZ_LOG(sAVIFLog, LogLevel::Debug, ("[this=%p] writing to surface", this)); const uint8_t* endOfRgbBuf = {rgbBuf.get() + rgbBufLength.value()}; WriteState writeBufferResult = WriteState::NEED_MORE_DATA; + uint8_t* grayLine = nullptr; + int32_t multiplier = 1; + if (mTransform && !mUsePipeTransform) { + if (mHasAlpha) { + multiplier = 2; + } + // We know this calculation doesn't overflow because rgbStride is a larger + // value and is valid here. + grayLine = new uint8_t[multiplier * rgbSize.width]; + } for (uint8_t* rowPtr = rgbBuf.get(); rowPtr < endOfRgbBuf; rowPtr += rgbStride.value()) { + if (mTransform && !mUsePipeTransform) { + // format is B8G8R8A8 or B8G8R8X8, so 1 offset picks G + for (int32_t i = 0; i < rgbSize.width; i++) { + grayLine[multiplier * i] = rowPtr[i * bytesPerPixel + 1]; + if (mHasAlpha) { + grayLine[multiplier * i + 1] = rowPtr[i * bytesPerPixel + 3]; + } + } + qcms_transform_data(mTransform, grayLine, rowPtr, rgbSize.width); + } + writeBufferResult = pipe->WriteBuffer(reinterpret_cast(rowPtr)); Maybe invalidRect = pipe->TakeInvalidRect(); @@ -2000,6 +2028,9 @@ nsAVIFDecoder::DecodeResult nsAVIFDecoder::DoDecodeInternal( MOZ_ASSERT(rowPtr + rgbStride.value() == endOfRgbBuf); } } + if (mTransform && !mUsePipeTransform) { + delete[] grayLine; + } MOZ_LOG(sAVIFLog, LogLevel::Debug, ("[this=%p] writing to surface complete", this)); diff --git a/image/decoders/nsAVIFDecoder.h b/image/decoders/nsAVIFDecoder.h index df9983a513ae..3e496deea270 100644 --- a/image/decoders/nsAVIFDecoder.h +++ b/image/decoders/nsAVIFDecoder.h @@ -90,6 +90,7 @@ class nsAVIFDecoder final : public Decoder { bool mIsAnimated = false; bool mHasAlpha = false; + bool mUsePipeTransform = true; }; class AVIFDecoderStream : public ByteStream { diff --git a/image/test/reftest/avif/grayscale-alpha-icc.avif b/image/test/reftest/avif/grayscale-alpha-icc.avif new file mode 100644 index 0000000000000000000000000000000000000000..666f2304f7f1f02f85175b520b1dc1a41e37f33a GIT binary patch literal 746 zcmZuvy=zoK5TAYCU5thpP0$#EISbK3F_%gn2qsUI6he$CjE$^&@66!ty=C9td1qsS zg_WRfD#3rifLd7wY_16S@ejmaM7i3TeRn95ftlaTZ$92I`v73J;k#D0u>mBE1{*bl zlH%xyTS#qsQl1p)c9aHV%(xPAt%UQM4c&)uG2VRhBWFCo}svi-V>f8 zT|#VzEkb67f|N_RL$>E@Nv5-~z~2m|IkTwkhUCf_bZIdGU|hC3I((D*#V54R623;O zGOC=y+@{<@QXeR7-a#hBVKQT~T}!rLmg6~_y)y4I@>-VZ7 zd$c8ehefK~-nw&Rt%we?{clC>=SzIF(f-qO&->e#&*tZ=J1@#V4(j-aE7jASFZz!3 z{$p?X9rj449<8k=gx^Ugly%JiCLJ5plMChHBXPjOy9Wn;-3~j6lRedP?Sca#wgG(F z;aQL*^8@;SnB?;`mG02FBdBar{t8{6w}F;x#>S=(gIS6dDjVnB+@~gwsRlJl*Nx@y z1?SM3@RZa_|4Ug!&q`&8N`w`nu9~{^C*VkB`t;7eTv<3e0kAugy#4xQ{|nKhf!-2_ XRu+yOe_5Hn_ObV~)>u{_-faE>bFGYg literal 0 HcmV?d00001 diff --git a/image/test/reftest/avif/grayscale-alpha-icc.png b/image/test/reftest/avif/grayscale-alpha-icc.png new file mode 100644 index 0000000000000000000000000000000000000000..b4f2bc173faac314c781025141fec93616f23ffe GIT binary patch literal 392 zcmeAS@N?(olHy`uVBq!ia0vp^DIm5=qoPi>d5<#ud%b~Xs$eZ(xHl5;P~|EB2yS%ay#fu zk`;Qwvt&NQMu#FtL61M&0-qhOHZy3SuuD1Ykk`J!j=kyb5sfmnom*D$Xq`P_(cQRE zWh+yPSo7x1jAq{%7(b~Xb5xpH`{MWo7rM(C=t=_*lPDVPNSo^kC^ zPz`NrujKfR6hJE(HgiHp?KZFJ(JFM~8pAlE98VHk1Yme50$)HJN1sxv63>n+!$@q7 zGBFY5P0MpU8p*Dd_BGlvx25eM%j<0OPbMzwH?w2G zWbVFZe6{-GkJetY1KP*~`p!|PX^+q7{RbOs7UCy*uhc&2Kj`VCA5$=zOC~}_Z;uoB zUX9!>;@pTt8^DwRz8vcLzdQ4kxmsjo{m$j4=U}7qz^Y^Fn-={AfI+i kBFsQ{*Zk1^hv2;9dM?{{%`OH7JQ~|CSHa!wd+n?37f8}+L;wH) literal 0 HcmV?d00001 diff --git a/image/test/reftest/avif/grayscale-icc.png b/image/test/reftest/avif/grayscale-icc.png new file mode 100644 index 0000000000000000000000000000000000000000..1da813ff1a85fa2512cb287eb54c1fbaddf5a820 GIT binary patch literal 317 zcmeAS@N?(olHy`uVBq!ia0vp^DIm-NBp59;d6B0zud7jfv3ra{haaLQih`*?(r=uq*VgG>x94!eGX2irikbWa}s6n`9 zH(OwJr1?f2#iW*Vx%~G*=!y=}^TjaD4i7ktqx>xgB&S z$qGH;Su&qtqeGFSpvNC>fzJ+Cn;EoE*rgnH$ZOwV$KG`Jh(?*(&Mhl=w9cNe=x$u7 zvXv=Cta3_UNj+3x+j^ato-TTd6qkcv5P&oXi{Fz_%haE<@6)k0=V!^=hw i6`@WSrHQzyPxg{MtetWhnfLdD0>RVO&t;ucLK6Um%W