Merge lp:~vanvugt/mir/resize-BufferBundle into lp:mir

[Daniel van Vugt]

Add resize() support to BufferBundle. This is the first step and lowest level
of surface resize support.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1184
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/293/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/22/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/293/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

46 +void mc::SwitchingBundle::resize(const geometry::Size &newsize)
47 +{
48 + bundle_properties.size = newsize;
49 +}

We should guard this with a lock, since bundle_properties is not immutable any more.
The same applies to mc::SwitchingBundle::properties(), and also for allow_framedropping(),
framedropping_allowed() (existing problem).

review: Needs Fixing

[Kevin DuBois (kdub)]

apart from the locking, looks good to me. will 'pre-approve'

review: Approve

[Daniel van Vugt (vanvugt)]

Thanks for catching that. Locking added.

There is an argument that simple setting and getting of bools is "atomic enough" and that they don't need to be locked. But I think we should lock them, if for no other reason than to keep the helgrind warnings down.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1187
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/297/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/63
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/62/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/26
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/26/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/26
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/26/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/297/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Ignore the clang failure. That's bug 1246590.

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/test/mir_test_doubles/mock_buffer_bundle.h'
--- include/test/mir_test_doubles/mock_buffer_bundle.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_buffer_bundle.h	2013-10-31 03:22:31 +0000
@@ -47,6 +47,7 @@
     MOCK_CONST_METHOD0(properties, graphics::BufferProperties());
     MOCK_METHOD0(force_client_abort, void());
     MOCK_METHOD0(force_requests_to_complete, void());
+    MOCK_METHOD1(resize, void(const geometry::Size &));
 };
 
 }

=== modified file 'src/server/compositor/buffer_bundle.h'
--- src/server/compositor/buffer_bundle.h	2013-08-28 03:41:48 +0000
+++ src/server/compositor/buffer_bundle.h	2013-10-31 03:22:31 +0000
@@ -45,6 +45,7 @@
     virtual graphics::BufferProperties properties() const = 0;
     virtual void allow_framedropping(bool dropping_allowed) = 0;
     virtual void force_requests_to_complete() = 0;
+    virtual void resize(const geometry::Size &newsize) = 0;
 protected:
     BufferBundle() = default;
     BufferBundle(BufferBundle const&) = delete;

=== modified file 'src/server/compositor/switching_bundle.cpp'
--- src/server/compositor/switching_bundle.cpp	2013-10-18 09:54:49 +0000
+++ src/server/compositor/switching_bundle.cpp	2013-10-31 03:22:31 +0000
@@ -239,6 +239,12 @@
         }
     }
 
+    if (ret->size() != bundle_properties.size)
+    {
+        ret = gralloc->alloc_buffer(bundle_properties);
+        ring[client].buf = ret;
+    }
+
     return ret;
 }
 
@@ -391,15 +397,24 @@
 
 void mc::SwitchingBundle::allow_framedropping(bool allow_dropping)
 {
+    std::unique_lock<std::mutex> lock(guard);
     framedropping = allow_dropping;
 }
 
 bool mc::SwitchingBundle::framedropping_allowed() const
 {
+    std::unique_lock<std::mutex> lock(guard);
     return framedropping;
 }
 
 mg::BufferProperties mc::SwitchingBundle::properties() const
 {
+    std::unique_lock<std::mutex> lock(guard);
     return bundle_properties;
 }
+
+void mc::SwitchingBundle::resize(const geometry::Size &newsize)
+{
+    std::unique_lock<std::mutex> lock(guard);
+    bundle_properties.size = newsize;
+}

=== modified file 'src/server/compositor/switching_bundle.h'
--- src/server/compositor/switching_bundle.h	2013-08-28 03:41:48 +0000
+++ src/server/compositor/switching_bundle.h	2013-10-31 03:22:31 +0000
@@ -55,6 +55,14 @@
     void allow_framedropping(bool dropping_allowed);
     bool framedropping_allowed() const;
 
+    /**
+     * Change the dimensions of the buffers contained in the bundle. For
+     * client_acquire, the change is guaranteed to be immediate. For
+     * compositors and snapshotters however, the change may be delayed by
+     * nbuffers frames while old frames of the old size are consumed.
+     */
+    void resize(const geometry::Size &newsize) override;
+
 private:
     graphics::BufferProperties bundle_properties;
     std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;
