Merge lp:~albaguirre/mir/recycle-compositor-threads into lp:mir

[Alberto Aguirre]

Recycle compositor threads whenever possible (LP: #1362841)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1879
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2563/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1550
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1526
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1085
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1085/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/463
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/463/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2603
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12409

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2563/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Sounds like a good-old thread pool pattern [1]. Does it therefore have to be compositor specific? That would seem to limit code reuse.

[1] http://en.wikipedia.org/wiki/Thread_pool_pattern

review: Needs Information

[Daniel van Vugt (vanvugt)]

OK, so two questions right now:

(1) Does the thread pool really need to be compositor-specific? That does limit code reuse.

(2) Given this is a workaround for one specific chipset, isn't "1605 lines (+996/-137)" an overkill? Seems rather large given it's a workaround for just one specific chip. Such workarounds you would hope are temporary, and so should be small so that they might be eliminated in future.

The size of the hammer seems disproportionate to the nail :)

[Alexandros Frantzis (afrantzis)]

If it's feasible I would too like to reduce the changes in the Compositor class. Based on Daniel's ThreadPool observation an alternative idea is:

class ThreadPool
{
// When the shared_ptr is released the thread will be marked as available for reuse
// The 'id' is used to enable reusing threads if the ThreadPool implementation supports it.
virtual std::shared_ptr<std::thread> run(void const* id, std::function<void()> const& functor) = 0;
virtual void clear_unused_threads() = 0;
};

// And either dependency on abstract ThreadPool:
MultiThreadedCompositor(...std::shared_ptr<ThreadPool> const& tp...);
// or it might as well use an object variable (abstract class not necessary) for now if this doesn't hinder testing (probably not a problem for testing since we are currently using threads directly internally anyway):
class MultiThreadedCompositor { ReusableThreadPool thread_pool; };

So MultiThreadedCompositor stays as it is more or less, and reverting to the previous behavior is easy: either revert the thread pool changes in MTC, or use a simple thread pool implementation that doesn't reuse threads.

"Needs discussion"

review: Needs Information

[Alberto Aguirre (albaguirre)]

> OK, so two questions right now:
>
> (1) Does the thread pool really need to be compositor-specific? That does
> limit code reuse.
>
> (2) Given this is a workaround for one specific chipset, isn't "1605 lines
> (+996/-137)" an overkill? Seems rather large given it's a workaround for just
> one specific chip. Such workarounds you would hope are temporary, and so
> should be small so that they might be eliminated in future.
>
> The size of the hammer seems disproportionate to the nail :)

1) No it doesn't need to be. I'll make it generic.

2) I don't think so, since a bulk of the MP is adding tests (which we should never see as overkill).

[Alberto Aguirre (albaguirre)]

> If it's feasible I would too like to reduce the changes in the Compositor
> class. Based on Daniel's ThreadPool observation an alternative idea is:
>
> class ThreadPool
> {
> // When the shared_ptr is released the thread will be marked as available for
> reuse
> // The 'id' is used to enable reusing threads if the ThreadPool implementation
> supports it.
> virtual std::shared_ptr<std::thread> run(void const* id, std::function<void()>
> const& functor) = 0;
> virtual void clear_unused_threads() = 0;
> };
>
> // And either dependency on abstract ThreadPool:
> MultiThreadedCompositor(...std::shared_ptr<ThreadPool> const& tp...);
> // or it might as well use an object variable (abstract class not necessary)
> for now if this doesn't hinder testing (probably not a problem for testing
> since we are currently using threads directly internally anyway):
> class MultiThreadedCompositor { ReusableThreadPool thread_pool; };
>
> So MultiThreadedCompositor stays as it is more or less, and reverting to the
> previous behavior is easy: either revert the thread pool changes in MTC, or
> use a simple thread pool implementation that doesn't reuse threads.
>
> "Needs discussion"

Ok, I'll rework.

[Daniel van Vugt (vanvugt)]

Just being careful what we wish for...

If you do rework it and find the result is more complex/larger than this branch then maybe we should keep this one. A non-generalised solution like this one is easier to remove later if a driver fix or simpler workaround materializes.

Unmerged revisions

1879. By Alberto Aguirre on 2014-08-30

cleanup tests

1878. By Alberto Aguirre on 2014-08-30

merge lp:mir/devel

1877. By Alberto Aguirre on 2014-08-30

Recycle compositor threads whenever possible

=== modified file 'include/test/mir_test/signal.h'
--- include/test/mir_test/signal.h	2014-05-22 20:48:20 +0000
+++ include/test/mir_test/signal.h	2014-08-30 16:57:50 +0000
@@ -37,6 +37,7 @@
 
     void raise();
     bool raised();
+    void reset();
 
     void wait();
     template<typename rep, typename period>

=== added file 'include/test/mir_test_doubles/mock_compositor_loop.h'
--- include/test/mir_test_doubles/mock_compositor_loop.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_doubles/mock_compositor_loop.h	2014-08-30 16:57:50 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright © 2014 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 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_COMPOSITOR_LOOP_H_
+#define MIR_TEST_DOUBLES_MOCK_COMPOSITOR_LOOP_H_
+
+#include "src/server/compositor/compositor_thread.h"
+#include <gmock/gmock.h>
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+class MockCompositorLoop : public compositor::CompositorLoop
+{
+public:
+    MOCK_METHOD0(run, void());
+    MOCK_METHOD0(stop, void());
+    MOCK_METHOD1(schedule_compositing, void(int));
+};
+
+}
+}
+}
+
+#endif

