From a159835bf7ec2628ee477bd9b09da1cad6104096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 6 Oct 2023 21:28:22 +0200 Subject: [PATCH] DebugTools: check framebuffer completeness in textureSubImage(). Ugh. Was using this to verify that the glyph cache was correctly populated, only to end up with a GL error that I thought was coming from the glyph cache itself and not here. Wasted too much time on that. --- doc/changelog.dox | 3 + src/Magnum/DebugTools/CMakeLists.txt | 4 +- src/Magnum/DebugTools/Test/CMakeLists.txt | 2 +- .../DebugTools/Test/TextureImageGLTest.cpp | 140 ++++++++++++++++++ src/Magnum/DebugTools/TextureImage.cpp | 32 +++- src/Magnum/DebugTools/TextureImage.h | 32 ++-- 6 files changed, 187 insertions(+), 26 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 095ebe235..f75fdcec7 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -527,6 +527,9 @@ See also: match sRGB and normalization properties of the expected image format - @ref DebugTools::CompareImage now accepts @ref MutableImageView2D as well, in addition to @ref Image2D, @ref ImageView2D and @ref Trade::ImageData2D +- @ref DebugTools::textureSubImage() now checks that the framebuffer is + complete before attempting to read from it to avoid silent failures when + the texture format isn't framebuffer readable @subsubsection changelog-latest-changes-gl GL library diff --git a/src/Magnum/DebugTools/CMakeLists.txt b/src/Magnum/DebugTools/CMakeLists.txt index 962da3581..44b20a6a9 100644 --- a/src/Magnum/DebugTools/CMakeLists.txt +++ b/src/Magnum/DebugTools/CMakeLists.txt @@ -51,7 +51,9 @@ endif() if(MAGNUM_TARGET_GL) list(APPEND MagnumDebugTools_SRCS ResourceManager.cpp - Screenshot.cpp + Screenshot.cpp) + + list(APPEND MagnumDebugTools_GracefulAssert_SRCS TextureImage.cpp) list(APPEND MagnumDebugTools_HEADERS diff --git a/src/Magnum/DebugTools/Test/CMakeLists.txt b/src/Magnum/DebugTools/Test/CMakeLists.txt index d82114309..496e71405 100644 --- a/src/Magnum/DebugTools/Test/CMakeLists.txt +++ b/src/Magnum/DebugTools/Test/CMakeLists.txt @@ -113,7 +113,7 @@ if(MAGNUM_TARGET_GL) corrade_add_test(DebugToolsFrameProfilerGLTest FrameProfilerGLTest.cpp LIBRARIES MagnumDebugTools MagnumOpenGLTester) corrade_add_test(DebugToolsTextureImageGLTest TextureImageGLTest.cpp - LIBRARIES MagnumDebugTools MagnumOpenGLTester) + LIBRARIES MagnumDebugToolsTestLib MagnumOpenGLTester) if(NOT (MAGNUM_TARGET_WEBGL AND MAGNUM_TARGET_GLES2)) corrade_add_test(DebugToolsBufferDataGLTest BufferDataGLTest.cpp diff --git a/src/Magnum/DebugTools/Test/TextureImageGLTest.cpp b/src/Magnum/DebugTools/Test/TextureImageGLTest.cpp index 12147d215..4e940fb4f 100644 --- a/src/Magnum/DebugTools/Test/TextureImageGLTest.cpp +++ b/src/Magnum/DebugTools/Test/TextureImageGLTest.cpp @@ -23,7 +23,9 @@ DEALINGS IN THE SOFTWARE. */ +#include #include +#include #include "Magnum/Image.h" #include "Magnum/ImageView.h" @@ -53,14 +55,18 @@ struct TextureImageGLTest: GL::OpenGLTester { glGetBufferSubData(). */ void subImage2D(); + void subImage2DNotReadable(); #ifndef MAGNUM_TARGET_GLES2 void subImage2DBuffer(); + void subImage2DBufferNotReadable(); #endif void subImage2DGeneric(); void subImageCube(); + void subImageCubeNotReadable(); #ifndef MAGNUM_TARGET_GLES2 void subImageCubeBuffer(); + void subImageCubeBufferNotReadable(); #endif #ifndef MAGNUM_TARGET_GLES2 @@ -72,14 +78,18 @@ struct TextureImageGLTest: GL::OpenGLTester { TextureImageGLTest::TextureImageGLTest() { addTests({&TextureImageGLTest::subImage2D, + &TextureImageGLTest::subImage2DNotReadable, #ifndef MAGNUM_TARGET_GLES2 &TextureImageGLTest::subImage2DBuffer, + &TextureImageGLTest::subImage2DBufferNotReadable, #endif &TextureImageGLTest::subImage2DGeneric, &TextureImageGLTest::subImageCube, + &TextureImageGLTest::subImageCubeNotReadable, #ifndef MAGNUM_TARGET_GLES2 &TextureImageGLTest::subImageCubeBuffer, + &TextureImageGLTest::subImageCubeBufferNotReadable, #endif #ifndef MAGNUM_TARGET_GLES2 @@ -115,6 +125,35 @@ void TextureImageGLTest::subImage2D() { Containers::arrayView(Data2D), TestSuite::Compare::Container); } +void TextureImageGLTest::subImage2DNotReadable() { + CORRADE_SKIP_IF_NO_ASSERT(); + + #ifndef MAGNUM_TARGET_GLES + if(!GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::EXT::texture_shared_exponent::string() << "not supported, can't test"); + if(GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::ARB::get_texture_sub_image::string() << "supported, can't test"); + #endif + + GL::Texture2D texture; + #ifdef MAGNUM_TARGET_GLES2 + texture.setImage(0, GL::TextureFormat::Luminance, ImageView2D{GL::PixelFormat::Luminance, GL::PixelType::UnsignedByte, Vector2i{2}, Data2D}); + #else + texture.setImage(0, GL::TextureFormat::RGB9E5, ImageView2D{GL::PixelFormat::RGB, GL::PixelType::UnsignedInt5999Rev, Vector2i{2}, Data2D}); + #endif + + std::ostringstream out; + Error redirectError{&out}; + /* The read type doesn't have to match, it doesn't get that far */ + textureSubImage(texture, 0, {{}, Vector2i{2}}, {GL::PixelFormat::RGBA, GL::PixelType::UnsignedByte}); + MAGNUM_VERIFY_NO_GL_ERROR(); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::Unsupported\n"); + #else + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::IncompleteAttachment\n"); + #endif +} + #ifndef MAGNUM_TARGET_GLES2 void TextureImageGLTest::subImage2DBuffer() { GL::Texture2D texture; @@ -132,6 +171,31 @@ void TextureImageGLTest::subImage2DBuffer() { Containers::arrayView(Data2D), TestSuite::Compare::Container); } + +void TextureImageGLTest::subImage2DBufferNotReadable() { + CORRADE_SKIP_IF_NO_ASSERT(); + + #ifndef MAGNUM_TARGET_GLES + if(!GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::EXT::texture_shared_exponent::string() << "not supported, can't test"); + if(GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::ARB::get_texture_sub_image::string() << "supported, can't test"); + #endif + + GL::Texture2D texture; + texture.setImage(0, GL::TextureFormat::RGB9E5, ImageView2D{GL::PixelFormat::RGB, GL::PixelType::UnsignedInt5999Rev, Vector2i{2}, Data2D}); + + std::ostringstream out; + Error redirectError{&out}; + /* The read type doesn't have to match, it doesn't get that far */ + textureSubImage(texture, 0, {{}, Vector2i{2}}, {GL::PixelFormat::RGBA, GL::PixelType::UnsignedByte}, GL::BufferUsage::StaticRead); + MAGNUM_VERIFY_NO_GL_ERROR(); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::Unsupported\n"); + #else + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::IncompleteAttachment\n"); + #endif +} #endif void TextureImageGLTest::subImage2DGeneric() { @@ -181,6 +245,44 @@ void TextureImageGLTest::subImageCube() { Containers::arrayView(Data2D), TestSuite::Compare::Container); } +void TextureImageGLTest::subImageCubeNotReadable() { + CORRADE_SKIP_IF_NO_ASSERT(); + + #ifndef MAGNUM_TARGET_GLES + if(!GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::EXT::texture_shared_exponent::string() << "not supported, can't test"); + if(GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::ARB::get_texture_sub_image::string() << "supported, can't test"); + #endif + + #ifdef MAGNUM_TARGET_GLES2 + GL::TextureFormat format = GL::TextureFormat::Luminance; + ImageView2D view{GL::PixelFormat::Luminance, GL::PixelType::UnsignedByte, Vector2i{2}, Data2D}; + #else + GL::TextureFormat format = GL::TextureFormat::RGB9E5; + ImageView2D view{GL::PixelFormat::RGB, GL::PixelType::UnsignedInt5999Rev, Vector2i{2}, Data2D}; + #endif + + GL::CubeMapTexture texture; + texture.setImage(GL::CubeMapCoordinate::PositiveX, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeX, 0, format, view) + .setImage(GL::CubeMapCoordinate::PositiveY, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeY, 0, format, view) + .setImage(GL::CubeMapCoordinate::PositiveZ, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeZ, 0, format, view); + + std::ostringstream out; + Error redirectError{&out}; + /* The read type doesn't have to match, it doesn't get that far */ + textureSubImage(texture, GL::CubeMapCoordinate::PositiveX, 0, {{}, Vector2i{2}}, {GL::PixelFormat::RGBA, GL::PixelType::UnsignedByte}); + MAGNUM_VERIFY_NO_GL_ERROR(); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::Unsupported\n"); + #else + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::IncompleteAttachment\n"); + #endif +} + #ifndef MAGNUM_TARGET_GLES2 void TextureImageGLTest::subImageCubeBuffer() { ImageView2D view{GL::PixelFormat::RGBA, GL::PixelType::UnsignedByte, Vector2i{2}, Data2D}; @@ -205,6 +307,44 @@ void TextureImageGLTest::subImageCubeBuffer() { Containers::arrayView(Data2D), TestSuite::Compare::Container); } + +void TextureImageGLTest::subImageCubeBufferNotReadable() { + CORRADE_SKIP_IF_NO_ASSERT(); + + #ifndef MAGNUM_TARGET_GLES + if(!GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::EXT::texture_shared_exponent::string() << "not supported, can't test"); + if(GL::Context::current().isExtensionSupported()) + CORRADE_SKIP(GL::Extensions::ARB::get_texture_sub_image::string() << "supported, can't test"); + #endif + + #ifdef MAGNUM_TARGET_GLES2 + GL::TextureFormat format = GL::TextureFormat::Luminance; + ImageView2D view{GL::PixelFormat::Luminance, GL::PixelType::UnsignedByte, Vector2i{2}, Data2D}; + #else + GL::TextureFormat format = GL::TextureFormat::RGB9E5; + ImageView2D view{GL::PixelFormat::RGB, GL::PixelType::UnsignedInt5999Rev, Vector2i{2}, Data2D}; + #endif + + GL::CubeMapTexture texture; + texture.setImage(GL::CubeMapCoordinate::PositiveX, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeX, 0, format, view) + .setImage(GL::CubeMapCoordinate::PositiveY, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeY, 0, format, view) + .setImage(GL::CubeMapCoordinate::PositiveZ, 0, format, view) + .setImage(GL::CubeMapCoordinate::NegativeZ, 0, format, view); + + std::ostringstream out; + Error redirectError{&out}; + /* The read type doesn't have to match, it doesn't get that far */ + textureSubImage(texture, GL::CubeMapCoordinate::PositiveX, 0, {{}, Vector2i{2}}, {GL::PixelFormat::RGBA, GL::PixelType::UnsignedByte}, GL::BufferUsage::StaticRead); + MAGNUM_VERIFY_NO_GL_ERROR(); + #ifndef MAGNUM_TARGET_GLES + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::Unsupported\n"); + #else + CORRADE_COMPARE(out.str(), "DebugTools::textureSubImage(): texture format not framebuffer-readable: GL::Framebuffer::Status::IncompleteAttachment\n"); + #endif +} #endif #ifndef MAGNUM_TARGET_GLES2 diff --git a/src/Magnum/DebugTools/TextureImage.cpp b/src/Magnum/DebugTools/TextureImage.cpp index 9ab5f2ea7..dfac7900f 100644 --- a/src/Magnum/DebugTools/TextureImage.cpp +++ b/src/Magnum/DebugTools/TextureImage.cpp @@ -180,8 +180,12 @@ void textureSubImage(GL::Texture2D& texture, const Int level, const Range2Di& ra #endif GL::Framebuffer fb{range}; - fb.attachTexture(GL::Framebuffer::ColorAttachment{0}, texture, level) - .read(range, image); + fb.attachTexture(GL::Framebuffer::ColorAttachment{0}, texture, level); + + CORRADE_ASSERT(fb.checkStatus(GL::FramebufferTarget::Read) == GL::Framebuffer::Status::Complete, + "DebugTools::textureSubImage(): texture format not framebuffer-readable:" << fb.checkStatus(GL::FramebufferTarget::Read), ); + + fb.read(range, image); } Image2D textureSubImage(GL::Texture2D& texture, const Int level, const Range2Di& range, Image2D&& image) { @@ -199,8 +203,12 @@ void textureSubImage(GL::Texture2D& texture, const Int level, const Range2Di& ra #endif GL::Framebuffer fb{range}; - fb.attachTexture(GL::Framebuffer::ColorAttachment{0}, texture, level) - .read(range, image, usage); + fb.attachTexture(GL::Framebuffer::ColorAttachment{0}, texture, level); + + CORRADE_ASSERT(fb.checkStatus(GL::FramebufferTarget::Read) == GL::Framebuffer::Status::Complete, + "DebugTools::textureSubImage(): texture format not framebuffer-readable:" << fb.checkStatus(GL::FramebufferTarget::Read), ); + + fb.read(range, image, usage); } GL::BufferImage2D textureSubImage(GL::Texture2D& texture, const Int level, const Range2Di& range, GL::BufferImage2D&& image, const GL::BufferUsage usage) { @@ -211,8 +219,12 @@ GL::BufferImage2D textureSubImage(GL::Texture2D& texture, const Int level, const void textureSubImage(GL::CubeMapTexture& texture, const GL::CubeMapCoordinate coordinate, const Int level, const Range2Di& range, Image2D& image) { GL::Framebuffer fb{range}; - fb.attachCubeMapTexture(GL::Framebuffer::ColorAttachment{0}, texture, coordinate, level) - .read(range, image); + fb.attachCubeMapTexture(GL::Framebuffer::ColorAttachment{0}, texture, coordinate, level); + + CORRADE_ASSERT(fb.checkStatus(GL::FramebufferTarget::Read) == GL::Framebuffer::Status::Complete, + "DebugTools::textureSubImage(): texture format not framebuffer-readable:" << fb.checkStatus(GL::FramebufferTarget::Read), ); + + fb.read(range, image); } Image2D textureSubImage(GL::CubeMapTexture& texture, const GL::CubeMapCoordinate coordinate, const Int level, const Range2Di& range, Image2D&& image) { @@ -223,8 +235,12 @@ Image2D textureSubImage(GL::CubeMapTexture& texture, const GL::CubeMapCoordinate #ifndef MAGNUM_TARGET_GLES2 void textureSubImage(GL::CubeMapTexture& texture, const GL::CubeMapCoordinate coordinate, const Int level, const Range2Di& range, GL::BufferImage2D& image, const GL::BufferUsage usage) { GL::Framebuffer fb{range}; - fb.attachCubeMapTexture(GL::Framebuffer::ColorAttachment{0}, texture, coordinate, level) - .read(range, image, usage); + fb.attachCubeMapTexture(GL::Framebuffer::ColorAttachment{0}, texture, coordinate, level); + + CORRADE_ASSERT(fb.checkStatus(GL::FramebufferTarget::Read) == GL::Framebuffer::Status::Complete, + "DebugTools::textureSubImage(): texture format not framebuffer-readable:" << fb.checkStatus(GL::FramebufferTarget::Read), ); + + fb.read(range, image, usage); } GL::BufferImage2D textureSubImage(GL::CubeMapTexture& texture, const GL::CubeMapCoordinate coordinate, const Int level, const Range2Di& range, GL::BufferImage2D&& image, const GL::BufferUsage usage) { diff --git a/src/Magnum/DebugTools/TextureImage.h b/src/Magnum/DebugTools/TextureImage.h index 0dc7d2dcc..fa64d496d 100644 --- a/src/Magnum/DebugTools/TextureImage.h +++ b/src/Magnum/DebugTools/TextureImage.h @@ -46,13 +46,13 @@ Emulates @ref GL::Texture2D::subImage() call on platforms that don't support it @ref GL::Framebuffer::read(). On desktop OpenGL, if @gl_extension{ARB,get_texture_sub_image} is available, it's just an alias to @ref GL::Texture2D::subImage(). -Note that only @ref GL::PixelFormat and @ref GL::PixelType values that are -marked as framebuffer readable are supported; their generic -@ref Magnum::PixelFormat "PixelFormat" counterparts are supported as well. In -addition, on OpenGL ES 3.0, images with @ref GL::PixelType::Float are supported ---- they are reinterpreted as @ref GL::PixelType::UnsignedInt using an -additional shader and the @glsl floatBitsToUint() @ce GLSL function and then -reinterpreted back to @ref GL::PixelType::Float when read to client memory. +The function expects the @ref GL::PixelFormat and @ref GL::PixelType +combination or the generic @relativeref{Magnum,PixelFormat} to be framebuffer +readable. In addition, on OpenGL ES 3.0, images with @ref GL::PixelType::Float +are supported --- they are reinterpreted as @ref GL::PixelType::UnsignedInt +using an additional shader and the @glsl floatBitsToUint() @ce GLSL function +and then reinterpreted back to @ref GL::PixelType::Float when read to client +memory. @note This function is available only if Magnum is compiled with @ref MAGNUM_TARGET_GL "TARGET_GL" enabled (done by default). See @@ -80,9 +80,9 @@ Emulates @ref GL::CubeMapTexture::subImage() call on platforms that don't support it (such as OpenGL ES) by creating a framebuffer object and using @ref GL::Framebuffer::read(). -Note that only @ref GL::PixelFormat and @ref GL::PixelType values that are -marked as framebuffer readable are supported; their generic -@ref Magnum::PixelFormat "PixelFormat" counterparts are supported as well. +The function expects the @ref GL::PixelFormat and @ref GL::PixelType +combination or the generic @relativeref{Magnum,PixelFormat} to be framebuffer +readable. @note This function is available only if Magnum is compiled with @ref MAGNUM_TARGET_GL "TARGET_GL" enabled (done by default). See @@ -113,9 +113,9 @@ Emulates @ref GL::Texture2D::subImage() call on platforms that don't support it @gl_extension{ARB,get_texture_sub_image} is available, it's just an alias to @ref GL::Texture2D::subImage(). -Note that only @ref GL::PixelFormat and @ref GL::PixelType values that are -marked as framebuffer readable are supported; their generic -@ref Magnum::PixelFormat "PixelFormat" counterparts are supported as well. +The function expects the @ref GL::PixelFormat and @ref GL::PixelType +combination or the generic @relativeref{Magnum,PixelFormat} to be framebuffer +readable. @requires_gles30 Pixel buffer objects are not available in OpenGL ES 2.0. @requires_webgl20 Pixel buffer objects are not available in WebGL 1.0. @@ -145,9 +145,9 @@ Emulates @ref GL::CubeMapTexture::subImage() call on platforms that don't support it (such as OpenGL ES) by creating a framebuffer object and using @ref GL::Framebuffer::read(). -Note that only @ref GL::PixelFormat and @ref GL::PixelType values that are -marked as framebuffer readable are supported; their generic -@ref Magnum::PixelFormat "PixelFormat" counterparts are supported as well. +The function expects the @ref GL::PixelFormat and @ref GL::PixelType +combination or the generic @relativeref{Magnum,PixelFormat} to be framebuffer +readable. @requires_gles30 Pixel buffer objects are not available in OpenGL ES 2.0. @requires_webgl20 Pixel buffer objects are not available in WebGL 1.0.