@@ -86,7 +94,7 @@
     int snapshot;
     int nsnapshotters;
 
-    std::mutex guard;
+    mutable std::mutex guard;
     std::condition_variable cond;
 
     unsigned long last_consumed;

=== modified file 'tests/unit-tests/compositor/test_switching_bundle.cpp'
--- tests/unit-tests/compositor/test_switching_bundle.cpp	2013-10-18 09:54:49 +0000
+++ tests/unit-tests/compositor/test_switching_bundle.cpp	2013-10-31 03:22:31 +0000
@@ -754,3 +754,89 @@
     }
 }
 
+TEST_F(SwitchingBundleTest, resize_affects_client_acquires_immediately)
+{
+    for (int nbuffers = 1; nbuffers <= 5; ++nbuffers)
+    {
+        mc::SwitchingBundle bundle(nbuffers, allocator, basic_properties);
+        unsigned long frameno = 1;
+
+        for (int width = 1; width < 100; ++width)
+        {
+            const geom::Size expect_size{width, width * 2};
+
+            for (int subframe = 0; subframe < 3; ++subframe)
+            {
+                bundle.resize(expect_size);
+                auto client = bundle.client_acquire();
+                ASSERT_EQ(expect_size, client->size());
+                bundle.client_release(client);
+    
+                auto compositor = bundle.compositor_acquire(frameno++);
+                ASSERT_EQ(expect_size, compositor->size());
+                bundle.compositor_release(compositor);
+            }
+        }
+    }
+}
+
+TEST_F(SwitchingBundleTest, compositor_acquires_resized_frames)
+{
+    for (int nbuffers = 1; nbuffers <= 5; ++nbuffers)
+    {
+        mc::SwitchingBundle bundle(nbuffers, allocator, basic_properties);
+        mg::BufferID history[5];
+        unsigned long frameno = 1;
+
+        const int width0 = 123;
+        const int height0 = 456;
+        const int dx = 2;
+        const int dy = -3;
+        int width = width0;
+        int height = height0;
+
+        for (int produce = 0; produce < nbuffers; ++produce)
+        {
+            geom::Size new_size{width, height};
+            width += dx;
+            height += dy;
+
+            bundle.resize(new_size);
+            auto client = bundle.client_acquire();
+            history[produce] = client->id();
+            ASSERT_EQ(new_size, client->size());
+            bundle.client_release(client);
+        }
+
+        width = width0;
+        height = height0;
+    
+        for (int consume = 0; consume < nbuffers; ++consume)
+        {
+            geom::Size expect_size{width, height};
+            width += dx;
+            height += dy;
+
+            auto compositor = bundle.compositor_acquire(frameno++);
+
+            // Verify the compositor gets resized buffers, eventually
+            ASSERT_EQ(expect_size, compositor->size());
+
+            // Verify the compositor gets buffers with *contents*, ie. that
+            // they have not been resized prematurely and are empty.
+            ASSERT_EQ(history[consume], compositor->id());
+
+            bundle.compositor_release(compositor);
+        }
+
+        // Verify the final buffer size sticks
+        const geom::Size final_size{width - dx, height - dy};
+        for (int unchanging = 0; unchanging < 100; ++unchanging)
+        {
+            auto compositor = bundle.compositor_acquire(frameno++);
+            ASSERT_EQ(final_size, compositor->size());
+            bundle.compositor_release(compositor);
+        }
+    }
+}
+

=== modified file 'tests/unit-tests/surfaces/test_surface_stack.cpp'
--- tests/unit-tests/surfaces/test_surface_stack.cpp	2013-10-25 09:01:53 +0000
+++ tests/unit-tests/surfaces/test_surface_stack.cpp	2013-10-31 03:22:31 +0000
@@ -78,6 +78,7 @@
     void force_requests_to_complete() {}
     virtual void allow_framedropping(bool) {}
     virtual mg::BufferProperties properties() const { return mg::BufferProperties{}; };
+    void resize(const geom::Size &) override {}
 };