=== added file 'include/test/mir_test_doubles/mock_compositor_thread.h'
--- include/test/mir_test_doubles/mock_compositor_thread.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_doubles/mock_compositor_thread.h	2014-08-30 16:57:50 +0000
@@ -0,0 +1,66 @@
+/*
+ * Copyright © 2014 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 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_COMPOSITOR_THREAD_H_
+#define MIR_TEST_DOUBLES_MOCK_COMPOSITOR_THREAD_H_
+
+#include "src/server/compositor/compositor_thread.h"
+#include <gmock/gmock.h>
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+class MockCompositorThread : public compositor::CompositorThread
+{
+public:
+    MockCompositorThread(std::unique_ptr<compositor::CompositorLoop>& loop)
+        : compositor::CompositorThread(std::move(loop))
+    {
+    }
+
+    ~MockCompositorThread()
+    {
+        destroyed();
+    }
+
+    void run(std::unique_ptr<compositor::CompositorLoop> loop) override
+    {
+        run_();
+        compositor::CompositorThread::run(std::move(loop));
+    }
+
+    void pause() override
+    {
+        pause_();
+        compositor::CompositorThread::pause();
+    }
+
+    MOCK_METHOD0(run_, void());
+    MOCK_METHOD0(pause_, void());
+    MOCK_METHOD0(destroyed, void());
+};
+
+}
+}
+}
+
+#endif

=== modified file 'src/server/compositor/CMakeLists.txt'
--- src/server/compositor/CMakeLists.txt	2014-08-08 02:41:51 +0000
+++ src/server/compositor/CMakeLists.txt	2014-08-30 16:57:50 +0000
@@ -16,6 +16,8 @@
   timeout_frame_dropping_policy_factory.cpp
   buffer_queue.cpp
   recently_used_cache.cpp
+  compositor_thread.cpp
+  compositor_thread_factory.cpp
 )
 
 ADD_LIBRARY(

=== added file 'src/server/compositor/compositor_thread.cpp'
--- src/server/compositor/compositor_thread.cpp	1970-01-01 00:00:00 +0000
+++ src/server/compositor/compositor_thread.cpp	2014-08-30 16:57:50 +0000
@@ -0,0 +1,141 @@
+/*
+ * Copyright © 2014 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,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#include "compositor_thread.h"
+#include "mir/run_mir.h"
+#include "mir/thread_name.h"
+
+#include <boost/throw_exception.hpp>
+
+namespace mc = mir::compositor;
+
+mc::CompositorThread::CompositorThread(std::unique_ptr<mc::CompositorLoop> loop)
+    : compositor_loop{std::move(loop)},
+      state{CompositorThreadState::running},
+      thread{&mc::CompositorThread::thread_entry, this}
+{
+}
+
+mc::CompositorThread::~CompositorThread()
+{
+    stop();
+    if (thread.joinable())
+        thread.join();
+}
+
+void mc::CompositorThread::thread_entry() noexcept  // noexcept is important! (LP: #1237332)
+try
+{
+   mir::set_thread_name("Mir/Comp");
+
+   std::unique_lock<std::mutex> lock{run_mutex};
+   while (state != CompositorThreadState::stopping)
+   {
+       run_cv.wait(lock, [&]{ return state !=  CompositorThreadState::paused; });
+
+       if (state == CompositorThreadState::running)
+       {
+           lock.unlock();
+           compositor_loop->run();
+           lock.lock();
+       }
+
+       if (state == CompositorThreadState::pausing)
+       {
+           state = CompositorThreadState::paused;
+           paused_cv.notify_all();
+       }
+   }
+}
+catch(...)
+{
+    std::lock_guard<std::mutex> lock{run_mutex};
+    state = CompositorThreadState::stopping;
+    compositor_loop = nullptr;
+    paused_cv.notify_all();
+
+    mir::terminate_with_current_exception();
+}
+
+void mc::CompositorThread::run(std::unique_ptr<CompositorLoop> loop)
+{
+    std::lock_guard<std::mutex> lock{run_mutex};
+
+    if (state == CompositorThreadState::running)
+    {
+        BOOST_THROW_EXCEPTION(std::logic_error("Another compositor loop is already running!"));
+    }
+
+    compositor_loop = std::move(loop);
+
+    state = CompositorThreadState::running;
+    run_cv.notify_one();
+}
+
+void mc::CompositorThread::pause()
+{
+    std::unique_lock<std::mutex> lock{run_mutex};
+    pause(lock);
+}
+
+void mc::CompositorThread::stop()
+{
+    std::lock_guard<std::mutex> lock{run_mutex};
+
+    state = CompositorThreadState::stopping;
+
+    if (compositor_loop)
+        compositor_loop->stop();
+
+    run_cv.notify_one();
+}
+
+void mc::CompositorThread::schedule_compositing(int num_frames)
+{
+    std::lock_guard<std::mutex> lock{run_mutex};
+
+    if (state == CompositorThreadState::running)
+    {
+        compositor_loop->schedule_compositing(num_frames);
+    }
+}
+
+bool mc::CompositorThread::is_running() const
+{
+    std::lock_guard<std::mutex> lock{run_mutex};
+    return state == CompositorThreadState::running;
+}
+
+void mc::CompositorThread::pause(std::unique_lock<std::mutex>& lock)
+{
+    if (state != CompositorThreadState::running)
+    {
+        return;
+    }
+
+    state = mc::CompositorThreadState::pausing;
+    compositor_loop->stop();
+    run_cv.notify_one();
+
+    paused_cv.wait(lock, [&]{
+        return state == CompositorThreadState::paused ||
+               state == CompositorThreadState::stopping;
+    });
+
+    compositor_loop = nullptr;
+}

=== added file 'src/server/compositor/compositor_thread.h'
--- src/server/compositor/compositor_thread.h	1970-01-01 00:00:00 +0000
+++ src/server/compositor/compositor_thread.h	2014-08-30 16:57:50 +0000
@@ -0,0 +1,89 @@
+/*
+ * Copyright © 2014 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,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_COMPOSITOR_THREAD_H_
+#define MIR_COMPOSITOR_THREAD_H_
+
+#include "mir/compositor/compositor.h"
+
+#include <mutex>
+#include <memory>
+#include <thread>
+#include <condition_variable>
+
+namespace mir
+{
+namespace compositor
+{
+
+class CompositorLoop
+{
+public:
+    CompositorLoop() = default;
+    virtual ~CompositorLoop() = default;
+
+    virtual void run() = 0;
+    virtual void stop() = 0;
+    virtual void schedule_compositing(int num_frames) = 0;
+
+private:
+    CompositorLoop(CompositorLoop const&) = delete;
+    CompositorLoop& operator=(CompositorLoop const&) = delete;
+};
+
+enum class CompositorThreadState
+{
+    stopping,
+    pausing,
+    paused,
+    running
+};
+
+class CompositorThread
+{
+public:
+    CompositorThread(std::unique_ptr<CompositorLoop> loop);
+    virtual ~CompositorThread();
+
+    virtual void run(std::unique_ptr<CompositorLoop> loop);
+    virtual void pause();
+    virtual void schedule_compositing(int num_frames);
+
+    virtual bool is_running() const;
+
+private:
+    void stop();
+    void thread_entry() noexcept;
+    void pause(std::unique_lock<std::mutex>& lock);
+
+    CompositorThread(CompositorThread&) = delete;
+    CompositorThread(CompositorThread const&) = delete;
+    CompositorThread& operator=(CompositorThread const&) = delete;
+
+    std::unique_ptr<CompositorLoop> compositor_loop;
+
+    CompositorThreadState state;
+    std::mutex mutable run_mutex ;
+    std::condition_variable run_cv;
+    std::condition_variable paused_cv;
+    std::thread thread;
+};
+
+}
+}
+#endif

=== added file 'src/server/compositor/compositor_thread_factory.cpp'
--- src/server/compositor/compositor_thread_factory.cpp	1970-01-01 00:00:00 +0000
+++ src/server/compositor/compositor_thread_factory.cpp	2014-08-30 16:57:50 +0000
@@ -0,0 +1,32 @@
+/*
+ * Copyright © 2014 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,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#include "compositor_thread_factory.h"
+#include "compositor_thread.h"
+
+namespace mc = mir::compositor;
+
+mc::CompositorThreadFactory::CompositorThreadFactory() = default;
+mc::CompositorThreadFactory::~CompositorThreadFactory() = default;
+
+std::unique_ptr<mc::CompositorThread>
+mc::CompositorThreadFactory::create_compositor_thread_for(std::unique_ptr<mc::CompositorLoop> loop)
+{
+    std::unique_ptr<CompositorThread> compositor_thread{new CompositorThread(std::move(loop))};
+    return compositor_thread;
+}

=== added file 'src/server/compositor/compositor_thread_factory.h'
--- src/server/compositor/compositor_thread_factory.h	1970-01-01 00:00:00 +0000
+++ src/server/compositor/compositor_thread_factory.h	2014-08-30 16:57:50 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright © 2014 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,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_COMPOSITOR_COMPOSITOR_THREAD_FACTORY_H_
+#define MIR_COMPOSITOR_COMPOSITOR_THREAD_FACTORY_H_
+
+#include <memory>
+
+namespace mir
+{
+namespace compositor
+{
+class CompositorThread;
+class CompositorLoop;
+class CompositorThreadFactory
+{
+public:
+    CompositorThreadFactory();
+    virtual ~CompositorThreadFactory();
+
+    virtual std::unique_ptr<CompositorThread> create_compositor_thread_for(
+            std::unique_ptr<CompositorLoop> loop);
+};
+
+}
+}
+
+
+#endif

=== modified file 'src/server/compositor/default_configuration.cpp'
--- src/server/compositor/default_configuration.cpp	2014-08-06 03:10:56 +0000
+++ src/server/compositor/default_configuration.cpp	2014-08-30 16:57:50 +0000
@@ -20,6 +20,7 @@
 
 #include "buffer_stream_factory.h"
 #include "default_display_buffer_compositor_factory.h"
+#include "compositor_thread_factory.h"
 #include "multi_threaded_compositor.h"
 #include "gl_renderer_factory.h"
 #include "compositing_screencast.h"
@@ -78,6 +79,7 @@
                 the_display(),
                 the_scene(),
                 the_display_buffer_compositor_factory(),
+                std::make_shared<mc::CompositorThreadFactory>(),
                 the_compositor_report(),
                 !the_options()->is_set(options::host_socket_opt));
         });

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-08-06 03:10:56 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-08-30 16:57:50 +0000
@@ -13,10 +13,14 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
- * Authored by: Alexandros Frantzis <[e-mail]>
+ * Authored by: Alberto Aguirre <[e-mail]>
+ *              Alexandros Frantzis <[e-mail]>
  */
 
 #include "multi_threaded_compositor.h"
