Merge lp:~vanvugt/mir/unfreeze-1 into lp:mir

[Daniel van Vugt]

Ensure frame dropping never drops the newest frame. Dropping the
newest frame causes stuttering at best (LP: #1379685) or in the case
of an event-driven app could make it appear to freeze indefinitely
(LP: #1396006).

If you want to see an immediate benefit of this branch, try glmark2 in fullscreen so that it uses bypass. Without the branch it stutters like it's at 30Hz. With this branch it's perfectly smooth.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2124
http://jenkins.qa.ubuntu.com/job/mir-ci/2291/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/387/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/387
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/361
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/282
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/282/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/361
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/361/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3602
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16338

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2125
http://jenkins.qa.ubuntu.com/job/mir-ci/2300/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/397
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/397
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/371
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/291
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/291/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/371
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/371/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3611
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16362

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Can confirm the visual improvement and the code seems sensible.

A couple of questions/concerns about the overallocation behavior:

135 + * 5. Overallocate; more buffers! Yes, see below.
136 + */
137 + auto const& buffer = gralloc->alloc_buffer(the_properties);

1. In framedropping mode, could we end up allocating more buffers than needed (e.g. because of some kind of spike) which we will then not release?

2. In framedropping mode, could we end up in a situation in which we continuously allocate buffers, eventually exhausting graphics memory? Could a malicious client force such behavior?

review: Needs Information

[Daniel van Vugt (vanvugt)]

(1) Yes a framedropping surface could end up overallocated if the compositor demand spikes and then subsides (e.g. fullscreen bypass and then restored to a window). However dynamic monitoring of real buffer demand is a tricky problem that I have a separate branch for (hopefully ready this year) and would prefer to keep separate. Also a very uncommon occurrence whose penalty is quite low. So that's an optimization more than a bug.

(2) That concerned me too. However an upper limit on number of buffers allocated is implicitly enforced by the behaviour of the rest of the class. A new buffer is only allocated if the free list is empty and the ready list is size 0 or 1. So a new buffer only comes into being if the compositors (and/or client) are behaving in a way that it's actually required. As soon as the lifecycle moves along a bit, we prefer to reuse buffers already in the free or ready lists, so no new buffers are created.

If you explicitly set a fixed upper limit on the number of buffers, that's actually option #4 described in the comments. It works and I have a branch for that too. But less preferable than what's in this branch I think.

[Alexandros Frantzis (afrantzis)]

OK. Looks good, then.

review: Approve

[Alan Griffiths (alan-griffiths)]

I share the above concerns, but this is an improvement on status quo

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/930/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/434/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/434
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/408/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/110
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/110/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/408
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/408/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3643/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16472

review: Needs Fixing (continuous-integration)

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-11-24 02:16:00 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-12-01 02:09:23 +0000
@@ -135,37 +135,6 @@
     framedrop_policy = policy_provider.create_policy([this]
     {
        std::unique_lock<decltype(guard)> lock{guard};
-
-       if (pending_client_notifications.empty())
-       {
-           /*
-            * This framedrop handler may be in the process of being dispatched
-            * when we try to cancel it by calling swap_unblocked() when we
-            * get a buffer to give back to the client. In this case we cannot
-            * cancel and this function may be called without any pending client
-            * notifications. This is a benign race that we can deal with by
-            * just ignoring the framedrop request.
-            */
-           return;
-       }
-
-       if (ready_to_composite_queue.empty())
-       {
-           /*
-            * NOTE: This can only happen under two circumstances:
-            * 1) Client is single buffered. Don't do that, it's a bad idea.
-            * 2) Client already has a buffer, and is asking for a new one
-            *    without submitting the old one.
-            *
-            *    This shouldn't be exposed to the client as swap_buffers
-            *    blocking, as the client already has something to render to.
-            *
-            *    Our current implementation will never hit this case. If we
-            *    later want clients to be able to own multiple buffers simultaneously
-            *    then we might want to add an entry to the CompositorReport here.
-            */
-           return;
-       }
        drop_frame(std::move(lock));
     });
 }
@@ -198,7 +167,7 @@
     }
 
     /* Last resort, drop oldest buffer from the ready queue */
