Merge lp:~mir-team/mir/introduce-scene-observer into lp:mir

[Robert Carr]

Introduce a mir::scene::Observer interface as the top level entrypoint to scene observation.

An alternative to:

https://code.launchpad.net/~mir-team/mir/multiple-change-callbacks

Which better mirrors the other interfaces and supports the future needs of the compositor.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1529
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1343/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1619/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1617/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1192/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1075/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1080/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1193/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1530
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1345/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1623
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1621
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1077
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1077/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1082
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1082/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1197
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1197/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1107
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6073

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good overall.

Nits:

29 + virtual void add_observer(std::shared_ptr<scene::Observer> const& observer) = 0;
30 + virtual void remove_observer(std::shared_ptr<scene::Observer> const& observer) = 0;

Adding or removing an observer from within a scene::Observer can cause a deadlock. It would be good to document this.

442 + std::unique_lock<decltype(mutex)> lg(mutex);

We should prefer std::lock_guard<> which is slighly more efficient.

review: Approve

[Kevin DuBois (kdub)]

+1! I hope that in the future the MultiThreadedCompositor's threads subscribe themselves as observers and are a bit more intelligent about when they elect to recomposite as the Scene changes.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1533
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1349/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1637
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1635/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1210/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1081/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1086/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1211/console

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

~~~
784 + scene->throw_on_add_observer(true);
~~~

The test needs to be modified so starts throws to check for cleanup. The above won't do it anymore.

~~~
427 + std::unique_lock<decltype(mutex)> lg(mutex);
428 +
429 + for (auto observer : observers)
430 + observer->surface_added(surface);

and the rest...
~~~

During a callback if an observer modifies the scene either directly or indirectly (which could be subtle) you can end up in deadlock. Maybe copy the observer list and release the lock before calling back?

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

I'm happy with this approach, but...

491 + void remove_observer(std::shared_ptr<Observer> const& observer);

I think that the observed object should own the observer and that the client code should only ever need a weak_ptr (and then only if it intended to remove it at some point). Accordingly this signature should be:

   void remove_observer(std::weak_ptr<Observer> const& observer);

~~~~

124 +// A simple implementation of surface observer which forwards all changes to a provided callback.
125 +// Also installs surface observers on each added surface which in turn forward each change to
126 +// said callback.
127 +class SimpleObserver : public Observer

I think a better name would be LegacySceneChangeNotification (c.f. LegacySurfaceChangeNotification)

A (pre-existing and non-blocking) issue I've just noticed is that we don't remove SurfaceObservers when surfaces are removed from the scene - so that *if* surfaces were repeatedly added and removed then we'd have an increasing number of them.

~~~~

74 + virtual void surface_added(std::shared_ptr<Surface> const& surface) = 0;
75 + virtual void surface_removed(std::shared_ptr<Surface> const& surface) = 0;

I'm not (yet) convinced that observers should be given shared ownership like this. I note that a pointer/reference to the surface is needed in SimpleObserver, but the ownership transfer implied by shared_ptr worries me.

~~~~

We'll likely want to supply more information about reordering than the current interface provides for.

review: Needs Fixing

