Merge into development-branch : non-blocking-swap-buffers : Code : Mir

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre (community)		2014-04-08	Approve on 2014-04-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-10
Kevin DuBois (community)			Abstain on 2014-04-09
Review via email: mp+214755@code.launchpad.net

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-08:

#

~~~
231+ static int const vsync_period_microsec{16666};
232+
233+ auto now = duration_cast<microseconds>(
234+ steady_clock::now().time_since_epoch());
235+ microseconds next_vsync{
236+ (now.count() / vsync_period_microsec + 1) * vsync_period_microsec};
237+ std::this_thread::sleep_until(steady_clock::time_point{next_vsync});
~~~

You can let chrono do the conversions instead

typedef std::chrono::duration<int, std::ratio<1, 60>> vsync_periods;

//truncate to vsync periods
auto now = std::chrono::duration_cast<vsync_periods>(std::chrono::steady_clock::now().time_since_epoch());
//convert back to a timepoint
auto now_tp = std::chrono::time_point<std::chrono::steady_clock, vsync_periods>{now};
//Next vsync time point
auto next_vsync = now_tp + vsync_periods(1);
std::this_thread::sleep_until(next_vsync);

For test at line 353 - doesn't it need a timeout mechanism in case it does block?

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-08:

#

comments
216 + if (r->visible())
I intend on removing visible and the saved resources vector soon anyways.

needs fixing:
220 + wait_until_next_vsync();
I'd rather this be "wait_a_reasonable_amount()" or something like that

discussion:
given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be solved more in the BufferStream/SwitchingBundle?

another thought:
could we do this in one loop/thread? (and only generate the renderlist once?)

188 + run_compositing_loop([&] { return display_buffer_compositor->composite();});
could we do:

run_compositing_loop([&] {
return display_buffer_compositor->composite(list);
//consume hidden surfaces?
});

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-09:

#

> 216 + if (r->visible())
> I intend on removing visible and the saved resources vector soon anyways.

Does that mean that scene->generate_renderable_list() will return only renderables that have buffers for rendering?

@saved_resources

I guess the plan is to keep buffers "alive" using some other mechanism. Will we be able to use that mechanism from outside the DisplayBufferCompositor, or do we need to continue use the saved resources vector in the BufferConsumingThread?

220 + wait_until_next_vsync();

wait_until_next_fake_vsync() ?

> given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via
> BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be
> solved more in the BufferStream/SwitchingBundle?

I thought a bit about this (but not much really), but couldn't find a satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is that this is not unconditional framedropping, but "framedropping" only if not rendered, which is only known at the compositor level. Even if we find a way to do it in BufferStream/SwitchingBundle, we need to take into account the complexity we are going to add to an already complex component. In any case, I am open to ideas :)

> another thought:
> could we do this in one loop/thread? (and only generate the renderlist once?)

We need to support the cases of multiple active monitors and of no active monitors (e.g. screen off on android). For multiple active monitors we could do as you propose if we have a way for the display_buffer_compositor to return all the surface buffers it didn't consume. Note, however, that in terms of consumed buffers it's almost the same as (or worse than) having the extra thread proposed in this MP. Imagine for example two compositing threads T1,T2 for two outputs and two sets of surfaces A (visible on first output) and B (visible on second output). If we consume hidden surfaces in the compositing threads then T1 will consume AUB and and T2 will consume BUA. With an extra thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both ways consume A twice and B twice. For more than two outputs however, consuming in the compositing thread themselves leads to more consumptions than with the extra thread.

For no active monitors and hence no compositing threads, we need the extra consuming thread anyway, so I decided to use it unconditionally, since it provides the behavior we want and it's conceptually simple (essentially just another (fake) output from the POV of our buffer handling).

Granted that for only one output what you describe is a win: we only use one thread and consume the hidden buffers only once. However, it does add some complexity that I wanted to avoid at this point, since this is a matter of optimization not correctness. I am fine with optimizing this use case when things have settled.

> 216 + if (r->visible())
> I intend on removing visible and the saved resources vector soon anyways.

Does that mean that scene->generate_renderable_list() will return only renderables that have buffers for rendering?

@saved_resources

I guess the plan is to keep buffers "alive" using some other mechanism. Will we be able to use that mechanism from outside the DisplayBufferCompositor, or do we need to continue use the saved resources vector in the BufferConsumingThread?

