Mir

Merge lp:~kdub/mir/multiple-bufferstream-api into lp:mir

multiple-bufferstream-api
Merge into development-branch

Proposed by Kevin DuBois on 2015-06-04

Status:	Merged
Approved by:	Kevin DuBois on 2015-06-16
Approved revision:	2479
Merged at revision:	2670
Proposed branch:	lp:~kdub/mir/multiple-bufferstream-api
Merge into:	lp:mir
Prerequisite:	lp:~kdub/mir/add-streams-to-surface
Diff against target:	644 lines (+402/-15) 18 files modified include/client/mir_toolkit/client_types.h (+10/-0) include/client/mir_toolkit/mir_surface.h (+22/-0) include/server/mir/scene/session.h (+3/-0) src/client/mir_surface.cpp (+13/-0) src/client/mir_surface.h (+1/-0) src/client/mir_surface_api.cpp (+20/-0) src/client/symbols.map (+1/-0) src/server/frontend/session_mediator.cpp (+10/-7) src/server/scene/application_session.cpp (+9/-0) src/server/scene/application_session.h (+1/-0) src/server/scene/basic_surface.cpp (+10/-7) src/server/shell/canonical_window_manager.cpp (+8/-1) tests/acceptance-tests/CMakeLists.txt (+1/-0) tests/acceptance-tests/test_buffer_stream_arrangement.cpp (+239/-0) tests/include/mir_test_doubles/mock_scene_session.h (+2/-0) tests/include/mir_test_doubles/mock_surface.h (+1/-0) tests/include/mir_test_doubles/stub_scene_session.h (+4/-0) tests/unit-tests/scene/test_application_session.cpp (+47/-0)
To merge this branch:	bzr merge lp:~kdub/mir/multiple-bufferstream-api
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre			Approve on 2015-06-16
PS Jenkins bot	continuous-integration		Approve on 2015-06-16
Alexandros Frantzis (community)		2015-06-04	Approve on 2015-06-10
Review via email: mp+261123@code.launchpad.net

Commit Message

Add the client api to associate multiple streams together. Connect bits throughout the system to get the acceptance test for this feature to pass.

Description of the Change

Add the client api to associate multiple streams together. Connect bits throughout the system to get the acceptance test for this feature to pass.

notes:
The acceptance test here is about the same as the one from the forerunner mp:
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/256186
The only difference is that it only adds one spec-modifying function though... the other client api functions there don't seem useful.

This is the multibufferstream with-async-streams plumbed all the way through. It gets the other bufferstreams onscreen. Currently working on renovating scroll.cpp to have a demo.

PS Jenkins bot (ps-jenkins) wrote on 2015-06-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4016/rebuild

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-08

2460. By Kevin DuBois on 2015-06-08: merge in mir
2461. By Kevin DuBois on 2015-06-08: fix up clang

Alexandros Frantzis (afrantzis) wrote on 2015-06-08:

10+ MirBufferStream * stream;

'*' with type

