Skip to content

Commit a642edd

Browse files
fix(impeller): fix params to glDiscardFrameBufferEXT (flutter#175589)
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> Fix parameters to [glDiscardFramebufferEXT](https://registry.khronos.org/OpenGL/extensions/EXT/EXT_discard_framebuffer.txt) when an application-defined framebuffer is bound. The `is_default_fbo` parameter is set to `true` if `color_gles.IsWrapped()` is true, however according to the GLES spec, a non-default framebuffer is bound if `glBindFramebuffer(GL_FRAMEBUFFER, framebuffer)` is called with `framebuffer != 0`: > _The namespace for framebuffer objects is the unsigned integers, with zero re- served by OpenGL ES to refer to the default framebuffer. A framebuffer object is created by binding an unused name to the target FRAMEBUFFER._ To reproduce this issue, run a flutter application with `--enable-impeller` on a platform where `glDiscardFramebufferEXT` is available. The following error is reported: ``` [FATAL:flutter/impeller/renderer/backend/gles/proc_table_gles.h(43)] Fatal GL Error GL_INVALID_ENUM(1280) encountered on call to glDiscardFramebufferEXT ``` *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* closes flutter#175588 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent f863e87 commit a642edd

File tree

5 files changed

+150
-6
lines changed

5 files changed

+150
-6
lines changed

engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ impeller_component("gles_unittests") {
1717
"blit_command_gles_unittests.cc",
1818
"buffer_bindings_gles_unittests.cc",
1919
"device_buffer_gles_unittests.cc",
20+
"render_pass_gles_unittests.cc",
2021
"test/capabilities_unittests.cc",
2122
"test/formats_gles_unittests.cc",
2223
"test/gpu_tracer_gles_unittests.cc",

engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,10 +213,10 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) {
213213
#endif // IMPELLER_DEBUG
214214

215215
TextureGLES& color_gles = TextureGLES::Cast(*pass_data.color_attachment);
216-
const bool is_default_fbo = color_gles.IsWrapped();
216+
const bool is_wrapped_fbo = color_gles.IsWrapped();
217217

218218
std::optional<GLuint> fbo = 0;
219-
if (is_default_fbo) {
219+
if (is_wrapped_fbo) {
220220
if (color_gles.GetFBO().has_value()) {
221221
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
222222
gl.BindFramebuffer(GL_FRAMEBUFFER, *color_gles.GetFBO());
@@ -524,7 +524,7 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) {
524524

525525
if (pass_data.resolve_attachment &&
526526
!gl.GetCapabilities()->SupportsImplicitResolvingMSAA() &&
527-
!is_default_fbo) {
527+
!is_wrapped_fbo) {
528528
FML_DCHECK(pass_data.resolve_attachment != pass_data.color_attachment);
529529
// Perform multisample resolve via blit.
530530
// Create and bind a resolve FBO.
@@ -570,6 +570,10 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) {
570570
gl.BindFramebuffer(GL_FRAMEBUFFER, fbo.value());
571571
}
572572

573+
GLint framebuffer_id = 0;
574+
gl.GetIntegerv(GL_FRAMEBUFFER_BINDING, &framebuffer_id);
575+
const bool is_default_fbo = framebuffer_id == 0;
576+
573577
if (gl.DiscardFramebufferEXT.IsAvailable()) {
574578
std::array<GLenum, 3> attachments;
575579
size_t attachment_count = 0;
@@ -583,6 +587,7 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) {
583587
attachments[attachment_count++] =
584588
(is_default_fbo ? GL_COLOR_EXT : GL_COLOR_ATTACHMENT0);
585589
}
590+
586591
if (pass_data.discard_depth_attachment && angle_safe) {
587592
attachments[attachment_count++] =
588593
(is_default_fbo ? GL_DEPTH_EXT : GL_DEPTH_ATTACHMENT);
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include <memory>
6+
#include "flutter/testing/testing.h" // IWYU pragma: keep
7+
#include "gmock/gmock.h"
8+
#include "gtest/gtest.h"
9+
#include "impeller/core/formats.h"
10+
#include "impeller/renderer/backend/gles/command_buffer_gles.h"
11+
#include "impeller/renderer/backend/gles/context_gles.h"
12+
#include "impeller/renderer/backend/gles/proc_table_gles.h"
13+
#include "impeller/renderer/backend/gles/reactor_gles.h"
14+
#include "impeller/renderer/backend/gles/test/mock_gles.h"
15+
#include "impeller/renderer/backend/gles/texture_gles.h"
16+
#include "impeller/renderer/context.h"
17+
#include "impeller/renderer/render_pass.h"
18+
#include "impeller/renderer/render_target.h"
19+
20+
namespace impeller {
21+
namespace testing {
22+
23+
using ::testing::_;
24+
using ::testing::Args;
25+
using ::testing::ElementsAreArray;
26+
using ::testing::NiceMock;
27+
using ::testing::Return;
28+
using ::testing::SetArgPointee;
29+
using ::testing::TestWithParam;
30+
31+
class TestReactorGLES : public ReactorGLES {
32+
public:
33+
TestReactorGLES()
34+
: ReactorGLES(std::make_unique<ProcTableGLES>(kMockResolverGLES)) {}
35+
36+
~TestReactorGLES() = default;
37+
};
38+
39+
class MockWorker final : public ReactorGLES::Worker {
40+
public:
41+
MockWorker() = default;
42+
43+
// |ReactorGLES::Worker|
44+
bool CanReactorReactOnCurrentThreadNow(
45+
const ReactorGLES& reactor) const override {
46+
return true;
47+
}
48+
};
49+
50+
struct DiscardFrameBufferParams {
51+
GLuint frame_buffer_id;
52+
std::array<GLenum, 3> expected_attachments;
53+
};
54+
55+
class RenderPassGLESWithDiscardFrameBufferExtTest
56+
: public TestWithParam<DiscardFrameBufferParams> {};
57+
58+
namespace {
59+
std::shared_ptr<ContextGLES> CreateFakeGLESContext() {
60+
auto dummy_gl_procs = std::make_unique<ProcTableGLES>(kMockResolverGLES);
61+
auto dummy_shader_library = std::vector<std::shared_ptr<fml::Mapping>>{};
62+
auto flags = Flags{};
63+
return ContextGLES::Create(flags, std::move(dummy_gl_procs),
64+
dummy_shader_library, false);
65+
}
66+
} // namespace
67+
68+
TEST_P(RenderPassGLESWithDiscardFrameBufferExtTest, DiscardFramebufferExt) {
69+
auto mock_gl_impl = std::make_unique<NiceMock<MockGLESImpl>>();
70+
auto& mock_gl_impl_ref = *mock_gl_impl;
71+
auto mock_gl =
72+
MockGLES::Init(std::move(mock_gl_impl), {{"GL_EXT_discard_framebuffer"}});
73+
74+
auto context = CreateFakeGLESContext();
75+
auto dummy_worker = std::make_shared<MockWorker>();
76+
context->AddReactorWorker(dummy_worker);
77+
auto reactor = context->GetReactor();
78+
79+
const auto command_buffer =
80+
std::static_pointer_cast<Context>(context)->CreateCommandBuffer();
81+
auto render_target = RenderTarget{};
82+
const auto description = TextureDescriptor{
83+
.format = PixelFormat::kR8G8B8A8UNormInt, .size = {10, 10}};
84+
85+
const auto& test_params = GetParam();
86+
auto framebuffer_texture =
87+
TextureGLES::WrapFBO(reactor, description, test_params.frame_buffer_id);
88+
89+
auto color_attachment = ColorAttachment{Attachment{
90+
.texture = framebuffer_texture, .store_action = StoreAction::kDontCare}};
91+
render_target.SetColorAttachment(color_attachment, 0);
92+
const auto render_pass = command_buffer->CreateRenderPass(render_target);
93+
94+
EXPECT_CALL(mock_gl_impl_ref, GetIntegerv(GL_FRAMEBUFFER_BINDING, _))
95+
.WillOnce(SetArgPointee<1>(test_params.frame_buffer_id));
96+
97+
EXPECT_CALL(mock_gl_impl_ref, DiscardFramebufferEXT(GL_FRAMEBUFFER, _, _))
98+
.With(Args<2, 1>(ElementsAreArray(test_params.expected_attachments)))
99+
.Times(1);
100+
ASSERT_TRUE(render_pass->EncodeCommands());
101+
ASSERT_TRUE(reactor->React());
102+
}
103+
104+
INSTANTIATE_TEST_SUITE_P(
105+
FrameBufferObject,
106+
RenderPassGLESWithDiscardFrameBufferExtTest,
107+
::testing::ValuesIn(std::vector<DiscardFrameBufferParams>{
108+
{.frame_buffer_id = 0,
109+
.expected_attachments = {GL_COLOR_EXT, GL_DEPTH_EXT, GL_STENCIL_EXT}},
110+
{.frame_buffer_id = 1,
111+
.expected_attachments = {GL_COLOR_ATTACHMENT0, GL_DEPTH_ATTACHMENT,
112+
GL_STENCIL_ATTACHMENT}}}),
113+
[](const ::testing::TestParamInfo<DiscardFrameBufferParams>& info) {
114+
return (info.param.frame_buffer_id == 0) ? "Default" : "NonDefault";
115+
});
116+
117+
} // namespace testing
118+
} // namespace impeller

engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ void mockGetIntegerv(GLenum name, int* value) {
103103
*value = 4096;
104104
break;
105105
default:
106-
*value = 0;
106+
CallMockMethod(&IMockGLESImpl::GetIntegerv, name, value);
107107
break;
108108
}
109109
}
@@ -246,8 +246,15 @@ void mockReadPixels(GLint x,
246246
type, data);
247247
}
248248

249-
static_assert(CheckSameSignature<decltype(mockReadPixels), //
250-
decltype(glReadPixels)>::value);
249+
void mockDiscardFramebufferEXT(GLenum target,
250+
GLsizei numAttachments,
251+
const GLenum* attachments) {
252+
return CallMockMethod(&IMockGLESImpl::DiscardFramebufferEXT, target,
253+
numAttachments, attachments);
254+
}
255+
256+
static_assert(CheckSameSignature<decltype(mockDiscardFramebufferEXT), //
257+
decltype(glDiscardFramebufferEXT)>::value);
251258

252259
// static
253260
std::shared_ptr<MockGLES> MockGLES::Init(
@@ -322,6 +329,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) {
322329
return reinterpret_cast<void*>(mockGenFramebuffers);
323330
} else if (strcmp(name, "glBindFramebuffer") == 0) {
324331
return reinterpret_cast<void*>(mockBindFramebuffer);
332+
} else if (strcmp(name, "glDiscardFramebufferEXT") == 0) {
333+
return reinterpret_cast<void*>(mockDiscardFramebufferEXT);
325334
} else {
326335
return reinterpret_cast<void*>(&doNothing);
327336
}

engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ class IMockGLESImpl {
6666
virtual void GenBuffers(GLsizei n, GLuint* buffers) {}
6767
virtual void DeleteBuffers(GLsizei n, const GLuint* buffers) {}
6868
virtual GLboolean IsTexture(GLuint texture) { return true; }
69+
virtual void DiscardFramebufferEXT(GLenum target,
70+
GLsizei numAttachments,
71+
const GLenum* attachments) {};
72+
virtual void GetIntegerv(GLenum name, GLint* attachments) {};
6973
};
7074

7175
class MockGLESImpl : public IMockGLESImpl {
@@ -149,6 +153,13 @@ class MockGLESImpl : public IMockGLESImpl {
149153
(GLsizei n, const GLuint* buffers),
150154
(override));
151155
MOCK_METHOD(GLboolean, IsTexture, (GLuint texture), (override));
156+
MOCK_METHOD(void,
157+
DiscardFramebufferEXT,
158+
(GLenum target,
159+
GLsizei numAttachments,
160+
const GLenum* attachments),
161+
(override));
162+
MOCK_METHOD(void, GetIntegerv, (GLenum name, GLint* value), (override));
152163
};
153164

154165
/// @brief Provides a mocked version of the |ProcTableGLES| class.

0 commit comments

Comments
 (0)