+#include "compositor_thread.h"
+#include "compositor_thread_factory.h"
+
 #include "mir/graphics/display.h"
 #include "mir/graphics/display_buffer.h"
 #include "mir/compositor/display_buffer_compositor.h"
@@ -85,12 +89,13 @@
     mg::DisplayBuffer& buffer;
 };
 
-class CompositingFunctor
+class DisplayBufferCompositingLoop : public CompositorLoop
 {
 public:
-    CompositingFunctor(std::shared_ptr<mc::DisplayBufferCompositorFactory> const& db_compositor_factory,
-                       mg::DisplayBuffer& buffer,
-                       std::shared_ptr<CompositorReport> const& report)
+    DisplayBufferCompositingLoop(
+        std::shared_ptr<mc::DisplayBufferCompositorFactory> const& db_compositor_factory,
+        mg::DisplayBuffer& buffer,
+        std::shared_ptr<CompositorReport> const& report)
         : display_buffer_compositor_factory{db_compositor_factory},
           buffer(buffer),
           running{true},
@@ -99,11 +104,8 @@
     {
     }
 
-    void operator()() noexcept  // noexcept is important! (LP: #1237332)
-    try
+    void run()
     {
-        mir::set_thread_name("Mir/Comp");
-
         /*
          * Make the buffer the current rendering target, and release
          * it when the thread is finished.
@@ -148,10 +150,6 @@
             }
         }
     }
-    catch(...)
-    {
-        mir::terminate_with_current_exception();
-    }
 
     void schedule_compositing(int num_frames)
     {
@@ -188,11 +186,13 @@
     std::shared_ptr<mg::Display> const& display,
     std::shared_ptr<mc::Scene> const& scene,
     std::shared_ptr<DisplayBufferCompositorFactory> const& db_compositor_factory,
+    std::shared_ptr<CompositorThreadFactory> const& compositor_thread_factory,
     std::shared_ptr<CompositorReport> const& compositor_report,
     bool compose_on_start)
     : display{display},
       scene{scene},
       display_buffer_compositor_factory{db_compositor_factory},
+      compositor_thread_factory{compositor_thread_factory},
       report{compositor_report},
       state{CompositorState::stopped},
       compose_on_start{compose_on_start}
@@ -218,8 +218,8 @@
     std::unique_lock<std::mutex> lk(state_guard);
 
     report->scheduled();
-    for (auto& f : thread_functors)
-        f->schedule_compositing(num);
+    for (auto& t : threads)
+        t.second->schedule_compositing(num);
 }
 
 void mc::MultiThreadedCompositor::start()
@@ -236,14 +236,15 @@
     /* To cleanup state if any code below throws */
     ApplyIfUnwinding cleanup_if_unwinding{
         [this, &lk]{
-            destroy_compositing_threads(lk);
+            stop_compositing_threads(lk);
+            threads.clear();
         }};
 
     lk.unlock();
     scene->add_observer(observer);
     lk.lock();
 
-    create_compositing_threads();
+    start_compositing_threads();
 
     /* Optional first render */
     if (compose_on_start)
@@ -274,29 +275,49 @@
     scene->remove_observer(observer);
     lk.lock();
 
-    destroy_compositing_threads(lk);
+    stop_compositing_threads(lk);
 
     // If the compositor is restarted we've likely got clients blocked
     // so we will need to schedule compositing immediately
     compose_on_start = true;
 }
 
-void mc::MultiThreadedCompositor::create_compositing_threads()
+void mc::MultiThreadedCompositor::start_compositing_threads()
 {
     /* Start the display buffer compositing threads */
     display->for_each_display_buffer([this](mg::DisplayBuffer& buffer)
     {
-        auto thread_functor_raw = new mc::CompositingFunctor{display_buffer_compositor_factory, buffer, report};
-        auto thread_functor = std::unique_ptr<mc::CompositingFunctor>(thread_functor_raw);
-
-        threads.push_back(std::thread{std::ref(*thread_functor)});
-        thread_functors.push_back(std::move(thread_functor));
+
+        auto loop_raw = new mc::DisplayBufferCompositingLoop{display_buffer_compositor_factory, buffer, report};
+        auto loop = std::unique_ptr<mc::DisplayBufferCompositingLoop>(loop_raw);
+
+        auto it = threads.find(&buffer);
+        if (it != threads.end())
+        {
+            auto& compositor_thread = it->second;
+            compositor_thread->run(std::move(loop));
+        }
+        else
+        {
+            auto compositor_thread = compositor_thread_factory->create_compositor_thread_for(std::move(loop));
+            threads[&buffer] = std::move(compositor_thread);
+        }
     });
 
+    // Some display buffers may not be active anymore - clean up their respective threads.
+    // Note: std::remove_if does not apply to associative containers.
+    for (auto it = threads.begin(), end = threads.end(); it != end;)
+    {
+        if (!it->second->is_running())
+            it = threads.erase(it);
+        else
+            ++it;
+    }
+
     state = CompositorState::started;
 }
 
-void mc::MultiThreadedCompositor::destroy_compositing_threads(std::unique_lock<std::mutex>& lock)
+void mc::MultiThreadedCompositor::stop_compositing_threads(std::unique_lock<std::mutex>& lock)
 {
     /* Could be called during unwinding,
      * ensure the lock is held before changing state
@@ -304,14 +325,8 @@
     if(!lock.owns_lock())
         lock.lock();
 
-    for (auto& f : thread_functors)
-        f->stop();
-
     for (auto& t : threads)
-        t.join();
-
-    thread_functors.clear();
-    threads.clear();
+        t.second->pause();
 
     report->stopped();
 

=== modified file 'src/server/compositor/multi_threaded_compositor.h'
--- src/server/compositor/multi_threaded_compositor.h	2014-08-06 03:10:56 +0000
+++ src/server/compositor/multi_threaded_compositor.h	2014-08-30 16:57:50 +0000
@@ -23,7 +23,7 @@
 
 #include <mutex>
 #include <memory>
-#include <vector>
+#include <map>
 #include <thread>
 
 namespace mir
@@ -31,6 +31,7 @@
 namespace graphics
 {
 class Display;
+class DisplayBuffer;
 }
 namespace scene
 {
@@ -41,8 +42,9 @@
 {
 
 class DisplayBufferCompositorFactory;
-class CompositingFunctor;
+class CompositorThreadFactory;
 class Scene;
+class CompositorThread;
 class CompositorReport;
 
 enum class CompositorState
@@ -59,6 +61,7 @@
     MultiThreadedCompositor(std::shared_ptr<graphics::Display> const& display,
                             std::shared_ptr<Scene> const& scene,
                             std::shared_ptr<DisplayBufferCompositorFactory> const& db_compositor_factory,
+                            std::shared_ptr<CompositorThreadFactory> const& compositor_thread_factory,
                             std::shared_ptr<CompositorReport> const& compositor_report,
                             bool compose_on_start);
     ~MultiThreadedCompositor();
@@ -67,23 +70,22 @@
     void stop();
 
 private:
-    void create_compositing_threads();
-    void destroy_compositing_threads(std::unique_lock<std::mutex>& lock);
+    void start_compositing_threads();
+    void stop_compositing_threads(std::unique_lock<std::mutex>& lock);
 
     std::shared_ptr<graphics::Display> const display;
     std::shared_ptr<Scene> const scene;
     std::shared_ptr<DisplayBufferCompositorFactory> const display_buffer_compositor_factory;
+    std::shared_ptr<CompositorThreadFactory> const compositor_thread_factory;
     std::shared_ptr<CompositorReport> const report;
 
-    std::vector<std::unique_ptr<CompositingFunctor>> thread_functors;
-    std::vector<std::thread> threads;
-
+    std::map<graphics::DisplayBuffer*, std::unique_ptr<CompositorThread>> threads;
     std::mutex state_guard;
     CompositorState state;
     bool compose_on_start;
 
     void schedule_compositing(int number_composites);
-    
+
     std::shared_ptr<mir::scene::Observer> observer;
 };
 

=== modified file 'tests/integration-tests/test_surface_stack_with_compositor.cpp'
--- tests/integration-tests/test_surface_stack_with_compositor.cpp	2014-08-06 03:10:56 +0000
+++ tests/integration-tests/test_surface_stack_with_compositor.cpp	2014-08-30 16:57:50 +0000
@@ -22,6 +22,7 @@
 #include "src/server/compositor/gl_renderer_factory.h"
 #include "src/server/scene/basic_surface.h"
 #include "src/server/compositor/default_display_buffer_compositor_factory.h"
+#include "src/server/compositor/compositor_thread_factory.h"
 #include "src/server/compositor/multi_threaded_compositor.h"
 #include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_buffer_stream.h"
@@ -158,6 +159,7 @@
         mt::fake_shared(stack),
         mt::fake_shared(renderer_factory),
         null_comp_report};
+    mc::CompositorThreadFactory comp_thread_factory;
 };
 }
 
@@ -167,6 +169,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, true);
     mt_compositor.start();
 
@@ -180,6 +183,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
     mt_compositor.start();
 
@@ -193,6 +197,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
     mt_compositor.start();
 
@@ -214,6 +219,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
     mt_compositor.start();
 
@@ -236,6 +242,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
     mt_compositor.start();
 
@@ -256,6 +263,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
     mt_compositor.start();
 
@@ -280,6 +288,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
 
     mt_compositor.start();
@@ -298,6 +307,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
 
     mt_compositor.start();
@@ -318,6 +328,7 @@
         mt::fake_shared(stub_display),
         mt::fake_shared(stack),
         mt::fake_shared(dbc_factory),
+        mt::fake_shared(comp_thread_factory),
         null_comp_report, false);
 
     mt_compositor.start();

=== modified file 'tests/mir_test/signal.cpp'
--- tests/mir_test/signal.cpp	2014-05-22 20:48:20 +0000
+++ tests/mir_test/signal.cpp	2014-08-30 16:57:50 +0000
@@ -44,3 +44,9 @@
     std::unique_lock<decltype(mutex)> lock(mutex);
     cv.wait(lock, [this]() { return signalled; });
 }
+
+void mt::Signal::reset()
+{
+    std::unique_lock<decltype(mutex)> lock(mutex);
+    signalled = false;
+}

=== modified file 'tests/unit-tests/compositor/CMakeLists.txt'
--- tests/unit-tests/compositor/CMakeLists.txt	2014-08-06 03:10:56 +0000
+++ tests/unit-tests/compositor/CMakeLists.txt	2014-08-30 16:57:50 +0000
@@ -10,6 +10,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_screencast_display_buffer.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_compositing_screencast.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_timeout_frame_dropping_policy.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_compositor_thread.cpp
 )
 
 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)

=== added file 'tests/unit-tests/compositor/test_compositor_thread.cpp'
--- tests/unit-tests/compositor/test_compositor_thread.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/compositor/test_compositor_thread.cpp	2014-08-30 16:57:50 +0000
@@ -0,0 +1,256 @@
+/*
+ * Copyright © 2014 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 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alberto Aguirre <[e-mail]>
+ */
+
+#include "src/server/compositor/compositor_thread.h"
+
+#include "mir_test_doubles/mock_compositor_loop.h"
+#include "mir_test/current_thread_name.h"
+#include "mir_test/signal.h"
+#include "mir/raii.h"
+
+#include <memory>
+#include <chrono>
+#include <thread>
+#include <exception>
+#include <csignal>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace mc = mir::compositor;
+
+namespace mt = mir::test;
+namespace mtd = mir::test::doubles;
+
+namespace
+{
+
+extern "C" { typedef void (*sig_handler)(int); }
+volatile sig_handler old_signal_handler = nullptr;
+mt::Signal sigterm_signal;
+
+extern "C" void signal_handler(int sig)
+{
+    if (sig == SIGTERM) {
+        sigterm_signal.raise();
+    }
+
+    signal(sig, old_signal_handler);
+}
+
+extern "C" void ignore_signal(int sig)
+{
+    signal(sig, old_signal_handler);
+}
+
+class StubCompositorLoop : public mc::CompositorLoop
+{
+public:
+    void run() override {}
+    void stop() override {}
+    void schedule_compositing(int) override {}
+};
+
+class CompositorThread : public testing::Test
+{
+public:
+    CompositorThread()
+    {
+        new_mock_loop();
+    }
+
+    void new_mock_loop()
+    {
+        std::unique_ptr<mtd::MockCompositorLoop> loop{new testing::NiceMock<mtd::MockCompositorLoop>()};
+        mock_loop = std::move(loop);
+        ON_CALL(*mock_loop, run())
+            .WillByDefault(InvokeWithoutArgs(this, &CompositorThread::raise_run_signal));
+        run_signal.reset();
+    }
+
+    void raise_run_signal()
+    {
+        run_signal.raise();
+        std::this_thread::yield();
+    }
+
+    std::unique_ptr<mtd::MockCompositorLoop> mock_loop;
+    mt::Signal run_signal;
+};
+}
+
+TEST_F(CompositorThread, runs_immediately_on_construction)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, run())
+        .Times(AnyNumber());
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    run_signal.wait_for(std::chrono::seconds{1});
+    EXPECT_TRUE(run_signal.raised());
+}
+
+TEST_F(CompositorThread, can_pause)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, stop());
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    thread.pause();
+
+    EXPECT_FALSE(thread.is_running());
+}
+
+TEST_F(CompositorThread, only_pauses_when_thread_is_already_running)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, stop())
+        .Times(1);
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    thread.pause();
+
+    EXPECT_FALSE(thread.is_running());
+
+    thread.pause();
+}
+
+TEST_F(CompositorThread, destructor_stops_loop)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, stop());
+    mc::CompositorThread thread{std::move(mock_loop)};
+}
+
+TEST_F(CompositorThread, raises_sigterm_on_thread_exception)
+{
+    using namespace testing;
+
+    sigterm_signal.reset();
+
+    auto const raii = mir::raii::paired_calls(
+        [&]{ old_signal_handler = signal(SIGTERM, signal_handler); },
+        [&]{ signal(SIGTERM, old_signal_handler); });
+
+    EXPECT_CALL(*mock_loop, run())
+        .WillOnce(Throw(std::logic_error("")));
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    sigterm_signal.wait_for(std::chrono::seconds{1});
+    EXPECT_TRUE(sigterm_signal.raised());
+}
+
+TEST_F(CompositorThread, pause_does_not_block_after_thread_exception)
+{
+    using namespace testing;
+
+    auto const raii = mir::raii::paired_calls(
+        [&]{ old_signal_handler = signal(SIGTERM, ignore_signal); },
+        [&]{ signal(SIGTERM, old_signal_handler); });
+
+    auto throw_on_run = [this] {
+        run_signal.raise();
+        throw std::runtime_error("");
+    };
+    EXPECT_CALL(*mock_loop, run())
+        .WillOnce(Invoke(throw_on_run));
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    run_signal.wait_for(std::chrono::seconds{1});
+    EXPECT_TRUE(run_signal.raised());
+
+    thread.pause();
+}
+
+TEST_F(CompositorThread, names_itself)
+{
+    using namespace testing;
+
+    std::string thread_name;
+    auto check_thread_name = [&]{
+        thread_name = mt::current_thread_name();
+        run_signal.raise();
+    };
+
+    EXPECT_CALL(*mock_loop, run())
+        .WillRepeatedly(InvokeWithoutArgs(check_thread_name));
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    run_signal.wait_for(std::chrono::seconds{1});
+
+    EXPECT_TRUE(run_signal.raised());
+    EXPECT_THAT(thread_name, Eq("Mir/Comp"));
+}
+
+TEST_F(CompositorThread, makes_loop_schedule_compositing)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, schedule_compositing(_));
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+    int const arbitrary_num_frames = 1;
+    thread.schedule_compositing(arbitrary_num_frames);
+}
+
+TEST_F(CompositorThread, throws_when_run_invoked_before_pause)
+{
+    using namespace testing;
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    new_mock_loop();
+
+    EXPECT_THROW(thread.run(std::move(mock_loop)), std::logic_error);
+}
+
+TEST_F(CompositorThread, can_run_another_loop)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_loop, run())
+        .Times(AnyNumber());
+
+    mc::CompositorThread thread{std::move(mock_loop)};
+
+    run_signal.wait_for(std::chrono::seconds{1});
+    EXPECT_TRUE(run_signal.raised());
+
+    new_mock_loop();
+
+    EXPECT_FALSE(run_signal.raised());
+
+    EXPECT_CALL(*mock_loop, run())
+        .Times(AnyNumber());
+
+    thread.pause();
+    thread.run(std::move(mock_loop));
+
+    run_signal.wait_for(std::chrono::seconds{1});
+    EXPECT_TRUE(run_signal.raised());
+}

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-08-06 03:10:56 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-08-30 16:57:50 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2014 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 as
@@ -13,11 +13,15 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
- * Authored by: Alexandros Frantzis <[e-mail]>
+ * Authored by:
+ *  Alberto Aguirre <[e-mail]>
+ *  Alexandros Frantzis <[e-mail]>
  */
 
 #include "src/server/compositor/multi_threaded_compositor.h"
 #include "src/server/report/null_report_factory.h"
