Skip to content

Commit

Permalink
Don't enforce attributes for Native GLSurfaces in command decoder
Browse files Browse the repository at this point in the history 
Before this CL validating command decoder did enforce alpha, depth and
stencil attributes for GLSurface that is created from window, e.g
if GLSurface was created with depth or stencil, but client did specify
that it wants no depth, command decoder would enforce that depth isn't
used.

The only code path that still uses native GLSurfaces with command
decoder is WebXR path on Android [1]. We never create real GLSurface
with depth or stencil [2], but unit tests still pretend to use real
surface.

This CL removes the enforcement (so if GLSurface actually has depth
buffer then it can be used) and removes unit tests that verified the
enforcement.

This doesn't change anything for offscreen command buffers.

[1] https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:components/webxr/mailbox_to_surface_bridge_impl.cc;l=248-263;drc=2425fac374aaa944c34b2340b8f53c9c7fc49533;bpv=0;bpt=1
[2] https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:gpu/ipc/service/gles2_command_buffer_stub.cc;l=251-253;drc=2425fac374aaa944c34b2340b8f53c9c7fc49533;bpv=0;bpt=1

Bug: 1445523
Change-Id: I948ede9685ab661272cd8bfcd593830a176466c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575405
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1151431}
  • Loading branch information
@vasilyt
vasilyt authored and Chromium LUCI CQ committed May 31, 2023
1 parent 0eb5676 commit 8a32e2b
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 285 deletions.
11 changes: 3 additions & 8 deletions gpu/command_buffer/service/gles2_cmd_decoder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3880,14 +3880,9 @@ gpu::ContextResult GLES2DecoderImpl::Initialize(
api()->glGetIntegervFn(GL_STENCIL_BITS, &stencil_bits);
}

