Merge lp:~afrantzis/mir/consume-only-not-rendered-buffers into lp:mir

[Alexandros Frantzis]

compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers

Also resolves regressions - LP: #1308843 and LP: #1308844.

compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers

Mix the best aspects of our various approaches to provide non-blocking swap buffers.

This MP uses the buffer consuming thread at the MultiThreadedCompositor level that's already present in mir/devel, but changes it so that it only consumes buffers that are not otherwise consumed by the real compositor, like https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246 does. This avoids "beating" effects that are inherent in our handling of multiple compositors (i.e. multiple monitors).

Like 1hz-rendering-always, the selective buffer rendering is based on knowning when buffers are not used for some amount of time, so our consumption rate needs to be smaller than the least real compositor consumption rate (i.e. vsync). I have used 10Hz in this MP but we can change it to whatever value we want; we can even set it dynamically depending on the vsync rates of the connected screens.

[Alan Griffiths (alan-griffiths)]

I've nothing specifically against this MP but we seem to have a flurry of MPs trying to balance incompatible requirements. Vis:

1. The compositor can always get a buffer for a surface
2. A client can always swap buffers without blocking
3. We don't drop frames
4. There's a finite limit to how many buffers we allocate

As it is logically impossible to do all of these things some requirement has to be relaxed. Which is it?

review: Needs Information

[Robert Carr (robertcarr)]

>> As it is logically impossible to do all of these things some requirement has to be relaxed. Which is it?

The way I have been modeling it in my head is when the screen is off or a client is occluded we may "drop frames"

[Kevin DuBois (kdub)]

+ virtual std::chrono::steady_clock::time_point time_last_buffer_acquired() const = 0;

this makes Renderable leak the buffer stream abstraction even worse than it has before (I expound on this in my mp review: https://code.launchpad.net/~kdub/mir/produce-renderlist-for-particular-compositor/+merge/215052/comments/514979)

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> + virtual std::chrono::steady_clock::time_point time_last_buffer_acquired()
> const = 0;
>
> this makes Renderable leak the buffer stream abstraction even worse than it
> has before (I expound on this in my mp review:
> https://code.launchpad.net/~kdub/mir/produce-renderlist-for-particular-
> compositor/+merge/215052/comments/514979)

Note that the "buffer_acquired" in time_last_buffer_acquired() doesn't refer to the underlying buffer stream, but to Renderable::buffer(). That is, it returns the time point when someone last called Renderable::buffer(), regardless of the underlying implementation.

[Daniel van Vugt (vanvugt)]

I think this is a slightly awkward way to express what I proposed (without using time measurement):
https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694

I'd prefer all future work avoided the unreliability of time measurement. Because with some careful planning you can avoid it.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> I think this is a slightly awkward way to express what I proposed (without
> using time measurement):
> https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694
>
> I'd prefer all future work avoided the unreliability of time measurement.
> Because with some careful planning you can avoid it.

The two proposal are not equivalent. As noted in https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694, the judder branch "reintroduces eglSwapBuffers() blocking for buffers that are not rendered by the real compositors (i.e. for surfaces that are either occluded or off screen). That is, it only keeps eglSwapBuffers() running when all monitors are off."

This proposal keeps eglSwapBuffers() running in all cases (monitors off, surface occluded, surface off-screen).

[Daniel van Vugt (vanvugt)]

Yes, that's what the unocclude branch solves.

I'm not rejecting this one, but I think it needs to be better (avoid time measurement), particularly since I've already proved doing so is possible.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1568
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1370/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1666/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1664
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1238/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1102
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1102/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1107/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1240/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1370/rebuild

review: Needs Fixing (continuous-integration)

[lp:~afrantzis/mir/consume-only-not-rendered-buffers]

1569. By Alexandros Frantzis on 2014-04-23

tests: Fix android build

[Alexandros Frantzis (afrantzis)]

The team has decided to go with an approach that handles non blocking inside the SwitchingBundle (for after 0.1.9).
That approach will also solve the "judder" issue, so since the "judder" issue is not critical (or even noticeable in many cases) we will probably ignore it for 0.1.9. Rejecting this MP.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1569
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1386/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1682
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1257
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1118
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1118/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1123
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1123/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1259
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1259/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1162
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6318

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1386/rebuild

