Mir

Merge lp:~mir-team/mir/enable-dynamic-buffer-queue into lp:mir

enable-dynamic-buffer-queue
Merge into development-branch

Proposed by Alberto Aguirre on 2014-06-27

Status:

Work in progress

Proposed branch:

lp:~mir-team/mir/enable-dynamic-buffer-queue

Merge into:

lp:mir

Diff against target:

1537 lines (+400/-361)

10 files modified

include/test/mir_test/signal.h (+1/-0)
src/server/compositor/buffer_queue.cpp (+85/-33)
src/server/compositor/buffer_queue.h (+12/-3)
src/server/compositor/buffer_stream_factory.cpp (+2/-3)
tests/integration-tests/compositor/test_buffer_stream.cpp (+77/-63)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+1/-1)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+2/-1)
tests/integration-tests/test_surface_first_frame_sync.cpp (+1/-1)
tests/mir_test/signal.cpp (+6/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+213/-256)

To merge this branch:

bzr merge lp:~mir-team/mir/enable-dynamic-buffer-queue

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Kevin DuBois (community)			Needs Fixing on 2014-07-07
Daniel van Vugt			Needs Fixing on 2014-07-04
Alexandros Frantzis (community)			Needs Fixing on 2014-07-03
Alan Griffiths		2014-06-27	Abstain on 2014-07-02
PS Jenkins bot (community)	continuous-integration		Approve on 2014-07-02
Review via email: mp+224742@code.launchpad.net

Commit message

Enable dynamic reallocation in BufferQueue

Description of the change

Default to double buffering and enable dynamic reallocation in BufferQueue

This continues the work by Daniel from https://code.launchpad.net/~vanvugt/mir/double/+merge/221431

Change to double buffering by default - it'll will allocate more for async clients (async clients are not possible with the current client api) or in special cases when frame dropping is enabled (multiple compositors or a compositor in bypass).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-27:

FAILED: Continuous integration, rev:1678
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2015/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/723
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/729
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/723/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/536/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/534/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2166/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-06-27:

[ FAILED ] BufferQueueTest.slow_client_framerate_matches_compositor

review: Needs Fixing

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-06-27

1679. By Alberto Aguirre on 2014-06-27: Fix failing test
1680. By Alberto Aguirre on 2014-06-27: merge lp:mir/devel

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-06-30:

In TEST_F(BufferQueueTest, slow_client_framerate_matches_compositor) we're removing the expectation ASSERT_THAT(client_frames, Lt(compose_frames * 1.2f)) - which means we're no longer validating that the "client framerate matches compositor".

Maybe we just need a better name for the test?

review: Needs Information

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-06-30

1681. By Alberto Aguirre on 2014-06-30: ASSERT upperbound in framerate matching tests.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-06-30:

> In TEST_F(BufferQueueTest, slow_client_framerate_matches_compositor) we're
> removing the expectation ASSERT_THAT(client_frames, Lt(compose_frames * 1.2f))
> - which means we're no longer validating that the "client framerate matches
> compositor".
>
> Maybe we just need a better name for the test?

I've added back the upper bound assertion.

Revision history for this message

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

FAILED: Continuous integration, rev:1681
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2032/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/759
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/765
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/758/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/553
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/553/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/551
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/551/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2294/console

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

review: Needs Fixing (continuous-integration)

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-06-30

1682. By Alberto Aguirre on 2014-06-30: Increase test granularity

Revision history for this message

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

FAILED: Continuous integration, rev:1682
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2037/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/765
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/771
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/764/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/558
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/558/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/556/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2306/console

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

review: Needs Fixing (continuous-integration)

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-06-30

1683. By Alberto Aguirre on 2014-06-30: test slow_client_framerate_matches_compositor:
Give a bit more time diff to account for thread switching in arm platform

Revision history for this message

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

FAILED: Continuous integration, rev:1683
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2041/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/771
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/777/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/770
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/562
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/562/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/560
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/560/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2319
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2319/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1929
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9059

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-07-01:

^-- "Start 53: mir_integration_tests.SurfaceStackCompositor.*
Build timed out (after 120 minutes). Marking the build as failed."

Seems like https://bugs.launchpad.net/mir/+bug/1335735

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-07-01

1684. By Alberto Aguirre on 2014-07-01: empty commit to kick CI

Revision history for this message

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

FAILED: Continuous integration, rev:1684
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2045/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/780
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/786
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/779/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/566
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/566/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/564
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/564/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2363/console

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

review: Needs Fixing (continuous-integration)

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-07-02

1685. By Alberto Aguirre on 2014-07-02: merge lp:mir/devel

Revision history for this message

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

(1) A useful FIXME comment is missing from mc::BufferQueue::min_buffers() -
// FIXME: Sometimes required_buffers > nbuffers (LP: #1317403)

(2) An important test has been deleted:
1061 -TEST_F(BufferQueueTest, bypass_clients_get_more_than_two_buffers)
It needs to stay, and to pass(!). At least bypass needed 3+ buffers at some peak times last I checked. Otherwise it will intermittently deadlock. Although I forget the full reasoning right now.

(3) You have fudged slow_client_framerate_matches_compositor, which is kind of cheating as mentioned in https://code.launchpad.net/~vanvugt/mir/double/+merge/221431/comments/530360
Not sure how safe the fudge is or if it risks regression. I would be more sure if I knew how to reintroduce the bug it protects against :)

review: Needs Fixing

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-07-02:

Still haven't entirely got my head around this - so changing to Abstain.

~~~~

1211 + auto const compositor_holding_time = std::chrono::milliseconds{16};

Any reason for writing in this verbose style? C.f.

std::chrono::milliseconds const compositor_holding_time{16};

review: Abstain

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-07-02:

> (1) A useful FIXME comment is missing from mc::BufferQueue::min_buffers() -
> // FIXME: Sometimes required_buffers > nbuffers (LP: #1317403)
>
> (2) An important test has been deleted:
> 1061 -TEST_F(BufferQueueTest, bypass_clients_get_more_than_two_buffers)
> It needs to stay, and to pass(!). At least bypass needed 3+ buffers at some
> peak times last I checked. Otherwise it will intermittently deadlock. Although
> I forget the full reasoning right now.
>
> (3) You have fudged slow_client_framerate_matches_compositor, which is kind of
> cheating as mentioned in
> https://code.launchpad.net/~vanvugt/mir/double/+merge/221431/comments/530360
> Not sure how safe the fudge is or if it risks regression. I would be more sure
> if I knew how to reintroduce the bug it protects against :)

(1) is not applicable anymore

(2) It's no longer an important test. Bypass does not need more than 2 buffers. If you find a deadlock scenario, let's fix the deadlock not increase the number of buffers.

(3) No cheating...you have to account for the thread/context switch time since there's no pipelining - which under valgrind running on armhf seems to be significant.

Revision history for this message

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

(1) Bug 1317403 was really a consequence of bypass needing triple buffers, so read on... Also if you find a bug invalid, the first place it should be mentioned is in the bug.

(2) That's a bit surprising. For most of the year or so since I implemented bypass, 3 buffers was the minimum. Less than that and the client/server would be waiting for each other indefinitely (hence visibly freeze). And a month ago I could still reproduce some deadlocks, sometimes. But I can't today. It looks like possibly:
(a) Alan's newish async client buffer swapping solved the primary problem, as there is now significant time during swap_buffers where the client owns zero buffers. So that leaves the minimum 2 available for the server to bypass with, and then release one back to the client. Also...
(b) Bug 1319765 was unsolved around the time I was seeing deadlocks most recently so that could have been all of the remaining deadlocks, now solved.

Although I'm not yet 100% confident, it does look like bypass can now work with only two buffers. A win for performance!

(3) It's cheating because I do remember I could only reproduce the original performance bug at 16ms. It was right on the edge but I guess too brittle to rely on such timing. But I can't see either bug 1241371 or bug 1241369 occurring any more so I guess that is the important thing.

(4) BufferAllocBehavior: I think is poorly named/unnecessary. A more straightforward design is to simply provide "min_buffers" and "max_buffers" per my original branch. No enum required.