=== modified file 'include/server/mir/compositor/scene.h'
--- include/server/mir/compositor/scene.h	2014-04-15 05:31:19 +0000
+++ include/server/mir/compositor/scene.h	2014-04-23 22:49:30 +0000
@@ -27,6 +27,11 @@
 
 namespace mir
 {
+namespace scene
+{
+class Observer;
+}
+
 namespace compositor
 {
 
@@ -41,15 +46,8 @@
      */
     virtual graphics::RenderableList generate_renderable_list() const = 0;
 
-    /**
-     * Sets a callback to be called whenever the state of the
-     * Scene changes.
-     *
-     * The supplied callback should not directly or indirectly (e.g.,
-     * by changing a property of a surface) change the state of
-     * the Scene, otherwise a deadlock may occur.
-     */
-    virtual void set_change_callback(std::function<void()> const& f) = 0;
+    virtual void add_observer(std::shared_ptr<scene::Observer> const& observer) = 0;
+    virtual void remove_observer(std::weak_ptr<scene::Observer> const& observer) = 0;
 
 protected:
     Scene() = default;

=== added file 'include/server/mir/scene/legacy_scene_change_notification.h'
--- include/server/mir/scene/legacy_scene_change_notification.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/scene/legacy_scene_change_notification.h	2014-04-23 22:49:30 +0000
@@ -0,0 +1,58 @@
+/*
+ * 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: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_SIMPLE_OBSERVER_H_
+#define MIR_SCENE_SIMPLE_OBSERVER_H_
+
+#include "mir/scene/observer.h"
+
+#include <functional>
+#include <map>
+#include <mutex>
+
+namespace mir
+{
+namespace scene
+{
+class SurfaceObserver;
+
+// A simple implementation of surface observer which forwards all changes to a provided callback.
+// Also installs surface observers on each added surface which in turn forward each change to 
+// said callback.
+class LegacySceneChangeNotification : public Observer
+{
+public:
+    LegacySceneChangeNotification(std::function<void()> const& notify_change);
+    ~LegacySceneChangeNotification();
+
+    void surface_added(std::shared_ptr<Surface> const& surface) override;
+    void surface_removed(std::shared_ptr<Surface> const& surface) override;
+    void surfaces_reordered() override;
+
+private:
+    std::function<void()> notify_change;
+    
+    std::mutex surface_observers_guard;
+    std::map<std::weak_ptr<Surface>, std::shared_ptr<SurfaceObserver>,
+        std::owner_less<std::weak_ptr<Surface>>> surface_observers;
+};
+
+}
+} // namespace mir
+
+#endif // MIR_SCENE_SIMPLE_OBSERVER_H_

=== added file 'include/server/mir/scene/observer.h'
--- include/server/mir/scene/observer.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/scene/observer.h	2014-04-23 22:49:30 +0000
@@ -0,0 +1,50 @@
+/*
+ * 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: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_OBSERVER_H_
+#define MIR_SCENE_OBSERVER_H_
+
+#include <memory>
+
+namespace mir
+{
+namespace scene
+{
+class Surface;
+
+/// An observer for top level notifications of scene changes. In order
+/// to receive more granular change notifications a user may install
+/// mir::scene::SurfaceObserver in surface_added.
+class Observer
+{
+public:
+    virtual void surface_added(std::shared_ptr<Surface> const& surface) = 0;
+    virtual void surface_removed(std::shared_ptr<Surface> const& surface) = 0;
+    virtual void surfaces_reordered() = 0;
+
+protected:
+    Observer() = default;
+    virtual ~Observer() = default;
+    Observer(Observer const&) = delete;
+    Observer& operator=(Observer const&) = delete;
+};
+
+}
+} // namespace mir
+
+#endif // MIR_SCENE_OBSERVER_H_

=== modified file 'include/test/mir_test_doubles/mock_scene.h'
--- include/test/mir_test_doubles/mock_scene.h	2014-04-15 05:31:19 +0000
+++ include/test/mir_test_doubles/mock_scene.h	2014-04-23 22:49:30 +0000
@@ -38,9 +38,9 @@
             .WillByDefault(testing::Return(graphics::RenderableList{}));
     }
     MOCK_CONST_METHOD0(generate_renderable_list, graphics::RenderableList());
-    MOCK_METHOD1(set_change_callback, void(std::function<void()> const&));
-    MOCK_METHOD0(lock, void());
-    MOCK_METHOD0(unlock, void());
+    
+    MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::Observer> const&));
+    MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::Observer> const&));
 };
 
 } // namespace doubles

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2014-04-15 05:31:19 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2014-04-23 22:49:30 +0000
@@ -63,6 +63,7 @@
     MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
     MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));
     MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
+    MOCK_METHOD1(remove_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
 };
 
 }

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-04-17 01:34:35 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-04-23 22:49:30 +0000
@@ -23,6 +23,9 @@
 #include "mir/compositor/display_buffer_compositor_factory.h"
 #include "mir/compositor/scene.h"
 #include "mir/compositor/compositor_report.h"
+#include "mir/scene/legacy_scene_change_notification.h"
+#include "mir/scene/surface_observer.h"
+#include "mir/scene/surface.h"
 
 #include <thread>
 #include <condition_variable>
@@ -31,6 +34,7 @@
 
 namespace mc = mir::compositor;
 namespace mg = mir::graphics;
+namespace ms = mir::scene;
 
 namespace
 {
@@ -259,6 +263,7 @@
 void mc::MultiThreadedCompositor::schedule_compositing()
 {
     std::unique_lock<std::mutex> lk(state_guard);
+
     report->scheduled();
     for (auto& f : thread_functors)
         f->schedule_compositing();
@@ -282,8 +287,7 @@
         }};
 
     lk.unlock();
-    /* Recomposite whenever the scene changes */
-    scene->set_change_callback([this]() { schedule_compositing(); });
+    add_observer();
     lk.lock();
 
     create_compositing_threads();
@@ -314,7 +318,7 @@
         }};
 
     lk.unlock();
