Mir

Merge lp:~vanvugt/mir/fix-1510218 into lp:mir

Proposed by Daniel van Vugt on 2015-10-27
Status: Merged
Approved by: Daniel van Vugt on 2015-10-29
Approved revision: 3066
Merged at revision: 3060
Proposed branch: lp:~vanvugt/mir/fix-1510218
Merge into: lp:mir
Diff against target: 221 lines (+122/-31)
5 files modified
src/platforms/android/client/android_client_platform.cpp (+3/-14)
src/platforms/common/client/mir/CMakeLists.txt (+1/-0)
src/platforms/common/client/mir/weak_egl.cpp (+65/-0)
src/platforms/common/client/mir/weak_egl.h (+47/-0)
src/platforms/mesa/client/client_platform.cpp (+6/-17)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1510218
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) 2015-10-28 Approve on 2015-10-28
Alan Griffiths 2015-10-27 Approve on 2015-10-28
PS Jenkins bot (community) continuous-integration Approve on 2015-10-28
Review via email: mp+275820@code.launchpad.net

Commit message

Avoid __attribute__((weak)) as that will fail to resolve symbols from
lazily loaded libraries (like Qt platform plugins apparently).
(LP: #1510218)

This implementation is written such that the tests introduced in r2771
still work. But if you want a new test specifically for RTLD_LAZY,
that's going to take a whole lot more work. I'm not sure it's worth it
as long as the initial tests from r2771 still work.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/fix-1510218 updated on 2015-10-27
3065. By Daniel van Vugt on 2015-10-27

Header guard!

Alan Griffiths (alan-griffiths) wrote :

code does not need to be inlined in a header

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/fix-1510218 updated on 2015-10-28
3066. By Daniel van Vugt on 2015-10-28

Don't inline WeakEGL.

It was inlined intentionally because I didn't think there was an appropriate
library to put it in. But then I discovered src/platforms/common/client/

Alan Griffiths (alan-griffiths) wrote :

Nit:

+namespace mir { namespace client {

in a .cpp isn't our "house style"

review: Approve
Alberto Aguirre (albaguirre) wrote :

Given that the API requires an EGLConfig parameter - which needs to be obtained from another call to libEGL, I guess it's not unreasonable to expect the caller of mir_connection_get_egl_pixel_format has linked to libEGL.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/platforms/android/client/android_client_platform.cpp'
--- src/platforms/android/client/android_client_platform.cpp 2015-07-21 04:30:03 +0000
+++ src/platforms/android/client/android_client_platform.cpp 2015-10-28 07:23:08 +0000
@@ -24,6 +24,7 @@
24#include "android_client_buffer_factory.h"24#include "android_client_buffer_factory.h"
25#include "egl_native_surface_interpreter.h"25#include "egl_native_surface_interpreter.h"
2626
27#include "mir/weak_egl.h"
27#include <EGL/egl.h>28#include <EGL/egl.h>
2829
29#include <boost/throw_exception.hpp>30#include <boost/throw_exception.hpp>
@@ -124,19 +125,6 @@
124 return buf->anwb();125 return buf->anwb();
125}126}
126127
127/*
128 * Driver modules get dlopened with RTLD_NOW, meaning that if the below egl
129 * functions aren't found in memory the driver fails to load. This would
130 * normally prevent software clients (those not linked to libEGL) from
131 * successfully loading our client module, but if we mark the undefined
132 * egl function symbols as "weak" then their absence is no longer an error,
133 * even with RTLD_NOW.
134 */
135extern "C" EGLAPI EGLBoolean EGLAPIENTRY
136 eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
137 EGLint attribute, EGLint *value)
138 __attribute__((weak));
139
140MirPixelFormat mcla::AndroidClientPlatform::get_egl_pixel_format(128MirPixelFormat mcla::AndroidClientPlatform::get_egl_pixel_format(
141 EGLDisplay disp, EGLConfig conf) const129 EGLDisplay disp, EGLConfig conf) const
142{130{
@@ -144,7 +132,8 @@
144132
145 // EGL_KHR_platform_android says this will always work...133 // EGL_KHR_platform_android says this will always work...
146 EGLint vis = 0;134 EGLint vis = 0;
147 if (eglGetConfigAttrib(disp, conf, EGL_NATIVE_VISUAL_ID, &vis))135 mcl::WeakEGL weak;
136 if (weak.eglGetConfigAttrib(disp, conf, EGL_NATIVE_VISUAL_ID, &vis))
148 mir_format = mir::graphics::android::to_mir_format(vis);137 mir_format = mir::graphics::android::to_mir_format(vis);
149138
150 return mir_format;139 return mir_format;
151140
=== modified file 'src/platforms/common/client/mir/CMakeLists.txt'
--- src/platforms/common/client/mir/CMakeLists.txt 2015-01-19 14:16:46 +0000
+++ src/platforms/common/client/mir/CMakeLists.txt 2015-10-28 07:23:08 +0000
@@ -2,4 +2,5 @@
22
3add_library(client_platform_common OBJECT3add_library(client_platform_common OBJECT
4 aging_buffer.cpp4 aging_buffer.cpp
5 weak_egl.cpp
5)6)
67
=== added file 'src/platforms/common/client/mir/weak_egl.cpp'
--- src/platforms/common/client/mir/weak_egl.cpp 1970-01-01 00:00:00 +0000
+++ src/platforms/common/client/mir/weak_egl.cpp 2015-10-28 07:23:08 +0000
@@ -0,0 +1,65 @@
1/*
2 * EGL without any linkage requirements!
3 * ~~~
4 * Copyright © 2015 Canonical Ltd.
5 *
6 * This program is free software: you can redistribute it and/or modify it
7 * under the terms of the GNU Lesser General Public License version 3,
8 * as published by the Free Software Foundation.
9 *
10 * This program is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU Lesser General Public License for more details.
14 *
15 * You should have received a copy of the GNU Lesser General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 *
18 * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
19 */
20
21#include "weak_egl.h"
22#include <dlfcn.h>
23
24namespace mir { namespace client {
25
26WeakEGL::WeakEGL()
27 : egl1(nullptr)
28 , pGetConfigAttrib(nullptr)
29{
30}
31
32WeakEGL::~WeakEGL()
33{
34 if (egl1)
35 dlclose(egl1);
36}
37
38EGLBoolean WeakEGL::eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
39 EGLint attribute, EGLint* value)
40{
41 if (find("eglGetConfigAttrib", (void**)&pGetConfigAttrib))
42 return pGetConfigAttrib(dpy, config, attribute, value);
43 else
44 return EGL_FALSE;
45}
46
47bool WeakEGL::find(char const* name, void** func)
48{
49 if (!*func)
50 {
51 // RTLD_DEFAULT is first choice to support wrappers like MockEGL
52 if (!(*func = dlsym(RTLD_DEFAULT, name)))
53 {
54 // This will work more in real-world situations if the library
55 // is hidden behind an RTLD_LOCAL (e.g. a Qt plugin)
56 if (!egl1)
57 egl1 = dlopen("libEGL.so.1", RTLD_NOLOAD|RTLD_LAZY);
58 if (egl1)
59 *func = dlsym(egl1, name);
60 }
61 }
62 return *func != nullptr;
63}
64
65}} // namespace mir::client
066
=== added file 'src/platforms/common/client/mir/weak_egl.h'
--- src/platforms/common/client/mir/weak_egl.h 1970-01-01 00:00:00 +0000
+++ src/platforms/common/client/mir/weak_egl.h 2015-10-28 07:23:08 +0000
@@ -0,0 +1,47 @@
1/*
2 * EGL without any linkage requirements!
3 * ~~~
4 * Copyright © 2015 Canonical Ltd.
5 *
6 * This program is free software: you can redistribute it and/or modify it
7 * under the terms of the GNU Lesser General Public License version 3,
8 * as published by the Free Software Foundation.
9 *
10 * This program is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU Lesser General Public License for more details.
14 *
15 * You should have received a copy of the GNU Lesser General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 *
18 * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
19 */
20
21#ifndef MIR_CLIENT_WEAK_EGL_H_
22#define MIR_CLIENT_WEAK_EGL_H_
23
24#include <EGL/egl.h>
25#include <dlfcn.h>
26
27namespace mir { namespace client {
28
29class WeakEGL
30{
31public:
32 WeakEGL();
33 ~WeakEGL();
34 EGLBoolean eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
35 EGLint attribute, EGLint* value);
36
37private:
38 bool find(char const* name, void** func);
39
40 void* egl1;
41 EGLBoolean (*pGetConfigAttrib)(EGLDisplay dpy, EGLConfig config,
42 EGLint attribute, EGLint* value);
43};
44
45}} // namespace mir::client
46
47#endif // MIR_CLIENT_WEAK_EGL_H_
048
=== modified file 'src/platforms/mesa/client/client_platform.cpp'
--- src/platforms/mesa/client/client_platform.cpp 2015-10-19 12:10:38 +0000
+++ src/platforms/mesa/client/client_platform.cpp 2015-10-28 07:23:08 +0000
@@ -23,6 +23,7 @@
23#include "native_surface.h"23#include "native_surface.h"
24#include "mir/client_buffer_factory.h"24#include "mir/client_buffer_factory.h"
25#include "mir/client_context.h"25#include "mir/client_context.h"
26#include "mir/weak_egl.h"
26#include "mir_toolkit/mesa/platform_operation.h"27#include "mir_toolkit/mesa/platform_operation.h"
2728
28#include <cstring>29#include <cstring>
@@ -163,19 +164,6 @@
163}164}
164165
165166
166/*
167 * Driver modules get dlopened with RTLD_NOW, meaning that if the below egl
168 * functions aren't found in memory the driver fails to load. This would
169 * normally prevent software clients (those not linked to libEGL) from
170 * successfully loading our client module, but if we mark the undefined
171 * egl function symbols as "weak" then their absence is no longer an error,
172 * even with RTLD_NOW.
173 */
174extern "C" EGLAPI EGLBoolean EGLAPIENTRY
175 eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
176 EGLint attribute, EGLint *value)
177 __attribute__((weak));
178
179MirPixelFormat mclm::ClientPlatform::get_egl_pixel_format(167MirPixelFormat mclm::ClientPlatform::get_egl_pixel_format(
180 EGLDisplay disp, EGLConfig conf) const168 EGLDisplay disp, EGLConfig conf) const
181{169{
@@ -192,10 +180,11 @@
192 * EGL_NATIVE_VISUAL_ID, so ignore that for now.180 * EGL_NATIVE_VISUAL_ID, so ignore that for now.
193 */181 */
194 EGLint r = 0, g = 0, b = 0, a = 0;182 EGLint r = 0, g = 0, b = 0, a = 0;
195 eglGetConfigAttrib(disp, conf, EGL_RED_SIZE, &r);183 mcl::WeakEGL weak;
196 eglGetConfigAttrib(disp, conf, EGL_GREEN_SIZE, &g);184 weak.eglGetConfigAttrib(disp, conf, EGL_RED_SIZE, &r);
197 eglGetConfigAttrib(disp, conf, EGL_BLUE_SIZE, &b);185 weak.eglGetConfigAttrib(disp, conf, EGL_GREEN_SIZE, &g);
198 eglGetConfigAttrib(disp, conf, EGL_ALPHA_SIZE, &a);186 weak.eglGetConfigAttrib(disp, conf, EGL_BLUE_SIZE, &b);
187 weak.eglGetConfigAttrib(disp, conf, EGL_ALPHA_SIZE, &a);
199188
200 if (r == 8 && g == 8 && b == 8)189 if (r == 8 && g == 8 && b == 8)
201 {190 {

Subscribers

People subscribed via source and target branches