220 + wait_until_next_vsync();

wait_until_next_fake_vsync() ?

> given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via
> BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be
> solved more in the BufferStream/SwitchingBundle?

I thought a bit about this (but not much really), but couldn't find a satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is that this is not unconditional framedropping, but "framedropping" only if not rendered, which is only known at the compositor level. Even if we find a way to do it in BufferStream/SwitchingBundle, we need to take into account the complexity we are going to add to an already complex component. In any case, I am open to ideas :)

> another thought:
> could we do this in one loop/thread? (and only generate the renderlist once?)

We need to support the cases of multiple active monitors and of no active monitors (e.g. screen off on android). For multiple active monitors we could do as you propose if we have a way for the display_buffer_compositor to return all the surface buffers it didn't consume. Note, however, that in terms of consumed buffers it's almost the same as (or worse than) having the extra thread proposed in this MP. Imagine for example two compositing threads T1,T2 for two outputs and two sets of surfaces A (visible on first output) and B (visible on second output). If we consume hidden surfaces in the compositing threads then T1 will consume AUB and and T2 will consume BUA. With an extra thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both ways consume A twice and B twice. For more than two outputs however, consuming in the compositing thread themselves leads to more consumptions than with the extra thread.

For no active monitors and hence no compositing threads, we need the extra consuming thread anyway, so I decided to use it unconditionally, since it provides the behavior we want and it's conceptually simple (essentially just another (fake) output from the POV of our buffer handling).

Granted that for only one output what you describe is a win: we only use one thread and consume the hidden buffers only once. However, it does add some complexity that I wanted to avoid at this point, since this is a matter of optimization not correctness. I am fine with optimizing this use case when things have settled.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-09:

#

> You can let chrono do the conversions instead

Good idea. Fixed.

> For test at line 353 - doesn't it need a timeout mechanism in case it does block?

Fixed.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-09:

#

FAILED: Continuous integration, rev:1543
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1542
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1118/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1018/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1023/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1119/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

#

Looks good.

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-09:

#

Download full text (4.1 KiB)

> > 216 + if (r->visible())
> > I intend on removing visible and the saved resources vector soon anyways.
>
> Does that mean that scene->generate_renderable_list() will return only
> renderables that have buffers for rendering?

Right, the scene should be trimming out surfaces that are not visible().

>
> @saved_resources
>
> I guess the plan is to keep buffers "alive" using some other mechanism. Will
> we be able to use that mechanism from outside the DisplayBufferCompositor, or
> do we need to continue use the saved resources vector in the
> BufferConsumingThread?

We talked a bit about this during the standup with Daniel, I guess we'll have to see where it moves to. I was intending on moving it to the renderer or the RenderableList, although I quite liked moving it to the snapshots in the RenderableList on my first pass.

>
> 220 + wait_until_next_vsync();
>
> wait_until_next_fake_vsync() ?

This is fine, although I like
wait_until_next_client_unblock_event() a bit better. Either one.

>
> > given mc::BufferStream::allow_framedropping(), which is set by the client,
> it seems strange drop frames via
> > BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't
> problems with swapping and ordering be
> > solved more in the BufferStream/SwitchingBundle?
>
> I thought a bit about this (but not much really), but couldn't find a
> satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is
> that this is not unconditional framedropping, but "framedropping" only if not
> rendered, which is only known at the compositor level. Even if we find a way
> to do it in BufferStream/SwitchingBundle, we need to take into account the
> complexity we are going to add to an already complex component. In any case, I
> am open to ideas :)

Yeah, I don't have any ideas that don't involve complicating SwitchingBundle, so I'm happy to go with this solution... and hopefully follow up with an approach that doesn't involve a new thread once the dust has settled.

>
> > another thought:
> > could we do this in one loop/thread? (and only generate the renderlist
> once?)
>
> We need to support the cases of multiple active monitors and of no active
> monitors (e.g. screen off on android). For multiple active monitors we could
> do as you propose if we have a way for the display_buffer_compositor to return
> all the surface buffers it didn't consume. Note, however, that in terms of
> consumed buffers it's almost the same as (or worse than) having the extra
> thread proposed in this MP. Imagine for example two compositing threads T1,T2
> for two outputs and two sets of surfaces A (visible on first output) and B
> (visible on second output). If we consume hidden surfaces in the compositing
> threads then T1 will consume AUB and and T2 will consume BUA. With an extra
> thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both
> ways consume A twice and B twice. For more than two outputs however, consuming
> in the compositing thread themselves leads to more consumptions than with the
> extra thread.
>
> For no active monitors and hence no compositing threads, we need the extra
> consuming thread anyway, so I d...

