From 8a7ad933a27bf66fa2445546e65908d2eedeabb1 Mon Sep 17 00:00:00 2001 From: Yuxin Hu Date: Wed, 13 Sep 2023 15:11:46 +0000 Subject: [PATCH] Revert "Make egl surface uncurrent when being destroyed" This reverts commit 497440cdcb7d2ee59bca612dd07fc13cf09a6a57. Reason for revert: this caused chromium webview tests failures: https://chromium-review.googlesource.com/c/chromium/src/+/4860891. Original change's description: > Make egl surface uncurrent when being destroyed > > This is to workaround errors when app does below behaviors: > > 1) while there is a context still bound to the current > rendering thread and the surface, call eglDestroySurface() > 2) create a new surface eglCreateWindowSurface() > 3) call eglMakeCurrent() with the surface created in step 2) > 4) does work on the new surface > > The old surface won't be destroyed in step 1) because > it was still bound by the context of the current rendering > thread. When creating new surface on step 2), some hardware > will return error code EGL_BAD_ALLOC, because the old egl > surface is still associated with the native window. > > To workaround, when destroying surface, if the surface > is still bound by the context of the current rendering > thread, release the context and surface by passing > EGL_NO_CONTEXT and EGL_NO_SURFACE to eglMakeCurrent(). > > The workaround is controlled by a frontend feature > uncurrentEglSurfaceUponSurfaceDestroy. This feature > is only enabled on vulkan and gl backends. > > Bug: b/292285899 > Change-Id: I872d2e116ba6860f58d1176f011a5ef7c5a5af4e > Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4851255 > Reviewed-by: Yiwei Zhang > Reviewed-by: Charlie Lao > Reviewed-by: Shahbaz Youssefi > Commit-Queue: Yuxin Hu Bug: b/292285899 Change-Id: I760054d856294e6691e79e165fd73ce9e560621f No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4862958 Bot-Commit: Rubber Stamper Auto-Submit: Yuxin Hu Commit-Queue: Rubber Stamper --- .../autogen/FrontendFeatures_autogen.h | 7 --- include/platform/frontend_features.json | 8 ---- src/libANGLE/renderer/gl/renderergl_utils.cpp | 3 -- src/libANGLE/renderer/vulkan/RendererVk.cpp | 3 -- src/libGLESv2/egl_stubs.cpp | 27 ------------ src/tests/egl_tests/EGLSurfaceTest.cpp | 44 ------------------- util/autogen/angle_features_autogen.cpp | 1 - util/autogen/angle_features_autogen.h | 1 - 8 files changed, 94 deletions(-) diff --git a/include/platform/autogen/FrontendFeatures_autogen.h b/include/platform/autogen/FrontendFeatures_autogen.h index 9f9ccf7a0..e53944460 100644 --- a/include/platform/autogen/FrontendFeatures_autogen.h +++ b/include/platform/autogen/FrontendFeatures_autogen.h @@ -161,13 +161,6 @@ struct FrontendFeatures : FeatureSetBase &members, "http://anglebug.com/8280" }; - FeatureInfo uncurrentEglSurfaceUponSurfaceDestroy = { - "uncurrentEglSurfaceUponSurfaceDestroy", - FeatureCategory::FrontendWorkarounds, - "Make egl surface uncurrent when calling eglDestroySurface(), if the surface is still bound by the context of current render thread", - &members, "https://issuetracker.google.com/292285899" - }; - }; inline FrontendFeatures::FrontendFeatures() = default; diff --git a/include/platform/frontend_features.json b/include/platform/frontend_features.json index 91e26c695..3ad6a904d 100644 --- a/include/platform/frontend_features.json +++ b/include/platform/frontend_features.json @@ -161,14 +161,6 @@ "Check the filesystem for translated shaders to use instead of the shader translator's" ], "issue": "http://anglebug.com/8280" - }, - { - "name": "uncurrent_egl_surface_upon_surface_destroy", - "category": "Workarounds", - "description": [ - "Make egl surface uncurrent when calling eglDestroySurface(), if the surface is still bound by the context of current render thread" - ], - "issue": "https://issuetracker.google.com/292285899" } ] } diff --git a/src/libANGLE/renderer/gl/renderergl_utils.cpp b/src/libANGLE/renderer/gl/renderergl_utils.cpp index 6b1675c2e..b83b09095 100644 --- a/src/libANGLE/renderer/gl/renderergl_utils.cpp +++ b/src/libANGLE/renderer/gl/renderergl_utils.cpp @@ -2608,9 +2608,6 @@ void InitializeFrontendFeatures(const FunctionsGL *functions, angle::FrontendFea // https://crbug.com/480992 // Disable shader program cache to workaround PowerVR Rogue issues. ANGLE_FEATURE_CONDITION(features, disableProgramBinary, IsPowerVrRogue(functions)); - - // https://issuetracker.google.com/292285899 - ANGLE_FEATURE_CONDITION(features, uncurrentEglSurfaceUponSurfaceDestroy, true); } void ReInitializeFeaturesAtGPUSwitch(const FunctionsGL *functions, angle::FeaturesGL *features) diff --git a/src/libANGLE/renderer/vulkan/RendererVk.cpp b/src/libANGLE/renderer/vulkan/RendererVk.cpp index d1008be09..255b67c9a 100644 --- a/src/libANGLE/renderer/vulkan/RendererVk.cpp +++ b/src/libANGLE/renderer/vulkan/RendererVk.cpp @@ -5014,9 +5014,6 @@ void RendererVk::initializeFrontendFeatures(angle::FrontendFeatures *features) c ANGLE_FEATURE_CONDITION(features, forceGlErrorChecking, (IsAndroid() && isSwiftShader)); ANGLE_FEATURE_CONDITION(features, cacheCompiledShader, true); - - // https://issuetracker.google.com/292285899 - ANGLE_FEATURE_CONDITION(features, uncurrentEglSurfaceUponSurfaceDestroy, true); } angle::Result RendererVk::getPipelineCacheSize(DisplayVk *displayVk, size_t *pipelineCacheSizeOut) diff --git a/src/libGLESv2/egl_stubs.cpp b/src/libGLESv2/egl_stubs.cpp index ade6dc674..b035a3269 100644 --- a/src/libGLESv2/egl_stubs.cpp +++ b/src/libGLESv2/egl_stubs.cpp @@ -331,33 +331,6 @@ EGLBoolean DestroySurface(Thread *thread, Display *display, egl::SurfaceID surfa { Surface *eglSurface = display->getSurface(surfaceID); - // Workaround https://issuetracker.google.com/292285899 - // When destroying surface, if the surface - // is still bound by the context of the current rendering - // thread, release the surface by passing EGL_NO_SURFACE to eglMakeCurrent(). - if (display->getFrontendFeatures().uncurrentEglSurfaceUponSurfaceDestroy.enabled && - eglSurface->isCurrentOnAnyContext() && - (thread->getCurrentDrawSurface() == eglSurface || - thread->getCurrentReadSurface() == eglSurface)) - { - SurfaceID drawSurface = PackParam(EGL_NO_SURFACE); - SurfaceID readSurface = PackParam(EGL_NO_SURFACE); - const gl::Context *currentContext = thread->getContext(); - const gl::ContextID contextID = currentContext->id(); - - // if surfaceless context is supported, only release the surface. - if (display->getExtensions().surfacelessContext) - { - MakeCurrent(thread, display, drawSurface, readSurface, contextID); - } - else - { - // if surfaceless context is not supported, release the context, too. - MakeCurrent(thread, display, drawSurface, readSurface, - PackParam(EGL_NO_CONTEXT)); - } - } - ANGLE_EGL_TRY_RETURN(thread, display->prepareForCall(), "eglDestroySurface", GetDisplayIfValid(display), EGL_FALSE); diff --git a/src/tests/egl_tests/EGLSurfaceTest.cpp b/src/tests/egl_tests/EGLSurfaceTest.cpp index b5b6fc631..34a5444a0 100644 --- a/src/tests/egl_tests/EGLSurfaceTest.cpp +++ b/src/tests/egl_tests/EGLSurfaceTest.cpp @@ -2792,50 +2792,6 @@ TEST_P(EGLSurfaceTest, DISABLED_RandomClearTearing) mOSWindow->resize(kInitialSize, kInitialSize); } -// Make sure a surface (from the same window) can be recreated after being destroyed, even if it's -// still current. -TEST_P(EGLSurfaceTest, DestroyAndRecreateWhileCurrent) -{ - setWindowVisible(mOSWindow, true); - - initializeDisplay(); - - mConfig = chooseDefaultConfig(true); - ASSERT_NE(mConfig, nullptr); - - EGLint surfaceType = EGL_NONE; - eglGetConfigAttrib(mDisplay, mConfig, EGL_SURFACE_TYPE, &surfaceType); - ASSERT_NE((surfaceType & EGL_WINDOW_BIT), 0); - - initializeWindowSurfaceWithAttribs(mConfig, {}, EGL_SUCCESS); - initializeMainContext(); - - eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext); - ASSERT_EGL_SUCCESS(); - - // Draw with this surface to make sure it's used. - ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), essl1_shaders::fs::Red()); - glViewport(0, 0, 64, 64); - drawQuad(program, essl1_shaders::PositionAttrib(), 0.5f); - ASSERT_GL_NO_ERROR(); - - // Destroy the surface while it's current; it won't actually be destroyed. - eglDestroySurface(mDisplay, mWindowSurface); - mWindowSurface = EGL_NO_SURFACE; - - // Create another surface from the same window right away. - initializeWindowSurfaceWithAttribs(mConfig, {}, EGL_SUCCESS); - - // Make the new surface current; this leads to the actual destruction of the previous surface. - EXPECT_EGL_TRUE(eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext)); - ASSERT_EGL_SUCCESS(); - - // Verify everything still works - ANGLE_GL_PROGRAM(program2, essl1_shaders::vs::Simple(), essl1_shaders::fs::Green()); - drawQuad(program2, essl1_shaders::PositionAttrib(), 0.5f); - EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green); - ASSERT_GL_NO_ERROR(); -} } // anonymous namespace GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(EGLSingleBufferTest); diff --git a/util/autogen/angle_features_autogen.cpp b/util/autogen/angle_features_autogen.cpp index 5b1fdc77f..28ad5c8f0 100644 --- a/util/autogen/angle_features_autogen.cpp +++ b/util/autogen/angle_features_autogen.cpp @@ -346,7 +346,6 @@ constexpr PackedEnumMap kFeatureNames = {{ {Feature::SyncMonolithicPipelinesToBlobCache, "syncMonolithicPipelinesToBlobCache"}, {Feature::SyncVertexArraysToDefault, "syncVertexArraysToDefault"}, {Feature::UnbindFBOBeforeSwitchingContext, "unbindFBOBeforeSwitchingContext"}, - {Feature::UncurrentEglSurfaceUponSurfaceDestroy, "uncurrentEglSurfaceUponSurfaceDestroy"}, {Feature::UnfoldShortCircuits, "unfoldShortCircuits"}, {Feature::UnpackLastRowSeparatelyForPaddingInclusion, "unpackLastRowSeparatelyForPaddingInclusion"}, {Feature::UnpackOverlappingRowsSeparatelyUnpackBuffer, "unpackOverlappingRowsSeparatelyUnpackBuffer"}, diff --git a/util/autogen/angle_features_autogen.h b/util/autogen/angle_features_autogen.h index f3531648b..dfda8e38f 100644 --- a/util/autogen/angle_features_autogen.h +++ b/util/autogen/angle_features_autogen.h @@ -346,7 +346,6 @@ enum class Feature SyncMonolithicPipelinesToBlobCache, SyncVertexArraysToDefault, UnbindFBOBeforeSwitchingContext, - UncurrentEglSurfaceUponSurfaceDestroy, UnfoldShortCircuits, UnpackLastRowSeparatelyForPaddingInclusion, UnpackOverlappingRowsSeparatelyUnpackBuffer,