diff --git a/src/MagnumPlugins/TgaImageConverter/Test/TgaImageConverterTest.cpp b/src/MagnumPlugins/TgaImageConverter/Test/TgaImageConverterTest.cpp index 798fa71c7..b9374a783 100644 --- a/src/MagnumPlugins/TgaImageConverter/Test/TgaImageConverterTest.cpp +++ b/src/MagnumPlugins/TgaImageConverter/Test/TgaImageConverterTest.cpp @@ -61,6 +61,7 @@ struct TgaImageConverterTest: TestSuite::Tester { void rleRgb(); void rleRgba(); void rleDisabled(); + void rleFallbackIfLarger(); void unsupportedMetadata(); @@ -245,6 +246,37 @@ const struct { }}, 128 + 31 + 3, {}}, }; +const struct { + const char* name; + char data[2]; + Containers::Array expected; + Containers::Optional rle; + Containers::Optional rleFallbackIfLarger; + ImageConverterFlags flags; + const char* message; +} RleFallbackIfLargerData[]{ + {"RLE smaller, verbose", {7, 7}, {InPlaceInit, { + /* well, not smaller but not larger either, so we pick what's less + work (which is to not discard all the already-done RLE work) */ + 0x80|1, 7 + }}, {}, {}, ImageConverterFlag::Verbose, ""}, + {"RLE smaller, RLE disabled, verbose", {7, 7}, {InPlaceInit, { + 7, 7 + }}, false, {}, ImageConverterFlag::Verbose, ""}, + {"uncompressed smaller", {7, 13}, {InPlaceInit, { + 7, 13 + }}, {}, {}, {}, ""}, + {"uncompressed smaller, verbose", {7, 13}, {InPlaceInit, { + 7, 13 + }}, {}, {}, ImageConverterFlag::Verbose, "Trade::TgaImageConverter::convertToData(): RLE output 1 bytes larger than uncompressed, falling back to uncompressed\n"}, + {"uncompressed smaller, fallback disabled, verbose", {7, 13}, {InPlaceInit, { + 0x00|1, 7, 13 + }}, {}, false, ImageConverterFlag::Verbose, ""}, + {"uncompressed smaller, RLE disabled, verbose", {7, 13}, {InPlaceInit, { + 7, 13 + }}, false, false, ImageConverterFlag::Verbose, ""}, +}; + const struct { const char* name; ImageFlags2D flags; @@ -271,6 +303,9 @@ TgaImageConverterTest::TgaImageConverterTest() { &TgaImageConverterTest::rleRgba, &TgaImageConverterTest::rleDisabled}); + addInstancedTests({&TgaImageConverterTest::rleFallbackIfLarger}, + Containers::arraySize(RleFallbackIfLargerData)); + addInstancedTests({&TgaImageConverterTest::unsupportedMetadata}, Containers::arraySize(UnsupportedMetadataData)); @@ -435,9 +470,11 @@ void TgaImageConverterTest::rle() { ImageView2D image{PixelStorage{}.setAlignment(1), PixelFormat::R8Unorm, size, data.data}; Containers::Pointer converter = _converterManager.instantiate("TgaImageConverter"); - if(data.rleAcrossScanlines) converter->configuration().setValue("rleAcrossScanlines", *data.rleAcrossScanlines); + /* Force RLE to be used even if larger than uncompressed. This behavior is + tested in rleFallbackIfLarger() instead. */ + converter->configuration().setValue("rleFallbackIfLarger", false); Containers::Optional> array = converter->convertToData(image); CORRADE_VERIFY(array); @@ -566,6 +603,49 @@ void TgaImageConverterTest::rleDisabled() { /* No need to verify a roundtrip, that's tested enough in uncompressed*() */ } +void TgaImageConverterTest::rleFallbackIfLarger() { + auto&& data = RleFallbackIfLargerData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + /* Skip/alignment handling tested in rleRgb() */ + ImageView2D image{PixelStorage{}.setAlignment(1), PixelFormat::R8Unorm, {2, 1}, data.data}; + + Containers::Pointer converter = _converterManager.instantiate("TgaImageConverter"); + converter->setFlags(data.flags); + if(data.rle) + converter->configuration().setValue("rle", *data.rle); + if(data.rleFallbackIfLarger) + converter->configuration().setValue("rleFallbackIfLarger", *data.rleFallbackIfLarger); + + std::ostringstream out; + Containers::Optional> array; + { + Debug redirectOutput{&out}; + array = converter->convertToData(image); + } + CORRADE_VERIFY(array); + CORRADE_COMPARE_AS( + Containers::arrayCast(*array) + .exceptPrefix(sizeof(Implementation::TgaHeader)), + data.expected, + TestSuite::Compare::Container); + CORRADE_COMPARE(out.str(), data.message); + + if(!(_importerManager.loadState("TgaImporter") & PluginManager::LoadState::Loaded)) + CORRADE_SKIP("TgaImporter plugin not enabled, can't test the result"); + + Containers::Pointer importer = _importerManager.instantiate("TgaImporter"); + CORRADE_VERIFY(importer->openData(*array)); + Containers::Optional converted = importer->image2D(0); + CORRADE_VERIFY(converted); + + CORRADE_COMPARE(converted->size(), (Vector2i{2, 1})); + CORRADE_COMPARE(converted->format(), PixelFormat::R8Unorm); + CORRADE_COMPARE_AS(converted->data(), + Containers::arrayView(data.data), + TestSuite::Compare::Container); +} + void TgaImageConverterTest::unsupportedMetadata() { auto&& data = UnsupportedMetadataData[testCaseInstanceId()]; setTestCaseDescription(data.name); diff --git a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.conf b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.conf index 0bc7ae0f1..2bf268459 100644 --- a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.conf +++ b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.conf @@ -3,6 +3,10 @@ # Run-length encode the data for smaller file size rle=true +# Fall back to uncompressed output if the RLE output is larger. Disable to +# always produce a RLE output. +rleFallbackIfLarger=true + # Allow RLE to go across scanlines. Can result in even smaller files but # considered invalid in the TGA 2.0 specification and thus may cause issues # in certain importers. diff --git a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp index f74282b02..ab25797b0 100644 --- a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp +++ b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp @@ -220,12 +220,13 @@ Containers::Optional> TgaImageConverter::doConvertToData not then allocate exactly the amount of bytes so we don't need to copy after. */ const auto pixelSize = UnsignedByte(image.pixelSize()); + const std::size_t uncompressedSize = sizeof(Implementation::TgaHeader) + pixelSize*image.size().product(); const bool rle = configuration().value("rle"); Containers::Array data; if(rle) { arrayAppend(data, NoInit, sizeof(Implementation::TgaHeader)); } else { - data = Containers::Array{NoInit, sizeof(Implementation::TgaHeader) + pixelSize*image.size().product()}; + data = Containers::Array{NoInit, uncompressedSize}; } /* Clear the header and fill non-zero values */ @@ -270,14 +271,24 @@ Containers::Optional> TgaImageConverter::doConvertToData break; default: CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } + } + + /* If RLE wasn't used or if a RLE output is larger than uncompressed + output, write an uncompressed output instead */ + if(!rle || (data.size() > uncompressedSize && configuration().value("rleFallbackIfLarger"))) { + if(rle) { + if(flags() & ImageConverterFlag::Verbose) + Debug{} << "Trade::TgaImageConverter::convertToData(): RLE output" << data.size() - uncompressedSize << "bytes larger than uncompressed, falling back to uncompressed"; - /* Turn the array back into a non-growable to avoid a dangling deleter - on plugin unload */ - arrayShrink(data); + /* Resize the array to exactly the uncompressed size (this will + always shrink, never grow) */ + arrayResize(data, NoInit, uncompressedSize); + + /* Remove the RLE bit from the header. Can't use the header + variable from above as it may be dangling at this point. */ + reinterpret_cast(data.begin())->imageType &= ~8; + } - /* Otherwise directly dopy the pixels into output, dropping padding (if - any), and do the BGR(A) conversion on the copied data */ - } else { const Containers::ArrayView pixels = data.exceptPrefix(sizeof(Implementation::TgaHeader)); Utility::copy(image.pixels(), Containers::StridedArrayView3D{pixels, {std::size_t(image.size().y()), std::size_t(image.size().x()), pixelSize}}); @@ -291,6 +302,10 @@ Containers::Optional> TgaImageConverter::doConvertToData } } + /* If we started with a RLE-encoded file, turn the array back into a + non-growable to avoid a dangling deleter on plugin unload */ + if(rle) arrayShrink(data); + /* GCC 4.8 needs extra help here */ return Containers::optional(std::move(data)); } diff --git a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.h b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.h index a7ce08c8c..a483f253b 100644 --- a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.h +++ b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.h @@ -91,10 +91,12 @@ information. @section Trade-TgaImageConverter-behavior Behavior and limitations -The output is RLE-compressed by default, you can produce uncompressed files by -disabling the @cb{.ini} rle @ce @ref Trade-TgaImageConverter-configuration "configuration option". -Enabling @cb{.ini} rleAcrossScanlines @ce will result in even smaller files -but [such files are considered invalid in the TGA 2.0 spec](https://en.wikipedia.org/wiki/Truevision_TGA#Specification_discrepancies) +The output is RLE-compressed by default unless the uncompressed output is +smaller than RLE. You can always produce uncompressed files by disabling the +@cb{.ini} rle @ce @ref Trade-TgaImageConverter-configuration "configuration option", and always RLE files by disabling the +@cb{.ini} rleFallbackIfLarger @ce option. Enabling +@cb{.ini} rleAcrossScanlines @ce can result in even smaller files but +[such files are considered invalid in the TGA 2.0 spec](https://en.wikipedia.org/wiki/Truevision_TGA#Specification_discrepancies) and thus may cause issues in certain importers. The TGA file format doesn't have a way to distinguish between 2D and 1D array