Merge lp:~vanvugt/mir/unfreeze-2 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 would make it appear to freeze indefinitely
(LP: #1396006).

Unfortunately, despite the many options described in the code, occasionally not framedropping is the only option that actually works without any concession to visual quality.

In future I'd like to support a higher max nbuffers that will make the warning message disappear. This is required to properly support evil combinations of bypass/overlay/multimonitor/framedropping. However before that happens I want to land other work that ensures longer buffer queues don't also increase visible lag (work in progress: lp:~vanvugt/mir/ddouble).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2117
http://jenkins.qa.ubuntu.com/job/mir-ci/2281/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/377
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/377
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/351/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/270
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/270/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/351
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/351/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3590/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16305

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

failure logged as lp:1396978

[Daniel van Vugt (vanvugt)]

Hmm, this compromise might not be necessary. I think I know how to work around the (other) Android issue that was blocking overallocation from working (and other work too).

[lp:~vanvugt/mir/unfreeze-2]

2118. By Daniel van Vugt on 2014-11-28

Merge latest trunk

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2118
http://jenkins.qa.ubuntu.com/job/mir-ci/2285/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/381
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/381
    ABORTED: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/355/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/276
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/276/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/355
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/355/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

Unmerged revisions

2118. By Daniel van Vugt on 2014-11-28

Merge latest trunk

2117. By Daniel van Vugt on 2014-11-27

Update comments

2116. By Daniel van Vugt on 2014-11-27

And more comments

2115. By Daniel van Vugt on 2014-11-27

Update comments more

2114. By Daniel van Vugt on 2014-11-27

Better comments

2113. By Daniel van Vugt on 2014-11-27

Log a warning when frame dropping becomes unsafe

2112. By Daniel van Vugt on 2014-11-27

Fix test case that expected framedropping to work properly with only
two buffers.

2111. By Daniel van Vugt on 2014-11-27

Prototype "only drop when it's safe to do so"

2110. By Daniel van Vugt on 2014-11-27

Start again. And again start with the (failing) test cases for
frame dropping.

=== 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-11-28 02:40:47 +0000
@@ -19,6 +19,7 @@
 
 #include "mir/graphics/graphic_buffer_allocator.h"
 #include "mir/graphics/buffer_id.h"
+#include "mir/logging/logger.h"
 
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
@@ -135,38 +136,8 @@
     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));
+       if (!pending_client_notifications.empty())
+           drop_frame(std::move(lock));
     });
 }
 
@@ -198,7 +169,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;
@@ -474,18 +445,68 @@
 
 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);
-    }
-    give_buffer_to_client(buffer_to_give, std::move(lock));
+    mg::Buffer* drop = nullptr;
+
+    if (!free_buffers.empty())
+    {  // We expect this to usually be empty, but always check free list first
+        drop = 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 always silently the front
+        // of the ready queue.
+        drop = 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)
+    {
+        // You can take from the ready queue, so long as it's never the
+        // tail of the queue (the newest frame).
+        drop = pop(ready_to_composite_queue);
+    }
+
+    if (drop)
+    {
+        give_buffer_to_client(drop, std::move(lock));
+    }
+    else
+    {
+        /*
+         * Insufficient nbuffers for frame dropping? This means you're either
+         * trying to use frame dropping with bypass/multimonitor or have
+         * supplied nbuffers < 3. So consider the options...
+         *  1. Crash. No, that's really unhelpful.
+         *  2. Drop the visible frame. Probably not; it looks pretty awful.
+         *     Not just tearing but you'll see very ugly polygon rendering
+         *     artefacts.
+         *  3. Drop the newest ready frame. Absolutely not; that will cause
+         *     indefinite freezes or at least stuttering.
+         *  4. Overallocate; more buffers. Maybe in future but we don't yet
+         *     have a safe and reliable implementation. It seems to
+         *     kill Android rather quickly using four buffers (try glmark2).
+         *  5. Just give a warning and carry on at regular frame rate
+         *     as if framedropping was disabled. Yes, see below.
+         */
+        std::call_once(warn_dropping_failed, [this]()
+        {
+            auto n = std::to_string(nbuffers);
+            auto consuming = std::to_string(buffers_sent_to_compositor.size());
+            auto ready = std::to_string(ready_to_composite_queue.size());
+            auto producing = std::to_string(buffers_owned_by_client.size());
+            mir::logging::log(mir::logging::Severity::warning,
+                              "Insufficient nbuffers("+n+") to support "
+                              "frame dropping ("+
+                              consuming+" compositing, "+
+                              ready+" ready, "+
+                              producing+" with client)",
+                              "BufferQueue");
+        });
+    }
 }
 
 void mc::BufferQueue::drop_old_buffers()

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-11-24 02:16:00 +0000
+++ src/server/compositor/buffer_queue.h	2014-11-28 02:40:47 +0000
@@ -98,6 +98,8 @@
     // Ensure framedrop_policy gets destroyed first so the callback installed
     // does not access dead objects.
     std::unique_ptr<FrameDroppingPolicy> framedrop_policy;
+
+    std::once_flag warn_dropping_failed;
 };
 
 }

=== 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-11-28 02:40:47 +0000
@@ -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;
@@ -1079,7 +1160,7 @@
 
 TEST_F(BufferQueueTest, partially_composited_client_swaps_when_policy_triggered)
 {
-    for (int nbuffers = 2;
+    for (int nbuffers = 3;  // framedropping is only truly safe with 3
          nbuffers <= max_nbuffers_to_test;
          nbuffers++)
     {