-    scene->set_change_callback([]{});
+    scene->remove_observer(observer);
     lk.lock();
 
     destroy_compositing_threads(lk);
@@ -370,3 +374,16 @@
 
     state = CompositorState::stopped;
 }
+
+void mc::MultiThreadedCompositor::add_observer()
+{
+    auto strong_observer = std::make_shared<ms::LegacySceneChangeNotification>([this]()
+        {
+            /* Recomposite whenever the scene changes */
+            schedule_compositing();
+        });
+    observer = strong_observer;
+
+    scene->add_observer(strong_observer);
+}
+

=== modified file 'src/server/compositor/multi_threaded_compositor.h'
--- src/server/compositor/multi_threaded_compositor.h	2014-04-14 18:34:06 +0000
+++ src/server/compositor/multi_threaded_compositor.h	2014-04-23 22:49:30 +0000
@@ -32,6 +32,11 @@
 {
 class Display;
 }
+namespace scene
+{
+class Observer;
+}
+
 namespace compositor
 {
 
@@ -78,6 +83,9 @@
     bool compose_on_start;
 
     void schedule_compositing();
+    
+    void add_observer();
+    std::weak_ptr<mir::scene::Observer> observer;
 };
 
 }

=== modified file 'src/server/scene/CMakeLists.txt'
--- src/server/scene/CMakeLists.txt	2014-04-17 13:55:46 +0000
+++ src/server/scene/CMakeLists.txt	2014-04-23 22:49:30 +0000
@@ -8,7 +8,6 @@
   default_session_container.cpp
   gl_pixel_buffer.cpp
   global_event_sender.cpp
-  legacy_surface_change_notification.cpp
   mediating_display_changer.cpp
   session_manager.cpp
   surface_allocator.cpp
@@ -18,4 +17,6 @@
   surface_event_source.cpp
   surface_observer.cpp
   threaded_snapshot_strategy.cpp
+  legacy_scene_change_notification.cpp
+  legacy_surface_change_notification.cpp
 )

=== added file 'src/server/scene/legacy_scene_change_notification.cpp'
--- src/server/scene/legacy_scene_change_notification.cpp	1970-01-01 00:00:00 +0000
+++ src/server/scene/legacy_scene_change_notification.cpp	2014-04-23 22:49:30 +0000
@@ -0,0 +1,71 @@
+/*
+ * 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: Robert Carr <[e-mail]>
+ */
+
+#include "legacy_surface_change_notification.h"
+
+#include "mir/scene/legacy_scene_change_notification.h"
+#include "mir/scene/surface.h"
+
+#include <boost/throw_exception.hpp>
+
+namespace ms = mir::scene;
+
+ms::LegacySceneChangeNotification::LegacySceneChangeNotification(std::function<void()> const& notify_change)
+    : notify_change(notify_change)
+{
+}
+
+ms::LegacySceneChangeNotification::~LegacySceneChangeNotification()
+{
+    std::unique_lock<decltype(surface_observers_guard)> lg(surface_observers_guard);
+    for (auto &kv : surface_observers)
+    {
+        auto surface = kv.first.lock();
+        if (surface)
+            surface->remove_observer(kv.second);
+    }
+}
+
+void ms::LegacySceneChangeNotification::surface_added(std::shared_ptr<ms::Surface> const& surface)
+{
+    auto observer = std::make_shared<ms::LegacySurfaceChangeNotification>(notify_change);
+    surface->add_observer(observer);
+    
+    {
+        std::unique_lock<decltype(surface_observers_guard)> lg(surface_observers_guard);
+        surface_observers[surface] = observer;
+    }
+    notify_change();
+}
+    
+void ms::LegacySceneChangeNotification::surface_removed(std::shared_ptr<ms::Surface> const& surface)
+{
+    {
+        std::unique_lock<decltype(surface_observers_guard)> lg(surface_observers_guard);
+        auto it = surface_observers.find(surface);
+        if (it != surface_observers.end())
+            surface_observers.erase(it);
+    }
+    notify_change();
+}
+
+void ms::LegacySceneChangeNotification::surfaces_reordered()
+{
+    notify_change();
+}
+

=== modified file 'src/server/scene/surface_stack.cpp'
--- src/server/scene/surface_stack.cpp	2014-04-22 21:37:27 +0000
+++ src/server/scene/surface_stack.cpp	2014-04-23 22:49:30 +0000
@@ -23,7 +23,6 @@
 #include "surface_stack.h"
 #include "mir/compositor/buffer_stream.h"
 #include "mir/scene/input_registrar.h"
-#include "legacy_surface_change_notification.h"
 #include "mir/input/input_channel_factory.h"
 #include "mir/scene/scene_report.h"
 