(5) BufferAllocationPolicy: There is precedent but I maintain that anything-"Policy" is a poor choice of class design. Policy is not as concrete as other concepts to name classes after, which would be easier to understand. "Policy" is an indication that the class design needs rearrangement. Both BufferAllocBehavior and BufferAllocationPolicy can be replaced by three ints; min_buffers, max_buffers and shrink_threshold. One of those ints already exist so it's obvious that replacing an enum and class with two ints is a more straightforward design.

This is just a high level review. I haven't started to read the code really but it mostly looks like my double branch.

(1) Bug 1317403 was really a consequence of bypass needing triple buffers, so read on... Also if you find a bug invalid, the first place it should be mentioned is in the bug.

(2) That's a bit surprising. For most of the year or so since I implemented bypass, 3 buffers was the minimum. Less than that and the client/server would be waiting for each other indefinitely (hence visibly freeze). And a month ago I could still reproduce some deadlocks, sometimes. But I can't today. It looks like possibly:
  (a) Alan's newish async client buffer swapping solved the primary problem, as there is now significant time during swap_buffers where the client owns zero buffers. So that leaves the minimum 2 available for the server to bypass with, and then release one back to the client. Also...
  (b) Bug 1319765 was unsolved around the time I was seeing deadlocks most recently so that could have been all of the remaining deadlocks, now solved.

Although I'm not yet 100% confident, it does look like bypass can now work with only two buffers. A win for performance!

(4) BufferAllocBehavior: I think is poorly named/unnecessary. A more straightforward design is to simply provide "min_buffers" and "max_buffers" per my original branch. No enum required.

This is just a high level review. I haven't started to read the code really but it mostly looks like my double branch.

review: Needs Fixing

Revision history for this message

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

(6) Why delete this regression test?
973 -/* Regression test for LP#1270964 */
974 -TEST_F(BufferQueueTest, compositor_client_interleaved)
I had it passing in the double branch.

Revision history for this message

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

(7) It might be slightly unnecessary but I think nice to have the extra coverage starting with 2 buffers:
1498 -/* FIXME (enabling this optimization breaks timing tests) */
1499 -TEST_F(BufferQueueTest, DISABLED_synchronous_clients_only_get_two_real_buffers)
1500 +TEST_F(BufferQueueTest, synchronous_clients_only_get_two_real_buffers)
1501 {
1502 - for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
1503 + for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)

That test case is the most important one of this whole branch. And the best indication that it's working is if you can enable the test case without modifying it at all.

Revision history for this message

Gerry Boland (gerboland) wrote on 2014-07-03:

Just a probably stupid question from me:

> (2) That's a bit surprising. For most of the year or so since I implemented
> bypass, 3 buffers was the minimum. Less than that and the client/server would
> be waiting for each other indefinitely (hence visibly freeze). And a month ago
> I could still reproduce some deadlocks, sometimes. But I can't today. It looks
> like possibly:
> (a) Alan's newish async client buffer swapping solved the primary problem,
> as there is now significant time during swap_buffers where the client owns
> zero buffers. So that leaves the minimum 2 available for the server to bypass
> with, and then release one back to the client. Also...
> (b) Bug 1319765 was unsolved around the time I was seeing deadlocks most
> recently so that could have been all of the remaining deadlocks, now solved.
>
> Although I'm not yet 100% confident, it does look like bypass can now work
> with only two buffers. A win for performance!

If the client is being blocked from having any buffer to render into, doesn't that reduce the amount of time it has a buffer to render into? So for a client which takes just under 16ms to render each frame, and the server holds onto 2 buffers for >1ms, then the client will keep missing vsync?

Revision history for this message

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

Some suggestions from a first pass (a weak Needs Fixing).

61 + for(int i = 0; i < nalloc; i++)

Space between for and '('.

75 + if (alloc_policy.alloc_behavior == BufferAllocBehavior::on_demand &&
76 + allocated_buffers < min_buffers())

If alloc_behavior != on_demand isn't "allocated_buffers < min_buffers()" always false (we have pre-allocated the maximum number of buffers). So, we could just write "if (allocated_buffers < min_buffers())"

85 + bool use_current_buffer = force_use_current_buffer;

How about replacing the current logic (which has become a bit complex) with something more straightforward (IMO) like:

bool current_buffer_is_used_but_not_by(void const* user_id)
{
return !current_buffer_users.empty() && !is_a_current_buffer_user(user_id);
}

bool have_new_compositor_buffer()
{
return !ready_to_composite_queue.empty();
}

compositor_acquire(void const* user_id)
{
    bool const use_current_buffer =
        force_use_current_buffer ||
        (current_buffer_is_used_but_not_by(user_id) && !have_new_compositor_buffer());

    if (!use_current_buffer)
    {
       ...
    }
    else if (!is_a_current_buffer_user(user_id))
    {
       current_buffer_users.push_back(user_id);
    }
}

150 + int const alloced_buffers = buffers.size();

"allocated_buffers", no need to save a few characters (also for consistency with other "allocated_buffers" in the code)

review: Needs Fixing

Revision history for this message

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

> 85 + bool use_current_buffer = force_use_current_buffer;
>
> How about replacing the current logic (which has become a bit complex) with
> something more straightforward (IMO) like:

Actually, what I proposed above probably doesn't work, but the point is that we should try to simplify the paths in that function since it has become too complex.

Revision history for this message

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

> > 85 + bool use_current_buffer = force_use_current_buffer;
> >
> > How about replacing the current logic (which has become a bit complex) with
> > something more straightforward (IMO) like:
>
> Actually, what I proposed above probably doesn't work, but the point is that
> we should try to simplify the paths in that function since it has become too
> complex.

But this version seems to work (at least it passes all the tests):

bool const use_current_buffer =
    force_use_current_buffer ||
    current_buffer_is_used_but_not_by(user_id) ||
    !have_new_compositor_buffer();

Revision history for this message

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

> Just a probably stupid question from me:
> ...
> If the client is being blocked from having any buffer to render into, doesn't
> that reduce the amount of time it has a buffer to render into? So for a client
> which takes just under 16ms to render each frame, and the server holds onto 2
> buffers for >1ms, then the client will keep missing vsync?

That's a good point actually. Having 2 buffers increases the risk of missing vsync compared to 3, however the increase in risk is still negligible I think. Mir is now much better at meeting frame deadlines than ever and it's regularly improving still. I don't think the slight delay of 2 vs 3 buffers is likely to ever cause skipped frames on the systems Mir supports. What's more important (and certain) is that the latency is now halved, and that's always going to be true.

It's also worth noting that double buffering probably only impacts the client render time window for a fullscreen bypassed surface (where the server requires ownership of two buffers briefly during the flip). For general compositing, the server only ever holds one brief (but mostly zero) surface buffers and the client is never delayed in getting one.

Revision history for this message

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

Correction: Since SwitchingBundle became BufferQueue, the server always holds a minimum one surface buffer for compositing. Never zero.

Revision history for this message

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

(8) I just added some printfs and the code is actually still triple buffering on bypass (mostly but not always). If you're not seeing that behaviour then try:
mir_demo_client_egltriangle -f
and Ctrl+Alt+arrows to rotate a few times (force compositing) before going back to bypass (Ctrl+Alt+Up).

So either:
  (a) that old behaviour is still correct and you need to reinstate tests for it like bypass_clients_get_more_than_two_buffers; or
  (b) it shouldn't be triple buffering bypass clients and it's a bug; or
  (c) it's just bad timing in the wrong phase and sometimes really does need 3 buffers to keep up.

It looks like the bypass detection code is still present and that's what's pushing it up to 3:
increase_demand = multiple_compositor_buffers

Revision history for this message

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

(9) This test needs to be retained, and ported per the double branch...
1102 -TEST_F(BufferQueueTest, framedropping_clients_get_all_buffers)

Because there's no value in allowing framedropping clients to have more than 2 buffers while the (slower) compositor uses a third. One of the two client buffers is newest (will eventually be composited) and the other can be safely discarded, overwritten by the client. This was working well in May but it seems some recent changes around framedropping policy has broken it. Certainly needs fixing because the new behaviour only wastes resources without any possible gain in performance.

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-07-07:

BufferAllocationPolicy seems simple enough that the two members could just be BufferQueue constructor parameters, and avoid having to have the struct defined.

