Merge lp:~kdub/mir/client-resize-logic into lp:mir

[Kevin DuBois]

client: add logic to free and allocate buffers of the proper size after a buffer size change.

client: add logic to free and allocate buffers of the proper size after a buffer size change.

There's a bit of plumbing to pass the resize messages to the stream that goes along with this; but its dependent on other yet-unlanded branches.

I tried a few places in the buffer lifecycle to free and allocate. Between the compositor release and the client usage is where we currently do this for exchange-semantics, and this place is the analogous place on the client side. Its a good place to avoid spamming the server with allocation and free requests when rapid resizing occurs.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2848
http://jenkins.qa.ubuntu.com/job/mir-ci/4591/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3555
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/417/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/1089/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3503/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/740/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3503
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3503/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6280/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22631

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2848
http://jenkins.qa.ubuntu.com/job/mir-ci/4620/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3585
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2496
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3535
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/769
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/769/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3535
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3535/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6307
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22691

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

«Insert obligatory comment about how I'm pretty sure this all goes away after transition ☺»

186 +TEST_F(StartedBufferVault, frees_incoming_buffers_of_incorrect_size_with_immediate_response)
207 +TEST_F(StartedBufferVault, frees_incoming_buffers_of_incorrect_size_with_delayed_response)

There doesn't seem to be any reason why these use InSequence tests. The implementation happens to free before allocating, but would be perfectly correct to allocate before free; the InSequence isn't protecting a useful invariant of the code.

207 +TEST_F(StartedBufferVault, frees_incoming_buffers_of_incorrect_size_with_delayed_response)

There doesn't seem to be any reason to expect an allocation here? We're testing that the buffer is freed, right? We have
226 +TEST_F(StartedBufferVault, withdraw_gives_only_newly_sized_buffers_after_resize)
to test that we actually get correctly-sized buffers.

The code itself looks sane ;).

review: Needs Fixing

[lp:~kdub/mir/client-resize-logic]

2849. By Kevin DuBois on 2015-08-20

merge in mir

2850. By Kevin DuBois on 2015-08-20

no need to use InSequence

2851. By Kevin DuBois on 2015-08-20

rename test to say that reallocates instead of just frees

[Kevin DuBois (kdub)]

@InSequence
Removed the requirement, it could free or allocate in either order.
@allocation expectation
It is useful to test where the allocation happens. Another (bad) approach could be to allocate a new buffer on the resize() and this would create some buffers that may not get used if rapidly resizing. Opted to change the test name instead.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2851
http://jenkins.qa.ubuntu.com/job/mir-ci/4631/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3609
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2520
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3559
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/780
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/780/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3559
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3559/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6332
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22727

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Fair chop.

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good overall.

55 + lk.unlock();
56 + server_requests->free_buffer(it->first);
57 + server_requests->allocate_buffer(size, format, usage);
58 + lk.lock();

I am bit wary of unlocking to perform the reallocation, as, in theory at least, it could allow other threads to come and mess up the state. Not sure if this could happen in practice in this case, but it would be good to know.

review: Approve

[lp:~kdub/mir/client-resize-logic]

2852. By Kevin DuBois on 2015-08-25

merge in mir

2853. By Kevin DuBois on 2015-08-25

make sure buffers is only accessed under lock

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2853
http://jenkins.qa.ubuntu.com/job/mir-ci/4673/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3666
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/87
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2577
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3616
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/822
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/822/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3616
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3616/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6387
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22810

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

review: Approve (continuous-integration)

=== modified file 'src/client/buffer_vault.cpp'
--- src/client/buffer_vault.cpp	2015-08-10 17:04:35 +0000
+++ src/client/buffer_vault.cpp	2015-08-25 13:27:45 +0000
@@ -41,7 +41,9 @@
     geom::Size size, MirPixelFormat format, int usage, unsigned int initial_nbuffers) :
     factory(client_buffer_factory),
     server_requests(server_requests),
-    format(format)
+    format(format),
+    usage(usage),
+    size(size)
 {
     for (auto i = 0u; i < initial_nbuffers; i++)
         server_requests->allocate_buffer(size, format, usage);
@@ -58,7 +60,8 @@
     std::lock_guard<std::mutex> lk(mutex);
     std::promise<std::shared_ptr<mcl::ClientBuffer>> promise;
     auto it = std::find_if(buffers.begin(), buffers.end(),
-        [](std::pair<int, BufferEntry> const& entry) { return entry.second.owner == Owner::Self; });
+        [this](std::pair<int, BufferEntry> const& entry) { 
+            return ((entry.second.owner == Owner::Self) && (size == entry.second.buffer->size())); });
 
     auto future = promise.get_future();
     if (it != buffers.end())
