Merge lp:~raof/mir/buffer-age-misc-cleanups into lp:~mir-team/mir/trunk

[Chris Halse Rogers]

Miscellaneous cleanups left over from non-blocking merge reviews of
lp:~raof/mir/client-side-buffer-age.

Miscellaneous cleanups left over from non-blocking merge reviews of
lp:~raof/mir/client-side-buffer-age.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:501
http://jenkins.qa.ubuntu.com/job/mir-ci/189/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/190
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/190/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

180 - buffer_depository(depository),
181 logger(logger)
182 {
183 + buffer_depository = std::make_shared<mcl::ClientBufferDepository>(factory, 3);
184 +

Use initializers where possible. Vis:

      buffer_depository(std::make_shared<mcl::ClientBufferDepository>(factory, 3)),

~~~~

in test_client_mir_surface.cpp

226 +#include <cstring>
227 +

Is anything from this header used?

~~~~

$ bin/unit-tests --gtest_filter=MirBufferDepositoryTest* | grep "GMOCK WARNING:" | wc -l
66

review: Needs Fixing

[Robert Carr (robertcarr)]

Looks good to me besides Alan's comments.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:503
http://jenkins.qa.ubuntu.com/job/mir-ci/203/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/134
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/204
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/204/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

> in test_client_mir_surface.cpp
>
>226 +#include <cstring>
>227 +
>
>Is anything from this header used?

Yes - memcmp, for the BufferPackageMatches matcher.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:504
http://jenkins.qa.ubuntu.com/job/mir-ci/204/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/205
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/205/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

=== modified file 'src/client/android/android_client_buffer_factory.cpp'
--- src/client/android/android_client_buffer_factory.cpp	2013-03-20 06:14:48 +0000
+++ src/client/android/android_client_buffer_factory.cpp	2013-03-26 09:39:22 +0000
@@ -24,12 +24,12 @@
 namespace mcla=mir::client::android;
 namespace geom=mir::geometry;
 
-mcla::AndroidClientBufferFactory::AndroidClientBufferFactory(std::shared_ptr<AndroidRegistrar> const &buffer_registrar)
+mcla::AndroidClientBufferFactory::AndroidClientBufferFactory(std::shared_ptr<AndroidRegistrar> const& buffer_registrar)
  : registrar(buffer_registrar)
 {
 }
 
-std::shared_ptr<mcl::ClientBuffer> mcla::AndroidClientBufferFactory::create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package, geom::Size size, geom::PixelFormat pf)
+std::shared_ptr<mcl::ClientBuffer> mcla::AndroidClientBufferFactory::create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package, geom::Size size, geom::PixelFormat pf)
 {
     return std::make_shared<mcla::AndroidClientBuffer>(registrar, package, size, pf);
 }
 
=== modified file 'src/client/android/android_client_buffer_factory.h'
--- src/client/android/android_client_buffer_factory.h	2013-03-20 06:14:48 +0000
+++ src/client/android/android_client_buffer_factory.h	2013-03-26 09:39:22 +0000
@@ -41,9 +41,9 @@
 class AndroidClientBufferFactory : public ClientBufferFactory
 {
 public:
-    explicit AndroidClientBufferFactory(std::shared_ptr<AndroidRegistrar> const &);
+    explicit AndroidClientBufferFactory(std::shared_ptr<AndroidRegistrar> const&);
 
-    virtual std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package,
+    virtual std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package,
                                                         geometry::Size size, geometry::PixelFormat pf);
 private:
     std::shared_ptr<AndroidRegistrar> registrar;
 
=== modified file 'src/client/client_buffer_depository.cpp'
--- src/client/client_buffer_depository.cpp	2013-03-18 05:58:51 +0000
+++ src/client/client_buffer_depository.cpp	2013-03-26 09:39:22 +0000
@@ -27,14 +27,14 @@
 
 namespace mcl=mir::client;
 
-mcl::ClientBufferDepository::ClientBufferDepository(std::shared_ptr<ClientBufferFactory> const &factory, int max_buffers)
+mcl::ClientBufferDepository::ClientBufferDepository(std::shared_ptr<ClientBufferFactory> const& factory, int max_buffers)
     : factory(factory),
       current_buffer_iter(buffers.end()),
       max_age(max_buffers - 1)
 {
 }
 