+#include "src/server/compositor/compositor_thread.h"
+#include "src/server/compositor/compositor_thread_factory.h"
 
 #include "mir/compositor/display_buffer_compositor.h"
 #include "mir/compositor/scene.h"
@@ -31,6 +35,7 @@
 #include "mir_test_doubles/mock_compositor_report.h"
 #include "mir_test_doubles/mock_scene.h"
 #include "mir_test_doubles/stub_scene.h"
+#include "mir_test_doubles/mock_compositor_thread.h"
 
 #include <boost/throw_exception.hpp>
 
@@ -42,6 +47,7 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include "mir_test/gmock_fixes.h"
 
 namespace mc = mir::compositor;
 namespace mg = mir::graphics;
@@ -65,6 +71,12 @@
             f(db);
     }
 
+    void new_buffers(unsigned int nbuffers)
+    {
+        buffers.clear();
+        buffers = std::vector<mtd::NullDisplayBuffer>{nbuffers};
+    }
+
 private:
     std::vector<mtd::NullDisplayBuffer> buffers;
 };
@@ -114,7 +126,7 @@
     {
         {
             std::lock_guard<std::mutex> lock{observer_mutex};
-            
+
             // Any old event will do.
             if (observer)
                 observer->surfaces_reordered();
@@ -315,34 +327,66 @@
     }
 };
 
