[Fix] (MG_Backend/DirectGLES, MG_Impl/GLImpl): ReadPixels - fall back to wide-format conversion when the ES driver rejects a legacy native read combo (Adreno errors on e.g. GL_RED/GL_UNSIGNED_INT and leaves the buffer untouched), and enforce packed-type/format pairing at the state layer via shared ValidateClientFormatTypePairing (GL_RED + GL_UNSIGNED_SHORT_5_6_5 now raises GL_INVALID_OPERATION)

This commit is contained in:
2026-07-16 06:11:53 -04:00
parent 12a67f596c
commit bae222227a
5 changed files with 92 additions and 8 deletions
@@ -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");
@@ -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();
+27 -8
View File
@@ -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<GenericErrorInfo>("MG_Impl/GLImpl", "ValidateTextureInternalFormatCompatibleWithInput",
message));
MakeUnique<GenericErrorInfo>("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<GenericErrorInfo>("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");
@@ -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);
@@ -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;