Merge lp:~vanvugt/mir/fix-1193020 into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Cache the transformation matrix; only recalculate it when some part of the
transformation changes. (LP: #1193020)

[Thomas Voß (thomas-voss)]

Looks good, a minor niggle, though: If we cache the transformation matrix we should return a const reference instead of a copy. I won't block on it, though, and we can have a follow-up mp.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:771
http://jenkins.qa.ubuntu.com/job/mir-ci/802/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1007
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/892
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/40
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/40/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/478

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

unit tests first in tdd :) I think the behavior with checking transformation_size is correct, but it would be nice to have a test to check the case where the buffer stream's size changes. (esp since it should be a short test)
other than that, seems ok.

[Daniel van Vugt (vanvugt)]

Thomas: Good point about a reference. It would change interfaces though.

Kevin: Good point about TDD. However the tests and testability changed significantly as my approach changed. The design of the tests unfortunately depended very much on the implementation in this case.

[Thomas Voß (thomas-voss)]

LGTM, and Daniel convinced me that the const-qualifier (despite the required mutable's) is a good idea for const callers.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:773
http://jenkins.qa.ubuntu.com/job/mir-ci/814/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1023
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/908
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/52
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/52/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

review: Approve

[Chris Halse Rogers (raof)]

Looks good to me.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/363/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/generic-cleanup-mbs/440/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1025/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/910/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-autolanding/17/console

review: Needs Fixing (continuous-integration)

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/server/mir/graphics/renderable.h'
--- include/server/mir/graphics/renderable.h	2013-05-01 18:07:31 +0000
+++ include/server/mir/graphics/renderable.h	2013-06-25 05:59:27 +0000
@@ -47,7 +47,7 @@
     virtual geometry::Point top_left() const = 0;
     virtual geometry::Size size() const = 0;
     virtual std::shared_ptr<surfaces::GraphicRegion> graphic_region() const = 0;
-    virtual glm::mat4 transformation() const = 0;
+    virtual const glm::mat4& transformation() const = 0;
     virtual float alpha() const = 0;
     virtual bool should_be_rendered() const = 0;
 
 
=== modified file 'include/server/mir/surfaces/surface.h'
--- include/server/mir/surfaces/surface.h	2013-06-24 22:54:22 +0000
+++ include/server/mir/surfaces/surface.h	2013-06-25 05:59:27 +0000
@@ -67,7 +67,7 @@
     geometry::Point top_left() const;
     geometry::Size size() const;
     std::shared_ptr<GraphicRegion> graphic_region() const;
-    glm::mat4 transformation() const;
+    const glm::mat4& transformation() const override;
     float alpha() const;
     bool should_be_rendered() const;
 
@@ -90,6 +90,9 @@
     std::shared_ptr<input::InputChannel> const input_channel;
 
     glm::mat4 rotation_matrix;
+    mutable glm::mat4 transformation_matrix;
+    mutable geometry::Size transformation_size;
+    mutable bool transformation_dirty;
     float alpha_value;
 
     bool is_hidden;
 
=== modified file 'include/test/mir_test_doubles/mock_renderable.h'
--- include/test/mir_test_doubles/mock_renderable.h	2013-05-01 18:07:31 +0000
+++ include/test/mir_test_doubles/mock_renderable.h	2013-06-25 05:59:27 +0000
@@ -44,7 +44,7 @@
     MOCK_CONST_METHOD0(top_left, geometry::Point());
     MOCK_CONST_METHOD0(size, geometry::Size());
     MOCK_CONST_METHOD0(graphic_region, std::shared_ptr<surfaces::GraphicRegion>());
-    MOCK_CONST_METHOD0(transformation, glm::mat4());
+    MOCK_CONST_METHOD0(transformation, const glm::mat4&());
     MOCK_CONST_METHOD0(alpha, float());
     MOCK_CONST_METHOD0(should_be_rendered, bool());
 
 
=== modified file 'src/server/surfaces/surface.cpp'
--- src/server/surfaces/surface.cpp	2013-06-24 22:54:22 +0000
+++ src/server/surfaces/surface.cpp	2013-06-25 05:59:27 +0000
@@ -46,6 +46,7 @@
     top_left_point(top_left),
     buffer_stream(buffer_stream),
     input_channel(input_channel),
+    transformation_dirty(true),
     alpha_value(1.0f),
     is_hidden(false),
     buffer_count(0),
@@ -73,12 +74,14 @@
 void ms::Surface::move_to(geometry::Point const& top_left)
 {
     top_left_point = top_left;
+    transformation_dirty = true;
     notify_change();
 }
 
 void ms::Surface::set_rotation(float degrees, glm::vec3 const& axis)
 {
     rotation_matrix = glm::rotate(glm::mat4{1.0f}, degrees, axis);
+    transformation_dirty = true;
     notify_change();
 }
 
@@ -109,38 +112,43 @@
     return compositor_buffer();
 }
 
