From 3fd3ed36dfd3cc1e1af0ce364a562df4237839df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 19 Aug 2015 17:55:13 +0200 Subject: [PATCH] Pixel storage support, part 6: accepting only sized data arrays in images. Makes it far easier to detect pixel storage misconfigurations and improperly sized data arrays. Data owning classes (Image, Trade::ImageData) accept Containers::Array while wrappers (ImageView, BufferImage) accept Containers::ArrayView. ImageView reinterprets the passed array as const char to enable pointer arithmetic on the data. The old way (constructor/setData() call accepting void*) is now marked as deprecated and will be removed in some future release. Because decay of fixed-size arrays to void* is preferred to calling Containers::ArrayView constructor, there are two more overloads to have proper handling of const T(&)[n] and std::nullptr_t arguments. Currently the TgaImporter and TgaImageConverter fail on images with row length not aligned to 4, will fix that in followup commits. --- src/Magnum/AbstractFramebuffer.cpp | 12 +-- src/Magnum/AbstractTexture.cpp | 18 ++-- src/Magnum/BufferImage.cpp | 10 ++- src/Magnum/BufferImage.h | 48 +++++++++-- src/Magnum/CubeMapTexture.cpp | 18 ++-- src/Magnum/Image.cpp | 10 ++- src/Magnum/Image.h | 85 +++++++++++-------- src/Magnum/ImageView.h | 65 +++++++++++--- src/Magnum/PixelStorage.h | 7 ++ src/Magnum/Test/ImageTest.cpp | 18 ++-- src/Magnum/Test/ImageViewTest.cpp | 2 +- src/Magnum/Trade/ImageData.cpp | 4 + src/Magnum/Trade/ImageData.h | 33 ++++--- .../Trade/Test/AbstractImageConverterTest.cpp | 2 +- src/Magnum/Trade/Test/ImageDataTest.cpp | 16 ++-- .../TgaImageConverter/TgaImageConverter.cpp | 2 +- src/MagnumPlugins/TgaImporter/TgaImporter.cpp | 8 +- 17 files changed, 233 insertions(+), 125 deletions(-) diff --git a/src/Magnum/AbstractFramebuffer.cpp b/src/Magnum/AbstractFramebuffer.cpp index 26ebdc712..bdbecce59 100644 --- a/src/Magnum/AbstractFramebuffer.cpp +++ b/src/Magnum/AbstractFramebuffer.cpp @@ -280,10 +280,9 @@ AbstractFramebuffer& AbstractFramebuffer::clear(const FramebufferClearMask mask) void AbstractFramebuffer::read(const Range2Di& rectangle, Image2D& image) { bindInternal(FramebufferTarget::Read); - const std::size_t dataSize = Implementation::imageDataSizeFor(image, rectangle.size()); - char* const data = new char[dataSize]; - (Context::current()->state().framebuffer->readImplementation)(rectangle, image.format(), image.type(), dataSize, data); - image.setData(image.storage(), image.format(), image.type(), rectangle.size(), data); + Containers::Array data{Implementation::imageDataSizeFor(image, rectangle.size())}; + (Context::current()->state().framebuffer->readImplementation)(rectangle, image.format(), image.type(), data.size(), data); + image.setData(image.storage(), image.format(), image.type(), rectangle.size(), std::move(data)); } Image2D AbstractFramebuffer::read(const Range2Di& rectangle, Image2D&& image) { @@ -296,11 +295,12 @@ void AbstractFramebuffer::read(const Range2Di& rectangle, BufferImage2D& image, bindInternal(FramebufferTarget::Read); /* If the buffer doesn't have sufficient size, resize it */ /** @todo Explicitly reset also when buffer usage changes */ + const std::size_t dataSize = Implementation::imageDataSizeFor(image, rectangle.size()); if(image.size() != rectangle.size()) - image.setData(image.storage(), image.format(), image.type(), rectangle.size(), nullptr, usage); + image.setData(image.storage(), image.format(), image.type(), rectangle.size(), {nullptr, dataSize}, usage); image.buffer().bindInternal(Buffer::TargetHint::PixelPack); - (Context::current()->state().framebuffer->readImplementation)(rectangle, image.format(), image.type(), Implementation::imageDataSizeFor(image, rectangle.size()), nullptr); + (Context::current()->state().framebuffer->readImplementation)(rectangle, image.format(), image.type(), dataSize, nullptr); } BufferImage2D AbstractFramebuffer::read(const Range2Di& rectangle, BufferImage2D&& image, BufferUsage usage) { diff --git a/src/Magnum/AbstractTexture.cpp b/src/Magnum/AbstractTexture.cpp index 149268648..cac07b17f 100644 --- a/src/Magnum/AbstractTexture.cpp +++ b/src/Magnum/AbstractTexture.cpp @@ -1494,12 +1494,11 @@ void AbstractTexture::invalidateSubImageImplementationARB(GLint level, const Vec #ifndef MAGNUM_TARGET_GLES template void AbstractTexture::image(GLint level, Image& image) { const Math::Vector size = DataHelper::imageSize(*this, level); - const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); - char* data = new char[dataSize]; + Containers::Array data{Implementation::imageDataSizeFor(image, size)}; Buffer::unbindInternal(Buffer::TargetHint::PixelPack); - (this->*Context::current()->state().texture->getImageImplementation)(level, image.format(), image.type(), dataSize, data); - image.setData(image.storage(), image.format(), image.type(), size, data); + (this->*Context::current()->state().texture->getImageImplementation)(level, image.format(), image.type(), data.size(), data); + image.setData(image.storage(), image.format(), image.type(), size, std::move(data)); } template void MAGNUM_EXPORT AbstractTexture::image<1>(GLint, Image<1>&); @@ -1510,7 +1509,7 @@ template void AbstractTexture::image(GLint level, Buffer const Math::Vector size = DataHelper::imageSize(*this, level); const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); if(image.size() != size) - image.setData(image.storage(), image.format(), image.type(), size, nullptr, usage); + image.setData(image.storage(), image.format(), image.type(), size, {nullptr, dataSize}, usage); image.buffer().bindInternal(Buffer::TargetHint::PixelPack); (this->*Context::current()->state().texture->getImageImplementation)(level, image.format(), image.type(), dataSize, nullptr); @@ -1560,12 +1559,11 @@ template void AbstractTexture::subImage(const GLint leve const Math::Vector size = range.size(); const Vector3i paddedOffset = Vector3i::pad(range.min()); const Vector3i paddedSize = Vector3i::pad(size, 1); - const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); - char* data = new char[dataSize]; + Containers::Array data{Implementation::imageDataSizeFor(image, size)}; Buffer::unbindInternal(Buffer::TargetHint::PixelPack); - glGetTextureSubImage(_id, level, paddedOffset.x(), paddedOffset.y(), paddedOffset.z(), paddedSize.x(), paddedSize.y(), paddedSize.z(), GLenum(image.format()), GLenum(image.type()), dataSize, data); - image.setData(image.storage(), image.format(), image.type(), size, data); + glGetTextureSubImage(_id, level, paddedOffset.x(), paddedOffset.y(), paddedOffset.z(), paddedSize.x(), paddedSize.y(), paddedSize.z(), GLenum(image.format()), GLenum(image.type()), data.size(), data); + image.setData(image.storage(), image.format(), image.type(), size, std::move(data)); } template void MAGNUM_EXPORT AbstractTexture::subImage<1>(GLint, const Range1Di&, Image<1>&); @@ -1580,7 +1578,7 @@ template void AbstractTexture::subImage(const GLint leve const Vector3i paddedOffset = Vector3i::pad(range.min()); const Vector3i paddedSize = Vector3i::pad(size, 1); if(image.size() != size) - image.setData(image.storage(), image.format(), image.type(), size, nullptr, usage); + image.setData(image.storage(), image.format(), image.type(), size, {nullptr, dataSize}, usage); image.buffer().bindInternal(Buffer::TargetHint::PixelPack); glGetTextureSubImage(_id, level, paddedOffset.x(), paddedOffset.y(), paddedOffset.z(), paddedSize.x(), paddedSize.y(), paddedSize.z(), GLenum(image.format()), GLenum(image.type()), dataSize, nullptr); diff --git a/src/Magnum/BufferImage.cpp b/src/Magnum/BufferImage.cpp index e3a4c0b59..ae8b793c9 100644 --- a/src/Magnum/BufferImage.cpp +++ b/src/Magnum/BufferImage.cpp @@ -28,18 +28,20 @@ namespace Magnum { #ifndef MAGNUM_TARGET_GLES2 -template BufferImage::BufferImage(const PixelStorage storage, const PixelFormat format, const PixelType type, const VectorTypeFor& size, const void* const data, const BufferUsage usage): _storage{storage}, _format{format}, _type{type}, _size{size}, _buffer{Buffer::TargetHint::PixelPack} { - _buffer.setData({data, Implementation::imageDataSizeFor(*this, size)}, usage); +template BufferImage::BufferImage(const PixelStorage storage, const PixelFormat format, const PixelType type, const VectorTypeFor& size, Containers::ArrayView const data, const BufferUsage usage): _storage{storage}, _format{format}, _type{type}, _size{size}, _buffer{Buffer::TargetHint::PixelPack} { + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= data.size(), "BufferImage::BufferImage(): bad image data size, got" << data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); + _buffer.setData(data, usage); } template BufferImage::BufferImage(const PixelStorage storage, const PixelFormat format, const PixelType type): _storage{storage}, _format{format}, _type{type}, _buffer{Buffer::TargetHint::PixelPack} {} -template void BufferImage::setData(const PixelStorage storage, const PixelFormat format, const PixelType type, const VectorTypeFor& size, const void* const data, const BufferUsage usage) { +template void BufferImage::setData(const PixelStorage storage, const PixelFormat format, const PixelType type, const VectorTypeFor& size, Containers::ArrayView const data, const BufferUsage usage) { _storage = storage; _format = format; _type = type; _size = size; - _buffer.setData({data, Implementation::imageDataSizeFor(*this, size)}, usage); + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= data.size(), "BufferImage::setData(): bad image data size, got" << data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); + _buffer.setData(data, usage); } template CompressedBufferImage::CompressedBufferImage( diff --git a/src/Magnum/BufferImage.h b/src/Magnum/BufferImage.h index 7cc9f6b60..e6a8f36c2 100644 --- a/src/Magnum/BufferImage.h +++ b/src/Magnum/BufferImage.h @@ -64,16 +64,30 @@ template class BufferImage { * @param data Image data * @param usage Image buffer usage * - * The data are *not* deleted after filling the buffer. * @todo Make it more flexible (usable with * @extension{ARB,buffer_storage}, avoiding relocations...) */ - explicit BufferImage(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage); + explicit BufferImage(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data, BufferUsage usage); /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - explicit BufferImage(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage): BufferImage{{}, format, type, size, data, usage} {} + explicit BufferImage(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data, BufferUsage usage): BufferImage{{}, format, type, size, data, usage} {} + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief BufferImage(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) + * @deprecated Use @ref BufferImage(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) + * instead. + */ + explicit CORRADE_DEPRECATED("use BufferImage(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) instead") BufferImage(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage): BufferImage{{}, format, type, size, {data, Implementation::imageDataSizeFor(format, type, size)}, usage} {} + + #ifndef DOXYGEN_GENERATING_OUTPUT + /* To avoid decay of sized arrays and nullptr to const void* and + unwanted use of deprecated function */ + template explicit BufferImage(PixelFormat format, PixelType type, const VectorTypeFor& size, const T(&data)[dataSize], BufferUsage usage): BufferImage{{}, format, type, size, Containers::ArrayView{data}, usage} {} + explicit BufferImage(PixelFormat format, PixelType type, const VectorTypeFor& size, std::nullptr_t, BufferUsage usage): BufferImage{{}, format, type, size, Containers::ArrayView{nullptr}, usage} {} + #endif + #endif /** * @brief Constructor @@ -146,21 +160,41 @@ template class BufferImage { * @param data Image data * @param usage Image buffer usage * - * Updates the image buffer with given data. The data are *not* deleted - * after filling the buffer. + * Updates the image buffer with given data. * @see @ref Buffer::setData() * @todo Make it more flexible (usable with * @extension{ARB,buffer_storage}, avoiding relocations...) */ - void setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage); + void setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data, BufferUsage usage); /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage) { + void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data, BufferUsage usage) { setData({}, format, type, size, data, usage); } + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) + * @deprecated Use @ref setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) + * instead. + */ + void CORRADE_DEPRECATED("use setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView, BufferUsage) instead") setData(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data, BufferUsage usage) { + setData({}, format, type, size, {data, Implementation::imageDataSizeFor(format, type, size)}, usage); + } + + #ifndef DOXYGEN_GENERATING_OUTPUT + /* To avoid decay of sized char arrays and nullptr to const void* and + unwanted use of deprecated function */ + template void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, const T(&data)[dataSize], BufferUsage usage) { + setData({}, format, type, size, Containers::ArrayView{data}, usage); + } + void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, std::nullptr_t, BufferUsage usage) { + setData({}, format, type, size, Containers::ArrayView{nullptr}, usage); + } + #endif + #endif + private: PixelStorage _storage; PixelFormat _format; diff --git a/src/Magnum/CubeMapTexture.cpp b/src/Magnum/CubeMapTexture.cpp index 509071b30..8a5c9cbe7 100644 --- a/src/Magnum/CubeMapTexture.cpp +++ b/src/Magnum/CubeMapTexture.cpp @@ -60,11 +60,10 @@ void CubeMapTexture::image(const Int level, Image3D& image) { createIfNotAlready(); const Vector3i size{imageSize(level), 6}; - const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); - char* data = new char[dataSize]; + Containers::Array data{Implementation::imageDataSizeFor(image, size)}; Buffer::unbindInternal(Buffer::TargetHint::PixelPack); - glGetTextureImage(_id, level, GLenum(image.format()), GLenum(image.type()), dataSize, data); - image.setData(image.storage(), image.format(), image.type(), size, data); + glGetTextureImage(_id, level, GLenum(image.format()), GLenum(image.type()), data.size(), data); + image.setData(image.storage(), image.format(), image.type(), size, std::move(data)); } Image3D CubeMapTexture::image(const Int level, Image3D&& image) { @@ -78,7 +77,7 @@ void CubeMapTexture::image(const Int level, BufferImage3D& image, const BufferUs const Vector3i size{imageSize(level), 6}; const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); if(image.size() != size) - image.setData(image.storage(), image.format(), image.type(), size, nullptr, usage); + image.setData(image.storage(), image.format(), image.type(), size, {nullptr, dataSize}, usage); image.buffer().bindInternal(Buffer::TargetHint::PixelPack); glGetTextureImage(_id, level, GLenum(image.format()), GLenum(image.type()), dataSize, nullptr); @@ -132,12 +131,11 @@ CompressedBufferImage3D CubeMapTexture::compressedImage(const Int level, Compres void CubeMapTexture::image(const Coordinate coordinate, const Int level, Image2D& image) { const Vector2i size = imageSize(level); - const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); - char* data = new char[dataSize]; + Containers::Array data{Implementation::imageDataSizeFor(image, size)}; Buffer::unbindInternal(Buffer::TargetHint::PixelPack); - (this->*Context::current()->state().texture->getCubeImageImplementation)(coordinate, level, size, image.format(), image.type(), dataSize, data); - image.setData(image.storage(), image.format(), image.type(), size, data); + (this->*Context::current()->state().texture->getCubeImageImplementation)(coordinate, level, size, image.format(), image.type(), data.size(), data); + image.setData(image.storage(), image.format(), image.type(), size, std::move(data)); } Image2D CubeMapTexture::image(const Coordinate coordinate, const Int level, Image2D&& image) { @@ -149,7 +147,7 @@ void CubeMapTexture::image(const Coordinate coordinate, const Int level, BufferI const Vector2i size = imageSize(level); const std::size_t dataSize = Implementation::imageDataSizeFor(image, size); if(image.size() != size) - image.setData(image.storage(), image.format(), image.type(), size, nullptr, usage); + image.setData(image.storage(), image.format(), image.type(), size, {nullptr, dataSize}, usage); image.buffer().bindInternal(Buffer::TargetHint::PixelPack); (this->*Context::current()->state().texture->getCubeImageImplementation)(coordinate, level, size, image.format(), image.type(), dataSize, nullptr); diff --git a/src/Magnum/Image.cpp b/src/Magnum/Image.cpp index a7003a513..2b94fff65 100644 --- a/src/Magnum/Image.cpp +++ b/src/Magnum/Image.cpp @@ -27,13 +27,17 @@ namespace Magnum { -template void Image::setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, void* data) { - delete[] _data; +template Image::Image(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data): _storage{storage}, _format{format}, _type{type}, _size{size}, _data{std::move(data)} { + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= _data.size(), "Image::Image(): bad image data size, got" << _data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); +} + +template void Image::setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data) { _storage = storage; _format = format; _type = type; _size = size; - _data = reinterpret_cast(data); + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= data.size(), "Image::setData(): bad image data size, got" << data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); + _data = std::move(data); } template void CompressedImage::setData( diff --git a/src/Magnum/Image.h b/src/Magnum/Image.h index 6f7f2a1df..66ff73f1f 100644 --- a/src/Magnum/Image.h +++ b/src/Magnum/Image.h @@ -56,15 +56,23 @@ template class Image { * @param size Image size * @param data Image data * - * Note that the image data are not copied on construction, but they - * are deleted on class destruction. + * The data are expected to be of proper size for given @p storage + * parameters. */ - explicit Image(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): _storage{storage}, _format{format}, _type{type}, _size{size}, _data{reinterpret_cast(data)} {} + explicit Image(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data); /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - explicit Image(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): Image{{}, format, type, size, data} {} + explicit Image(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data): Image{{}, format, type, size, std::move(data)} {} + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief Image(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) + * @deprecated Use @ref Image(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) + * instead. + */ + explicit CORRADE_DEPRECATED("use Image(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) instead") Image(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): Image{{}, format, type, size, Containers::Array{reinterpret_cast(data), Implementation::imageDataSizeFor(format, type, size)}} {} + #endif /** * @brief Constructor @@ -98,9 +106,6 @@ template class Image { /** @brief Move assignment */ Image& operator=(Image&& other) noexcept; - /** @brief Destructor */ - ~Image() { delete[] _data; } - /** @brief Conversion to view */ /*implicit*/ operator ImageView() #ifndef CORRADE_GCC47_COMPATIBILITY @@ -143,17 +148,23 @@ template class Image { } /** - * @brief Pointer to raw data + * @brief Raw data * * @see @ref release() */ + Containers::ArrayView data() { return _data; } + + /** @overload */ + Containers::ArrayView data() const { return _data; } + + /** @overload */ template T* data() { - return reinterpret_cast(_data); + return reinterpret_cast(_data.data()); } /** @overload */ template const T* data() const { - return reinterpret_cast(_data); + return reinterpret_cast(_data.data()); } /** @@ -164,34 +175,44 @@ template class Image { * @param size Image size * @param data Image data * - * Deletes previous data and replaces them with new. Note that the - * data are not copied, but they are deleted on destruction. + * Deletes previous data and replaces them with new. The data are + * expected to be of proper size for given @p storage parameters. * @see @ref release() */ - void setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, void* data); + void setData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data); /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data) { - setData({}, format, type, size, data); + void setData(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data) { + setData({}, format, type, size, std::move(data)); + } + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) + * @deprecated Use @ref setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) + * instead. + */ + void CORRADE_DEPRECATED("use setData(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView) instead") setData(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data) { + setData({}, format, type, size, Containers::Array{reinterpret_cast(data), Implementation::imageDataSizeFor(format, type, size)}); } + #endif /** * @brief Release data storage * - * Releases the ownership of the data pointer and resets internal state - * to default. Deleting the returned array is then user responsibility. - * @see @ref setData() + * Releases the ownership of the data array and resets internal state + * to default. + * @see @ref data() */ - char* release(); + Containers::Array release(); private: PixelStorage _storage; PixelFormat _format; PixelType _type; Math::Vector _size; - char* _data; + Containers::Array _data; }; /** @brief One-dimensional image */ @@ -327,17 +348,17 @@ template class CompressedImage { } #endif - /** @brief Raw data */ + /** + * @brief Raw data + * + * @see @ref release() + */ Containers::ArrayView data() { return _data; } /** @overload */ Containers::ArrayView data() const { return _data; } - /** - * @brief Pointer to raw data - * - * @see @ref release() - */ + /** @overload */ template T* data() { return reinterpret_cast(_data.data()); } @@ -379,7 +400,7 @@ template class CompressedImage { /** * @brief Release data storage * - * Releases the ownership of the data pointer and resets internal state + * Releases the ownership of the data array and resets internal state * to default. * @see @ref setData() */ @@ -405,7 +426,6 @@ typedef CompressedImage<3> CompressedImage3D; template inline Image::Image(Image&& other) noexcept: _storage{std::move(other._storage)}, _format{std::move(other._format)}, _type{std::move(other._type)}, _size{std::move(other._size)}, _data{std::move(other._data)} { other._size = {}; - other._data = nullptr; } template inline CompressedImage::CompressedImage(CompressedImage&& other) noexcept: @@ -415,7 +435,6 @@ template inline CompressedImage::CompressedI _format{std::move(other._format)}, _size{std::move(other._size)}, _data{std::move(other._data)} { other._size = {}; - other._data = nullptr; } template inline Image& Image::operator=(Image&& other) noexcept { @@ -463,19 +482,15 @@ const _format, _size, _data}; } -template inline char* Image::release() { - /** @todo I need `std::exchange` NOW. */ - char* const data = _data; +template inline Containers::Array Image::release() { + Containers::Array data{std::move(_data)}; _size = {}; - _data = nullptr; return data; } template inline Containers::Array CompressedImage::release() { - /** @todo I need `std::exchange` NOW. */ Containers::Array data{std::move(_data)}; _size = {}; - _data = nullptr; return data; } diff --git a/src/Magnum/ImageView.h b/src/Magnum/ImageView.h index 6e4e339d3..8ec92ed05 100644 --- a/src/Magnum/ImageView.h +++ b/src/Magnum/ImageView.h @@ -66,13 +66,32 @@ template class ImageView { * @param type Data type of pixel data * @param size Image size * @param data Image data + * + * The data are expected to be of proper size for given @p storage + * parameters. */ - constexpr explicit ImageView(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data) noexcept: _storage{storage}, _format{format}, _type{type}, _size{size}, _data{reinterpret_cast(data)} {} + explicit ImageView(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data) noexcept: _storage{storage}, _format{format}, _type{type}, _size{size}, _data{reinterpret_cast(data.data()), data.size()} { + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= _data.size(), "ImageView::ImageView(): bad image data size, got" << _data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); + } /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - constexpr explicit ImageView(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data) noexcept: ImageView{{}, format, type, size, data} {} + explicit ImageView(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::ArrayView data) noexcept: ImageView{{}, format, type, size, data} {} + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief ImageView(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView) + * @deprecated Use @ref ImageView(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView) instead. + */ + explicit CORRADE_DEPRECATED("use ImageView(PixelFormat, PixelType, const VectorTypeFor&, Containers::ArrayView) instead") ImageView(PixelFormat format, PixelType type, const VectorTypeFor& size, const void* data) noexcept: ImageView{{}, format, type, size, {reinterpret_cast(data), Implementation::imageDataSizeFor(format, type, size)}} {} + + #ifndef DOXYGEN_GENERATING_OUTPUT + /* To avoid decay of sized arrays and nullptr to const void* and + unwanted use of deprecated function */ + template explicit ImageView(PixelFormat format, PixelType type, const VectorTypeFor& size, const T(&data)[dataSize]): ImageView{{}, format, type, size, Containers::ArrayView{data}} {} + explicit ImageView(PixelFormat format, PixelType type, const VectorTypeFor& size, std::nullptr_t): ImageView{{}, format, type, size, Containers::ArrayView{nullptr}} {} + #endif + #endif /** * @brief Constructor @@ -119,32 +138,54 @@ template class ImageView { return Implementation::imageDataProperties(*this); } - /** @brief Pointer to raw data */ - constexpr const char* data() const { return _data; } + /** @brief Raw data */ + constexpr Containers::ArrayView data() const { return _data; } /** @overload */ - template const T* data() const { - return reinterpret_cast(_data); + template const T* data() const { + return reinterpret_cast(_data.data()); } /** * @brief Set image data * @param data Image data * - * Dimensions, color compnents and data type remains the same as - * passed in constructor. The data are not copied nor deleted on - * destruction. + * Storage parameters, pixel format, type and size remain the same as + * passed in constructor. The data are expected to be of proper size + * for given @p storage parameters. + */ + void setData(Containers::ArrayView data) { + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= data.size(), "ImageView::setData(): bad image data size, got" << data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); + _data = {reinterpret_cast(data.data()), data.size()}; + } + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief setData(Containers::ArrayView) + * @deprecated Use @ref setData(Containers::ArrayView) + * instead. */ - void setData(const void* data) { - _data = reinterpret_cast(data); + void CORRADE_DEPRECATED("use setData(Containers::ArrayView) instead") setData(const void* data) { + setData({reinterpret_cast(data), Implementation::imageDataSize(*this)}); } + #ifndef DOXYGEN_GENERATING_OUTPUT + /* To avoid decay of sized arrays and nullptr to const void* and + unwanted use of deprecated function */ + template void setData(const T(&data)[size]) { + setData(Containers::ArrayView{data}); + } + void setData(std::nullptr_t) { + setData(Containers::ArrayView{nullptr}); + } + #endif + #endif + private: PixelStorage _storage; PixelFormat _format; PixelType _type; Math::Vector _size; - const char* _data; + Containers::ArrayView _data; }; /** @brief One-dimensional image view */ diff --git a/src/Magnum/PixelStorage.h b/src/Magnum/PixelStorage.h index 1782ce796..9e2d1c0c5 100644 --- a/src/Magnum/PixelStorage.h +++ b/src/Magnum/PixelStorage.h @@ -376,6 +376,13 @@ namespace Implementation { return imageDataSizeFor(image, image.size()); } + #ifdef MAGNUM_BUILD_DEPRECATED + /* Uses default pixel storage */ + template std::size_t imageDataSizeFor(PixelFormat format, PixelType type, const Math::Vector& size) { + return std::get<1>(PixelStorage{}.dataProperties(format, type, Vector3i::pad(size, 1))).product(); + } + #endif + #ifndef MAGNUM_TARGET_GLES /* Used in image query functions */ template std::size_t compressedImageDataSizeFor(const T& image, const Math::Vector& size, std::size_t dataSize) { diff --git a/src/Magnum/Test/ImageTest.cpp b/src/Magnum/Test/ImageTest.cpp index e729fb82b..e9be84555 100644 --- a/src/Magnum/Test/ImageTest.cpp +++ b/src/Magnum/Test/ImageTest.cpp @@ -67,7 +67,7 @@ ImageTest::ImageTest() { void ImageTest::construct() { auto data = new char[3]; Image2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; CORRADE_COMPARE(a.storage().alignment(), 1); CORRADE_COMPARE(a.format(), PixelFormat::Red); @@ -106,7 +106,7 @@ void ImageTest::constructCopyCompressed() { void ImageTest::constructMove() { auto data = new char[3]; Image2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; Image2D b(std::move(a)); CORRADE_COMPARE(a.data(), nullptr); @@ -119,7 +119,7 @@ void ImageTest::constructMove() { CORRADE_COMPARE(b.data(), data); auto data2 = new char[12*4*2]; - Image2D c(PixelFormat::RGBA, PixelType::UnsignedShort, {2, 6}, data2); + Image2D c{PixelFormat::RGBA, PixelType::UnsignedShort, {2, 6}, Containers::Array{data2, 12*4*2}}; c = std::move(b); CORRADE_COMPARE(b.data(), data2); @@ -172,9 +172,9 @@ void ImageTest::constructMoveCompressed() { void ImageTest::setData() { auto data = new char[3]; Image2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; - auto data2 = new char[2*4]; - a.setData(PixelFormat::RGBA, PixelType::UnsignedShort, {2, 1}, data2); + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; + auto data2 = new char[2*2*4]; + a.setData(PixelFormat::RGBA, PixelType::UnsignedShort, {2, 1}, Containers::Array{data2, 2*2*4}); CORRADE_COMPARE(a.storage().alignment(), 4); CORRADE_COMPARE(a.format(), PixelFormat::RGBA); @@ -205,7 +205,7 @@ void ImageTest::setDataCompressed() { void ImageTest::toReference() { auto data = new char[3]; const Image2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; ImageView2D b = a; CORRADE_COMPARE(b.storage().alignment(), 1); @@ -253,8 +253,8 @@ void ImageTest::toReferenceCommpressed() { void ImageTest::release() { char data[] = {'c', 'a', 'f', 'e'}; - Image2D a(PixelFormat::Red, PixelType::UnsignedByte, {1, 4}, data); - const char* const pointer = a.release(); + Image2D a(PixelFormat::Red, PixelType::UnsignedByte, {4, 1}, Containers::Array{data, 4}); + const char* const pointer = a.release().release(); CORRADE_COMPARE(pointer, data); CORRADE_COMPARE(a.data(), nullptr); diff --git a/src/Magnum/Test/ImageViewTest.cpp b/src/Magnum/Test/ImageViewTest.cpp index f42138c7f..55477ec83 100644 --- a/src/Magnum/Test/ImageViewTest.cpp +++ b/src/Magnum/Test/ImageViewTest.cpp @@ -80,7 +80,7 @@ void ImageViewTest::setData() { const char data[3]{}; ImageView2D a{PixelStorage{}.setAlignment(1), PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; - const char data2[8]{}; + const char data2[3]{}; a.setData(data2); CORRADE_COMPARE(a.storage().alignment(), 1); diff --git a/src/Magnum/Trade/ImageData.cpp b/src/Magnum/Trade/ImageData.cpp index 83ec7c179..08de17bdf 100644 --- a/src/Magnum/Trade/ImageData.cpp +++ b/src/Magnum/Trade/ImageData.cpp @@ -27,6 +27,10 @@ namespace Magnum { namespace Trade { +template ImageData::ImageData(const PixelStorage storage, const PixelFormat format, const PixelType type, const VectorTypeFor& size, Containers::Array&& data): _compressed{false}, _storage{storage}, _format{format}, _type{type}, _size{size}, _data{std::move(data)} { + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= _data.size(), "Trade::ImageDat::ImageData(): bad image data size, got" << _data.size() << "but expected at least" << Implementation::imageDataSize(*this), ); +} + template PixelStorage ImageData::storage() const { CORRADE_ASSERT(!_compressed, "Trade::ImageData::storage(): the image is compressed", {}); return _storage; diff --git a/src/Magnum/Trade/ImageData.h b/src/Magnum/Trade/ImageData.h index fcf842d7a..169d2adda 100644 --- a/src/Magnum/Trade/ImageData.h +++ b/src/Magnum/Trade/ImageData.h @@ -64,15 +64,22 @@ template class ImageData { * @param size Image size * @param data Image data * - * Note that the image data are not copied on construction, but they - * are deleted on class destruction. + * The data are expected to be of proper size for given @p storage + * parameters. */ - explicit ImageData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): _compressed{false}, _storage{storage}, _format{format}, _type{type}, _size{size}, _data{reinterpret_cast(data), Implementation::imageDataSizeFor(*this, size)} {} + explicit ImageData(PixelStorage storage, PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data); /** @overload * Similar to the above, but uses default @ref PixelStorage parameters. */ - explicit ImageData(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): ImageData{{}, format, type, size, data} {} + explicit ImageData(PixelFormat format, PixelType type, const VectorTypeFor& size, Containers::Array&& data): ImageData{{}, format, type, size, std::move(data)} {} + + #ifdef MAGNUM_BUILD_DEPRECATED + /** @copybrief ImageData(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) + * @deprecated Use @ref ImageData(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) instead. + */ + explicit CORRADE_DEPRECATED("use ImageData(PixelFormat, PixelType, const VectorTypeFor&, Containers::Array&&) instead") ImageData(PixelFormat format, PixelType type, const VectorTypeFor& size, void* data): ImageData{format, type, size, Containers::Array{reinterpret_cast(data), Magnum::Implementation::imageDataSizeFor(format, type, size)}} {} + #endif #ifndef MAGNUM_TARGET_GLES /** @@ -222,17 +229,17 @@ template class ImageData { are not setting any block size pixel storage properties to avoid needless state changes -- thus the calculation can't be done */ - /** @brief Raw data */ + /** + * @brief Raw data + * + * @see @ref release() + */ Containers::ArrayView data() { return _data; } /** @overload */ Containers::ArrayView data() const { return _data; } - /** - * @brief Pointer to raw data - * - * @see @ref release() - */ + /** @overload */ template T* data() { return reinterpret_cast(_data.data()); } @@ -245,8 +252,8 @@ template class ImageData { /** * @brief Release data storage * - * Releases the ownership of the data pointer and resets internal state - * to default. Deleting the returned array is then user responsibility. + * Releases the ownership of the data array and resets internal state + * to default. * @see @ref data() */ Containers::Array release(); @@ -305,7 +312,6 @@ template inline ImageData::ImageData(ImageDa } other._size = {}; - other._data = nullptr; } template inline ImageData& ImageData::operator=(ImageData&& other) noexcept { @@ -330,7 +336,6 @@ template inline ImageData& ImageData inline Containers::Array ImageData::release() { Containers::Array data{std::move(_data)}; _size = {}; - _data = nullptr; return data; } diff --git a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp index 92c619b0c..644e330da 100644 --- a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp @@ -62,7 +62,7 @@ void AbstractImageConverterTest::exportToFile() { /* doExportToFile() should call doExportToData() */ DataExporter exporter; - ImageView2D image(PixelFormat::RGBA, PixelType::UnsignedByte, {0xfe, 0xed}, nullptr); + ImageView2D image(PixelFormat::RGBA, PixelType::UnsignedByte, {0xfe, 0xed}, {nullptr, 0xfe*0xed*4}); CORRADE_VERIFY(exporter.exportToFile(image, Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "image.out"))); CORRADE_COMPARE_AS(Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "image.out"), "\xFE\xED", TestSuite::Compare::FileToString); diff --git a/src/Magnum/Trade/Test/ImageDataTest.cpp b/src/Magnum/Trade/Test/ImageDataTest.cpp index f31625a8b..ec6f4e9ce 100644 --- a/src/Magnum/Trade/Test/ImageDataTest.cpp +++ b/src/Magnum/Trade/Test/ImageDataTest.cpp @@ -62,7 +62,7 @@ ImageDataTest::ImageDataTest() { void ImageDataTest::construct() { auto data = new char[3]; Trade::ImageData2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; CORRADE_VERIFY(!a.isCompressed()); CORRADE_COMPARE(a.storage().alignment(), 1); @@ -98,7 +98,7 @@ void ImageDataTest::constructCopy() { void ImageDataTest::constructMove() { auto data = new char[3]; Trade::ImageData2D a{PixelStorage{}.setAlignment(1), - PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data}; + PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, Containers::Array{data, 3}}; Trade::ImageData2D b(std::move(a)); CORRADE_COMPARE(a.data(), nullptr); @@ -111,8 +111,8 @@ void ImageDataTest::constructMove() { CORRADE_COMPARE(b.size(), Vector2i(1, 3)); CORRADE_COMPARE(b.data(), data); - auto data2 = new char[3]; - Trade::ImageData2D c(PixelFormat::RGBA, PixelType::UnsignedShort, {2, 6}, data2); + auto data2 = new char[2*2*6*4]; + Trade::ImageData2D c{PixelFormat::RGBA, PixelType::UnsignedShort, {2, 6}, Containers::Array{data2, 2*2*6*4}}; c = std::move(b); CORRADE_COMPARE(b.data(), data2); @@ -166,13 +166,13 @@ void ImageDataTest::constructMoveCompressed() { } void ImageDataTest::toReference() { - auto data = new char[3]; - const Trade::ImageData2D a(PixelFormat::Red, PixelType::UnsignedByte, {1, 3}, data); + auto data = new char[4]; + const Trade::ImageData2D a{PixelFormat::Red, PixelType::UnsignedByte, {4, 1}, Containers::Array{data, 4}}; ImageView2D b = a; CORRADE_COMPARE(b.format(), PixelFormat::Red); CORRADE_COMPARE(b.type(), PixelType::UnsignedByte); - CORRADE_COMPARE(b.size(), Vector2i(1, 3)); + CORRADE_COMPARE(b.size(), Vector2i(4, 1)); CORRADE_COMPARE(b.data(), data); CORRADE_VERIFY((std::is_convertible::value)); @@ -207,7 +207,7 @@ void ImageDataTest::toReferenceCompressed() { void ImageDataTest::release() { char data[] = {'b', 'e', 'e', 'r'}; - Trade::ImageData2D a(PixelFormat::Red, PixelType::UnsignedByte, {1, 4}, data); + Trade::ImageData2D a{PixelFormat::Red, PixelType::UnsignedByte, {4, 1}, Containers::Array{data, 4}}; const char* const pointer = a.release().release(); CORRADE_COMPARE(pointer, data); diff --git a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp index 82c9a3d6a..aed8dc2d4 100644 --- a/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp +++ b/src/MagnumPlugins/TgaImageConverter/TgaImageConverter.cpp @@ -91,7 +91,7 @@ Containers::Array TgaImageConverter::doExportToData(const ImageView2D& ima header->height = UnsignedShort(Utility::Endianness::littleEndian(image.size().y())); /* Fill data */ - std::copy(image.data(), image.data()+pixelSize*image.size().product(), data.begin()+sizeof(TgaHeader)); + std::copy(image.data().data(), image.data()+pixelSize*image.size().product(), data.begin()+sizeof(TgaHeader)); if(image.format() == PixelFormat::RGB) { auto pixels = reinterpret_cast*>(data.begin()+sizeof(TgaHeader)); diff --git a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp index 263eade48..9a8ceb5e3 100644 --- a/src/MagnumPlugins/TgaImporter/TgaImporter.cpp +++ b/src/MagnumPlugins/TgaImporter/TgaImporter.cpp @@ -133,22 +133,22 @@ std::optional TgaImporter::doImage2D(UnsignedInt) { } const std::size_t dataSize = header.width*header.height*header.bpp/8; - char* const data = new char[dataSize]; + Containers::Array data{dataSize}; in->read(data, dataSize); Vector2i size(header.width, header.height); if(format == PixelFormat::RGB) { - auto pixels = reinterpret_cast*>(data); + auto pixels = reinterpret_cast*>(data.data()); std::transform(pixels, pixels + size.product(), pixels, [](Math::Vector3 pixel) { return Math::swizzle<'b', 'g', 'r'>(pixel); }); } else if(format == PixelFormat::RGBA) { - auto pixels = reinterpret_cast*>(data); + auto pixels = reinterpret_cast*>(data.data()); std::transform(pixels, pixels + size.product(), pixels, [](Math::Vector4 pixel) { return Math::swizzle<'b', 'g', 'r', 'a'>(pixel); }); } - return ImageData2D(format, PixelType::UnsignedByte, size, data); + return ImageData2D{format, PixelType::UnsignedByte, size, std::move(data)}; } }}