Mir

Merge lp:~alan-griffiths/mir/fix-1695221 into lp:mir

Proposed by Alan Griffiths on 2017-06-05
Status: Merged
Approved by: Alan Griffiths on 2017-06-06
Approved revision: 4186
Merged at revision: 4186
Proposed branch: lp:~alan-griffiths/mir/fix-1695221
Merge into: lp:mir
Diff against target: 103 lines (+52/-6)
2 files modified
examples/CMakeLists.txt (+4/-0)
examples/eglsquare.cpp (+48/-6)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1695221
Reviewer Review Type Date Requested Status
Daniel van Vugt 2017-06-05 Approve on 2017-06-06
Mir CI Bot continuous-integration Approve on 2017-06-05
Review via email: mp+325066@code.launchpad.net

Commit message

Correct eglsquare example: Don't paint from the window event handler. (LP: #1695221)

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

FAILED: Continuous integration, rev:4185
https://mir-jenkins.ubuntu.com/job/mir-ci/3438/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4682/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4815
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4805
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4805
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4805
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4718/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4718/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4718/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4718/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4718/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4718/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4718/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4718
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4718/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Hmm. Looks like a clang issue on artful and zesty...

https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4718/console

09:57:51 /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/atomic:236: error: undefined reference to '__atomic_load_8'
09:57:51 /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/atomic:225: error: undefined reference to '__atomic_store_8'
09:57:51 clang: error: linker command failed with exit code 1 (use -v to see invocation)
09:57:51 examples/CMakeFiles/mir_demo_client_eglsquare.dir/build.make:114: recipe for target 'bin/mir_demo_client_eglsquare.bin' failed

https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4718/console

09:55:32 make[3]: Leaving directory '/<<BUILDDIR>>/mir-0.27.0+artful4805bzr4185/obj-x86_64-linux-gnu'
09:55:32 /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/atomic:236: error: undefined reference to '__atomic_load_8'
09:55:32 /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/atomic:225: error: undefined reference to '__atomic_store_8'

lp:~alan-griffiths/mir/fix-1695221 updated on 2017-06-05
4186. By Alan Griffiths on 2017-06-05

Workaround clang linker error on artful and zesty

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

PASSED: Continuous integration, rev:4186
https://mir-jenkins.ubuntu.com/job/mir-ci/3439/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4683
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4816
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4806
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4806
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4806
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4719/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4719
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4719/artifact/output/*zip*/output.zip

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

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

Without testing this, the fix sounds reasonable.

Using GL in a Mir callback is expected to fail. Even if you remember context.make_current() and even if it works, I recall you could still hit deadlocks making Mir/GL calls from in that event callback thread.

P.S. mir_demo_client_target is intended to replace and supersede mir_demo_client_eglsquare. See also bug 1433925 and bug 1651695, which we probably don't care to fix. Those bugs don't exist in mir_demo_client_target. As such I wouldn't spend too much time trying to fix eglsquare - it's just too broken and not necessary.

review: Approve
Alan Griffiths (alan-griffiths) wrote :

> Without testing this, the fix sounds reasonable.
>
> Using GL in a Mir callback is expected to fail. Even if you remember
> context.make_current() and even if it works, I recall you could still hit
> deadlocks making Mir/GL calls from in that event callback thread.

That's exactly what was happening here.

> P.S. mir_demo_client_target is intended to replace and supersede
> mir_demo_client_eglsquare. See also bug 1433925 and bug 1651695, which we
> probably don't care to fix. Those bugs don't exist in mir_demo_client_target.
> As such I wouldn't spend too much time trying to fix eglsquare - it's just too
> broken and not necessary.

I spent the time because I wanted to understand the problem. Fixing was a quick confirmation of the diagnosis.

(Happy to see eglsquare deleted.)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/CMakeLists.txt'
2--- examples/CMakeLists.txt 2017-05-26 08:33:17 +0000
3+++ examples/CMakeLists.txt 2017-06-05 11:06:43 +0000
4@@ -153,6 +153,10 @@
5 mir_add_wrapped_executable(mir_demo_client_eglsquare eglsquare.cpp)
6 target_link_libraries(mir_demo_client_eglsquare eglapp)
7
8+if ("${CMAKE_CXX_COMPILER}" MATCHES "clang")
9+ target_link_libraries(mir_demo_client_eglsquare atomic)
10+endif()
11+
12 mir_add_wrapped_executable(mir_demo_client_flicker
13 flicker.c
14 )
15
16=== modified file 'examples/eglsquare.cpp'
17--- examples/eglsquare.cpp 2017-05-23 10:51:47 +0000
18+++ examples/eglsquare.cpp 2017-06-05 11:06:43 +0000
19@@ -26,6 +26,8 @@
20 #include <cstring>
21 #include <unistd.h>
22 #include <signal.h>
23+#include <condition_variable>
24+#include <atomic>
25
26 #include "client_helpers.h"
27
28@@ -107,11 +109,24 @@
29 dimensions(active_output_dimensions(connection)),
30 context{connection, swap_interval, dimensions.width, dimensions.height},
31 window{connection, dimensions.width, dimensions.height, context},
32- program{context, dimensions.width, dimensions.height}
33+ program{context, dimensions.width, dimensions.height},
34+ pos{Pos{0,0}},
35+ worker{[this] { do_work(); }}
36 {
37 mir_window_set_event_handler(window, &on_event, this);
38 }
39
40+ ~SquareRenderingSurface()
41+ {
42+ {
43+ std::lock_guard<decltype(mutex)> lock{mutex};
44+ running = false;
45+ cv.notify_one();
46+ }
47+
48+ if (worker.joinable()) worker.join();
49+ }
50+
51 void on_event(MirEvent const* ev)
52 {
53 if (mir_event_get_type(ev) != mir_event_type_input)
54@@ -135,11 +150,9 @@
55 {
56 return;
57 }
58- context.make_current();
59- program.draw(
60- x/static_cast<float>(dimensions.width)*2.0 - 1.0,
61- y/static_cast<float>(dimensions.height)*-2.0 + 1.0);
62- context.swapbuffers();
63+
64+ pos = Pos{x, y};
65+ cv.notify_one();
66 }
67
68 SquareRenderingSurface(SquareRenderingSurface const&) = delete;
69@@ -184,6 +197,35 @@
70 auto surface = reinterpret_cast<SquareRenderingSurface*>(context);
71 if (surface) surface->on_event(event);
72 }
73+
74+private:
75+ void do_work()
76+ {
77+ std::unique_lock<decltype(mutex)> lock(mutex);
78+
79+ while (true)
80+ {
81+ cv.wait(lock);
82+
83+ if (!running) return;
84+
85+ Pos pos = this->pos;
86+
87+ context.make_current();
88+ program.draw(
89+ pos.x/static_cast<float>(dimensions.width)*2.0 - 1.0,
90+ pos.y/static_cast<float>(dimensions.height)*-2.0 + 1.0);
91+ context.swapbuffers();
92+ }
93+ }
94+
95+ struct Pos { float x; float y; };
96+ std::atomic<Pos> pos;
97+
98+ std::thread worker;
99+ std::condition_variable cv;
100+ std::mutex mutex;
101+ bool running{true};
102 };
103 }
104

Subscribers

People subscribed via source and target branches