-void mcl::ClientBufferDepository::deposit_package(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package, int id, geometry::Size size, geometry::PixelFormat pf)
+void mcl::ClientBufferDepository::deposit_package(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package, int id, geometry::Size size, geometry::PixelFormat pf)
 {
     for (auto next = buffers.begin(); next != buffers.end();)
     {
 
=== modified file 'src/client/client_buffer_depository.h'
--- src/client/client_buffer_depository.h	2013-03-24 23:33:49 +0000
+++ src/client/client_buffer_depository.h	2013-03-26 09:39:22 +0000
@@ -47,13 +47,13 @@
 class ClientBufferDepository
 {
 public:
-    ClientBufferDepository(std::shared_ptr<ClientBufferFactory> const &factory, int max_buffers);
+    ClientBufferDepository(std::shared_ptr<ClientBufferFactory> const& factory, int max_buffers);
 
     /// Construct a ClientBuffer from the IPC data, and use it as the current buffer.
 
     /// This also marks the previous current buffer (if any) as being submitted to the server.
     /// \post current_buffer() will return a ClientBuffer constructed from this IPC data.
-    void deposit_package(std::shared_ptr<mir_toolkit::MirBufferPackage> const &, int id,
+    void deposit_package(std::shared_ptr<mir_toolkit::MirBufferPackage> const&, int id,
                          geometry::Size, geometry::PixelFormat);
     std::shared_ptr<ClientBuffer> current_buffer();
 
 
=== modified file 'src/client/client_buffer_factory.h'
--- src/client/client_buffer_factory.h	2013-03-20 06:14:48 +0000
+++ src/client/client_buffer_factory.h	2013-03-26 09:39:22 +0000
@@ -35,8 +35,14 @@
 class ClientBufferFactory
 {
 public:
-    virtual std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package,
+    virtual std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package,
                                                         geometry::Size size, geometry::PixelFormat pf) = 0;
+
+protected:
+	ClientBufferFactory() = default;
+	ClientBufferFactory(ClientBufferFactory const &) = delete;
+	ClientBufferFactory &operator=(ClientBufferFactory const &) = delete;	
+	virtual ~ClientBufferFactory() {}
 };
 
 }
 
=== modified file 'src/client/client_platform.h'
--- src/client/client_platform.h	2013-03-24 23:33:49 +0000
+++ src/client/client_platform.h	2013-03-26 09:39:22 +0000
@@ -36,7 +36,7 @@
     ClientPlatform(const ClientPlatform& p) = delete;
     ClientPlatform& operator=(const ClientPlatform& p) = delete;
 
-    virtual std::shared_ptr<ClientBufferFactory> create_buffer_factory () = 0;
+    virtual std::shared_ptr<ClientBufferFactory> create_buffer_factory() = 0;
     virtual std::shared_ptr<EGLNativeWindowType> create_egl_native_window(ClientSurface *surface) = 0;
     virtual std::shared_ptr<EGLNativeDisplayType> create_egl_native_display() = 0;
 };
 
=== modified file 'src/client/gbm/gbm_client_buffer_factory.cpp'
--- src/client/gbm/gbm_client_buffer_factory.cpp	2013-03-20 23:50:00 +0000
+++ src/client/gbm/gbm_client_buffer_factory.cpp	2013-03-26 09:39:22 +0000
@@ -29,7 +29,7 @@
 {
 }
 
-std::shared_ptr<mcl::ClientBuffer> mclg::GBMClientBufferFactory::create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package, geometry::Size size, geometry::PixelFormat pf)
+std::shared_ptr<mcl::ClientBuffer> mclg::GBMClientBufferFactory::create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package, geometry::Size size, geometry::PixelFormat pf)
 {
     return std::make_shared<mclg::GBMClientBuffer>(drm_fd_handler, package, size, pf);
 }
 
