Mir

Merge lp:~kdub/mir/fix-1663062-step2 into lp:mir

Proposed by Kevin DuBois on 2017-02-14
Status: Merged
Approved by: Daniel van Vugt on 2017-02-21
Approved revision: 4033
Merged at revision: 4044
Proposed branch: lp:~kdub/mir/fix-1663062-step2
Merge into: lp:mir
Diff against target: 303 lines (+102/-31)
8 files modified
include/platform/mir/graphics/gl_format.h (+31/-0)
src/platforms/common/server/shm_buffer.cpp (+5/-7)
src/server/CMakeLists.txt (+1/-0)
src/server/graphics/nested/CMakeLists.txt (+1/-0)
src/server/graphics/nested/buffer.cpp (+36/-14)
src/server/graphics/nested/mir_client_host_connection.cpp (+3/-6)
src/server/graphics/nested/platform.cpp (+1/-4)
tests/unit-tests/platforms/nested/test_buffer.cpp (+24/-0)
To merge this branch: bzr merge lp:~kdub/mir/fix-1663062-step2
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve on 2017-02-21
Mir CI Bot continuous-integration Approve on 2017-02-20
Brandon Schaefer (community) 2017-02-14 Approve on 2017-02-15
Review via email: mp+317247@code.launchpad.net

Commit message

fix 1663062 for the 1.0 series by uploading ShmBuffers properly in nested servers. This re-enables the software/mesa buffer passthrough pass, while ensuring that lp: #1663062 doesn't happen.

Description of the change

fix 1663062 for the 1.0 series by uploading ShmBuffers properly in nested servers. This re-enables the software/mesa buffer passthrough pass, while ensuring that lp: #1663062 doesn't happen.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4030
https://mir-jenkins.ubuntu.com/job/mir-ci/2996/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3991/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4077
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4067
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4018/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4018/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4018/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4018/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4018/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4018/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4018/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2996/rebuild

review: Needs Fixing (continuous-integration)
Brandon Schaefer (brandontschaefer) wrote :

Small nit

19:09:59 I: mir_demo_client_animated_cursor
19:09:59 I: Smoke testing complete with returncode -1

https://bugs.launchpad.net/mir/+bug/1660889

review: Approve
Brandon Schaefer (brandontschaefer) wrote :

The nit is in a diff comment, the other comment is about the CI failure!

Daniel van Vugt (vanvugt) wrote :

Nit:

#include <GLES2/gl2.h>

should be:

#include MIR_SERVER_GL_H

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

Another nit:

+ * Authored by: Kevin DuBois <email address hidden>