26+/** Set the streams associated with the spec.
27+ * streams[0] is the bottom-most stream, and streams[size-1] is the topmost.

Space before this line.

28+ * On application of the spec, A stream that is present in the surface,

Space before this line (different topic started).
'a stream'

30+ * On application of the spec, A stream that is not present in the surface,
31+ * but is in the list will be associated with the surface.

'<some other connecting phrase>, a stream that is not present' ...

32+ * Streams set a displacement from the top-left corner of the surface.

Space before this line (change of topic), or perhaps move this information to the documentation of the MirBufferStreamInfo structure?

57+#include <list>
69+ virtual void configure_streams(Surface& surface, std::list<shell::StreamSpecification> const& config) = 0;

Somewhat pre-existing: Is a vector<> not sufficient for this use case? I see that Surface::set_streams() uses a list<> which I don't think offers any advantages over vector<> for our usage patterns and is a probably a pessimization ("use vector<> by default").

91+ for(auto
110+ for(auto
... and elsewhere

'for ('

517+ primary_stream = std::unique_ptr<Stream>(
525+ streams.emplace_back(std::unique_ptr<Stream>(

make_unique

517+ primary_stream = std::unique_ptr<Stream>(
549+ infos[i++] = MirBufferStreamInfo{

I doesn't seem that we need to treat the primary stream specially. Treating it as yet another stream simplifies the code.

553+ for(auto &stream

'auto const&'

442+ bool ensure_last_ordering_is_consistent_with(443+ std::vector<geom::Displacement> const& arrangement)
444+ {

... return arrangment == displacement;

Or even better, change:

574+ EXPECT_TRUE(ordering->ensure_last_ordering_is_consistent_with(displacements))

to EXPECT_THAT(ordering->last_arrangement(), ContainerEq(displacements)) to get more detailed information in case of failure.

review: Needs Fixing

PS Jenkins bot (ps-jenkins) wrote on 2015-06-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4028/rebuild

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-08

2462. By Kevin DuBois on 2015-06-08: fix comments
2463. By Kevin DuBois on 2015-06-08: whitespace
2464. By Kevin DuBois on 2015-06-08: store all streams without having a primary one
2465. By Kevin DuBois on 2015-06-08: less homespun container checking
2466. By Kevin DuBois on 2015-06-08: easy to change to a list
2467. By Kevin DuBois on 2015-06-08: merge mir again

Kevin DuBois (kdub) wrote on 2015-06-08:

Issues should be fixed.
With:
'for(' vs 'for ('
I obviously don't find this to affect readability... maybe will suggest a change to style.

PS Jenkins bot (ps-jenkins) wrote on 2015-06-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4037/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-06-09:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4045/rebuild

review: Needs Fixing (continuous-integration)

Alberto Aguirre (albaguirre) wrote on 2015-06-09:

===
45 + * \param [in] size The size of streams.

49 + unsigned int size);
===

Maybe change size to num_elements?
"The number of elements in the streams array" to avoid confusion?

===
148 +void mir_surface_spec_set_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, unsigned int size)
===

The implementation should validate its preconditions with mir::require(mir_buffer_stream_is_valid(...)) for all buffer streams.

===
172 +MIR_CLIENT_9.1 { # New in Mir 0.14
173 + global:
174 + mir_surface_spec_set_streams;
175 +} MIR_CLIENT_9;
===

No need for a 9.1 stanza as we haven't released anything with MIR_CLIENT_9 yet.

review: Needs Fixing

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-09

2468. By Kevin DuBois on 2015-06-09: do not introduce stanza 9.1. check that every stream is valid before dispatching call, and change name in the public header file to num_elements
2469. By Kevin DuBois on 2015-06-09: merge in mir

PS Jenkins bot (ps-jenkins) wrote on 2015-06-09:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4056/rebuild

review: Needs Fixing (continuous-integration)

Alberto Aguirre (albaguirre) wrote on 2015-06-09:

===
45 + * \param [in] size The number of elements in the streams array.
===

doxygen comment still refers to size instead of num_streams.

"[0;32m[ RUN ] [mBufferStreamArrangement.arrangements_are_applied
/tmp/buildd/mir-0.14.0bzr2469pkg0wily2729+autopilot0/tests/acceptance-tests/test_buffer_stream_arrangement.cpp:236: Failure
Value of: ordering->wait_for_another_post_within(1s)
Actual: false
Expected: true
timed out waiting for another post"

===
561 + EXPECT_TRUE(ordering->wait_for_another_post_within(1s))
===
I guess running with Valgrind in CI needs more than 1s.

Looks good otherwise.

review: Needs Fixing

Alexandros Frantzis (afrantzis) wrote on 2015-06-10:

Looks good (besides what others have already mentioned, of course)

Typo:

34+ * On application of the spec, A stream that is not present in the surface,

'a stream'

review: Approve

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-10

2470. By Kevin DuBois on 2015-06-10: merge in mir, resolve 3 conflicts
2471. By Kevin DuBois on 2015-06-10: another capitalization fix
2472. By Kevin DuBois on 2015-06-10: increase timeout for valgrind

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4066/rebuild

review: Needs Fixing (continuous-integration)

Alberto Aguirre (albaguirre) wrote on 2015-06-10:

LGTM.

review: Approve

Kevin DuBois (kdub) wrote on 2015-06-10:

gl2mark error?

Kevin DuBois (kdub) wrote on 2015-06-10:

I wonder if this is https://bugs.launchpad.net/mir/+bug/1341490

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-10

2473. By Kevin DuBois on 2015-06-10: bump protobuf soname abi number

Kevin DuBois (kdub) wrote on 2015-06-10:

If you take the 'artifacts' zip and install it on the device (along with the glmark debian package, like the script does), the glmark error is reproducible. It seems that there was some abi-incompatible mixing and matching of (mirclient8, mirclient9) and libmirprotobuf0. Seems appropriate to bump the protobuf versioning so that the right libraries get picked up.

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4069/rebuild

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-10

2474. By Kevin DuBois on 2015-06-10: remove printf

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4070/rebuild

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-10

2475. By Kevin DuBois on 2015-06-10: merge in mir, fix a conflict

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

FAILED: Continuous integration, rev:2474
http://jenkins.qa.ubuntu.com/job/mir-ci/4072/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2807/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/320/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2755/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/228/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2755/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4072/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4073/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-06-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4074/rebuild

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/multiple-bufferstream-api updated on 2015-06-16

2476. By Kevin DuBois on 2015-06-16: reverse protobuf bump
2477. By Kevin DuBois on 2015-06-16: merge in miri
2478. By Kevin DuBois on 2015-06-16: remove the protobuf changes, they were added just for creation, and were interested in only modifying the surfaces
2479. By Kevin DuBois on 2015-06-16: move back file

Kevin DuBois (kdub) wrote on 2015-06-16:

side-stepped protobuf problem by not adding new field. The SurfaceParameter changes broke it, but the test in this MP only needed already existing protobuf fields.

PS Jenkins bot (ps-jenkins) wrote on 2015-06-16:

PASSED: Continuous integration, rev:2479
http://jenkins.qa.ubuntu.com/job/mir-ci/4112/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2866
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/379
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2814
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/268
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/268/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2814
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2814/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5644
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21219

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4112/rebuild

review: Approve (continuous-integration)

Alberto Aguirre (albaguirre) on 2015-06-16:

review: Approve

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

Kevin DuBois

Mir development team

 === modified file 'include/client/mir_toolkit/client_types.h'
 --- include/client/mir_toolkit/client_types.h	2015-05-29 04:23:56 +0000
 +++ include/client/mir_toolkit/client_types.h	2015-06-16 12:28:06 +0000
@@ -283,6 +283,16 @@
      MirDisplayCard *cards;
  } MirDisplayConfiguration;
++/**
++ * The displacement from the top-left corner of the surface.
++ */
++typedef struct MirBufferStreamInfo
++{
++    MirBufferStream* stream;
++    int displacement_x;
++    int displacement_y;
++} MirBufferStreamInfo;
++
  typedef struct MirRectangle