review: Approve (continuous-integration)

Unmerged revisions

1569. By Alexandros Frantzis on 2014-04-23

tests: Fix android build

1568. By Alexandros Frantzis on 2014-04-22

compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers

=== modified file 'examples/render_overlays.cpp'
--- examples/render_overlays.cpp	2014-04-15 05:31:19 +0000
+++ examples/render_overlays.cpp	2014-04-23 14:10:27 +0000
@@ -31,6 +31,7 @@
 #include <chrono>
 #include <csignal>
 #include <iostream>
+#include <atomic>
 
 namespace mg=mir::graphics;
 namespace ml=mir::logging;
@@ -101,7 +102,8 @@
 public:
     DemoRenderable(std::shared_ptr<DemoOverlayClient> const& client, geom::Rectangle rect)
         : client(client),
-          position(rect)
+          position(rect),
+          duration_last_buffer_acquired{std::chrono::steady_clock::duration{0}}
     {
     }
 
@@ -112,6 +114,8 @@
 
     std::shared_ptr<mg::Buffer> buffer(void const*) const override
     {
+        duration_last_buffer_acquired =
+            std::chrono::steady_clock::now().time_since_epoch();
         return client->last_rendered();
     }
 
@@ -150,10 +154,16 @@
         return 1;
     }
 
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const
+    {
+        return std::chrono::steady_clock::time_point{duration_last_buffer_acquired};
+    }
+
 private:
     std::shared_ptr<DemoOverlayClient> const client;
     geom::Rectangle const position;
     glm::mat4 const trans;
+    mutable std::atomic<std::chrono::steady_clock::duration> duration_last_buffer_acquired;
 };
 }
 

=== modified file 'include/platform/mir/graphics/renderable.h'
--- include/platform/mir/graphics/renderable.h	2014-04-15 05:31:19 +0000
+++ include/platform/mir/graphics/renderable.h	2014-04-23 14:10:27 +0000
@@ -23,6 +23,7 @@
 #include <glm/glm.hpp>
 #include <memory>
 #include <list>
+#include <chrono>
 
 namespace mir
 {
@@ -84,6 +85,7 @@
 
     virtual bool shaped() const = 0;  // meaning the pixel format has alpha
     virtual int buffers_ready_for_compositor() const = 0;
+    virtual std::chrono::steady_clock::time_point time_last_buffer_acquired() const = 0;
 
 protected:
     Renderable() = default;

=== modified file 'include/test/mir_test_doubles/fake_renderable.h'
--- include/test/mir_test_doubles/fake_renderable.h	2014-04-15 05:31:19 +0000
+++ include/test/mir_test_doubles/fake_renderable.h	2014-04-23 14:10:27 +0000
@@ -23,6 +23,7 @@
 #define GLM_FORCE_RADIANS
 #include <glm/gtc/matrix_transform.hpp>
 #include <gmock/gmock.h>
+#include <atomic>
 
 namespace mir
 {
@@ -43,7 +44,8 @@
           opacity(opacity),
           rectangular(rectangular),
           visible_(visible),
-          posted(posted)
+          posted(posted),
+          duration_last_buffer_acquired{std::chrono::steady_clock::duration{0}}
     {
     }
 
@@ -79,6 +81,7 @@
 
     std::shared_ptr<graphics::Buffer> buffer(void const*) const override
     {
+        duration_last_buffer_acquired = std::chrono::steady_clock::now().time_since_epoch();
         return buf;
     }
 
@@ -97,6 +100,11 @@
         return 1;
     }
 
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const override
+    {
+        return std::chrono::steady_clock::time_point{duration_last_buffer_acquired};
+    }
+
 private:
     std::shared_ptr<graphics::Buffer> buf;
     mir::geometry::Rectangle rect;
@@ -104,6 +112,7 @@
     bool rectangular;
     bool visible_;
     bool posted;
+    mutable std::atomic<std::chrono::steady_clock::duration> duration_last_buffer_acquired;
 };
 
 } // namespace doubles

