From 5d8a89e44f97a1622a0b5d5c3caf4597362b30fc Mon Sep 17 00:00:00 2001 From: Tim Van Patten Date: Tue, 2 Nov 2021 19:57:04 -0600 Subject: [PATCH] Vulkan: Always override surface format GL_RGB8 to GL_RGBA8 If an app requests to create a surface with GL_RGB8, override it to be GL_RGBA8 for Android. This is to workaround an issue with the Android Vulkan loader which limits which formats can be used with swapchains. This CL also adds GL_RGB8 back to DisplayVkAndroid::generateConfigs(), effectively reverting the following CL: https://chromium-review.googlesource.com/c/angle/angle/+/3235466 This is being done with this CL (rather than reverting) since these changes are required to handle surfaces created with GL_RGB8. Bug: angleproject:6277 Bug: angleproject:6651 Change-Id: Iad78ea0d7bdf12e1e309ed6a7181f08fac38b9de Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3258143 Reviewed-by: Tim Van Patten Reviewed-by: Ian Elliott Reviewed-by: Jamie Madill Commit-Queue: Tim Van Patten --- include/platform/FeaturesVk.h | 5 ++ src/libANGLE/renderer/vulkan/RendererVk.cpp | 7 +++ src/libANGLE/renderer/vulkan/SurfaceVk.cpp | 40 ++++++++++++---- .../vulkan/android/DisplayVkAndroid.cpp | 2 +- .../renderer/vulkan/vk_caps_utils.cpp | 9 +++- src/libANGLE/renderer/vulkan/vk_helpers.cpp | 7 +-- src/libANGLE/renderer/vulkan/vk_helpers.h | 3 +- .../egl_tests/EGLNoConfigContextTest.cpp | 16 +++++-- src/tests/gl_tests/ClearTest.cpp | 46 ++++++++++++++++++- 9 files changed, 116 insertions(+), 19 deletions(-) diff --git a/include/platform/FeaturesVk.h b/include/platform/FeaturesVk.h index 3b287b213..950c433fd 100644 --- a/include/platform/FeaturesVk.h +++ b/include/platform/FeaturesVk.h @@ -588,6 +588,11 @@ struct FeaturesVk : FeatureSetBase Feature supportsSurfaceProtectedSwapchains = { "supportsSurfaceProtectedSwapchains", FeatureCategory::VulkanFeatures, "VkSurface supportsProtected for protected swapchains", &members}; + + // Whether surface format GL_RGB8 should be overridden to GL_RGBA8. + Feature overrideSurfaceFormatRGB8toRGBA8 = { + "overrideSurfaceFormatRGB8toRGBA8", FeatureCategory::VulkanWorkarounds, + "Override surface format GL_RGB8 to GL_RGBA8", &members, "http://anglebug.com/6651"}; }; inline FeaturesVk::FeaturesVk() = default; diff --git a/src/libANGLE/renderer/vulkan/RendererVk.cpp b/src/libANGLE/renderer/vulkan/RendererVk.cpp index 28ed47c7e..52d24f68f 100644 --- a/src/libANGLE/renderer/vulkan/RendererVk.cpp +++ b/src/libANGLE/renderer/vulkan/RendererVk.cpp @@ -2813,6 +2813,13 @@ void RendererVk::initFeatures(DisplayVk *displayVk, // descriptor counts for such immutable samplers ANGLE_FEATURE_CONDITION(&mFeatures, useMultipleDescriptorsForExternalFormats, true); + // http://anglebug.com/6651 + // When creating a surface with the format GL_RGB8, override the format to be GL_RGBA8, since + // Android prevents creating swapchain images with VK_FORMAT_R8G8B8_UNORM. + // Do this for all platforms, since few (none?) IHVs support 24-bit formats with their HW + // natively anyway. + ANGLE_FEATURE_CONDITION(&mFeatures, overrideSurfaceFormatRGB8toRGBA8, true); + angle::PlatformMethods *platform = ANGLEPlatformCurrent(); platform->overrideFeaturesVk(platform, &mFeatures); diff --git a/src/libANGLE/renderer/vulkan/SurfaceVk.cpp b/src/libANGLE/renderer/vulkan/SurfaceVk.cpp index b4313e584..1dbc42a79 100644 --- a/src/libANGLE/renderer/vulkan/SurfaceVk.cpp +++ b/src/libANGLE/renderer/vulkan/SurfaceVk.cpp @@ -132,13 +132,21 @@ angle::Result InitImageHelper(DisplayVk *displayVk, VkExtent3D extents = {std::max(static_cast(width), 1u), std::max(static_cast(height), 1u), 1u}; + angle::FormatID renderableFormatId = vkFormat.getActualRenderableImageFormatID(); + RendererVk *rendererVk = displayVk->getRenderer(); + // For devices that don't support creating swapchain images with RGB8, emulate with RGBA8. + if (rendererVk->getFeatures().overrideSurfaceFormatRGB8toRGBA8.enabled && + renderableFormatId == angle::FormatID::R8G8B8_UNORM) + { + renderableFormatId = angle::FormatID::R8G8B8A8_UNORM; + } + VkImageCreateFlags imageCreateFlags = hasProtectedContent ? VK_IMAGE_CREATE_PROTECTED_BIT : vk::kVkImageCreateFlagsNone; ANGLE_TRY(imageHelper->initExternal( displayVk, gl::TextureType::_2D, extents, vkFormat.getIntendedFormatID(), - vkFormat.getActualRenderableImageFormatID(), samples, usage, imageCreateFlags, - vk::ImageLayout::Undefined, nullptr, gl::LevelIndex(0), 1, 1, isRobustResourceInitEnabled, - nullptr, hasProtectedContent)); + renderableFormatId, samples, usage, imageCreateFlags, vk::ImageLayout::Undefined, nullptr, + gl::LevelIndex(0), 1, 1, isRobustResourceInitEnabled, nullptr, hasProtectedContent)); return angle::Result::Continue; } @@ -858,6 +866,13 @@ angle::Result WindowSurfaceVk::initializeImpl(DisplayVk *displayVk) const vk::Format &format = renderer->getFormat(mState.config->renderTargetFormat); VkFormat nativeFormat = format.getActualRenderableImageVkFormat(); + RendererVk *rendererVk = displayVk->getRenderer(); + // For devices that don't support creating swapchain images with RGB8, emulate with RGBA8. + if (rendererVk->getFeatures().overrideSurfaceFormatRGB8toRGBA8.enabled && + nativeFormat == VK_FORMAT_R8G8B8_UNORM) + { + nativeFormat = VK_FORMAT_R8G8B8A8_UNORM; + } bool surfaceFormatSupported = false; VkColorSpaceKHR colorSpace = MapEglColorSpaceToVkColorSpace( @@ -1094,8 +1109,16 @@ angle::Result WindowSurfaceVk::createSwapChain(vk::Context *context, RendererVk *renderer = context->getRenderer(); VkDevice device = renderer->getDevice(); - const vk::Format &format = renderer->getFormat(mState.config->renderTargetFormat); - VkFormat nativeFormat = format.getActualRenderableImageVkFormat(); + const vk::Format &format = renderer->getFormat(mState.config->renderTargetFormat); + angle::FormatID actualFormatID = format.getActualRenderableImageFormatID(); + angle::FormatID intendedFormatID = format.getIntendedFormatID(); + + // For devices that don't support creating swapchain images with RGB8, emulate with RGBA8. + if (renderer->getFeatures().overrideSurfaceFormatRGB8toRGBA8.enabled && + intendedFormatID == angle::FormatID::R8G8B8_UNORM) + { + actualFormatID = angle::FormatID::R8G8B8A8_UNORM; + } gl::Extents rotatedExtents = extents; if (Is90DegreeRotation(getPreTransform())) @@ -1129,7 +1152,7 @@ angle::Result WindowSurfaceVk::createSwapChain(vk::Context *context, swapchainInfo.flags = mState.hasProtectedContent() ? VK_SWAPCHAIN_CREATE_PROTECTED_BIT_KHR : 0; swapchainInfo.surface = mSurface; swapchainInfo.minImageCount = mMinImageCount; - swapchainInfo.imageFormat = nativeFormat; + swapchainInfo.imageFormat = vk::GetVkFormatFromFormatID(actualFormatID); swapchainInfo.imageColorSpace = MapEglColorSpaceToVkColorSpace( static_cast(mState.attributes.get(EGL_GL_COLORSPACE, EGL_NONE))); // Note: Vulkan doesn't allow 0-width/height swapchains. @@ -1205,9 +1228,10 @@ angle::Result WindowSurfaceVk::createSwapChain(vk::Context *context, for (uint32_t imageIndex = 0; imageIndex < imageCount; ++imageIndex) { SwapchainImage &member = mSwapchainImages[imageIndex]; + member.image.init2DWeakReference(context, swapchainImages[imageIndex], extents, - Is90DegreeRotation(getPreTransform()), format, 1, - robustInit); + Is90DegreeRotation(getPreTransform()), intendedFormatID, + actualFormatID, 1, robustInit); member.imageViews.init(renderer); member.mFrameNumber = 0; } diff --git a/src/libANGLE/renderer/vulkan/android/DisplayVkAndroid.cpp b/src/libANGLE/renderer/vulkan/android/DisplayVkAndroid.cpp index ce20f4860..a4b2cfeec 100644 --- a/src/libANGLE/renderer/vulkan/android/DisplayVkAndroid.cpp +++ b/src/libANGLE/renderer/vulkan/android/DisplayVkAndroid.cpp @@ -54,7 +54,7 @@ egl::ConfigSet DisplayVkAndroid::generateConfigs() // https://cs.android.com/android/platform/superproject/+/master:frameworks/native/vulkan/libvulkan/swapchain.cpp;l=465-486?q=GetNativePixelFormat // TODO (Issue 4062): Add conditional support for GL_RGB10_A2 and GL_RGBA16F when the // Android Vulkan loader adds conditional support for them. - const std::array kColorFormats = {GL_RGBA8, GL_RGB565}; + const std::array kColorFormats = {GL_RGBA8, GL_RGB8, GL_RGB565}; std::vector depthStencilFormats( egl_vk::kConfigDepthStencilFormats, diff --git a/src/libANGLE/renderer/vulkan/vk_caps_utils.cpp b/src/libANGLE/renderer/vulkan/vk_caps_utils.cpp index 27f084a19..f4b5ad6b6 100644 --- a/src/libANGLE/renderer/vulkan/vk_caps_utils.cpp +++ b/src/libANGLE/renderer/vulkan/vk_caps_utils.cpp @@ -1165,6 +1165,13 @@ egl::Config GenerateDefaultConfig(DisplayVk *display, EGLint es2Support = (maxSupportedESVersion.major >= 2 ? EGL_OPENGL_ES2_BIT : 0); EGLint es3Support = (maxSupportedESVersion.major >= 3 ? EGL_OPENGL_ES3_BIT : 0); + EGLint surfaceType = EGL_WINDOW_BIT; + // Don't support RGB8 PBuffers. + if (colorFormat.internalFormat != GL_RGB8) + { + surfaceType |= EGL_PBUFFER_BIT; + } + egl::Config config; config.renderTargetFormat = colorFormat.internalFormat; @@ -1195,7 +1202,7 @@ egl::Config GenerateDefaultConfig(DisplayVk *display, config.renderableType = es1Support | es2Support | es3Support; config.sampleBuffers = (sampleCount > 0) ? 1 : 0; config.samples = sampleCount; - config.surfaceType = EGL_WINDOW_BIT | EGL_PBUFFER_BIT; + config.surfaceType = surfaceType; // Vulkan surfaces use a different origin than OpenGL, always prefer to be flipped vertically if // possible. config.optimalOrientation = EGL_SURFACE_ORIENTATION_INVERT_Y_ANGLE; diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp index 7d4172d9d..0c60008b9 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp +++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp @@ -4599,7 +4599,8 @@ void ImageHelper::init2DWeakReference(Context *context, VkImage handle, const gl::Extents &glExtents, bool rotatedAspectRatio, - const Format &format, + angle::FormatID intendedFormatID, + angle::FormatID actualFormatID, GLint samples, bool isRobustResourceInitEnabled) { @@ -4609,8 +4610,8 @@ void ImageHelper::init2DWeakReference(Context *context, gl_vk::GetExtent(glExtents, &mExtents); mRotatedAspectRatio = rotatedAspectRatio; - mIntendedFormatID = format.getIntendedFormatID(); - mActualFormatID = format.getActualRenderableImageFormatID(); + mIntendedFormatID = intendedFormatID; + mActualFormatID = actualFormatID; mSamples = std::max(samples, 1); mImageSerial = context->getRenderer()->getResourceSerialFactory().generateImageSerial(); mCurrentLayout = ImageLayout::Undefined; diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h index ed84fa677..a53cae04d 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.h +++ b/src/libANGLE/renderer/vulkan/vk_helpers.h @@ -1612,7 +1612,8 @@ class ImageHelper final : public Resource, public angle::Subject VkImage handle, const gl::Extents &glExtents, bool rotatedAspectRatio, - const Format &format, + angle::FormatID intendedFormatID, + angle::FormatID actualFormatID, GLint samples, bool isRobustResourceInitEnabled); void resetImageWeakReference(); diff --git a/src/tests/egl_tests/EGLNoConfigContextTest.cpp b/src/tests/egl_tests/EGLNoConfigContextTest.cpp index 0215cea4b..916e60b43 100644 --- a/src/tests/egl_tests/EGLNoConfigContextTest.cpp +++ b/src/tests/egl_tests/EGLNoConfigContextTest.cpp @@ -98,13 +98,23 @@ TEST_P(EGLNoConfigContextTest, RenderCheck) EGLint configId; EXPECT_EGL_TRUE(eglGetConfigAttrib(mDisplay, config, EGL_CONFIG_ID, &configId)); + + EGLint surfaceType; + EXPECT_EGL_TRUE(eglGetConfigAttrib(mDisplay, config, EGL_SURFACE_TYPE, &surfaceType)); + EGLint bufferSize; + EXPECT_EGL_TRUE(eglGetConfigAttrib(mDisplay, config, EGL_BUFFER_SIZE, &bufferSize)); + constexpr int kRGB8BitSize = 24; // RGB8 is 24 bits + if (isVulkanRenderer() && bufferSize == kRGB8BitSize && + (surfaceType & EGL_PBUFFER_BIT) != EGL_PBUFFER_BIT) + { + // Skip this config, since the Vulkan backend doesn't support RGB8 pbuffer surfaces. + continue; + } + EGLint surfattrs[] = {EGL_WIDTH, kWidth, EGL_HEIGHT, kHeight, EGL_NONE}; surface = eglCreatePbufferSurface(mDisplay, config, surfattrs); EXPECT_TRUE(surface != EGL_NO_SURFACE); - EGLint bufferSize = 0; - EXPECT_EGL_TRUE(eglGetConfigAttrib(mDisplay, config, EGL_BUFFER_SIZE, &bufferSize)); - EXPECT_EGL_TRUE(eglMakeCurrent(mDisplay, surface, surface, mContext)); ASSERT_EGL_SUCCESS() << "eglMakeCurrent failed with Config: " << configId << '\n'; diff --git a/src/tests/gl_tests/ClearTest.cpp b/src/tests/gl_tests/ClearTest.cpp index 97e3ee5a2..b673f7137 100644 --- a/src/tests/gl_tests/ClearTest.cpp +++ b/src/tests/gl_tests/ClearTest.cpp @@ -362,13 +362,55 @@ TEST_P(ClearTestRGB, InvalidateDefaultFramebufferRGB) ANGLE_SKIP_TEST_IF(getClientMajorVersion() < 3); ANGLE_SKIP_TEST_IF(IsD3D11()); + glClearColor(0.0f, 0.0f, 0.0f, 0.0f); + glClear(GL_COLOR_BUFFER_BIT); + // Verify that even though Alpha is cleared to 0.0 for this RGB FBO, it should be read back as + // 1.0, since the glReadPixels() is issued with GL_RGBA. + // OpenGL ES 3.2 spec: + // 16.1.3 Obtaining Pixels from the Framebuffer + // If G, B, or A values are not present in the internal format, they are taken to be zero, + // zero, and one respectively. + EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::black); + const GLenum discards[] = {GL_COLOR}; glInvalidateFramebuffer(GL_FRAMEBUFFER, 1, discards); - EXPECT_PIXEL_NEAR(0, 0, 0, 0, 0, 255, 1.0); // Don't explicitly clear, but draw blue (make sure alpha is not cleared) drawQuad(blueProgram, essl1_shaders::PositionAttrib(), 0.5f); - EXPECT_PIXEL_NEAR(0, 0, 0, 0, 255, 255, 1.0); + EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::blue); +} + +// Draw with a shader that outputs alpha=0.5. Readback and ensure that alpha=1. +TEST_P(ClearTestRGB, ShaderOutputsAlphaVerifyReadingAlphaIsOne) +{ + ANGLE_GL_PROGRAM(blueProgram, essl1_shaders::vs::Simple(), essl1_shaders::fs::UniformColor()); + glUseProgram(blueProgram); + + // Some GPUs don't support RGB format default framebuffer, + // so skip if the back buffer has alpha bits. + EGLWindow *window = getEGLWindow(); + EGLDisplay display = window->getDisplay(); + EGLConfig config = window->getConfig(); + EGLint backbufferAlphaBits = 0; + eglGetConfigAttrib(display, config, EGL_ALPHA_SIZE, &backbufferAlphaBits); + ANGLE_SKIP_TEST_IF(backbufferAlphaBits != 0); + // glInvalidateFramebuffer() isn't supported with GLES 2.0 + ANGLE_SKIP_TEST_IF(getClientMajorVersion() < 3); + ANGLE_SKIP_TEST_IF(IsD3D11()); + + glClearColor(0.0f, 0.0f, 0.0f, 0.0f); + glClear(GL_COLOR_BUFFER_BIT); + EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::black); + + GLint colorUniformLocation = + glGetUniformLocation(blueProgram, angle::essl1_shaders::ColorUniform()); + ASSERT_NE(colorUniformLocation, -1); + glUniform4f(colorUniformLocation, 0.0f, 0.0f, 1.0f, 0.5f); + ASSERT_GL_NO_ERROR(); + + // Don't explicitly clear, but draw blue (make sure alpha is not cleared) + drawQuad(blueProgram, essl1_shaders::PositionAttrib(), 0.5f); + EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::blue); } // Test clearing a RGBA8 Framebuffer