> > 216 + if (r->visible())
> > I intend on removing visible and the saved resources vector soon anyways.
> 
> Does that mean that scene->generate_renderable_list() will return only
> renderables that have buffers for rendering?

Right, the scene should be trimming out surfaces that are not visible().

> 
> @saved_resources
> 
> I guess the plan is to keep buffers "alive" using some other mechanism. Will
> we be able to use that mechanism from outside the DisplayBufferCompositor, or
> do we need to continue use the saved resources vector in the
> BufferConsumingThread?

We talked a bit about this during the standup with Daniel, I guess we'll have to see where it moves to. I was intending on moving it to the renderer or the RenderableList, although I quite liked moving it to the snapshots in the RenderableList on my first pass.

> 
> 220 + wait_until_next_vsync();
> 
> wait_until_next_fake_vsync() ?

This is fine, although I like
wait_until_next_client_unblock_event() a bit better. Either one.

> 
> > given mc::BufferStream::allow_framedropping(), which is set by the client,
> it seems strange drop frames via
> > BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't
> problems with swapping and ordering be
> > solved more in the BufferStream/SwitchingBundle?
> 
> I thought a bit about this (but not much really), but couldn't find a
> satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is
> that this is not unconditional framedropping, but "framedropping" only if not
> rendered, which is only known at the compositor level. Even if we find a way
> to do it in BufferStream/SwitchingBundle, we need to take into account the
> complexity we are going to add to an already complex component. In any case, I
> am open to ideas :)

Yeah, I don't have any ideas that don't involve complicating SwitchingBundle, so I'm happy to go with this solution... and hopefully follow up with an approach that doesn't involve a new thread once the dust has settled.

> 
> > another thought:
> > could we do this in one loop/thread? (and only generate the renderlist
> once?)
> 
> We need to support the cases of multiple active monitors and of no active
> monitors (e.g. screen off on android). For multiple active monitors we could
> do as you propose if we have a way for the display_buffer_compositor to return
> all the surface buffers it didn't consume. Note, however, that in terms of
> consumed buffers it's almost the same as (or worse than) having the extra
> thread proposed in this MP. Imagine for example two compositing threads T1,T2
> for two outputs and two sets of surfaces A (visible on first output) and B
> (visible on second output). If we consume hidden surfaces in the compositing
> threads then T1 will consume AUB and and T2 will consume BUA. With an extra
> thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both
> ways consume A twice and B twice. For more than two outputs however, consuming
> in the compositing thread themselves leads to more consumptions than with the
> extra thread.
> 
> For no active monitors and hence no compositing threads, we need the extra
> consuming thread anyway, so I decided to use it unconditionally, since it
> provides the behavior we want and it's conceptually simple (essentially just
> another (fake) output from the POV of our buffer handling).
> 
> Granted that for only one output what you describe is a win: we only use one
> thread and consume the hidden buffers only once. However, it does add some
> complexity that I wanted to avoid at this point, since this is a matter of
> optimization not correctness. I am fine with optimizing this use case when
> things have settled.

ack, I'm okay with this approach, hopefully once the DisplayBufferCompositor is easily writable we can work around keeping that class easy to override, and figure out what to do with buffer consuming in the MultiThreadedCompositor case. I still smell that there are some logic-based approaches instead of timing/thread-based ones, but we can work through that a bit later.

I guess I'll abstain about the function naming, not a big enough objection to block the MP

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

#

434+ auto const surface = mir_connection_create_surface_sync(connection, &request_params);

Missing corresponding mir_surface_release_sync(surface);

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-10:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) on 2014-04-10:

#

review: Approve

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-10:

#

A prototype of an alternative solution to provide non-blocking SwapBuffers involving only SwitchingBundle changes is:

https://code.launchpad.net/~raof/mir/1hz-rendering-always/