=== modified file 'include/test/mir_test_doubles/mock_renderable.h'
--- include/test/mir_test_doubles/mock_renderable.h	2014-04-15 05:31:19 +0000
+++ include/test/mir_test_doubles/mock_renderable.h	2014-04-23 14:10:27 +0000
@@ -56,6 +56,7 @@
     MOCK_CONST_METHOD0(visible, bool());
     MOCK_CONST_METHOD0(shaped, bool());
     MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
+    MOCK_CONST_METHOD0(time_last_buffer_acquired, std::chrono::steady_clock::time_point());
 };
 }
 }

=== modified file 'include/test/mir_test_doubles/stub_renderable.h'
--- include/test/mir_test_doubles/stub_renderable.h	2014-04-15 05:31:19 +0000
+++ include/test/mir_test_doubles/stub_renderable.h	2014-04-23 14:10:27 +0000
@@ -22,6 +22,7 @@
 #include "mir_test_doubles/stub_buffer.h"
 #include <mir/graphics/renderable.h>
 #include <memory>
+#include <atomic>
 
 namespace mir
 {
@@ -33,12 +34,18 @@
 class StubRenderable : public graphics::Renderable
 {
 public:
+    StubRenderable() :
+        duration_last_buffer_acquired{std::chrono::steady_clock::duration{0}}
+    {
+    }
+
     ID id() const override
     {
         return this;
     }
     std::shared_ptr<graphics::Buffer> buffer(void const*) const override
     {
+        duration_last_buffer_acquired = std::chrono::steady_clock::now().time_since_epoch();
         return std::make_shared<StubBuffer>();
     }
     bool alpha_enabled() const
@@ -71,8 +78,14 @@
         return 1;
     }
 
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const override
+    {
+        return std::chrono::steady_clock::time_point{duration_last_buffer_acquired};
+    }
+
 private:
     glm::mat4 trans;
+    mutable std::atomic<std::chrono::steady_clock::duration> duration_last_buffer_acquired;
 };
 
 }

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-04-17 01:34:35 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-04-23 14:10:27 +0000
@@ -183,11 +183,20 @@
     std::shared_ptr<CompositorReport> const report;
 };
 
+/*
+ * Consumes buffers that haven't been consumed by the real compositors
+ * for more than "buffer_consumption_period" time, forcing clients to
+ * render at 1/sec(buffer_consumption_period) Hz even when the respective
+ * surfaces are not rendered in any display. Note that for this to work
+ * properly the buffer consumption period needs to be smaller than the
+ * least real compositor period.
+ */
 class BufferConsumingFunctor : public CompositingFunctor
 {
 public:
     BufferConsumingFunctor(std::shared_ptr<mc::Scene> const& scene)
-        : scene{scene}
+        : scene{scene},
+          buffer_consumption_period{std::chrono::milliseconds{100}}
     {
     }
 
@@ -196,41 +205,28 @@
         run_compositing_loop(
             [this]
             {
-                std::vector<std::shared_ptr<mg::Buffer>> saved_resources;
-
-                auto const& renderables = scene->generate_renderable_list();
-
-                for (auto const& r : renderables)
+                auto const one_buffer_consumption_period_ago =
+                    std::chrono::steady_clock::now();
+
+                std::this_thread::sleep_for(buffer_consumption_period);
+
+                auto const& all = scene->generate_renderable_list();
+                for (auto const& r : all)
                 {
-                    if (r->visible())
-                        saved_resources.push_back(r->buffer(this));
+                    if (r->buffers_ready_for_compositor() &&
+                        r->time_last_buffer_acquired() <= one_buffer_consumption_period_ago)
+                    {
+                        static_cast<void>(r->buffer(this));
+                    }
                 }
 
-                wait_until_next_fake_vsync();
-
                 return false;
             });
     }
 
 private:
-    void wait_until_next_fake_vsync()
-    {
-        using namespace std::chrono;
-        typedef duration<int64_t, std::ratio<1, 60>> vsync_periods;
-
-        /* Truncate to vsync periods */
-        auto const previous_vsync =
-            duration_cast<vsync_periods>(steady_clock::now().time_since_epoch());
-        /* Convert back to a timepoint */
-        auto const previous_vsync_tp =
-            time_point<steady_clock, vsync_periods>{previous_vsync};
-        /* Next vsync time point */
-        auto const next_vsync = previous_vsync_tp + vsync_periods(1);
-
-        std::this_thread::sleep_until(next_vsync);
-    }
-
     std::shared_ptr<mc::Scene> const scene;
