Merge lp:~afrantzis/mir/fix-1169186 into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

compositor: Release the display buffers' contexts when stopping the compositor

Release the display buffers' contexts when stopping the compositor, so that
the contexts can be made current to potentially different threads if the
compositor is restarted.

compositor: Release the display buffers' contexts when stopping the compositor

Release the display buffers' contexts when stopping the compositor, so that
the contexts can be made current to potentially different threads if the
compositor is restarted.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:589
http://jenkins.qa.ubuntu.com/job/mir-ci/387/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/266
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/392
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/392/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The functionality looks OK, but I don't think the name is right.

Essentially we've a pair "make_current()/release_current()" - I don't have a good antonym for "current" so I'll suggest "make_not_current()" as an improvement on release_current().

Actually, I suspect "make_current()" of being a bad name too - but am struggling for improvements. "become_current()"? "set_as_current()"?

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> The functionality looks OK, but I don't think the name is right.
>
> Essentially we've a pair "make_current()/release_current()" - I don't have a
> good antonym for "current" so I'll suggest "make_not_current()" as an
> improvement on release_current().
>
> Actually, I suspect "make_current()" of being a bad name too - but am
> struggling for improvements. "become_current()"? "set_as_current()"?

I'd rather keep make_current since it's based on egl/glxMakeCurrent, i.e., it's a well known term in the world of graphics. The term "release" is also widely used in this context, e.g. from the EGL spec: "To release the current context without assigning a new one...", although I am more amenable to changing the "release_current" name.

I agree they are not the most intuitive names, but the big win is that someone familiar with the GL concepts and terminology can instantly recognize them and get a very good idea of what to expect (because they essentially are wrappers around the respective GL functionality).

[Robert Ancell (robert-ancell)]

The naming sounds a little weird to me to, but the functionality looks correct.

The eglMakeCurrent man page describes it as "attach an EGL rendering context to EGL surfaces". So attach () and detach () could be useful method names.

review: Approve

[Alan Griffiths (alan-griffiths)]

> I agree they are not the most intuitive names, but the big win is that someone
> familiar with the GL concepts and terminology can instantly recognize them and
> get a very good idea of what to expect (because they essentially are wrappers
> around the respective GL functionality).

I'll accept the "prior art" argument.

review: Approve

=== modified file 'include/server/mir/graphics/display_buffer.h'
--- include/server/mir/graphics/display_buffer.h	2013-04-12 04:55:11 +0000
+++ include/server/mir/graphics/display_buffer.h	2013-04-15 14:22:45 +0000
@@ -33,6 +33,7 @@
 
     virtual geometry::Rectangle view_area() const = 0;
     virtual void make_current() = 0;
+    virtual void release_current() = 0;
     virtual void clear() = 0;
     virtual bool post_update() = 0;
 

=== modified file 'include/test/mir_test_doubles/mock_display_buffer.h'
--- include/test/mir_test_doubles/mock_display_buffer.h	2013-04-12 04:55:11 +0000
+++ include/test/mir_test_doubles/mock_display_buffer.h	2013-04-15 14:22:45 +0000
@@ -35,6 +35,7 @@
 public:
     MOCK_CONST_METHOD0(view_area, geometry::Rectangle());
     MOCK_METHOD0(make_current, void());
+    MOCK_METHOD0(release_current, void());
     MOCK_METHOD0(clear, void());
     MOCK_METHOD0(post_update, bool());
 };

=== modified file 'include/test/mir_test_doubles/null_display_buffer.h'
--- include/test/mir_test_doubles/null_display_buffer.h	2013-04-12 04:55:11 +0000
+++ include/test/mir_test_doubles/null_display_buffer.h	2013-04-15 14:22:45 +0000
@@ -33,6 +33,7 @@
 public:
     geometry::Rectangle view_area() const { return geometry::Rectangle(); }
     void make_current() {}
+    void release_current() {}
     void clear() {}
     bool post_update() { return true; }
 };

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2013-04-12 04:55:11 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2013-04-15 14:22:45 +0000
@@ -68,6 +68,12 @@
                 lock.lock();
             }
         }
+
+        /*
+         * Release the display buffer from this thread, so that it can be
+         * made current in another thread.
+         */
+        buffer.release_current();
     }
 
     void schedule_compositing()