We can change the rate to be whatever we like, as long it's less than the smallest vsync rate of any of the attached displays (plus some safety margin).

!!!Note: the functionality is currently broken in r1545 of that branch

For now we will go with the current branch to unblock our stack with Qt 5.2, and we can discuss more about the alternative as it matures.

My 2 cents so I don't forget:

Pros of the alternative: completely transparent to compositor, only switching bundle needs to be changed, consumes only buffers that are not otherwise consumed

Cons of the alternative: not obviously correct, makes an already complex component more complex, can't stop all buffer consumption by just stopping the compositor, can't reach up to vsync rate

Mir

Merge lp:~afrantzis/mir/non-blocking-swap-buffers into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === added file 'include/test/mir_test/spin_wait.h'
 --- include/test/mir_test/spin_wait.h	1970-01-01 00:00:00 +0000
 +++ include/test/mir_test/spin_wait.h	2014-04-10 07:45:21 +0000
@@ -0,0 +1,38 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
++ */
++
++#ifndef MIR_TEST_SPIN_WAIT_H_
++#define MIR_TEST_SPIN_WAIT_H_
++
++#include <functional>
++#include <chrono>
++
++namespace mir
++{
++namespace test
++{
++
++bool spin_wait_for_condition_or_timeout(
++    std::function<bool()> const& condition,
++    std::chrono::milliseconds timeout,
++    std::chrono::milliseconds spin_period = std::chrono::milliseconds{10});
++
++}
++}
++
++#endif /* MIR_TEST_SPIN_WAIT_H_ */
 === added file 'include/test/mir_test_doubles/null_display_buffer_compositor_factory.h'
 --- include/test/mir_test_doubles/null_display_buffer_compositor_factory.h	1970-01-01 00:00:00 +0000
 +++ include/test/mir_test_doubles/null_display_buffer_compositor_factory.h	2014-04-10 07:45:21 +0000
@@ -0,0 +1,52 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program. If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
++ */
++
++#ifndef MIR_TEST_DOUBLES_NULL_DISPLAY_BUFFER_COMPOSITOR_FACTORY_H_
++#define MIR_TEST_DOUBLES_NULL_DISPLAY_BUFFER_COMPOSITOR_FACTORY_H_
++
++#include "mir/compositor/display_buffer_compositor_factory.h"
++#include "mir/compositor/display_buffer_compositor.h"
++
++namespace mir
++{
++namespace test
++{
++namespace doubles
++{
++
++class NullDisplayBufferCompositorFactory : public compositor::DisplayBufferCompositorFactory
++{
++public:
++    auto create_compositor_for(graphics::DisplayBuffer&)
++        -> std::unique_ptr<compositor::DisplayBufferCompositor> override
++    {
++        struct NullDisplayBufferCompositor : compositor::DisplayBufferCompositor
++        {
++            bool composite() { return false; }
++        };
++
++        auto raw = new NullDisplayBufferCompositor{};
++        return std::unique_ptr<NullDisplayBufferCompositor>(raw);
++    }
++};
++
++}
++}
++}
++
++#endif /* MIR_TEST_DOUBLES_NULL_DISPLAY_BUFFER_COMPOSITOR_FACTORY_H_ */
 === modified file 'src/server/compositor/multi_threaded_compositor.cpp'
 --- src/server/compositor/multi_threaded_compositor.cpp	2014-03-07 03:15:55 +0000
 +++ src/server/compositor/multi_threaded_compositor.cpp	2014-04-10 07:45:21 +0000
@@ -26,6 +26,8 @@
  #include <thread>
  #include <condition_variable>
++#include <cstdint>
++#include <chrono>
  namespace mc = mir::compositor;
  namespace mg = mir::graphics;
@@ -56,37 +58,29 @@
  class CompositingFunctor