=== modified file 'src/client/gbm/gbm_client_buffer_factory.h'
--- src/client/gbm/gbm_client_buffer_factory.h	2013-03-20 23:50:00 +0000
+++ src/client/gbm/gbm_client_buffer_factory.h	2013-03-26 09:39:22 +0000
@@ -39,7 +39,7 @@
 public:
     explicit GBMClientBufferFactory(std::shared_ptr<DRMFDHandler> const& drm_fd_handler);
 
-    std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const & package,
+    std::shared_ptr<ClientBuffer> create_buffer(std::shared_ptr<mir_toolkit::MirBufferPackage> const& package,
                                                 geometry::Size size, geometry::PixelFormat pf);
 private:
     std::shared_ptr<DRMFDHandler> drm_fd_handler;
 
=== modified file 'src/client/gbm/gbm_client_platform.h'
--- src/client/gbm/gbm_client_platform.h	2013-03-24 23:33:49 +0000
+++ src/client/gbm/gbm_client_platform.h	2013-03-26 09:39:22 +0000
@@ -39,7 +39,7 @@
                       std::shared_ptr<DRMFDHandler> const& drm_fd_handler,
                       EGLNativeDisplayContainer& display_container);
 
-    std::shared_ptr<ClientBufferFactory> create_buffer_factory ();
+    std::shared_ptr<ClientBufferFactory> create_buffer_factory();
     std::shared_ptr<EGLNativeWindowType> create_egl_native_window(ClientSurface *surface);
     std::shared_ptr<EGLNativeDisplayType> create_egl_native_display();
 
 
=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-03-24 23:33:49 +0000
+++ src/client/mir_connection.cpp	2013-03-26 09:39:22 +0000
@@ -66,9 +66,8 @@
     mir_surface_lifecycle_callback callback,
     void * context)
 {
-    auto depository = std::make_shared<mir::client::ClientBufferDepository>(platform->create_buffer_factory(), 3);
     auto null_log = std::make_shared<mir::client::NullLogger>();
-    auto surface = new MirSurface(this, server, null_log, depository, params, callback, context);
+    auto surface = new MirSurface(this, server, null_log, platform->create_buffer_factory(), params, callback, context);
     return surface->get_create_wait_handle();
 }
 
 
=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-03-24 23:33:49 +0000
+++ src/client/mir_surface.cpp	2013-03-26 09:39:22 +0000
@@ -32,13 +32,13 @@
 mir_toolkit::MirSurface::MirSurface(
     MirConnection *allocating_connection,
     mp::DisplayServer::Stub & server,
-    const std::shared_ptr<mir::client::Logger>& logger,
-    const std::shared_ptr<mcl::ClientBufferDepository>& depository,
-    MirSurfaceParameters const & params,
+    std::shared_ptr<mir::client::Logger> const& logger,
+    std::shared_ptr<mcl::ClientBufferFactory> const& factory,
+    MirSurfaceParameters const& params,
     mir_surface_lifecycle_callback callback, void * context)
     : server(server),
       connection(allocating_connection),
-      buffer_depository(depository),
+      buffer_depository(std::make_shared<mcl::ClientBufferDepository>(factory, 3)),
       logger(logger)
 {
     mir::protobuf::SurfaceParameters message;
 
=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-03-24 23:33:49 +0000
+++ src/client/mir_surface.h	2013-03-26 09:39:22 +0000
@@ -49,9 +49,9 @@
     MirSurface(
         MirConnection *allocating_connection,
         mir::protobuf::DisplayServer::Stub & server,
-        const std::shared_ptr<mir::client::Logger>& logger,
-        const std::shared_ptr<mir::client::ClientBufferDepository>& depository,
-        MirSurfaceParameters const & params,
+        std::shared_ptr<mir::client::Logger> const& logger,
+        std::shared_ptr<mir::client::ClientBufferFactory> const& buffer_factory,
+        MirSurfaceParameters const& params,
         mir_surface_lifecycle_callback callback, void * context);
 
     ~MirSurface();
 
=== modified file 'tests/unit-tests/client/test_client_buffer_depository.cpp'
--- tests/unit-tests/client/test_client_buffer_depository.cpp	2013-03-21 07:54:30 +0000
+++ tests/unit-tests/client/test_client_buffer_depository.cpp	2013-03-26 09:39:22 +0000
@@ -36,6 +36,9 @@
         using namespace testing;
         ON_CALL(*this, mark_as_submitted())
             .WillByDefault(Invoke([this](){this->AgingBuffer::mark_as_submitted();}));
+
+        // By default we expect all buffers to be destroyed.        
+        EXPECT_CALL(*this, Destroy()).Times(1);
     }
 
     MOCK_METHOD0(Destroy, void());