@@ -68,7 +71,6 @@
     }
     else
     {
-        //TODO: We'll eventually overallocate a limited number of buffers here instead of promising.
         promises.emplace_back(std::move(promise));
     }
     return future;
@@ -113,7 +115,7 @@
     package->width = protobuf_buffer.width();
     package->height = protobuf_buffer.height();
 
-    std::lock_guard<std::mutex> lk(mutex);
+    std::unique_lock<std::mutex> lk(mutex);
     auto it = buffers.find(protobuf_buffer.buffer_id());
     if (it == buffers.end())
     {
@@ -122,8 +124,20 @@
     }
     else
     {
-        it->second.owner = Owner::Self;
-        it->second.buffer->update_from(*package);
+        if (size == it->second.buffer->size())
+        { 
+            it->second.owner = Owner::Self;
+            it->second.buffer->update_from(*package);
+        }
+        else
+        {
+            int id = it->first;
+            buffers.erase(it);
+            lk.unlock();
+            server_requests->free_buffer(id);
+            server_requests->allocate_buffer(size, format, usage);
+            return;
+        }
     }
 
     if (!promises.empty())
@@ -133,3 +147,9 @@
         promises.pop_front();
     }
 }
+
+void mcl::BufferVault::set_size(geom::Size sz)
+{
+    std::lock_guard<std::mutex> lk(mutex);
+    size = sz;
+}

=== modified file 'src/client/buffer_vault.h'
--- src/client/buffer_vault.h	2015-07-30 17:06:34 +0000
+++ src/client/buffer_vault.h	2015-08-25 13:27:45 +0000
@@ -61,12 +61,13 @@
     void deposit(std::shared_ptr<ClientBuffer> const& buffer);
     void wire_transfer_inbound(protobuf::Buffer const&);
     void wire_transfer_outbound(std::shared_ptr<ClientBuffer> const& buffer);
-    //TODO: class will handle allocation, transition, and destruction around resize notification
+    void set_size(geometry::Size);
 
 private:
     std::shared_ptr<ClientBufferFactory> const factory;
     std::shared_ptr<ServerBufferRequests> const server_requests;
     MirPixelFormat const format;
+    int const usage;
 
     enum class Owner;
     struct BufferEntry
@@ -78,6 +79,7 @@
     std::mutex mutex;
     std::map<int, BufferEntry> buffers;
     std::deque<std::promise<std::shared_ptr<ClientBuffer>>> promises;
+    geometry::Size size;
 };
 }
 }

=== modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
--- tests/unit-tests/client/test_buffer_vault.cpp	2015-08-10 17:04:35 +0000
+++ tests/unit-tests/client/test_buffer_vault.cpp	2015-08-25 13:27:45 +0000
@@ -42,8 +42,10 @@
 {
 struct MockBuffer : public mcl::AgingBuffer
 {
-    MockBuffer()
+    MockBuffer(geom::Size sz)
     {
+        ON_CALL(*this, size())
+            .WillByDefault(Return(sz));
         ON_CALL(*this, mark_as_submitted())
             .WillByDefault(Invoke([this](){this->AgingBuffer::mark_as_submitted();}));
     }
@@ -65,9 +67,12 @@
     MockClientBufferFactory()
     {
         ON_CALL(*this, create_buffer(_,_,_))
-            .WillByDefault(InvokeWithoutArgs([] { return std::make_shared<NiceMock<MockBuffer>>(); }));
+            .WillByDefault(Invoke([](
+                    std::shared_ptr<MirBufferPackage> const&, geom::Size size, MirPixelFormat)
+                {
+                    return std::make_shared<NiceMock<MockBuffer>>(size);
+                }));
     }
-
     MOCK_METHOD3(create_buffer, std::shared_ptr<mcl::ClientBuffer>(
         std::shared_ptr<MirBufferPackage> const&, geom::Size, MirPixelFormat));
 };
@@ -85,6 +90,10 @@
     {
         package.set_width(size.width.as_int());
         package.set_height(size.height.as_int());
+        package2.set_width(size.width.as_int());
+        package2.set_height(size.height.as_int());
+        package3.set_width(size.width.as_int());
+        package3.set_height(size.height.as_int());
 
         package.set_buffer_id(1);
         package2.set_buffer_id(2);
@@ -145,7 +154,7 @@
 
 TEST_F(BufferVault, updates_buffer_on_subsequent_insertions)
 {
-    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>();
+    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>(size);
     EXPECT_CALL(*mock_buffer, update_from(_));
     ON_CALL(mock_factory, create_buffer(_,_,_))
         .WillByDefault(Return(mock_buffer));
@@ -195,7 +204,7 @@
 
 TEST_F(StartedBufferVault, depositing_external_buffer_throws)
 {
-    auto buffer = std::make_shared<MockBuffer>(); 
+    auto buffer = std::make_shared<MockBuffer>(size); 
     EXPECT_THROW({ 
         vault.deposit(buffer);
     }, std::logic_error);
@@ -265,7 +274,7 @@
 
 TEST_F(BufferVault, ages_buffer_on_deposit)
 {
-    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>();
+    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>(size);
     EXPECT_CALL(*mock_buffer, increment_age());
     ON_CALL(mock_factory, create_buffer(_,_,_))
         .WillByDefault(Return(mock_buffer));
@@ -278,7 +287,7 @@
 
 TEST_F(BufferVault, marks_as_submitted_on_transfer)
 {
-    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>();
+    auto mock_buffer = std::make_shared<NiceMock<MockBuffer>>(size);
     EXPECT_CALL(*mock_buffer, mark_as_submitted());
     ON_CALL(mock_factory, create_buffer(_,_,_))
         .WillByDefault(Return(mock_buffer));
@@ -304,3 +313,71 @@
     }
     EXPECT_THAT(buffers, Each(buffers[0]));
 }
+
+
+TEST_F(StartedBufferVault, reallocates_incoming_buffers_of_incorrect_size_with_immediate_response)
+{
+    mp::Buffer package4;
+    geom::Size new_size{80, 100}; 
+    EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
+    EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_))
+        .WillOnce(Invoke(
+        [&, this](geom::Size sz, MirPixelFormat, int)
+        {
+            package4.set_width(sz.width.as_int());
+            package4.set_height(sz.height.as_int());
+            package4.set_buffer_id(4);
+            vault.wire_transfer_inbound(package4);
+        }));
+
+    vault.set_size(new_size);
+    vault.wire_transfer_inbound(package);
+    Mock::VerifyAndClearExpectations(&mock_requests);
+}
+
+TEST_F(StartedBufferVault, reallocates_incoming_buffers_of_incorrect_size_with_delayed_response)
+{
+    geom::Size new_size{80, 100}; 
+    mp::Buffer package4;
+    package4.set_width(new_size.width.as_int());
+    package4.set_height(new_size.height.as_int());
+    package4.set_buffer_id(4);
+
+    EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
+    EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_));
+
+    vault.set_size(new_size);
+    vault.wire_transfer_inbound(package);
+    vault.wire_transfer_inbound(package4);
+    EXPECT_THAT(vault.withdraw().get()->size(), Eq(new_size));
+    Mock::VerifyAndClearExpectations(&mock_requests);
+}
+
+TEST_F(StartedBufferVault, withdraw_gives_only_newly_sized_buffers_after_resize)
+{
+    mp::Buffer package4;
+    geom::Size new_size{80, 100};
+    package4.set_width(new_size.width.as_int());
+    package4.set_height(new_size.height.as_int());
+    package4.set_buffer_id(4);
+ 
+    vault.set_size(new_size);
+    vault.wire_transfer_inbound(package4);
+
+    EXPECT_THAT(vault.withdraw().get()->size(), Eq(new_size));
+    Mock::VerifyAndClearExpectations(&mock_requests);
+}
+
+TEST_F(StartedBufferVault, simply_setting_size_triggers_no_server_interations)
+{
+    EXPECT_CALL(mock_requests, free_buffer(_)).Times(0);
+    EXPECT_CALL(mock_requests, allocate_buffer(_,_,_)).Times(0);
+    auto const cycles = 30u;
+    geom::Size new_size(80, 100);
+    for(auto i = 0u; i < cycles; i++)
+    {
+        new_size = geom::Size(geom::Width(i), new_size.height);
+        vault.set_size(new_size);
+    }
+    Mock::VerifyAndClearExpectations(&mock_requests);
+}