+ {
      int left;
 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2015-06-10 09:00:22 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2015-06-16 12:28:06 +0000
@@ -433,6 +433,28 @@
  void mir_surface_spec_release(MirSurfaceSpec* spec);
  /**
++ * Set the streams associated with the spec.
++ * streams[0] is the bottom-most stream, and streams[size-1] is the topmost.
++ * On application of the spec, a stream that is present in the surface,
++ * but is not in the list will be disassociated from the surface.
++ * On application of the spec, a stream that is not present in the surface,
++ * but is in the list will be associated with the surface.
++ * Streams set a displacement from the top-left corner of the surface.
++ *
++ * \warning disassociating streams from the surface will not release() them.
++ * \warning It is wiser to arrange the streams within the bounds of the
++ *          surface the spec is applied to. Shells can define their own
++ *          behavior as to what happens to an out-of-bound stream.
++ *
++ * \param [in] spec        The spec to accumulate the request in.
++ * \param [in] streams     The an array of non-null streams info.
++ * \param [in] num_streams The number of elements in the streams array.
++ */
++void mir_surface_spec_set_streams(MirSurfaceSpec* spec,
++                                  MirBufferStreamInfo* streams,
++                                  unsigned int num_streams);
++
++/**
   * Set the event handler to be called when events arrive for a surface.
   *   \warning event_handler could be called from another thread. You must do
   *            any locking appropriate to protect your data accessed in the
 === modified file 'include/server/mir/scene/session.h'
 --- include/server/mir/scene/session.h	2015-05-19 21:34:34 +0000
 +++ include/server/mir/scene/session.h	2015-06-16 12:28:06 +0000
@@ -22,10 +22,12 @@
  #include "mir/frontend/session.h"
  #include "mir/scene/snapshot.h"
++#include <vector>
  #include <sys/types.h>
  namespace mir
+ {
++namespace shell { struct StreamSpecification; }
  namespace scene
+ {
  class Surface;
@@ -60,6 +62,7 @@
      virtual frontend::BufferStreamId create_buffer_stream(graphics::BufferProperties const& props) = 0;
      virtual void destroy_buffer_stream(frontend::BufferStreamId stream) = 0;
++    virtual void configure_streams(Surface& surface, std::vector<shell::StreamSpecification> const& config) = 0;
  };
+ }
+ }
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2015-06-10 11:13:15 +0000
 +++ src/client/mir_surface.cpp	2015-06-16 12:28:06 +0000
@@ -23,6 +23,7 @@
  #include "cursor_configuration.h"
  #include "client_buffer_stream_factory.h"
  #include "mir_connection.h"
++#include "client_buffer_stream.h"
  #include "mir/dispatch/threaded_dispatcher.h"
  #include "mir/input/input_platform.h"
  #include "mir/input/xkb_mapper.h"
@@ -664,6 +665,18 @@
          aspect->set_height(spec.max_aspect.value().height);
+     }
++    if (spec.streams.is_set())
++    {
++        for(auto const& stream : spec.streams.value())
++        {
++            auto const new_stream = surface_specification->add_stream();
++            new_stream->set_displacement_x(stream.displacement_x);
++            new_stream->set_displacement_y(stream.displacement_y);
++            new_stream->mutable_id()->set_value(
++                reinterpret_cast<mcl::ClientBufferStream*>(stream.stream)->rpc_id().as_value());
++        }
++    }
++
      modify_wait_handle.expect_result();
      server->modify_surface(0, &mods, &modify_result,
                google::protobuf::NewCallback(this, &MirSurface::on_modified));
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2015-05-29 04:23:56 +0000
 +++ src/client/mir_surface.h	2015-06-16 12:28:06 +0000
@@ -97,6 +97,7 @@
      mir::optional_value<int> height_inc;
      mir::optional_value<AspectRatio> min_aspect;
      mir::optional_value<AspectRatio> max_aspect;
++    mir::optional_value<std::vector<MirBufferStreamInfo>> streams;
  };
  struct MirPersistentId
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2015-06-10 09:00:22 +0000
 +++ src/client/mir_surface_api.cpp	2015-06-16 12:28:06 +0000
@@ -516,9 +516,29 @@
      // Keep calm and carry on
+ }
++void mir_surface_spec_set_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, unsigned int size)
++try
++{
++    mir::require(spec);
++
++    std::vector<MirBufferStreamInfo> copy;
++    for (auto i = 0u; i < size; i++)
++    {
++        mir::require(mir_buffer_stream_is_valid(streams[i].stream));
++        copy.emplace_back(streams[i]);
++    }
++    spec->streams = copy;
++}
++catch (std::exception const& ex)
++{
++    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
++}
++
++
  void mir_surface_spec_set_parent(MirSurfaceSpec* spec, MirSurface* parent)
  try
+ {
++    mir::require(spec);
      spec->parent = parent;
+ }
  catch (std::exception const& ex)
 === modified file 'src/client/symbols.map'
 --- src/client/symbols.map	2015-06-10 23:37:28 +0000
 +++ src/client/symbols.map	2015-06-16 12:28:06 +0000
@@ -142,6 +142,7 @@
      mir_surface_spec_set_parent;
      mir_surface_spec_set_pixel_format;
      mir_surface_spec_set_preferred_orientation;
++    mir_surface_spec_set_streams;
      mir_surface_spec_set_type;
      mir_surface_spec_set_width;
      mir_surface_spec_set_width_increment;
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2015-06-09 07:20:56 +0000
 +++ src/server/frontend/session_mediator.cpp	2015-06-16 12:28:06 +0000
@@ -451,15 +451,18 @@
      // max_aspect is a special case (below)
  #undef COPY_IF_SET
--    std::vector<msh::StreamSpecification> stream_spec;
--    for(auto& stream : surface_specification.stream())
++    if (surface_specification.stream_size() > 0)
+     {
--        stream_spec.emplace_back(
--            msh::StreamSpecification{
--                mf::BufferStreamId{stream.id().value()},
--                geom::Displacement{stream.displacement_x(), stream.displacement_y()}});
++        std::vector<msh::StreamSpecification> stream_spec;
++        for (auto& stream : surface_specification.stream())
++        {
++            stream_spec.emplace_back(
++                msh::StreamSpecification{
++                    mf::BufferStreamId{stream.id().value()},
++                    geom::Displacement{stream.displacement_x(), stream.displacement_y()}});
++        }
++        mods.streams = std::move(stream_spec);
+     }
--    mods.streams = std::move(stream_spec);
      if (surface_specification.has_aux_rect())
+     {
 === modified file 'src/server/scene/application_session.cpp'
 --- src/server/scene/application_session.cpp	2015-06-02 13:41:40 +0000
 +++ src/server/scene/application_session.cpp	2015-06-16 12:28:06 +0000
@@ -322,3 +322,12 @@
      std::unique_lock<std::mutex> lock(surfaces_and_streams_mutex);
      streams.erase(checked_find(id));
+ }
++
++void ms::ApplicationSession::configure_streams(
++    ms::Surface& surface, std::vector<shell::StreamSpecification> const& streams)
++{
++    std::list<ms::StreamInfo> list;
++    for (auto& stream : streams)
++        list.emplace_back(ms::StreamInfo{checked_find(stream.stream_id)->second, stream.displacement});
++    surface.set_streams(list);
++}
 === modified file 'src/server/scene/application_session.h'
 --- src/server/scene/application_session.h	2015-05-19 21:34:34 +0000
 +++ src/server/scene/application_session.h	2015-06-16 12:28:06 +0000
@@ -85,6 +85,7 @@
      std::shared_ptr<frontend::BufferStream> get_buffer_stream(frontend::BufferStreamId stream) const override;
      frontend::BufferStreamId create_buffer_stream(graphics::BufferProperties const& params) override;
      void destroy_buffer_stream(frontend::BufferStreamId stream) override;
++    void configure_streams(Surface& surface, std::vector<shell::StreamSpecification> const& config) override;
  protected:
      ApplicationSession(ApplicationSession const&) = delete;
 === modified file 'src/server/scene/basic_surface.cpp'
 --- src/server/scene/basic_surface.cpp	2015-06-08 17:22:32 +0000
 +++ src/server/scene/basic_surface.cpp	2015-06-16 12:28:06 +0000
@@ -844,15 +844,18 @@
  void ms::BasicSurface::set_streams(std::list<scene::StreamInfo> const& s)
+ {
--    std::unique_lock<std::mutex> lk(guard);
--
--    if (s.end() == std::find_if(s.begin(), s.end(),
--        [this] (ms::StreamInfo const& info) { return info.stream == surface_buffer_stream; }))
+     {
--        BOOST_THROW_EXCEPTION(std::logic_error("cannot remove the created-with buffer stream yet"));
++        std::unique_lock<std::mutex> lk(guard);
++
++        if (s.end() == std::find_if(s.begin(), s.end(),
++            [this] (ms::StreamInfo const& info) { return info.stream == surface_buffer_stream; }))
++        {
++            BOOST_THROW_EXCEPTION(std::logic_error("cannot remove the created-with buffer stream yet"));
++        }
++
++        layers = s;
+     }
--
--    layers = s;
++    observers.moved_to(surface_rect.top_left);
+ }
  mg::RenderableList ms::BasicSurface::generate_renderables(mc::CompositorID id) const
 === modified file 'src/server/shell/canonical_window_manager.cpp'
 --- src/server/shell/canonical_window_manager.cpp	2015-06-11 09:41:30 +0000
 +++ src/server/shell/canonical_window_manager.cpp	2015-06-16 12:28:06 +0000
@@ -274,7 +274,7 @@
+ }
  void msh::CanonicalWindowManagerPolicy::handle_modify_surface(
--    std::shared_ptr<scene::Session> const& /*session*/,
++    std::shared_ptr<scene::Session> const& session,
      std::shared_ptr<scene::Surface> const& surface,
      SurfaceSpecification const& modifications)