=== modified file 'src/server/graphics/android/android_display.cpp'
--- src/server/graphics/android/android_display.cpp	2013-04-15 08:48:37 +0000
+++ src/server/graphics/android/android_display.cpp	2013-04-15 14:22:45 +0000
@@ -162,3 +162,8 @@
     if (eglMakeCurrent(egl_display, egl_surface, egl_surface, egl_context) == EGL_FALSE)
         BOOST_THROW_EXCEPTION(std::runtime_error("could not activate surface with eglMakeCurrent\n"));
 }
+
+void mga::AndroidDisplay::release_current()
+{
+    eglMakeCurrent(egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
+}

=== modified file 'src/server/graphics/android/android_display.h'
--- src/server/graphics/android/android_display.h	2013-04-15 08:48:37 +0000
+++ src/server/graphics/android/android_display.h	2013-04-15 14:22:45 +0000
@@ -59,6 +59,8 @@
     void resume();
 
     void make_current();
+    void release_current();
+
 private:
     std::shared_ptr<AndroidFramebufferWindowQuery> native_window;
     EGLDisplay egl_display;

=== modified file 'src/server/graphics/gbm/gbm_display_buffer.cpp'
--- src/server/graphics/gbm/gbm_display_buffer.cpp	2013-04-15 08:48:37 +0000
+++ src/server/graphics/gbm/gbm_display_buffer.cpp	2013-04-15 14:22:45 +0000
@@ -276,6 +276,11 @@
     }
 }
 
+void mgg::GBMDisplayBuffer::release_current()
+{
+    egl.release_current();
+}
+
 void mgg::GBMDisplayBuffer::schedule_set_crtc()
 {
     needs_set_crtc = true;

=== modified file 'src/server/graphics/gbm/gbm_display_buffer.h'
--- src/server/graphics/gbm/gbm_display_buffer.h	2013-04-15 08:48:37 +0000
+++ src/server/graphics/gbm/gbm_display_buffer.h	2013-04-15 14:22:45 +0000
@@ -53,6 +53,7 @@
 
     geometry::Rectangle view_area() const;
     void make_current();
+    void release_current();
     void clear();
     bool post_update();
 

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-04-11 10:56:45 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-04-15 14:22:45 +0000
@@ -21,6 +21,7 @@
 #include "mir/compositor/renderables.h"
 #include "mir/graphics/display.h"
 #include "mir_test_doubles/null_display_buffer.h"
+#include "mir_test_doubles/mock_display_buffer.h"
 
 #include <unordered_map>
 #include <unordered_set>
@@ -65,6 +66,38 @@
     std::vector<mtd::NullDisplayBuffer> buffers;
 };
 
+class StubDisplayWithMockBuffers : public mg::Display
+{
+ public:
+    StubDisplayWithMockBuffers(unsigned int nbuffers) : buffers{nbuffers} {}
+    geom::Rectangle view_area() const { return geom::Rectangle(); }
+    void for_each_display_buffer(std::function<void(mg::DisplayBuffer&)> const& f)
+    {
+        for (auto& db : buffers)
+            f(db);
+    }
+    std::shared_ptr<mg::DisplayConfiguration> configuration()
+    {
+        return std::shared_ptr<mg::DisplayConfiguration>();
+    }
+    void register_pause_resume_handlers(mir::MainLoop&,
+                                        std::function<void()> const&,
+                                        std::function<void()> const&)
+    {
+    }
+    void pause() {}
+    void resume() {}
+
+    void for_each_mock_buffer(std::function<void(mtd::MockDisplayBuffer&)> const& f)
+    {
+        for (auto& db : buffers)
+            f(db);
+    }
+
+private:
+    std::vector<mtd::MockDisplayBuffer> buffers;
+};
+
 class StubRenderables : public mc::Renderables
 {
 public:
@@ -216,6 +249,12 @@
     unsigned int render_count;
 };
 
+class NullCompositingStrategy : public mc::CompositingStrategy
+{
+public:
+    void render(mg::DisplayBuffer&) {}
+};
+
 }
 
 TEST(MultiThreadedCompositor, compositing_happens_in_different_threads)
@@ -303,3 +342,23 @@
 
     compositor.stop();
 }
+
+TEST(MultiThreadedCompositor, releases_display_buffer_context_when_stopping)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers{3};
+
+    auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
+    auto renderables = std::make_shared<StubRenderables>();
+    auto strategy = std::make_shared<NullCompositingStrategy>();
+    mc::MultiThreadedCompositor compositor{display, renderables, strategy};
+
+    display->for_each_mock_buffer([](mtd::MockDisplayBuffer& mock_buf)
+    {
+        EXPECT_CALL(mock_buf, release_current()).Times(1);
+    });
+
+    compositor.start();
+    compositor.stop();
+}