// This checks if the user requested RGBA and we have RGBA then RGBA. If
// the user requested RGB then RGB. If the user did not specify a
// preference than use whatever we were given. Same for DEPTH and STENCIL.
back_buffer_color_format_ =
(attrib_helper.alpha_size != 0 && alpha_bits > 0) ? GL_RGBA : GL_RGB;
back_buffer_has_depth_ = attrib_helper.depth_size != 0 && depth_bits > 0;
back_buffer_has_stencil_ =
attrib_helper.stencil_size != 0 && stencil_bits > 0;
back_buffer_color_format_ = alpha_bits > 0 ? GL_RGBA : GL_RGB;
back_buffer_has_depth_ = depth_bits > 0;
back_buffer_has_stencil_ = stencil_bits > 0;
num_stencil_bits_ = stencil_bits;
} else {
num_stencil_bits_ = attrib_helper.stencil_size;
Expand Down
7 changes: 1 addition & 6 deletions gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,9 +498,6 @@ ContextResult GLES2DecoderTestBase::MaybeInitDecoderWithWorkarounds(
ClearSharedMemory();

ContextCreationAttribs attribs;
attribs.alpha_size = normalized_init.request_alpha ? 8 : 0;
attribs.depth_size = normalized_init.request_depth ? 24 : 0;
attribs.stencil_size = normalized_init.request_stencil ? 8 : 0;
attribs.lose_context_when_out_of_memory =
normalized_init.lose_context_when_out_of_memory;
attribs.context_type = init.context_type;
Expand Down Expand Up @@ -2429,7 +2426,6 @@ void GLES2DecoderPassthroughTestBase::SetUp() {
ui::OzonePlatform::InitializeForGPU(params);
#endif

context_creation_attribs_.offscreen_framebuffer_size = gfx::Size(4, 4);
context_creation_attribs_.bind_generates_resource = true;

gl::init::InitializeStaticGLBindingsImplementation(
Expand All @@ -2451,8 +2447,7 @@ void GLES2DecoderPassthroughTestBase::SetUp() {
nullptr /* progress_reporter */, GpuFeatureInfo(), &discardable_manager_,
&passthrough_discardable_manager_, &shared_image_manager_);

surface_ = gl::init::CreateOffscreenGLSurface(
display_, context_creation_attribs_.offscreen_framebuffer_size);
surface_ = gl::init::CreateOffscreenGLSurface(display_, gfx::Size(4, 4));
context_ = gl::init::CreateGLContext(
nullptr, surface_.get(),
GenerateGLContextAttribs(context_creation_attribs_, group_.get()));
Expand Down
102 changes: 0 additions & 102 deletions gpu/command_buffer/service/gles2_cmd_decoder_unittest_drawing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,57 +431,6 @@ TEST_P(GLES2DecoderManualInitTest, DepthEnableWithDepth) {
EXPECT_EQ(1, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, DepthEnableWithoutRequestedDepth) {
InitState init;
init.has_depth = true;
init.bind_generates_resource = true;
InitDecoder(init);

cmds::Enable cmd;
cmd.Init(GL_DEPTH_TEST);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
EXPECT_EQ(GL_NO_ERROR, GetGLError());

SetupDefaultProgram();
SetupTexture();
AddExpectationsForSimulatedAttrib0(kNumVertices, 0);
SetupExpectationsForApplyingDirtyState(true, // Framebuffer is RGB
false, // Framebuffer has depth
false, // Framebuffer has stencil
0x1110, // color bits
false, // depth mask
false, // depth enabled
0, // front stencil mask
0, // back stencil mask
false); // stencil enabled

EXPECT_CALL(*gl_, DrawArrays(GL_TRIANGLES, 0, kNumVertices))
.Times(1)
.RetiresOnSaturation();
cmds::DrawArrays draw_cmd;
draw_cmd.Init(GL_TRIANGLES, 0, kNumVertices);
EXPECT_EQ(error::kNoError, ExecuteCmd(draw_cmd));
EXPECT_EQ(GL_NO_ERROR, GetGLError());

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_DEPTH_TEST, _))
.Times(0)
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_DEPTH_TEST, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_DEPTH_TEST),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(1, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, StencilEnableWithStencil) {
InitState init;
init.has_stencil = true;
Expand Down Expand Up @@ -535,57 +484,6 @@ TEST_P(GLES2DecoderManualInitTest, StencilEnableWithStencil) {
EXPECT_EQ(1, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, StencilEnableWithoutRequestedStencil) {
InitState init;
init.has_stencil = true;
init.bind_generates_resource = true;
InitDecoder(init);

cmds::Enable cmd;
cmd.Init(GL_STENCIL_TEST);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
EXPECT_EQ(GL_NO_ERROR, GetGLError());

SetupDefaultProgram();
SetupTexture();
AddExpectationsForSimulatedAttrib0(kNumVertices, 0);
SetupExpectationsForApplyingDirtyState(true, // Framebuffer is RGB
false, // Framebuffer has depth
false, // Framebuffer has stencil
0x1110, // color bits
false, // depth mask
false, // depth enabled
0, // front stencil mask
0, // back stencil mask
false); // stencil enabled

EXPECT_CALL(*gl_, DrawArrays(GL_TRIANGLES, 0, kNumVertices))
.Times(1)
.RetiresOnSaturation();
cmds::DrawArrays draw_cmd;
draw_cmd.Init(GL_TRIANGLES, 0, kNumVertices);
EXPECT_EQ(error::kNoError, ExecuteCmd(draw_cmd));
EXPECT_EQ(GL_NO_ERROR, GetGLError());

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_STENCIL_TEST, _))
.Times(0)
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_STENCIL_TEST, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_STENCIL_TEST),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(1, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, CachedColorMask) {
InitState init;
init.has_alpha = true;
Expand Down
166 changes: 1 addition & 165 deletions gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1640,130 +1640,6 @@ TEST_P(GLES2DecoderManualInitTest, ActualAlphaMatchesRequestedAlpha) {
EXPECT_EQ(8, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, ActualAlphaDoesNotMatchRequestedAlpha) {
InitState init;
init.has_alpha = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_ALPHA_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_ALPHA_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(0, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, ActualDepthMatchesRequestedDepth) {
InitState init;
init.has_depth = true;
init.request_depth = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_DEPTH_BITS, _))
.WillOnce(SetArgPointee<1>(24))
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_DEPTH_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_DEPTH_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(24, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, ActualDepthDoesNotMatchRequestedDepth) {
InitState init;
init.has_depth = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_DEPTH_BITS, _))
.WillOnce(SetArgPointee<1>(24))
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_DEPTH_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_DEPTH_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(0, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, ActualStencilMatchesRequestedStencil) {
InitState init;
init.has_stencil = true;
init.request_stencil = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_STENCIL_BITS, _))
.WillOnce(SetArgPointee<1>(8))
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_STENCIL_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_STENCIL_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(8, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, ActualStencilDoesNotMatchRequestedStencil) {
InitState init;
init.has_stencil = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
EXPECT_CALL(*gl_, GetIntegerv(GL_STENCIL_BITS, _))
.WillOnce(SetArgPointee<1>(8))
.RetiresOnSaturation();
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_STENCIL_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_STENCIL_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(0, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, PackedDepthStencilReportsCorrectValues) {
InitState init;
init.extensions = "GL_OES_packed_depth_stencil";
Expand Down Expand Up @@ -1806,47 +1682,6 @@ TEST_P(GLES2DecoderManualInitTest, PackedDepthStencilReportsCorrectValues) {
EXPECT_EQ(24, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, PackedDepthStencilNoRequestedStencil) {
InitState init;
init.extensions = "GL_OES_packed_depth_stencil";
init.gl_version = "OpenGL ES 2.0";
init.has_depth = true;
init.has_stencil = true;
init.request_depth = true;
init.bind_generates_resource = true;
InitDecoder(init);

EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation();
auto* result =
static_cast<cmds::GetIntegerv::Result*>(shared_memory_address_);
result->size = 0;
cmds::GetIntegerv cmd2;
cmd2.Init(GL_STENCIL_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_CALL(*gl_, GetIntegerv(GL_STENCIL_BITS, _))
.WillOnce(SetArgPointee<1>(8))
.RetiresOnSaturation();
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_STENCIL_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(0, result->GetData()[0]);
result->size = 0;
cmd2.Init(GL_DEPTH_BITS, shared_memory_id_, shared_memory_offset_);
EXPECT_CALL(*gl_, GetIntegerv(GL_DEPTH_BITS, _))
.WillOnce(SetArgPointee<1>(24))
.RetiresOnSaturation();
EXPECT_EQ(error::kNoError, ExecuteCmd(cmd2));
EXPECT_EQ(decoder_->GetGLES2Util()->GLGetNumValuesReturned(GL_DEPTH_BITS),
result->GetNumResults());
EXPECT_EQ(GL_NO_ERROR, GetGLError());
EXPECT_EQ(24, result->GetData()[0]);
}

TEST_P(GLES2DecoderManualInitTest, PackedDepthStencilRenderbufferDepth) {
InitState init;
init.extensions = "GL_OES_packed_depth_stencil";
Expand Down Expand Up @@ -3529,6 +3364,7 @@ TEST_P(GLES2DecoderManualInitTest,
init.extensions = "GL_EXT_discard_framebuffer";
init.gl_version = "OpenGL ES 2.0";
init.has_alpha = true;
init.request_alpha = true;
init.bind_generates_resource = true;
InitDecoder(init);

Expand Down
13 changes: 9 additions & 4 deletions gpu/gles2_conform_support/egl/context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,16 @@ bool Context::CreateService(gl::GLSurface* gl_surface) {

gl_context->MakeCurrent(gl_surface);

gpu::ContextCreationAttribs helper;
config_->GetAttrib(EGL_ALPHA_SIZE, &helper.alpha_size);
config_->GetAttrib(EGL_DEPTH_SIZE, &helper.depth_size);
config_->GetAttrib(EGL_STENCIL_SIZE, &helper.stencil_size);
EGLint alpha_size, depth_size, stencil_size;
config_->GetAttrib(EGL_ALPHA_SIZE, &alpha_size);
config_->GetAttrib(EGL_DEPTH_SIZE, &depth_size);
config_->GetAttrib(EGL_STENCIL_SIZE, &stencil_size);

CHECK_EQ(alpha_size, 0);
CHECK_EQ(depth_size, 0);
CHECK_EQ(stencil_size, 0);

gpu::ContextCreationAttribs helper;
helper.buffer_preserved = false;
helper.bind_generates_resource = kBindGeneratesResources;
helper.fail_if_major_perf_caveat = false;
Expand Down

0 comments on commit 8a32e2b

Please sign in to comment.