+ {
  public:
--    CompositingFunctor(std::shared_ptr<mc::DisplayBufferCompositorFactory> const& db_compositor_factory,
--                       mg::DisplayBuffer& buffer,
--                       std::shared_ptr<CompositorReport> const& report)
--        : display_buffer_compositor_factory{db_compositor_factory},
--          buffer(buffer),
--          running{true},
--          frames_scheduled{0},
--          report{report}
--    {
--    }
--
--    void operator()() noexcept  // noexcept is important! (LP: #1237332)
++    CompositingFunctor() : running{true}, frames_scheduled{false} {}
++    virtual ~CompositingFunctor() = default;
++
++    void schedule_compositing()
++    {
++        std::lock_guard<std::mutex> lock{run_mutex};
++
++        frames_scheduled = true;
++        run_cv.notify_one();
++    }
++
++    void stop()
++    {
++        std::lock_guard<std::mutex> lock{run_mutex};
++        running = false;
++        run_cv.notify_one();
++    }
++
++protected:
++    void run_compositing_loop(std::function<bool()> const& composite)
+     {
          std::unique_lock<std::mutex> lock{run_mutex};
--        /*
--         * Make the buffer the current rendering target, and release
--         * it when the thread is finished.
--         */
--        CurrentRenderingTarget target{buffer};
--
--        auto display_buffer_compositor = display_buffer_compositor_factory->create_compositor_for(buffer);
--
--        CompositorReport::SubCompositorId report_id =
--            display_buffer_compositor.get();
--
--        const auto& r = buffer.view_area();
--        report->added_display(r.size.width.as_int(), r.size.height.as_int(),
--                              r.top_left.x.as_int(), r.top_left.y.as_int(),
--                              report_id);
--
          while (running)
+         {
              /* Wait until compositing has been scheduled or we are stopped */
@@ -100,7 +94,8 @@
+             {
                  frames_scheduled = false;
                  lock.unlock();
--                auto more_frames_pending = display_buffer_compositor->composite();
++
++                auto more_frames_pending = composite();
                  /*
                   * Each surface could have a number of frames ready in its buffer
@@ -115,31 +110,103 @@
+         }
+     }
--    void schedule_compositing()
--    {
--        std::lock_guard<std::mutex> lock{run_mutex};
--
--        frames_scheduled = true;
--        run_cv.notify_one();
--    }
--
--    void stop()
--    {
--        std::lock_guard<std::mutex> lock{run_mutex};
--        running = false;
--        run_cv.notify_one();
--    }
--
  private:
--    std::shared_ptr<mc::DisplayBufferCompositorFactory> const display_buffer_compositor_factory;
--    mg::DisplayBuffer& buffer;
      bool running;
      bool frames_scheduled;
      std::mutex run_mutex;
      std::condition_variable run_cv;
++};
++
++class DisplayBufferCompositingFunctor : public CompositingFunctor
++{
++public:
++    DisplayBufferCompositingFunctor(
++        std::shared_ptr<mc::DisplayBufferCompositorFactory> const& db_compositor_factory,
++        mg::DisplayBuffer& buffer,
++        std::shared_ptr<mc::CompositorReport> const& report)
++        : display_buffer_compositor_factory{db_compositor_factory},
++          buffer(buffer),
++          report{report}
++    {
++    }
++
++    void operator()() noexcept // noexcept is important! (LP: #1237332)
++    {
++        /*
++         * Make the buffer the current rendering target, and release
++         * it when the thread is finished.
++         */
++        CurrentRenderingTarget target{buffer};
++
++        auto display_buffer_compositor = display_buffer_compositor_factory->create_compositor_for(buffer);
++
++        CompositorReport::SubCompositorId report_id =
++            display_buffer_compositor.get();
++
++        const auto& r = buffer.view_area();
++        report->added_display(r.size.width.as_int(), r.size.height.as_int(),
++                              r.top_left.x.as_int(), r.top_left.y.as_int(),
++                              report_id);
++
++        run_compositing_loop([&] { return display_buffer_compositor->composite();});
++    }
++
++private:
++    std::shared_ptr<mc::DisplayBufferCompositorFactory> const display_buffer_compositor_factory;
++    mg::DisplayBuffer& buffer;
      std::shared_ptr<CompositorReport> const report;
  };
++class BufferConsumingFunctor : public CompositingFunctor
++{
++public:
++    BufferConsumingFunctor(std::shared_ptr<mc::Scene> const& scene)
++        : scene{scene}
++    {
++    }
++
++    void operator()() noexcept // noexcept is important! (LP: #1237332)
++    {
++        run_compositing_loop(
++            [this]
++            {
++                std::vector<std::shared_ptr<mg::Buffer>> saved_resources;
++
++                auto const& renderables = scene->generate_renderable_list();
++
++                for (auto const& r : renderables)
++                {
++                    if (r->visible())
++                        saved_resources.push_back(r->buffer(this));
++                }
++
++                wait_until_next_fake_vsync();
++
++                return false;
++            });
++    }
++
++private:
++    void wait_until_next_fake_vsync()
++    {
++        using namespace std::chrono;
++        typedef duration<int64_t, std::ratio<1, 60>> vsync_periods;
++
++        /* Truncate to vsync periods */
++        auto const previous_vsync =
++            duration_cast<vsync_periods>(steady_clock::now().time_since_epoch());
++        /* Convert back to a timepoint */
++        auto const previous_vsync_tp =
++            time_point<steady_clock, vsync_periods>{previous_vsync};
++        /* Next vsync time point */
++        auto const next_vsync = previous_vsync_tp + vsync_periods(1);
++
++        std::this_thread::sleep_until(next_vsync);
++    }
++
++    std::shared_ptr<mc::Scene> const scene;
++};
++
+ }
+ }
@@ -184,13 +251,25 @@
      /* Start the compositing threads */
      display->for_each_display_buffer([this](mg::DisplayBuffer& buffer)
+     {
--        auto thread_functor_raw = new mc::CompositingFunctor{display_buffer_compositor_factory, buffer, report};
--        auto thread_functor = std::unique_ptr<mc::CompositingFunctor>(thread_functor_raw);
++        auto thread_functor_raw =
++            new mc::DisplayBufferCompositingFunctor{
++                display_buffer_compositor_factory, buffer, report};
++        auto thread_functor =
++            std::unique_ptr<mc::DisplayBufferCompositingFunctor>(thread_functor_raw);
          threads.push_back(std::thread{std::ref(*thread_functor)});
          thread_functors.push_back(std::move(thread_functor));
      });