-glm::mat4 ms::Surface::transformation() const
+const glm::mat4& ms::Surface::transformation() const
 {
     const geom::Size sz = size();
 
-    const glm::vec3 top_left_vec{top_left_point.x.as_int(),
-                                 top_left_point.y.as_int(),
+    if (transformation_dirty || transformation_size != sz)
+    {
+        const glm::vec3 top_left_vec{top_left_point.x.as_int(),
+                                     top_left_point.y.as_int(),
+                                     0.0f};
+        const glm::vec3 size_vec{sz.width.as_uint32_t(),
+                                 sz.height.as_uint32_t(),
                                  0.0f};
-    const glm::vec3 size_vec{sz.width.as_uint32_t(),
-                             sz.height.as_uint32_t(),
-                             0.0f};
-
-    /* Get the center of the renderable's area */
-    const glm::vec3 center_vec{top_left_vec + 0.5f * size_vec};
-
-    /*
-     * Every renderable is drawn using a 1x1 quad centered at 0,0.
-     * We need to transform and scale that quad to get to its final position
-     * and size.
-     *
-     * 1. We scale the quad vertices (from 1x1 to wxh)
-     * 2. We move the quad to its final position. Note that because the quad
-     *    is centered at (0,0), we need to translate by center_vec, not
-     *    top_left_vec.
-     */
-    glm::mat4 pos_size_matrix;
-    pos_size_matrix = glm::translate(pos_size_matrix, center_vec);
-    pos_size_matrix = glm::scale(pos_size_matrix, size_vec);
-
-    // Rotate, then scale, then translate
-    const glm::mat4 transformation = pos_size_matrix * rotation_matrix;
-
-    return transformation;
+
+        /* Get the center of the renderable's area */
+        const glm::vec3 center_vec{top_left_vec + 0.5f * size_vec};
+
+        /*
+         * Every renderable is drawn using a 1x1 quad centered at 0,0.
+         * We need to transform and scale that quad to get to its final position
+         * and size.
+         *
+         * 1. We scale the quad vertices (from 1x1 to wxh)
+         * 2. We move the quad to its final position. Note that because the quad
+         *    is centered at (0,0), we need to translate by center_vec, not
+         *    top_left_vec.
+         */
+        glm::mat4 pos_size_matrix;
+        pos_size_matrix = glm::translate(pos_size_matrix, center_vec);
+        pos_size_matrix = glm::scale(pos_size_matrix, size_vec);
+
+        // Rotate, then scale, then translate
+        transformation_matrix = pos_size_matrix * rotation_matrix;
+        transformation_size = sz;
+        transformation_dirty = false;
+    }
+
+    return transformation_matrix;
 }
 
 float ms::Surface::alpha() const
 
=== modified file 'tests/unit-tests/graphics/test_gl_renderer.cpp'
--- tests/unit-tests/graphics/test_gl_renderer.cpp	2013-06-12 15:36:31 +0000
+++ tests/unit-tests/graphics/test_gl_renderer.cpp	2013-06-25 05:59:27 +0000
@@ -34,6 +34,7 @@
 using testing::SetArgPointee;
 using testing::InSequence;
 using testing::Return;
+using testing::ReturnRef;
 using testing::_;
 
 namespace mtd=mir::test::doubles;
@@ -277,6 +278,7 @@
     mtd::MockGL         mock_gl;
     mir::geometry::Size display_size;
     std::unique_ptr<mg::GLRenderer> renderer;
+    glm::mat4           trans;
 };
 
 void NullGraphicRegionDeleter(mtd::MockGraphicRegion * /* gr */)
@@ -303,7 +305,6 @@
 
     mir::geometry::Point tl;
     mir::geometry::Size  s;
-    glm::mat4            transformation;
 
     tl.x = mir::geometry::X(1);
     tl.y = mir::geometry::Y(2);
@@ -319,7 +320,7 @@
     EXPECT_CALL(mock_gl, glActiveTexture(GL_TEXTURE0));
 
     EXPECT_CALL(rd, transformation())
-        .WillOnce(Return(transformation));
+        .WillOnce(ReturnRef(trans));
     EXPECT_CALL(mock_gl, glUniformMatrix4fv(transform_uniform_location, 1, GL_FALSE, _));
     EXPECT_CALL(rd, alpha())
         .WillOnce(Return(0));
 
=== modified file 'tests/unit-tests/surfaces/test_surface.cpp'
--- tests/unit-tests/surfaces/test_surface.cpp	2013-06-24 22:54:22 +0000
+++ tests/unit-tests/surfaces/test_surface.cpp	2013-06-25 05:59:27 +0000
@@ -171,6 +171,8 @@
 
         ON_CALL(*mock_buffer_stream, secure_client_buffer())
             .WillByDefault(Return(std::make_shared<mtd::StubBuffer>()));
+        ON_CALL(*mock_buffer_stream, stream_size())
+            .WillByDefault(Return(size));
     }
 
     std::string surface_name;
@@ -340,6 +342,27 @@
     surf.set_rotation(60.0f, glm::vec3{0.0f, 0.0f, 1.0f});
 }
 
+TEST_F(SurfaceCreation, test_surface_transformation_cache_refreshes)
+{
+    using namespace testing;
+
+    const geom::Point origin{geom::X{77}, geom::Y{88}};
+
+    ms::Surface surf{surface_name, origin, mock_buffer_stream,
+        std::shared_ptr<mi::InputChannel>(), null_change_cb};
+
+    glm::mat4 t0 = surf.transformation();
+    surf.move_to(geom::Point{geom::X{55}, geom::Y{66}});
+    EXPECT_NE(t0, surf.transformation());
+
+    surf.move_to(origin);
+    EXPECT_EQ(t0, surf.transformation());
+
+    surf.set_rotation(60.0f, glm::vec3{0.0f, 0.0f, 1.0f});
+    glm::mat4 t1 = surf.transformation();
+    EXPECT_NE(t0, t1);
+}
+
 TEST_F(SurfaceCreation, test_surface_texture_locks_back_buffer_from_stream)
 {
     using namespace testing;