Skip to content

Commit b68d190

Browse files
gaaclarkemboetger
gaaclarke
authored and
mboetger
committed
Bind GL_FRAMEBUFFER with glReadPixels for gles2 compatibility (flutter#174668)
fixes flutter#164459 ## 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 38a184c commit b68d190

File tree

5 files changed

+223
-4
lines changed

5 files changed

+223
-4
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
@@ -14,6 +14,7 @@ config("gles_config") {
1414
impeller_component("gles_unittests") {
1515
testonly = true
1616
sources = [
17+
"blit_command_gles_unittests.cc",
1718
"buffer_bindings_gles_unittests.cc",
1819
"device_buffer_gles_unittests.cc",
1920
"test/capabilities_unittests.cc",

‎engine/src/flutter/impeller/renderer/backend/gles/blit_command_gles.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,10 +344,10 @@ bool BlitCopyTextureToBufferCommandGLES::Encode(
344344

345345
GLuint read_fbo = GL_NONE;
346346
fml::ScopedCleanupClosure delete_fbos(
347-
[&gl, &read_fbo]() { DeleteFBO(gl, read_fbo, GL_READ_FRAMEBUFFER); });
347+
[&gl, &read_fbo]() { DeleteFBO(gl, read_fbo, GL_FRAMEBUFFER); });
348348

349349
{
350-
auto read = ConfigureFBO(gl, source, GL_READ_FRAMEBUFFER);
350+
auto read = ConfigureFBO(gl, source, GL_FRAMEBUFFER);
351351
if (!read.has_value()) {
352352
return false;
353353
}

‎engine/src/flutter/impeller/renderer/backend/gles/blit_command_gles_unittests.cc‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
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 "flutter/testing/testing.h" // IWYU pragma: keep
6+
#include "gtest/gtest.h"
7+
#include "impeller/renderer/backend/gles/blit_command_gles.h"
8+
#include "impeller/renderer/backend/gles/device_buffer_gles.h"
9+
#include "impeller/renderer/backend/gles/test/mock_gles.h"
10+
#include "impeller/renderer/backend/gles/texture_gles.h"
11+
12+
namespace impeller {
13+
namespace testing {
14+
15+
using ::testing::_;
16+
using ::testing::Return;
17+
18+
class TestReactorGLES : public ReactorGLES {
19+
public:
20+
TestReactorGLES()
21+
: ReactorGLES(std::make_unique<ProcTableGLES>(kMockResolverGLES)) {}
22+
23+
~TestReactorGLES() = default;
24+
};
25+
26+
class MockWorker final : public ReactorGLES::Worker {
27+
public:
28+
MockWorker() = default;
29+
30+
// |ReactorGLES::Worker|
31+
bool CanReactorReactOnCurrentThreadNow(
32+
const ReactorGLES& reactor) const override {
33+
return true;
34+
}
35+
};
36+
37+
// This test makes sure we bind to GL_FRAMEBUFFER so that it's compatible for
38+
// OpenGLES 2 and OpenGLES 3.
39+
TEST(BlitCommandGLESTest, BlitCopyTextureToBufferCommandGLESBindsFramebuffer) {
40+
auto mock_gles_impl = std::make_unique<MockGLESImpl>();
41+
auto& mock_gles_impl_ref = *mock_gles_impl;
42+
43+
EXPECT_CALL(mock_gles_impl_ref, GenFramebuffers(1, _))
44+
.WillOnce(::testing::SetArgPointee<1>(3));
45+
EXPECT_CALL(mock_gles_impl_ref, GenTextures(1, _))
46+
.WillOnce(::testing::SetArgPointee<1>(1));
47+
EXPECT_CALL(mock_gles_impl_ref, BindFramebuffer(GL_FRAMEBUFFER, 3)).Times(1);
48+
EXPECT_CALL(mock_gles_impl_ref, CheckFramebufferStatus(GL_FRAMEBUFFER))
49+
.WillOnce(Return(GL_FRAMEBUFFER_COMPLETE));
50+
EXPECT_CALL(mock_gles_impl_ref, ReadPixels(_, _, _, _, _, _, _)).Times(1);
51+
EXPECT_CALL(mock_gles_impl_ref, BindFramebuffer(GL_FRAMEBUFFER, 0)).Times(1);
52+
53+
std::shared_ptr<MockGLES> mock_gl = MockGLES::Init(std::move(mock_gles_impl));
54+
auto reactor = std::make_shared<TestReactorGLES>();
55+
auto worker = std::make_shared<MockWorker>();
56+
reactor->AddWorker(worker);
57+
58+
// Create source texture.
59+
TextureDescriptor src_tex_desc;
60+
src_tex_desc.format = PixelFormat::kR8G8B8A8UNormInt;
61+
src_tex_desc.size = {10, 10};
62+
src_tex_desc.usage =
63+
static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);
64+
auto source_texture = std::make_shared<TextureGLES>(reactor, src_tex_desc);
65+
// Avoids the flip which would crash.
66+
source_texture->SetCoordinateSystem(TextureCoordinateSystem::kUploadFromHost);
67+
68+
// Create destination buffer.
69+
DeviceBufferDescriptor dest_buffer_desc;
70+
dest_buffer_desc.size = 10 * 10 * 4;
71+
dest_buffer_desc.storage_mode = StorageMode::kHostVisible;
72+
auto allocation = std::make_shared<Allocation>();
73+
ASSERT_TRUE(allocation->Truncate(Bytes(dest_buffer_desc.size)));
74+
auto dest_buffer =
75+
std::make_shared<DeviceBufferGLES>(dest_buffer_desc, reactor, allocation);
76+
77+
ASSERT_TRUE(reactor->React());
78+
79+
BlitCopyTextureToBufferCommandGLES command;
80+
command.source = source_texture;
81+
command.destination = dest_buffer;
82+
command.source_region =
83+
IRect::MakeSize(source_texture->GetTextureDescriptor().size);
84+
command.label = "TestBlit";
85+
86+
EXPECT_TRUE(command.Encode(*reactor));
87+
88+
source_texture.reset();
89+
dest_buffer.reset();
90+
91+
ASSERT_TRUE(reactor->React());
92+
}
93+
94+
} // namespace testing
95+
} // namespace impeller

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

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ void mockGetIntegerv(GLenum name, int* value) {
9999
case GL_MAX_LABEL_LENGTH_KHR:
100100
*value = 64;
101101
break;
102+
case GL_MAX_TEXTURE_SIZE:
103+
*value = 4096;
104+
break;
102105
default:
103106
*value = 0;
104107
break;
@@ -208,8 +211,43 @@ GLboolean mockIsTexture(GLuint texture) {
208211
return CallMockMethod(&IMockGLESImpl::IsTexture, texture);
209212
}
210213

211-
static_assert(CheckSameSignature<decltype(mockGenTextures), //
212-
decltype(glGenTextures)>::value);
214+
static_assert(CheckSameSignature<decltype(mockIsTexture), //
215+
decltype(glIsTexture)>::value);
216+
217+
GLenum mockCheckFramebufferStatus(GLenum target) {
218+
return CallMockMethod(&IMockGLESImpl::CheckFramebufferStatus, target);
219+
}
220+
221+
static_assert(CheckSameSignature<decltype(mockCheckFramebufferStatus), //
222+
decltype(glCheckFramebufferStatus)>::value);
223+
224+
void mockGenFramebuffers(GLsizei n, GLuint* ids) {
225+
return CallMockMethod(&IMockGLESImpl::GenFramebuffers, n, ids);
226+
}
227+
228+
static_assert(CheckSameSignature<decltype(mockGenFramebuffers), //
229+
decltype(glGenFramebuffers)>::value);
230+
231+
void mockBindFramebuffer(GLenum target, GLuint framebuffer) {
232+
return CallMockMethod(&IMockGLESImpl::BindFramebuffer, target, framebuffer);
233+
}
234+
235+
static_assert(CheckSameSignature<decltype(mockBindFramebuffer), //
236+
decltype(glBindFramebuffer)>::value);
237+
238+
void mockReadPixels(GLint x,
239+
GLint y,
240+
GLsizei width,
241+
GLsizei height,
242+
GLenum format,
243+
GLenum type,
244+
void* data) {
245+
return CallMockMethod(&IMockGLESImpl::ReadPixels, x, y, width, height, format,
246+
type, data);
247+
}
248+
249+
static_assert(CheckSameSignature<decltype(mockReadPixels), //
250+
decltype(glReadPixels)>::value);
213251

214252
// static
215253
std::shared_ptr<MockGLES> MockGLES::Init(
@@ -276,6 +314,14 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) {
276314
return reinterpret_cast<void*>(mockGenBuffers);
277315
} else if (strcmp(name, "glIsTexture") == 0) {
278316
return reinterpret_cast<void*>(mockIsTexture);
317+
} else if (strcmp(name, "glCheckFramebufferStatus") == 0) {
318+
return reinterpret_cast<void*>(mockCheckFramebufferStatus);
319+
} else if (strcmp(name, "glReadPixels") == 0) {
320+
return reinterpret_cast<void*>(mockReadPixels);
321+
} else if (strcmp(name, "glGenFramebuffers") == 0) {
322+
return reinterpret_cast<void*>(mockGenFramebuffers);
323+
} else if (strcmp(name, "glBindFramebuffer") == 0) {
324+
return reinterpret_cast<void*>(mockBindFramebuffer);
279325
} else {
280326
return reinterpret_cast<void*>(&doNothing);
281327
}

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

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,34 @@ class IMockGLESImpl {
2121
virtual ~IMockGLESImpl() = default;
2222
virtual void DeleteTextures(GLsizei size, const GLuint* queries) {}
2323
virtual void GenTextures(GLsizei n, GLuint* textures) {}
24+
virtual void BindTexture(GLenum target, GLuint texture) {}
25+
virtual void TexImage2D(GLenum target,
26+
GLint level,
27+
GLint internalformat,
28+
GLsizei width,
29+
GLsizei height,
30+
GLint border,
31+
GLenum format,
32+
GLenum type,
33+
const void* pixels) {}
34+
virtual void GenFramebuffers(GLsizei n, GLuint* framebuffers) {}
35+
virtual void BindFramebuffer(GLenum target, GLuint framebuffer) {}
36+
virtual void FramebufferTexture2D(GLenum target,
37+
GLenum attachment,
38+
GLenum textarget,
39+
GLuint texture,
40+
GLint level) {}
41+
virtual GLenum CheckFramebufferStatus(GLenum target) {
42+
return GL_FRAMEBUFFER_COMPLETE;
43+
}
44+
virtual void ReadPixels(GLint x,
45+
GLint y,
46+
GLsizei width,
47+
GLsizei height,
48+
GLenum format,
49+
GLenum type,
50+
void* pixels) {}
51+
virtual void DeleteFramebuffers(GLsizei n, const GLuint* framebuffers) {}
2452
virtual void ObjectLabelKHR(GLenum identifier,
2553
GLuint name,
2654
GLsizei length,
@@ -36,6 +64,7 @@ class IMockGLESImpl {
3664
GLuint64* result) {}
3765
virtual void DeleteQueriesEXT(GLsizei size, const GLuint* queries) {}
3866
virtual void GenBuffers(GLsizei n, GLuint* buffers) {}
67+
virtual void DeleteBuffers(GLsizei n, const GLuint* buffers) {}
3968
virtual GLboolean IsTexture(GLuint texture) { return true; }
4069
};
4170

@@ -46,6 +75,50 @@ class MockGLESImpl : public IMockGLESImpl {
4675
(GLsizei size, const GLuint* queries),
4776
(override));
4877
MOCK_METHOD(void, GenTextures, (GLsizei n, GLuint* textures), (override));
78+
MOCK_METHOD(void, BindTexture, (GLenum target, GLuint texture), (override));
79+
MOCK_METHOD(void,
80+
TexImage2D,
81+
(GLenum target,
82+
GLint level,
83+
GLint internalformat,
84+
GLsizei width,
85+
GLsizei height,
86+
GLint border,
87+
GLenum format,
88+
GLenum type,
89+
const void* pixels),
90+
(override));
91+
MOCK_METHOD(void,
92+
GenFramebuffers,
93+
(GLsizei n, GLuint* framebuffers),
94+
(override));
95+
MOCK_METHOD(void,
96+
BindFramebuffer,
97+
(GLenum target, GLuint framebuffer),
98+
(override));
99+
MOCK_METHOD(void,
100+
FramebufferTexture2D,
101+
(GLenum target,
102+
GLenum attachment,
103+
GLenum textarget,
104+
GLuint texture,
105+
GLint level),
106+
(override));
107+
MOCK_METHOD(GLenum, CheckFramebufferStatus, (GLenum target), (override));
108+
MOCK_METHOD(void,
109+
ReadPixels,
110+
(GLint x,
111+
GLint y,
112+
GLsizei width,
113+
GLsizei height,
114+
GLenum format,
115+
GLenum type,
116+
void* pixels),
117+
(override));
118+
MOCK_METHOD(void,
119+
DeleteFramebuffers,
120+
(GLsizei n, const GLuint* framebuffers),
121+
(override));
49122
MOCK_METHOD(
50123
void,
51124
ObjectLabelKHR,
@@ -71,6 +144,10 @@ class MockGLESImpl : public IMockGLESImpl {
71144
(GLsizei size, const GLuint* queries),
72145
(override));
73146
MOCK_METHOD(void, GenBuffers, (GLsizei n, GLuint* buffers), (override));
147+
MOCK_METHOD(void,
148+
DeleteBuffers,
149+
(GLsizei n, const GLuint* buffers),
150+
(override));
74151
MOCK_METHOD(GLboolean, IsTexture, (GLuint texture), (override));
75152
};
76153

0 commit comments

Comments
 (0)