Merge lp:~alan-griffiths/mir/does-jenkins-like-MultiThreadedCompositor-refactoring into lp:mir

[Alan Griffiths]

compositor: clearer logic for MultiThreadedCompositor::start()/stop()

compositor: clearer logic for MultiThreadedCompositor::start()/stop()

This should be a pure refactoring extracted from lp:~alan-griffiths/mir/better-error-reporting-in-MultiThreadedCompositor-start (as I've not identified the problem causing CI failures there).

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

PASSED: Continuous integration, rev:3274
https://mir-jenkins.ubuntu.com/job/mir-ci/166/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/166/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3274
http://jenkins.qa.ubuntu.com/job/mir-ci/6135/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5703
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4610
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5659
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/340
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/459
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/459/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/459
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/459/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5656
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5656/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8088
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27072
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/336
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/336/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/192
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27074

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6135/rebuild

review: Approve (continuous-integration)

[lp:~alan-griffiths/mir/does-jenkins-like-MultiThreadedCompositor-refactoring]

3275. By Alan Griffiths on 2016-01-27

  Report errors starting compositor threads by propagating exception from start()

3276. By Alan Griffiths on 2016-01-27

Experiment to diagnose Jenkins failures

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

PASSED: Continuous integration, rev:3276
https://mir-jenkins.ubuntu.com/job/mir-ci/168/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/168/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3276
http://jenkins.qa.ubuntu.com/job/mir-ci/6138/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5706
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4613
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5662
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/341
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/462
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/462/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/462
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/462/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5659
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5659/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8089
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27077
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/337
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/337/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/193
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27088

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6138/rebuild

review: Approve (continuous-integration)

Unmerged revisions

3276. By Alan Griffiths on 2016-01-27

Experiment to diagnose Jenkins failures

3275. By Alan Griffiths on 2016-01-27

  Report errors starting compositor threads by propagating exception from start()

3274. By Alan Griffiths on 2016-01-27

  Don't mix level of abstraction within a function

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2016-01-20 23:59:18 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2016-01-27 13:57:45 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013-2015 Canonical Ltd.
+ * Copyright © 2013-2016 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3,
@@ -110,81 +110,82 @@
 
         started.set_value();
 
-        std::unique_lock<std::mutex> lock{run_mutex};
-        while (running)
+        try
         {
-            /* Wait until compositing has been scheduled or we are stopped */
-            run_cv.wait(lock, [&]{ return (frames_scheduled > 0) || !running; });
-
-            /*
-             * Check if we are running before compositing, since we may have
-             * been stopped while waiting for the run_cv above.
-             */
-            if (running)
+            std::unique_lock<std::mutex> lock{run_mutex};
+            while (running)
             {
-                /*
-                 * Each surface could have a number of frames ready in its buffer
-                 * queue. And we need to ensure that we render all of them so that
-                 * none linger in the queue indefinitely (seen as input lag).
-                 * frames_scheduled indicates the number of frames that are scheduled
-                 * to ensure all surfaces' queues are fully drained.
-                 */
-                frames_scheduled--;
-                lock.unlock();
-
-                for (auto& tuple : compositors)
-                {
-                    auto& compositor = std::get<1>(tuple);
-                    compositor->composite(scene->scene_elements_for(compositor.get()));
-                }
-                group.post();
-
-                /*
-                 * "Predictive bypass" optimization: If the last frame was
-                 * bypassed/overlayed or you simply have a fast GPU, it is
-                 * beneficial to sleep for most of the next frame. This reduces
-                 * the latency between snapshotting the scene and post()
-                 * completing by almost a whole frame.
-                 */
-                auto delay = force_sleep >= std::chrono::milliseconds::zero() ?
-                             force_sleep : group.recommended_sleep();
-                std::this_thread::sleep_for(delay);
-
-                lock.lock();
-
-                /*
-                 * Note the compositor may have chosen to ignore any number
-                 * of renderables and not consumed buffers from them. So it's
-                 * important to re-count number of frames pending, separately
-                 * to the initial scene_elements_for()...
-                 */
-                int pending = 0;
-                for (auto& compositor : compositors)
-                {
-                    auto const comp_id = std::get<1>(compositor).get();
-                    int pend = scene->frames_pending(comp_id);
-                    if (pend > pending)
-                        pending = pend;
-                }
-
-                if (pending > frames_scheduled)
-                    frames_scheduled = pending;
+                /* Wait until compositing has been scheduled or we are stopped */
+                run_cv.wait(lock, [&]{ return (frames_scheduled > 0) || !running; });
+
+                /*
+                 * Check if we are running before compositing, since we may have
+                 * been stopped while waiting for the run_cv above.
+                 */
+                if (running)
+                {
+                    /*
+                     * Each surface could have a number of frames ready in its buffer
+                     * queue. And we need to ensure that we render all of them so that
+                     * none linger in the queue indefinitely (seen as input lag).
+                     * frames_scheduled indicates the number of frames that are scheduled
+                     * to ensure all surfaces' queues are fully drained.
+                     */
+                    frames_scheduled--;
+                    lock.unlock();
+
+                    for (auto& tuple : compositors)
+                    {
+                        auto& compositor = std::get<1>(tuple);
+                        compositor->composite(scene->scene_elements_for(compositor.get()));
+                    }
+                    group.post();
+
+                    /*
+                     * "Predictive bypass" optimization: If the last frame was
+                     * bypassed/overlayed or you simply have a fast GPU, it is
+                     * beneficial to sleep for most of the next frame. This reduces
+                     * the latency between snapshotting the scene and post()
+                     * completing by almost a whole frame.
+                     */
+                    auto delay = force_sleep >= std::chrono::milliseconds::zero() ?
+                                 force_sleep : group.recommended_sleep();
+                    std::this_thread::sleep_for(delay);
+
+                    lock.lock();
+
+                    /*
+                     * Note the compositor may have chosen to ignore any number
+                     * of renderables and not consumed buffers from them. So it's
+                     * important to re-count number of frames pending, separately
+                     * to the initial scene_elements_for()...
+                     */
+                    int pending = 0;
+                    for (auto& compositor : compositors)
+                    {
+                        auto const comp_id = std::get<1>(compositor).get();
+                        int pend = scene->frames_pending(comp_id);
+                        if (pend > pending)
+                            pending = pend;
+                    }
+
+                    if (pending > frames_scheduled)
+                        frames_scheduled = pending;
+                }
             }
         }
+        catch (...)
+        {
+            mir::terminate_with_current_exception();
+        }
     }
     catch(...)
     {
-        try
-        {
-            //Move the promise so that the promise destructor occurs here rather than in the thread
-            //destroying CompositingFunctor, mostly to appease TSan
-            auto promise = std::move(started);
-            promise.set_exception(std::current_exception());
-        }
-        catch(...)
-        {
-        }
-        mir::terminate_with_current_exception();
+        started.set_exception(std::current_exception());
+
+        //Move the promise so that the promise destructor occurs here rather than in the thread
+        //destroying CompositingFunctor, mostly to appease TSan
+        auto promise = std::move(started);
     }
 
     void schedule_compositing(int num_frames)
