diff --git a/gpu/command_buffer/service/shared_image/gl_texture_holder.cc b/gpu/command_buffer/service/shared_image/gl_texture_holder.cc index 8c35e9e706544..3046e7147f567 100644 --- a/gpu/command_buffer/service/shared_image/gl_texture_holder.cc +++ b/gpu/command_buffer/service/shared_image/gl_texture_holder.cc @@ -399,9 +399,11 @@ bool GLTextureHolder::ReadbackToMemory(const SkPixmap& pixmap) { } gl::GLApi* api = gl::g_current_gl_context; + GLint prev_framebuffer = 0; + api->glGetIntegervFn(GL_FRAMEBUFFER_BINDING, &prev_framebuffer); GLuint framebuffer; api->glGenFramebuffersEXTFn(1, &framebuffer); - gl::ScopedFramebufferBinder scoped_framebuffer_binder(framebuffer); + api->glBindFramebufferEXTFn(GL_FRAMEBUFFER, framebuffer); // This uses GL_FRAMEBUFFER instead of GL_READ_FRAMEBUFFER as the target for // GLES2 compatibility. api->glFramebufferTexture2DEXTFn(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, @@ -434,6 +436,8 @@ bool GLTextureHolder::ReadbackToMemory(const SkPixmap& pixmap) { } else { DLOG(ERROR) << format_.ToString() << " is not supported by glReadPixels()"; + api->glBindFramebufferEXTFn(GL_FRAMEBUFFER, prev_framebuffer); + api->glDeleteFramebuffersEXTFn(1, &framebuffer); return false; } } @@ -480,6 +484,12 @@ bool GLTextureHolder::ReadbackToMemory(const SkPixmap& pixmap) { pixels); } + // Restore the previous framebuffer binding before deleting the temporary + // FBO. Some drivers retain an internal reference to the previously bound FBO + // across bind transitions; deleting it while bound and then rebinding can + // dereference freed driver state. See the + // ensure_previous_framebuffer_not_deleted driver workaround. + api->glBindFramebufferEXTFn(GL_FRAMEBUFFER, prev_framebuffer); api->glDeleteFramebuffersEXTFn(1, &framebuffer); if (!unpack_buffer.empty()) { diff --git a/gpu/BUILD.gn b/gpu/BUILD.gn index 225e91875e477..f7ce76d314f98 100644 --- a/gpu/BUILD.gn +++ b/gpu/BUILD.gn @@ -492,6 +492,7 @@ test("gpu_unittests") { "command_buffer/service/shared_context_state_unittest.cc", "command_buffer/service/shared_image/compound_image_backing_unittest.cc", "command_buffer/service/shared_image/gl_repack_utils_unittest.cc", + "command_buffer/service/shared_image/gl_texture_holder_unittest.cc", "command_buffer/service/shared_memory_region_wrapper_unittest.cc", "command_buffer/service/sync_point_manager_unittest.cc", "command_buffer/service/task_graph_unittest.cc", diff --git a/gpu/command_buffer/service/shared_image/gl_texture_holder_unittest.cc b/gpu/command_buffer/service/shared_image/gl_texture_holder_unittest.cc new file mode 100644 index 0000000000000..7840145cb3703 --- /dev/null +++ b/gpu/command_buffer/service/shared_image/gl_texture_holder_unittest.cc @@ -0,0 +1,113 @@ +// Copyright 2026 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "gpu/command_buffer/service/shared_image/gl_texture_holder.h" + +#include "base/memory/scoped_refptr.h" +#include "components/viz/common/resources/shared_image_format.h" +#include "gpu/command_buffer/service/shared_image/shared_image_format_service_utils.h" +#include "gpu/command_buffer/service/texture_manager.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/skia/include/core/SkBitmap.h" +#include "ui/gl/gl_bindings.h" +#include "ui/gl/gl_context_stub.h" +#include "ui/gl/gl_mock.h" +#include "ui/gl/gl_surface_stub.h" +#include "ui/gl/test/gl_surface_test_support.h" + +namespace gpu { +namespace { + +using ::testing::_; +using ::testing::InSequence; +using ::testing::Ne; +using ::testing::NiceMock; +using ::testing::Pointee; +using ::testing::Return; +using ::testing::SetArgPointee; + +class GLTextureHolderTest : public testing::Test { + protected: + void SetUp() override { + gl::SetGLGetProcAddressProc(gl::MockGLInterface::GetGLProcAddress); + display_ = gl::GLSurfaceTestSupport::InitializeOneOffWithMockBindings(); + gl_ = std::make_unique>(); + gl::MockGLInterface::SetGLInterface(gl_.get()); + + surface_ = base::MakeRefCounted(); + context_ = base::MakeRefCounted(); + context_->SetGLVersionString("OpenGL ES 2.0"); + context_->SetExtensionsString(""); + context_->Initialize(surface_.get(), {}); + context_->MakeCurrent(surface_.get()); + + ON_CALL(*gl_, CheckFramebufferStatusEXT(_)) + .WillByDefault(Return(GL_FRAMEBUFFER_COMPLETE)); + } + + void TearDown() override { + context_ = nullptr; + surface_ = nullptr; + gl::MockGLInterface::SetGLInterface(nullptr); + gl_.reset(); + gl::GLSurfaceTestSupport::ShutdownGL(display_); + } + + std::unique_ptr> gl_; + scoped_refptr context_; + scoped_refptr surface_; + raw_ptr display_ = nullptr; +}; + +// ReadbackToMemory creates a temporary FBO for glReadPixels. The previous +// framebuffer binding must be restored before the temporary FBO is deleted so +// that the temporary FBO is never the previously bound FBO at the next bind +// transition. Some drivers retain an internal reference to the previously +// bound FBO across bind transitions; see the +// ensure_previous_framebuffer_not_deleted driver workaround. +TEST_F(GLTextureHolderTest, ReadbackToMemoryRestoresFramebufferBeforeDelete) { + constexpr GLuint kTextureId = 11; + constexpr GLuint kTempFboId = 22; + constexpr GLint kPrevFboId = 33; + constexpr gfx::Size kSize(4, 4); + + GLTextureHolder holder(viz::SinglePlaneFormat::kRGBA_8888, kSize, + /*is_passthrough=*/true, + /*progress_reporter=*/nullptr); + GLFormatDesc format_desc; + format_desc.data_format = GL_RGBA; + format_desc.data_type = GL_UNSIGNED_BYTE; + format_desc.target = GL_TEXTURE_2D; + auto texture = base::MakeRefCounted(kTextureId, + GL_TEXTURE_2D); + holder.InitializeWithTexture(format_desc, texture); + + ON_CALL(*gl_, GetIntegerv(GL_FRAMEBUFFER_BINDING, _)) + .WillByDefault(SetArgPointee<1>(kPrevFboId)); + + { + InSequence seq; + EXPECT_CALL(*gl_, GenFramebuffersEXT(1, _)) + .WillOnce(SetArgPointee<1>(kTempFboId)); + EXPECT_CALL(*gl_, BindFramebufferEXT(GL_FRAMEBUFFER, kTempFboId)); + EXPECT_CALL(*gl_, ReadPixels(0, 0, kSize.width(), kSize.height(), GL_RGBA, + GL_UNSIGNED_BYTE, _)); + // The previous framebuffer binding must be restored before the temporary + // FBO is deleted. + EXPECT_CALL(*gl_, BindFramebufferEXT(GL_FRAMEBUFFER, Ne(kTempFboId))); + EXPECT_CALL(*gl_, DeleteFramebuffersEXT(1, Pointee(kTempFboId))); + } + + SkBitmap bitmap; + bitmap.allocPixels(SkImageInfo::Make(kSize.width(), kSize.height(), + kRGBA_8888_SkColorType, + kPremul_SkAlphaType)); + EXPECT_TRUE(holder.ReadbackToMemory(bitmap.pixmap())); + + texture->MarkContextLost(); +} + +} // namespace +} // namespace gpu