GL: new "nv-cubemap-broken-dsa-compressed-subimage-upload" workaround.

Yet another broken behavior with compressed textures on NVidia unearthed by the recent changes with compressed block properties being set almost always.
10 months ago · 05069848a4
4 changed files with 129 additions and 40 deletions
--- a/doc/changelog.dox
+++ b/doc/changelog.dox
@ -254,6 +254,10 @@ See also:
 -   A new @cpp "nv-framebuffer-invalidation-wants-draw-binding" @ce workaround
    for @ref GL::Framebuffer::invalidate() affecting unrelated framebuffers on
    NVidia. See @ref opengl-workarounds for more information.
+-   A new @cpp "nv-cubemap-broken-dsa-compressed-subimage-upload" @ce
+    workaround for yet another broken case with compressed formats used in
+    @ref GL::CubeMapTexture on NVidia. See @ref opengl-workarounds for more
+    information.

@subsubsection changelog-latest-new-math Math library

--- a/src/Magnum/GL/Implementation/TextureState.cpp
+++ b/src/Magnum/GL/Implementation/TextureState.cpp
@ -196,11 +196,10 @@ TextureState::TextureState(Context& context,
        #endif
    }

-    /* DSA/non-DSA implementation for cubemaps, because Intel (and AMD) Windows
-       drivers have to be broken in a special way */
+    /* DSA/non-DSA implementation for cubemaps, because ... well, basically all
+       non-Mesa drivers have to be broken in a special way */
    #ifndef MAGNUM_TARGET_GLES
    if(context.isExtensionSupported<Extensions::ARB::direct_state_access>()) {
-
        #ifdef CORRADE_TARGET_WINDOWS
        if((context.detectedDriver() & Context::DetectedDriver::IntelWindows) && !context.isDriverWorkaroundDisabled("intel-windows-broken-dsa-for-cubemaps"_s)) {
            getCubeLevelParameterivImplementation = &CubeMapTexture::getLevelParameterImplementationDefault;
@ -213,7 +212,12 @@ TextureState::TextureState(Context& context,
            cubeCompressedSubImageImplementation = &CubeMapTexture::compressedSubImageImplementationDefault;
        } else
        #endif
-        {
+        if((context.detectedDriver() & Context::DetectedDriver::NVidia) && !context.isDriverWorkaroundDisabled("nv-cubemap-broken-dsa-compressed-subimage-upload"_s)) {
+            getCubeLevelParameterivImplementation = &CubeMapTexture::getLevelParameterImplementationDSA;
+            cubeSubImageImplementation = &CubeMapTexture::subImageImplementationDSA;
+            /* This one is broken, the others are not */
+            cubeCompressedSubImageImplementation = &CubeMapTexture::compressedSubImageImplementationDefault;
+        } else {
            /* Extension name added above */

            getCubeLevelParameterivImplementation = &CubeMapTexture::getLevelParameterImplementationDSA;
--- a/src/Magnum/GL/Implementation/driverSpecific.cpp
+++ b/src/Magnum/GL/Implementation/driverSpecific.cpp
@ -196,6 +196,18 @@ constexpr Containers::StringView KnownWorkarounds[]{
   image when querying all six slices using the ARB_DSA API */
 "nv-cubemap-broken-full-compressed-image-query"_s,

+/* NVidia drivers (575.64, but likely also any before) behave wrong when
+   uploading compressed 2D cubemap subimages when
+    - a DSA API is used,
+    - the cubemap doesn't have immutable storage,
+    - the query is done to client memory and not a buffer,
+    - *and* non-zero compressed block size is set in pixel storage
+   With just any of the four missing, it works well. As it's too restrictive to
+   require users to either always use setStorage() for cubemaps or to always
+   upload full slices, the workaround is simply to use the classic non-DSA
+   codepath. See CubeMapGLTest::compressedSubImage() for a repro case. */
+"nv-cubemap-broken-dsa-compressed-subimage-upload"_s,
+
 /* NVidia drivers return 0 when asked for GL_CONTEXT_PROFILE_MASK, so it needs
   to be worked around by asking for GL_ARB_compatibility */
 "nv-zero-context-profile-mask"_s,
--- a/src/Magnum/GL/Test/CubeMapTextureGLTest.cpp
+++ b/src/Magnum/GL/Test/CubeMapTextureGLTest.cpp
@ -228,6 +228,40 @@ const struct {
    #endif
 };

+const struct {
+    const char* name;
+    Containers::ArrayView<const UnsignedByte> data;
+    CompressedPixelStorage storage;
+    Containers::ArrayView<const UnsignedByte> dataSparse;
+    std::size_t offset;
+    bool immutable;
+} CompressedSubImageData[]{
+    {"default pixel storage",
+        Containers::arrayView(CompressedData).exceptPrefix(16),
+        {},
+        Containers::arrayView(CompressedData).exceptPrefix(16), 0, false},
+    #ifndef MAGNUM_TARGET_GLES
+    {"skip Y",
+        Containers::arrayView(CompressedData).exceptPrefix(16),
+        CompressedPixelStorage{}
+            .setSkip({0, 4, 0}),
+        Containers::arrayView(CompressedData), 16, false},
+    #endif
+    #ifndef MAGNUM_TARGET_GLES2
+    {"immutable storage, default pixel storage",
+        Containers::arrayView(CompressedData).exceptPrefix(16),
+        {},
+        Containers::arrayView(CompressedData).exceptPrefix(16), 0, true},
+    #ifndef MAGNUM_TARGET_GLES
+    {"immutable storage, skip Y",
+        Containers::arrayView(CompressedData).exceptPrefix(16),
+        CompressedPixelStorage{}
+            .setSkip({0, 4, 0}),
+        Containers::arrayView(CompressedData), 16, true},
+    #endif
+    #endif
+};
+
 constexpr UnsignedByte FullData[]{
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,

@ -403,7 +437,7 @@ CubeMapTextureGLTest::CubeMapTextureGLTest() {
        #ifndef MAGNUM_TARGET_GLES2
        &CubeMapTextureGLTest::compressedSubImageBuffer,
        #endif
-        }, Containers::arraySize(CompressedPixelStorageData));
+        }, Containers::arraySize(CompressedSubImageData));

    addInstancedTests({
        &CubeMapTextureGLTest::image3D,
@ -1411,7 +1445,7 @@ constexpr UnsignedByte CompressedSubDataComplete[]{
 #endif

 void CubeMapTextureGLTest::compressedSubImage() {
-    auto&& data = CompressedPixelStorageData[testCaseInstanceId()];
+    auto&& data = CompressedSubImageData[testCaseInstanceId()];
    setTestCaseDescription(data.name);

    #ifndef MAGNUM_TARGET_GLES
@ -1431,6 +1465,24 @@ void CubeMapTextureGLTest::compressedSubImage() {
    #endif

    CubeMapTexture texture;
+    #ifndef MAGNUM_TARGET_GLES2
+    if(data.immutable) {
+        texture.setStorage(1, TextureFormat::CompressedRGBAS3tcDxt3, Vector2i{12});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveX, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeX, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveY, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeY, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveZ, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeZ, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+    } else
+    #endif
+    {
        texture.setCompressedImage(CubeMapCoordinate::PositiveX, 0,
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
        texture.setCompressedImage(CubeMapCoordinate::NegativeX, 0,
@ -1443,6 +1495,7 @@ void CubeMapTextureGLTest::compressedSubImage() {
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
        texture.setCompressedImage(CubeMapCoordinate::NegativeZ, 0,
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+    }
    texture.setCompressedSubImage(CubeMapCoordinate::PositiveX, 0, Vector2i{4}, CompressedImageView2D{
        data.storage,
        CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{4},
@ -1456,21 +1509,21 @@ void CubeMapTextureGLTest::compressedSubImage() {
    MAGNUM_VERIFY_NO_GL_ERROR();

    CORRADE_COMPARE(image.size(), Vector2i{12});
-
-    {
-        CORRADE_EXPECT_FAIL_IF(data.storage != CompressedPixelStorage{} && Context::current().isExtensionSupported<Extensions::ARB::direct_state_access>() && (Context::current().detectedDriver() & Context::DetectedDriver::NVidia),
-            "Non-default compressed pixel storage for cube map textures behaves weirdly on NVidia for client-memory images when using ARB_direct_state_access");
-
+    /* This fails if the "nv-cubemap-broken-dsa-compressed-subimage-upload" is
+       disabled, but only if pixel storage is non-default and setStorage()
+       isn't used. Thus, the "skip Y" case will fail, and "default pixel
+       storage" case will fail if run after any other test that sets pixel
+       storage compressed block properties. Running it as a first test
+       works. */
    CORRADE_COMPARE_AS(Containers::arrayCast<UnsignedByte>(image.data()),
        Containers::arrayView(CompressedSubDataComplete),
        TestSuite::Compare::Container);
-    }
    #endif
 }

 #ifndef MAGNUM_TARGET_GLES2
 void CubeMapTextureGLTest::compressedSubImageBuffer() {
-    auto&& data = CompressedPixelStorageData[testCaseInstanceId()];
+    auto&& data = CompressedSubImageData[testCaseInstanceId()];
    setTestCaseDescription(data.name);

    #ifndef MAGNUM_TARGET_GLES
@ -1490,6 +1543,21 @@ void CubeMapTextureGLTest::compressedSubImageBuffer() {
    #endif

    CubeMapTexture texture;
+    if(data.immutable) {
+        texture.setStorage(1, TextureFormat::CompressedRGBAS3tcDxt3, Vector2i{12});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveX, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeX, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveY, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeY, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::PositiveZ, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+        texture.setCompressedSubImage(CubeMapCoordinate::NegativeZ, 0, {},
+            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+    } else {
        texture.setCompressedImage(CubeMapCoordinate::PositiveX, 0,
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
        texture.setCompressedImage(CubeMapCoordinate::NegativeX, 0,
@ -1502,6 +1570,7 @@ void CubeMapTextureGLTest::compressedSubImageBuffer() {
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
        texture.setCompressedImage(CubeMapCoordinate::NegativeZ, 0,
            CompressedImageView2D{CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{12}, CompressedZero});
+    }
    texture.setCompressedSubImage(CubeMapCoordinate::PositiveX, 0, Vector2i{4}, CompressedBufferImage2D{
        data.storage,
        CompressedPixelFormat::RGBAS3tcDxt3, Vector2i{4},