240 + BufferAllocationPolicy(BufferAllocBehavior behavior, int treshold)
251 + int const shrink_treshold;
typo

273 + int allocated_buffers() const;
This is public and is only used in test code, maybe BufferQueueTest could monitor how many times the GraphicBufferAllocator allocates buffers. (a bit more test rigging, but saves on introducing public functions of the class)

review: Needs Fixing

lp:~mir-team/mir/enable-dynamic-buffer-queue updated on 2014-07-08

1686. By Daniel van Vugt on 2014-07-08: Merge latest development-branch
1687. By Daniel van Vugt on 2014-07-08: Start simplifying; replaced Policy and Behaviour classes with a simple
integer range.
1688. By Daniel van Vugt on 2014-07-08: Shrink the diff (remove whitespace changes)

Revision history for this message

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

Work in progress... I'll carry on with this tomorrow. Or fix the original branch. Not sure.

Unmerged revisions

1688. By Daniel van Vugt on 2014-07-08: Shrink the diff (remove whitespace changes)
1687. By Daniel van Vugt on 2014-07-08: Start simplifying; replaced Policy and Behaviour classes with a simple
integer range.
1686. By Daniel van Vugt on 2014-07-08: Merge latest development-branch
1685. By Alberto Aguirre on 2014-07-02: merge lp:mir/devel
1684. By Alberto Aguirre on 2014-07-01: empty commit to kick CI
1683. By Alberto Aguirre on 2014-06-30: test slow_client_framerate_matches_compositor:
Give a bit more time diff to account for thread switching in arm platform
1682. By Alberto Aguirre on 2014-06-30: Increase test granularity
1681. By Alberto Aguirre on 2014-06-30: ASSERT upperbound in framerate matching tests.
1680. By Alberto Aguirre on 2014-06-27: merge lp:mir/devel
1679. By Alberto Aguirre on 2014-06-27: Fix failing test

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:

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/test/mir_test/signal.h'
 --- include/test/mir_test/signal.h	2014-05-22 20:48:20 +0000
 +++ include/test/mir_test/signal.h	2014-07-08 10:11:16 +0000
@@ -37,6 +37,7 @@
      void raise();
      bool raised();
++    void reset();
      void wait();
      template<typename rep, typename period>
 === modified file 'src/server/compositor/buffer_queue.cpp'
 --- src/server/compositor/buffer_queue.cpp	2014-06-12 15:35:08 +0000
 +++ src/server/compositor/buffer_queue.cpp	2014-07-08 10:11:16 +0000
@@ -37,12 +37,14 @@
      return buffer;
+ }
--bool remove(mg::Buffer const* item, std::vector<mg::Buffer*>& list)
++template<typename Type>
++bool remove(mg::Buffer const* item, std::vector<Type>& list)
+ {
      int const size = list.size();
      for (int i = 0; i < size; ++i)
+     {
--        if (list[i] == item)
++        //The &* is so that it works with both shared_ptrs and raw pointers
++        if (&*list[i] == item)
+         {
              list.erase(list.begin() + i);
              return true;
@@ -92,26 +94,28 @@
+ }
  mc::BufferQueue::BufferQueue(
--    int nbuffers,
++    int static_min_buffers, int max_buffers,
      std::shared_ptr<graphics::GraphicBufferAllocator> const& gralloc,
      graphics::BufferProperties const& props,
--    mc::FrameDroppingPolicyFactory const& policy_provider)
--    : nbuffers{nbuffers},
++    mc::FrameDroppingPolicyFactory const& policy_provider,
++    int shrink_threshold)
++    : static_min_buffers{static_min_buffers},
++      max_buffers{max_buffers},
++      shrink_threshold{shrink_threshold},
++      queue_size_excess{0},
        frame_dropping_enabled{false},
++      force_use_current_buffer{false},
++      multiple_compositor_buffers{false},
        the_properties{props},
        gralloc{gralloc}
+ {
--    if (nbuffers < 1)
++    if (static_min_buffers < 0 || max_buffers < 1 || static_min_buffers > max_buffers)
+     {
          BOOST_THROW_EXCEPTION(
              std::logic_error("invalid number of buffers for BufferQueue"));
+     }
--    /* 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)
--     */
--    for(int i = 0; i < std::min(nbuffers, 2); i++)
++    for (int i = 0; i < static_min_buffers; i++)
+     {
          buffers.push_back(gralloc->alloc_buffer(the_properties));
+     }
@@ -127,7 +131,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]
@@ -169,12 +173,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 < dynamic_min_buffers())
+     {
          auto const& buffer = gralloc->alloc_buffer(the_properties);
          buffers.push_back(buffer);
@@ -221,7 +221,7 @@
+ {
      std::unique_lock<decltype(guard)> lock(guard);
--    bool use_current_buffer = false;
++    bool use_current_buffer = force_use_current_buffer;
      if (!current_buffer_users.empty() && !is_a_current_buffer_user(user_id))
+     {
          use_current_buffer = true;
@@ -245,9 +245,17 @@
          current_buffer_users.push_back(user_id);
          current_compositor_buffer = pop(ready_to_composite_queue);
+     }
++    else if (current_buffer_users.empty())
++    {   // current_buffer_users and ready_to_composite_queue both empty
++        current_buffer_users.push_back(user_id);
++    }
++    force_use_current_buffer = false;
      buffers_sent_to_compositor.push_back(current_compositor_buffer);
++    multiple_compositor_buffers =
++        buffers_sent_to_compositor.size() > current_buffer_users.size();
++
      std::shared_ptr<mg::Buffer> const acquired_buffer =
          buffer_for(current_compositor_buffer, buffers);
@@ -271,7 +279,7 @@
      if (contains(buffer.get(), buffers_sent_to_compositor))
          return;
--    if (nbuffers <= 1)
++    if (max_buffers <= 1)
          return;
      /*
@@ -289,8 +297,7 @@
          // Ensure current_compositor_buffer gets reused by the next
          // compositor_acquire:
          current_buffer_users.clear();
--        void const* const impossible_user_id = this;
--        current_buffer_users.push_back(impossible_user_id);
++        force_use_current_buffer = true;
+     }
      if (current_compositor_buffer != buffer.get())
@@ -334,6 +341,11 @@
      return frame_dropping_enabled;
+ }
++int mc::BufferQueue::allocated_buffers() const
++{
++    return buffers.size();
++}
++
  void mc::BufferQueue::force_requests_to_complete()
+ {
      std::unique_lock<std::mutex> lock(guard);
@@ -369,14 +381,7 @@
  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(
@@ -396,7 +401,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;
+     }
@@ -432,13 +437,61 @@
      mg::Buffer* buffer,
      std::unique_lock<std::mutex> lock)
+ {
--    if (!pending_client_notifications.empty())
++    int const alloced_buffers = buffers.size();
++
++    // To avoid reallocating buffers too often (which may be slow), only drop
++    // a buffer after it's continually been in excess for a long time...
++    if (alloced_buffers > dynamic_min_buffers())
++        ++queue_size_excess;
++    else
++        queue_size_excess = 0;
++
++    // If too many frames have had excess buffers then start dropping them now
++    if (queue_size_excess > shrink_threshold &&
++        alloced_buffers > static_min_buffers)
++    {
++        remove(buffer, buffers);
++        queue_size_excess = 0;
++    }
++    else if (!pending_client_notifications.empty())
+     {
          framedrop_policy->swap_unblocked();
          give_buffer_to_client(buffer, std::move(lock));
+     }
      else
++    {
          free_buffers.push_back(buffer);
++    }
++}
++
++/**
++ * Measure the actual number of buffers we presently need concurrently
++ * to avoid starving any compositors of fresh frames or starving clients of
++ * any buffers at all.
++ */
++int mc::BufferQueue::dynamic_min_buffers() const
++{
++    if (max_buffers == 1)
++        return 1;
++
++    if (static_min_buffers == max_buffers)
++        return static_min_buffers;
++
++    /* When framedropping is enabled and there are multiple compositors
++     * they can be out of phase enough that the compositor buffer is
++     * practically always owned by one of the compositors, which prevents it
++     * from being updated to the latest client content during frame-dropping
++     */
++    bool const increase_demand = multiple_compositor_buffers ||
++                                 current_buffer_users.size() > 1;
++    int const frame_drop_demand = increase_demand ? 1 : 0;
++    int const client_requests = buffers_owned_by_client.size() +
++                                pending_client_notifications.size();
++    int const client_demand = std::max(1, client_requests);
++    int const compositor_demand = 1;
++    int const required_buffers = compositor_demand + client_demand + frame_drop_demand;
++
++    return std::min(max_buffers, required_buffers);
+ }
  void mc::BufferQueue::drop_frame(std::unique_lock<std::mutex> lock)
@@ -450,8 +503,7 @@
      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);
++       force_use_current_buffer = true;
         std::swap(buffer_to_give, current_compositor_buffer);
+     }
      give_buffer_to_client(buffer_to_give, std::move(lock));
 === modified file 'src/server/compositor/buffer_queue.h'
 --- src/server/compositor/buffer_queue.h	2014-06-12 15:35:08 +0000
 +++ src/server/compositor/buffer_queue.h	2014-07-08 10:11:16 +0000