@@ -52,9 +51,7 @@
     std::shared_ptr<InputRegistrar> const& input_registrar,
     std::shared_ptr<SceneReport> const& report) :
     input_registrar{input_registrar},
-    report{report},
-    change_cb{[this]() { emit_change_notification(); }},
-    notify_change{[]{}}
+    report{report}
 {
 }
 
@@ -125,13 +122,6 @@
     return list;
 }
 
-void ms::SurfaceStack::set_change_callback(std::function<void()> const& f)
-{
-    std::lock_guard<decltype(notify_change_mutex)> lg(notify_change_mutex);
-    assert(f);
-    notify_change = f;
-}
-
 void ms::SurfaceStack::add_surface(
     std::shared_ptr<Surface> const& surface,
     DepthId depth,
@@ -142,10 +132,9 @@
         layers_by_depth[depth].push_back(surface);
     }
     input_registrar->input_channel_opened(surface->input_channel(), surface, input_mode);
+    observers.surface_added(surface);
+
     report->surface_added(surface.get(), surface.get()->name());
-    auto const observer = std::make_shared<LegacySurfaceChangeNotification>(change_cb);
-    surface->add_observer(observer);
-    emit_change_notification();
 }
 
 
@@ -174,12 +163,14 @@
     if (found_surface)
     {
         input_registrar->input_channel_closed(keep_alive->input_channel());
+        observers.surface_removed(keep_alive);
+
         report->surface_removed(keep_alive.get(), keep_alive.get()->name());
-        emit_change_notification();
     }
     // TODO: error logging when surface not found
 }
 
+<<<<<<< TREE
 void ms::SurfaceStack::emit_change_notification()
 {
     std::lock_guard<decltype(notify_change_mutex)> lg(notify_change_mutex);
@@ -187,6 +178,9 @@
 }
 
 void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::Surface> const&)> const& callback)
+=======
+void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::InputChannel> const&)> const& callback)
+>>>>>>> MERGE-SOURCE
 {
     std::lock_guard<decltype(guard)> lg(guard);
     for (auto &layer : layers_by_depth)
@@ -213,7 +207,7 @@
                 surfaces.push_back(surface);
 
                 ul.unlock();
-                emit_change_notification();
+                observers.surfaces_reordered();
 
                 return;
             }
@@ -222,3 +216,69 @@
 
     BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
 }
+
+void ms::SurfaceStack::add_observer(std::shared_ptr<ms::Observer> const& observer)
+{
+    observers.add_observer(observer);
+
+    // Notify observer of existing surfaces
+    {
+        std::unique_lock<decltype(guard)> ul(guard);
+        for (auto &layer : layers_by_depth)
+        {
+            for (auto &surface : layer.second)
+                observer->surface_added(surface);
+        }
+    }
+}
+
+void ms::SurfaceStack::remove_observer(std::weak_ptr<ms::Observer> const& observer)
+{
+    observers.remove_observer(observer);
+}
+
+void ms::Observers::surface_added(std::shared_ptr<ms::Surface> const& surface) 
+{
+    std::unique_lock<decltype(mutex)> lg(mutex);
+    
+    for (auto observer : observers)
+        observer->surface_added(surface);
+}
+
+void ms::Observers::surface_removed(std::shared_ptr<ms::Surface> const& surface)
+{
+    std::unique_lock<decltype(mutex)> lg(mutex);
+
+    for (auto observer : observers)
+        observer->surface_removed(surface);
+}
+
+void ms::Observers::surfaces_reordered()
+{
+    std::unique_lock<decltype(mutex)> lg(mutex);
+    
+    for (auto observer : observers)
+        observer->surfaces_reordered();
+}
+
+void ms::Observers::add_observer(std::shared_ptr<ms::Observer> const& observer)
+{
+    std::unique_lock<decltype(mutex)> lg(mutex);
+
+    observers.push_back(observer);
+}
+
+void ms::Observers::remove_observer(std::weak_ptr<ms::Observer> const& observer)
+{
+    std::unique_lock<decltype(mutex)> lg(mutex);
+    
+    auto o = observer.lock();
+    if (!o)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid observer (destroyed)"));
+    
+    auto it = std::find(observers.begin(), observers.end(), o);
+    if (it == observers.end())
+        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid observer (not previously added)"));
+    
+    observers.erase(it);
+}

=== modified file 'src/server/scene/surface_stack.h'
--- src/server/scene/surface_stack.h	2014-04-15 16:41:27 +0000
+++ src/server/scene/surface_stack.h	2014-04-23 22:49:30 +0000
@@ -23,6 +23,7 @@
 
 #include "mir/compositor/scene.h"
 #include "mir/scene/depth_id.h"
