diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index c28e30d6..b1075234 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -3760,7 +3760,30 @@ namespace MobileGL::MG_Backend::DirectGLES { } MGLOG_D("ReadPixels: glReadPixels()"); + DrainESErrors(); g_GLESFuncs.glReadPixels(x, y, width, height, format, type, pixels); + const GLenum nativeReadError = g_GLESFuncs.glGetError(); + if (nativeReadError != GL_NO_ERROR) { + // ES drivers only guarantee GL_RGBA/GL_UNSIGNED_BYTE, GL_RGBA_INTEGER/(U)INT, float RGBA and one + // implementation-defined pair; legacy combos like GL_RED/GL_UNSIGNED_INT are rejected by e.g. + // Adreno with a GL error and an untouched destination (GL CTS packed_pixels r8_format_red). The + // failed read wrote nothing (client memory and PBO alike), so re-service the request through the + // wide-format conversion path before any PBO writeback can capture stale contents. The conversion + // helper saves/restores the ES pixel-pack binding and handles the state-layer PBO itself. + DrainESErrors(); + MGLOG_D("ReadPixels: native read of %s/%s failed (%s), retrying via client-format conversion", + MG_Util::ConvertGLEnumToString(format).c_str(), MG_Util::ConvertGLEnumToString(type).c_str(), + MG_Util::ConvertGLEnumToString(nativeReadError).c_str()); + if (ReadPixelsViaFormatConversion(x, y, width, height, format, type, pixels)) { + MGLOG_D("ReadPixels: finished via client-format conversion after native failure"); + return; + } + MGLOG_E("ReadPixels: native read of %s/%s failed (%s) and no conversion path covers it, " + "skipping readback", + MG_Util::ConvertGLEnumToString(format).c_str(), MG_Util::ConvertGLEnumToString(type).c_str(), + MG_Util::ConvertGLEnumToString(nativeReadError).c_str()); + return; + } if (usePBO) { // pull back to client memory if PBO is used MGLOG_D("ReadPixels: PBO used, mapping buffer to client memory"); diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index e06f9032..f987cdfa 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -1681,6 +1681,13 @@ namespace MobileGL::MG_Impl::GLImpl { } } + // Packed-type/format pairing (GL CTS packed_pixels: e.g. GL_RED with GL_UNSIGNED_SHORT_5_6_5 must + // raise an error instead of reaching the backend). Shared with the TexImage/GetTexImage validators; + // runs after the depth-stencil branch above so DEPTH_STENCIL with a wrong type keeps GL_INVALID_ENUM. + if (!TextureImpl::ValidateClientFormatTypePairing(textureInputFormat, texturePixelDataType)) { + return false; + } + // Check PBO state const auto& pixelPackBufferObject = MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::PixelPack).GetBoundObject(); diff --git a/MobileGL/MG_Impl/GLImpl/Texture/Validators.cpp b/MobileGL/MG_Impl/GLImpl/Texture/Validators.cpp index bb060efe..833c63cc 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/Validators.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/Validators.cpp @@ -235,17 +235,15 @@ namespace MobileGL::MG_Impl::GLImpl::TextureImpl { format == TextureInputFormat::StencilIndex; } - // Mirrors the desktop-GL validity matrix used by GL CTS packed_pixels (glcPackedPixelsTests - // isFormatValid, INPUT_TEXIMAGE): packed-type/format pairing, depth-vs-color mismatch, and - // integer-ness matching all raise GL_INVALID_OPERATION instead of reaching the upload path. - Bool ValidateTextureInternalFormatCompatibleWithInput(TextureInputFormat format, - TextureInternalFormat internalFormat, - TexturePixelDataType type) { + // Client-memory format<->type pairing rules shared by pixel uploads (TexImage*) and readbacks + // (ReadPixels, GetTexImage). Mirrors the desktop-GL validity matrix used by GL CTS packed_pixels + // (glcPackedPixelsTests isFormatValid): packed types constrain the formats they may pair with, and + // integer formats reject floating-point types; violations raise GL_INVALID_OPERATION. + Bool ValidateClientFormatTypePairing(TextureInputFormat format, TexturePixelDataType type) { const auto recordInvalidOperation = [](const char* message) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", "ValidateTextureInternalFormatCompatibleWithInput", - message)); + MakeUnique("MG_Impl/GLImpl", "ValidateClientFormatTypePairing", message)); return false; }; @@ -288,6 +286,27 @@ namespace MobileGL::MG_Impl::GLImpl::TextureImpl { return recordInvalidOperation("Integer format cannot be used with a floating-point type"); } + return true; + } + + // Mirrors the desktop-GL validity matrix used by GL CTS packed_pixels (glcPackedPixelsTests + // isFormatValid, INPUT_TEXIMAGE): packed-type/format pairing, depth-vs-color mismatch, and + // integer-ness matching all raise GL_INVALID_OPERATION instead of reaching the upload path. + Bool ValidateTextureInternalFormatCompatibleWithInput(TextureInputFormat format, + TextureInternalFormat internalFormat, + TexturePixelDataType type) { + const auto recordInvalidOperation = [](const char* message) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", "ValidateTextureInternalFormatCompatibleWithInput", + message)); + return false; + }; + + if (!ValidateClientFormatTypePairing(format, type)) { + return false; + } + // TexImage in core 3.3 has no stencil-only upload path (that arrived with GL 4.4). if (format == TextureInputFormat::StencilIndex) { return recordInvalidOperation("STENCIL_INDEX is not a valid texture upload format"); diff --git a/MobileGL/MG_Impl/GLImpl/Texture/Validators.h b/MobileGL/MG_Impl/GLImpl/Texture/Validators.h index b88fdc6d..32291304 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/Validators.h +++ b/MobileGL/MG_Impl/GLImpl/Texture/Validators.h @@ -23,6 +23,7 @@ namespace MobileGL::MG_Impl::GLImpl::TextureImpl { Bool ValidateTextureSizeRange(Int width, Int height, Int depth); Bool ValidateTextureInternalFormat(TextureInternalFormat format); Bool ValidateTextureBorderNumber(Int border); + Bool ValidateClientFormatTypePairing(TextureInputFormat format, TexturePixelDataType type); Bool ValidateTextureInternalFormatCompatibleWithInput(TextureInputFormat format, TextureInternalFormat internalFormat, TexturePixelDataType type); diff --git a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp index 70ae7d11..919f06b5 100644 --- a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp +++ b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp @@ -221,6 +221,40 @@ TEST_F(FramebufferTest, ReadPixelsAllowsPersistentMappedPixelPackBuffer) { EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); } +TEST_F(FramebufferTest, ReadPixelsRejectsMismatchedPackedTypeFormatPairs) { + GLuint framebuffer = 0; + GLuint texture = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &framebuffer); + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &texture); + MG_Impl::GLImpl::TextureStorage2D(texture, 1, GL_RGBA8, 4, 4); + MG_Impl::GLImpl::NamedFramebufferTexture(framebuffer, GL_COLOR_ATTACHMENT0, texture, 0); + MG_Impl::GLImpl::BindFramebuffer(GL_READ_FRAMEBUFFER, framebuffer); + + MG_Backend::gBackendFunctionsTable.GL.ReadPixels = RecordReadPixels; + Uint8 pixelStorage[4 * 4 * 4] = {}; + + // Packed RGB type with a non-RGB format must never reach the backend (GL CTS packed_pixels + // reads GL_RED with GL_UNSIGNED_SHORT_5_6_5 and expects an error). + MG_Impl::GLImpl::ReadPixels(0, 0, 4, 4, GL_RED, GL_UNSIGNED_SHORT_5_6_5, pixelStorage); + EXPECT_EQ(g_readPixelsCallCount, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + + // Packed RGBA type with a non-RGBA/BGRA format is rejected as well. + MG_Impl::GLImpl::ReadPixels(0, 0, 4, 4, GL_RGB, GL_UNSIGNED_INT_8_8_8_8, pixelStorage); + EXPECT_EQ(g_readPixelsCallCount, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + + // Packed depth-stencil type requires the DEPTH_STENCIL format. + MG_Impl::GLImpl::ReadPixels(0, 0, 4, 4, GL_RGBA, GL_UNSIGNED_INT_24_8, pixelStorage); + EXPECT_EQ(g_readPixelsCallCount, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + + // A plain RGBA/UNSIGNED_BYTE readback keeps working. + MG_Impl::GLImpl::ReadPixels(0, 0, 4, 4, GL_RGBA, GL_UNSIGNED_BYTE, pixelStorage); + EXPECT_EQ(g_readPixelsCallCount, 1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + TEST_F(FramebufferTest, NamedRenderbufferStorageAndFramebufferAttachDoNotChangeBindings) { GLuint framebuffer = 0; GLuint renderbuffer = 0;