@@ -67,7 +70,7 @@
     }
 
     MOCK_METHOD3(create_buffer,
-                 std::shared_ptr<mcl::ClientBuffer>(std::shared_ptr<MirBufferPackage> const &,
+                 std::shared_ptr<mcl::ClientBuffer>(std::shared_ptr<MirBufferPackage> const&,
                                                     geom::Size, geom::PixelFormat));
 
     std::shared_ptr<mcl::ClientBuffer> buffer;
@@ -265,19 +268,23 @@
     depository.deposit_package(packages[0], 1, size, pf);
     // Raw pointer so we don't influence the buffer's life-cycle
     MockBuffer *first_buffer = static_cast<MockBuffer *>(depository.current_buffer().get());
+    // We expect this to not be destroyed before we deposit the fourth buffer.
+    bool buffer_destroyed = false;
+    ON_CALL(*first_buffer, Destroy()).WillByDefault(Invoke([&buffer_destroyed] () {buffer_destroyed = true;}));
+
 
     depository.deposit_package(packages[1], 2, size, pf);
     depository.deposit_package(packages[2], 3, size, pf);
 
     // We've deposited three different buffers now; the fourth should trigger the destruction
     // of the first buffer.
-    EXPECT_CALL(*first_buffer, Destroy());
+    ASSERT_FALSE(buffer_destroyed);
 
     depository.deposit_package(packages[3], 4, size, pf);
 
     // Explicitly verify that the buffer has been destroyed here, before the depository goes out of scope
     // and its destructor cleans everything up.
-    Mock::VerifyAndClearExpectations(first_buffer);
+    EXPECT_TRUE(buffer_destroyed);
 }
 
 TEST_F(MirBufferDepositoryTest, depositing_packages_implicitly_submits_current_buffer)
@@ -300,33 +307,33 @@
     using namespace testing;
     std::shared_ptr<mcl::ClientBufferDepository> depository;
     std::shared_ptr<MirBufferPackage> packages[10];
-    MockBuffer *buffers[10];
+    bool buffer_destroyed[10];
 
     for (int num_buffers = 2; num_buffers < 10; ++num_buffers)
     {
         depository = std::make_shared<mcl::ClientBufferDepository>(mock_factory, num_buffers);
-        
-        depository->deposit_package(packages[0], 1, size, pf);
-        // Raw pointer so we don't influence the buffer's life-cycle
-        MockBuffer* first_buffer = static_cast<MockBuffer *>(depository->current_buffer().get());
+
+        // Reset destroyed tracking; resetting the depository will have destroyed all the buffers
+        for (bool& destroyed_flag : buffer_destroyed)
+            destroyed_flag = false;
 
         int i;
-        for (i = 1; i < num_buffers ; ++i)
+        for (i = 0; i < num_buffers ; ++i)
         {
+            MockBuffer *buffer;
             depository->deposit_package(packages[i], i + 1, size, pf);
-            buffers[i] = static_cast<MockBuffer *>(depository->current_buffer().get());
-            // None of these buffers should be destroyed
-            EXPECT_CALL(*buffers[i], Destroy()).Times(0);
+            buffer = static_cast<MockBuffer *>(depository->current_buffer().get());
+            ON_CALL(*buffer, Destroy()).WillByDefault(Invoke([&buffer_destroyed, i] () {buffer_destroyed[i] = true;}));
         }
 
         // Next deposit should destroy first buffer
-        EXPECT_CALL(*first_buffer, Destroy()).Times(1);
+        ASSERT_FALSE(buffer_destroyed[0]);
         depository->deposit_package(packages[i], i+1, size, pf);
-        EXPECT_TRUE(Mock::VerifyAndClearExpectations(first_buffer));
+        EXPECT_TRUE(buffer_destroyed[0]);
 
         // Verify none of the other buffers have been destroyed
         for (i = 1; i < num_buffers; ++i)
-            EXPECT_TRUE(Mock::VerifyAndClearExpectations(buffers[i]));
+            EXPECT_FALSE(buffer_destroyed[i]);
     }
 }
 
 
