From b4c49081c97c7bf1926cc635a1a800fc539a29dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 22 Nov 2018 16:43:31 +0100 Subject: [PATCH] GL: make PixelStorage image height and Z skip work properly on ES3. Looks like it was never tested, as it asserted on usual usage. Also the test was doing something completely crazy, that's fixed now as well. --- doc/changelog.dox | 2 + .../GL/Implementation/RendererState.cpp | 46 +++++++++---------- src/Magnum/GL/Implementation/RendererState.h | 4 +- src/Magnum/GL/Test/PixelStorageGLTest.cpp | 41 ++++++++--------- 4 files changed, 45 insertions(+), 48 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 21738acb1..8a076a063 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -120,6 +120,8 @@ r>>) - Fixed @ref Text::DistanceFieldGlyphCache internal formats on ES2 devices that support @gl_extension{EXT,texture_storage} (see [mosra/magnum#289](https://github.com/mosra/magnum/pull/289)) +- @ref PixelStorage::imageHeight() and Z value of @ref PixelStorage::skip() + was not properly handled on ES3 / WebGL 2 builds. @subsection changelog-latest-docs Documentation diff --git a/src/Magnum/GL/Implementation/RendererState.cpp b/src/Magnum/GL/Implementation/RendererState.cpp index b036f16d3..b3a3548ff 100644 --- a/src/Magnum/GL/Implementation/RendererState.cpp +++ b/src/Magnum/GL/Implementation/RendererState.cpp @@ -168,14 +168,22 @@ void RendererState::applyPixelStorageInternal(const Magnum::PixelStorage& storag "GL: non-default PixelStorage::rowLength() is not supported in WebGL 1.0", ); #endif - #ifndef MAGNUM_TARGET_GLES - /* Image height (on ES for unpack only, taken care of below) */ - if(state.imageHeight == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.imageHeight != storage.imageHeight()) + /* Image height (not on ES2, on ES3 for unpack only) */ + #ifndef MAGNUM_TARGET_GLES2 + if(state.imageHeight == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.imageHeight != storage.imageHeight()) { + #ifndef MAGNUM_TARGET_GLES glPixelStorei(isUnpack ? GL_UNPACK_IMAGE_HEIGHT : GL_PACK_IMAGE_HEIGHT, state.imageHeight = storage.imageHeight()); + #else + if(isUnpack) glPixelStorei(GL_UNPACK_IMAGE_HEIGHT, + state.imageHeight = storage.imageHeight()); + else CORRADE_ASSERT(!storage.imageHeight(), + "GL: non-default PixelStorage::imageHeight() for pack is not supported in OpenGL ES", ); + #endif + } #else CORRADE_ASSERT(!storage.imageHeight(), - "GL: non-default PixelStorage::imageHeight() is not supported in OpenGL ES", ); + "GL: non-default PixelStorage::imageHeight() is not supported in OpenGL ES 2", ); #endif /* On ES2 done by modifying data pointer */ @@ -190,28 +198,18 @@ void RendererState::applyPixelStorageInternal(const Magnum::PixelStorage& storag glPixelStorei(isUnpack ? GL_UNPACK_SKIP_ROWS : GL_PACK_SKIP_ROWS, state.skip.y() = storage.skip().y()); - #ifndef MAGNUM_TARGET_GLES - /* Skip images (on ES for unpack only, taken care of below) */ - if(state.skip.z() == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.skip.z() != storage.skip().z()) + /* Skip images (on ES3 for unpack only) */ + if(state.skip.z() == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.skip.z() != storage.skip().z()) { + #ifndef MAGNUM_TARGET_GLES glPixelStorei(isUnpack ? GL_UNPACK_SKIP_IMAGES : GL_PACK_SKIP_IMAGES, state.skip.z() = storage.skip().z()); - #endif - #endif -} - -void RendererState::applyPixelStorageUnpack(const Magnum::PixelStorage& storage) { - applyPixelStorageInternal(storage, true); - - #if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_GLES2) - PixelStorage& state = unpackPixelStorage; - - /* Image height (on ES for unpack only) */ - if(state.imageHeight == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.imageHeight != storage.imageHeight()) - glPixelStorei(GL_UNPACK_IMAGE_HEIGHT, state.imageHeight = storage.imageHeight()); - - /* Skip images (on ES for unpack only) */ - if(state.skip.z() == GL::Implementation::RendererState::PixelStorage::DisengagedValue || state.skip.z() != storage.skip().z()) - glPixelStorei(GL_UNPACK_SKIP_IMAGES, state.skip.z() = storage.skip().z()); + #else + if(isUnpack) glPixelStorei(GL_UNPACK_SKIP_IMAGES, + state.skip.z() = storage.skip().z()); + else CORRADE_ASSERT(!storage.skip().z(), + "GL: non-default PixelStorage::skip().z() for pack is not supported in OpenGL ES", ); + #endif + } #endif } diff --git a/src/Magnum/GL/Implementation/RendererState.h b/src/Magnum/GL/Implementation/RendererState.h index ff294e205..0f66e79bc 100644 --- a/src/Magnum/GL/Implementation/RendererState.h +++ b/src/Magnum/GL/Implementation/RendererState.h @@ -83,7 +83,9 @@ struct RendererState { void applyPixelStoragePack(const Magnum::PixelStorage& storage) { applyPixelStorageInternal(storage, false); } - void applyPixelStorageUnpack(const Magnum::PixelStorage& storage); + void applyPixelStorageUnpack(const Magnum::PixelStorage& storage) { + applyPixelStorageInternal(storage, true); + } /* Bool parameter is ugly, but this is implementation detail of internal API so who cares */ diff --git a/src/Magnum/GL/Test/PixelStorageGLTest.cpp b/src/Magnum/GL/Test/PixelStorageGLTest.cpp index e13b59dde..4ea53eb6d 100644 --- a/src/Magnum/GL/Test/PixelStorageGLTest.cpp +++ b/src/Magnum/GL/Test/PixelStorageGLTest.cpp @@ -83,16 +83,14 @@ PixelStorageGLTest::PixelStorageGLTest() { namespace { constexpr const char Data2D[] = { - /* Skip */ + /* Row length ------------------------------------------------------ */ /* Alignment */ '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', - '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', - - /* Data */ /* Row length */ /* Alignment */ - '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x00', '\x00', '\x00', '\x00', - '\x06', '\x07', '\x08', '\x09', '\x0a', '\x0b', '\x00', '\x00', '\x00', '\x00', - '\x0c', '\x0d', '\x0e', '\x0f', '\x10', '\x11', '\x00', '\x00', '\x00', '\x00' + /* ------------ Skip */ /* Data ------------------------------------ */ /* Alignment */ + '\x00', '\x00', '\x00', '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x00', + '\x00', '\x00', '\x00', '\x06', '\x07', '\x08', '\x09', '\x0a', '\x0b', '\x00', + '\x00', '\x00', '\x00', '\x0c', '\x0d', '\x0e', '\x0f', '\x10', '\x11', '\x00', }; constexpr const char ActualData[] = { @@ -108,12 +106,11 @@ void PixelStorageGLTest::unpack2D() { CORRADE_SKIP(Extensions::EXT::unpack_subimage::string() + std::string(" is not supported.")); #endif - PixelStorage storage; - storage.setAlignment(2) + ImageView2D image{PixelStorage{} + .setAlignment(2) .setRowLength(3) - .setSkip({2, 3, 0}); - - ImageView2D image{storage, PixelFormat::RGB, PixelType::UnsignedByte, {2, 3}, Data2D}; + .setSkip({1, 3, 0}), + PixelFormat::RGB, PixelType::UnsignedByte, {2, 3}, Data2D}; Texture2D texture; texture.setStorage(1, TextureFormat::RGB8, {2, 3}) @@ -158,7 +155,7 @@ void PixelStorageGLTest::pack2D() { Image2D image{PixelStorage{} .setAlignment(2) .setRowLength(3) - .setSkip({2, 3, 0}), + .setSkip({1, 3, 0}), PixelFormat::RGB, PixelType::UnsignedByte, {}, Containers::Array{Containers::ValueInit, sizeof(Data2D)}}; #ifndef MAGNUM_TARGET_GLES @@ -178,22 +175,20 @@ void PixelStorageGLTest::pack2D() { #ifndef MAGNUM_TARGET_GLES2 namespace { constexpr const char Data3D[] = { - /* Skip */ + /* Row length ------------------------------------------------------ */ /* Alignment */ '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', + /* Row length ------------------------------------------------------ */ /* Alignment */ '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', - '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', - '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', - - /* Data */ /* Row length */ /* Alignment */ - '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x00', '\x00', '\x00', '\x00', - '\x06', '\x07', '\x08', '\x09', '\x0a', '\x0b', '\x00', '\x00', '\x00', '\x00', - '\x0c', '\x0d', '\x0e', '\x0f', '\x10', '\x11', '\x00', '\x00', '\x00', '\x00', + /* ------------ Skip */ /* Data ------------------------------------ */ /* Alignment */ + '\x00', '\x00', '\x00', '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x00', + '\x00', '\x00', '\x00', '\x06', '\x07', '\x08', '\x09', '\x0a', '\x0b', '\x00', + '\x00', '\x00', '\x00', '\x0c', '\x0d', '\x0e', '\x0f', '\x10', '\x11', '\x00', /* Filling to image height not needed */ }; @@ -204,7 +199,7 @@ void PixelStorageGLTest::unpack3D() { storage.setAlignment(2) .setRowLength(3) .setImageHeight(5) - .setSkip({2, 3, 1}); + .setSkip({1, 2, 1}); ImageView3D image{storage, PixelFormat::RGB, PixelType::UnsignedByte, {2, 3, 1}, Data3D}; @@ -253,7 +248,7 @@ void PixelStorageGLTest::pack3D() { .setAlignment(2) .setRowLength(3) .setImageHeight(5) - .setSkip({2, 3, 1}), + .setSkip({1, 2, 1}), PixelFormat::RGB, PixelType::UnsignedByte, {}, Containers::Array{Containers::ValueInit, sizeof(Data3D)}}; texture.image(0, image);