-    if (frame_dropping_enabled && !ready_to_composite_queue.empty())
+    if (frame_dropping_enabled)
     {
         drop_frame(std::move(lock));
         return;
@@ -468,23 +437,82 @@
         framedrop_policy->swap_unblocked();
         give_buffer_to_client(buffer, std::move(lock));
     }
+    else if (!frame_dropping_enabled && buffers.size() > size_t(nbuffers))
+    {
+        /*
+         * We're overallocated.
+         * If frame_dropping_enabled, keep it that way to avoid having
+         * to repeatedly reallocate. We must need the overallocation due to a
+         * greedy compositor and insufficient nbuffers (LP: #1379685).
+         * If not framedropping then we only overallocated to briefly
+         * guarantee the framedropping policy and poke the client. Safe
+         * to free it then because that's a rare occurrence.
+         */
+        for (auto i = buffers.begin(); i != buffers.end(); ++i)
+        {
+            if (i->get() == buffer)
+            {
+                buffers.erase(i);
+                break;
+            }
+        }
+    }
     else
         free_buffers.push_back(buffer);
 }
 
 void mc::BufferQueue::drop_frame(std::unique_lock<std::mutex> lock)
 {
-    auto buffer_to_give = pop(ready_to_composite_queue);
-    /* Advance compositor buffer so it always points to the most recent
-     * client content
-     */
-    if (!contains(current_compositor_buffer, buffers_sent_to_compositor))
-    {
-       current_buffer_users.clear();
-       void const* const impossible_user_id = this;
-       current_buffer_users.push_back(impossible_user_id);
-       std::swap(buffer_to_give, current_compositor_buffer);
-    }
+    // Make sure there is a client waiting for the frame before we drop it.
+    // If not, then there's nothing to do.
+    if (pending_client_notifications.empty())
+        return;
+
+    mg::Buffer* buffer_to_give = nullptr;
+
+    if (!free_buffers.empty())
+    {  // We expect this to usually be empty, but always check free list first
+        buffer_to_give = free_buffers.back();
+        free_buffers.pop_back();
+    }
+    else if (!ready_to_composite_queue.empty() &&
+             !contains(current_compositor_buffer, buffers_sent_to_compositor))
+    {
+        // Remember current_compositor_buffer is implicitly the front
+        // of the ready queue.
+        buffer_to_give = current_compositor_buffer;
+        current_compositor_buffer = pop(ready_to_composite_queue);
+        current_buffer_users.clear();
+        void const* const impossible_user_id = this;
+        current_buffer_users.push_back(impossible_user_id);
+    }
+    else if (ready_to_composite_queue.size() > 1)
+    {
+        buffer_to_give = pop(ready_to_composite_queue);
+    }
+    else 
+    {
+        /*
+         * Insufficient nbuffers for frame dropping? This means you're either
+         * trying to use frame dropping with bypass/overlays/multimonitor or
+         * have chosen nbuffers too low, or some combination thereof. So
+         * consider the options...
+         *  1. Crash. No, that's really unhelpful.
+         *  2. Drop the visible frame. Probably not; that looks pretty awful.
+         *     Not just tearing but you'll see very ugly polygon rendering
+         *     artifacts.
+         *  3. Drop the newest ready frame. Absolutely not; that will cause
+         *     indefinite freezes or at least stuttering.
+         *  4. Just give a warning and carry on at regular frame rate
+         *     as if framedropping was disabled. That's pretty nice, but we
+         *     can do better still...
+         *  5. Overallocate; more buffers! Yes, see below.
+         */
+        auto const& buffer = gralloc->alloc_buffer(the_properties);
+        buffers.push_back(buffer);
+        buffer_to_give = buffer.get();
+    }
+        
     give_buffer_to_client(buffer_to_give, std::move(lock));
 }
 

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-11-24 02:16:00 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-12-01 02:09:23 +0000
@@ -806,7 +806,7 @@
             handle->release_buffer();
         }
 