+    std::chrono::milliseconds const buffer_consumption_period;
 };
 
 }

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-04-15 05:31:19 +0000
+++ src/server/scene/basic_surface.cpp	2014-04-23 14:10:27 +0000
@@ -124,6 +124,7 @@
     surface_alpha(1.0f),
     first_frame_posted(false),
     hidden(false),
+    duration_last_buffer_acquired(std::chrono::steady_clock::duration{0}),
     nonrectangular(nonrectangular),
     input_rectangles{surface_rect},
     surface_buffer_stream(buffer_stream),
@@ -332,6 +333,7 @@
 
 std::shared_ptr<mg::Buffer> ms::BasicSurface::buffer(void const* user_id) const
 {
+    duration_last_buffer_acquired = std::chrono::steady_clock::now().time_since_epoch();
     return buffer_stream()->lock_compositor_buffer(user_id);
 }
 
@@ -350,6 +352,12 @@
     return surface_buffer_stream->buffers_ready_for_compositor();
 }
 
+std::chrono::steady_clock::time_point
+ms::BasicSurface::time_last_buffer_acquired() const
+{
+    return std::chrono::steady_clock::time_point{duration_last_buffer_acquired};
+}
+
 void ms::BasicSurface::with_most_recent_buffer_do(
     std::function<void(mg::Buffer&)> const& exec)
 {

=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h	2014-04-15 05:31:19 +0000
+++ src/server/scene/basic_surface.h	2014-04-23 14:10:27 +0000
@@ -31,6 +31,7 @@
 #include <memory>
 #include <mutex>
 #include <string>
+#include <atomic>
 
 namespace mir
 {
@@ -127,6 +128,7 @@
     bool alpha_enabled() const override;
     geometry::Rectangle screen_position() const override;
     int buffers_ready_for_compositor() const override;
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const override;
 
     void with_most_recent_buffer_do(
         std::function<void(graphics::Buffer&)> const& exec) override;
@@ -153,6 +155,7 @@
     float surface_alpha;
     bool first_frame_posted;
     bool hidden;
+    mutable std::atomic<std::chrono::steady_clock::duration> duration_last_buffer_acquired;
     const bool nonrectangular;
     std::vector<geometry::Rectangle> input_rectangles;
     std::shared_ptr<compositor::BufferStream> const surface_buffer_stream;

=== modified file 'src/server/scene/surface_stack.cpp'
--- src/server/scene/surface_stack.cpp	2014-04-17 01:34:35 +0000
+++ src/server/scene/surface_stack.cpp	2014-04-23 14:10:27 +0000
@@ -82,6 +82,9 @@
     int buffers_ready_for_compositor() const override
     { return underlying_renderable->buffers_ready_for_compositor(); }
 
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const override
+    { return underlying_renderable->time_last_buffer_acquired(); }
+
     bool visible() const override
     { return visible_; }
 

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-16 14:49:54 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-23 14:10:27 +0000
@@ -308,10 +308,10 @@
 public:
     BufferCountingRenderable() : buffers_requested_{0} {}
 
-    std::shared_ptr<mg::Buffer> buffer(void const*) const override
+    std::shared_ptr<mg::Buffer> buffer(void const* id) const override
     {
         ++buffers_requested_;
-        return std::make_shared<mtd::StubBuffer>();
+        return mtd::StubRenderable::buffer(id);
     }
 
     int buffers_requested() const

=== modified file 'tests/unit-tests/graphics/android/test_hwc_device.cpp'
--- tests/unit-tests/graphics/android/test_hwc_device.cpp	2014-04-15 05:31:19 +0000
+++ tests/unit-tests/graphics/android/test_hwc_device.cpp	2014-04-23 14:10:27 +0000
@@ -101,6 +101,12 @@
     {
         return 1;
     }
+
+    std::chrono::steady_clock::time_point time_last_buffer_acquired() const override
+    {
+        return std::chrono::steady_clock::time_point{};
+    }
+
 private:
     std::shared_ptr<mg::Buffer> buf;
     geom::Rectangle screen_pos;