2474: Fix FreeImage texture loader bugs, add test of image contents (#…

…2475)

* 2474: Fix FreeImage texture loader bugs, add test of image contents

The main one was not calling FreeImage_ConvertTo24Bits() on 32-bit
input images, but the std::memcpy() was also wrong.

Fixes #2474

* 2474: forgot to commit test image

* 2474: fix warning

* 2474: handle corrupt textures in FreeImageTextureReader instead of crashing

* 2474: add masked transparency support for FreeImage while I'm at it

* 2474: Use FreeImage_ConvertToRawBits, let OpenGL generate the mips

* 2474: avoid msvc extension

* 2474: fix implicit conversion warning

common/src/Assets/Texture.cpp

@@ -41,6 +41,7 @@ namespace TrenchBroom {
                 case GL_BGR:
                     return 3U;
                 case GL_RGBA:
+                case GL_BGRA:
                     return 4U;
             }
             ensure(false, "unknown format");
@@ -240,6 +241,18 @@ namespace TrenchBroom {
             }
         }
 
+        const TextureBuffer::List& Texture::buffersIfUnprepared() const {
+            return m_buffers;
+        }
+
+        GLenum Texture::format() const {
+            return m_format;
+        }
+
+        TextureType Texture::type() const {
+            return m_type;
+        }
+
         void Texture::setCollection(TextureCollection* collection) {
             m_collection = collection;
         }

common/src/Assets/Texture.h

