From 143dc48e87d040931113035dc3882c21b37bad1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 25 Jan 2025 23:56:15 +0100 Subject: [PATCH] Move the occupiedCompressedImageDataSize() helper to the GL library. It's used only there and only to supply a silly argument to a broken API. Unfortunately back when adding this utility in 2015 I didn't document what was it for, which initially made me think it's there for some suspicious reason. Well, the reason is suspicious, but for an entirely different reason. --- src/Magnum/GL/AbstractTexture.cpp | 27 +++++----- src/Magnum/GL/CMakeLists.txt | 1 + src/Magnum/GL/CubeMapTexture.cpp | 9 ++-- .../GL/Implementation/imageProperties.h | 54 +++++++++++++++++++ src/Magnum/Implementation/ImageProperties.h | 6 --- 5 files changed, 74 insertions(+), 23 deletions(-) create mode 100644 src/Magnum/GL/Implementation/imageProperties.h diff --git a/src/Magnum/GL/AbstractTexture.cpp b/src/Magnum/GL/AbstractTexture.cpp index c5ffe5847..630cfb2d2 100644 --- a/src/Magnum/GL/AbstractTexture.cpp +++ b/src/Magnum/GL/AbstractTexture.cpp @@ -44,6 +44,7 @@ #ifndef MAGNUM_TARGET_WEBGL #include "Magnum/GL/Implementation/DebugState.h" #endif +#include "Magnum/GL/Implementation/imageProperties.h" #include "Magnum/GL/Implementation/RendererState.h" #include "Magnum/GL/Implementation/State.h" #include "Magnum/GL/Implementation/TextureState.h" @@ -2173,7 +2174,7 @@ void AbstractTexture::DataHelper<1>::setCompressedImage(AbstractTexture& texture Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); - glCompressedTexImage1D(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size()[0], 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); + glCompressedTexImage1D(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size()[0], 0, Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); } void AbstractTexture::DataHelper<1>::setImage(AbstractTexture& texture, const GLint level, const TextureFormat internalFormat, BufferImage1D& image) { @@ -2187,7 +2188,7 @@ void AbstractTexture::DataHelper<1>::setCompressedImage(AbstractTexture& texture image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); - glCompressedTexImage1D(texture._target, level, GLenum(image.format()), image.size()[0], 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); + glCompressedTexImage1D(texture._target, level, GLenum(image.format()), image.size()[0], 0, Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); } void AbstractTexture::DataHelper<1>::setSubImage(AbstractTexture& texture, const GLint level, const Math::Vector<1, GLint>& offset, const ImageView1D& image) { @@ -2199,7 +2200,7 @@ void AbstractTexture::DataHelper<1>::setSubImage(AbstractTexture& texture, const void AbstractTexture::DataHelper<1>::setCompressedSubImage(AbstractTexture& texture, const GLint level, const Math::Vector<1, GLint>& offset, const CompressedImageView1D& image) { Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage1DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size())); + Context::current().state().texture.compressedSubImage1DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Implementation::occupiedCompressedImageDataSize(image, image.data().size())); } void AbstractTexture::DataHelper<1>::setSubImage(AbstractTexture& texture, const GLint level, const Math::Vector<1, GLint>& offset, BufferImage1D& image) { @@ -2211,7 +2212,7 @@ void AbstractTexture::DataHelper<1>::setSubImage(AbstractTexture& texture, const void AbstractTexture::DataHelper<1>::setCompressedSubImage(AbstractTexture& texture, const GLint level, const Math::Vector<1, GLint>& offset, CompressedBufferImage1D& image) { image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage1DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); + Context::current().state().texture.compressedSubImage1DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); } #endif @@ -2233,7 +2234,7 @@ void AbstractTexture::DataHelper<2>::setCompressedImage(AbstractTexture& texture #endif Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); - glCompressedTexImage2D(target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); + glCompressedTexImage2D(target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), 0, Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); } #ifndef MAGNUM_TARGET_GLES2 @@ -2248,7 +2249,7 @@ void AbstractTexture::DataHelper<2>::setCompressedImage(AbstractTexture& texture image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); - glCompressedTexImage2D(target, level, GLenum(image.format()), image.size().x(), image.size().y(), 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); + glCompressedTexImage2D(target, level, GLenum(image.format()), image.size().x(), image.size().y(), 0, Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); } #endif @@ -2269,7 +2270,7 @@ void AbstractTexture::DataHelper<2>::setCompressedSubImage(AbstractTexture& text Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); #endif Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage2DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size())); + Context::current().state().texture.compressedSubImage2DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Implementation::occupiedCompressedImageDataSize(image, image.data().size())); } #ifndef MAGNUM_TARGET_GLES2 @@ -2282,7 +2283,7 @@ void AbstractTexture::DataHelper<2>::setSubImage(AbstractTexture& texture, const void AbstractTexture::DataHelper<2>::setCompressedSubImage(AbstractTexture& texture, const GLint level, const Vector2i& offset, CompressedBufferImage2D& image) { image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage2DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); + Context::current().state().texture.compressedSubImage2DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); } #endif @@ -2306,9 +2307,9 @@ void AbstractTexture::DataHelper<3>::setCompressedImage(AbstractTexture& texture Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); #ifndef MAGNUM_TARGET_GLES2 - glCompressedTexImage3D(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), image.size().z(), 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); + glCompressedTexImage3D(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), image.size().z(), 0, Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); #else - glCompressedTexImage3DOES(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), image.size().z(), 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); + glCompressedTexImage3DOES(texture._target, level, GLenum(compressedPixelFormat(image.format())), image.size().x(), image.size().y(), image.size().z(), 0, Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); #endif } #endif @@ -2325,7 +2326,7 @@ void AbstractTexture::DataHelper<3>::setCompressedImage(AbstractTexture& texture image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); texture.bindInternal(); - glCompressedTexImage3D(texture._target, level, GLenum(image.format()), image.size().x(), image.size().y(), image.size().z(), 0, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); + glCompressedTexImage3D(texture._target, level, GLenum(image.format()), image.size().x(), image.size().y(), image.size().z(), 0, Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); } #endif @@ -2347,7 +2348,7 @@ void AbstractTexture::DataHelper<3>::setCompressedSubImage(AbstractTexture& text Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); #endif Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage3DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size())); + Context::current().state().texture.compressedSubImage3DImplementation(texture, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Implementation::occupiedCompressedImageDataSize(image, image.data().size())); } #endif @@ -2361,7 +2362,7 @@ void AbstractTexture::DataHelper<3>::setSubImage(AbstractTexture& texture, const void AbstractTexture::DataHelper<3>::setCompressedSubImage(AbstractTexture& texture, const GLint level, const Vector3i& offset, CompressedBufferImage3D& image) { image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.compressedSubImage3DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); + Context::current().state().texture.compressedSubImage3DImplementation(texture, level, offset, image.size(), image.format(), nullptr, Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); } #endif diff --git a/src/Magnum/GL/CMakeLists.txt b/src/Magnum/GL/CMakeLists.txt index 08f4c00a8..8005c7d3b 100644 --- a/src/Magnum/GL/CMakeLists.txt +++ b/src/Magnum/GL/CMakeLists.txt @@ -105,6 +105,7 @@ set(MagnumGL_PRIVATE_HEADERS Implementation/ContextState.h Implementation/compressedPixelFormatMapping.hpp Implementation/FramebufferState.h + Implementation/imageProperties.h Implementation/maxTextureSize.h Implementation/MeshState.h Implementation/QueryState.h diff --git a/src/Magnum/GL/CubeMapTexture.cpp b/src/Magnum/GL/CubeMapTexture.cpp index 58af137c4..2084e7097 100644 --- a/src/Magnum/GL/CubeMapTexture.cpp +++ b/src/Magnum/GL/CubeMapTexture.cpp @@ -38,6 +38,7 @@ #endif #include "Magnum/GL/Context.h" #include "Magnum/GL/PixelFormat.h" +#include "Magnum/GL/Implementation/imageProperties.h" #include "Magnum/GL/Implementation/maxTextureSize.h" #include "Magnum/GL/Implementation/RendererState.h" #include "Magnum/GL/Implementation/State.h" @@ -560,7 +561,7 @@ CubeMapTexture& CubeMapTexture::setCompressedSubImage(const Int level, const Vec Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - glCompressedTextureSubImage3D(_id, level, offset.x(), offset.y(), offset.z(), image.size().x(), image.size().y(), image.size().z(), GLenum(compressedPixelFormat(image.format())), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); + glCompressedTextureSubImage3D(_id, level, offset.x(), offset.y(), offset.z(), image.size().x(), image.size().y(), image.size().z(), GLenum(compressedPixelFormat(image.format())), Implementation::occupiedCompressedImageDataSize(image, image.data().size()), image.data()); return *this; } @@ -571,7 +572,7 @@ CubeMapTexture& CubeMapTexture::setCompressedSubImage(const Int level, const Vec image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - glCompressedTextureSubImage3D(_id, level, offset.x(), offset.y(), offset.z(), image.size().x(), image.size().y(), image.size().z(), GLenum(image.format()), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); + glCompressedTextureSubImage3D(_id, level, offset.x(), offset.y(), offset.z(), image.size().x(), image.size().y(), image.size().z(), GLenum(image.format()), Implementation::occupiedCompressedImageDataSize(image, image.dataSize()), nullptr); return *this; } #endif @@ -603,7 +604,7 @@ CubeMapTexture& CubeMapTexture::setCompressedSubImage(const CubeMapCoordinate co Buffer::unbindInternal(Buffer::TargetHint::PixelUnpack); #endif Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.cubeCompressedSubImageImplementation(*this, coordinate, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Magnum::Implementation::occupiedCompressedImageDataSize(image, image.data().size())); + Context::current().state().texture.cubeCompressedSubImageImplementation(*this, coordinate, level, offset, image.size(), compressedPixelFormat(image.format()), image.data(), Implementation::occupiedCompressedImageDataSize(image, image.data().size())); return *this; } @@ -611,7 +612,7 @@ CubeMapTexture& CubeMapTexture::setCompressedSubImage(const CubeMapCoordinate co CubeMapTexture& CubeMapTexture::setCompressedSubImage(const CubeMapCoordinate coordinate, const Int level, const Vector2i& offset, CompressedBufferImage2D& image) { image.buffer().bindInternal(Buffer::TargetHint::PixelUnpack); Context::current().state().renderer.applyPixelStorageUnpack(image.storage()); - Context::current().state().texture.cubeCompressedSubImageImplementation(*this, coordinate, level, offset, image.size(), image.format(), nullptr, Magnum::Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); + Context::current().state().texture.cubeCompressedSubImageImplementation(*this, coordinate, level, offset, image.size(), image.format(), nullptr, Implementation::occupiedCompressedImageDataSize(image, image.dataSize())); return *this; } #endif diff --git a/src/Magnum/GL/Implementation/imageProperties.h b/src/Magnum/GL/Implementation/imageProperties.h new file mode 100644 index 000000000..07ebe0467 --- /dev/null +++ b/src/Magnum/GL/Implementation/imageProperties.h @@ -0,0 +1,54 @@ +#ifndef Magnum_GL_Implementation_imageProperties_h +#define Magnum_GL_Implementation_imageProperties_h +/* + This file is part of Magnum. + + Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, + 2020, 2021, 2022, 2023, 2024, 2025 + Vladimír Vondruš + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included + in all copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. +*/ + +#include "Magnum/Implementation/ImageProperties.h" + +namespace Magnum { namespace GL { namespace Implementation { + +/* Used in compressed image upload functions. Unlike what common sense and + various robustness extensions would imply, where the size is the memory + range occupied by the data given various pixel storage parameters, it's + instead expected to stupidly be just the image dimensions (*not* row length + etc) in whole blocks. Which has absolutely NO RELATION to the actual memory + and thus is completely useless for enforcing any memory security in the + driver, it's only there to bully users. My suspicion is that whoever did + ARB_compressed_texture_pixel_storage (which makes skip, row length etc. + possible for compressed formats) didn't bother thinking about what the + existing parameter is for, just left it unchanged, and nobody else in the + commitee bothered either. + + In case the block size properties aren't set, the actual image data size is + used as a backup, which might still be correct in most cases. */ +template std::size_t occupiedCompressedImageDataSize(const T& image, std::size_t dataSize) { + return image.storage().compressedBlockSize().product() && image.storage().compressedBlockDataSize() + ? Magnum::Implementation::compressedImageDataOffsetSizeFor(image, image.size()).second : dataSize; +} + +}}} + +#endif diff --git a/src/Magnum/Implementation/ImageProperties.h b/src/Magnum/Implementation/ImageProperties.h index 888cd7850..a55a6ea77 100644 --- a/src/Magnum/Implementation/ImageProperties.h +++ b/src/Magnum/Implementation/ImageProperties.h @@ -117,12 +117,6 @@ template std::size_t compressedImageDataSizeFor return r.first + r.second; } -/* Use in compressed image upload functions */ -template std::size_t occupiedCompressedImageDataSize(const T& image, std::size_t dataSize) { - return image.storage().compressedBlockSize().product() && image.storage().compressedBlockDataSize() - ? compressedImageDataOffsetSizeFor(image, image.size()).second : dataSize; -} - template std::ptrdiff_t pixelStorageSkipOffsetFor(const T& image, const Math::Vector& size) { return image.storage().dataProperties(image.pixelSize(), Vector3i::pad(size, 1)).first.sum(); }