+#include "mir/scene/observer.h"
 #include "mir/input/input_targets.h"
 
 #include <memory>
@@ -57,6 +58,22 @@
 class BasicSurface;
 class SceneReport;
 
+class Observers : public Observer
+{
+public:
+   // ms::Observer
+   void surface_added(std::shared_ptr<Surface> const& surface) override;
+   void surface_removed(std::shared_ptr<Surface> const& surface) override;
+   void surfaces_reordered();
+
+   void add_observer(std::shared_ptr<Observer> const& observer);
+   void remove_observer(std::weak_ptr<Observer> const& observer);
+
+private:
+    std::mutex mutex;
+    std::vector<std::shared_ptr<Observer>> observers;
+};
+
 class SurfaceStack : public compositor::Scene, public input::InputTargets, public SurfaceStackModel
 {
 public:
@@ -67,7 +84,6 @@
 
     // From Scene
     graphics::RenderableList generate_renderable_list() const;
-    virtual void set_change_callback(std::function<void()> const& f);
     
     // From InputTargets
     void for_each(std::function<void(std::shared_ptr<input::Surface> const&)> const& callback);
@@ -80,23 +96,23 @@
         std::shared_ptr<Surface> const& surface,
         DepthId depth,
         input::InputReceptionMode input_mode) override;
+    
+    void add_observer(std::shared_ptr<Observer> const& observer) override;
+    void remove_observer(std::weak_ptr<Observer> const& observer) override;
 
 private:
     SurfaceStack(const SurfaceStack&) = delete;
     SurfaceStack& operator=(const SurfaceStack&) = delete;
 
-    void emit_change_notification();
-
     std::mutex mutable guard;
+
     std::shared_ptr<InputRegistrar> const input_registrar;
     std::shared_ptr<SceneReport> const report;
-    std::function<void()> const change_cb;
 
     typedef std::vector<std::shared_ptr<Surface>> Layer;
     std::map<DepthId, Layer> layers_by_depth;
 
-    std::mutex notify_change_mutex;
-    std::function<void()> notify_change;
+    Observers observers;
 };
 
 }

=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
--- tests/integration-tests/test_surface_first_frame_sync.cpp	2014-04-16 03:38:42 +0000
+++ tests/integration-tests/test_surface_first_frame_sync.cpp	2014-04-23 22:49:30 +0000
@@ -24,6 +24,7 @@
 #include "mir/compositor/display_buffer_compositor.h"
 #include "mir/compositor/display_buffer_compositor_factory.h"
 #include "mir/compositor/scene.h"
+#include "mir/scene/legacy_scene_change_notification.h"
 
 #include "mir_test_framework/display_server_test_fixture.h"
 #include "mir_test_doubles/null_display.h"
@@ -54,10 +55,10 @@
 class SynchronousCompositor : public mc::Compositor
 {
 public:
-    SynchronousCompositor(std::shared_ptr<mg::Display> const& display,
+    SynchronousCompositor(std::shared_ptr<mg::Display> const& display_,
                           std::shared_ptr<mc::Scene> const& scene,
                           std::shared_ptr<mc::DisplayBufferCompositorFactory> const& db_compositor_factory)
-        : display{display},
+        : display{display_},
           scene{scene}
     {
         display->for_each_display_buffer(
@@ -65,28 +66,31 @@
             {
                 display_buffer_compositor_map[&display_buffer] = db_compositor_factory->create_compositor_for(display_buffer);
             });
-}
+        
+        observer = std::make_shared<ms::LegacySceneChangeNotification>([this]() {
+                display->for_each_display_buffer([this](mg::DisplayBuffer& display_buffer)
+                    {  
+                        display_buffer_compositor_map[&display_buffer]->composite();
+                    });
+            });
+    }
 
     void start()
     {
-        scene->set_change_callback([this]()
-        {
-            display->for_each_display_buffer([this](mg::DisplayBuffer& display_buffer)
-            {
-                display_buffer_compositor_map[&display_buffer]->composite();
-            });
-        });
+        scene->add_observer(observer);
     }
 
     void stop()
     {
-        scene->set_change_callback([]{});
+        scene->remove_observer(observer);
     }
 
 private:
     std::shared_ptr<mg::Display> const display;
     std::shared_ptr<mc::Scene> const scene;
     std::unordered_map<mg::DisplayBuffer*,std::unique_ptr<mc::DisplayBufferCompositor>> display_buffer_compositor_map;
+    
+    std::shared_ptr<ms::Observer> observer;
 };
 
 class StubRenderer : public mtd::StubRenderer

