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

[Alan Griffiths]

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

[Mir CI Bot (mir-ci-bot)]

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)]

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'

[Mir CI Bot (mir-ci-bot)]

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)]

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)]

> 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.)

=== modified file 'examples/CMakeLists.txt'
--- examples/CMakeLists.txt	2017-05-26 08:33:17 +0000
+++ examples/CMakeLists.txt	2017-06-05 11:06:43 +0000
@@ -153,6 +153,10 @@
 mir_add_wrapped_executable(mir_demo_client_eglsquare eglsquare.cpp)
 target_link_libraries(mir_demo_client_eglsquare eglapp)
 
+if ("${CMAKE_CXX_COMPILER}" MATCHES "clang")
+  target_link_libraries(mir_demo_client_eglsquare atomic)
+endif()
+
 mir_add_wrapped_executable(mir_demo_client_flicker
   flicker.c
 )

=== modified file 'examples/eglsquare.cpp'
--- examples/eglsquare.cpp	2017-05-23 10:51:47 +0000
+++ examples/eglsquare.cpp	2017-06-05 11:06:43 +0000
@@ -26,6 +26,8 @@
 #include <cstring>
 #include <unistd.h>
 #include <signal.h>
+#include <condition_variable>
+#include <atomic>
 
 #include "client_helpers.h"
 
@@ -107,11 +109,24 @@
         dimensions(active_output_dimensions(connection)),
         context{connection, swap_interval, dimensions.width, dimensions.height},
         window{connection, dimensions.width, dimensions.height, context},
-        program{context, dimensions.width, dimensions.height}
+        program{context, dimensions.width, dimensions.height},
+        pos{Pos{0,0}},
+        worker{[this] { do_work(); }}
     {
         mir_window_set_event_handler(window, &on_event, this);
     }
 
+    ~SquareRenderingSurface()
+    {
+        {
+            std::lock_guard<decltype(mutex)> lock{mutex};
+            running = false;
+            cv.notify_one();
+        }
+
+        if (worker.joinable()) worker.join();
+    }
+
     void on_event(MirEvent const* ev)
     {
         if (mir_event_get_type(ev) != mir_event_type_input)
@@ -135,11 +150,9 @@
         {
             return;
         }
-        context.make_current();
-        program.draw(
-            x/static_cast<float>(dimensions.width)*2.0 - 1.0,
-            y/static_cast<float>(dimensions.height)*-2.0 + 1.0);
-        context.swapbuffers();
+
+        pos = Pos{x, y};
+        cv.notify_one();
     }
 
     SquareRenderingSurface(SquareRenderingSurface const&) = delete;
@@ -184,6 +197,35 @@
         auto surface = reinterpret_cast<SquareRenderingSurface*>(context);
         if (surface) surface->on_event(event);
     }
+
+private:
+    void do_work()
+    {
+        std::unique_lock<decltype(mutex)> lock(mutex);
+
+        while (true)
+        {
+            cv.wait(lock);
+
+            if (!running) return;
+
+            Pos  pos = this->pos;
+
+            context.make_current();
+            program.draw(
+                pos.x/static_cast<float>(dimensions.width)*2.0 - 1.0,
+                pos.y/static_cast<float>(dimensions.height)*-2.0 + 1.0);
+            context.swapbuffers();
+        }
+    }
+
+    struct Pos { float x; float y; };
+    std::atomic<Pos> pos;
+
+    std::thread worker;
+    std::condition_variable cv;
+    std::mutex mutex;
+    bool running{true};
 };
 }