@@ -209,6 +210,8 @@
     {
         if (started_future.wait_for(10s) != std::future_status::ready)
             BOOST_THROW_EXCEPTION(std::runtime_error("Compositor thread failed to start"));
+
+        started_future.get();
     }
 
 private:
@@ -273,7 +276,7 @@
 void mc::MultiThreadedCompositor::start()
 {
     auto stopped = CompositorState::stopped;
-    
+
     if (!state.compare_exchange_strong(stopped, CompositorState::starting))
         return;
 
@@ -281,7 +284,7 @@
 
     /* To cleanup state if any code below throws */
     auto cleanup_if_unwinding = on_unwind(
-        [this]{ destroy_compositing_threads(); });
+        [this]{ destroy_compositing_threads(); state = CompositorState::stopped; });
 
     create_compositing_threads();
 
@@ -291,6 +294,8 @@
     /* Optional first render */
     if (compose_on_start)
         schedule_compositing(1);
+
+    state = CompositorState::started;
 }
 
 void mc::MultiThreadedCompositor::stop()
@@ -300,8 +305,6 @@
     if (!state.compare_exchange_strong(started, CompositorState::stopping))
         return;
 
-    state = CompositorState::stopping;
-
     /* To cleanup state if any code below throws */
     auto cleanup_if_unwinding = on_unwind(
         [this]{ state = CompositorState::started; });
@@ -314,6 +317,10 @@
     // If the compositor is restarted we've likely got clients blocked
     // so we will need to schedule compositing immediately
     compose_on_start = true;
+
+    report->stopped();
+
+    state = CompositorState::stopped;
 }
 
 void mc::MultiThreadedCompositor::create_compositing_threads()
@@ -333,8 +340,6 @@
 
     for (auto& functor : thread_functors)
         functor->wait_until_started();
-
-    state = CompositorState::started;
 }
 
 void mc::MultiThreadedCompositor::destroy_compositing_threads()
@@ -347,8 +352,4 @@
 
     thread_functors.clear();
     futures.clear();
-
-    report->stopped();
-
-    state = CompositorState::stopped;
 }

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2016-01-20 23:59:18 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2016-01-27 13:57:45 +0000
@@ -889,7 +889,7 @@
     compositor.stop();
 }
 
-TEST(MultiThreadedCompositor, does_not_block_in_start_when_compositor_thread_fails)
+TEST(MultiThreadedCompositor, DISABLED_when_compositor_thread_fails_start_reports_error)
 {
     using namespace testing;
     unsigned int const nbuffers{3};
@@ -914,7 +914,7 @@
     EXPECT_CALL(*mock_display_listener, add_display(_))
         .WillRepeatedly(Throw(std::runtime_error("Failed to add display")));
 
-    compositor.start();
+    EXPECT_THROW(compositor.start(), std::runtime_error);
 }
 
 //LP: 1481418