-class ThreadNameDisplayBufferCompositorFactory : public mc::DisplayBufferCompositorFactory
-{
-public:
-    std::unique_ptr<mc::DisplayBufferCompositor> create_compositor_for(mg::DisplayBuffer&)
-    {
-        auto raw = new RecordingDisplayBufferCompositor{
-            [this]()
-            {
-                std::lock_guard<std::mutex> lock{thread_names_mutex};
-                thread_names.emplace_back(mt::current_thread_name());
-            }};
-        return std::unique_ptr<RecordingDisplayBufferCompositor>(raw);
-    }
-
-    size_t num_thread_names_gathered()
-    {
-        std::lock_guard<std::mutex> lock{thread_names_mutex};
-        return thread_names.size();
-    }
-
-    std::mutex thread_names_mutex;
-    std::vector<std::string> thread_names;
-};
-
 auto const null_report = mr::null_compositor_report();
 unsigned int const composites_per_update{1};
 }
 
+class CompositorThreadMockFactory : public mc::CompositorThreadFactory
+{
+public:
+    CompositorThreadMockFactory() : CompositorThreadMockFactory(
+        [](mtd::MockCompositorThread* t) {
+            //Can't use NiceMock on MockCompositorThread
+            using namespace testing;
+            EXPECT_CALL(*t, run_()).Times(AnyNumber());
+            EXPECT_CALL(*t, pause_()).Times(AnyNumber());
+            EXPECT_CALL(*t, destroyed());
+        })
+    {
+    }
+
+    CompositorThreadMockFactory(std::function<void(mtd::MockCompositorThread*)> notify_on_create)
+        : on_create{notify_on_create},
+          num_thread_deaths_{0}
+    {
+        using namespace testing;
+        ON_CALL(*this, create_thread(_))
+           .WillByDefault(Invoke(this, &CompositorThreadMockFactory::create_thread_));
+    }
+
+    void record_thread_death()
+    {
+        num_thread_deaths_++;
+    }
+
+    int num_thread_deaths() const
+    {
+        return num_thread_deaths_;
+    }
+
+    std::unique_ptr<mc::CompositorThread> create_compositor_thread_for(std::unique_ptr<mc::CompositorLoop> loop) override
+    {
+        return create_thread(loop);
+    }
+
+    std::unique_ptr<mc::CompositorThread> create_thread_(std::unique_ptr<mc::CompositorLoop>& loop)
+    {
+        using namespace testing;
+        auto mock_thread = new mtd::MockCompositorThread(loop);
+        ON_CALL(*mock_thread, destroyed())
+            .WillByDefault(Invoke(this, &CompositorThreadMockFactory::record_thread_death));
+
+        if (on_create)
+            on_create(mock_thread);
+        return std::unique_ptr<mc::CompositorThread>{mock_thread};
+    }
+
+    MOCK_METHOD1(create_thread, std::unique_ptr<mc::CompositorThread>(std::unique_ptr<mc::CompositorLoop>&));
+private:
+    std::function<void(mtd::MockCompositorThread*)> on_create;
+    int num_thread_deaths_;
+};
+
 TEST(MultiThreadedCompositor, compositing_happens_in_different_threads)
 {
     using namespace testing;
@@ -352,7 +396,8 @@
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, true};
 
     compositor.start();
 
@@ -373,9 +418,11 @@
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory =
         std::make_shared<RecordingDisplayBufferCompositorFactory>();
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
     auto mock_report = std::make_shared<mtd::MockCompositorReport>();
     mc::MultiThreadedCompositor compositor{display, scene,
                                            db_compositor_factory,
+                                           thread_factory,
                                            mock_report,
                                            true};
 
@@ -427,7 +474,8 @@
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, true};
 
     // Verify we're actually starting at zero frames
     EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
@@ -487,7 +535,8 @@
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, false};
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, false};
 
     // Verify we're actually starting at zero frames
     ASSERT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
@@ -510,7 +559,8 @@
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, false};
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, false};
 
     compositor.start();
 
@@ -545,7 +595,8 @@
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, true};
 
     compositor.start();
 
@@ -563,8 +614,10 @@
 
     auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
     auto scene = std::make_shared<StubScene>();
-    auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
+    auto db_compositor_factory =
+            std::make_shared<RecordingDisplayBufferCompositorFactory>();
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, thread_factory, null_report, true};
 
     display->for_each_mock_buffer([](mtd::MockDisplayBuffer& mock_buf)
     {
@@ -575,6 +628,10 @@
     });
 
     compositor.start();
+
+    while (!db_compositor_factory->enough_records_gathered(nbuffers, nbuffers))
+        scene->emit_change_event();
+
     compositor.stop();
 }
 
@@ -586,6 +643,7 @@
     auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
     auto mock_scene = std::make_shared<mtd::MockScene>();
     auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
+    auto thread_factory = std::make_shared<mc::CompositorThreadFactory>();
     auto mock_report = std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
 
     EXPECT_CALL(*mock_report, started())
@@ -600,7 +658,7 @@
         .Times(AtLeast(0))
         .WillRepeatedly(Return(mc::SceneElementSequence{}));
 
-    mc::MultiThreadedCompositor compositor{display, mock_scene, db_compositor_factory, mock_report, true};
+    mc::MultiThreadedCompositor compositor{display, mock_scene, db_compositor_factory, thread_factory, mock_report, true};
 
     compositor.start();
     compositor.start();
@@ -608,76 +666,165 @@
     compositor.stop();
 }
 
-TEST(MultiThreadedCompositor, cleans_up_after_throw_in_start)
+TEST(MultiThreadedCompositor, no_threads_created_when_adding_scene_observer_throws)
 {
+    using namespace testing;
+
     unsigned int const nbuffers{3};
-
-    auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
+    auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
-    auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>();
+
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .Times(0);
 
     scene->throw_on_add_observer(true);
-
     EXPECT_THROW(compositor.start(), std::runtime_error);
-
     scene->throw_on_add_observer(false);
-
-    /* No point in running the rest of the test if it throws again */
-    ASSERT_NO_THROW(compositor.start());
-
-    /* The minimum number of records here should be nbuffers *2, since we are checking for
-     * presence of at least one additional rogue compositor thread per display buffer
-     * However to avoid timing considerations like one good thread compositing the display buffer
-     * twice before the rogue thread gets a chance to, an arbitrary number of records are gathered
-     */
-    unsigned int min_number_of_records = 100;
-
-    /* Timeout here in case the exception from setting the scene callback put the compositor
-     * in a bad state that did not allow it to composite (hence no records gathered)
-     */
-    auto time_out = std::chrono::steady_clock::now() + std::chrono::seconds(1);
-    while (!db_compositor_factory->enough_records_gathered(nbuffers, min_number_of_records) &&
-           std::chrono::steady_clock::now() <= time_out)
+}
+
+TEST(MultiThreadedCompositor, cleans_up_allocated_threads_after_throw_during_next_thread_creation)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+
+    auto set_mock_thread_expectations = [&](mtd::MockCompositorThread* t) {
+        EXPECT_CALL(*t, pause_());
+        EXPECT_CALL(*t, destroyed());
+    };
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>(set_mock_thread_expectations);
+
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .WillOnce(Invoke(mock_thread_factory.get(), &CompositorThreadMockFactory::create_thread_))
+        .WillOnce(Invoke(mock_thread_factory.get(), &CompositorThreadMockFactory::create_thread_))
+        .WillOnce(Throw(std::runtime_error("")));
+
+    EXPECT_THROW(compositor.start(), std::runtime_error);
+    EXPECT_THAT(mock_thread_factory->num_thread_deaths(), Eq(2));
+}
+
+TEST(MultiThreadedCompositor, recycles_threads)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+
+    auto set_mock_thread_expectations = [&](mtd::MockCompositorThread* t) {
+        //Run is only called when a thread has been recycled
+        EXPECT_CALL(*t, run_());
+        EXPECT_CALL(*t, destroyed());
+        EXPECT_CALL(*t, pause_())
+            .Times(2);
+    };
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>(set_mock_thread_expectations);
+
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .Times(nbuffers);
+
+    compositor.start();
+    compositor.stop();
+    compositor.start();
+    compositor.stop();
+
+    EXPECT_THAT(mock_thread_factory->num_thread_deaths(), Eq(0));
+}
+
+TEST(MultiThreadedCompositor, allocates_new_threads_for_new_display_buffers)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>();
+
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .Times(nbuffers*2);
+
+    compositor.start();
+    compositor.stop();
+
+    display->new_buffers(nbuffers);
+
+    compositor.start();
+    compositor.stop();
+}
+
+TEST(MultiThreadedCompositor, cleans_up_threads_for_unused_display_buffers_at_next_start)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+
+    auto set_mock_thread_expectations = [&](mtd::MockCompositorThread* t) {
+        EXPECT_CALL(*t, run_())
+            .Times(0);
+        EXPECT_CALL(*t, pause_());
+        EXPECT_CALL(*t, destroyed());
+    };
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>(set_mock_thread_expectations);
+
+    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .Times(nbuffers*2);
+
+    compositor.start();
+    compositor.stop();
+
+    display->new_buffers(nbuffers);
+
+    compositor.start();
+
+    EXPECT_THAT(mock_thread_factory->num_thread_deaths(), Eq(nbuffers));
+}
+
+TEST(MultiThreadedCompositor, destructor_cleans_up_threads)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto db_compositor_factory = std::make_shared<SurfaceUpdatingDisplayBufferCompositorFactory>(scene);
+
+    auto set_mock_thread_expectations = [&](mtd::MockCompositorThread* t) {
+        EXPECT_CALL(*t, run_())
+            .Times(0);
+        EXPECT_CALL(*t, pause_());
+        EXPECT_CALL(*t, destroyed());
+    };
+    auto mock_thread_factory = std::make_shared<CompositorThreadMockFactory>(set_mock_thread_expectations);
+
+    EXPECT_CALL(*mock_thread_factory, create_thread(_))
+        .Times(nbuffers);
+
     {
-        scene->emit_change_event();
-        std::this_thread::yield();
+        mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, mock_thread_factory, null_report, true};
+        compositor.start();
     }
 
