From: =?utf-8?q?Lars_M=C3=BCller?= <34514239+appgurueu@users.noreply.github.com> Date: Tue, 19 Nov 2024 13:37:05 +0100 Subject: Fix rendering regression with TGA type 1 files with BGRA8 color (#15402) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Origin: backport: https://github.com/minetest/minetest/commit/15e8f9e6a06c3ad87bae3ffaf4ec58f551612ff6 Forwarded: not-needed Applied-Upstream: 5.11.0 TGA uses BGR(A)8, stored in memory in that order. Irrlicht typically expects 0xAARRGGBB, which depends on endianness. (This means that on little endian, no [B][G][R][A] -> 0xAARRGGBB conversion needs to be done, but Irrlicht was swapping the bytes.) This makes both conversion functions consistently convert from [B][G][R]([A]) to 0xAARRGGBB (SColor), documents them properly and moves them to CImageLoaderTGA.cpp so no poor soul shall be fooled by them in the near future. --------- Co-authored-by: Ælla Chiana Moskopp --- irr/src/CColorConverter.cpp | 38 +++++++++----------------------------- irr/src/CColorConverter.h | 2 -- irr/src/CImage.cpp | 2 ++ irr/src/CImageLoaderTGA.cpp | 23 +++++++++++++++++++++-- 4 files changed, 32 insertions(+), 33 deletions(-) diff --git a/irr/src/CColorConverter.cpp b/irr/src/CColorConverter.cpp index 96b22c5..d3a5cf6 100644 --- a/irr/src/CColorConverter.cpp +++ b/irr/src/CColorConverter.cpp @@ -7,6 +7,15 @@ #include "os.h" #include "irrString.h" +// Warning: The naming of Irrlicht color formats +// is not consistent regarding actual component order in memory. +// E.g. in CImage, ECF_R8G8B8 is handled per-byte and stored as [R][G][B] in memory +// while ECF_A8R8G8B8 is handled as an u32 0xAARRGGBB so [B][G][R][A] (little endian) in memory. +// The conversions suffer from the same inconsistencies, e.g. +// convert_R8G8B8toA8R8G8B8 converts [R][G][B] into 0xFFRRGGBB = [B][G][R][FF] (little endian); +// convert_A1R5G5B5toR8G8B8 converts 0bARRRRRGGGGGBBBBB into [R][G][B]. +// This also means many conversions may be broken on big endian. + namespace irr { namespace video @@ -480,19 +489,6 @@ void CColorConverter::convert_R8G8B8toA1R5G5B5(const void *sP, s32 sN, void *dP) } } -void CColorConverter::convert_B8G8R8toA8R8G8B8(const void *sP, s32 sN, void *dP) -{ - u8 *sB = (u8 *)sP; - u32 *dB = (u32 *)dP; - - for (s32 x = 0; x < sN; ++x) { - *dB = 0xff000000 | (sB[2] << 16) | (sB[1] << 8) | sB[0]; - - sB += 3; - ++dB; - } -} - void CColorConverter::convert_A8R8G8B8toR8G8B8A8(const void *sP, s32 sN, void *dP) { const u32 *sB = (const u32 *)sP; @@ -515,22 +511,6 @@ void CColorConverter::convert_A8R8G8B8toA8B8G8R8(const void *sP, s32 sN, void *d } } -void CColorConverter::convert_B8G8R8A8toA8R8G8B8(const void *sP, s32 sN, void *dP) -{ - u8 *sB = (u8 *)sP; - u8 *dB = (u8 *)dP; - - for (s32 x = 0; x < sN; ++x) { - dB[0] = sB[3]; - dB[1] = sB[2]; - dB[2] = sB[1]; - dB[3] = sB[0]; - - sB += 4; - dB += 4; - } -} - void CColorConverter::convert_R8G8B8toB8G8R8(const void *sP, s32 sN, void *dP) { u8 *sB = (u8 *)sP; diff --git a/irr/src/CColorConverter.h b/irr/src/CColorConverter.h index 7f16282..0bffa26 100644 --- a/irr/src/CColorConverter.h +++ b/irr/src/CColorConverter.h @@ -74,8 +74,6 @@ class CColorConverter static void convert_R8G8B8toA1R5G5B5(const void *sP, s32 sN, void *dP); static void convert_R8G8B8toB8G8R8(const void *sP, s32 sN, void *dP); static void convert_R8G8B8toR5G6B5(const void *sP, s32 sN, void *dP); - static void convert_B8G8R8toA8R8G8B8(const void *sP, s32 sN, void *dP); - static void convert_B8G8R8A8toA8R8G8B8(const void *sP, s32 sN, void *dP); static void convert_A8R8G8B8toR8G8B8A8(const void *sP, s32 sN, void *dP); static void convert_A8R8G8B8toA8B8G8R8(const void *sP, s32 sN, void *dP); diff --git a/irr/src/CImage.cpp b/irr/src/CImage.cpp index 2095901..0c6d705 100644 --- a/irr/src/CImage.cpp +++ b/irr/src/CImage.cpp @@ -95,6 +95,8 @@ SColor CImage::getPixel(u32 x, u32 y) const case ECF_A8R8G8B8: return ((u32 *)Data)[y * Size.Width + x]; case ECF_R8G8B8: { + // FIXME this interprets the memory as [R][G][B], whereas SColor is stored as + // 0xAARRGGBB, meaning it is lies in memory as [B][G][R][A] on a little endian machine. u8 *p = Data + (y * 3) * Size.Width + (x * 3); return SColor(255, p[0], p[1], p[2]); } diff --git a/irr/src/CImageLoaderTGA.cpp b/irr/src/CImageLoaderTGA.cpp index 274b155..75b0b56 100644 --- a/irr/src/CImageLoaderTGA.cpp +++ b/irr/src/CImageLoaderTGA.cpp @@ -93,6 +93,25 @@ bool CImageLoaderTGA::isALoadableFileFormat(io::IReadFile *file) const return (!strcmp(footer.Signature, "TRUEVISION-XFILE.")); // very old tgas are refused. } +/// Converts *byte order* BGR to *endianness order* ARGB (SColor "=" u32) +static void convert_BGR8_to_SColor(const u8 *src, u32 n, u32 *dst) +{ + for (u32 i = 0; i < n; ++i) { + const u8 *bgr = &src[3 * i]; + dst[i] = 0xff000000 | (bgr[2] << 16) | (bgr[1] << 8) | bgr[0]; + } +} + +/// Converts *byte order* BGRA to *endianness order* ARGB (SColor "=" u32) +/// Note: This just copies from src to dst on little endian. +static void convert_BGRA8_to_SColor(const u8 *src, u32 n, u32 *dst) +{ + for (u32 i = 0; i < n; ++i) { + const u8 *bgra = &src[4 * i]; + dst[i] = (bgra[3] << 24) | (bgra[2] << 16) | (bgra[1] << 8) | bgra[0]; + } +} + //! creates a surface from the file IImage *CImageLoaderTGA::loadImage(io::IReadFile *file) const { @@ -139,10 +158,10 @@ IImage *CImageLoaderTGA::loadImage(io::IReadFile *file) const CColorConverter::convert_A1R5G5B5toA8R8G8B8(colorMap, header.ColorMapLength, palette); break; case 24: - CColorConverter::convert_B8G8R8toA8R8G8B8(colorMap, header.ColorMapLength, palette); + convert_BGR8_to_SColor(colorMap, header.ColorMapLength, palette); break; case 32: - CColorConverter::convert_B8G8R8A8toA8R8G8B8(colorMap, header.ColorMapLength, palette); + convert_BGRA8_to_SColor(colorMap, header.ColorMapLength, palette); break; } delete[] colorMap;