Merge lp:~kdub/mir/compositor-double-start-stop into lp:mir

[Kevin DuBois]

compositor: ignore double requests to start or stop the MultiThreadedCompositor

compositor: ignore double requests to start or stop the MultiThreadedCompositor.

guard against this scenario, because sometimes unity-mir calls this twice in a row, and this class doesn't handle that very well. (we're still working out power policy, so lets guard against mir errors here)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1331
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/660/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/633
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/629
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/386
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/386/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/389
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/389/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/237
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/256
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3002

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

Is that case something we should care about?

TEST(MultiThreadedCompositor, double_threaded_start_ignored)
{
    unsigned int const nbuffers{3};
    auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
    auto mock_scene = std::make_shared<MockScene>();
    auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
    auto mock_report = std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
    EXPECT_CALL(*mock_report, started())
        .Times(1);

    mc::MultiThreadedCompositor compositor{display, mock_scene, db_compositor_factory, mock_report};

    std::thread{[&](){ compositor.start()} };
    std::thread{[&](){ compositor.start()} };
}

review: Needs Information

[Daniel van Vugt (vanvugt)]

Looks good.

Just noting "geometry::Rectangle{{0,0},{0,0}}" is probably better written as "geometry::Rectangle()".

review: Approve

[Alan Griffiths (alan-griffiths)]

I'd expect multiple calls to ::MultiThreadedCompositor::start() to be a precondition violation.

Do we really need to handle what sounds like a bug in unity-mir?

If so, is ignoring it really better than aborting?

review: Needs Information

[Alexandros Frantzis (afrantzis)]

Since code external to mir is now manipulating the compositor I have concerns about coordinating requests and events involving the compositor. MultiThreadedCompositor doesn't handle multiple starts/stops, because in many cases internal synchronization is not enough; starts/stops need to be coordinated at a higher level and are therefore serialized through our main loop.

Allowing arbitrary code to start/stop the compositor is bound to break Mir. Consider, for example, the dire consequences of restarting the compositor while it is disabled for a display reconfiguration event.

I am not familiar with the code that needs to stop/start the compositor in unity-mir, but we probably need to provide a way (interface) for unity-mir to do what it wants, while we do the right thing internally (go through our main loop).

"Needs Discussion"

review: Needs Information

[Kevin DuBois (kdub)]

I agree that calling start twice is a violation of our preconditions, and that exposing the compositor start/stop to unity-mir might break other things in Mir. We should expose something simpler.

This is what unity-mir is doing at the moment though:
http://bazaar.launchpad.net/~mir-team/unity-mir/trunk/view/head:/src/unity-mir/dbusscreen.cpp

