[Fix, Test] (RenderState, DirectGLES, MG_IntegrationTest): tell a deliberately empty scissor box apart from one that was never written

This commit is contained in:
2026-08-20 22:27:14 -04:00
parent 7480bf4490
commit cd07d42a47
5 changed files with 341 additions and 13 deletions
+24 -12
View File
@@ -2134,19 +2134,31 @@ namespace MobileGL::MG_Backend::DirectGLES {
if (tailSpanDirty) { // Scissor box. Resolved and shadowed like the viewport above, and
// for the same reason: what has to reach the driver is NOT simply the parameter
// field. (0,0,0,0) is where RenderStateParameters::ScissorBox starts and the only
// thing that ever writes it is glScissor, so that value means "the application has
// never called glScissor" - it is not a GL scissor box. GL's initial box is the
// whole window, which the frontend has no way to spell before a surface exists.
// The pre-resync code got away with pushing the field verbatim only by accident:
// the shadow held the same default, the field never compared unequal, and the ES
// context kept its own correct default. Under the forced full push that accident
// is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle, and
// everything drawn with GL_SCISSOR_TEST enabled before the app's first glScissor
// is clipped away - Minecraft 26.2 keeps only its unscissored sky and hand and
// loses the terrain and the whole GUI.
// field. RenderStateParameters::ScissorBoxes starts all-zero, which means "the
// application has never called glScissor" - it is not a GL scissor box. GL's
// initial box is the whole window, which the frontend has no way to spell before a
// surface exists. The pre-resync code got away with pushing the field verbatim only
// by accident: the shadow held the same default, the field never compared unequal,
// and the ES context kept its own correct default. Under the forced full push that
// accident is gone, glScissor(0,0,0,0) shrinks the scissor to an EMPTY rectangle,
// and everything drawn with GL_SCISSOR_TEST enabled before the app's first
// glScissor is clipped away - Minecraft 26.2 keeps only its unscissored sky and
// hand and loses the terrain and the whole GUI.
//
// The condition is the WRITTEN FLAG, not the extent. An empty rectangle is a
// perfectly legal thing to ask for - glScissor(0,0,0,0) means "the scissor test
// rejects every fragment" - so testing `width <= 0 || height <= 0` substituted the
// whole surface for a deliberately empty box and inverted the request into "accept
// every fragment", no matter how many times the application had already called
// glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is exactly that: all 16
// boxes zero-sized with the test enabled, requiring the draw to be clipped away
// entirely. Reading the flag preserves the Minecraft protection bit-for-bit - before
// the first glScissor the bit is clear and the surface size is still substituted -
// while an explicit empty box now reaches the driver verbatim. Negative extents
// cannot arrive here at all: all three entry points reject them with
// GL_INVALID_VALUE before storing (GL_RenderState.cpp's ValidateNonNegativeExtent).
IntVec4 backendScissorBox = parameters.ScissorBoxes[0];
if (backendScissorBox.z() <= 0 || backendScissorBox.w() <= 0) {
if ((parameters.ScissorBoxWrittenMask & 1u) == 0) {
Int surfaceWidth = 0;
Int surfaceHeight = 0;
if (QueryCurrentSurfaceSize(surfaceWidth, surfaceHeight)) {
@@ -520,5 +520,215 @@ void main() { fragColor = vec4(float(gsIndex) * 16.0 / 255.0, 0.0, 0.0, 1.0); }
DestroyIntTarget(target);
}
// --- 4. an explicitly EMPTY scissor box clips, it does not mean "never written" --------
//
// Deliberately NOT a ViewportArrayScenario case, because it must run on DirectGLES - the
// backend that got it wrong - and that fixture skips there. It needs none of the routing:
// one viewport, one scissor rectangle, no geometry stage.
//
// glScissor(0, 0, 0, 0) is legal GL meaning "the scissor test rejects every fragment",
// but it is byte-identical to the all-zero rectangle a context starts with, whose meaning
// is the OPPOSITE ("the whole window", which the frontend cannot spell before a surface
// exists). DirectGLES resolved the collision from the EXTENT, so it substituted the whole
// surface for a deliberately empty box and inverted the request into "clip nothing" -
// and did so on every draw, at any origin, no matter how many times the application had
// already called glScissor. KHR-GL43.viewport_array.scissor_zero_dimension is the
// conformance shape of exactly this, and it is what the written-flag now separates.
const char* const kFullScreenVertexSource = R"(#version 330 core
void main() {
// One clip-space-covering triangle straight from gl_VertexID: no buffers, no attributes,
// and nothing that could clip the draw except the scissor rectangle under test.
const vec2 corners[3] = vec2[3](vec2(-1.0, -1.0), vec2(3.0, -1.0), vec2(-1.0, 3.0));
gl_Position = vec4(corners[gl_VertexID], 0.0, 1.0);
}
)";
const char* const kConstantIntFragmentSource = R"(#version 330 core
layout(location = 0) out int fragColor;
void main() { fragColor = 7; }
)";
constexpr GLint kPainted = 7;
class EmptyScissorScenario : public ScenarioTest {
protected:
void SetUp() override {
ScenarioTest::SetUp();
if (!Ready()) return;
m_program = BuildQuadProgram();
ASSERT_NE(m_program, 0u) << "full-screen program failed to build: " << m_buildLog;
glGenVertexArrays(1, &m_vao);
glBindVertexArray(m_vao);
glGenTextures(1, &m_texture);
glBindTexture(GL_TEXTURE_2D, m_texture);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
glTexImage2D(GL_TEXTURE_2D, 0, GL_R32I, kSurfaceSide, kSurfaceSide, 0, GL_RED_INTEGER, GL_INT,
nullptr);
glGenFramebuffers(1, &m_fbo);
glBindFramebuffer(GL_FRAMEBUFFER, m_fbo);
glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, m_texture, 0);
ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), GL_FRAMEBUFFER_COMPLETE)
<< "R32I is required to be colour-renderable; an incomplete target would make every "
"assertion below vacuous";
glViewport(0, 0, kSurfaceSide, kSurfaceSide);
glDisable(GL_DEPTH_TEST);
ResetScissorState();
ASSERT_EQ(glGetError(), GL_NO_ERROR) << "setup left a GL error behind";
}
void TearDown() override {
if (!Ready() || IsSkipped()) return;
// The context is shared with every other scenario in the process, and a leftover
// 0x0 scissor box with the test enabled would silently blank whatever runs next.
ResetScissorState();
glScissor(0, 0, kSurfaceSide, kSurfaceSide);
if (m_vao != 0) glDeleteVertexArrays(1, &m_vao);
if (m_program != 0) glDeleteProgram(m_program);
glBindFramebuffer(GL_FRAMEBUFFER, 0);
if (m_fbo != 0) glDeleteFramebuffers(1, &m_fbo);
if (m_texture != 0) glDeleteTextures(1, &m_texture);
while (glGetError() != GL_NO_ERROR) {
}
}
static void ResetScissorState() {
for (int i = 0; i < kViewportCount; ++i) {
glDisablei(GL_SCISSOR_TEST, static_cast<GLuint>(i));
}
glDisable(GL_SCISSOR_TEST);
}
// Uploaded, not cleared, for the reason FillIntTarget gives - and here for a second
// one that is decisive: glClear is ITSELF scissored, so a clear issued under the very
// state this case is testing would be clipped away and prove nothing.
void FillTarget() const {
const std::vector<GLint> unwritten(static_cast<size_t>(kSurfaceSide) * kSurfaceSide, kUnwritten);
glBindTexture(GL_TEXTURE_2D, m_texture);
glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT,
unwritten.data());
}
static std::vector<GLint> ReadTarget() {
std::vector<GLint> pixels(static_cast<size_t>(kSurfaceSide) * kSurfaceSide, 0);
glReadPixels(0, 0, kSurfaceSide, kSurfaceSide, GL_RED_INTEGER, GL_INT, pixels.data());
return pixels;
}
GLuint BuildQuadProgram() {
const GLuint vs = CompileOne(GL_VERTEX_SHADER, kFullScreenVertexSource);
if (vs == 0) return 0;
const GLuint fs = CompileOne(GL_FRAGMENT_SHADER, kConstantIntFragmentSource);
if (fs == 0) {
glDeleteShader(vs);
return 0;
}
const GLuint program = glCreateProgram();
glAttachShader(program, vs);
glAttachShader(program, fs);
glLinkProgram(program);
GLint linked = 0;
glGetProgramiv(program, GL_LINK_STATUS, &linked);
glDeleteShader(vs);
glDeleteShader(fs);
if (linked) return program;
GLint length = 0;
glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length);
std::vector<char> log(static_cast<size_t>(length > 1 ? length : 1), '\0');
glGetProgramInfoLog(program, static_cast<GLsizei>(log.size()), nullptr, log.data());
m_buildLog = log.data();
glDeleteProgram(program);
return 0;
}
GLuint CompileOne(GLenum stage, const char* source) {
const GLuint shader = glCreateShader(stage);
glShaderSource(shader, 1, &source, nullptr);
glCompileShader(shader);
GLint compiled = 0;
glGetShaderiv(shader, GL_COMPILE_STATUS, &compiled);
if (compiled) return shader;
GLint length = 0;
glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);
std::vector<char> log(static_cast<size_t>(length > 1 ? length : 1), '\0');
glGetShaderInfoLog(shader, static_cast<GLsizei>(log.size()), nullptr, log.data());
m_buildLog = log.data();
glDeleteShader(shader);
return 0;
}
std::string m_buildLog;
GLuint m_program = 0;
GLuint m_vao = 0;
GLuint m_fbo = 0;
GLuint m_texture = 0;
};
TEST_F(EmptyScissorScenario, AnExplicitlyEmptyScissorBoxClipsEveryFragment) {
// Positive control FIRST. Without it a regression that simply lost the draw entirely
// would sail through the half below, which only asserts that nothing was painted.
FillTarget();
glEnable(GL_SCISSOR_TEST);
glScissor(0, 0, kSurfaceSide, kSurfaceSide);
glUseProgram(m_program);
glBindVertexArray(m_vao);
glDrawArrays(GL_TRIANGLES, 0, 3);
ASSERT_EQ(glGetError(), GL_NO_ERROR);
{
const std::vector<GLint> pixels = ReadTarget();
ASSERT_EQ(pixels.front(), kPainted) << "control: a full-surface scissor box must not clip";
ASSERT_EQ(pixels.back(), kPainted) << "control: a full-surface scissor box must not clip";
}
// The case itself, and note it runs AFTER an explicit glScissor - the old
// extent-based sentinel misfired here too, which is what made this a live rendering
// bug and not just a first-frame startup quirk.
FillTarget();
glScissor(0, 0, 0, 0);
glDrawArrays(GL_TRIANGLES, 0, 3);
ASSERT_EQ(glGetError(), GL_NO_ERROR);
{
const std::vector<GLint> pixels = ReadTarget();
for (size_t i = 0; i < pixels.size(); ++i) {
ASSERT_EQ(pixels[i], kUnwritten)
<< "texel " << i << " was painted through a 0x0 scissor box: the empty rectangle was "
"substituted with the whole surface, inverting 'clip everything' into 'clip nothing'";
}
}
}
TEST_F(EmptyScissorScenario, IndexedZeroDimensionScissorBoxesClipEveryFragment) {
// The conformance shape: setup4x4Scissor(..., set_zeros=true) writes all 16 boxes
// through glScissorArrayv with zero extents at a 4x4 grid of origins and enables the
// test on every index. Index 0's box is (0, 0, 0, 0) - byte-identical to the
// never-written default - which is precisely the collision the written flag breaks.
// Backends that collapse every index to 0 (DirectGLES today) still pass: index 0's
// box is empty, so the draw is clipped away, which is what the case requires.
FillTarget();
std::vector<GLint> boxes(static_cast<size_t>(kViewportCount) * 4, 0);
for (int i = 0; i < kViewportCount; ++i) {
boxes[static_cast<size_t>(i) * 4 + 0] = (i % kGridSide) * kCellSize;
boxes[static_cast<size_t>(i) * 4 + 1] = (i / kGridSide) * kCellSize;
// width and height stay 0 - that IS the case.
}
glScissorArrayv(0, kViewportCount, boxes.data());
for (int i = 0; i < kViewportCount; ++i) {
glEnablei(GL_SCISSOR_TEST, static_cast<GLuint>(i));
}
glUseProgram(m_program);
glBindVertexArray(m_vao);
glDrawArrays(GL_TRIANGLES, 0, 3);
ASSERT_EQ(glGetError(), GL_NO_ERROR);
const std::vector<GLint> pixels = ReadTarget();
for (size_t i = 0; i < pixels.size(); ++i) {
ASSERT_EQ(pixels[i], kUnwritten) << "texel " << i << " was painted through a zero-extent indexed "
"scissor box";
}
}
} // namespace
} // namespace MGITest
@@ -843,6 +843,21 @@ namespace MobileGL {
stored = box;
stateChanged = true;
}
// "The application has written this rectangle" is a DIFFERENT predicate from "the
// value moved", and the backends need the first one: glScissor(0, 0, 0, 0) as the
// very first scissor call leaves every stored box byte-identical to its
// never-written default, and that call is precisely the one whose meaning a
// backend must stop guessing at (see ScissorBoxWrittenMask).
//
// The transition has to count as a state change for the version too. DirectGLES'
// SyncRenderState early-outs on an unchanged render-state version BEFORE it
// reaches the span memcmp that would otherwise notice the mask, so a version-less
// flag flip would sit in the parameter block and never be pushed. It is a
// once-per-index transition, so the steady state still costs nothing.
if (m_parameters.ScissorBoxWrittenMask != kAllViewportsMask) {
m_parameters.ScissorBoxWrittenMask = kAllViewportsMask;
stateChanged = true;
}
if (stateChanged) ++m_version;
}
@@ -855,9 +870,15 @@ namespace MobileGL {
MOBILEGL_ASSERT(false, "Scissor box index out of range: %u", index);
return;
}
if (m_parameters.ScissorBoxes[index] == box) return;
// See SetScissorBox: a first write is state even when it does not move the value,
// so the unchanged-value early-out may only fire once this index is already
// marked written.
const Uint32 writtenBit = 1u << index;
const Bool alreadyWritten = (m_parameters.ScissorBoxWrittenMask & writtenBit) != 0;
if (alreadyWritten && m_parameters.ScissorBoxes[index] == box) return;
m_parameters.ScissorBoxes[index] = box;
m_parameters.ScissorBoxWrittenMask |= writtenBit;
++m_version;
}
@@ -328,6 +328,18 @@ namespace MobileGL {
// turns it into a real glEnable/glDisable.
Uint32 ScissorTestEnabledMask = 0;
Array<IntVec4, MAX_VIEWPORTS> ScissorBoxes{}; // x, y, width, height
// One bit per viewport, set the first time the application writes that index's scissor
// rectangle - glScissor broadcasts and sets all 16, glScissorIndexed/glScissorArrayv set
// the indices they name. It exists because the RECTANGLE cannot answer "has the
// application spoken?": ScissorBoxes starts all-zero (its spec initial value is the size
// of a window the frontend does not know yet, see the RenderState constructor), and
// glScissor(0, 0, 0, 0) is a legal GL state meaning "the scissor test rejects every
// fragment". A backend that reads an empty rectangle as the never-written sentinel
// therefore INVERTS that request into "accept every fragment"; DirectGLES did exactly
// that and KHR-GL43.viewport_array.scissor_zero_dimension caught it. Deliberately beside
// ScissorBoxes so it shares their tail span (after LogicOp) and DirectGLES' span memcmp
// picks a transition up like any other state.
Uint32 ScissorBoxWrittenMask = 0;
// glEnable(GL_CLIP_DISTANCE0 + i) for i in [0, 8), one bit each. A bitmask rather than
// eight bools because every consumer wants the set, not an individual flag, and because
// the SYNC_CAPABILITY/SET_CAPABILITY macros key off a "<Name>Enabled" field name that
@@ -559,3 +559,76 @@ TEST_F(RenderStateTest, IndexedRectangleQueriesRejectAnOutOfRangeIndex) {
MG_Impl::GLImpl::GetDoublei_v(GL_DEPTH_RANGE, kMaxViewports - 1, doubles);
ExpectSingleGlError(GL_NO_ERROR);
}
// ---------------------------------------------------------------------------------------------
// "Has the application written this scissor rectangle?" - the flag, not the extent
// ---------------------------------------------------------------------------------------------
// glScissor(0, 0, 0, 0) is legal GL and means "the scissor test rejects every fragment", but it
// is byte-identical to the never-written default, whose meaning is the opposite ("the whole
// window", which the frontend cannot spell before a surface exists). DirectGLES resolved the two
// by looking at the EXTENT and so inverted every deliberately empty box into the full surface -
// KHR-GL43.viewport_array.scissor_zero_dimension is exactly that draw, and it came back holding
// the drawn colour where the untouched fill was required.
//
// These drive RenderState directly instead of the GL entry points on purpose: the flag's whole
// content is what it says BEFORE the first scissor call of a context, and this binary shares one
// context across every case in the file, so a pristine object is the only place that state
// still exists by the time these run.
namespace {
constexpr Uint32 kAllViewportsWritten =
RenderStateParameters::MAX_VIEWPORTS >= 32 ? ~0u : (1u << RenderStateParameters::MAX_VIEWPORTS) - 1u;
} // namespace
TEST_F(RenderStateTest, AnEmptyScissorBoxIsDistinguishableFromNeverHavingBeenWritten) {
MG_State::GLState::RenderState state;
EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 0u)
<< "a fresh context has never been given a scissor box, and the all-zero rectangle it "
"starts with must not be mistaken for one";
// Not one stored byte moves here - every box already held (0,0,0,0) - and yet this is the
// call that turns "the frontend does not know the window size" into "reject every fragment".
state.SetScissorBox(IntVec4(0, 0, 0, 0));
EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten);
for (GLuint index = 0; index < kMaxViewports; ++index) {
EXPECT_EQ(state.GetScissorBoxIndexed(index), IntVec4(0, 0, 0, 0)) << "index " << index;
}
}
TEST_F(RenderStateTest, AnIndexedScissorWriteClaimsOnlyItsOwnIndex) {
MG_State::GLState::RenderState state;
state.SetScissorBoxIndexed(5, IntVec4(0, 0, 0, 0));
EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, 1u << 5);
state.SetScissorBoxIndexed(0, IntVec4(0, 0, 0, 0));
EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, (1u << 5) | 1u);
// ARB_viewport_array makes the non-indexed setter a write to every index, so it claims all 16.
state.SetScissorBox(IntVec4(1, 2, 3, 4));
EXPECT_EQ(state.GetAllParameters().ScissorBoxWrittenMask, kAllViewportsWritten);
}
TEST_F(RenderStateTest, TheFirstScissorWriteBumpsTheVersionEvenWhenTheValueDoesNotMove) {
// Load-bearing, and not merely tidy: DirectGLES' SyncRenderState early-outs on an unchanged
// render-state version BEFORE it reaches the span memcmp that would otherwise notice the
// flag. A version-less transition would sit in the parameter block, never be pushed, and the
// empty box would go on rendering as the whole surface.
MG_State::GLState::RenderState state;
const Uint initial = state.GetVersion();
state.SetScissorBox(IntVec4(0, 0, 0, 0));
EXPECT_GT(state.GetVersion(), initial) << "claiming the rectangle is itself a state change";
// Once claimed, a genuinely redundant write stays free - the flag costs one transition, not
// a version bump per call.
const Uint settled = state.GetVersion();
state.SetScissorBox(IntVec4(0, 0, 0, 0));
EXPECT_EQ(state.GetVersion(), settled);
MG_State::GLState::RenderState indexed;
const Uint indexedInitial = indexed.GetVersion();
indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0));
EXPECT_GT(indexed.GetVersion(), indexedInitial);
const Uint indexedSettled = indexed.GetVersion();
indexed.SetScissorBoxIndexed(3, IntVec4(0, 0, 0, 0));
EXPECT_EQ(indexed.GetVersion(), indexedSettled);
}