=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-24 23:33:49 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-26 09:39:22 +0000
@@ -33,6 +33,8 @@
 #include "mir_test/test_protobuf_client.h"
 #include "mir_test/gmock_fixes.h"
 
+#include <cstring>
+
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
@@ -84,11 +86,11 @@
         int num_fd = 2, num_data = 8;
         for (auto i=0; i<num_fd; i++)
         {
-            server_package.fd[i] = i*3;
+            server_package.fd[i] = global_buffer_id * i;
         }
         for (auto i=0; i<num_data; i++)
         {
-            server_package.data[i] = i*2;
+            server_package.data[i] = (global_buffer_id + i) * 2;
         }
         server_package.stride = 66;
     }
@@ -134,9 +136,14 @@
 
 struct MockBuffer : public mcl::ClientBuffer
 {
-    MockBuffer()
+    explicit MockBuffer(std::shared_ptr<MirBufferPackage> const& contents)
     {
+        using namespace testing;
 
+        auto buffer_package = std::make_shared<MirBufferPackage>();
+        *buffer_package = *contents;
+        ON_CALL(*this, get_buffer_package())
+            .WillByDefault(Return(buffer_package));
     }
 
     MOCK_METHOD0(secure_for_cpu_write, std::shared_ptr<mcl::MemoryRegion>());
@@ -156,15 +163,20 @@
     {
         using namespace testing;
 
-        emptybuffer=std::make_shared<NiceMock<MockBuffer>>();
+        emptybuffer=std::make_shared<NiceMock<MockBuffer>>(std::make_shared<MirBufferPackage>());
+
         ON_CALL(*this, create_buffer(_,_,_))
-            .WillByDefault(Return(emptybuffer));
+            .WillByDefault(DoAll(SaveArg<0>(&current_package),
+                                 InvokeWithoutArgs([this] () {this->current_buffer = std::make_shared<NiceMock<MockBuffer>>(current_package);}),
+                                 ReturnPointee(&current_buffer)));
     }
 
     MOCK_METHOD3(create_buffer,
-                 std::shared_ptr<mcl::ClientBuffer>(std::shared_ptr<MirBufferPackage> const &,
+                 std::shared_ptr<mcl::ClientBuffer>(std::shared_ptr<MirBufferPackage> const&,
                                                     geom::Size, geom::PixelFormat));
 
+    std::shared_ptr<MirBufferPackage> current_package;
+    std::shared_ptr<mcl::ClientBuffer> current_buffer;
     std::shared_ptr<mcl::ClientBuffer> emptybuffer;
 };
 
@@ -219,7 +231,6 @@
         test_server->comm->start();
 
         mock_buffer_factory = std::make_shared<mt::MockClientBufferFactory>();
-        depository = std::make_shared<mcl::ClientBufferDepository>(mock_buffer_factory, 3);
 
         params = MirSurfaceParameters{"test", 33, 45, mir_pixel_format_abgr_8888,
                                       mir_buffer_usage_hardware};
@@ -250,7 +261,6 @@
 
     MirSurfaceParameters params;
     std::shared_ptr<mt::MockClientBufferFactory> mock_buffer_factory;
-    std::shared_ptr<mcl::ClientBufferDepository> depository;
 
     mir::protobuf::Connection response;
     mir::protobuf::ConnectParameters connect_parameters;
@@ -272,7 +282,13 @@
     EXPECT_CALL(*mock_buffer_factory, create_buffer(_,_,_))
         .Times(1);
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
 
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
@@ -290,7 +306,13 @@
     EXPECT_CALL(*mock_buffer_factory, create_buffer(_,_,_))
         .Times(1);
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
 
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
@@ -301,6 +323,22 @@
     buffer_wait_handle->wait_for_result();
 }
 