@@ -42,10 +42,12 @@
  public:
      typedef std::function<void(graphics::Buffer* buffer)> Callback;
--    BufferQueue(int nbuffers,
++    BufferQueue(int static_min_buffers,
++                int max_buffers,
                  std::shared_ptr<graphics::GraphicBufferAllocator> const& alloc,
                  graphics::BufferProperties const& props,
--                FrameDroppingPolicyFactory const& policy_provider);
++                FrameDroppingPolicyFactory const& policy_provider,
++                int shrink_threshold = 300);
      void client_acquire(Callback complete) override;
      void client_release(graphics::Buffer* buffer) override;
@@ -61,6 +63,7 @@
      int buffers_ready_for_compositor() const override;
      int buffers_free_for_client() const override;
      bool framedropping_allowed() const;
++    int allocated_buffers() const;
  private:
      void give_buffer_to_client(graphics::Buffer* buffer,
@@ -69,6 +72,7 @@
      void release(graphics::Buffer* buffer,
          std::unique_lock<std::mutex> lock);
      void drop_frame(std::unique_lock<std::mutex> lock);
++    int dynamic_min_buffers() const;
      mutable std::mutex guard;
@@ -84,8 +88,13 @@
      std::deque<Callback> pending_client_notifications;
--    int nbuffers;
++    int const static_min_buffers;
++    int const max_buffers;
++    int const shrink_threshold;
++    int queue_size_excess;
      bool frame_dropping_enabled;
++    bool force_use_current_buffer;
++    bool multiple_compositor_buffers;
      std::unique_ptr<FrameDroppingPolicy> framedrop_policy;
      graphics::BufferProperties the_properties;
 === modified file 'src/server/compositor/buffer_stream_factory.cpp'
 --- src/server/compositor/buffer_stream_factory.cpp	2014-06-09 17:16:32 +0000
 +++ src/server/compositor/buffer_stream_factory.cpp	2014-07-08 10:11:16 +0000
@@ -47,7 +47,6 @@
  std::shared_ptr<mc::BufferStream> mc::BufferStreamFactory::create_buffer_stream(
      mg::BufferProperties const& buffer_properties)
+ {
--    // Note: Framedropping and bypass both require a minimum 3 buffers
--    auto switching_bundle = std::make_shared<mc::BufferQueue>(3, gralloc, buffer_properties, *policy_factory);
--    return std::make_shared<mc::BufferStreamSurfaces>(switching_bundle);
++    auto buffer_q = std::make_shared<mc::BufferQueue>(2, 3, gralloc, buffer_properties, *policy_factory);
++    return std::make_shared<mc::BufferStreamSurfaces>(buffer_q);
+ }
 === modified file 'tests/integration-tests/compositor/test_buffer_stream.cpp'
 --- tests/integration-tests/compositor/test_buffer_stream.cpp	2014-06-09 17:16:32 +0000
 +++ tests/integration-tests/compositor/test_buffer_stream.cpp	2014-07-08 10:11:16 +0000
@@ -49,25 +49,33 @@
+ {
      using mc::BufferStreamSurfaces::BufferStreamSurfaces;
--    // Convenient function to allow tests to be written in linear style
++    void acquire_client_buffer_async(mg::Buffer*& buffer,
++        std::shared_ptr<mt::Signal> const& signal)
++    {
++        acquire_client_buffer(
++            [signal, &buffer](mg::Buffer* new_buffer)
++            {
++               buffer = new_buffer;
++               signal->raise();
++            });
++    }
++
++    // Convenient functions to allow tests to be written in linear style
++    mg::Buffer* acquire_client_buffer_blocking()
++    {
++        mg::Buffer* buffer = nullptr;
++        auto signal = std::make_shared<mt::Signal>();
++        acquire_client_buffer_async(buffer, signal);
++        signal->wait();
++        return buffer;
++    }
++
      void swap_client_buffers_blocking(mg::Buffer*& buffer)
+     {
--        swap_client_buffers_cancellable(buffer, std::make_shared<mt::Signal>());
--    }
--
--    void swap_client_buffers_cancellable(mg::Buffer*& buffer, std::shared_ptr<mt::Signal> const& signal)
--    {
          if (buffer)
              release_client_buffer(buffer);
--        acquire_client_buffer(
--            [signal, &buffer](mg::Buffer* new_buffer)
--             {
--                buffer = new_buffer;
--                signal->raise();
--             });
--
--        signal->wait();
++        buffer = acquire_client_buffer_blocking();
+     }
  };
@@ -95,8 +103,7 @@
                                          mg::BufferUsage::hardware};
          mc::TimeoutFrameDroppingPolicyFactory policy_factory{timer,
                                                               frame_drop_timeout};