+ {
@@ -369,6 +369,13 @@
      if (modifications.name.is_set())
          surface->rename(modifications.name.value());
++    if (modifications.streams.is_set())
++    {
++        auto v = modifications.streams.value();
++        std::vector<shell::StreamSpecification> l (v.begin(), v.end());
++        session->configure_streams(*surface, l);
++    }
++
      if (modifications.width.is_set() || modifications.height.is_set())
+     {
          auto new_size = surface->size();
 === modified file 'tests/acceptance-tests/CMakeLists.txt'
 --- tests/acceptance-tests/CMakeLists.txt	2015-06-11 09:41:30 +0000
 +++ tests/acceptance-tests/CMakeLists.txt	2015-06-16 12:28:06 +0000
@@ -32,6 +32,7 @@
    test_prompt_session_client_api.cpp
    test_client_screencast.cpp
    test_client_surface_visibility.cpp
++  test_buffer_stream_arrangement.cpp
    test_client_with_custom_display_config_deadlock.cpp
    test_server_without_active_outputs.cpp
    test_server_startup.cpp
 === added file 'tests/acceptance-tests/test_buffer_stream_arrangement.cpp'
 --- tests/acceptance-tests/test_buffer_stream_arrangement.cpp	1970-01-01 00:00:00 +0000
 +++ tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2015-06-16 12:28:06 +0000
@@ -0,0 +1,239 @@
++/*
++ * Copyright © 2015 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: Kevin DuBois <kevin.dubois@canonical.com>
++ */
++
++#include "mir_toolkit/mir_client_library.h"
++#include "mir_test_framework/connected_client_with_a_surface.h"
++#include "mir/compositor/display_buffer_compositor.h"
++#include "mir/compositor/display_buffer_compositor_factory.h"
++#include "mir/compositor/scene_element.h"
++#include "mir/graphics/renderable.h"
++#include "mir/graphics/cursor.h"
++#include "mir/geometry/displacement.h"
++
++#include <mutex>
++#include <condition_variable>
++#include <stdexcept>
++#include <gtest/gtest.h>
++#include <gmock/gmock.h>
++
++namespace mtf = mir_test_framework;
++namespace geom = mir::geometry;
++namespace mc = mir::compositor;
++namespace mg = mir::graphics;
++namespace
++{
++
++MirPixelFormat an_available_format(MirConnection* connection)
++{
++    using namespace testing;
++    MirPixelFormat format{mir_pixel_format_invalid};
++    unsigned int valid_formats{0};
++    mir_connection_get_available_surface_formats(connection, &format, 1, &valid_formats);
++    return format;
++}
++
++struct Stream
++{
++    Stream(MirBufferStream* stream, geom::Point pt) :
++        stream(stream),
++        pos{pt},
++        needs_release{false}
++    {
++        mir_buffer_stream_swap_buffers_sync(stream);
++    }
++
++    Stream(MirConnection* connection, geom::Rectangle rect) :
++        stream(mir_connection_create_buffer_stream_sync(
++            connection,
++            rect.size.width.as_int(),
++            rect.size.height.as_int(),
++            an_available_format(connection),
++            mir_buffer_usage_hardware)),
++        pos{rect.top_left},
++        needs_release{true}
++    {
++        mir_buffer_stream_swap_buffers_sync(stream);
++    }
++
++    ~Stream()
++    {
++        if (needs_release)
++            mir_buffer_stream_release_sync(stream);
++    }
++
++    MirBufferStream* handle() const
++    {
++        return stream;
++    }
++
++    geom::Point position()
++    {
++        return pos;
++    }
++
++    Stream(Stream const&) = delete;
++    Stream& operator=(Stream const&) = delete;
++private:
++    MirBufferStream* stream;
++    geom::Point const pos;
++    bool const needs_release;
++};
++
++struct Ordering
++{
++    void note_scene_element_sequence(mc::SceneElementSequence& sequence)
++    {
++        if (sequence.empty())
++            return;
++
++        std::unique_lock<decltype(mutex)> lk(mutex);
++        displacements.clear();
++
++        auto first_position = (*sequence.begin())->renderable()->screen_position().top_left;
++        for (auto const& element : sequence)
++            displacements.emplace_back(element->renderable()->screen_position().top_left - first_position);
++        post_count++;
++
++        cv.notify_all();
++    }
++
++    template<typename T, typename S>
++    bool wait_for_another_post_within(std::chrono::duration<T,S> duration)
++    {
++        std::unique_lock<decltype(mutex)> lk(mutex);
++        auto count = post_count + 1;
++        return cv.wait_for(lk, duration, [this, count]{return (post_count >= count);});
++    }
++
++    std::vector<geom::Displacement> last_ordering()
++    {
++        return displacements;
++    }
++private:
++    std::mutex mutex;
++    std::condition_variable cv;
++    unsigned int post_count{0};
++    std::vector<geom::Displacement> displacements;
++};
++
++struct OrderTrackingDBC : mc::DisplayBufferCompositor
++{
++    OrderTrackingDBC(
++        std::shared_ptr<mc::DisplayBufferCompositor> const& wrapped,
++        std::shared_ptr<Ordering> const& ordering) :
++        wrapped(wrapped),
++        ordering(ordering)
++    {
++    }
++
++    void composite(mc::SceneElementSequence&& scene_sequence) override
++    {
++        ordering->note_scene_element_sequence(scene_sequence);
++        wrapped->composite(std::move(scene_sequence));
++    }
++
++    std::shared_ptr<mc::DisplayBufferCompositor> const wrapped;
++    std::shared_ptr<Ordering> const ordering;
++};
++
++struct OrderTrackingDBCFactory : mc::DisplayBufferCompositorFactory
++{
++    OrderTrackingDBCFactory(
++        std::shared_ptr<mc::DisplayBufferCompositorFactory> const& wrapped,
++        std::shared_ptr<Ordering> const& ordering) :
++        wrapped(wrapped),
++        ordering(ordering)
++    {
++    }
++
++    std::unique_ptr<mc::DisplayBufferCompositor> create_compositor_for(mg::DisplayBuffer& db) override
++    {
++        return std::make_unique<OrderTrackingDBC>(wrapped->create_compositor_for(db), ordering);
++    }
++
++    std::shared_ptr<OrderTrackingDBC> last_dbc{nullptr};
++    std::shared_ptr<mc::DisplayBufferCompositorFactory> const wrapped;
++    std::shared_ptr<Ordering> const ordering;
++};
++
++struct BufferStreamArrangement : mtf::ConnectedClientWithASurface
++{
++    void SetUp() override
++    {
++        ordering = std::make_shared<Ordering>();
++        server.wrap_display_buffer_compositor_factory(
++            [this](std::shared_ptr<mc::DisplayBufferCompositorFactory> const& wrapped)
++            {
++                order_tracker = std::make_shared<OrderTrackingDBCFactory>(wrapped, ordering);
++                return order_tracker;
++            });
++
++        ConnectedClientWithASurface::SetUp();
++        server.the_cursor()->hide();
++
++        streams.emplace_back(
++            std::make_unique<Stream>(mir_surface_get_buffer_stream(surface), geom::Point{0,0}));
++        int const additional_streams{3};
++        for (auto i = 0; i < additional_streams; i++)
++        {
++            geom::Size size{30 * i + 1, 40* i + 1};
++            geom::Point position{i * 2, i * 3};
++            streams.emplace_back(std::make_unique<Stream>(connection, geom::Rectangle{position, size}));
++        }
++    }
++
++    void TearDown() override
++    {
++        streams.clear();
++        ConnectedClientWithASurface::TearDown();
++    }
++
++    std::shared_ptr<Ordering> ordering;
++    std::shared_ptr<OrderTrackingDBCFactory> order_tracker{nullptr};
++    std::vector<std::unique_ptr<Stream>> streams;
++};
++}
++
++TEST_F(BufferStreamArrangement, arrangements_are_applied)
++{
++    using namespace testing;
++    std::vector<MirBufferStreamInfo> infos(streams.size());
++    auto i = 0u;
++    for (auto const& stream : streams)
++    {
++        infos[i++] = MirBufferStreamInfo{
++            stream->handle(),
++            stream->position().x.as_int(),
++            stream->position().y.as_int()};
++    }
++
++    auto change_spec = mir_connection_create_spec_for_changes(connection);
++    mir_surface_spec_set_streams(change_spec, infos.data(), infos.size());
++    mir_surface_apply_spec(surface, change_spec);
++    mir_surface_spec_release(change_spec);
++
++    std::vector<geom::Displacement> displacements;
++    for (auto& info : infos)
++        displacements.emplace_back(geom::Displacement{info.displacement_x, info.displacement_y});
++
++    //check that the compositor rendered correctly
++    using namespace std::literals::chrono_literals;
++    EXPECT_TRUE(ordering->wait_for_another_post_within(5s))
++         << "timed out waiting for another post";
++    EXPECT_THAT(ordering->last_ordering(), ContainerEq(displacements));
++}
 === modified file 'tests/include/mir_test_doubles/mock_scene_session.h'
 --- tests/include/mir_test_doubles/mock_scene_session.h	2015-05-19 21:34:34 +0000
 +++ tests/include/mir_test_doubles/mock_scene_session.h	2015-06-16 12:28:06 +0000
@@ -20,6 +20,7 @@
  #define MIR_TEST_DOUBLES_MOCK_SCENE_SESSION_H_
  #include "mir/scene/session.h"
++#include "mir/scene/surface.h"
  #include "mir/scene/surface_creation_parameters.h"
  #include "mir/graphics/display_configuration.h"
@@ -64,6 +65,7 @@
      MOCK_METHOD1(destroy_buffer_stream, void(frontend::BufferStreamId));
      MOCK_METHOD1(create_buffer_stream, frontend::BufferStreamId(graphics::BufferProperties const&));
++    MOCK_METHOD2(configure_streams, void(scene::Surface&, std::vector<shell::StreamSpecification> const&));
  };
+ }
 === modified file 'tests/include/mir_test_doubles/mock_surface.h'
 --- tests/include/mir_test_doubles/mock_surface.h	2015-06-08 17:28:38 +0000
 +++ tests/include/mir_test_doubles/mock_surface.h	2015-06-16 12:28:06 +0000
