Merge lp:~vanvugt/mir/resize-server-Surfaces into lp:mir

[Daniel van Vugt]

Implement resize() for the server-side Surface classes.

This is fine for server-owned surfaces, but won't be fully functional with
clients till they get support for resize() too.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1201
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/338/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/125
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/67
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/67/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/67
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/67/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/338/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

A slight concern:

85 + surface_buffer_stream->resize(size);
86 +
87 + // Now the buffer stream has successfully resized, update the state second;
88 + surface_state->resize(size);

There is an opportunity for inconsistency here, but I am not sure if it has any significant effect:
1. buffer stream gets resized
2. external entity gets surface size which hasn't changed (or has it cached from before, because it hasn't received a notification yet), and makes a decision based on this information
3. external entity gets a new buffer which has size != surface size
4. surface state gets updated

review: Needs Information

[Daniel van Vugt (vanvugt)]

Alexandros,

That inconsistency is something I have planned for right from the beginning.

You cannot solve the inconsistency completely, because that would mean the server has to /wait/ on and rely on the client to fill new buffers quickly. But we don't and shouldn't trust clients.

So what I've done is treat resizes asynchronously. The surface size changes immediately. But its buffers will take a while to catch up (because they're coming from another process that's an unavoidable fact of life). This means there's no blocking and no interruption to what the user sees. Worst case is that you'll be rendering an old buffer size to the new surface size, so might briefly see some scaling happen while the client catches up. I think this is the ideal approach.

[Daniel van Vugt (vanvugt)]

Thus, rather than solve the inconsistency which isn't really feasible to solve, we just handle the fact that during rapid resizing the surface size might be different to the buffer size within it. So long as we're aware that's possible and /normal/, it's very easy to handle.

Also, we may choose to clamp instead of scale... as a matter of cosmetics. I'm not sure which looks best, but all existing windowing systems seem to take the clamp approach.

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

[Alan Griffiths (alan-griffiths)]

LGTM (well nothing about *Surface* really looks good but...)

review: Approve

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-11-01 14:27:00 +0000
+++ include/server/mir/shell/surface.h	2013-11-05 11:12:59 +0000
@@ -89,6 +89,8 @@
 
     virtual void raise(std::shared_ptr<SurfaceController> const& controller);
 
+    virtual void resize(geometry::Size const& size);
+
 private:
     bool set_type(MirSurfaceType t);  // Use configure() to make public changes
     bool set_state(MirSurfaceState s);

=== modified file 'include/server/mir/surfaces/surface.h'
--- include/server/mir/surfaces/surface.h	2013-11-04 09:03:31 +0000
+++ include/server/mir/surfaces/surface.h	2013-11-05 11:12:59 +0000
@@ -90,6 +90,9 @@
     std::shared_ptr<compositor::BufferStream> buffer_stream() const;
 
     std::shared_ptr<input::Surface> input_surface() const;
+
+    void resize(geometry::Size const& size);
+
 private:
     std::shared_ptr<surfaces::SurfaceState> surface_state;
     std::shared_ptr<compositor::BufferStream> surface_buffer_stream;

=== modified file 'include/test/mir_test_doubles/mock_surface_state.h'
--- include/test/mir_test_doubles/mock_surface_state.h	2013-10-07 07:56:05 +0000
+++ include/test/mir_test_doubles/mock_surface_state.h	2013-11-05 11:12:59 +0000
@@ -52,6 +52,7 @@
     MOCK_CONST_METHOD0(name, std::string const&());
     MOCK_METHOD2(apply_rotation, void(float, glm::vec3 const&));
     MOCK_METHOD1(move_to, void(geometry::Point));
+    MOCK_METHOD1(resize, void(geometry::Size const&));
     MOCK_CONST_METHOD1(contains, bool(geometry::Point const&));
     MOCK_METHOD1(set_input_region, void(std::vector<geometry::Rectangle> const&));
     MOCK_CONST_METHOD0(alpha, float());

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-10-15 08:53:10 +0000
+++ src/server/shell/surface.cpp	2013-11-05 11:12:59 +0000
@@ -240,3 +240,8 @@
 {
     controller->raise(surface);
 }
+
+void msh::Surface::resize(geom::Size const& size)
+{
+    surface->resize(size);
+}