And (for reasons I don't know) sometimes start/stop are called twice, which generates "error during eglMakeCurrent", which mir gets blamed for. Perhaps I should change this to throw if start or stop is called twice?

[Alan Griffiths (alan-griffiths)]

Andreas, Alexandros, Kevin and I are all agreed that calling start twice is a precondition violation, so lets take that as our premise.

Ideally preconditions are the responsibility of the calling code so my first inclination would be to fix that.

But as existing code is hitting this condition then a better diagnostic is the least we can do. We can certainly provide better information than "error during eglMakeCurrent"!

In DBusScreen::setScreenPowerMode() it looks as though start() will be called whenever 'mode == "on"' - regardless of the existing mode.

As Alexandros comments though it would be good to understand what the client code is intending - if the compositor really needs to be stopped when reconfiguring the display then it probably shouldn't be client code responsibility.

[Daniel van Vugt (vanvugt)]

I think we're falling on the side of pedantism over practicality. I'd say any interface with "start" and "stop" should do this internal state check, for robustness in two situations:
  (a) The caller doesn't know if it's started yet, but needs it started; and
  (b) The caller doesn't know if it's stopped yet, but needs it stopped.

For the caller to guarantee "preconditions" means each caller must track or query the state of the object being started/stopped, which is dangerous and redundant.

review: Approve

[Kevin DuBois (kdub)]

A good compromise seems to be to keep the changes to make it more robust and warn via the compositor log when this happens. Will work on that tomorrow.

[Gerry Boland (gerboland)]

If unity-mir was the sole consumer of the Compositor::start and Compositor::stop, then it could save the compositor running state itself. However I don't believe that is the case, is it?

If you supplied a "running" method to check for the compositor thread state, then unity-mir would have sufficient information to make the right decision.

However I do also believe a library should try to recover gracefully from mis-use whenever possible. Calling start() twice shouldn't crash.

[Alan Griffiths (alan-griffiths)]

> If unity-mir was the sole consumer of the Compositor::start and
> Compositor::stop, then it could save the compositor running state itself.
> However I don't believe that is the case, is it?

I'm *still* not convinced that unity-mir should be calling Compositor::start()/stop() in this case it smells of being a workaround for a Mir bug.

If justified, it would be possible for unity-mir to track the state correctly by overriding Compositor::start()/stop() and only calling the base version when appropriate.

> If you supplied a "running" method to check for the compositor thread state,
> then unity-mir would have sufficient information to make the right decision.

No it couldn't: start()/stop() could be called on another thread after running() returned.

> However I do also believe a library should try to recover gracefully from mis-
> use whenever possible.

I think that a clearly diagnosable and immediate crash is preferable to ignoring errors only to fail in an obscure way later.

> Calling start() twice shouldn't crash.

That is a matter on which reasonable minds can (and do) differ. But it would be moot if there were no need to call it in USC or unity-mir.

[Alan Griffiths (alan-griffiths)]

If, for the sake of moving forward, we accept the premise of this MP that we can have multiple callers of start() and stop() then we have to allow for the calls to occur on separate threads.

I.e. we need to make these functions thread safe.

review: Needs Fixing

[Kevin DuBois (kdub)]

I don't think this should land in the current form... we should provide unity mir with an easy lever they can push to stop and start the system without having to worry about stopping/starting the compositor and blanking/unblanking the screen. I'll move this to 'wip'

[Kevin DuBois (kdub)]

> I don't think this should land in the current form... we should provide unity
> mir with an easy lever they can push to stop and start the system without
> having to worry about stopping/starting the compositor and blanking/unblanking
> the screen. I'll move this to 'wip'

shortly after saying this, I have to stick my foot in my mouth because this averts a start-up bug for USC.

In investigating the problem that mterry was having with U-S-C today, it turns out what was going on was that we started the compositor when unity starts, and then later powerd will send us the first power on signal, and ignoring double-on calls averts the crash.

So I'll re-propose for now as a band-aid. The proper solution is to give USC/unity8 a class they can tinker with to turn the display server on and off.

[Kevin DuBois (kdub)]

> I.e. we need to make these functions thread safe.

should be threadsafe

[Chris Halse Rogers (raof)]

I'm happy to see this land now; it seems to be a pretty critical fix for our downstreams.

That said, this should be marked as technical debt - I'd suggest a comment on the test saying that it's testing temporary behaviour, and a TODO in start()/stop()?

review: Approve (for now)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1333
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/705/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/694
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/690
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/435
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/435/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/439
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/439/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/296
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/296/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/309
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3238

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

review: Approve (continuous-integration)

=== modified file 'include/test/mir_test_doubles/mock_display_buffer.h'
--- include/test/mir_test_doubles/mock_display_buffer.h	2014-01-17 19:42:09 +0000
+++ include/test/mir_test_doubles/mock_display_buffer.h	2014-01-22 00:23:56 +0000
@@ -33,6 +33,14 @@
 class MockDisplayBuffer : public graphics::DisplayBuffer
 {
 public:
+    MockDisplayBuffer()
+    {
+        using namespace testing;
+        ON_CALL(*this, can_bypass())
+            .WillByDefault(Return(false));
+        ON_CALL(*this, view_area())
+            .WillByDefault(Return(geometry::Rectangle{{0,0},{0,0}}));
+    }
     MOCK_CONST_METHOD0(view_area, geometry::Rectangle());
     MOCK_METHOD0(make_current, void());
     MOCK_METHOD0(release_current, void());

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-01-13 06:12:33 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-01-22 00:23:56 +0000
@@ -152,7 +152,8 @@
     : display{display},
       scene{scene},
       display_buffer_compositor_factory{db_compositor_factory},
-      report{compositor_report}
+      report{compositor_report},
+      started{false}
 {
 }
 
@@ -163,6 +164,12 @@
 
 void mc::MultiThreadedCompositor::start()
 {
+    std::unique_lock<std::mutex> lk(started_guard);
+    if (started)
+    {
+        return;
+    }
+
     report->started();
 
     /* Start the compositing threads */
@@ -186,10 +193,18 @@
     /* First render */
     for (auto& f : thread_functors)
         f->schedule_compositing();
+
+    started = true;
 }
 
 void mc::MultiThreadedCompositor::stop()
 {
+    std::unique_lock<std::mutex> lk(started_guard);
+    if (!started)
+    {
+        return;
+    }
+
     scene->set_change_callback([]{});
 
     for (auto& f : thread_functors)
@@ -202,4 +217,6 @@
     threads.clear();
 
     report->stopped();
+
+    started = false;
 }

=== modified file 'src/server/compositor/multi_threaded_compositor.h'
--- src/server/compositor/multi_threaded_compositor.h	2014-01-13 06:12:33 +0000
+++ src/server/compositor/multi_threaded_compositor.h	2014-01-22 00:23:56 +0000
@@ -21,6 +21,7 @@
 
 #include "mir/compositor/compositor.h"
 
+#include <mutex>
 #include <memory>
 #include <vector>
 #include <thread>
@@ -61,6 +62,9 @@
 
     std::vector<std::unique_ptr<CompositingFunctor>> thread_functors;
     std::vector<std::thread> threads;
+
+    std::mutex started_guard;
+    bool started;
 };
 
 }

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-01-13 06:12:33 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-01-22 00:23:56 +0000
@@ -76,7 +76,7 @@
     }
 
 private:
