Merge lp:~kdub/mir/fix-1503317 into lp:mir

[Kevin DuBois]

In multibuffer stream scenarios, ensure that the size of the renderable generated from the stream is the size of the stream.

fixes lp: #1503317

In multibuffer stream scenarios, ensure that the size of the renderable generated from the stream is the size of the stream.

fixes lp: #1503317

problem came to my attention when working on switching the demo servers from the 'titlebars' being a surface to being a stream.

[Alan Griffiths (alan-griffiths)]

Looks reasonable.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3004
http://jenkins.qa.ubuntu.com/job/mir-ci/5195/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4346
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3253
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4288
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/246/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1349
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1349/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/246/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4289
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4289/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6948
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24037
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/247/console

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

+ std::vector<std::vector<std::tuple<geom::Displacement, geom::Size>>> displacements;

This (in it's many positions) is no longer really a list of displacements. You're renamed it “positions” in one location, and that seems reasonable.

Also, tuple<Displacement, Size> is pretty much a geom::Rectangle. It looks like you could just drop that in (although if you wanted to type-encode you'd need a RelativeRectangle).

Otherwise looks good.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3006
http://jenkins.qa.ubuntu.com/job/mir-ci/5209/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4364
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3271
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4306
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/264/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1363
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1363/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/264/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4307
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4307/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6961
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24066
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/265/console

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

LGTM

review: Approve

=== modified file 'include/test/mir_test_framework/connected_client_with_a_surface.h'
--- include/test/mir_test_framework/connected_client_with_a_surface.h	2015-06-22 03:04:56 +0000
+++ include/test/mir_test_framework/connected_client_with_a_surface.h	2015-10-08 12:19:30 +0000
@@ -20,6 +20,7 @@
 #define MIR_TEST_FRAMEWORK_CONNECTED_CLIENT_WITH_A_SURFACE_H_
 
 #include "mir_test_framework/connected_client_headless_server.h"
+#include "mir/geometry/size.h"
 
 namespace mir_test_framework
 {
@@ -30,6 +31,8 @@
     void SetUp() override;
 
     void TearDown() override;
+
+    mir::geometry::Size const surface_size {640, 480};
 };
 }
 

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2015-10-02 01:56:04 +0000
+++ src/server/scene/basic_surface.cpp	2015-10-08 12:19:30 +0000
@@ -877,7 +877,7 @@
         {
             list.emplace_back(std::make_shared<SurfaceSnapshot>(
                 info.stream, id,
-                geom::Rectangle{surface_rect.top_left + info.displacement, surface_rect.size},
+                geom::Rectangle{surface_rect.top_left + info.displacement, info.stream->stream_size()},
                 transformation_matrix, surface_alpha, nonrectangular, info.stream.get()));
         }
     }