=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-04-16 03:38:42 +0000
+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-04-23 22:49:30 +0000
@@ -39,9 +39,11 @@
 namespace mg = mir::graphics;
 namespace mc = mir::compositor;
 namespace geom = mir::geometry;
+namespace ms = mir::scene;
+namespace mr = mir::report;
+
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
-namespace mr = mir::report;
 
 namespace
 {
@@ -58,7 +60,12 @@
         return renderlist;
     }
 
-    void set_change_callback(std::function<void()> const&) {}
+    void add_observer(std::shared_ptr<ms::Observer> const& /* observer */) override
+    {
+    }
+    void remove_observer(std::weak_ptr<ms::Observer> const& /* observer */) override
+    {
+    }
 
     void change(mg::RenderableList const& new_renderlist)
     {

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-16 14:49:54 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-23 22:49:30 +0000
@@ -17,10 +17,13 @@
  */
 
 #include "src/server/compositor/multi_threaded_compositor.h"
+#include "src/server/report/null_report_factory.h"
+
 #include "mir/compositor/display_buffer_compositor.h"
 #include "mir/compositor/scene.h"
 #include "mir/compositor/display_buffer_compositor_factory.h"
-#include "src/server/report/null_report_factory.h"
+#include "mir/scene/observer.h"
+
 #include "mir_test_doubles/null_display.h"
 #include "mir_test_doubles/null_display_buffer.h"
 #include "mir_test_doubles/mock_display_buffer.h"
@@ -30,6 +33,8 @@
 #include "mir_test_doubles/null_display_buffer_compositor_factory.h"
 #include "mir_test/spin_wait.h"
 
+#include <boost/throw_exception.hpp>
+
 #include <unordered_map>
 #include <unordered_set>
 #include <thread>
@@ -42,9 +47,10 @@
 
 namespace mc = mir::compositor;
 namespace mg = mir::graphics;
+namespace ms = mir::scene;
+namespace mr = mir::report;
 namespace geom = mir::geometry;
 namespace mtd = mir::test::doubles;
-namespace mr = mir::report;
 
 namespace
 {
@@ -89,9 +95,8 @@
 {
 public:
     StubScene(mg::RenderableList const& list)
-        : callback{[]{}},
-          throw_on_set_callback_{false}, 
-          renderable_list{list}
+        : renderable_list{list},
+          throw_on_add_observer_{false}
     {
     }
     StubScene() : StubScene(mg::RenderableList{}) {}
@@ -101,38 +106,51 @@
         return renderable_list;
     }
 
-    void set_change_callback(std::function<void()> const& f)
-    {
-        if (throw_on_set_callback_)
-            throw std::runtime_error("");
-        std::lock_guard<std::mutex> lock{callback_mutex};
-        assert(f);
-        callback = f;
+    void add_observer(std::shared_ptr<ms::Observer> const& observer_)
+    {
+        std::lock_guard<std::mutex> lock{observer_mutex};
+
+        if (throw_on_add_observer_)
+            BOOST_THROW_EXCEPTION(std::runtime_error(""));
+
+        assert(!observer);
+        observer = observer_;
+    }
+
+    void remove_observer(std::weak_ptr<ms::Observer> const& /* observer */)
+    {
+        std::lock_guard<std::mutex> lock{observer_mutex};
+        observer.reset();
     }
 
     void emit_change_event()
     {
         {
-            std::lock_guard<std::mutex> lock{callback_mutex};
-            callback();
+            std::lock_guard<std::mutex> lock{observer_mutex};
+            
+            // Any old event will do.
+            if (observer)
+                observer->surfaces_reordered();
         }
         /* Reduce run-time under valgrind */
         std::this_thread::yield();
     }
 
-    void throw_on_set_callback(bool flag)
+    void throw_on_add_observer(bool flag)
     {
-        throw_on_set_callback_ = flag;
+        throw_on_add_observer_ = flag;
     }
 
     void lock() {}
     void unlock() {}
 
 private:
-    std::function<void()> callback;
-    std::mutex callback_mutex;
-    bool throw_on_set_callback_;
+    std::mutex observer_mutex;
     mg::RenderableList renderable_list;
+
+    std::shared_ptr<ms::Observer> observer;
+    
+    bool throw_on_add_observer_;
 };
 
 class RecordingDisplayBufferCompositor : public mc::DisplayBufferCompositor
@@ -564,19 +582,22 @@
 
 TEST(MultiThreadedCompositor, double_start_or_stop_ignored)
 {
-    using namespace testing;
+    using namespace ::testing;
 
     unsigned int const nbuffers{3};
     auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
     auto mock_scene = std::make_shared<mtd::MockScene>();
     auto db_compositor_factory = std::make_shared<mtd::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(_))
-        .Times(2);
+    EXPECT_CALL(*mock_scene, add_observer(_))
+        .Times(1);
+    EXPECT_CALL(*mock_scene, remove_observer(_))
+        .Times(1);
     EXPECT_CALL(*mock_scene, generate_renderable_list())
         .Times(AtLeast(0))
         .WillRepeatedly(Return(mg::RenderableList{}));
@@ -632,11 +653,11 @@
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
     mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
 
-    scene->throw_on_set_callback(true);
+    scene->throw_on_add_observer(true);
 
     EXPECT_THROW(compositor.start(), std::runtime_error);
 
-    scene->throw_on_set_callback(false);
+    scene->throw_on_add_observer(false);
 
     /* No point in running the rest of the test if it throws again */
     ASSERT_NO_THROW(compositor.start());

=== modified file 'tests/unit-tests/scene/CMakeLists.txt'
--- tests/unit-tests/scene/CMakeLists.txt	2014-03-06 06:05:17 +0000
+++ tests/unit-tests/scene/CMakeLists.txt	2014-04-23 22:49:30 +0000
@@ -14,6 +14,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_basic_surface.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_stack.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_controller.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_legacy_scene_change_notification.cpp
 )
 
 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)