Since I wrote that function (whose cpp file has Alexandros' name on it still), it would be less inaccurate to just remove the author comment(s).

Kevin DuBois (kdub) wrote :

Corrected to fit the current system of compile time gl switching.

re authorship, best I can tell, we just we need a name on it for copyright person (and canonical owns the copyright either way).
Should it be the person who created the file, or who wrote the logic behind the header, or who wrote the function header doesn't seem like a fruitful discussion. Removed to avert having to figure out (or come up with) the policy specifics.

Kevin DuBois (kdub) wrote :

and maybe we don't need names on it at all? (c) Canonical Ltd 2017 at the top might be sufficient

lp:~kdub/mir/fix-1663062-step2 updated on 2017-02-20
4031. By Kevin DuBois on 2017-02-20

remove author line

4032. By Kevin DuBois on 2017-02-20

use system of compile time gl switching

4033. By Kevin DuBois on 2017-02-20

fix up clang werror

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4033
https://mir-jenkins.ubuntu.com/job/mir-ci/3018/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4030
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4117
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4057/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3018/rebuild

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

Great, thanks.

I haven't repeated manual testing but if anything is still broken in this area we'll notice soon enough.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'include/platform/mir/graphics/gl_format.h'
2--- include/platform/mir/graphics/gl_format.h 1970-01-01 00:00:00 +0000
3+++ include/platform/mir/graphics/gl_format.h 2017-02-20 13:18:16 +0000
4@@ -0,0 +1,31 @@
5+/*
6+ * Copyright © 2017 Canonical Ltd.
7+ *
8+ * This program is free software: you can redistribute it and/or modify it
9+ * under the terms of the GNU Lesser General Public License version 3,
10+ * as published by the Free Software Foundation.
11+ *
12+ * This program is distributed in the hope that it will be useful,
13+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+ * GNU Lesser General Public License for more details.
16+ *
17+ * You should have received a copy of the GNU Lesser General Public License
18+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19+ *
20+ */
21+
22+#ifndef MIR_GRAPHICS_COMMON_GL_FORMAT_H_
23+#define MIR_GRAPHICS_COMMON_GL_FORMAT_H_
24+#include MIR_SERVER_GL_H
25+#include "mir_toolkit/common.h"
26+
27+namespace mir
28+{
29+namespace graphics
30+{
31+bool get_gl_pixel_format(MirPixelFormat mir_format,
32+ GLenum& gl_format, GLenum& gl_type);
33+}
34+}
35+#endif /* MIR_GRAPHICS_COMMON_GL_FORMAT_H_ */
36
37=== modified file 'src/platforms/common/server/shm_buffer.cpp'
38--- src/platforms/common/server/shm_buffer.cpp 2017-01-18 02:29:37 +0000
39+++ src/platforms/common/server/shm_buffer.cpp 2017-02-20 13:18:16 +0000
40@@ -17,9 +17,11 @@
41 * Alexandros Frantzis <alexandros.frantzis@canonical.com>
42 */
43
44+#include "mir/graphics/gl_format.h"
45 #include "shm_file.h"
46 #include "shm_buffer.h"
47 #include "buffer_texture_binder.h"
48+
49 #include MIR_SERVER_GL_H
50 #include MIR_SERVER_GLEXT_H
51
52@@ -34,9 +36,7 @@
53 namespace mgc = mir::graphics::common;
54 namespace geom = mir::geometry;
55
56-namespace {
57-
58-bool get_gl_pixel_format(MirPixelFormat mir_format,
59+bool mg::get_gl_pixel_format(MirPixelFormat mir_format,
60 GLenum& gl_format, GLenum& gl_type)
61 {
62 #if __BYTE_ORDER == __LITTLE_ENDIAN
63@@ -84,12 +84,10 @@
64 return gl_format != GL_INVALID_ENUM && gl_type != GL_INVALID_ENUM;
65 }
66
67-} // anonymous namespace
68-
69 bool mgc::ShmBuffer::supports(MirPixelFormat mir_format)
70 {
71 GLenum gl_format, gl_type;
72- return get_gl_pixel_format(mir_format, gl_format, gl_type);
73+ return mg::get_gl_pixel_format(mir_format, gl_format, gl_type);
74 }
75
76 mgc::ShmBuffer::ShmBuffer(
77@@ -127,7 +125,7 @@
78 {
79 GLenum format, type;
80
81- if (get_gl_pixel_format(pixel_format_, format, type))
82+ if (mg::get_gl_pixel_format(pixel_format_, format, type))
83 {
84 /*
85 * All existing Mir logic assumes that strides are whole multiples of
86
87=== modified file 'src/server/CMakeLists.txt'
88--- src/server/CMakeLists.txt 2017-02-15 07:38:33 +0000
89+++ src/server/CMakeLists.txt 2017-02-20 13:18:16 +0000
90@@ -110,6 +110,7 @@
91 mirprotobuf
92 mircookie
93
94+ server_platform_common
95 ${GLog_LIBRARY}
96 ${GFlags_LIBRARY}
97 ${EGL_LDFLAGS} ${EGL_LIBRARIES}
98
99=== modified file 'src/server/graphics/nested/CMakeLists.txt'
100--- src/server/graphics/nested/CMakeLists.txt 2017-02-17 07:03:56 +0000
101+++ src/server/graphics/nested/CMakeLists.txt 2017-02-20 13:18:16 +0000
102@@ -1,6 +1,7 @@
103 include_directories(
104 ${PROJECT_SOURCE_DIR}/include/renderers/gl
105 ${PROJECT_SOURCE_DIR}/include/platforms/mesa
106+ ${PROJECT_SOURCE_DIR}/include/platforms/common
107 )
108
109 add_library(
110
111=== modified file 'src/server/graphics/nested/buffer.cpp'
112--- src/server/graphics/nested/buffer.cpp 2017-02-15 07:38:33 +0000
113+++ src/server/graphics/nested/buffer.cpp 2017-02-20 13:18:16 +0000
114@@ -21,6 +21,7 @@
115 #include "mir/renderer/gl/texture_target.h"
116 #include "mir/graphics/egl_extensions.h"
117 #include "mir/graphics/buffer_ipc_message.h"
118+#include "mir/graphics/gl_format.h"
119 #include "mir_toolkit/mir_buffer.h"
120 #include "host_connection.h"
121 #include "buffer.h"
122@@ -47,10 +48,8 @@
123 {
124 public:
125 TextureAccess(
126- mgn::Buffer& buffer,
127 std::shared_ptr<mgn::NativeBuffer> const& native_buffer,
128 std::shared_ptr<mgn::HostConnection> const& connection) :
129- buffer(buffer),
130 native_buffer(native_buffer),
131 connection(connection)
132 {
133@@ -108,11 +107,9 @@
134 bind();
135 }
136
137-protected:
138- mgn::Buffer& buffer;
139+private:
140 std::shared_ptr<mgn::NativeBuffer> const native_buffer;
141 std::shared_ptr<mgn::HostConnection> const connection;
142-private:
143 mg::EGLExtensions extensions;
144 typedef std::pair<EGLDisplay, EGLContext> ImageResources;
145 typedef std::unique_ptr<EGLImageKHR, std::function<void(EGLImageKHR*)>> EGLImageUPtr;
146@@ -120,17 +117,20 @@
147 };
148
149 class PixelAndTextureAccess :
150- public TextureAccess,
151- public mrs::PixelSource
152+ public mg::NativeBufferBase,
153+ public mrs::PixelSource,
154+ public mrg::TextureSource
155 {
156 public:
157 PixelAndTextureAccess(
158 mgn::Buffer& buffer,
159- std::shared_ptr<mgn::NativeBuffer> const& native_buffer,
160- std::shared_ptr<mgn::HostConnection> const& connection) :
161- TextureAccess(buffer, native_buffer, connection),
162+ std::shared_ptr<mgn::NativeBuffer> const& native_buffer) :
163+ buffer(buffer),
164+ native_buffer(native_buffer),
165 stride_(geom::Stride{native_buffer->get_graphics_region()->stride})
166 {
167+ if (!mg::get_gl_pixel_format(buffer.pixel_format(), format, type))
168+ BOOST_THROW_EXCEPTION(std::logic_error("buffer cannot be used as texture"));
169 }
170
171 void write(unsigned char const* pixels, size_t pixel_size) override
172@@ -163,17 +163,39 @@
173 return stride_;
174 }
175
176+ void bind() override
177+ {
178+ glPixelStorei(GL_UNPACK_ALIGNMENT, 1);
179+ glTexImage2D(
180+ GL_TEXTURE_2D, 0, format,
181+ buffer.size().width.as_int(), buffer.size().height.as_int(),
182+ 0, format, type, native_buffer->get_graphics_region()->vaddr);
183+ }
184+
185+ void gl_bind_to_texture() override
186+ {
187+ bind();
188+ }
189+
190+ void secure_for_render() override
191+ {
192+ }
193+
194 private:
195+ mgn::Buffer& buffer;
196+ std::shared_ptr<mgn::NativeBuffer> const native_buffer;
197 geom::Stride const stride_;
198+ GLenum format;
199+ GLenum type;
200 };
201 }
202
203 std::shared_ptr<mg::NativeBufferBase> mgn::Buffer::create_native_base(mg::BufferUsage const usage)
204 {
205 if (usage == mg::BufferUsage::software)
206- return std::make_shared<PixelAndTextureAccess>(*this, buffer, connection);
207+ return std::make_shared<PixelAndTextureAccess>(*this, buffer);
208 else if (usage == mg::BufferUsage::hardware)
209- return std::make_shared<TextureAccess>(*this, buffer, connection);
210+ return std::make_shared<TextureAccess>(buffer, connection);
211 else
212 BOOST_THROW_EXCEPTION(std::invalid_argument("usage not supported when creating nested::Buffer"));
213 }
214@@ -192,7 +214,7 @@
215 geom::Size size, uint32_t native_format, uint32_t native_flags) :
216 connection(connection),
217 buffer(connection->create_buffer(size, native_format, native_flags)),
218- native_base(std::make_shared<TextureAccess>(*this, buffer, connection))
219+ native_base(std::make_shared<TextureAccess>(buffer, connection))
220 {
221 }
222
223@@ -202,7 +224,7 @@
224 MirPixelFormat format) :
225 connection(connection),
226 buffer(connection->create_buffer(size, format)),
227- native_base(std::make_shared<PixelAndTextureAccess>(*this, buffer, connection))
228+ native_base(std::make_shared<PixelAndTextureAccess>(*this, buffer))
229 {
230 }
231
232
233=== modified file 'src/server/graphics/nested/mir_client_host_connection.cpp'
234--- src/server/graphics/nested/mir_client_host_connection.cpp 2017-02-17 07:03:56 +0000
235+++ src/server/graphics/nested/mir_client_host_connection.cpp 2017-02-20 13:18:16 +0000
236@@ -685,13 +685,10 @@
237 return std::make_unique<SurfaceSpec>(mir_connection);
238 }
239
240-bool mgn::MirClientHostConnection::supports_passthrough(mg::BufferUsage usage)
241+bool mgn::MirClientHostConnection::supports_passthrough(mg::BufferUsage)
242 {
243- //FIXME: ShmBuffers currently don't upload properly. The logic here uses
244- // passthrough where supported (ANativeWindowBuffer and gbm_bo)
245- // (LP: #1663062 for more details)
246- return (mir_extension_android_buffer_v1(mir_connection) ||
247- ((mir_extension_gbm_buffer_v1(mir_connection) && usage == mg::BufferUsage::hardware)));
248+ return mir_extension_android_buffer_v1(mir_connection) ||
249+ mir_extension_gbm_buffer_v1(mir_connection);
250 }
251
252 void mgn::MirClientHostConnection::apply_input_configuration(MirInputConfig const* config)
253
254=== modified file 'src/server/graphics/nested/platform.cpp'
255--- src/server/graphics/nested/platform.cpp 2017-02-15 07:38:33 +0000
256+++ src/server/graphics/nested/platform.cpp 2017-02-20 13:18:16 +0000
257@@ -75,10 +75,7 @@
258
259 std::shared_ptr<mg::Buffer> alloc_buffer(mg::BufferProperties const& properties) override
260 {
261- if (passthrough_candidate(properties.size, properties.usage))
262- return std::make_shared<mgn::Buffer>(connection, properties);
263- else
264- return guest_allocator->alloc_buffer(properties);
265+ return guest_allocator->alloc_buffer(properties);
266 }
267
268 std::shared_ptr<mg::Buffer> alloc_buffer(
269
270=== modified file 'tests/unit-tests/platforms/nested/test_buffer.cpp'
271--- tests/unit-tests/platforms/nested/test_buffer.cpp 2017-02-15 07:38:33 +0000
272+++ tests/unit-tests/platforms/nested/test_buffer.cpp 2017-02-20 13:18:16 +0000
273@@ -234,6 +234,30 @@
274 texture_source->gl_bind_to_texture();
275 }
276
277+TEST_F(NestedBuffer, binds_to_texture_gl_buffer)
278+{
279+ auto pf = sw_properties.format;
280+ auto format = GL_RGBA;
281+ auto type = GL_UNSIGNED_BYTE;
282+
283+ mgn::Buffer buffer(mt::fake_shared(mock_connection), size, pf);
284+
285+ auto native_base = buffer.native_buffer_base();
286+ ASSERT_THAT(native_base, Ne(nullptr));
287+ auto texture_source = dynamic_cast<mir::renderer::gl::TextureSource*>(native_base);
288+ ASSERT_THAT(texture_source, Ne(nullptr));
289+
290+ EXPECT_CALL(mock_egl, glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, _))
291+ .Times(0);
292+
293+ EXPECT_CALL(mock_gl, glPixelStorei(GL_UNPACK_ALIGNMENT, 1));
294+ EXPECT_CALL(mock_gl, glTexImage2D(
295+ GL_TEXTURE_2D, 0, format,
296+ buffer.size().width.as_int(), buffer.size().height.as_int(),
297+ 0, format, type, _));
298+ texture_source->gl_bind_to_texture();
299+}
300+
301 TEST_F(NestedBuffer, just_makes_one_bind_per_display_context_pair)
302 {
303 ON_CALL(*client_buffer, egl_image_creation_hints())

Subscribers

People subscribed via source and target branches