Mir

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

double
Merge into development-branch

Proposed by Daniel van Vugt on 2014-07-22

Status:

Work in progress

Proposed branch:

lp://qastaging/~vanvugt/mir/double

Merge into:

lp://qastaging/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://qastaging/~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.qastaging.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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~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