-    std::vector<mtd::MockDisplayBuffer> buffers;
+    std::vector<testing::NiceMock<mtd::MockDisplayBuffer>> buffers;
 };
 
 class StubScene : public mc::Scene
@@ -115,6 +115,16 @@
     std::mutex callback_mutex;
 };
 
+class MockScene : public mc::Scene
+{
+public:
+    MOCK_METHOD2(for_each_if, void(mc::FilterForScene&, mc::OperatorForScene&));
+    MOCK_METHOD2(reverse_for_each_if, void(mc::FilterForScene&, mc::OperatorForScene&));
+    MOCK_METHOD1(set_change_callback, void(std::function<void()> const&));
+    MOCK_METHOD0(lock, void());
+    MOCK_METHOD0(unlock, void());
+};
+
 class RecordingDisplayBufferCompositor : public mc::DisplayBufferCompositor
 {
 public:
@@ -477,3 +487,25 @@
     compositor.start();
     compositor.stop();
 }
+
+TEST(MultiThreadedCompositor, double_start_or_stop_ignored)
+{
+    unsigned int const nbuffers{3};
+    auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
+    auto mock_scene = std::make_shared<MockScene>();
+    auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
+    auto mock_report = std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
+    EXPECT_CALL(*mock_report, started())
+        .Times(1);
+    EXPECT_CALL(*mock_report, stopped())
+        .Times(1);
+    EXPECT_CALL(*mock_scene, set_change_callback(testing::_))
+        .Times(2);
+
+    mc::MultiThreadedCompositor compositor{display, mock_scene, db_compositor_factory, mock_report};
+
+    compositor.start();
+    compositor.start();
+    compositor.stop();
+    compositor.stop();
+}