=== modified file 'src/server/surfaces/mutable_surface_state.h'
--- src/server/surfaces/mutable_surface_state.h	2013-10-03 06:01:11 +0000
+++ src/server/surfaces/mutable_surface_state.h	2013-11-05 11:12:59 +0000
@@ -32,6 +32,7 @@
 {
 public:
     virtual void move_to(geometry::Point) = 0;
+    virtual void resize(geometry::Size const& size) = 0;
     virtual void frame_posted() = 0;
     virtual void set_hidden(bool hidden) = 0;
     virtual void apply_alpha(float alpha) = 0;

=== modified file 'src/server/surfaces/surface.cpp'
--- src/server/surfaces/surface.cpp	2013-10-23 15:07:55 +0000
+++ src/server/surfaces/surface.cpp	2013-11-05 11:12:59 +0000
@@ -168,3 +168,21 @@
 {
     surface_state->set_input_region(input_rectangles);
 }
+
+void ms::Surface::resize(geom::Size const& size)
+{
+    if (size.width <= geom::Width{0} || size.height <= geom::Height{0})
+    {
+        BOOST_THROW_EXCEPTION(std::logic_error("Impossible resize requested"));
+    }
+    /*
+     * Other combinations may still be invalid (like dimensions too big or
+     * insufficient resources), but those are runtime and platform-specific, so
+     * not predictable here. Such critical exceptions would arise from
+     * the platform buffer allocator as a runtime_error via:
+     */
+    surface_buffer_stream->resize(size);
+
+    // Now the buffer stream has successfully resized, update the state second;
+    surface_state->resize(size);
+}

=== modified file 'src/server/surfaces/surface_data.cpp'
--- src/server/surfaces/surface_data.cpp	2013-10-07 08:25:04 +0000
+++ src/server/surfaces/surface_data.cpp	2013-11-05 11:12:59 +0000
@@ -163,6 +163,16 @@
     notify_change();
 }
 
+void ms::SurfaceData::resize(geom::Size const& size)
+{
+    {
+        std::unique_lock<std::mutex> lock(guard);
+        surface_rect.size = size;
+        transformation_dirty = true;
+    }
+    notify_change();
+}
+
 bool ms::SurfaceData::contains(geom::Point const& point) const
 {
     if (hidden)

=== modified file 'src/server/surfaces/surface_data.h'
--- src/server/surfaces/surface_data.h	2013-10-07 09:53:57 +0000
+++ src/server/surfaces/surface_data.h	2013-11-05 11:12:59 +0000
@@ -51,6 +51,7 @@
 
     //ms::MutableSurfaceState
     void move_to(geometry::Point);
+    void resize(geometry::Size const& size);
     void frame_posted();
     void set_hidden(bool hidden);
     void apply_alpha(float alpha);

=== modified file 'tests/unit-tests/surfaces/test_surface.cpp'
--- tests/unit-tests/surfaces/test_surface.cpp	2013-10-22 16:47:53 +0000
+++ tests/unit-tests/surfaces/test_surface.cpp	2013-11-05 11:12:59 +0000
@@ -283,6 +283,55 @@
     surf.move_to(p);
 }
 
+TEST_F(SurfaceCreation, resize_updates_stream_then_state)
+{
+    using namespace testing;
+    geom::Size const new_size{123, 456};
+
+    Sequence seq;
+    EXPECT_CALL(*mock_buffer_stream, resize(new_size))
+        .InSequence(seq);
+    EXPECT_CALL(*mock_basic_state, resize(new_size))
+        .InSequence(seq);
+
+    ms::Surface surf(
+        mock_basic_state,
+        mock_buffer_stream,
+        std::shared_ptr<mi::InputChannel>(),
+        report);
+    surf.resize(new_size);
+}
+
+TEST_F(SurfaceCreation, impossible_resize_throws)
+{
+    using namespace testing;
+
+    geom::Size const bad_sizes[] =
+    {
+        {0, 123},
+        {456, 0},
+        {-1, -1},
+        {78, -10},
+        {0, 0}
+    };
+
+    ms::Surface surf(
+        mock_basic_state,
+        mock_buffer_stream,
+        std::shared_ptr<mi::InputChannel>(),
+        report);
+
+    EXPECT_CALL(*mock_buffer_stream, resize(size))
+        .Times(0);
+    EXPECT_CALL(*mock_basic_state, resize(size))
+        .Times(0);
+
+    for (auto &size : bad_sizes)
+    {
+        EXPECT_THROW(surf.resize(size), std::logic_error);
+    }
+}
+
 TEST_F(SurfaceCreation, test_surface_set_rotation)
 {
     using namespace testing;

=== modified file 'tests/unit-tests/surfaces/test_surface_data.cpp'
--- tests/unit-tests/surfaces/test_surface_data.cpp	2013-10-07 08:25:04 +0000
+++ tests/unit-tests/surfaces/test_surface_data.cpp	2013-11-05 11:12:59 +0000
@@ -87,6 +87,24 @@
     EXPECT_EQ(new_top_left, storage.position());
 }
 
+TEST_F(SurfaceDataTest, update_size)
+{ 
+    geom::Size const new_size{34, 56};
+
+    EXPECT_CALL(mock_callback, call())
+        .Times(1);
+
+    ms::SurfaceData storage{name, rect, mock_change_cb, false};
+    EXPECT_EQ(rect.size, storage.size());
+    EXPECT_NE(new_size, storage.size());
+
+    auto old_transformation = storage.transformation();
+
+    storage.resize(new_size);
+    EXPECT_EQ(new_size, storage.size());
+    EXPECT_NE(old_transformation, storage.transformation());
+}
+
 TEST_F(SurfaceDataTest, test_surface_set_rotation_updates_transform)
 {
     EXPECT_CALL(mock_callback, call())