From 15e4f9a8866834ac6e701b21a3ce913ce8b9bda3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Mon, 20 Nov 2023 19:15:04 +0100 Subject: [PATCH 1/5] Remove remaining obsolete `// ignore_convention` comments --- src/engine/client/graphics_threaded.cpp | 4 ++-- src/tools/map_replace_image.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/engine/client/graphics_threaded.cpp b/src/engine/client/graphics_threaded.cpp index a43038c4c..145c671f7 100644 --- a/src/engine/client/graphics_threaded.cpp +++ b/src/engine/client/graphics_threaded.cpp @@ -576,9 +576,9 @@ int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int Sto { pImg->m_pData = pImgBuffer; - if(ImageFormat == IMAGE_FORMAT_RGB) // ignore_convention + if(ImageFormat == IMAGE_FORMAT_RGB) pImg->m_Format = CImageInfo::FORMAT_RGB; - else if(ImageFormat == IMAGE_FORMAT_RGBA) // ignore_convention + else if(ImageFormat == IMAGE_FORMAT_RGBA) pImg->m_Format = CImageInfo::FORMAT_RGBA; else { diff --git a/src/tools/map_replace_image.cpp b/src/tools/map_replace_image.cpp index a1c755e5b..c9e655a8c 100644 --- a/src/tools/map_replace_image.cpp +++ b/src/tools/map_replace_image.cpp @@ -48,9 +48,9 @@ int LoadPNG(CImageInfo *pImg, const char *pFilename) { pImg->m_pData = pImgBuffer; - if(ImageFormat == IMAGE_FORMAT_RGB) // ignore_convention + if(ImageFormat == IMAGE_FORMAT_RGB) pImg->m_Format = CImageInfo::FORMAT_RGB; - else if(ImageFormat == IMAGE_FORMAT_RGBA) // ignore_convention + else if(ImageFormat == IMAGE_FORMAT_RGBA) pImg->m_Format = CImageInfo::FORMAT_RGBA; else { From 0427dfff2ef550727021fbec627db9eba61f5944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Mon, 20 Nov 2023 19:18:45 +0100 Subject: [PATCH 2/5] Use `bool` instead of `int` --- src/engine/client/graphics_threaded.cpp | 10 +++++----- src/engine/client/graphics_threaded.h | 2 +- src/engine/graphics.h | 2 +- src/tools/map_replace_image.cpp | 10 +++++----- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/engine/client/graphics_threaded.cpp b/src/engine/client/graphics_threaded.cpp index 145c671f7..27314cfa1 100644 --- a/src/engine/client/graphics_threaded.cpp +++ b/src/engine/client/graphics_threaded.cpp @@ -551,7 +551,7 @@ bool CGraphics_Threaded::UpdateTextTexture(CTextureHandle TextureID, int x, int return true; } -int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) +bool CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) { char aCompleteFilename[IO_MAX_PATH_LENGTH]; IOHANDLE File = m_pStorage->OpenFile(pFilename, IOFLAG_READ, StorageType, aCompleteFilename, sizeof(aCompleteFilename)); @@ -583,7 +583,7 @@ int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int Sto else { free(pImgBuffer); - return 0; + return false; } if(m_WarnPngliteIncompatibleImages && PngliteIncompatible != 0) @@ -613,16 +613,16 @@ int CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int Sto else { dbg_msg("game/png", "image had unsupported image format. filename='%s'", pFilename); - return 0; + return false; } } else { dbg_msg("game/png", "failed to open file. filename='%s'", pFilename); - return 0; + return false; } - return 1; + return true; } void CGraphics_Threaded::FreePNG(CImageInfo *pImg) diff --git a/src/engine/client/graphics_threaded.h b/src/engine/client/graphics_threaded.h index 03ca8dba9..514680749 100644 --- a/src/engine/client/graphics_threaded.h +++ b/src/engine/client/graphics_threaded.h @@ -965,7 +965,7 @@ public: // simple uncompressed RGBA loaders IGraphics::CTextureHandle LoadTexture(const char *pFilename, int StorageType, int Flags = 0) override; - int LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) override; + bool LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) override; void FreePNG(CImageInfo *pImg) override; bool CheckImageDivisibility(const char *pFileName, CImageInfo &Img, int DivX, int DivY, bool AllowResize) override; diff --git a/src/engine/graphics.h b/src/engine/graphics.h index dac4a3107..67043c5b3 100644 --- a/src/engine/graphics.h +++ b/src/engine/graphics.h @@ -320,7 +320,7 @@ public: virtual const TTWGraphicsGPUList &GetGPUs() const = 0; - virtual int LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) = 0; + virtual bool LoadPNG(CImageInfo *pImg, const char *pFilename, int StorageType) = 0; virtual void FreePNG(CImageInfo *pImg) = 0; virtual bool CheckImageDivisibility(const char *pFileName, CImageInfo &Img, int DivX, int DivY, bool AllowResize) = 0; diff --git a/src/tools/map_replace_image.cpp b/src/tools/map_replace_image.cpp index c9e655a8c..b67ce18ae 100644 --- a/src/tools/map_replace_image.cpp +++ b/src/tools/map_replace_image.cpp @@ -23,7 +23,7 @@ int g_NewDataID = -1; int g_NewDataSize = 0; void *g_pNewData = nullptr; -int LoadPNG(CImageInfo *pImg, const char *pFilename) +bool LoadPNG(CImageInfo *pImg, const char *pFilename) { IOHANDLE File = io_open(pFilename, IOFLAG_READ); if(File) @@ -55,16 +55,16 @@ int LoadPNG(CImageInfo *pImg, const char *pFilename) else { free(pImgBuffer); - return 0; + return false; } } } else - return 0; + return false; } else - return 0; - return 1; + return false; + return true; } void *ReplaceImageItem(int Index, CMapItemImage *pImgItem, const char *pImgName, const char *pImgFile, CMapItemImage *pNewImgItem) From 82b75ddfe0a5a3df206ed7527974356a9a58f806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Mon, 20 Nov 2023 19:20:30 +0100 Subject: [PATCH 3/5] Improve error log messages for PNG loading --- src/engine/client/graphics_threaded.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/engine/client/graphics_threaded.cpp b/src/engine/client/graphics_threaded.cpp index 27314cfa1..e4c4852ea 100644 --- a/src/engine/client/graphics_threaded.cpp +++ b/src/engine/client/graphics_threaded.cpp @@ -2,6 +2,7 @@ /* If you are missing that file, acquire a complete release at teeworlds.com. */ #include +#include #include #if defined(CONF_FAMILY_UNIX) @@ -583,6 +584,7 @@ bool CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int St else { free(pImgBuffer); + log_error("game/png", "image had unsupported image format. filename='%s' format='%d'", pFilename, (int)ImageFormat); return false; } @@ -612,13 +614,13 @@ bool CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int St } else { - dbg_msg("game/png", "image had unsupported image format. filename='%s'", pFilename); + log_error("game/png", "failed to load file. filename='%s'", pFilename); return false; } } else { - dbg_msg("game/png", "failed to open file. filename='%s'", pFilename); + log_error("game/png", "failed to open file. filename='%s'", pFilename); return false; } From f0a17435e6c8f3593bc0abd1e8fd38fa492f783d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Mon, 20 Nov 2023 19:24:33 +0100 Subject: [PATCH 4/5] Ensure freed image buffer is not propagated The image data is freed when the image format is unsupported, but `CImageInfo::m_pData` would still point to the freed memory, so double-frees were possible. --- src/engine/client/graphics_threaded.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/engine/client/graphics_threaded.cpp b/src/engine/client/graphics_threaded.cpp index e4c4852ea..a293e4a0f 100644 --- a/src/engine/client/graphics_threaded.cpp +++ b/src/engine/client/graphics_threaded.cpp @@ -575,8 +575,6 @@ bool CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int St int PngliteIncompatible; if(::LoadPNG(ImageByteBuffer, pFilename, PngliteIncompatible, pImg->m_Width, pImg->m_Height, pImgBuffer, ImageFormat)) { - pImg->m_pData = pImgBuffer; - if(ImageFormat == IMAGE_FORMAT_RGB) pImg->m_Format = CImageInfo::FORMAT_RGB; else if(ImageFormat == IMAGE_FORMAT_RGBA) @@ -587,6 +585,7 @@ bool CGraphics_Threaded::LoadPNG(CImageInfo *pImg, const char *pFilename, int St log_error("game/png", "image had unsupported image format. filename='%s' format='%d'", pFilename, (int)ImageFormat); return false; } + pImg->m_pData = pImgBuffer; if(m_WarnPngliteIncompatibleImages && PngliteIncompatible != 0) { From bcae7da6b4f90afcc2062e8a5f398d82de25b000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Mon, 20 Nov 2023 19:42:02 +0100 Subject: [PATCH 5/5] Handle all color channel counts in image loader Greyscale images with alpha channel (i.e. channel count = 2) were incorrectly handled as RGBA images, causing the client to crash when loading such images. Now the images can successfully be loaded with the image loader, but the client still only supports loading RGB and RGBA images like before. --- src/engine/gfx/image_loader.cpp | 21 +++++++++++++++------ src/engine/gfx/image_loader.h | 1 + 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/engine/gfx/image_loader.cpp b/src/engine/gfx/image_loader.cpp index 76ac23205..eefb597cd 100644 --- a/src/engine/gfx/image_loader.cpp +++ b/src/engine/gfx/image_loader.cpp @@ -59,12 +59,15 @@ static EImageFormat LibPNGGetImageFormat(int ColorChannelCount) { case 1: return IMAGE_FORMAT_R; + case 2: + return IMAGE_FORMAT_RA; case 3: return IMAGE_FORMAT_RGB; case 4: return IMAGE_FORMAT_RGBA; default: - return IMAGE_FORMAT_RGBA; + dbg_assert(false, "ColorChannelCount invalid"); + dbg_break(); } } @@ -276,14 +279,20 @@ static void FlushPNGWrite(png_structp png_ptr) {} static int ImageLoaderHelperFormatToColorChannel(EImageFormat Format) { - if(Format == IMAGE_FORMAT_R) + switch(Format) + { + case IMAGE_FORMAT_R: return 1; - else if(Format == IMAGE_FORMAT_RGB) + case IMAGE_FORMAT_RA: + return 2; + case IMAGE_FORMAT_RGB: return 3; - else if(Format == IMAGE_FORMAT_RGBA) + case IMAGE_FORMAT_RGBA: return 4; - - return 4; + default: + dbg_assert(false, "Format invalid"); + dbg_break(); + } } bool SavePNG(EImageFormat ImageFormat, const uint8_t *pRawBuffer, SImageByteBuffer &WrittenBytes, int Width, int Height) diff --git a/src/engine/gfx/image_loader.h b/src/engine/gfx/image_loader.h index 72a176e4e..0da2474a0 100644 --- a/src/engine/gfx/image_loader.h +++ b/src/engine/gfx/image_loader.h @@ -8,6 +8,7 @@ enum EImageFormat { IMAGE_FORMAT_R = 0, + IMAGE_FORMAT_RA, IMAGE_FORMAT_RGB, IMAGE_FORMAT_RGBA, };