++    /* Start the buffer consuming thread */
++    {
++        auto thread_functor_raw = new mc::BufferConsumingFunctor{scene};
++        auto thread_functor = std::unique_ptr<mc::BufferConsumingFunctor>(thread_functor_raw);
++
++        threads.push_back(std::thread{std::ref(*thread_functor)});
++        thread_functors.push_back(std::move(thread_functor));
++    }
++
      /* Recomposite whenever the scene changes */
      scene->set_change_callback([this]()
+     {
@@ -205,7 +284,6 @@
          lk.unlock();
          schedule_compositing();
+     }
--
+ }
  void mc::MultiThreadedCompositor::stop()
 === modified file 'tests/acceptance-tests/CMakeLists.txt'
 --- tests/acceptance-tests/CMakeLists.txt	2014-03-26 05:48:59 +0000
 +++ tests/acceptance-tests/CMakeLists.txt	2014-04-10 07:45:21 +0000
@@ -28,6 +28,7 @@
    test_client_library_drm.cpp
    test_protobuf.cpp
    test_client_screencast.cpp
++  test_client_surface_swap_buffers.cpp
    ${GENERATED_PROTOBUF_SRCS}
    ${GENERATED_PROTOBUF_HDRS}
+ )
 === added file 'tests/acceptance-tests/test_client_surface_swap_buffers.cpp'
 --- tests/acceptance-tests/test_client_surface_swap_buffers.cpp	1970-01-01 00:00:00 +0000
 +++ tests/acceptance-tests/test_client_surface_swap_buffers.cpp	2014-04-10 07:45:21 +0000