=== modified file 'tests/acceptance-tests/test_buffer_stream_arrangement.cpp'
--- tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2015-06-22 03:04:56 +0000
+++ tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2015-10-08 12:19:30 +0000
@@ -38,6 +38,23 @@
 namespace
 {
 
+struct RelativeRectangle
+{
+    RelativeRectangle() = default;
+
+    RelativeRectangle(geom::Displacement const& displacement, geom::Size const& size)
+        : displacement{displacement}, size{size}
+    {
+    }
+
+    geom::Displacement displacement;
+    geom::Size size;
+};
+bool operator==(RelativeRectangle const& a, RelativeRectangle const& b)
+{
+    return (a.displacement == b.displacement) && (a.size == b.size);
+}
+
 MirPixelFormat an_available_format(MirConnection* connection)
 {
     using namespace testing;
@@ -49,9 +66,10 @@
 
 struct Stream
 {
-    Stream(MirBufferStream* stream, geom::Point pt) :
+    Stream(MirBufferStream* stream, geom::Point pt, geom::Size sz) :
         stream(stream),
         pos{pt},
+        stream_size{sz},
         needs_release{false}
     {
         mir_buffer_stream_swap_buffers_sync(stream);
@@ -65,6 +83,7 @@
             an_available_format(connection),
             mir_buffer_usage_hardware)),
         pos{rect.top_left},
+        stream_size{rect.size},
         needs_release{true}
     {
         mir_buffer_stream_swap_buffers_sync(stream);
@@ -86,11 +105,17 @@
         return pos;
     }
 
+    geom::Size size()
+    {
+        return stream_size;
+    }
+
     Stream(Stream const&) = delete;
     Stream& operator=(Stream const&) = delete;
 private:
     MirBufferStream* stream;
     geom::Point const pos;
+    geom::Size stream_size;
     bool const needs_release;
 };
 
@@ -102,24 +127,26 @@
             return;
 
         std::unique_lock<decltype(mutex)> lk(mutex);
-        std::vector<geom::Displacement> displacement;
+        std::vector<RelativeRectangle> position;
         auto first_position = (*sequence.begin())->renderable()->screen_position().top_left;
         for (auto const& element : sequence)
-            displacement.emplace_back(element->renderable()->screen_position().top_left - first_position);
-        displacements.push_back(displacement);
+            position.emplace_back(
+                element->renderable()->screen_position().top_left - first_position,
+                element->renderable()->screen_position().size);
+        positions.push_back(position);
         cv.notify_all();
     }
 
     template<typename T, typename S>
-    bool wait_for_ordering_within(
-        std::vector<geom::Displacement> const& ordering,
+    bool wait_for_positions_within(
+        std::vector<RelativeRectangle> const& awaited_positions,
         std::chrono::duration<T,S> duration)
     {
         std::unique_lock<decltype(mutex)> lk(mutex);
-        return cv.wait_for(lk, duration, [this, ordering] {
-            for (auto& displacement : displacements)
-                if (displacement == ordering) return true;
-            displacements.clear();
+        return cv.wait_for(lk, duration, [this, awaited_positions] {
+            for (auto& position : positions)
+                if (position == awaited_positions) return true;
+            positions.clear();
             return false;
         });
     }
@@ -127,7 +154,7 @@
 private:
     std::mutex mutex;
     std::condition_variable cv;
-    std::vector<std::vector<geom::Displacement>> displacements;
+    std::vector<std::vector<RelativeRectangle>> positions;
 };
 
 struct OrderTrackingDBC : mc::DisplayBufferCompositor
@@ -186,7 +213,7 @@
         server.the_cursor()->hide();
 
         streams.emplace_back(
-            std::make_unique<Stream>(mir_surface_get_buffer_stream(surface), geom::Point{0,0}));
+            std::make_unique<Stream>(mir_surface_get_buffer_stream(surface), geom::Point{0,0}, surface_size));
         int const additional_streams{3};
         for (auto i = 0; i < additional_streams; i++)
         {
@@ -226,12 +253,17 @@
     mir_surface_apply_spec(surface, change_spec);
     mir_surface_spec_release(change_spec);
 
-    std::vector<geom::Displacement> displacements;
+    std::vector<RelativeRectangle> positions;
+    i = 0;
     for (auto& info : infos)
-        displacements.emplace_back(geom::Displacement{info.displacement_x, info.displacement_y});
+    {
+        positions.emplace_back(
+            geom::Displacement{info.displacement_x, info.displacement_y},
+            streams[i++]->size());
+    }
 
     //check that the compositor rendered correctly
     using namespace std::literals::chrono_literals;
-    EXPECT_TRUE(ordering->wait_for_ordering_within(displacements, 5s))
+    EXPECT_TRUE(ordering->wait_for_positions_within(positions, 5s))
          << "timed out waiting to see the compositor post the streams in the right arrangement";
 }

=== modified file 'tests/include/mir/test/doubles/mock_buffer_bundle.h'
--- tests/include/mir/test/doubles/mock_buffer_bundle.h	2015-09-14 16:46:44 +0000
+++ tests/include/mir/test/doubles/mock_buffer_bundle.h	2015-10-08 12:19:30 +0000
@@ -37,7 +37,9 @@
         ON_CALL(*this, buffers_ready_for_compositor(testing::_))
             .WillByDefault(testing::Return(1));
         ON_CALL(*this, properties())
-            .WillByDefault(testing::Return(graphics::BufferProperties()));
+            .WillByDefault(testing::Return(
+                graphics::BufferProperties{geometry::Size{1,1},
+                mir_pixel_format_abgr_8888, graphics::BufferUsage::hardware}));
     }
 
     MOCK_METHOD1(client_acquire,     void(std::function<void(graphics::Buffer*)>));