=== added file 'tests/unit-tests/scene/test_legacy_scene_change_notification.cpp'
--- tests/unit-tests/scene/test_legacy_scene_change_notification.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/scene/test_legacy_scene_change_notification.cpp	2014-04-23 22:49:30 +0000
@@ -0,0 +1,96 @@
+/*
+ * 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: Robert Carr <[e-mail]>
+ */
+
+#include "mir/scene/legacy_scene_change_notification.h"
+#include "mir/scene/surface_observer.h"
+
+#include "mir_test/fake_shared.h"
+#include "mir_test_doubles/mock_surface.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace ms = mir::scene;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
+
+namespace
+{
+struct MockCallback
+{
+    MOCK_METHOD0(invoke, void());
+};
+}
+
+TEST(LegacySceneChangeNotificationTest, fowards_all_observations_to_callback)
+{
+    MockCallback callback;
+    ms::LegacySceneChangeNotification observer([&](){ callback.invoke(); });
+    
+    testing::NiceMock<mtd::MockSurface> surface;
+    
+    EXPECT_CALL(callback, invoke()).Times(3);
+    observer.surface_added(mt::fake_shared(surface));
+    observer.surface_removed(mt::fake_shared(surface));
+    observer.surfaces_reordered();
+}
+
+TEST(LegacySceneChangeNotificationTest, registers_observer_with_surfaces)
+{
+    using namespace ::testing;
+
+    mtd::MockSurface surface;
+    ms::LegacySceneChangeNotification observer([](){});
+    
+    EXPECT_CALL(surface, add_observer(_)).Times(1);
+    observer.surface_added(mt::fake_shared(surface));
+}
+
+TEST(LegacySceneChangeNotification, observes_surface_changes)
+{
+    using namespace ::testing;
+    
+    mtd::MockSurface surface;
+    MockCallback callback;
+    ms::LegacySceneChangeNotification observer([&](){ callback.invoke(); });
+    
+    std::shared_ptr<ms::SurfaceObserver> surface_observer;
+    EXPECT_CALL(surface, add_observer(_)).Times(1)
+        .WillOnce(SaveArg<0>(&surface_observer));
+    
+    // Once for surface added and once for surface change
+    EXPECT_CALL(callback, invoke()).Times(2);
+    observer.surface_added(mt::fake_shared(surface));
+    
+    surface_observer->frame_posted();
+}
+
+TEST(LegacySceneChangeNotification, destroying_observer_unregisters_surface_observers)
+{
+    using namespace ::testing;
+    
+    auto surface = std::make_shared<mtd::MockSurface>();
+    
+    EXPECT_CALL(*surface, add_observer(_)).Times(1);
+    EXPECT_CALL(*surface, remove_observer(_)).Times(1);
+
+    {
+        ms::LegacySceneChangeNotification observer([](){});
+        observer.surface_added(surface);
+    }
+}

=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
--- tests/unit-tests/scene/test_surface_stack.cpp	2014-04-15 05:31:19 +0000
+++ tests/unit-tests/scene/test_surface_stack.cpp	2014-04-23 22:49:30 +0000
@@ -19,6 +19,7 @@
 #include "src/server/scene/surface_stack.h"
 #include "mir/graphics/buffer_properties.h"
 #include "mir/geometry/rectangle.h"