@@ -74,6 +74,7 @@
      MOCK_METHOD1(consume, void(MirEvent const&));
      MOCK_CONST_METHOD0(primary_buffer_stream, std::shared_ptr<frontend::BufferStream>());
++    MOCK_METHOD1(set_streams, void(std::list<scene::StreamInfo> const&));
  };
 === modified file 'tests/include/mir_test_doubles/stub_scene_session.h'
 --- tests/include/mir_test_doubles/stub_scene_session.h	2015-05-19 21:34:34 +0000
 +++ tests/include/mir_test_doubles/stub_scene_session.h	2015-06-16 12:28:06 +0000
@@ -117,6 +117,10 @@
          return frontend::BufferStreamId();
+     }
++    void configure_streams(scene::Surface&, std::vector<shell::StreamSpecification> const&)
++    {
++    }
++
      pid_t const pid;
  };
 === modified file 'tests/unit-tests/scene/test_application_session.cpp'
 --- tests/unit-tests/scene/test_application_session.cpp	2015-05-29 17:36:46 +0000
 +++ tests/unit-tests/scene/test_application_session.cpp	2015-06-16 12:28:06 +0000
@@ -515,6 +515,53 @@
      session->destroy_surface(id);
+ }
++MATCHER(StreamEq, "")
++{
++    return (std::get<0>(arg).stream == std::get<1>(arg).stream) &&
++        (std::get<0>(arg).displacement == std::get<1>(arg).displacement);
++}
++
++TEST_F(ApplicationSession, sets_and_looks_up_surface_streams)
++{
++    using namespace testing;
++    NiceMock<MockBufferStreamFactory> mock_bufferstream_factory;
++    NiceMock<MockSurfaceFactory> mock_surface_factory;
++
++    auto mock_surface = make_mock_surface();
++    EXPECT_CALL(mock_surface_factory, create_surface(_,_))
++        .WillOnce(Return(mock_surface));
++
++    std::array<std::shared_ptr<mc::BufferStream>,3> streams {{
++        std::make_shared<mtd::StubBufferStream>(),
++        std::make_shared<mtd::StubBufferStream>(),
++        std::make_shared<mtd::StubBufferStream>()
++    }};
++    EXPECT_CALL(mock_bufferstream_factory, create_buffer_stream(_))
++        .WillOnce(Return(streams[0]))
++        .WillOnce(Return(streams[1]))
++        .WillOnce(Return(streams[2]));
++
++    auto stream_properties = mg::BufferProperties{{8,8}, mir_pixel_format_argb_8888, mg::BufferUsage::hardware};
++    auto session = make_application_session(
++        mt::fake_shared(mock_bufferstream_factory),
++        mt::fake_shared(mock_surface_factory));
++    auto stream_id0 = mf::BufferStreamId(session->create_surface(ms::a_surface().of_position({1,1})).as_value());
++    auto stream_id1 = session->create_buffer_stream(stream_properties);
++    auto stream_id2 = session->create_buffer_stream(stream_properties);
++
++    std::list<ms::StreamInfo> info {
++        {streams[2], geom::Displacement{0,3}},
++        {streams[0], geom::Displacement{-1,1}},
++        {streams[1], geom::Displacement{0,2}}
++    };
++    EXPECT_CALL(*mock_surface, set_streams(Pointwise(StreamEq(), info)));
++    session->configure_streams(*mock_surface, {
++        {stream_id2, geom::Displacement{0,3}},
++        {stream_id0, geom::Displacement{-1,1}},
++        {stream_id1, geom::Displacement{0,2}}
++    });
++}
++
  TEST_F(ApplicationSession, buffer_stream_constructed_with_requested_parameters)
+ {
      using namespace ::testing;