Mir

Merge lp:~vanvugt/mir/double into lp:mir

double
Merge into development-branch

Proposed by Daniel van Vugt on 2014-07-22

Status:

Work in progress

Proposed branch:

lp:~vanvugt/mir/double

Merge into:

lp:mir

Diff against target:

850 lines (+446/-53)

4 files modified

src/server/compositor/buffer_queue.cpp (+122/-22)
src/server/compositor/buffer_queue.h (+7/-2)
tests/integration-tests/surface_composition.cpp (+8/-1)
tests/unit-tests/compositor/test_buffer_queue.cpp (+309/-28)

To merge this branch:

bzr merge lp:~vanvugt/mir/double

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-07-22	Approve on 2014-08-01
Daniel van Vugt			Needs Fixing on 2014-07-30
Alexandros Frantzis (community)		2014-07-22	Needs Fixing on 2014-07-29
Alberto Aguirre		2014-07-22	Pending
Review via email: mp+227701@code.launchpad.net

Commit message

Reintroduce double buffering! (LP: #1240909)

Default to double buffering where possible, to minimize visible lag and
resource usage. The queue will be expanded automatically to triple buffers
if you enable framedropping, or if it is observed that the client is not
keeping up with the compositor.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-22:

Fun fact: Adding the heuristic for detecting slow clients fixes:
BufferQueueTest.slow_client_framerate_matches_compositor
without any time-fudging required any more. Turns out it was a very good test.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-22:

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

review: Needs Fixing (continuous-integration)

lp:~vanvugt/mir/double updated on 2014-07-23

1726. By Daniel van Vugt on 2014-07-23: Merge latest development-branch

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-23:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-25:

The failure is bug 1348095. Unrelated to this proposal.

lp:~vanvugt/mir/double updated on 2014-07-28

1727. By Daniel van Vugt on 2014-07-24: Merge latest development-branch
1728. By Daniel van Vugt on 2014-07-25: Merge latest development-branch
1729. By Daniel van Vugt on 2014-07-28: Merge latest development-branch and fix conflicts.
1730. By Daniel van Vugt on 2014-07-28: Merge latest development-branch
1731. By Daniel van Vugt on 2014-07-28: Fix failing test case, merged from devel:
gives_compositor_the_newest_buffer_after_dropping_old_buffers
1732. By Daniel van Vugt on 2014-07-28: Make tests which require 3 buffers more consistent
1733. By Daniel van Vugt on 2014-07-28: Fix failing test: TEST_F(StaleFrames, are_dropped_when_restarting_compositor)
It was designed to assume three unique buffers available by default. Remove
that assumption.
1734. By Daniel van Vugt on 2014-07-28: Lower the resize delay to 100 frames so slow clients only take ~1.5s to
be detected as slow and change to triple buffers.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-28:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-07-28:

Not a thorough review yet, looks good overall on a first pass.

86 + if (client_behind && missed_frames < queue_resize_delay_frames)
87 + {
88 + ++missed_frames;
89 + if (missed_frames >= queue_resize_delay_frames)
90 + extra_buffers = 1;
91 + }
92 +
93 + if (!client_behind && missed_frames > 0)
102 + if (missed_frames < queue_resize_delay_frames)
103 + --missed_frames;

It would be nice to have tests for the expected behavior of this piece of code.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-28:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-29:

There are already tests for that portion of logic:

  TEST_F(BufferQueueTest, slow_client_framerate_matches_compositor)
  TEST_F(BufferQueueTest, queue_size_scales_instantly_on_framedropping)
  TEST_F(BufferQueueTest, queue_size_scales_for_slow_clients)

Although it's always possible to add more.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-07-29:

> Although it's always possible to add more.

One test that is missing (at least not tested explicitly) is to verify that: "If the ceiling is hit, keep it there with the extra buffer allocated so we don't shrink again and cause yet more missed frames."

lp:~vanvugt/mir/double updated on 2014-07-29

1735. By Daniel van Vugt on 2014-07-29

Merge latest development-branch

1736. By Daniel van Vugt on 2014-07-29

Merge latest development-branch

1737. By Daniel van Vugt on 2014-07-29

Add unit test: BufferQueueTest.switch_to_triple_buffers_is_permanent

This verifies we don't accidentally ping-pong between 2 and 3 buffers.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-29:

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

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-07-29:

> * A client that's keeping up will be in-phase with composition. That
> * means it will stay, or quickly equalize at a point where there are
> * no client buffers still held when composition finishes.

This criterion only works well when the client is actually trying to keep up with the compositor (i.e. 60Hz). Clients that refresh less often on purpose or only when reacting to user input will become triple-buffered if the compositor is triggered by another source and they are not updating fast enough.

This happens, for example, if we run both mir_demo_client_egltriangle and mir_demo_client_fingerpaint. The frames from egltriangle trigger the compositor and cause mir_demo_client_fingerpaint to become triple-buffered after a short while. The same happens if we just have mir_demo_client_fingerpaint and move its surface around in the demo shell.

We probably need additional logic/heurestics to make this work properly (haven't thought through what that logic might be, though).

review: Needs Fixing

lp:~vanvugt/mir/double updated on 2014-07-30

1738. By Daniel van Vugt on 2014-07-30: Merge latest development-branch
1739. By Daniel van Vugt on 2014-07-30: Add a regression test for the idle-vs-slow client problem Alexandros
described. Presently failing.
1740. By Daniel van Vugt on 2014-07-30: Move the "missed frames" decision to a point where it won't be skewed by
multiple compositors (multi-monitor).
1741. By Daniel van Vugt on 2014-07-30: Prototype a detection of "idle" clients. Other questionable tests still
failing.
1742. By Daniel van Vugt on 2014-07-30: Simplify
1743. By Daniel van Vugt on 2014-07-30: More readable.
1744. By Daniel van Vugt on 2014-07-30: Fix failing test: BufferQueueTest.queue_size_scales_for_slow_clients
It needed some enhancement to not be detected as an "idle" client by the
new logic.
1745. By Daniel van Vugt on 2014-07-30: Fix test case: BufferQueueTest.switch_to_triple_buffers_is_permanent
The new idle detection logic was not seeing any client activity and so
deemed it not trying to keep up. So added some "slow client" activity.
1746. By Daniel van Vugt on 2014-07-30: Simplified idle detection
1747. By Daniel van Vugt on 2014-07-30: Fix comment typo
1748. By Daniel van Vugt on 2014-07-30: Add another regression test which shows premature expansion for really
slow clients.
1749. By Daniel van Vugt on 2014-07-30: Fixed: Mis-detection of a really slow client (low self-determined frame rate)
as failing to keep up. Regression test now passes.
1750. By Daniel van Vugt on 2014-07-30: Remove debug code

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-30:

Excellent point Alexandros. That's now fixed with extra tests for the scenarios you describe.

Unfortunately I just realized that there's another situation that probably needs fixing: A system with a single slow-ticking client (like a clock) won't ever know that the compositor is capable of running any faster than the client is, and so will classify the client as slow --> triple buffers prematurely.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-30:

I'm now tempted to divide this proposal in two:
1. Basic test case fixes to support future double buffering; and
2. Heuristic switching between double and triple buffering (work in progress).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-30:

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

review: Approve (continuous-integration)

lp:~vanvugt/mir/double updated on 2014-08-01

1751. By Daniel van Vugt on 2014-07-31: Merge latest development-branch
1752. By Daniel van Vugt on 2014-08-01: Merge latest development-branch
1753. By Daniel van Vugt on 2014-08-01: Slightly simpler and more straight forward detection of slow vs idle clients.
1754. By Daniel van Vugt on 2014-08-01: Shrink the diff
1755. By Daniel van Vugt on 2014-08-01: More comments
1756. By Daniel van Vugt on 2014-08-01: Add a regression test for the slow-ticking-client-mostly-idle-desktop case.
1757. By Daniel van Vugt on 2014-08-01: test_buffer_queue.cpp: De-duplicate buffer counting logic
1758. By Daniel van Vugt on 2014-08-01: Fix indentation in new tests
1759. By Daniel van Vugt on 2014-08-01: test_buffer_queue.cpp: Major simplification - reuse a common function to
measure unique buffers.
1760. By Daniel van Vugt on 2014-08-01: Tidy up tests
1761. By Daniel van Vugt on 2014-08-01: Remove unnecessary sleeps from new tests
1762. By Daniel van Vugt on 2014-08-01: Go back to the old algorithm, keep the new tests.
1763. By Daniel van Vugt on 2014-08-01: A slightly more elegant measure of client_lag

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-01:

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

review: Approve (continuous-integration)

lp:~vanvugt/mir/double updated on 2014-11-23

1764. By Daniel van Vugt on 2014-08-04: Merge latest development-branch
1765. By Daniel van Vugt on 2014-08-05: Merge latest development-branch
1766. By Daniel van Vugt on 2014-08-06: Merge latest development-branch
1767. By Daniel van Vugt on 2014-08-07: Merge latest development-branch
1768. By Daniel van Vugt on 2014-08-08: Merge latest development-branch
1769. By Daniel van Vugt on 2014-08-12: Merge latest development-branch
1770. By Daniel van Vugt on 2014-08-18: Merge latest development-branch
1771. By Daniel van Vugt on 2014-09-02: Merge latest development-branch and fix conflicts.
1772. By Daniel van Vugt on 2014-09-22: Merge latest development-branch
1773. By Daniel van Vugt on 2014-09-25: Merge latest development-branch
1774. By Daniel van Vugt on 2014-10-01: Merge latest development-branch
1775. By Daniel van Vugt on 2014-10-01: Merge latest development-branch
1776. By Daniel van Vugt on 2014-10-02: Merge latest development-branch
1777. By Daniel van Vugt on 2014-10-03: Merge latest development-branch
1778. By Daniel van Vugt on 2014-10-03: Merge (rebase on) 'double-integration-tests' to shrink the diff of
this branch to its new parent.
1779. By Daniel van Vugt on 2014-10-06: Merge latest development-branch
1780. By Daniel van Vugt on 2014-10-13: Merge latest development-branch and fix conflicts.
1781. By Daniel van Vugt on 2014-10-14: Merge latest development-branch
1782. By Daniel van Vugt on 2014-10-15: Merge latest development-branch
1783. By Daniel van Vugt on 2014-10-21: Merge latest development-branch
1784. By Daniel van Vugt on 2014-10-22: Merge latest development-branch
1785. By Daniel van Vugt on 2014-10-29: Merge latest development-branch and fix a conflict.
1786. By Daniel van Vugt on 2014-11-03: Merge latest development-branch
1787. By Daniel van Vugt on 2014-11-04: Merge latest development-branch
1788. By Daniel van Vugt on 2014-11-05: Merge latest development-branch
1789. By Daniel van Vugt on 2014-11-06: Merge latest trunk
1790. By Daniel van Vugt on 2014-11-07: Merge latest trunk
1791. By Daniel van Vugt on 2014-11-10: Merge latest trunk
1792. By Daniel van Vugt on 2014-11-11: Merge latest trunk
1793. By Daniel van Vugt on 2014-11-12: Merge latest trunk
1794. By Daniel van Vugt on 2014-11-13: Merge latest trunk
1795. By Daniel van Vugt on 2014-11-14: Merge latest trunk
1796. By Daniel van Vugt on 2014-11-23: Merge latest trunk
1797. By Daniel van Vugt on 2014-11-23: Prototype fix for the crashing integration test
1798. By Daniel van Vugt on 2014-11-23: A simpler fix for the crashing test -- don't try to release the same
old_buffer multiple times.
1799. By Daniel van Vugt on 2014-11-23: Even simpler

Unmerged revisions

1799. By Daniel van Vugt on 2014-11-23: Even simpler
1798. By Daniel van Vugt on 2014-11-23: A simpler fix for the crashing test -- don't try to release the same
old_buffer multiple times.
1797. By Daniel van Vugt on 2014-11-23: Prototype fix for the crashing integration test
1796. By Daniel van Vugt on 2014-11-23: Merge latest trunk
1795. By Daniel van Vugt on 2014-11-14: Merge latest trunk
1794. By Daniel van Vugt on 2014-11-13: Merge latest trunk
1793. By Daniel van Vugt on 2014-11-12: Merge latest trunk
1792. By Daniel van Vugt on 2014-11-11: Merge latest trunk
1791. By Daniel van Vugt on 2014-11-10: Merge latest trunk
1790. By Daniel van Vugt on 2014-11-07: Merge latest trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/server/compositor/buffer_queue.cpp'
 --- src/server/compositor/buffer_queue.cpp	2014-10-29 06:21:10 +0000
 +++ src/server/compositor/buffer_queue.cpp	2014-11-23 08:19:33 +0000
@@ -92,18 +92,23 @@
+ }
  mc::BufferQueue::BufferQueue(
--    int nbuffers,
++    int max_buffers,
      std::shared_ptr<graphics::GraphicBufferAllocator> const& gralloc,
      graphics::BufferProperties const& props,
      mc::FrameDroppingPolicyFactory const& policy_provider)
--    : nbuffers{nbuffers},
++    : min_buffers{std::min(2, max_buffers)}, // TODO: Configurable in future
++      max_buffers{max_buffers},
++      missed_frames{0},
++      queue_resize_delay_frames{100},
++      extra_buffers{0},
++      client_lag{0},
        frame_dropping_enabled{false},
        the_properties{props},
        force_new_compositor_buffer{false},
        callbacks_allowed{true},
        gralloc{gralloc}
+ {
--    if (nbuffers < 1)
++    if (max_buffers < 1)
+     {
          BOOST_THROW_EXCEPTION(
              std::logic_error("invalid number of buffers for BufferQueue"));
@@ -111,9 +116,9 @@
      /* By default not all buffers are allocated.
       * If there is increased pressure by the client to acquire
--     * more buffers, more will be allocated at that time (up to nbuffers)
++     * more buffers, more will be allocated at that time (up to max_buffers)
       */
--    for(int i = 0; i < std::min(nbuffers, 2); i++)
++    for (int i = 0; i < min_buffers; ++i)
+     {
          buffers.push_back(gralloc->alloc_buffer(the_properties));
+     }
@@ -129,7 +134,7 @@
      /* Special case: with one buffer both clients and compositors
       * need to share the same buffer
       */
--    if (nbuffers == 1)
++    if (max_buffers == 1)
          free_buffers.push_back(current_compositor_buffer);
      framedrop_policy = policy_provider.create_policy([this]
@@ -173,6 +178,7 @@
  void mc::BufferQueue::client_acquire(mc::BufferQueue::Callback complete)
+ {
      std::unique_lock<decltype(guard)> lock(guard);
++//    fprintf(stderr, "%s\n", __FUNCTION__);
      pending_client_notifications.push_back(std::move(complete));
@@ -184,12 +190,8 @@
          return;
+     }
--    /* No empty buffers, attempt allocating more
--     * TODO: need a good heuristic to switch
--     * between double-buffering to n-buffering
--     */
      int const allocated_buffers = buffers.size();
--    if (allocated_buffers < nbuffers)
++    if (allocated_buffers < ideal_buffers())
+     {
          auto const& buffer = gralloc->alloc_buffer(the_properties);
          buffers.push_back(buffer);
@@ -215,6 +217,8 @@
+ {
      std::lock_guard<decltype(guard)> lock(guard);
++//    fprintf(stderr, "%s\n", __FUNCTION__);
++
      if (buffers_owned_by_client.empty())
+     {
          BOOST_THROW_EXCEPTION(
@@ -235,6 +239,7 @@
  mc::BufferQueue::compositor_acquire(void const* user_id)
+ {
      std::unique_lock<decltype(guard)> lock(guard);
++//    fprintf(stderr, "%s\n", __FUNCTION__);
      bool use_current_buffer = false;
      if (!current_buffer_users.empty() && !is_a_current_buffer_user(user_id))
@@ -280,7 +285,10 @@
          buffer_for(current_compositor_buffer, buffers);
      if (buffer_to_release)
++    {
++        client_lag = 1;
          release(buffer_to_release, std::move(lock));
++    }
      return acquired_buffer;
+ }
@@ -288,6 +296,7 @@
  void mc::BufferQueue::compositor_release(std::shared_ptr<graphics::Buffer> const& buffer)
+ {
      std::unique_lock<decltype(guard)> lock(guard);
++//    fprintf(stderr, "%s\n", __FUNCTION__);
      if (!remove(buffer.get(), buffers_sent_to_compositor))
+     {
@@ -299,7 +308,52 @@
      if (contains(buffer.get(), buffers_sent_to_compositor))
          return;
--    if (nbuffers <= 1)
++    /*
++     * Calculate if we need extra buffers in the queue to account for a slow
++     * client that can't keep up with composition.
++     */
++    if (frame_dropping_enabled)
++    {
++        missed_frames = 0;
++        extra_buffers = 0;
++    }
++    else
++    {
++        /*
++         * A client that's keeping up will be in-phase with composition. That
++         * means it will stay, or quickly equalize at a point where there are
++         * no client buffers still held when composition finishes.
++         */
++        if (client_lag == 1 && missed_frames < queue_resize_delay_frames)
++        {
++            ++missed_frames;
++            if (missed_frames >= queue_resize_delay_frames)
++                extra_buffers = 1;
++        }
++
++        if (client_lag != 1 && missed_frames > 0)
++        {
++            /*
++             * Allow missed_frames to recover back down to zero, so long as
++             * the ceiling is never hit (meaning you're keeping up most of the
++             * time). If the ceiling is hit, keep it there with the extra
++             * buffer allocated so we don't shrink again and cause yet more
++             * missed frames.
++             */
++            if (missed_frames < queue_resize_delay_frames)
++                --missed_frames;
++        }
++
++//        fprintf(stderr, "missed_frames %d, extra %d\n",
++//            missed_frames, extra_buffers);
++    }
++
++    // Let client_lag go above 1 to represent an idle client (one that's
++    // sleeping and not trying to keep up with the compositor)
++    if (client_lag)
++        ++client_lag;
++
++    if (max_buffers <= 1)
          return;
      /*
@@ -322,7 +376,10 @@
+     }
      if (current_compositor_buffer != buffer.get())
++    {
++        client_lag = 0;
          release(buffer.get(), std::move(lock));
++    }
+ }
  std::shared_ptr<mg::Buffer> mc::BufferQueue::snapshot_acquire()
@@ -354,6 +411,14 @@
+ {
      std::lock_guard<decltype(guard)> lock(guard);
      frame_dropping_enabled = flag;
++
++    while (static_cast<int>(buffers.size()) > ideal_buffers() &&
++           !free_buffers.empty())
++    {
++        auto surplus = free_buffers.back();
++        free_buffers.pop_back();
++        drop_buffer(surplus);
++    }
+ }
  bool mc::BufferQueue::framedropping_allowed() const
@@ -397,20 +462,14 @@
  int mc::BufferQueue::buffers_free_for_client() const
+ {
      std::lock_guard<decltype(guard)> lock(guard);
--    int ret = 1;
--    if (nbuffers > 1)
--    {
--        int nfree = free_buffers.size();
--        int future_growth = nbuffers - buffers.size();
--        ret = nfree + future_growth;
--    }
--    return ret;
++    return max_buffers > 1 ? free_buffers.size() : 1;
+ }
  void mc::BufferQueue::give_buffer_to_client(
      mg::Buffer* buffer,
      std::unique_lock<std::mutex> lock)
+ {
++//    fprintf(stderr, "%s\n", __FUNCTION__);
      /* Clears callback */
      auto give_to_client_cb = std::move(pending_client_notifications.front());
      pending_client_notifications.pop_front();
@@ -424,7 +483,7 @@
          /* Special case: the current compositor buffer also needs to be
           * replaced as it's shared with the client
           */
--        if (nbuffers == 1)
++        if (max_buffers == 1)
              current_compositor_buffer = buffer;
+     }
@@ -463,7 +522,13 @@
      mg::Buffer* buffer,
      std::unique_lock<std::mutex> lock)
+ {
--    if (!pending_client_notifications.empty())
++    int used_buffers = buffers.size() - free_buffers.size();
++
++    if (used_buffers > ideal_buffers() && max_buffers > 1)
++    {
++        drop_buffer(buffer);
++    }
++    else if (!pending_client_notifications.empty())
+     {
          framedrop_policy->swap_unblocked();
          give_buffer_to_client(buffer, std::move(lock));
@@ -472,6 +537,41 @@
          free_buffers.push_back(buffer);
+ }
++int mc::BufferQueue::ideal_buffers() const
++{
++    int result = frame_dropping_enabled ? 3 : 2;
++
++    // Add extra buffers if we can see the client's not keeping up with
++    // composition.
++    result += extra_buffers;
++
++    if (result < min_buffers)
++        result = min_buffers;
++    if (result > max_buffers)
++        result = max_buffers;
++
++    return result;
++}
++
++void mc::BufferQueue::set_resize_delay(int nframes)
++{
++    queue_resize_delay_frames = nframes;
++    if (nframes == 0)
++        extra_buffers = 1;
++}
++
++void mc::BufferQueue::drop_buffer(graphics::Buffer* buffer)
++{
++    for (auto i = buffers.begin(); i != buffers.end(); ++i)
++    {
++        if (i->get() == buffer)
++        {
++            buffers.erase(i);
++            break;
++        }
++    }
++}
++
  void mc::BufferQueue::drop_frame(std::unique_lock<std::mutex> lock)
+ {
      auto buffer_to_give = pop(ready_to_composite_queue);
 === modified file 'src/server/compositor/buffer_queue.h'
 --- src/server/compositor/buffer_queue.h	2014-10-29 06:21:10 +0000
 +++ src/server/compositor/buffer_queue.h	2014-11-23 08:19:33 +0000
@@ -42,7 +42,7 @@
  public:
      typedef std::function<void(graphics::Buffer* buffer)> Callback;
--    BufferQueue(int nbuffers,
++    BufferQueue(int max_buffers,
                  std::shared_ptr<graphics::GraphicBufferAllocator> const& alloc,
                  graphics::BufferProperties const& props,
                  FrameDroppingPolicyFactory const& policy_provider);
@@ -64,6 +64,7 @@
      bool is_a_current_buffer_user(void const* user_id) const;
      void drop_old_buffers() override;
      void drop_client_requests() override;
++    void set_resize_delay(int nframes);  ///< negative means never resize
  private:
      void give_buffer_to_client(graphics::Buffer* buffer,
@@ -71,6 +72,8 @@
      void release(graphics::Buffer* buffer,
          std::unique_lock<std::mutex> lock);
      void drop_frame(std::unique_lock<std::mutex> lock);
++    int ideal_buffers() const;
++    void drop_buffer(graphics::Buffer* buffer);
      mutable std::mutex guard;
@@ -86,7 +89,9 @@
      std::deque<Callback> pending_client_notifications;
--    int nbuffers;
++    int const min_buffers, max_buffers;
++    int missed_frames, queue_resize_delay_frames, extra_buffers;
++    int client_lag;
      bool frame_dropping_enabled;
      graphics::BufferProperties the_properties;
      bool force_new_compositor_buffer;
 === modified file 'tests/integration-tests/surface_composition.cpp'
 --- tests/integration-tests/surface_composition.cpp	2014-10-02 11:56:12 +0000
 +++ tests/integration-tests/surface_composition.cpp	2014-11-23 08:19:33 +0000
@@ -108,16 +108,23 @@
      mg::Buffer* old_buffer{nullptr};
++    bool called_back = true;
      auto const callback = [&] (mg::Buffer* new_buffer)
+         {
              // If surface is dead then callback is not expected
              EXPECT_THAT(surface.get(), NotNull());
              old_buffer = new_buffer;
++            called_back = true;
          };
      // Exhaust the buffers to ensure we have a pending swap to complete
--    for (auto i = 0; i != number_of_buffers; ++i)
++    // But also be careful to not pass a formerly released non-null old_buffer
++    // in to swap_buffers...
++    while (called_back)
++    {
++        called_back = false;
          surface->swap_buffers(old_buffer, callback);
++    }
      auto const renderable = surface->compositor_snapshot(this);
 === modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
 --- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-10-29 06:21:10 +0000
 +++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-11-23 08:19:33 +0000
@@ -304,6 +304,7 @@
      int const nbuffers = 3;
      mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++    q.set_resize_delay(0); // Force enabling of the third buffer
      int const prefill = q.buffers_free_for_client();
      ASSERT_THAT(prefill, Gt(0));
      for (int i = 0; i < prefill; ++i)
@@ -314,10 +315,7 @@
+     }
      auto handle1 = client_acquire_async(q);
--    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
--
      auto handle2 = client_acquire_async(q);
--    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
      for (int i = 0; i < nbuffers + 1; ++i)
          q.compositor_release(q.compositor_acquire(this));
@@ -386,7 +384,6 @@
      handle->release_buffer();
      handle = client_acquire_async(q);
--    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
      handle->release_buffer();
      auto comp_buffer = q.compositor_acquire(this);
@@ -400,6 +397,8 @@
+     {
          mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        q.set_resize_delay(0); // Force enabling of the third buffer
++
          auto handle1 = client_acquire_async(q);
          ASSERT_THAT(handle1->has_acquired_buffer(), Eq(true));
@@ -426,7 +425,7 @@
              compositor_thread, std::ref(q), std::ref(done));
          std::unordered_set<uint32_t> ids_acquired;
--        int const max_ownable_buffers = nbuffers - 1;
++        int const max_ownable_buffers = q.buffers_free_for_client();
          for (int i = 0; i < max_ownable_buffers*2; ++i)
+         {
              std::vector<mg::Buffer *> client_buffers;
@@ -445,7 +444,9 @@
+             }
+         }
--        EXPECT_THAT(ids_acquired.size(), Eq(nbuffers));
++        // Expect only two since we're now double-buffered and are not
++        // allowing frame dropping.
++        EXPECT_THAT(ids_acquired.size(), Eq(2));
+     }
+ }
@@ -499,7 +500,7 @@
+         {
              std::deque<mg::BufferID> client_release_sequence;
              std::vector<mg::Buffer *> buffers;
--            int const max_ownable_buffers = nbuffers - 1;
++            int const max_ownable_buffers = q.buffers_free_for_client();
              for (int i = 0; i < max_ownable_buffers; ++i)
+             {
                  auto handle = client_acquire_async(q);
@@ -632,13 +633,13 @@
      handle->release_buffer();
      handle = client_acquire_async(q);
--    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
      // in the original bug, compositor would be given the wrong buffer here
      auto compositor_buffer = q.compositor_acquire(this);
      EXPECT_THAT(compositor_buffer->id(), Eq(first_ready_buffer_id));
++    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
      handle->release_buffer();
      q.compositor_release(compositor_buffer);
+ }
@@ -806,7 +807,14 @@
              handle->release_buffer();
+         }
--        EXPECT_THAT(ids_acquired.size(), Eq(nbuffers));
++        /*
++         * Dynamic queue scaling sensibly limits the framedropping client to
++         * two non-overlapping buffers, before overwriting old ones. Allowing
++         * any more would just waste space (buffers). So this means a
++         * frame-dropping client won't ever see more than 3 unique buffers
++         */
++        int const max_ownable_buffers = std::min(nbuffers, 3);
++        EXPECT_THAT(ids_acquired.size(), Eq(max_ownable_buffers));
+     }
+ }
@@ -902,6 +910,7 @@
          auto const frame_time = std::chrono::milliseconds(16);
          q.allow_framedropping(false);
++        q.set_resize_delay(1);
          std::atomic<bool> done(false);
          std::mutex sync;
@@ -977,20 +986,13 @@
+     }
+ }
--namespace
--{
--int max_ownable_buffers(int nbuffers)
--{
--    return (nbuffers == 1) ? 1 : nbuffers - 1;
--}
--}
--
  TEST_F(BufferQueueTest, compositor_acquires_resized_frames)
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
          mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
          std::vector<mg::BufferID> history;
++        std::shared_ptr<AcquireWaitHandle> producing[5];
          const int width0 = 123;
          const int height0 = 456;
@@ -1001,8 +1003,7 @@
          int const nbuffers_to_use = q.buffers_free_for_client();
          ASSERT_THAT(nbuffers_to_use, Gt(0));
--        int max_buffers{max_ownable_buffers(nbuffers)};
--        for (int produce = 0; produce < max_buffers; ++produce)
++        for (int produce = 0; produce < nbuffers_to_use; ++produce)
+         {
              geom::Size new_size{width, height};
              width += dx;
@@ -1012,16 +1013,23 @@
              auto handle = client_acquire_async(q);
              ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
              history.emplace_back(handle->id());
++            producing[produce] = handle;
              auto buffer = handle->buffer();
              ASSERT_THAT(buffer->size(), Eq(new_size));
--            handle->release_buffer();
++        }
++
++        // Overlap all the client_acquires asyncronously. It's the only way
++        // the new dynamic queue scaling will let a client hold that many...
++        for (int complete = 0; complete < nbuffers_to_use; ++complete)
++        {
++            producing[complete]->release_buffer();
+         }
          width = width0;
          height = height0;
--        ASSERT_THAT(history.size(), Eq(max_buffers));
--        for (int consume = 0; consume < max_buffers; ++consume)
++        ASSERT_THAT(history.size(), Eq(nbuffers_to_use));
++        for (int consume = 0; consume < nbuffers_to_use; ++consume)
+         {
              geom::Size expect_size{width, height};
              width += dx;
@@ -1062,7 +1070,8 @@
                            basic_properties,
                            policy_factory);
--        for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++        auto max_ownable_buffers = q.buffers_free_for_client();
++        for (int i = 0; i < max_ownable_buffers; i++)
+         {
              auto client = client_acquire_sync(q);
              q.client_release(client);
@@ -1089,7 +1098,8 @@
                            basic_properties,
                            policy_factory);
--        for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++        int const max_ownable_buffers = q.buffers_free_for_client();
++        for (int i = 0; i < max_ownable_buffers; i++)
+         {
              auto client = client_acquire_sync(q);
              q.client_release(client);
@@ -1379,7 +1389,7 @@
             compositor_thread, std::ref(q), std::ref(done));
          std::unordered_set<mg::Buffer *> unique_buffers_acquired;
--        int const max_ownable_buffers = nbuffers - 1;
++        int const max_ownable_buffers = q.buffers_free_for_client();
          for (int frame = 0; frame < max_ownable_buffers*2; frame++)
+         {
              std::vector<mg::Buffer *> client_buffers;
@@ -1398,13 +1408,14 @@
+             }
+         }
--        EXPECT_THAT(unique_buffers_acquired.size(), Eq(nbuffers));
++        // Expect one more than max_ownable_buffers, to include the one that
++        // is silently reserved for compositing.
++        EXPECT_THAT(unique_buffers_acquired.size(), Eq(max_ownable_buffers+1));
+     }
+ }
--/* FIXME (enabling this optimization breaks timing tests) */
--TEST_F(BufferQueueTest, DISABLED_synchronous_clients_only_get_two_real_buffers)
++TEST_F(BufferQueueTest, synchronous_clients_only_get_two_real_buffers)
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
@@ -1413,6 +1424,13 @@
          std::atomic<bool> done(false);
          auto unblock = [&done] { done = true; };
++
++        // With an unthrottled compositor_thread it will look like the client
++        // isn't keeping up and the buffer queue would normally auto-expand.
++        // Increase the auto-expansion threshold to ensure that doesn't happen
++        // during this test...
++        q.set_resize_delay(-1);
++
          mt::AutoUnblockThread compositor(unblock,
             compositor_thread, std::ref(q), std::ref(done));
@@ -1495,6 +1513,9 @@
      int const nbuffers = 3;
      mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++    // Ensure this test gets 3 real buffers immediately.
++    q.set_resize_delay(0);
++
      auto handle1 = client_acquire_async(q);
      ASSERT_THAT(handle1->has_acquired_buffer(), Eq(true));
      handle1->release_buffer();
@@ -1537,3 +1558,263 @@
      auto comp2 = q.compositor_acquire(new_compositor_id);
      ASSERT_THAT(comp2->id(), Eq(handle2->id()));
+ }
++
++namespace
++{
++    int unique_buffers(mc::BufferQueue& q)
++    {
++        std::atomic<bool> done(false);
++
++        auto unblock = [&done] { done = true; };
++        mt::AutoUnblockThread compositor(unblock,
++           compositor_thread, std::ref(q), std::ref(done));
++
++        std::unordered_set<mg::Buffer*> buffers_acquired;
++        for (int frame = 0; frame < 100; frame++)
++        {
++            auto handle = client_acquire_async(q);
++            handle->wait_for(std::chrono::seconds(1));
++            if (!handle->has_acquired_buffer())
++                return -1;
++            buffers_acquired.insert(handle->buffer());
++            handle->release_buffer();
++        }
++
++        return static_cast<int>(buffers_acquired.size());
++    }
++
++    int unique_synchronous_buffers(mc::BufferQueue& q)
++    {
++        if (q.framedropping_allowed())
++            return 0;
++
++        int const max = 10;
++
++        // Flush the queue
++        for (int f = 0; f < max; ++f)
++            q.compositor_release(q.compositor_acquire(0));
++
++        auto compositor = q.compositor_acquire(0);
++        int count = 1;  // ^ count the compositor buffer
++
++        std::shared_ptr<AcquireWaitHandle> client;
++        while (count < max)
++        {
++            client = client_acquire_async(q);
++            if (!client->has_acquired_buffer())
++                break;
++            ++count;
++            client->release_buffer();
++        }
++
++        q.compositor_release(compositor);
++        EXPECT_TRUE(client->has_acquired_buffer());
++        client->release_buffer();
++
++        // Flush the queue
++        for (int f = 0; f < max; ++f)
++            q.compositor_release(q.compositor_acquire(0));
++
++        return count;
++    }
++} // namespace
++
++TEST_F(BufferQueueTest, queue_size_scales_instantly_on_framedropping)
++{
++    for (int max_buffers = 1; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.set_resize_delay(-1);
++
++        // Default: No frame dropping; expect double buffering
++        q.allow_framedropping(false);
++        EXPECT_EQ(std::min(max_buffers, 2), unique_buffers(q));
++
++        // Enable frame dropping; expect triple buffering immediately
++        q.allow_framedropping(true);
++        EXPECT_EQ(std::min(max_buffers, 3), unique_buffers(q));
++
++        // Revert back to no frame dropping; expect double buffering
++        q.allow_framedropping(false);
++        EXPECT_EQ(std::min(max_buffers, 2), unique_buffers(q));
++    }
++}
++
++TEST_F(BufferQueueTest, queue_size_scales_for_slow_clients)
++{
++    for (int max_buffers = 3; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.allow_framedropping(false);
++
++        int const delay = 10;
++        q.set_resize_delay(delay);
++
++        // Verify we're starting with double buffering
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++
++        // Simulate a slow client. Not an idle one, but one trying to keep up
++        // and repeatedly failing to miss the frame deadline.
++        for (int f = 0; f < delay*2; ++f)
++        {
++            auto client = client_acquire_async(q);
++            q.compositor_release(q.compositor_acquire(this));
++            ASSERT_TRUE(client->has_acquired_buffer());
++            client->release_buffer();
++        }
++
++        // Verify we now have triple buffers
++        EXPECT_EQ(3, unique_synchronous_buffers(q));
++    }
++}
++
++TEST_F(BufferQueueTest, switch_to_triple_buffers_is_permanent)
++{
++    for (int max_buffers = 3; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.allow_framedropping(false);
++
++        int const delay = 10;
++        q.set_resize_delay(delay);
++
++        // First, verify we only have 2 real buffers
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++
++        // Simulate a slow client. Not an idle one, but one trying to keep up
++        // and repeatedly failing to miss the frame deadline.
++        for (int f = 0; f < delay*2; ++f)
++        {
++            auto client = client_acquire_async(q);
++            q.compositor_release(q.compositor_acquire(this));
++            ASSERT_TRUE(client->has_acquired_buffer());
++            client->release_buffer();
++        }
++
++        EXPECT_EQ(3, unique_synchronous_buffers(q));
++
++        // Now let the client behave "well" and keep up.
++        for (int f = 0; f < delay*10; ++f)
++        {
++            q.client_release(client_acquire_sync(q));
++            q.compositor_release(q.compositor_acquire(this));
++        }
++
++        // Make sure the queue has stayed expanded. Do the original test
++        // again and verify clients get one more than before...
++        EXPECT_EQ(3, unique_synchronous_buffers(q));
++    }
++}
++
++TEST_F(BufferQueueTest, idle_clients_dont_get_expanded_buffers)
++{
++    for (int max_buffers = 3; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.allow_framedropping(false);
++
++        int const delay = 10;
++        q.set_resize_delay(delay);
++
++        // First, verify we only have 2 real buffers
++        int const contracted_nbuffers = 2;
++        for (int f = 0; f < contracted_nbuffers-1; ++f)
++        {
++            auto client1 = client_acquire_async(q);
++            ASSERT_TRUE(client1->has_acquired_buffer());
++            client1->release_buffer();
++        }
++        auto client2 = client_acquire_async(q);
++        ASSERT_FALSE(client2->has_acquired_buffer());
++        q.compositor_release(q.compositor_acquire(this));
++        ASSERT_TRUE(client2->has_acquired_buffer());
++
++        // Now hold client2 buffer for a little too long...
++        for (int f = 0; f < delay*2; ++f)
++            q.compositor_release(q.compositor_acquire(this));
++        // this should have resulted in the queue expanding.
++
++        client2->release_buffer();
++
++        // Verify we still only have double buffering. An idle client should
++        // not be a reason to expand to triple.
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++    }
++}
++
++TEST_F(BufferQueueTest, really_slow_clients_dont_get_expanded_buffers)
++{
++    for (int max_buffers = 3; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.allow_framedropping(false);
++
++        int const delay = 10;
++        q.set_resize_delay(delay);
++
++        // First, verify we only have 2 real buffers
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++
++        // Simulate a really slow client (one third frame rate) that can
++        // never be helped by triple buffering.
++        for (int f = 0; f < delay*2; ++f)
++        {
++            auto client = client_acquire_async(q);
++
++            q.compositor_release(q.compositor_acquire(this));
++            q.compositor_release(q.compositor_acquire(this));
++            q.compositor_release(q.compositor_acquire(this));
++
++            ASSERT_TRUE(client->has_acquired_buffer());
++            client->release_buffer();
++        }
++
++        // Verify we still only have double buffering. An idle client should
++        // not be a reason to expand to triple.
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++    }
++}
++
++TEST_F(BufferQueueTest, synchronous_clients_dont_get_expanded_buffers)
++{
++    for (int max_buffers = 3; max_buffers < max_nbuffers_to_test; ++max_buffers)
++    {
++        mc::BufferQueue q(max_buffers, allocator, basic_properties,
++                          policy_factory);
++
++        q.allow_framedropping(false);
++
++        int const delay = 3;
++        q.set_resize_delay(delay);
++
++        // First, verify we only have 2 real buffers
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++
++        // Simulate an idle shell with a single really slow client (like
++        // a clock -- not something that's trying to keep up with the
++        // refresh rate)
++        for (int f = 0; f < delay*2; ++f)
++        {
++            auto client = client_acquire_async(q);
++            ASSERT_TRUE(client->has_acquired_buffer());
++            client->release_buffer();
++
++            q.compositor_release(q.compositor_acquire(this));
++        }
++
++        // Verify we still only have double buffering. An idle desktop driven
++        // by a single slow client should not be a reason to expand to triple.
++        EXPECT_EQ(2, unique_synchronous_buffers(q));
++    }
++}
++

Mir

Merge lp:~vanvugt/mir/double into lp:mir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers