From d51921ff0088404b24018264a128967dc761b3e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 20 Jan 2025 23:24:47 +0100 Subject: [PATCH] Assert non-implementation-specific PixelFormat in Image* constructors. And document that. Because the pixel size cannot be determined for it, and one has to either pass it explicitly or use the templated overload that figures it out implicitly via ADL. This asserted before, but only deep inside in pixelFormatSize(), which may be confusing. I need to do a similar treatment for compressed images with block size properties so let's first make it behave properly for uncompressed. --- src/Magnum/Image.cpp | 4 ++-- src/Magnum/Image.h | 16 ++++++++++++++++ src/Magnum/ImageView.cpp | 4 ++-- src/Magnum/ImageView.h | 16 ++++++++++++++++ src/Magnum/Test/ImageTest.cpp | 22 ++++++++++++++++++++++ src/Magnum/Test/ImageViewTest.cpp | 24 ++++++++++++++++++++++++ src/Magnum/Trade/ImageData.cpp | 10 +++++++++- src/Magnum/Trade/ImageData.h | 8 ++++++++ src/Magnum/Trade/Test/ImageDataTest.cpp | 18 ++++++++++++++++++ 9 files changed, 117 insertions(+), 5 deletions(-) diff --git a/src/Magnum/Image.cpp b/src/Magnum/Image.cpp index 29262d1b2..ebd02211c 100644 --- a/src/Magnum/Image.cpp +++ b/src/Magnum/Image.cpp @@ -32,7 +32,7 @@ namespace Magnum { -template Image::Image(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, const ImageFlags flags) noexcept: Image{storage, format, {}, pixelFormatSize(format), size, Utility::move(data), flags} {} +template Image::Image(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, const ImageFlags flags) noexcept: Image{storage, format, {}, (CORRADE_CONSTEXPR_ASSERT(!isPixelFormatImplementationSpecific(format), "Image: can't determine size of an implementation-specific pixel format" << Debug::hex << pixelFormatUnwrap(format) << Debug::nospace << ", pass it explicitly"), pixelFormatSize(format)), size, Utility::move(data), flags} {} template Image::Image(const PixelStorage storage, const UnsignedInt format, const UnsignedInt formatExtra, const UnsignedInt pixelSize, const VectorTypeFor& size, Containers::Array&& data, const ImageFlags flags) noexcept: Image{storage, pixelFormatWrap(format), formatExtra, pixelSize, size, Utility::move(data), flags} {} @@ -43,7 +43,7 @@ template Image::Image(const PixelStorage sto #endif } -template Image::Image(const PixelStorage storage, const PixelFormat format) noexcept: Image{storage, format, {}, pixelFormatSize(format)} {} +template Image::Image(const PixelStorage storage, const PixelFormat format) noexcept: Image{storage, format, {}, (CORRADE_CONSTEXPR_ASSERT(!isPixelFormatImplementationSpecific(format), "Image: can't determine size of an implementation-specific pixel format" << Debug::hex << pixelFormatUnwrap(format) << Debug::nospace << ", pass it explicitly"), pixelFormatSize(format))} {} template Image::Image(const PixelStorage storage, const UnsignedInt format, const UnsignedInt formatExtra, const UnsignedInt pixelSize) noexcept: Image{storage, pixelFormatWrap(format), formatExtra, pixelSize} {} diff --git a/src/Magnum/Image.h b/src/Magnum/Image.h index 2eda2e841..f13f2b689 100644 --- a/src/Magnum/Image.h +++ b/src/Magnum/Image.h @@ -141,6 +141,14 @@ template class Image { * parameters. For a 3D image, if @p flags contain * @ref ImageFlag3D::CubeMap, the @p size is expected to match its * restrictions. + * + * The @p format is expected to not be implementation-specific, use the + * @ref Image(PixelStorage, PixelFormat, UnsignedInt, UnsignedInt, const VectorTypeFor&, Containers::Array&&, ImageFlags) + * overload to explicitly pass an implementation-specific + * @ref PixelFormat along with a pixel size, or the + * @ref Image(PixelStorage, T, const VectorTypeFor&, Containers::Array&&, ImageFlags) + * overload with the original implementation-specific enum type to have + * the pixel size determined implicitly. */ explicit Image(PixelStorage storage, PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, ImageFlags flags = {}) noexcept; @@ -164,6 +172,14 @@ template class Image { * Size is set to zero, data pointer to @cpp nullptr @ce and data * layout flags are empty. Move over a non-empty instance to make it * useful. + * + * The @p format is expected to not be implementation-specific, use the + * @ref Image(PixelStorage, PixelFormat, UnsignedInt, UnsignedInt) + * overload to explicitly pass an implementation-specific + * @ref PixelFormat along with a pixel size, or the + * @ref Image(PixelStorage, T) overload with the original + * implementation-specific enum type to have the pixel size determined + * implicitly. */ /* No ImageFlags parameter here as this constructor is mainly used to query GL textures, and there the flags are forcibly reset */ diff --git a/src/Magnum/ImageView.cpp b/src/Magnum/ImageView.cpp index af036e303..ae5a09ad0 100644 --- a/src/Magnum/ImageView.cpp +++ b/src/Magnum/ImageView.cpp @@ -31,7 +31,7 @@ namespace Magnum { -template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, const Containers::ArrayView data, const ImageFlags flags) noexcept: ImageView{storage, format, {}, pixelFormatSize(format), size, data, flags} {} +template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, const Containers::ArrayView data, const ImageFlags flags) noexcept: ImageView{storage, format, {}, (CORRADE_CONSTEXPR_ASSERT(!isPixelFormatImplementationSpecific(format), "ImageView: can't determine size of an implementation-specific pixel format" << Debug::hex << pixelFormatUnwrap(format) << Debug::nospace << ", pass it explicitly"), pixelFormatSize(format)), size, data, flags} {} template ImageView::ImageView(const PixelStorage storage, const UnsignedInt format, const UnsignedInt formatExtra, const UnsignedInt pixelSize, const VectorTypeFor& size, const Containers::ArrayView data, const ImageFlags flags) noexcept: ImageView{storage, pixelFormatWrap(format), formatExtra, pixelSize, size, data, flags} {} @@ -42,7 +42,7 @@ template ImageView::ImageView(co #endif } -template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, const ImageFlags flags) noexcept: ImageView{storage, format, {}, pixelFormatSize(format), size, flags} {} +template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, const ImageFlags flags) noexcept: ImageView{storage, format, {}, (CORRADE_CONSTEXPR_ASSERT(!isPixelFormatImplementationSpecific(format), "ImageView: can't determine size of an implementation-specific pixel format" << Debug::hex << pixelFormatUnwrap(format) << Debug::nospace << ", pass it explicitly"), pixelFormatSize(format)), size, flags} {} template ImageView::ImageView(const PixelStorage storage, const UnsignedInt format, const UnsignedInt formatExtra, const UnsignedInt pixelSize, const VectorTypeFor& size, const ImageFlags flags) noexcept: ImageView{storage, pixelFormatWrap(format), formatExtra, pixelSize, size, flags} {} diff --git a/src/Magnum/ImageView.h b/src/Magnum/ImageView.h index b34ec85b7..8ec676435 100644 --- a/src/Magnum/ImageView.h +++ b/src/Magnum/ImageView.h @@ -185,6 +185,14 @@ template class ImageView { * parameters. For a 3D image, if @p flags contain * @ref ImageFlag3D::CubeMap, the @p size is expected to match its * restrictions. + * + * The @p format is expected to not be implementation-specific, use the + * @ref ImageView(PixelStorage, PixelFormat, UnsignedInt, UnsignedInt, const VectorTypeFor&, Containers::ArrayView, ImageFlags) + * overload to explicitly pass an implementation-specific + * @ref PixelFormat along with a pixel size, or the + * @ref ImageView(PixelStorage, U, const VectorTypeFor&, Containers::ArrayView, ImageFlags) + * overload with the original implementation-specific enum type to have + * the pixel size determined implicitly. */ explicit ImageView(PixelStorage storage, PixelFormat format, const VectorTypeFor& size, Containers::ArrayView data, ImageFlags flags = {}) noexcept; @@ -211,6 +219,14 @@ template class ImageView { * assign a memory view to the image. For a 3D image, if @p flags * contain @ref ImageFlag3D::CubeMap, the @p size is expected to match * its restrictions. + * + * The @p format is expected to not be implementation-specific, use the + * @ref ImageView(PixelStorage, PixelFormat, UnsignedInt, UnsignedInt, const VectorTypeFor&, ImageFlags) + * overload to explicitly pass an implementation-specific + * @ref PixelFormat along with a pixel size, or the + * @ref ImageView(PixelStorage, U, const VectorTypeFor&, ImageFlags) + * overload with the original implementation-specific enum type to have + * the pixel size determined implicitly. */ explicit ImageView(PixelStorage storage, PixelFormat format, const VectorTypeFor& size, ImageFlags flags = {}) noexcept; diff --git a/src/Magnum/Test/ImageTest.cpp b/src/Magnum/Test/ImageTest.cpp index 0de7bd4e2..b9903bccf 100644 --- a/src/Magnum/Test/ImageTest.cpp +++ b/src/Magnum/Test/ImageTest.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include /** @todo remove once dataProperties() std::pair is gone */ #include "Magnum/ImageView.h" @@ -51,6 +52,7 @@ struct ImageTest: TestSuite::Tester { void constructCompressedGenericPlaceholder(); void constructCompressedImplementationSpecific(); + void constructUnknownImplementationSpecificPixelSize(); void constructInvalidSize(); void constructInvalidCubeMap(); void constructCompressedInvalidSize(); @@ -108,6 +110,7 @@ ImageTest::ImageTest() { &ImageTest::constructCompressedGenericPlaceholder, &ImageTest::constructCompressedImplementationSpecific, + &ImageTest::constructUnknownImplementationSpecificPixelSize, &ImageTest::constructInvalidSize, &ImageTest::constructInvalidCubeMap, &ImageTest::constructCompressedInvalidSize, @@ -471,6 +474,25 @@ void ImageTest::constructCompressedImplementationSpecific() { /* Manual properties not implemented yet */ } +void ImageTest::constructUnknownImplementationSpecificPixelSize() { + CORRADE_SKIP_IF_NO_ASSERT(); + + Containers::String out; + Error redirectError{&out}; + Image2D{pixelFormatWrap(0x666), {1, 1}, Containers::Array{NoInit, 1}}; + Image2D{pixelFormatWrap(0x777)}; + CORRADE_COMPARE_AS(out, + "Image: can't determine size of an implementation-specific pixel format 0x666, pass it explicitly\n" + /* The second message is printed because it cannot exit the + construction from the middle of the member initializer list. It does + so with non-graceful asserts tho and just one message is printed. */ + "pixelFormatSize(): can't determine size of an implementation-specific format 0x666\n" + + "Image: can't determine size of an implementation-specific pixel format 0x777, pass it explicitly\n" + "pixelFormatSize(): can't determine size of an implementation-specific format 0x777\n", + TestSuite::Compare::String); +} + void ImageTest::constructInvalidSize() { CORRADE_SKIP_IF_NO_ASSERT(); diff --git a/src/Magnum/Test/ImageViewTest.cpp b/src/Magnum/Test/ImageViewTest.cpp index d6b9dc731..018a8c1ee 100644 --- a/src/Magnum/Test/ImageViewTest.cpp +++ b/src/Magnum/Test/ImageViewTest.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include /** @todo remove once dataProperties() std::pair is gone */ #include "Magnum/PixelFormat.h" @@ -60,6 +61,7 @@ struct ImageViewTest: TestSuite::Tester { void constructCompressedFromMutable(); void constructNullptr(); + void constructUnknownImplementationSpecificPixelSize(); void constructInvalidSize(); void constructInvalidCubeMap(); void constructCompressedInvalidSize(); @@ -115,6 +117,7 @@ ImageViewTest::ImageViewTest() { &ImageViewTest::constructCompressedFromMutable, &ImageViewTest::constructNullptr, + &ImageViewTest::constructUnknownImplementationSpecificPixelSize, &ImageViewTest::constructInvalidSize, &ImageViewTest::constructInvalidCubeMap, &ImageViewTest::constructCompressedInvalidSize, @@ -665,6 +668,27 @@ void ImageViewTest::constructNullptr() { CORRADE_COMPARE(out, "ImageView: data too small, got 0 but expected at least 12 bytes\n"); } +void ImageViewTest::constructUnknownImplementationSpecificPixelSize() { + CORRADE_SKIP_IF_NO_ASSERT(); + + char data[1]; + + Containers::String out; + Error redirectError{&out}; + ImageView2D{pixelFormatWrap(0x666), {1, 1}, data}; + ImageView2D{pixelFormatWrap(0x777), {1, 1}}; + CORRADE_COMPARE_AS(out, + "ImageView: can't determine size of an implementation-specific pixel format 0x666, pass it explicitly\n" + /* The second message is printed because it cannot exit the + construction from the middle of the member initializer list. It does + so with non-graceful asserts tho and just one message is printed. */ + "pixelFormatSize(): can't determine size of an implementation-specific format 0x666\n" + + "ImageView: can't determine size of an implementation-specific pixel format 0x777, pass it explicitly\n" + "pixelFormatSize(): can't determine size of an implementation-specific format 0x777\n", + TestSuite::Compare::String); +} + void ImageViewTest::constructInvalidSize() { CORRADE_SKIP_IF_NO_ASSERT(); diff --git a/src/Magnum/Trade/ImageData.cpp b/src/Magnum/Trade/ImageData.cpp index 46f75245a..1751fe429 100644 --- a/src/Magnum/Trade/ImageData.cpp +++ b/src/Magnum/Trade/ImageData.cpp @@ -34,7 +34,15 @@ namespace Magnum { namespace Trade { -template ImageData::ImageData(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, const ImageFlags flags, const void* const importerState) noexcept: ImageData{storage, format, {}, pixelFormatSize(format), size, Utility::move(data), flags, importerState} {} +template ImageData::ImageData(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, const ImageFlags flags, const void* const importerState) noexcept: ImageData{storage, format, {}, ( + /* Unlike with Image and ImageView have to do it like this to avoid an + ungraceful assertion from pixelFormatSize() afterwards */ + #ifndef CORRADE_NO_ASSERT + isPixelFormatImplementationSpecific(format) ? + (CORRADE_CONSTEXPR_ASSERT(false, "Trade::ImageData: can't determine size of an implementation-specific pixel format" << Debug::hex << pixelFormatUnwrap(format) << Debug::nospace << ", pass it explicitly"), 0) : + #endif + pixelFormatSize(format) + ), size, Utility::move(data), flags, importerState} {} template ImageData::ImageData(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size, const DataFlags dataFlags, const Containers::ArrayView data, const ImageFlags flags, const void* const importerState) noexcept: ImageData{storage, format, size, Containers::Array{const_cast(static_cast(data.data())), data.size(), Implementation::nonOwnedArrayDeleter}, flags, importerState} { CORRADE_ASSERT(!(dataFlags & DataFlag::Owned), diff --git a/src/Magnum/Trade/ImageData.h b/src/Magnum/Trade/ImageData.h index 21b51ae3a..c5b67797e 100644 --- a/src/Magnum/Trade/ImageData.h +++ b/src/Magnum/Trade/ImageData.h @@ -165,6 +165,14 @@ template class ImageData { * @ref DataFlag::Owned and @ref DataFlag::Mutable. For non-owned data * use the @ref ImageData(PixelStorage, PixelFormat, const VectorTypeFor&, DataFlags, Containers::ArrayView, ImageFlags, const void*) * constructor instead. + * + * The @p format is expected to not be implementation-specific, use the + * @ref ImageData(PixelStorage, PixelFormat, UnsignedInt, UnsignedInt, const VectorTypeFor&, Containers::Array&&, ImageFlags, const void*) + * overload to explicitly pass an implementation-specific + * @ref PixelFormat along with a pixel size, or the + * @ref ImageData(PixelStorage, T, const VectorTypeFor&, Containers::Array&&, ImageFlags, const void*) + * overload with the original implementation-specific enum type to have + * the pixel size determined implicitly. */ explicit ImageData(PixelStorage storage, PixelFormat format, const VectorTypeFor& size, Containers::Array&& data, ImageFlags flags = {}, const void* importerState = nullptr) noexcept; diff --git a/src/Magnum/Trade/Test/ImageDataTest.cpp b/src/Magnum/Trade/Test/ImageDataTest.cpp index a1d36060e..a627801c1 100644 --- a/src/Magnum/Trade/Test/ImageDataTest.cpp +++ b/src/Magnum/Trade/Test/ImageDataTest.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include /** @todo remove once dataProperties() std::pair is gone */ #include "Magnum/ImageView.h" @@ -57,6 +58,7 @@ struct ImageDataTest: TestSuite::Tester { void constructCompressedGenericNotOwnedFlagOwned(); void constructCompressedImplementationSpecificNotOwnedFlagOwned(); + void constructUnknownImplementationSpecificPixelSize(); void constructInvalidSize(); void constructInvalidCubeMap(); void constructCompressedInvalidSize(); @@ -132,6 +134,7 @@ ImageDataTest::ImageDataTest() { &ImageDataTest::constructCompressedGenericNotOwnedFlagOwned, &ImageDataTest::constructCompressedImplementationSpecificNotOwnedFlagOwned, + &ImageDataTest::constructUnknownImplementationSpecificPixelSize, &ImageDataTest::constructInvalidSize, &ImageDataTest::constructInvalidCubeMap, &ImageDataTest::constructCompressedInvalidSize, @@ -655,6 +658,21 @@ void ImageDataTest::constructCompressedImplementationSpecificNotOwnedFlagOwned() "Trade::ImageData: can't construct a non-owned instance with Trade::DataFlag::Owned\n"); } +void ImageDataTest::constructUnknownImplementationSpecificPixelSize() { + CORRADE_SKIP_IF_NO_ASSERT(); + + char data[1]; + + Containers::String out; + Error redirectError{&out}; + ImageData2D{pixelFormatWrap(0x666), {1, 1}, Containers::Array{NoInit, 1}}; + ImageData2D{pixelFormatWrap(0x777), {1, 1}, DataFlags{}, data}; + CORRADE_COMPARE_AS(out, + "Trade::ImageData: can't determine size of an implementation-specific pixel format 0x666, pass it explicitly\n" + "Trade::ImageData: can't determine size of an implementation-specific pixel format 0x777, pass it explicitly\n", + TestSuite::Compare::String); +} + void ImageDataTest::constructInvalidSize() { CORRADE_SKIP_IF_NO_ASSERT();