+#include "mir/scene/observer.h"
 #include "mir/scene/surface_creation_parameters.h"
 #include "src/server/report/null_report_factory.h"
 #include "src/server/scene/basic_surface.h"
@@ -80,12 +81,18 @@
     int const c_fd;
 };
 
-class MockCallback
+struct MockCallback
 {
-public:
     MOCK_METHOD0(call, void());
 };
 
+struct MockSceneObserver : public ms::Observer
+{
+    MOCK_METHOD1(surface_added, void(std::shared_ptr<ms::Surface> const&));
+    MOCK_METHOD1(surface_removed, void(std::shared_ptr<ms::Surface> const&));
+    MOCK_METHOD0(surfaces_reordered, void());
+};
+
 struct SurfaceStack : public ::testing::Test
 {
     void SetUp()
@@ -333,6 +340,107 @@
         stack.remove_surface(surface);
 }
 
+TEST_F(SurfaceStack, scene_observer_notified_of_add_and_remove)
+{
+    using namespace ::testing;
+
+    ms::SurfaceStack stack(
+        mt::fake_shared(input_registrar), report);
+    MockSceneObserver observer;
+    
+    InSequence seq;
+    EXPECT_CALL(observer, surface_added(Eq(stub_surface1))).Times(1);
+    EXPECT_CALL(observer, surface_removed(Eq(stub_surface1)))
+        .Times(1);
+    
+    stack.add_observer(mt::fake_shared(observer));
+
+    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
+    stack.remove_surface(stub_surface1);
+}
+
+TEST_F(SurfaceStack, multiple_observers)
+{
+    using namespace ::testing;
+
+    ms::SurfaceStack stack(
+        mt::fake_shared(input_registrar), report);
+    MockSceneObserver observer1, observer2;
+    
+    InSequence seq;
+    EXPECT_CALL(observer1, surface_added(Eq(stub_surface1))).Times(1);
+    EXPECT_CALL(observer2, surface_added(Eq(stub_surface1))).Times(1);
+    
+    stack.add_observer(mt::fake_shared(observer1));
+    stack.add_observer(mt::fake_shared(observer2));
+
+    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
+}
+
+TEST_F(SurfaceStack, remove_scene_observer)
+{
+    using namespace ::testing;
+
+    ms::SurfaceStack stack(
+        mt::fake_shared(input_registrar), report);
+    MockSceneObserver observer;
+    
+    InSequence seq;
+    EXPECT_CALL(observer, surface_added(Eq(stub_surface1))).Times(1);
+    // We remove the scene observer before removing the surface, and thus
+    // expect to NOT see the surface_removed call
+    EXPECT_CALL(observer, surface_removed(Eq(stub_surface1)))
+        .Times(0);
+    
+    stack.add_observer(mt::fake_shared(observer));
+
+    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
+    stack.remove_observer(mt::fake_shared(observer));
+
+    stack.remove_surface(stub_surface1);
+}
+
+// Many clients of the scene observer wish to install surface observers to monitor surface
+// notifications. We offer them a surface_added event for existing surfaces to give them
+// a chance to do this.
+TEST_F(SurfaceStack, scene_observer_informed_of_existing_surfaces)
+{
+    using namespace ::testing;
+
+    using namespace ::testing;
+
+    ms::SurfaceStack stack(
+        mt::fake_shared(input_registrar), report);
+    MockSceneObserver observer;
+    
+    InSequence seq;
+    EXPECT_CALL(observer, surface_added(Eq(stub_surface1))).Times(1);
+    
+    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
+
+    stack.add_observer(mt::fake_shared(observer));
+}
+
+TEST_F(SurfaceStack, surfaces_reordered)
+{
+    using namespace ::testing;
+
+    ms::SurfaceStack stack(
+        mt::fake_shared(input_registrar), report);
+    MockSceneObserver observer;
+    
+    EXPECT_CALL(observer, surface_added(_)).Times(AnyNumber());
+    EXPECT_CALL(observer, surface_removed(_)).Times(AnyNumber());
+
+    EXPECT_CALL(observer, surfaces_reordered()).Times(1);
+    
+    stack.add_observer(mt::fake_shared(observer));
+
+    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
+    stack.add_surface(stub_surface2, default_params.depth, default_params.input_mode);
+    stack.raise(stub_surface1);
+}
+
 TEST_F(SurfaceStack, renderlist_is_snapshot_of_positioning_info)
 {
     size_t num_surfaces{3};