--
--        buffer_queue = std::make_shared<mc::BufferQueue>(nbuffers,
++        buffer_queue = std::make_shared<mc::BufferQueue>(2, nbuffers,
                                                           allocator,
                                                           properties,
                                                           policy_factory);
@@ -116,12 +123,9 @@
  TEST_F(BufferStreamTest, gives_same_back_buffer_until_more_available)
+ {
--    ASSERT_THAT(buffers_free_for_client(), Ge(2)); // else we will hang
--
--    mg::Buffer* client1{nullptr};
--    buffer_stream.swap_client_buffers_blocking(client1);
++    mg::Buffer* client1 = buffer_stream.acquire_client_buffer_blocking();
      auto client1_id = client1->id();
--    buffer_stream.swap_client_buffers_blocking(client1);
++    buffer_stream.release_client_buffer(client1);
      auto comp1 = buffer_stream.lock_compositor_buffer(nullptr);
      auto comp2 = buffer_stream.lock_compositor_buffer(nullptr);
@@ -131,10 +135,14 @@
      comp1.reset();
--    buffer_stream.swap_client_buffers_blocking(client1);
++    mg::Buffer* client2 = buffer_stream.acquire_client_buffer_blocking();
++    auto client2_id = client2->id();
++    buffer_stream.release_client_buffer(client2);
++
      auto comp3 = buffer_stream.lock_compositor_buffer(nullptr);
      EXPECT_NE(client1_id, comp3->id());
++    EXPECT_EQ(client2_id, comp3->id());
      comp2.reset();
      auto comp3_id = comp3->id();
@@ -165,16 +173,10 @@
  TEST_F(BufferStreamTest, gives_different_back_buffer_asap)
+ {
--    ASSERT_THAT(buffers_free_for_client(), Ge(2)); // else we will hang
--
--    mg::Buffer* client_buffer{nullptr};
--    buffer_stream.swap_client_buffers_blocking(client_buffer);
--
--    ASSERT_THAT(nbuffers, Gt(1));
--    buffer_stream.swap_client_buffers_blocking(client_buffer);
++    buffer_stream.release_client_buffer(buffer_stream.acquire_client_buffer_blocking());
      auto comp1 = buffer_stream.lock_compositor_buffer(nullptr);
--    buffer_stream.swap_client_buffers_blocking(client_buffer);
++    buffer_stream.release_client_buffer(buffer_stream.acquire_client_buffer_blocking());
      auto comp2 = buffer_stream.lock_compositor_buffer(nullptr);
      EXPECT_NE(comp1->id(), comp2->id());
@@ -185,13 +187,13 @@
  TEST_F(BufferStreamTest, resize_affects_client_buffers_immediately)
+ {
--    ASSERT_THAT(buffers_free_for_client(), Ge(2)); // else we will hang
--
      auto old_size = buffer_stream.stream_size();
--    mg::Buffer* client{nullptr};
--    buffer_stream.swap_client_buffers_blocking(client);
++    mg::Buffer* client = buffer_stream.acquire_client_buffer_blocking();
      EXPECT_EQ(old_size, client->size());
++    buffer_stream.release_client_buffer(client);
++
++    buffer_stream.lock_compositor_buffer(this);
      geom::Size const new_size
+     {
@@ -201,28 +203,28 @@
      buffer_stream.resize(new_size);
      EXPECT_EQ(new_size, buffer_stream.stream_size());
--    buffer_stream.swap_client_buffers_blocking(client);
++    client = buffer_stream.acquire_client_buffer_blocking();
      EXPECT_EQ(new_size, client->size());
++    buffer_stream.release_client_buffer(client);
++
++    buffer_stream.lock_compositor_buffer(this);
      buffer_stream.resize(old_size);
      EXPECT_EQ(old_size, buffer_stream.stream_size());
--    /* Release a buffer so client can acquire another */
--    auto comp = buffer_stream.lock_compositor_buffer(nullptr);
--    comp.reset();
++    buffer_stream.lock_compositor_buffer(this);
--    buffer_stream.swap_client_buffers_blocking(client);
++    client = buffer_stream.acquire_client_buffer_blocking();
      EXPECT_EQ(old_size, client->size());
++    buffer_stream.release_client_buffer(client);
+ }
  TEST_F(BufferStreamTest, compositor_gets_resized_buffers)
+ {
--    ASSERT_THAT(buffers_free_for_client(), Ge(2)); // else we will hang
--
      auto old_size = buffer_stream.stream_size();
--    mg::Buffer* client{nullptr};
--    buffer_stream.swap_client_buffers_blocking(client);
++    mg::Buffer* client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
      geom::Size const new_size
+     {
@@ -232,47 +234,54 @@
      buffer_stream.resize(new_size);
      EXPECT_EQ(new_size, buffer_stream.stream_size());
--    buffer_stream.swap_client_buffers_blocking(client);
--
      auto comp1 = buffer_stream.lock_compositor_buffer(nullptr);
++
++    client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
++
      EXPECT_EQ(old_size, comp1->size());
      comp1.reset();
--    buffer_stream.swap_client_buffers_blocking(client);
--
      auto comp2 = buffer_stream.lock_compositor_buffer(nullptr);
++
++    client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
++
      EXPECT_EQ(new_size, comp2->size());
      comp2.reset();
--    buffer_stream.swap_client_buffers_blocking(client);
--
      auto comp3 = buffer_stream.lock_compositor_buffer(nullptr);
++
++    client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
++
      EXPECT_EQ(new_size, comp3->size());
      comp3.reset();
      buffer_stream.resize(old_size);
      EXPECT_EQ(old_size, buffer_stream.stream_size());
++    auto comp4 = buffer_stream.lock_compositor_buffer(nullptr);
++
      // No client frames "drawn" since resize(old_size), so compositor gets new_size
--    buffer_stream.swap_client_buffers_blocking(client);
--    auto comp4 = buffer_stream.lock_compositor_buffer(nullptr);
++    client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
      EXPECT_EQ(new_size, comp4->size());
      comp4.reset();
++    auto comp5 = buffer_stream.lock_compositor_buffer(nullptr);
++
      // Generate a new frame, which should be back to old_size now
--    buffer_stream.swap_client_buffers_blocking(client);
--    auto comp5 = buffer_stream.lock_compositor_buffer(nullptr);
++    client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
      EXPECT_EQ(old_size, comp5->size());
      comp5.reset();
+ }
  TEST_F(BufferStreamTest, can_get_partly_released_back_buffer)
+ {
--    ASSERT_THAT(buffers_free_for_client(), Ge(2)); // else we will hang
--
--    mg::Buffer* client{nullptr};
--    buffer_stream.swap_client_buffers_blocking(client);
--    buffer_stream.swap_client_buffers_blocking(client);
++    mg::Buffer* client = buffer_stream.acquire_client_buffer_blocking();
++    buffer_stream.release_client_buffer(client);
      int a, b, c;
      auto comp1 = buffer_stream.lock_compositor_buffer(&a);
@@ -375,12 +384,17 @@
+ {
      using namespace testing;
--    mg::Buffer* placeholder{nullptr};
++    mg::Buffer* buffer{nullptr};
++    auto signal = std::make_shared<mt::Signal>();
--    // Grab all the buffers...
--    // TODO: the magic “nbuffers - 1” number should be removed
--    for (int i = 0; i < nbuffers - 1; ++i)
--        buffer_stream.swap_client_buffers_blocking(placeholder);
++    // acquire all possible buffers until blocked
++    do
++    {
++        signal->reset();
++        buffer_stream.acquire_client_buffer_async(buffer, signal);
++        if (signal->raised())
++            buffer_stream.release_client_buffer(buffer);
++    } while (signal->raised());
      auto swap_completed = std::make_shared<mt::Signal>();
      buffer_stream.acquire_client_buffer([swap_completed](mg::Buffer*) {swap_completed->raise();});
 === modified file 'tests/integration-tests/compositor/test_swapping_swappers.cpp'
 --- tests/integration-tests/compositor/test_swapping_swappers.cpp	2014-06-09 17:16:32 +0000
 +++ tests/integration-tests/compositor/test_swapping_swappers.cpp	2014-07-08 10:11:16 +0000
@@ -49,7 +49,7 @@
                                            mir_pixel_format_abgr_8888,
                                            mg::BufferUsage::hardware};
          mtd::StubFrameDroppingPolicyFactory policy_factory;
--        switching_bundle = std::make_shared<mc::BufferQueue>(3, allocator, properties, policy_factory);
++        switching_bundle = std::make_shared<mc::BufferQueue>(2, 3, allocator, properties, policy_factory);
+     }
      std::shared_ptr<mc::BufferQueue> switching_bundle;
 === modified file 'tests/integration-tests/graphics/android/test_buffer_integration.cpp'
 --- tests/integration-tests/graphics/android/test_buffer_integration.cpp	2014-06-09 17:16:32 +0000
 +++ tests/integration-tests/graphics/android/test_buffer_integration.cpp	2014-07-08 10:11:16 +0000
@@ -57,6 +57,7 @@
      mg::BufferProperties buffer_properties;
      std::shared_ptr<mtd::GraphicsRegionFactory> graphics_region_factory;
      mir::test::doubles::StubFrameDroppingPolicyFactory policy_factory;
++    mc::BufferAllocationPolicy alloc_policy;
  };
  auto client_acquire_blocking(mc::BufferQueue& switching_bundle)
@@ -119,7 +120,7 @@
      auto allocator = std::make_shared<mga::AndroidGraphicBufferAllocator>(null_buffer_initializer);
--    mc::BufferQueue swapper(2, allocator, buffer_properties, policy_factory);
++    mc::BufferQueue swapper(2, allocator, buffer_properties, policy_factory, alloc_policy);
      auto returned_buffer = client_acquire_blocking(swapper);
 === modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
 --- tests/integration-tests/test_surface_first_frame_sync.cpp	2014-06-17 11:21:31 +0000
 +++ tests/integration-tests/test_surface_first_frame_sync.cpp	2014-07-08 10:11:16 +0000
@@ -113,7 +113,7 @@
+     {
          for (auto const& r : renderables)
+         {
--            (void)r;
++            r->buffer(); // We need to consume a buffer to unblock client tests
              while (write(render_operations_fd, "a", 1) != 1) continue;
+         }
+     }
 === modified file 'tests/mir_test/signal.cpp'
 --- tests/mir_test/signal.cpp	2014-05-22 20:48:20 +0000
 +++ tests/mir_test/signal.cpp	2014-07-08 10:11:16 +0000
@@ -39,6 +39,12 @@
      return signalled;
+ }
++void mt::Signal::reset()
++{
++    std::lock_guard<decltype(mutex)> lock(mutex);
++    signalled = false;
++}
++
  void mt::Signal::wait()
+ {
      std::unique_lock<decltype(mutex)> lock(mutex);
 === modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
 --- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-07-07 09:28:00 +0000
 +++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-07-08 10:11:16 +0000
@@ -48,6 +48,25 @@
  namespace
+ {
++class TestBufferQueue : public mc::BufferQueue
++
++{
++public:
++    static int const default_shrink_threshold = 5;
++    TestBufferQueue(int nbuffers)
++    : TestBufferQueue(nbuffers, mtd::StubFrameDroppingPolicyFactory{})
++    {}
++
++    TestBufferQueue(int nbuffers, mc::FrameDroppingPolicyFactory const& policy_provider)
++        : mc::BufferQueue((nbuffers >= 2) ? 2 : 1, nbuffers,
++          std::make_shared<mtd::StubBufferAllocator>(),
++          { geom::Size{3, 4}, mir_pixel_format_abgr_8888, mg::BufferUsage::hardware },
++          policy_provider,
++          default_shrink_threshold)
++    {}
++
++};
++
  class BufferQueueTest : public ::testing::Test
+ {
  public:
@@ -55,21 +74,8 @@
          : max_nbuffers_to_test{5}
      {};
--    void SetUp()
--    {
--        allocator = std::make_shared<mtd::StubBufferAllocator>();
--        basic_properties =
--        {
--            geom::Size{3, 4},
--            mir_pixel_format_abgr_8888,
--            mg::BufferUsage::hardware
--        };
--    }
  protected:
--    int max_nbuffers_to_test;
--    std::shared_ptr<mtd::StubBufferAllocator> allocator;
--    mg::BufferProperties basic_properties;
--    mtd::StubFrameDroppingPolicyFactory policy_factory;
++    int const max_nbuffers_to_test;
  };
  class AcquireWaitHandle
@@ -196,10 +202,8 @@
  TEST_F(BufferQueueTest, buffer_queue_of_one_is_supported)
+ {
--    std::unique_ptr<mc::BufferQueue> q;
--    ASSERT_NO_THROW(q = std::move(
--        std::unique_ptr<mc::BufferQueue>(
--            new mc::BufferQueue(1, allocator, basic_properties, policy_factory))));
++    std::unique_ptr<TestBufferQueue> q;
++    ASSERT_NO_THROW(q = std::move(std::unique_ptr<TestBufferQueue>(new TestBufferQueue(1))));
      ASSERT_THAT(q, Ne(nullptr));
      auto handle = client_acquire_async(*q);
@@ -234,7 +238,7 @@
  TEST_F(BufferQueueTest, buffer_queue_of_one_supports_resizing)
+ {
--    mc::BufferQueue q(1, allocator, basic_properties, policy_factory);
++    TestBufferQueue q(1);
      const geom::Size expect_size{10, 20};
      q.resize(expect_size);
@@ -259,22 +263,22 @@
  TEST_F(BufferQueueTest, framedropping_is_disabled_by_default)
+ {
--    mc::BufferQueue bundle(2, allocator, basic_properties, policy_factory);
--    EXPECT_THAT(bundle.framedropping_allowed(), Eq(false));
++    TestBufferQueue q(2);
++    EXPECT_THAT(q.framedropping_allowed(), Eq(false));
+ }
  TEST_F(BufferQueueTest, throws_when_creating_with_invalid_num_buffers)
+ {
--    EXPECT_THROW(mc::BufferQueue a(0, allocator, basic_properties, policy_factory), std::logic_error);
--    EXPECT_THROW(mc::BufferQueue a(-1, allocator, basic_properties, policy_factory), std::logic_error);
--    EXPECT_THROW(mc::BufferQueue a(-10, allocator, basic_properties, policy_factory), std::logic_error);
++    EXPECT_THROW(TestBufferQueue a(0), std::logic_error);
++    EXPECT_THROW(TestBufferQueue a(-1), std::logic_error);
++    EXPECT_THROW(TestBufferQueue a(-10), std::logic_error);
+ }
  TEST_F(BufferQueueTest, client_can_acquire_and_release_buffer)
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle = client_acquire_async(q);
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
@@ -286,7 +290,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          int const max_ownable_buffers = q.buffers_free_for_client();
          ASSERT_THAT(max_ownable_buffers, Gt(0));
@@ -302,7 +306,7 @@
  TEST_F(BufferQueueTest, clients_can_have_multiple_pending_completions)
+ {
      int const nbuffers = 3;
--    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++    TestBufferQueue q(nbuffers);
      int const prefill = q.buffers_free_for_client();
      ASSERT_THAT(prefill, Gt(0));
@@ -317,7 +321,7 @@
      ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
      auto handle2 = client_acquire_async(q);
--    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
++    ASSERT_THAT(handle2->has_acquired_buffer(), Eq(false));
      for (int i = 0; i < nbuffers + 1; ++i)
          q.compositor_release(q.compositor_acquire(this));
@@ -334,7 +338,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          ASSERT_THAT(q.framedropping_allowed(), Eq(false));
          void const* main_compositor = reinterpret_cast<void const*>(0);
@@ -363,7 +367,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          q.allow_framedropping(true);
          for (int i = 0; i < 1000; i++)
@@ -375,30 +379,37 @@
+     }
+ }
--/* Regression test for LP: #1210042 */
--TEST_F(BufferQueueTest, clients_dont_recycle_startup_buffer)
++/* Regression test for LP: #1210042 LP#1270964*/
++TEST_F(BufferQueueTest, client_and_compositor_requests_interleaved)
+ {
--    mc::BufferQueue q(3, allocator, basic_properties, policy_factory);
--
--    auto handle = client_acquire_async(q);
--    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--    auto client_id = handle->id();
--    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);
--    EXPECT_THAT(client_id, Eq(comp_buffer->id()));
--    q.compositor_release(comp_buffer);
++    int const nbuffers = 3;
++    TestBufferQueue q(nbuffers);
++
++    std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++    for (int i = 0; i < nbuffers; i++)
++    {
++        handles.push_back(client_acquire_async(q));
++    }
++
++    for (int i = 0; i < nbuffers; i++)
++    {
++        auto& handle = handles[i];
++        ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++
++        auto client_id = handle->id();
++        handle->release_buffer();
++
++        auto comp_buffer = q.compositor_acquire(this);
++        EXPECT_THAT(client_id, Eq(comp_buffer->id()));
++        q.compositor_release(comp_buffer);
++    }
+ }
  TEST_F(BufferQueueTest, throws_on_out_of_order_client_release)
+ {
      for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle1 = client_acquire_async(q);
          ASSERT_THAT(handle1->has_acquired_buffer(), Eq(true));
@@ -416,36 +427,30 @@
  TEST_F(BufferQueueTest, async_client_cycles_through_all_buffers)
+ {
--    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
++    for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
--
--        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<uint32_t> ids_acquired;
--        int const max_ownable_buffers = nbuffers - 1;
--        for (int i = 0; i < max_ownable_buffers*2; ++i)
--        {
--            std::vector<mg::Buffer *> client_buffers;
--            for (int acquires = 0; acquires < max_ownable_buffers; ++acquires)
--            {
--                auto handle = client_acquire_async(q);
--                handle->wait_for(std::chrono::seconds(1));
--                ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--                ids_acquired.insert(handle->id().as_uint32_t());
--                client_buffers.push_back(handle->buffer());
--            }
--
--            for (auto const& buffer : client_buffers)
--            {
--                q.client_release(buffer);
--            }
--        }
--
--        EXPECT_THAT(ids_acquired.size(), Eq(nbuffers));
++        TestBufferQueue q(nbuffers);
++
++        std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++        for (int i = 0; i < nbuffers; i++)
++        {
++            handles.push_back(client_acquire_async(q));
++        }
++
++        std::unordered_set<mg::Buffer *> buffers_acquired;
++        for (int i = 0; i < nbuffers; i++)
++        {
++            auto& handle = handles[i];
++            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++
++            buffers_acquired.insert(handle->buffer());
++            handle->release_buffer();
++
++            auto comp_buffer = q.compositor_acquire(this);
++            q.compositor_release(comp_buffer);
++        }
++
++        EXPECT_THAT(buffers_acquired.size(), Eq(nbuffers));
+     }
+ }
@@ -453,7 +458,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle = client_acquire_async(q);
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
@@ -471,7 +476,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle = client_acquire_async(q);
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
@@ -493,7 +498,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          for (int i = 0; i < 10; ++i)
+         {
@@ -527,7 +532,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          for (int i = 0; i < 100; i++)
+         {
@@ -541,7 +546,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          q.allow_framedropping(false);
          std::atomic<bool> done(false);
@@ -579,7 +584,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          mg::BufferID client_id;
@@ -608,7 +613,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle = client_acquire_async(q);
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
@@ -620,35 +625,12 @@
+     }
+ }
--/* Regression test for LP#1270964 */
--TEST_F(BufferQueueTest, compositor_client_interleaved)
--{
--    mc::BufferQueue q(3, allocator, basic_properties, policy_factory);
--
--    auto handle = client_acquire_async(q);
--    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--
--    auto first_ready_buffer_id = handle->id();
--    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));
--
--    handle->release_buffer();
--    q.compositor_release(compositor_buffer);
--}
--
  TEST_F(BufferQueueTest, overlapping_compositors_get_different_frames)
+ {
      // This test simulates bypass behaviour
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          std::shared_ptr<mg::Buffer> compositor[2];
@@ -685,7 +667,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto comp_buffer = q.compositor_acquire(this);
          auto snapshot = q.snapshot_acquire();
@@ -699,7 +681,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          int const num_snapshots = 100;
          std::shared_ptr<mg::Buffer> buf[num_snapshots];
@@ -715,7 +697,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          auto handle = client_acquire_async(q);
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
@@ -739,7 +721,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          std::atomic<bool> done(false);
@@ -756,85 +738,23 @@
                                             std::ref(done));
          q.allow_framedropping(false);
--        mt::AutoJoinThread client1(client_thread, std::ref(q), 1000);
++        mt::AutoJoinThread client1(client_thread, std::ref(q), 100);
          client1.stop();
          q.allow_framedropping(true);
--        mt::AutoJoinThread client2(client_thread, std::ref(q), 1000);
++        mt::AutoJoinThread client2(client_thread, std::ref(q), 100);
          client2.stop();
--        mt::AutoJoinThread client3(switching_client_thread, std::ref(q), 1000);
++        mt::AutoJoinThread client3(switching_client_thread, std::ref(q), 100);
          client3.stop();
+     }
+ }
--TEST_F(BufferQueueTest, bypass_clients_get_more_than_two_buffers)
--{
--    for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
--    {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
--
--        std::shared_ptr<mg::Buffer> compositor[2];
--
--        auto handle = client_acquire_async(q);
--        ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--        handle->release_buffer();
--        compositor[0] = q.compositor_acquire(this);
--
--        handle = client_acquire_async(q);
--        ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--        handle->release_buffer();
--        compositor[1] = q.compositor_acquire(this);
--
--        for (int i = 0; i < 20; i++)
--        {
--            // Two compositors acquired, and they're always different...
--            ASSERT_THAT(compositor[0]->id(), Ne(compositor[1]->id()));
--
--            handle = client_acquire_async(q);
--            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--            ASSERT_THAT(compositor[0]->id(), Ne(handle->id()));
--            ASSERT_THAT(compositor[1]->id(), Ne(handle->id()));
--            handle->release_buffer();
--
--            // One of the compositors (the oldest one) gets a new buffer...
--            int oldest = i & 1;
--            q.compositor_release(compositor[oldest]);
--
--            compositor[oldest] = q.compositor_acquire(this);
--        }
--
--        q.compositor_release(compositor[0]);
--        q.compositor_release(compositor[1]);
--    }
--}
--
--TEST_F(BufferQueueTest, framedropping_clients_get_all_buffers)
--{
--    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
--    {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
--        q.allow_framedropping(true);
--
--        int const nframes = 100;
--        std::unordered_set<uint32_t> ids_acquired;
--        for (int i = 0; i < nframes; ++i)
--        {
--            auto handle = client_acquire_async(q);
--            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--            ids_acquired.insert(handle->id().as_uint32_t());
--            handle->release_buffer();
--        }
--
--        EXPECT_THAT(ids_acquired.size(), Eq(nbuffers));
--    }
--}
--
  TEST_F(BufferQueueTest, waiting_clients_unblock_on_shutdown)
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          q.allow_framedropping(false);
          int const max_ownable_buffers = q.buffers_free_for_client();
@@ -858,31 +778,26 @@
  TEST_F(BufferQueueTest, client_framerate_matches_compositor)
+ {
--    for (int nbuffers = 2; nbuffers <= 3; nbuffers++)
++    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; nbuffers++)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          unsigned long client_frames = 0;
          const unsigned long compose_frames = 20;
--
--        q.allow_framedropping(false);
++        auto const frame_time = std::chrono::milliseconds{1};
          std::atomic<bool> done(false);
--        mt::AutoJoinThread monitor1([&]
++        mt::AutoJoinThread compositor_thread([&]
+         {
--            for (unsigned long frame = 0; frame != compose_frames+3; frame++)
++            for (unsigned long frame = 0; frame != compose_frames + 1; frame++)
+             {
--                std::this_thread::sleep_for(std::chrono::milliseconds(10));
--
                  auto buf = q.compositor_acquire(this);
++                std::this_thread::sleep_for(frame_time);
                  q.compositor_release(buf);
                  if (frame == compose_frames)
+                 {
--                    // Tell the "client" to stop after compose_frames, but
--                    // don't stop rendering immediately to avoid blocking
--                    // if we rendered any twice
--                    done.store(true);
++                    done = true;
+                 }
+             }
          });
@@ -891,7 +806,7 @@
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
          handle->release_buffer();
--        while (!done.load())
++        while (!done)
+         {
              auto handle = client_acquire_async(q);
              handle->wait_for(std::chrono::seconds(1));
@@ -900,48 +815,43 @@
              client_frames++;
+         }
--        monitor1.stop();
++        compositor_thread.stop();
--        // Roughly compose_frames == client_frames within 50%
--        ASSERT_THAT(client_frames, Gt(compose_frames / 2));
--        ASSERT_THAT(client_frames, Lt(compose_frames * 3 / 2));
++        // Roughly compose_frames == client_frames within 90%
++        ASSERT_THAT(client_frames, Ge(compose_frames * 0.90f));
++        ASSERT_THAT(client_frames, Le(compose_frames * 1.1f));
+     }
+ }
  /* Regression test LP: #1241369 / LP: #1241371 */
  TEST_F(BufferQueueTest, slow_client_framerate_matches_compositor)