=== modified file 'tests/include/mir/test/doubles/mock_buffer_stream.h'
--- tests/include/mir/test/doubles/mock_buffer_stream.h	2015-09-14 14:45:36 +0000
+++ tests/include/mir/test/doubles/mock_buffer_stream.h	2015-10-08 12:19:30 +0000
@@ -53,6 +53,8 @@
             .WillByDefault(testing::Return(true));
         ON_CALL(*this, pixel_format())
             .WillByDefault(testing::Return(mir_pixel_format_abgr_8888));
+        ON_CALL(*this, stream_size())
+            .WillByDefault(testing::Return(geometry::Size{0,0}));
     }
     MOCK_METHOD1(acquire_client_buffer, void(std::function<void(graphics::Buffer* buffer)>));
     MOCK_METHOD1(release_client_buffer, void(graphics::Buffer*));

=== modified file 'tests/mir_test_framework/connected_client_with_a_surface.cpp'
--- tests/mir_test_framework/connected_client_with_a_surface.cpp	2015-06-17 05:20:42 +0000
+++ tests/mir_test_framework/connected_client_with_a_surface.cpp	2015-10-08 12:19:30 +0000
@@ -25,7 +25,7 @@
     ConnectedClientHeadlessServer::SetUp();
 
     auto const spec = mir_connection_create_spec_for_normal_surface(
-        connection, 640, 480, mir_pixel_format_abgr_8888);
+        connection, surface_size.width.as_int(), surface_size.height.as_int(), mir_pixel_format_abgr_8888);
     mir_surface_spec_set_name(spec, "ConnectedClientWithASurfaceFixtureSurface");
     mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_hardware);
 

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2015-07-27 07:24:50 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2015-10-08 12:19:30 +0000
@@ -760,6 +760,11 @@
     return (pos == arg->screen_position().top_left);
 }
 
+MATCHER_P(IsRenderableOfSize, size, "is renderable with size")
+{
+    return (size == arg->screen_position().size);
+}
+
 MATCHER_P(IsRenderableOfAlpha, alpha, "is renderable with alpha")
 {
     EXPECT_THAT(static_cast<float>(alpha), testing::FloatEq(arg->alpha()));
@@ -965,3 +970,28 @@
     EXPECT_THAT(surface.buffers_ready_for_compositor(this), Eq(0));
     EXPECT_THAT(surface.buffers_ready_for_compositor(this), Eq(2));
 }
+
+TEST_F(BasicSurfaceTest, buffer_streams_produce_correctly_sized_renderables)
+{
+    using namespace testing;
+    auto buffer_stream = std::make_shared<NiceMock<mtd::MockBufferStream>>();
+    geom::Displacement d0{0,0};
+    geom::Displacement d1{19,99};
+    geom::Size size0 {100, 101};
+    geom::Size size1 {102, 103};
+    ON_CALL(*mock_buffer_stream, stream_size())
+        .WillByDefault(Return(size0));
+    ON_CALL(*buffer_stream, stream_size())
+        .WillByDefault(Return(size1));
+
+    std::list<ms::StreamInfo> streams = {
+        { mock_buffer_stream, d0 },
+        { buffer_stream, d1 },
+    };
+    surface.set_streams(streams);
+
+    auto renderables = surface.generate_renderables(this);
+    ASSERT_THAT(renderables.size(), Eq(2));
+    EXPECT_THAT(renderables[0], IsRenderableOfSize(size0));
+    EXPECT_THAT(renderables[1], IsRenderableOfSize(size1));
+}