From 667e8c1ca9aacb7f4ac14b8844abb4e0439a83cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 24 Jun 2022 17:02:14 +0200 Subject: [PATCH] Trade: restrict multi-level image conversion to same flags as well. They are required to have the same format already, so flags make sense as well -- what's the point of saving a multi-level cube map with one of the levels being just a 2D array, anyway? --- src/Magnum/Trade/AbstractImageConverter.cpp | 6 + src/Magnum/Trade/AbstractImageConverter.h | 110 ++++++++++-------- .../Trade/Test/AbstractImageConverterTest.cpp | 44 +++++++ 3 files changed, 109 insertions(+), 51 deletions(-) diff --git a/src/Magnum/Trade/AbstractImageConverter.cpp b/src/Magnum/Trade/AbstractImageConverter.cpp index ffc82cbf3..9c51de7c9 100644 --- a/src/Magnum/Trade/AbstractImageConverter.cpp +++ b/src/Magnum/Trade/AbstractImageConverter.cpp @@ -537,6 +537,7 @@ template bool checkImageValidity(const char* const messa const PixelFormat format = imageLevels[0].format(); const UnsignedInt formatExtra = imageLevels[0].formatExtra(); + const ImageFlags flags = imageLevels[0].flags(); /* Going through *all* levels although the format assertion is never fired in the first iteration in order to properly check also the first one for zero size / nullptr. */ @@ -549,6 +550,8 @@ template bool checkImageValidity(const char* const messa messagePrefix << "levels don't have the same format, expected" << format << "but got" << imageLevels[i].format() << "for image" << i, false); CORRADE_ASSERT(imageLevels[i].formatExtra() == formatExtra, messagePrefix << "levels don't have the same extra format field, expected" << formatExtra << "but got" << imageLevels[i].formatExtra() << "for image" << i, false); + CORRADE_ASSERT(imageLevels[i].flags() == flags, + messagePrefix << "levels don't have the same flags, expected" << flags << "but got" << imageLevels[i].flags() << "for image" << i, false); } return true; @@ -559,6 +562,7 @@ template bool checkImageValidity(const char* const messa messagePrefix << "at least one image has to be specified", false); const CompressedPixelFormat format = imageLevels[0].format(); + const ImageFlags flags = imageLevels[0].flags(); /* Going through *all* levels although the format assertion is never fired in the first iteration in order to properly check also the first one for zero size / nullptr. */ @@ -569,6 +573,8 @@ template bool checkImageValidity(const char* const messa messagePrefix << "can't convert image" << i << "with a nullptr view", false); CORRADE_ASSERT(imageLevels[i].format() == format, messagePrefix << "levels don't have the same format, expected" << format << "but got" << imageLevels[i].format() << "for image" << i, false); + CORRADE_ASSERT(imageLevels[i].flags() == flags, + messagePrefix << "levels don't have the same flags, expected" << flags << "but got" << imageLevels[i].flags() << "for image" << i, false); } return true; diff --git a/src/Magnum/Trade/AbstractImageConverter.h b/src/Magnum/Trade/AbstractImageConverter.h index aef6e72c1..621e1589e 100644 --- a/src/Magnum/Trade/AbstractImageConverter.h +++ b/src/Magnum/Trade/AbstractImageConverter.h @@ -612,10 +612,10 @@ checked by the implementation: - All @ref doConvertToData() and @ref doConvertToFile() functions taking multiple (compressed) images are called only if the list has at least one image, each of the images has a non-zero size, the views are not - @cpp nullptr @ce and additionally all views have the same pixel format. - Since file formats have varying requirements on image level sizes and their - order and some don't impose any requirements at all, the plugin - implementation is expected to check the sizes on its own. + @cpp nullptr @ce and additionally all views have the same pixel format and + layout flags. Since file formats have varying requirements on image level + sizes and their order and some don't impose any requirements at all, the + plugin implementation is expected to check the sizes on its own. @m_class{m-block m-warning} @@ -1082,11 +1082,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * Available only if @ref ImageConverterFeature::ConvertLevels1DToData * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a - * non-zero size, and all of them sharing the same pixel format. Note - * that certain converters may impose additional size and order - * restrictions on the images, see documentation of a particular plugin - * for more information. On failure prints a message to - * @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * non-zero size, and all of them sharing the same pixel format and + * layout flags. Note that certain converters may impose additional + * size and order restrictions on the images, see documentation of a + * particular plugin for more information. On failure prints a message + * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1115,10 +1115,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1147,10 +1148,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1178,11 +1180,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * Available only if @ref ImageConverterFeature::ConvertCompressedLevels1DToData * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a - * non-zero size, and all views sharing the same pixel format. Note - * that certain converters may impose additional size and order - * restrictions on the images, see documentation of a particular plugin - * for more information. On failure prints a message to - * @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * non-zero size, and all views sharing the same pixel format and + * layout flags. Note that certain converters may impose additional + * size and order restrictions on the images, see documentation of a + * particular plugin for more information. On failure prints a message + * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1211,10 +1213,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1243,10 +1246,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @ref Containers::NullOpt. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @ref Containers::NullOpt. * @see @ref features(), @ref convertToData(Containers::ArrayView), * @ref convert(), @ref convertToFile() */ @@ -1465,10 +1469,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @cpp false @ce. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @cpp false @ce. * @see @ref features(), @ref convertToFile(Containers::ArrayView, Containers::StringView), * @ref convert(), @ref convertToData() */ @@ -1487,10 +1492,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @cpp false @ce. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @cpp false @ce. * @see @ref features(), @ref convertToFile(Containers::ArrayView, Containers::StringView), * @ref convert(), @ref convertToData() */ @@ -1508,11 +1514,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * Available only if @ref ImageConverterFeature::ConvertCompressedLevels1DToFile * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a - * non-zero size, and all views sharing the same pixel format. Note - * that certain converters may impose additional size and order - * restrictions on the images, see documentation of a particular plugin - * for more information. On failure prints a message to - * @relativeref{Magnum,Error} and returns @cpp false @ce. + * non-zero size, and all views sharing the same pixel format and + * layout flags. Note that certain converters may impose additional + * size and order restrictions on the images, see documentation of a + * particular plugin for more information. On failure prints a message + * to @relativeref{Magnum,Error} and returns @cpp false @ce. * @see @ref features(), @ref convertToFile(Containers::ArrayView, Containers::StringView), * @ref convert(), @ref convertToData() */ @@ -1531,10 +1537,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @cpp false @ce. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @cpp false @ce. * @see @ref features(), @ref convertToFile(Containers::ArrayView, Containers::StringView), * @ref convert(), @ref convertToData() */ @@ -1553,10 +1560,11 @@ class MAGNUM_TRADE_EXPORT AbstractImageConverter: public PluginManager::Abstract * is supported. The function expects at least one image to be passed, * with each view expected to not be @cpp nullptr @ce, to have a * non-zero size in all dimensions, and all views sharing the same - * pixel format. Note that certain converters may impose additional - * size and order restrictions on the images, see documentation of a - * particular plugin for more information. On failure prints a message - * to @relativeref{Magnum,Error} and returns @cpp false @ce. + * pixel format and layout flags. Note that certain converters may + * impose additional size and order restrictions on the images, see + * documentation of a particular plugin for more information. On + * failure prints a message to @relativeref{Magnum,Error} and returns + * @cpp false @ce. * @see @ref features(), @ref convertToFile(Containers::ArrayView, Containers::StringView), * @ref convert(), @ref convertToData() */ diff --git a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp index 745ffb21c..9a50296e9 100644 --- a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp @@ -142,6 +142,7 @@ struct AbstractImageConverterTest: TestSuite::Tester { void convertLevels2DToDataNullptr(); void convertLevels2DToDataInconsistentFormat(); void convertLevels2DToDataInconsistentFormatExtra(); + void convertLevels2DToDataInconsistentFlags(); void convertLevels3DToDataInvalidImage(); void convertLevels1DToDataNotImplemented(); void convertLevels2DToDataNotImplemented(); @@ -165,6 +166,7 @@ struct AbstractImageConverterTest: TestSuite::Tester { void convertCompressedLevels2DToDataZeroSize(); void convertCompressedLevels2DToDataNullptr(); void convertCompressedLevels2DToDataInconsistentFormat(); + void convertCompressedLevels2DToDataInconsistentFlags(); void convertCompressedLevels3DToDataInvalidImage(); void convertCompressedLevels1DToDataNotImplemented(); void convertCompressedLevels2DToDataNotImplemented(); @@ -384,6 +386,7 @@ AbstractImageConverterTest::AbstractImageConverterTest() { &AbstractImageConverterTest::convertLevels2DToDataNullptr, &AbstractImageConverterTest::convertLevels2DToDataInconsistentFormat, &AbstractImageConverterTest::convertLevels2DToDataInconsistentFormatExtra, + &AbstractImageConverterTest::convertLevels2DToDataInconsistentFlags, &AbstractImageConverterTest::convertLevels3DToDataInvalidImage, &AbstractImageConverterTest::convertLevels1DToDataNotImplemented, &AbstractImageConverterTest::convertLevels2DToDataNotImplemented, @@ -403,6 +406,7 @@ AbstractImageConverterTest::AbstractImageConverterTest() { &AbstractImageConverterTest::convertCompressedLevels2DToDataZeroSize, &AbstractImageConverterTest::convertCompressedLevels2DToDataNullptr, &AbstractImageConverterTest::convertCompressedLevels2DToDataInconsistentFormat, + &AbstractImageConverterTest::convertCompressedLevels2DToDataInconsistentFlags, &AbstractImageConverterTest::convertCompressedLevels3DToDataInvalidImage, &AbstractImageConverterTest::convertCompressedLevels1DToDataNotImplemented, &AbstractImageConverterTest::convertCompressedLevels2DToDataNotImplemented, @@ -1986,6 +1990,26 @@ void AbstractImageConverterTest::convertLevels2DToDataInconsistentFormatExtra() CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::convertToData(): levels don't have the same extra format field, expected 1037 but got 4467 for image 2\n"); } +void AbstractImageConverterTest::convertLevels2DToDataInconsistentFlags() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImageConverter { + ImageConverterFeatures doFeatures() const override { return ImageConverterFeature::ConvertLevels2DToData; } + } converter; + + const char data[16]{}; + std::ostringstream out; + Error redirectError{&out}; + converter.convertToData({ + ImageView2D{PixelFormat::RGBA8Unorm, {2, 2}, data, ImageFlag2D::Array}, + ImageView2D{PixelFormat::RGBA8Unorm, {1, 1}, data, ImageFlag2D::Array}, + ImageView2D{PixelFormat::RGBA8Unorm, {4, 1}, data} + }); + CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::convertToData(): levels don't have the same flags, expected ImageFlag2D::Array but got ImageFlags2D{} for image 2\n"); +} + void AbstractImageConverterTest::convertLevels3DToDataInvalidImage() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -2308,6 +2332,26 @@ void AbstractImageConverterTest::convertCompressedLevels2DToDataInconsistentForm CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::convertToData(): levels don't have the same format, expected CompressedPixelFormat::Bc1RGBAUnorm but got CompressedPixelFormat::Bc1RGBASrgb for image 2\n"); } +void AbstractImageConverterTest::convertCompressedLevels2DToDataInconsistentFlags() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImageConverter { + ImageConverterFeatures doFeatures() const override { return ImageConverterFeature::ConvertCompressedLevels2DToData; } + } converter; + + const char data[16]{}; + std::ostringstream out; + Error redirectError{&out}; + converter.convertToData({ + CompressedImageView2D{CompressedPixelFormat::Bc1RGBAUnorm, {8, 4}, data, ImageFlag2D::Array}, + CompressedImageView2D{CompressedPixelFormat::Bc1RGBAUnorm, {4, 4}, data, ImageFlag2D::Array}, + CompressedImageView2D{CompressedPixelFormat::Bc1RGBAUnorm, {4, 4}, data}, + }); + CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::convertToData(): levels don't have the same flags, expected ImageFlag2D::Array but got ImageFlags2D{} for image 2\n"); +} + void AbstractImageConverterTest::convertCompressedLevels3DToDataInvalidImage() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");