-        EXPECT_THAT(ids_acquired.size(), Eq(nbuffers));
+        EXPECT_THAT(ids_acquired.size(), Ge(nbuffers));
     }
 }
 
@@ -1050,6 +1050,87 @@
     }
 }
 
+TEST_F(BufferQueueTest, framedropping_policy_never_drops_newest_frame)
+{  // Regression test for LP: #1396006
+    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+    {
+        mtd::MockFrameDroppingPolicyFactory policy_factory;
+        mc::BufferQueue q(nbuffers,
+                          allocator,
+                          basic_properties,
+                          policy_factory);
+
+        auto first = client_acquire_sync(q);
+        q.client_release(first);
+
+        // Start rendering one (don't finish)
+        auto d = q.compositor_acquire(nullptr);
+        ASSERT_EQ(first, d.get());
+
+        auto second = client_acquire_sync(q);
+        q.client_release(second);
+
+        // Client waits for a new frame
+        auto end = client_acquire_async(q);
+
+        // Surface goes offscreen or occluded; trigger a timeout
+        policy_factory.trigger_policies();
+
+        // If the queue is still willing to drop under these difficult
+        // circumstances (and we don't mind if it doesn't), then ensure
+        // it's never the newest frame that's been discarded.
+        // That could be catastrophic as you never know if a client ever
+        // will produce another frame.
+        if (end->has_acquired_buffer())
+            ASSERT_NE(second, end->buffer());
+
+        q.compositor_release(d);
+    }
+}
+
+TEST_F(BufferQueueTest, framedropping_surface_never_drops_newest_frame)
+{  // Second regression test for LP: #1396006, LP: #1379685
+    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+    {
+        mc::BufferQueue q(nbuffers,
+                          allocator,
+                          basic_properties,
+                          policy_factory);
+
+        q.allow_framedropping(true);
+
+        // Fill 'er up
+        std::vector<mg::Buffer*> order;
+        for (int f = 0; f < nbuffers; ++f)
+        {
+            auto b = client_acquire_sync(q);
+            order.push_back(b);
+            q.client_release(b);
+        }
+
+        // Composite all but one
+        std::vector<std::shared_ptr<mg::Buffer>> compositing;
+        for (int n = 0; n < nbuffers-1; ++n)
+        {
+            auto c = q.compositor_acquire(nullptr);
+            compositing.push_back(c);
+            ASSERT_EQ(order[n], c.get());
+        }
+
+        // Ensure it's not the newest frame that gets dropped to satisfy the
+        // client.
+        auto end = client_acquire_async(q);
+
+        // The queue could solve this problem a few ways. It might choose to
+        // defer framedropping till it's safe, or even allocate additional
+        // buffers. We don't care which, just verify it's not losing the
+        // latest frame. Because the screen could be indefinitely out of date
+        // if that happens...
+        ASSERT_TRUE(!end->has_acquired_buffer() ||
+                    end->buffer() != order.back());
+    }
+}
+
 TEST_F(BufferQueueTest, uncomposited_client_swaps_when_policy_triggered)
 {
     for (int nbuffers = 2;
@@ -1238,15 +1319,18 @@
      * so the next client request should not be satisfied until
      * a compositor releases its buffers */
     auto handle = client_acquire_async(q);
-    EXPECT_THAT(handle->has_acquired_buffer(), Eq(false));
-
-    /* Release compositor buffers so that the client can get one */
-    for (auto const& buffer : buffers)
+    /* ... unless the BufferQueue is overallocating. In that case it will
+     * have succeeding in acquiring immediately.
+     */ 
+    if (!handle->has_acquired_buffer())
     {
-        q.compositor_release(buffer);
+        /* Release compositor buffers so that the client can get one */
+        for (auto const& buffer : buffers)
+        {
+            q.compositor_release(buffer);
+        }
+        EXPECT_THAT(handle->has_acquired_buffer(), Eq(true));
     }
-
-    EXPECT_THAT(handle->has_acquired_buffer(), Eq(true));
 }
 
 TEST_F(BufferQueueTest, compositor_never_owns_client_buffers)