From 7a37d3acf5329acfe32bb3ecfe49ea23ff2bfd06 Mon Sep 17 00:00:00 2001 From: Kenneth Russell Date: Sun, 5 Jan 2020 13:52:03 -0800 Subject: [PATCH] Work around Intel driver bug with CopyTex{Sub}Image2D/DeleteTextures. Dependencies seem to be incorrectly tracked in some Intel OpenGL drivers (on macOS specifically), causing crashes in glDeleteTextures if a GL command buffer is being constructed where those textures are destinations of CopyTexImage2D/CopyTexSubImage2D. Work around this bug by flushing before texture deletion if CopyTex{Sub}Image have been called recently. The tracking is only done on a per-context rather than a per-device basis, but seems sufficient to work around the problem as identified. Tested both with new ANGLE test on affected hardware, and in WebKit's ANGLE backend for WebGL. Works around the crash reported in https://bugs.webkit.org/show_bug.cgi?id=205707 . Bug: angleproject:4267 Change-Id: I2266a5590759f6a3f19080def08710ef4b66d463 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1987932 Commit-Queue: Kenneth Russell Reviewed-by: Geoff Lang Reviewed-by: Shahbaz Youssefi --- include/platform/FeaturesGL.h | 10 ++++ src/libANGLE/renderer/gl/ContextGL.cpp | 11 +++++ src/libANGLE/renderer/gl/ContextGL.h | 3 ++ src/libANGLE/renderer/gl/RendererGL.cpp | 15 ++++++ src/libANGLE/renderer/gl/RendererGL.h | 6 +++ src/libANGLE/renderer/gl/TextureGL.cpp | 26 +++++++++- src/libANGLE/renderer/gl/renderergl_utils.cpp | 3 ++ src/tests/gl_tests/CopyTexImageTest.cpp | 47 +++++++++++++++++++ 8 files changed, 119 insertions(+), 2 deletions(-) diff --git a/include/platform/FeaturesGL.h b/include/platform/FeaturesGL.h index 440c6ebdc..2fad7b60d 100644 --- a/include/platform/FeaturesGL.h +++ b/include/platform/FeaturesGL.h @@ -394,6 +394,16 @@ struct FeaturesGL : FeatureSetBase "Quite some OpenGL ES drivers don't implement readPixels for RGBA/UNSIGNED_SHORT from " "EXT_texture_norm16 correctly", &members, "http://anglebug.com/4214"}; + + // Bugs exist in some Intel drivers where dependencies are incorrectly + // tracked for textures which are copy destinations (via CopyTexImage2D, for + // example). Flush before DeleteTexture if these entry points have been + // called recently. + Feature flushBeforeDeleteTextureIfCopiedTo = { + "flush_before_delete_texture_if_copied_to", FeatureCategory::OpenGLWorkarounds, + "Some drivers track CopyTex{Sub}Image texture dependencies incorrectly. Flush" + " before glDeleteTextures in this case", + &members, "http://anglebug.com/4267"}; }; inline FeaturesGL::FeaturesGL() = default; diff --git a/src/libANGLE/renderer/gl/ContextGL.cpp b/src/libANGLE/renderer/gl/ContextGL.cpp index 7c5ec13c7..1c365da0e 100644 --- a/src/libANGLE/renderer/gl/ContextGL.cpp +++ b/src/libANGLE/renderer/gl/ContextGL.cpp @@ -901,4 +901,15 @@ void ContextGL::validateState() const stateManager->validateState(); } +void ContextGL::setNeedsFlushBeforeDeleteTextures() +{ + mRenderer->setNeedsFlushBeforeDeleteTextures(); +} + +void ContextGL::flushIfNecessaryBeforeDeleteTextures() +{ + mRenderer->flushIfNecessaryBeforeDeleteTextures(); +} + + } // namespace rx diff --git a/src/libANGLE/renderer/gl/ContextGL.h b/src/libANGLE/renderer/gl/ContextGL.h index 977ea2ecb..2645be052 100644 --- a/src/libANGLE/renderer/gl/ContextGL.h +++ b/src/libANGLE/renderer/gl/ContextGL.h @@ -265,6 +265,9 @@ class ContextGL : public ContextImpl void validateState() const; + void setNeedsFlushBeforeDeleteTextures(); + void flushIfNecessaryBeforeDeleteTextures(); + private: angle::Result setDrawArraysState(const gl::Context *context, GLint first, diff --git a/src/libANGLE/renderer/gl/RendererGL.cpp b/src/libANGLE/renderer/gl/RendererGL.cpp index 3f9fd312a..f526cb415 100644 --- a/src/libANGLE/renderer/gl/RendererGL.cpp +++ b/src/libANGLE/renderer/gl/RendererGL.cpp @@ -284,6 +284,7 @@ RendererGL::~RendererGL() angle::Result RendererGL::flush() { mFunctions->flush(); + mNeedsFlushBeforeDeleteTextures = false; return angle::Result::Continue; } @@ -295,6 +296,7 @@ angle::Result RendererGL::finish() } mFunctions->finish(); + mNeedsFlushBeforeDeleteTextures = false; if (mFeatures.finishDoesNotCauseQueriesToBeAvailable.enabled && mUseDebugOutput) { @@ -682,6 +684,19 @@ void RendererGL::setMaxShaderCompilerThreads(GLuint count) } } +void RendererGL::setNeedsFlushBeforeDeleteTextures() +{ + mNeedsFlushBeforeDeleteTextures = true; +} + +void RendererGL::flushIfNecessaryBeforeDeleteTextures() +{ + if (mNeedsFlushBeforeDeleteTextures) + { + (void) flush(); + } +} + ScopedWorkerContextGL::ScopedWorkerContextGL(RendererGL *renderer, std::string *infoLog) : mRenderer(renderer) { diff --git a/src/libANGLE/renderer/gl/RendererGL.h b/src/libANGLE/renderer/gl/RendererGL.h index ced11f4e4..98d5942c5 100644 --- a/src/libANGLE/renderer/gl/RendererGL.h +++ b/src/libANGLE/renderer/gl/RendererGL.h @@ -194,6 +194,9 @@ class RendererGL : angle::NonCopyable static unsigned int getMaxWorkerContexts(); + void setNeedsFlushBeforeDeleteTextures(); + void flushIfNecessaryBeforeDeleteTextures(); + protected: virtual WorkerContext *createWorkerContext(std::string *infoLog) = 0; @@ -231,6 +234,9 @@ class RendererGL : angle::NonCopyable bool mNativeParallelCompileEnabled; angle::FeaturesGL mFeatures; + + // Workaround for anglebug.com/4267 + bool mNeedsFlushBeforeDeleteTextures; }; } // namespace rx diff --git a/src/libANGLE/renderer/gl/TextureGL.cpp b/src/libANGLE/renderer/gl/TextureGL.cpp index d53e0e39c..c56289619 100644 --- a/src/libANGLE/renderer/gl/TextureGL.cpp +++ b/src/libANGLE/renderer/gl/TextureGL.cpp @@ -151,6 +151,7 @@ TextureGL::~TextureGL() void TextureGL::onDestroy(const gl::Context *context) { + GetImplAs(context)->flushIfNecessaryBeforeDeleteTextures(); StateManagerGL *stateManager = GetStateManagerGL(context); stateManager->deleteTexture(mTextureID); mTextureID = 0; @@ -734,6 +735,11 @@ angle::Result TextureGL::copyImage(const gl::Context *context, setLevelInfo(context, target, level, 1, levelInfo); } + if (features.flushBeforeDeleteTextureIfCopiedTo.enabled) + { + contextGL->setNeedsFlushBeforeDeleteTextures(); + } + return angle::Result::Continue; } @@ -745,6 +751,7 @@ angle::Result TextureGL::copySubImage(const gl::Context *context, { const FunctionsGL *functions = GetFunctionsGL(context); StateManagerGL *stateManager = GetStateManagerGL(context); + const angle::FeaturesGL &features = GetFeaturesGL(context); gl::TextureTarget target = index.getTarget(); size_t level = static_cast(index.getLevelIndex()); @@ -792,6 +799,12 @@ angle::Result TextureGL::copySubImage(const gl::Context *context, } } + if (features.flushBeforeDeleteTextureIfCopiedTo.enabled) + { + ContextGL *contextGL = GetImplAs(context); + contextGL->setNeedsFlushBeforeDeleteTextures(); + } + return angle::Result::Continue; } @@ -852,13 +865,22 @@ angle::Result TextureGL::copySubTextureHelper(const gl::Context *context, bool unpackUnmultiplyAlpha, const gl::Texture *source) { - const FunctionsGL *functions = GetFunctionsGL(context); - BlitGL *blitter = GetBlitGL(context); + const FunctionsGL *functions = GetFunctionsGL(context); + const angle::FeaturesGL &features = GetFeaturesGL(context); + BlitGL *blitter = GetBlitGL(context); TextureGL *sourceGL = GetImplAs(source); const gl::ImageDesc &sourceImageDesc = sourceGL->mState.getImageDesc(NonCubeTextureTypeToTarget(source->getType()), sourceLevel); + if (features.flushBeforeDeleteTextureIfCopiedTo.enabled) + { + // Conservatively indicate that this workaround is necessary. Not clear + // if it is on this code path, but added for symmetry. + ContextGL *contextGL = GetImplAs(context); + contextGL->setNeedsFlushBeforeDeleteTextures(); + } + // Check is this is a simple copySubTexture that can be done with a copyTexSubImage ASSERT(sourceGL->getType() == gl::TextureType::_2D || source->getType() == gl::TextureType::External || diff --git a/src/libANGLE/renderer/gl/renderergl_utils.cpp b/src/libANGLE/renderer/gl/renderergl_utils.cpp index b7fb81f8f..b4eed36bc 100644 --- a/src/libANGLE/renderer/gl/renderergl_utils.cpp +++ b/src/libANGLE/renderer/gl/renderergl_utils.cpp @@ -1633,6 +1633,9 @@ void InitializeFeatures(const FunctionsGL *functions, angle::FeaturesGL *feature functions->standard == STANDARD_GL_ES && functions->isAtLeastGLES(gl::Version(3, 1)) && functions->hasGLESExtension("GL_EXT_texture_norm16")); + + // anglebug.com/4267 + ANGLE_FEATURE_CONDITION(features, flushBeforeDeleteTextureIfCopiedTo, IsApple() && isIntel); } void InitializeFrontendFeatures(const FunctionsGL *functions, angle::FrontendFeatures *features) diff --git a/src/tests/gl_tests/CopyTexImageTest.cpp b/src/tests/gl_tests/CopyTexImageTest.cpp index 9acad4aa1..edba4e655 100644 --- a/src/tests/gl_tests/CopyTexImageTest.cpp +++ b/src/tests/gl_tests/CopyTexImageTest.cpp @@ -482,6 +482,53 @@ TEST_P(CopyTexImageTest, CopyTexSubImageToNonCubeCompleteDestination) } } +// Deleting textures after copying to them. http://anglebug.com/4267 +TEST_P(CopyTexImageTest, DeleteAfterCopyingToTextures) +{ + // Asserts on Vulkan backend. http://anglebug.com/4274 + ANGLE_SKIP_TEST_IF(IsVulkan()); + + GLTexture texture; + glBindTexture(GL_TEXTURE_2D, texture); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 2, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); + + GLTexture texture2; + glBindTexture(GL_TEXTURE_2D, texture2); + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 2, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); + + GLFramebuffer framebuffer; + glBindFramebuffer(GL_FRAMEBUFFER, framebuffer); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0); + ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER); + + // Perform CopyTexImage2D + glBindTexture(GL_TEXTURE_2D, texture); + glCopyTexImage2D(GL_TEXTURE_2D, 1, GL_RGBA, 0, 0, 2, 2, 0); + ASSERT_GL_NO_ERROR(); + // Not necessary to do any CopyTexImage2D operations to texture2. + + // Perform CopyTexSubImage2D + glBindTexture(GL_TEXTURE_2D, texture); + glCopyTexSubImage2D(GL_TEXTURE_2D, 1, 0, 0, 0, 0, 1, 1); + ASSERT_GL_NO_ERROR(); + glBindTexture(GL_TEXTURE_2D, texture2); + glCopyTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 0, 0, 1, 1); + ASSERT_GL_NO_ERROR(); + + // Clean up - provokes crash on buggy drivers. + texture.reset(); + // Crashes on Intel GPUs on macOS. + texture2.reset(); +} + // specialization of CopyTexImageTest is added so that some tests can be explicitly run with an ES3 // context class CopyTexImageTestES3 : public CopyTexImageTest