@@ -0,0 +1,102 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
++ */
++
++#include "mir_toolkit/mir_client_library.h"
++
++#include "mir_test_framework/stubbed_server_configuration.h"
++#include "mir_test_framework/in_process_server.h"
++#include "mir_test_framework/using_stub_client_platform.h"
++#include "mir_test_doubles/null_display_buffer_compositor_factory.h"
++#include "mir_test/spin_wait.h"
++
++#include <gtest/gtest.h>
++
++#include <atomic>
++
++namespace mtf = mir_test_framework;
++namespace mtd = mir::test::doubles;
++namespace mc = mir::compositor;
++
++namespace
++{
++
++struct StubServerConfig : mtf::StubbedServerConfiguration
++{
++    auto the_display_buffer_compositor_factory()
++        -> std::shared_ptr<mc::DisplayBufferCompositorFactory> override
++    {
++        return std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
++    }
++};
++
++class MirSurfaceSwapBuffersTest : public mir_test_framework::InProcessServer
++{
++public:
++    mir::DefaultServerConfiguration& server_config() override { return server_config_; }
++
++    StubServerConfig server_config_;
++    mtf::UsingStubClientPlatform using_stub_client_platform;
++};
++
++void swap_buffers_callback(MirSurface*, void* ctx)
++{
++    auto buffers_swapped = static_cast<std::atomic<bool>*>(ctx);
++    *buffers_swapped = true;
++}
++
++}
++
++TEST_F(MirSurfaceSwapBuffersTest, swap_buffers_does_not_block_when_surface_is_not_composited)
++{
++    using namespace testing;
++
++    auto const connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
++    ASSERT_TRUE(mir_connection_is_valid(connection));
++
++    MirSurfaceParameters request_params =
++    {
++        __PRETTY_FUNCTION__,
++        640, 480,
++        mir_pixel_format_abgr_8888,
++        mir_buffer_usage_hardware,
++        mir_display_output_id_invalid
++    };
++
++    auto const surface = mir_connection_create_surface_sync(connection, &request_params);
++    ASSERT_NE(nullptr, surface);
++
++    for (int i = 0; i < 10; ++i)
++    {
++        std::atomic<bool> buffers_swapped{false};
++
++        mir_surface_swap_buffers(surface, swap_buffers_callback, &buffers_swapped);
++
++        mir::test::spin_wait_for_condition_or_timeout(
++            [&] { return buffers_swapped.load(); },
++            std::chrono::seconds{5});
++
++        /*
++         * ASSERT instead of EXPECT, since if we continue we will block in future
++         * mir client calls (e.g mir_connection_release).
++         */
++        ASSERT_TRUE(buffers_swapped.load());
++    }
++
++    mir_surface_release_sync(surface);
++    mir_connection_release(connection);
++}
 === modified file 'tests/mir_test/CMakeLists.txt'
 --- tests/mir_test/CMakeLists.txt	2013-08-28 03:41:48 +0000
 +++ tests/mir_test/CMakeLists.txt	2014-04-10 07:45:21 +0000
@@ -4,4 +4,5 @@
    pipe.cpp
    display_config_matchers.cpp
    cross_process_action.cpp
++  spin_wait.cpp
+ )
 === added file 'tests/mir_test/spin_wait.cpp'
 --- tests/mir_test/spin_wait.cpp	1970-01-01 00:00:00 +0000
 +++ tests/mir_test/spin_wait.cpp	2014-04-10 07:45:21 +0000
@@ -0,0 +1,36 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
++ */
++
++#include "mir_test/spin_wait.h"
++
++#include <thread>
++
++bool mir::test::spin_wait_for_condition_or_timeout(
++    std::function<bool()> const& condition,
++    std::chrono::milliseconds timeout,
++    std::chrono::milliseconds spin_period)
++{
++    auto const end = std::chrono::steady_clock::now() + timeout;
++
++    while (std::chrono::steady_clock::now() < end && !condition())
++    {
++        std::this_thread::sleep_for(spin_period);
++    }
++
++    return condition();
++}
 === modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
 --- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-03 00:14:45 +0000
 +++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-10 07:45:21 +0000
@@ -26,12 +26,16 @@
  #include "mir_test_doubles/mock_display_buffer.h"
  #include "mir_test_doubles/mock_compositor_report.h"
  #include "mir_test_doubles/mock_scene.h"
++#include "mir_test_doubles/stub_renderable.h"
++#include "mir_test_doubles/null_display_buffer_compositor_factory.h"
++#include "mir_test/spin_wait.h"
  #include <unordered_map>
  #include <unordered_set>
  #include <thread>
  #include <mutex>
  #include <chrono>
++#include <atomic>
  #include <gmock/gmock.h>
  #include <gtest/gtest.h>
@@ -84,11 +88,15 @@
  class StubScene : public mc::Scene