+MATCHER_P(BufferPackageMatches, package, "")
+{
+    // Can't simply use memcmp() on the whole struct because age is not sent over the wire
+    if (package.data_items != arg.data_items)
+        return false;
+    if (package.fd_items != arg.fd_items)
+        return false;
+    if (memcmp(package.data, arg.data, sizeof(package.data[0]) * package.data_items))
+        return false;
+    if (memcmp(package.fd, arg.fd, sizeof(package.fd[0]) * package.fd_items))
+        return false;
+    if (package.stride != arg.stride)
+        return false;
+    return true;
+}
+
 TEST_F(MirClientSurfaceTest, client_buffer_uses_ipc_message_from_server_on_create)
 {
     using namespace testing;
@@ -311,16 +349,18 @@
         .WillOnce(DoAll(SaveArg<0>(&submitted_package),
                         Return(mock_buffer_factory->emptybuffer)));
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
 
     /* check for same contents */
-    ASSERT_EQ(submitted_package->data_items, mock_server_tool->server_package.data_items);
-    ASSERT_EQ(submitted_package->fd_items,   mock_server_tool->server_package.fd_items);
-    ASSERT_EQ(submitted_package->stride,   mock_server_tool->server_package.stride);
-    for (auto i=0; i< submitted_package->data_items; i++)
-        EXPECT_EQ(submitted_package->data[i], mock_server_tool->server_package.data[i]);
+    EXPECT_THAT(*submitted_package, BufferPackageMatches(mock_server_tool->server_package));
 }
 
 TEST_F(MirClientSurfaceTest, message_width_used_in_buffer_creation )
@@ -335,7 +375,13 @@
         .WillOnce(DoAll(SaveArg<1>(&sz),
                         Return(mock_buffer_factory->emptybuffer)));
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
 
@@ -354,7 +400,13 @@
         .WillOnce(DoAll(SaveArg<1>(&sz),
                         Return(mock_buffer_factory->emptybuffer)));
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
 
@@ -373,13 +425,47 @@
         .WillOnce(DoAll(SaveArg<2>(&pf),
                         Return(mock_buffer_factory->emptybuffer)));
 
-    auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel, logger, depository, params, &empty_callback, (void*) NULL);
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
+
     auto wait_handle = surface->get_create_wait_handle();
     wait_handle->wait_for_result();
 
     EXPECT_EQ(pf, geom::PixelFormat::abgr_8888);
 }
 
+TEST_F(MirClientSurfaceTest, get_buffer_returns_last_received_buffer_package)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*mock_buffer_factory, create_buffer(_,_,_))
+        .Times(1);
+
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_buffer_factory,
+                                                 params,
+                                                 &empty_callback,
+                                                 nullptr);
+    auto wait_handle = surface->get_create_wait_handle();
+    wait_handle->wait_for_result();
+
+    EXPECT_THAT(*surface->get_current_buffer_package(), BufferPackageMatches(mock_server_tool->server_package));
+
+    EXPECT_CALL(*mock_buffer_factory, create_buffer(_,_,_))
+        .Times(1);
+    auto buffer_wait_handle = surface->next_buffer(&empty_surface_callback, nullptr);
+    buffer_wait_handle->wait_for_result();
+
+    EXPECT_THAT(*surface->get_current_buffer_package(), BufferPackageMatches(mock_server_tool->server_package));
+}
+
 TEST_F(MirClientSurfaceTest, default_surface_type)
 {
     using namespace testing;
@@ -391,10 +477,10 @@
     auto surface = std::make_shared<MirSurface> (connection.get(),
                                                  *client_comm_channel,
                                                  logger,
-                                                 depository,
+                                                 mock_buffer_factory,
                                                  params,
                                                  &empty_callback,
-                                                 (void*) NULL);
+                                                 nullptr);
     surface->get_create_wait_handle()->wait_for_result();
 
     EXPECT_EQ(mir_surface_type_normal,