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

@@ -120,6 +120,8 @@ r<Player<T, K>>>)
 -   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
 

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
 }
 

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 */

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<char>{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<char>{Containers::ValueInit, sizeof(Data3D)}};
     texture.image(0, image);