-    /* Check expectation in case a timeout happened */
-    EXPECT_TRUE(db_compositor_factory->enough_records_gathered(nbuffers, min_number_of_records));
-
-    compositor.stop();
-
-    /* Only one thread should be rendering each display buffer
-     * If the compositor failed to cleanup correctly more than one thread could be
-     * compositing the same display buffer
-     */
-    EXPECT_TRUE(db_compositor_factory->each_buffer_rendered_in_single_thread());
-}
-
-TEST(MultiThreadedCompositor, names_compositor_threads)
-{
-    using namespace testing;
-
-    unsigned int const nbuffers{3};
-
-    auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
-    auto scene = std::make_shared<StubScene>();
-    auto db_compositor_factory = std::make_shared<ThreadNameDisplayBufferCompositorFactory>();
-    mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
-
-    compositor.start();
-
-    unsigned int const min_number_of_thread_names = 10;
-
-    while (db_compositor_factory->num_thread_names_gathered() < min_number_of_thread_names)
-        scene->emit_change_event();
-
-    compositor.stop();
-
-    auto const& thread_names = db_compositor_factory->thread_names;
-
-    for (size_t i = 0; i < thread_names.size(); ++i)
-        EXPECT_THAT(thread_names[i], Eq("Mir/Comp")) << "i=" << i;
+    EXPECT_THAT(mock_thread_factory->num_thread_deaths(), Eq(nbuffers));
 }