+ {
  public:
--    StubScene() : callback{[]{}} {}
++    StubScene(mg::RenderableList const& list)
++        : callback{[]{}}, renderable_list{list}
++    {
++    }
++    StubScene() : StubScene(mg::RenderableList{}) {}
      mg::RenderableList generate_renderable_list() const
+     {
--        return mg::RenderableList{};
++        return renderable_list;
+     }
      void set_change_callback(std::function<void()> const& f)
@@ -114,6 +122,7 @@
  private:
      std::function<void()> callback;
      std::mutex callback_mutex;
++    mg::RenderableList renderable_list;
  };
  class RecordingDisplayBufferCompositor : public mc::DisplayBufferCompositor
@@ -286,19 +295,24 @@
      unsigned int render_count;
  };
--class NullDisplayBufferCompositorFactory : public mc::DisplayBufferCompositorFactory
++class BufferCountingRenderable : public mtd::StubRenderable
+ {
  public:
--    std::unique_ptr<mc::DisplayBufferCompositor> create_compositor_for(mg::DisplayBuffer&)
--    {
--        struct NullDisplayBufferCompositor : mc::DisplayBufferCompositor
--        {
--            bool composite() { return false; }
--        };
--
--        auto raw = new NullDisplayBufferCompositor{};
--        return std::unique_ptr<NullDisplayBufferCompositor>(raw);
--    }
++    BufferCountingRenderable() : buffers_requested_{0} {}
++
++    std::shared_ptr<mg::Buffer> buffer(void const*) const override
++    {
++        ++buffers_requested_;
++        return std::make_shared<mtd::StubBuffer>();
++    }
++
++    int buffers_requested() const
++    {
++        return buffers_requested_;
++    }
++
++private:
++    mutable std::atomic<int> buffers_requested_;
  };
  auto const null_report = mr::null_compositor_report();
@@ -525,7 +539,7 @@
      auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
      auto scene = std::make_shared<StubScene>();
--    auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
++    auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
      mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory, null_report, true};
      display->for_each_mock_buffer([](mtd::MockDisplayBuffer& mock_buf)
@@ -542,17 +556,22 @@
  TEST(MultiThreadedCompositor, double_start_or_stop_ignored)
+ {
++    using namespace testing;
++
      unsigned int const nbuffers{3};
      auto display = std::make_shared<StubDisplayWithMockBuffers>(nbuffers);
      auto mock_scene = std::make_shared<mtd::MockScene>();
--    auto db_compositor_factory = std::make_shared<NullDisplayBufferCompositorFactory>();
++    auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
      auto mock_report = std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
      EXPECT_CALL(*mock_report, started())
          .Times(1);
      EXPECT_CALL(*mock_report, stopped())
          .Times(1);
--    EXPECT_CALL(*mock_scene, set_change_callback(testing::_))
++    EXPECT_CALL(*mock_scene, set_change_callback(_))
          .Times(2);
++    EXPECT_CALL(*mock_scene, generate_renderable_list())
++        .Times(AtLeast(0))
++        .WillRepeatedly(Return(mg::RenderableList{}));
      mc::MultiThreadedCompositor compositor{display, mock_scene, db_compositor_factory, mock_report, true};
@@ -561,3 +580,37 @@
      compositor.stop();
      compositor.stop();
+ }
++
++TEST(MultiThreadedCompositor, consumes_buffers_for_renderables_that_are_not_rendered)
++{
++    using namespace testing;
++
++    unsigned int const nbuffers{2};
++    auto renderable = std::make_shared<BufferCountingRenderable>();
++    auto display = std::make_shared<StubDisplay>(nbuffers);
++    auto stub_scene = std::make_shared<StubScene>(mg::RenderableList{renderable});
++    // We use NullDisplayBufferCompositors to simulate DisplayBufferCompositors
++    // not rendering a renderable.
++    auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
++
++    mc::MultiThreadedCompositor compositor{
++        display, stub_scene, db_compositor_factory, null_report, true};
++
++    compositor.start();
++
++    mir::test::spin_wait_for_condition_or_timeout(
++        [&] { return renderable->buffers_requested() == 1; },
++        std::chrono::seconds{5});
++
++    EXPECT_THAT(renderable->buffers_requested(), Eq(1));
++
++    stub_scene->emit_change_event();
++
++    mir::test::spin_wait_for_condition_or_timeout(
++        [&] { return renderable->buffers_requested() == 2; },
++        std::chrono::seconds{5});
++
++    EXPECT_THAT(renderable->buffers_requested(), Eq(2));
++
++    compositor.stop();
++}