@@ -86,6 +86,19 @@ namespace TrenchBroom {
 
             void activate() const;
             void deactivate() const;
+
+        public: // exposed for tests only
+            /**
+             * Returns the texture data in the format returned by format().
+             * Once prepare() is called, this will be an empty vector.
+             */
+            const TextureBuffer::List& buffersIfUnprepared() const;
+            /**
+             * Will be one of GL_RGB, GL_BGR, GL_RGBA, GL_BGRA.
+             */
+            GLenum format() const;
+            TextureType type() const;
+
         private:
             void setCollection(TextureCollection* collection);
             friend class TextureCollection;

common/src/IO/FreeImageTextureReader.cpp

@@ -30,10 +30,32 @@
 
 namespace TrenchBroom {
     namespace IO {
-        FreeImageTextureReader::FreeImageTextureReader(const NameStrategy& nameStrategy, const size_t mipCount) :
-        TextureReader(nameStrategy),
-        m_mipCount(mipCount) {
-            assert(m_mipCount > 0);
+        FreeImageTextureReader::FreeImageTextureReader(const NameStrategy& nameStrategy) :
+        TextureReader(nameStrategy) {}
+
+        /**
+         * The byte order of a 32bpp FIBITMAP is defined by the macros FI_RGBA_RED,
+         * FI_RGBA_GREEN, FI_RGBA_BLUE, FI_RGBA_ALPHA.
+         * From looking at FreeImage.h, there are only two possible orders,
+         * so we can handle both possible orders and map them to the relevant GL_RGBA
+         * or GL_BGRA constant.
+         */
+        static constexpr GLenum freeImage32BPPFormatToGLFormat() {            
+            if (FI_RGBA_RED == 0
+                && FI_RGBA_GREEN == 1
+                && FI_RGBA_BLUE == 2
+                && FI_RGBA_ALPHA == 3) {
+
+                return GL_RGBA;
+            } else if (FI_RGBA_BLUE == 0
+                && FI_RGBA_GREEN == 1
+                && FI_RGBA_RED == 2
+                && FI_RGBA_ALPHA == 3) {
+
+                return GL_BGRA;
+            } else {
+                throw std::runtime_error("Expected FreeImage to use RGBA or BGRA");
+            }
         }
 
         Assets::Texture* FreeImageTextureReader::doReadTexture(const char* const begin, const char* const end, const Path& path) const {
@@ -42,38 +64,47 @@ namespace TrenchBroom {
             FIMEMORY*                   imageMemory     = FreeImage_OpenMemory(imageBegin, static_cast<DWORD>(imageSize));
             const FREE_IMAGE_FORMAT     imageFormat     = FreeImage_GetFileTypeFromMemory(imageMemory);
             FIBITMAP*                   image           = FreeImage_LoadFromMemory(imageFormat, imageMemory);
-
             const String                imageName       = path.filename();
+
+            if (image == nullptr) {
+                FreeImage_CloseMemory(imageMemory);
+                return new Assets::Texture(textureName(imageName, path), 64, 64);
+            }
+
             const size_t                imageWidth      = static_cast<size_t>(FreeImage_GetWidth(image));
             const size_t                imageHeight     = static_cast<size_t>(FreeImage_GetHeight(image));
             const FREE_IMAGE_COLOR_TYPE imageColourType = FreeImage_GetColorType(image);
 
-            const auto format = GL_BGR;
-            Assets::TextureBuffer::List buffers(m_mipCount);
-            Assets::setMipBufferSize(buffers, m_mipCount, imageWidth, imageHeight, format);
+            // This is supposed to indicate whether any pixels are transparent (alpha < 100%)
+            const auto transparent = FreeImage_IsTransparent(image);
 
-            // TODO: Alpha channel seems to be unsupported by the Texture class
-            if (imageColourType != FIC_RGB) {
-                FIBITMAP* tempImage = FreeImage_ConvertTo24Bits(image);
+            const auto mipCount = 1U;
+            constexpr auto format = freeImage32BPPFormatToGLFormat();
+            Assets::TextureBuffer::List buffers(mipCount);
+            Assets::setMipBufferSize(buffers, mipCount, imageWidth, imageHeight, format);
+
+            const auto inputBytesPerPixel = FreeImage_GetLine(image) / FreeImage_GetWidth(image);
+            if (imageColourType != FIC_RGBALPHA || inputBytesPerPixel != 4) {
+                FIBITMAP* tempImage = FreeImage_ConvertTo32Bits(image);
                 FreeImage_Unload(image);
                 image = tempImage;
             }
 
-            FreeImage_FlipVertical(image);
+            const auto bytesPerPixel = FreeImage_GetLine(image) / FreeImage_GetWidth(image);
+            ensure(bytesPerPixel == 4, "expected to have converted image to 32-bit");
 
-            std::memcpy(buffers[0].ptr(), FreeImage_GetBits(image), buffers[0].size());
-            for (size_t mip = 1; mip < m_mipCount; ++mip) {
-                const auto mipSize = Assets::sizeAtMipLevel(imageWidth, imageHeight, mip);
-                FIBITMAP* mipImage = FreeImage_Rescale(image, static_cast<int>(mipSize.x()), static_cast<int>(mipSize.y()), FILTER_BICUBIC);
-                std::memcpy(buffers[mip].ptr(), FreeImage_GetBits(mipImage), buffers[mip].size());
-                FreeImage_Unload(mipImage);
-            }
+            unsigned char* outBytes = buffers.at(0).ptr();
+            const auto outBytesPerRow = static_cast<int>(imageWidth * 4);
+
+            FreeImage_ConvertToRawBits(outBytes, image, outBytesPerRow, 32, FI_RGBA_RED_MASK, FI_RGBA_GREEN_MASK, FI_RGBA_BLUE_MASK, TRUE);
 
             FreeImage_Unload(image);
             FreeImage_CloseMemory(imageMemory);
 
-            return new Assets::Texture(textureName(imageName, path), imageWidth, imageHeight, Color(), buffers, format, Assets::TextureType::Opaque);
+
+            const auto textureType = transparent ? Assets::TextureType::Masked : Assets::TextureType::Opaque;
+
+            return new Assets::Texture(textureName(imageName, path), imageWidth, imageHeight, Color(), buffers, format, textureType);
         }
     }
-
 }

common/src/IO/FreeImageTextureReader.h

@@ -28,10 +28,8 @@ namespace TrenchBroom {
         class Path;
 
         class FreeImageTextureReader : public TextureReader {
-        private:
-            size_t m_mipCount;
         public:
-            FreeImageTextureReader(const NameStrategy& nameStrategy, size_t mipCount);
+            FreeImageTextureReader(const NameStrategy& nameStrategy);
         private:
             Assets::Texture* doReadTexture(const char* const begin, const char* const end, const Path& path) const override;
         };

common/src/IO/SkinLoader.cpp

@@ -45,7 +45,7 @@ namespace TrenchBroom {
                     IO::WalTextureReader reader(IO::TextureReader::PathSuffixNameStrategy(1, true), palette);
                     return reader.readTexture(file);
                 } else {
-                    IO::FreeImageTextureReader reader(IO::TextureReader::PathSuffixNameStrategy(1, true), 1);
+                    IO::FreeImageTextureReader reader(IO::TextureReader::PathSuffixNameStrategy(1, true));
                     return reader.readTexture(file);
                 }
             } catch (FileSystemException& e) {

common/src/IO/TextureLoader.cpp

@@ -57,7 +57,7 @@ namespace TrenchBroom {
                 return std::make_unique<WalTextureReader>(nameStrategy, loadPalette(gameFS, textureConfig, logger));
             } else if (textureConfig.format.format == "image") {
                 TextureReader::PathSuffixNameStrategy nameStrategy(2, true);
-                return std::make_unique<FreeImageTextureReader>(nameStrategy, 4);
+                return std::make_unique<FreeImageTextureReader>(nameStrategy);
             } else {
                 throw GameException("Unknown texture format '" + textureConfig.format.format + "'");
             }

test/src/IO/FreeImageTextureReaderTest.cpp

@@ -27,27 +27,147 @@
 
 namespace TrenchBroom {
     namespace IO {
-        static void assertTexture(const String& name, const size_t width, const size_t height, const FileSystem& fs, const TextureReader& loader) {
-            const Assets::Texture* texture = loader.readTexture(fs.openFile(Path(name)));
+        static std::unique_ptr<const Assets::Texture> loadTexture(const String& name) {
+            TextureReader::TextureNameStrategy nameStrategy;
+            FreeImageTextureReader textureLoader(nameStrategy);
+
+            const auto imagePath = Disk::getCurrentWorkingDir() + Path("data/IO/Image/");
+            DiskFileSystem diskFS(imagePath);
+
+            return std::unique_ptr<const Assets::Texture>{ textureLoader.readTexture(diskFS.openFile(Path(name))) };
+        }
+
+        static void assertTexture(const String& name, const size_t width, const size_t height) {
+            const auto texture = loadTexture(name);
+
             ASSERT_TRUE(texture != nullptr);
             ASSERT_EQ(name, texture->name());
             ASSERT_EQ(width, texture->width());
             ASSERT_EQ(height, texture->height());
-            delete texture;
+            ASSERT_TRUE(GL_BGRA == texture->format() || GL_RGBA == texture->format());
+            ASSERT_EQ(Assets::TextureType::Opaque, texture->type());
         }
-        
+
         TEST(FreeImageTextureReaderTest, testLoadPngs) {
-            DiskFileSystem fs(IO::Disk::getCurrentWorkingDir());
+            assertTexture("5x5.png",          5,   5);
+            assertTexture("707x710.png",      707, 710);
+        }
 
-            TextureReader::TextureNameStrategy nameStrategy;
-            const auto mips = 4;
-            FreeImageTextureReader textureLoader(nameStrategy, mips);
+        TEST(FreeImageTextureReaderTest, testLoadCorruptPng) {
+            const auto texture = loadTexture("corruptPngTest.png");
+
+            // TextureReader::readTexture is supposed to return a placeholder for corrupt textures
+            ASSERT_TRUE(texture != nullptr);
+            ASSERT_EQ("corruptPngTest.png", texture->name());
+            ASSERT_NE(0, texture->width());
+            ASSERT_NE(0, texture->height());
+        }
+
+        enum class Component {
+            R, G, B, A
+        };
+
+        static uint8_t getComponentOfPixel(const Assets::Texture* texture, const int x, const int y, const Component component) {
+            const auto format = texture->format();
+
+            ensure(GL_BGRA == format || GL_RGBA == format, "expected GL_BGRA or GL_RGBA");
+
+            int componentIndex;
+            if (format == GL_RGBA) {
+                switch (component) {
+                    case Component::R: componentIndex = 0; break;
+                    case Component::G: componentIndex = 1; break;
+                    case Component::B: componentIndex = 2; break;
+                    case Component::A: componentIndex = 3; break;
+                }
+            } else {
+                switch (component) {
+                    case Component::R: componentIndex = 2; break;
+                    case Component::G: componentIndex = 1; break;
+                    case Component::B: componentIndex = 0; break;
+                    case Component::A: componentIndex = 3; break;
+                }
+            }
+
+            const auto& mip0DataBuffer = texture->buffersIfUnprepared().at(0);
+            ensure(texture->width() * texture->height() * 4 == mip0DataBuffer.size(), "unexpected texture data size");
+            ensure(x >= 0 && x < static_cast<int>(texture->width()), "x out of range");
+            ensure(y >= 0 && y < static_cast<int>(texture->height()), "y out of range");
+
+            const uint8_t* mip0Data = mip0DataBuffer.ptr();
+
+            return mip0Data[(static_cast<int>(texture->width()) * 4 * y) + (x * 4) + componentIndex];
+        }
+
+        // https://github.com/kduske/TrenchBroom/issues/2474
+        TEST(FreeImageTextureReaderTest, testPNGContents) {
+            const auto texture = loadTexture("pngContentsTest.png");
+            const auto w = 64;
+            const auto h = 64;
+
+            ASSERT_TRUE(texture != nullptr);
+            ASSERT_EQ(w, texture->width());
+            ASSERT_EQ(h, texture->height());
+            ASSERT_EQ(1, texture->buffersIfUnprepared().size());
+            ASSERT_TRUE(GL_BGRA == texture->format() || GL_RGBA == texture->format());
+            ASSERT_EQ(Assets::TextureType::Opaque, texture->type());
+
+            auto* texturePtr = texture.get();
+            for (int y = 0; y < h; ++y) {
+                for (int x = 0; x < w; ++x) {
+                    if (x == 0 && y == 0) {
+                        // top left pixel is red
+                        ASSERT_EQ(255 /* R */, getComponentOfPixel(texturePtr, x, y, Component::R));
+                        ASSERT_EQ(0   /* G */, getComponentOfPixel(texturePtr, x, y, Component::G));
+                        ASSERT_EQ(0   /* B */, getComponentOfPixel(texturePtr, x, y, Component::B));
+                        ASSERT_EQ(255 /* A */, getComponentOfPixel(texturePtr, x, y, Component::A));
+                    } else if (x == (w - 1) && y == (h - 1)) {
+                        // bottom right pixel is green
+                        ASSERT_EQ(0   /* R */, getComponentOfPixel(texturePtr, x, y, Component::R));
+                        ASSERT_EQ(255 /* G */, getComponentOfPixel(texturePtr, x, y, Component::G));
+                        ASSERT_EQ(0   /* B */, getComponentOfPixel(texturePtr, x, y, Component::B));
+                        ASSERT_EQ(255 /* A */, getComponentOfPixel(texturePtr, x, y, Component::A));
+                    } else {
+                        // others are 161, 161, 161
+                        ASSERT_EQ(161 /* R */, getComponentOfPixel(texturePtr, x, y, Component::R));
+                        ASSERT_EQ(161 /* G */, getComponentOfPixel(texturePtr, x, y, Component::G));
+                        ASSERT_EQ(161 /* B */, getComponentOfPixel(texturePtr, x, y, Component::B));
+                        ASSERT_EQ(255 /* A */, getComponentOfPixel(texturePtr, x, y, Component::A));
+                    }
+                }
+            }
+        }
+
+        TEST(FreeImageTextureReaderTest, alphaMaskTest) {
+            const auto texture = loadTexture("alphaMaskTest.png");
+            const auto w = 25;
+            const auto h = 10;
+
+            ASSERT_TRUE(texture != nullptr);
+            ASSERT_EQ(w, texture->width());
+            ASSERT_EQ(h, texture->height());
+            ASSERT_EQ(1, texture->buffersIfUnprepared().size());
+            ASSERT_TRUE(GL_BGRA == texture->format() || GL_RGBA == texture->format());
+            ASSERT_EQ(Assets::TextureType::Masked, texture->type());
 
-            const Path imagePath = Disk::getCurrentWorkingDir() + Path("data/IO/Image/");
-            DiskFileSystem diskFS(imagePath );
+            auto& mip0Data = texture->buffersIfUnprepared().at(0);
+            ASSERT_EQ(w * h * 4, mip0Data.size());
 
-            assertTexture("5x5.png",          5,   5,   diskFS, textureLoader);
-            assertTexture("707x710.png",      707, 710, diskFS, textureLoader);
+            auto* texturePtr = texture.get();
+            for (int y = 0; y < h; ++y) {
+                for (int x = 0; x < w; ++x) {
+                    if (x == 0 && y == 0) {
+                        // top left pixel is green opaque
+                        ASSERT_EQ(0   /* R */, getComponentOfPixel(texturePtr, x, y, Component::R));
+                        ASSERT_EQ(255 /* G */, getComponentOfPixel(texturePtr, x, y, Component::G));
+                        ASSERT_EQ(0   /* B */, getComponentOfPixel(texturePtr, x, y, Component::B));
+                        ASSERT_EQ(255 /* A */, getComponentOfPixel(texturePtr, x, y, Component::A));
+                    } else {
+                        // others are fully transparent (RGB values are unknown)
+                        ASSERT_EQ(0   /* A */, getComponentOfPixel(texturePtr, x, y, Component::A));
+                    }
+                }
+            }
         }
     }
 }