+ {
--    /* BufferQueue can only satify this for nbuffers >= 3
--     * since a client can only own up to nbuffers - 1 at any one time
--     */
--    for (int nbuffers = 3; nbuffers <= 3; nbuffers++)
++    for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; nbuffers++)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          unsigned long client_frames = 0;
--        unsigned long const compose_frames = 100;
--        auto const frame_time = std::chrono::milliseconds(16);
++        unsigned long const compose_frames = 20;
++        auto const compositor_holding_time = std::chrono::milliseconds{16};
++        //simulate a client rendering time that's very close to the deadline
++        //simulated by the compositor holding time but not the same as that
++        //would imply the client will always misses its window to the next vsync
++        //and its framerate would be roughly half of the compositor rate
++        auto const client_rendering_time = std::chrono::milliseconds{12};
          q.allow_framedropping(false);
          std::atomic<bool> done(false);
--        std::mutex sync;
--        mt::AutoJoinThread monitor1([&]
++        mt::AutoJoinThread compositor_thread([&]
+         {
--            for (unsigned long frame = 0; frame != compose_frames+3; frame++)
++            for (unsigned long frame = 0; frame != compose_frames + 1; frame++)
+             {
--                std::this_thread::sleep_for(frame_time);
--                sync.lock();
                  auto buf = q.compositor_acquire(this);
++                std::this_thread::sleep_for(compositor_holding_time);
                  q.compositor_release(buf);
--                sync.unlock();
--
                  if (frame == compose_frames)
+                 {
--                    // Tell the "client" to stop after compose_frames, but
--                    // don't stop rendering immediately to avoid blocking
--                    // if we rendered any twice
--                    done.store(true);
++                    done = true;
+                 }
+             }
          });
@@ -950,23 +860,21 @@
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
          handle->release_buffer();
--        while (!done.load())
++        while (!done)
+         {
--            sync.lock();
--            sync.unlock();
              auto handle = client_acquire_async(q);
              handle->wait_for(std::chrono::seconds(1));
              ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--            std::this_thread::sleep_for(frame_time);
++            std::this_thread::sleep_for(client_rendering_time);
              handle->release_buffer();
              client_frames++;
+         }
--        monitor1.stop();
++        compositor_thread.stop();
          // Roughly compose_frames == client_frames within 20%
--        ASSERT_THAT(client_frames, Gt(compose_frames * 0.8f));
--        ASSERT_THAT(client_frames, Lt(compose_frames * 1.2f));
++        ASSERT_THAT(client_frames, Ge(compose_frames * 0.8f));
++        ASSERT_THAT(client_frames, Le(compose_frames * 1.2f));
+     }
+ }
@@ -974,7 +882,7 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          for (int width = 1; width < 100; ++width)
+         {
@@ -1009,8 +917,9 @@
+ {
      for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
--        mg::BufferID history[5];
++        TestBufferQueue q(nbuffers);
++        std::vector<mg::BufferID> history;
++        std::vector<std::shared_ptr<AcquireWaitHandle>> producing;
          const int width0 = 123;
          const int height0 = 456;
@@ -1018,7 +927,7 @@
          const int dy = -3;
          int width = width0;
          int height = height0;
--        int const nbuffers_to_use = q.buffers_free_for_client();
++        int const nbuffers_to_use = max_ownable_buffers(nbuffers);
          ASSERT_THAT(nbuffers_to_use, Gt(0));
          for (int produce = 0; produce < max_ownable_buffers(nbuffers); ++produce)
@@ -1030,10 +939,17 @@
              q.resize(new_size);
              auto handle = client_acquire_async(q);
              ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
--            history[produce] = handle->id();
++            history.push_back(handle->id());
++            producing.push_back(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;
@@ -1075,20 +991,22 @@
           nbuffers++)
+     {
          mtd::MockFrameDroppingPolicyFactory policy_factory;
--        mc::BufferQueue q(nbuffers,
--                          allocator,
--                          basic_properties,
--                          policy_factory);
++        TestBufferQueue q(nbuffers, policy_factory);
++
++        std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++        for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++            handles.push_back(client_acquire_async(q));
          for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
+         {
--            auto client = client_acquire_sync(q);
--            q.client_release(client);
++            auto& handle = handles[i];
++            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++            handle->release_buffer();
+         }
          auto handle = client_acquire_async(q);
--        EXPECT_FALSE(handle->has_acquired_buffer());
++        ASSERT_FALSE(handle->has_acquired_buffer());
          policy_factory.trigger_policies();
          EXPECT_TRUE(handle->has_acquired_buffer());
@@ -1102,15 +1020,17 @@
           nbuffers++)
+     {
          mtd::MockFrameDroppingPolicyFactory policy_factory;
--        mc::BufferQueue q(nbuffers,
--                          allocator,
--                          basic_properties,
--                          policy_factory);
++        TestBufferQueue q(nbuffers, policy_factory);
++
++        std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++        for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++            handles.push_back(client_acquire_async(q));
          for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
+         {
--            auto client = client_acquire_sync(q);
--            q.client_release(client);
++            auto& handle = handles[i];
++            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++            handle->release_buffer();
+         }
          /* Queue up two pending swaps */
@@ -1140,7 +1060,7 @@
      int const nbuffers{1};
      geom::Size const new_size{123,456};
--    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++    TestBufferQueue q(nbuffers);
      q.client_release(client_acquire_sync(q));
      q.resize(new_size);
@@ -1160,7 +1080,7 @@
  {  // Regression test for LP: #1319765
      using namespace testing;
--    mc::BufferQueue q{2, allocator, basic_properties, policy_factory};
++    TestBufferQueue q{2};
      q.client_release(client_acquire_sync(q));
      auto a = q.compositor_acquire(this);
@@ -1183,7 +1103,7 @@
  {  // Extended regression test for LP: #1319765
      using namespace testing;
--    mc::BufferQueue q{2, allocator, basic_properties, policy_factory};
++    TestBufferQueue q{2};
      for (int i = 0; i < 100; ++i)
+     {
@@ -1225,7 +1145,7 @@
      int const nbuffers{3};
--    mc::BufferQueue q{nbuffers, allocator, basic_properties, policy_factory};
++    TestBufferQueue q(nbuffers);
      q.allow_framedropping(true);
      std::vector<std::shared_ptr<mg::Buffer>> buffers;
@@ -1237,9 +1157,14 @@
      /* Let client release all possible buffers so they go into
       * the ready queue
       */
--    for (int i = 0; i < nbuffers; ++i)
--    {
--        auto handle = client_acquire_async(q);
++    std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++    for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++    {
++        handles.push_back(client_acquire_async(q));
++    }
++    for (int i = 0; i < max_ownable_buffers(nbuffers); ++i)
++    {
++        auto& handle = handles[i];
          ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
          /* Check the client never got the compositor buffer acquired above */
          ASSERT_THAT(handle->id(), Ne(comp_buffer->id()));
@@ -1247,7 +1172,7 @@
+     }
      /* Let the compositor acquire all ready buffers */
--    for (int i = 0; i < nbuffers; ++i)
++    for (int i = 0; i < max_ownable_buffers(nbuffers); ++i)
+     {
          buffers.push_back(q.compositor_acquire(this));
+     }
@@ -1273,7 +1198,7 @@
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          std::mutex client_buffer_guard;
          std::condition_variable client_buffer_cv;
@@ -1329,7 +1254,7 @@
+ {
      for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          for (int i = 0; i < 100; ++i)
+         {
              auto handle = client_acquire_async(q);
@@ -1365,7 +1290,7 @@
+ {
      for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++        TestBufferQueue q(nbuffers);
          void const* main_compositor = reinterpret_cast<void const*>(0);
          void const* second_compositor = reinterpret_cast<void const*>(1);
@@ -1416,18 +1341,16 @@
+             }
+         }
--        EXPECT_THAT(unique_buffers_acquired.size(), Eq(nbuffers));
++        EXPECT_THAT(unique_buffers_acquired.size(), Ge(nbuffers));
+     }
+ }
--/* 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)
++    for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+     {
--        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
--        q.allow_framedropping(false);
++        TestBufferQueue q(nbuffers);
          std::atomic<bool> done(false);
          auto unblock = [&done] { done = true; };
@@ -1450,6 +1373,40 @@
+     }
+ }
++TEST_F(BufferQueueTest, queue_shrinks_after_expanding)
++{
++    for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
++    {
++        TestBufferQueue q(nbuffers);
++
++        std::vector<std::shared_ptr<AcquireWaitHandle>> handles;
++        for (int i = 0; i < max_ownable_buffers(nbuffers); i++)
++        {
++            handles.push_back(client_acquire_async(q));
++        }
++
++        //Make sure the queue expanded
++        ASSERT_THAT(q.allocated_buffers(), Eq(nbuffers));
++
++        for (auto& handle : handles)
++        {
++            handle->release_buffer();
++        }
++
++        for (int i = 0; i <= q.default_shrink_threshold; i++)
++        {
++            auto comp_buffer = q.compositor_acquire(this);
++            q.compositor_release(comp_buffer);
++
++            auto handle = client_acquire_async(q);
++            ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++            handle->release_buffer();
++        }
++
++        EXPECT_THAT(q.allocated_buffers(), Lt(nbuffers));
++    }
++}
++
  /*
   * This is a regression test for bug lp:1317801. This bug is a race and
   * very difficult to reproduce with pristine code. By carefully placing
@@ -1467,7 +1424,7 @@
  TEST_F(BufferQueueTest, DISABLED_lp_1317801_regression_test)
+ {
      int const nbuffers = 3;
--    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
++    TestBufferQueue q(nbuffers);
      q.client_release(client_acquire_sync(q));