Merge lp:~kdub/mir/RFC-surface-arrangements into lp:mir

[Kevin DuBois]

[still] Just a RFC branch!

In looking at how to bypass nested servers, it seemed useful that advanced clients need a way to specify the ordering of the streams that they have created.

I got to the point of writing a failing acceptance test for this feature when I thought I'd gather some comments on the client api change.

[Kevin DuBois (kdub)]

Just an RFC on an unimplemented client api change. Resubmit after implementation.

review: Needs Resubmitting

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2436
http://jenkins.qa.ubuntu.com/job/mir-ci/3403/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1875
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1874/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1825
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1400/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1825
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1825/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

(0) I've proposed an attribute batching API that will probably land soon and might be helpful. Check it out:
https://code.launchpad.net/~vanvugt/mir/rename-api/+merge/252864

Using that you could put relative ordering preferences of child surfaces in their MirSurfaceSpec instead. Which I think makes more sense.

(1) I think "order" is a more widely understood term than "arrangement" here. But also think the functions should be different in other ways than just name. See (0).

Instead, child surfaces could have an "int child_depth". Then their final Z-order is determined by (a) the depth of the parent surface within the main stack, plus (b) their relative int child_depth.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

*Discussion*

I think this only addresses a part of a range of problems and would like to be sure that we have a consistent solution. As Daniel mentions, the stacking order of surfaces should be part of the problem domain here (at least to the extent that a client can e.g. "raise" some of its surfaces). We should also allow a client to shift the focus (or default focus) to a specific surface.

As proposed, this is making requests for a specific action without sufficient semantic content for the server to implement good alternative behavior if the request can't be met in full: if a window encounters an edge to the display should it overlap, resize or have an adjusted position?

Specifying top-left locations of surfaces seems like a simplistic layout rule. We should consider the existing solution domain of layout managers and, at least /consider/, allowing the specification of anchors, insets, fills, weights, etc.

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> Specifying top-left locations of surfaces seems like a simplistic layout rule.
> We should consider the existing solution domain of layout managers and, at
> least /consider/, allowing the specification of anchors, insets, fills,
> weights, etc.

I'm not saying we can't do something adequate for now and evolve a more complex solution as needed, but that the current proposal is too specific to facilitate that evolution.

[Kevin DuBois (kdub)]

0) The batching api looks like it could be helpful in making sure that changes are applied at once. Packing the z-order into the MirSurfaceSpec might be more uniform, although I did not see a way to change the z order or the position once the surface was created.
1) I did consider specifying an int-depth as an attribute, but that seemed like additional processing for us, when the client is really just trying to establish a list of its surfaces. Also, if you have 2 surfaces at z-depth 4 and 5, and want 3rd surface in between, the client has to reassign one of the surfaces to fit that surface where they want (which seemed a bit easier to do if its just an array). A list or an array also eliminates the corner case of two surfaces at the same level that we'd have to check for.

I guess the other thing to point out is that I put this as a mir_connection_ function to avoid the idea of parent/child surfaces. It seems more flexible to just have a collection of surfaces ordered by the client in the way it prefers (while still sticking to our policies)

[Kevin DuBois (kdub)]

> I think this only addresses a part of a range of problems and would like to be
> sure that we have a consistent solution. As Daniel mentions, the stacking
> order of surfaces should be part of the problem domain here (at least to the
> extent that a client can e.g. "raise" some of its surfaces). We should also
> allow a client to shift the focus (or default focus) to a specific surface.

So the intent here is to let the client specify its own surface ordering, we're making no guarantees about what the final z-order or position the surface will be presented in on-screen. Its more that we're guaranteeing the client that their surfaces will be composited in a specific way amongst themselves.

>
> As proposed, this is making requests for a specific action without sufficient
> semantic content for the server to implement good alternative behavior if the
> request can't be met in full: if a window encounters an edge to the display
> should it overlap, resize or have an adjusted position?

I don't know that we want to reinterpret what the client has said it wants to do... If it violates our window policies, we'll reject the request. (or perhaps I've misunderstood the point)

>
> Specifying top-left locations of surfaces seems like a simplistic layout rule.
> We should consider the existing solution domain of layout managers and, at
> least /consider/, allowing the specification of anchors, insets, fills,
> weights, etc.

I'm interested in being able to represent a SceneElementSequence-like specification of the composition that the client wants, although don't mind further expansion.

[Andreas Pokorny (andreas-pokorny)]

> Specifying top-left locations of surfaces seems like a simplistic layout rule.
> We should consider the existing solution domain of layout managers and, at
> least /consider/, allowing the specification of anchors, insets, fills,
> weights, etc.

I think we need to distinguish between surfaces and renderbuffers/scenelements here. What you wrote would be true for forwarding surfaces...

I do not think this is part of the problem to be solved (at least for now). A solution is needed that allows a nested server to forward all information necessary to allow the hosting server to draw the scene as if the nested server did the composition. At the moment and in the near future the super set of our efficient drawing operations (hwc + setting a scan out buffer + atomic planes) is what DefaultDisplayBufferCompositor implements. Hence the nested server has to submit the information a DefaultDisplayBufferCompositor needs for rendering. Thus a sequence of Renderables (i think we can ignore the Decoration accessor in SceneElement for that, and also put aside the oddness around the semantics Renderable::transformation() vs Renderable::screen_position()).

Still the idea of forwarding surfaces could be interesting too.

[Andreas Pokorny (andreas-pokorny)]

PS: oops hit save comment too soon.

So I see two different cases, a nested server should be allowed to forward surfaces as surfaces. That would nicely fulfill the case of the OS virtualizer, forwarding all windows of the guest system to the host system, hence the host is supposed to do all the window management and dispatch logic. In that case the nested server creates a surface at the host server for each client surfaces, and relays changes between those.

The other topic is that a nested server should be able to just relay all Renderable changes within the scene and buffer changes to the host server. So as a result a Surface then HasA sequence of Renderables.

[Andreas Pokorny (andreas-pokorny)]

Back to the nested bypass: On the client maybe:

MirBufferStreamAttachment* mir_surface_create_buffer_stream_attachment(MirSurface*, MirBufferStream*, int relative_pos_x, int relative_pos_y, float* transformation?, float opacity);

and respective setters, and a mir_surface_submit_buffer_stream_changes that sends all pending changes in a single batch. Or make that more generic mir_surfae_submit_changes?

[Chris Halse Rogers (raof)]

So, relevant prior art here is:
http://cgit.freedesktop.org/wayland/wayland/tree/protocol/wayland.xml#n1994 (for the purposes of that section, wl_surface == MirBufferStream, wl_surface.commit == mir_buffer_stream_swap_buffers)

The API contains the three obvious things - set_position, place_above, place_below - but also the non-obvious but important set_sync/set_desync. This serves two purposes - one is to batch up changes into a transaction, which we can easily satisfy by other means, the other is to ensure that the surface content of the whole subsurface tree is consistent.

When in synchronised mode, swap_buffers in all the subsurfaces is deferred until the parent surface enters swap_buffers; this ensures that the client can change the subsurface graph without flashing or components transiently moving out of place.

Like Andreas, I think that Surfaces are the wrong level to do this nesting. Surfaces have a whole bunch of semantics that USC simply doesn't care about, and can't do anything sensible with. What happens with input? Does USC generate enter/leave events? Etc.

Since Robert's animated-cursor branch has landed we've got a client API to create a raw BufferStream; What would seem most sensible to me is for this to add the ability to attach extra buffer streams to a surface.

mir_subsurface_position(MirSurface* in, MirBufferStream* to_place, int x, int y);
mir_subsurface_stack_above(MirSurface* in, MirBufferStream* to_raise, MirBufferStream* below);
mir_subsurface_stack_below(MirSurface* in, MirBufferStream* to_lower, MirBufferStream* above);
... etc

Here we never need to reject a (well-formed) subsurface request; as far as the shell is concerned there is only one surface, just with funky content.

[Daniel van Vugt (vanvugt)]

> Specifying top-left locations of surfaces seems like a simplistic layout rule.
> We should consider the existing solution domain of layout managers and, at
> least /consider/, allowing the specification of anchors, insets, fills,
> weights, etc.

That's going way too far down the path of making Mir a full-blown toolkit. Making Mir a toolkit with enforced semantics just makes it less welcoming to supporting actual toolkits with their own semantics.

[Daniel van Vugt (vanvugt)]

If you want a good example of what level of complexity to target this functionality at, have a look at:
  http://tronche.com/gui/x/xlib/window/stacking-order.html

That's _not_ a suggestion for a design, but an example of the level of detail we should be considering.

[Alan Griffiths (alan-griffiths)]

> > I think this only addresses a part of a range of problems and would like to
> be
> > sure that we have a consistent solution. As Daniel mentions, the stacking
> > order of surfaces should be part of the problem domain here (at least to the
> > extent that a client can e.g. "raise" some of its surfaces). We should also
> > allow a client to shift the focus (or default focus) to a specific surface.
>
> So the intent here is to let the client specify its own surface ordering,
> we're making no guarantees about what the final z-order or position the
> surface will be presented in on-screen. Its more that we're guaranteeing the
> client that their surfaces will be composited in a specific way amongst
> themselves.

I think the use case of "raise these surfaces and direct input towards this one" needs more explicit support than having to list all surfaces.

> > As proposed, this is making requests for a specific action without
> sufficient
> > semantic content for the server to implement good alternative behavior if
> the
> > request can't be met in full: if a window encounters an edge to the display
> > should it overlap, resize or have an adjusted position?
>
> I don't know that we want to reinterpret what the client has said it wants to
> do... If it violates our window policies, we'll reject the request. (or
> perhaps I've misunderstood the point)

The general approach we've taken with other requests is that the client makes a request and the server decided what to do and (mostly) tells the client that something happened.

But in the case of positioning the client doesn't know what it has to work with:

  o if a surface has a "snapped" border that limits its positioning.
  o The client is not informed when surfaces are moved by the user.
  o The client usually has no indication which screen(s) its surfaces are on.
  o if the WM is doing tiling there's no information on current tile size.

So, unless we provide more information to the client it can't always determine the perfect layout. Allowing the server the flexibility to make a "best effort" is consistent with other APIs.

[Kevin DuBois (kdub)]

Yay, the rfc is working (and has generated comments)!

With the client api structuring... (MirConnections have MirSurfaces, etc)
We currently have MirConnections have multiple MirSurfaces, and MirConnections have a single MirBufferStream. I didn't quite see why MirConnections with multiple MirSurfaces, each with multiple BufferStreams is better than keeping the structure that we have.

I think we're all pointing out that some sort of batching of position updates is needed if the client is to request arrangements of its own surfaces. The start of the batching work in Daniel's rename-api branch seems like a sensible starting point. The other suggestion seems to be the one in this original rfc (which seems to underpowered), and the suggestion in the wayland subsurface protocol of having a 'synced' and 'unsynced' surface. I don't know that we need swaps to batch with positioning updates, and I also don't know that we need a 'desynchronized mode'

[Kevin DuBois (kdub)]

> I think the use case of "raise these surfaces and direct input towards this
> one" needs more explicit support than having to list all surfaces.

Fair enough, and I think the rename-api detaches the user from having to list all the surfaces, and it also avoids the wart of "if you don't specify what to do with a surface, the server can do whatever it wants", as the other surfaces wo,uld just keep their state.

[Kevin DuBois (kdub)]

> But in the case of positioning the client doesn't know what it has to work
> with:
>
> o if a surface has a "snapped" border that limits its positioning.
> o The client is not informed when surfaces are moved by the user.
> o The client usually has no indication which screen(s) its surfaces are on.
> o if the WM is doing tiling there's no information on current tile size.

It is somewhat the goal to keep the client from knowing what its absolute final position though, and to mesh the preference the client gives us in terms of the coordinates of 'the clients plane' with the other policies that the window manager wants.

Like, if the client says "surface A at 100, 100, surface B at 200,100" the server won't honor that if it violates some of the rules, or if the user goes and moves the surface otherwise. Maybe the client needs a way to know if its request is being honored (or maybe how its being honored) via the event system so it can sort out what it wants to do.

[Chris Halse Rogers (raof)]

> Yay, the rfc is working (and has generated comments)!
>
> With the client api structuring... (MirConnections have MirSurfaces, etc)
> We currently have MirConnections have multiple MirSurfaces, and MirConnections
> have a single MirBufferStream. I didn't quite see why MirConnections with
> multiple MirSurfaces, each with multiple BufferStreams is better than keeping
> the structure that we have.

I think the idea of letting clients do relative positioning of their own surfaces is a poor one - Alan's raised all sorts of issues with it, and to that I'll add “what happens when a user moves one of the surfaces”?

The MirConnection has zero or more MirSurfaces which have one or more MirBufferStreams arrangement resolves this by providing a layer at which the client is in total control. The shell sees and acts on MirConnections and MirSurfaces, and Mir handles compositing (or not) the MirBufferStreams internally. In this case there's never any need to reject a MirBufferStream request (as long as all the objects it references exist and are valid, of course).

> I think we're all pointing out that some sort of batching of position updates
> is needed if the client is to request arrangements of its own surfaces. The
> start of the batching work in Daniel's rename-api branch seems like a sensible
> starting point. The other suggestion seems to be the one in this original rfc
> (which seems to underpowered), and the suggestion in the wayland subsurface
> protocol of having a 'synced' and 'unsynced' surface. I don't know that we
> need swaps to batch with positioning updates,

The obvious case where you need swaps to batch with positioning updates is when you're not drawing anything behind the overlaying surface. When you move the overlaying surface you need the underlay to be updated on the same frame as the move, otherwise you see bits of the unrendered area.

> and I also don't know that we need a 'desynchronized mode'

We don't, really. It's basically a client-courtesy-optimisation, where if you *don't* need frame-accurate moving etc you don't have to continually swap buffers on your main surface.

[Robert Carr (robertcarr)]

>> I think the idea of letting clients do relative positioning of their own surfaces is a poor >> one - Alan's raised all sorts of issues with it, and to that I'll add “what happens when a >> user moves one of the surfaces”?

>> The MirConnection has zero or more MirSurfaces which have one or more MirBufferStreams
>> arrangement resolves this by providing a layer at which the client is in total control. The >> shell sees and acts on MirConnections and MirSurfaces, and Mir handles compositing (or not) >> the MirBufferStreams internally.

+1

[Kevin DuBois (kdub)]

Still just an RFC, tried to take the above comments into consideration with the new acceptance test.

review: Needs Resubmitting

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2439
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/254982/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3429/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1920
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1919/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1870/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1426/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1870
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1870/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4821/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19368

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

54 +/** Specify the position of the stream within the surface. This establishes
55 + * positions relative to the other streams in the surface, and is not a
56 + * guarantee of the final composited ons

You appear to have garbled a comment here ☺. I'm not sure if what the comment says is right; we *should* be able to say that the relative position is guaranteed to be correct, but we guarantee nothing about the position of (0,0) onscreen.

This does raise the question of whether or not we want to clip streams to the parent surface; Wayland doesn't for subsurfaces, and I think GTK uses this for tooltips and such. That said, we know how to position tooltips and such better than the toolkit, so they shouldn't really be using it for this purpose.

I don't know if we should clip to the parent window or not; whichever way we go, we should document and test it, though.

This otherwise looks like sensible API to me. I do think we'll need at least an optional synchronise-on-swap_buffers mode, though.

[Alan Griffiths (alan-griffiths)]

76 +unsigned int mir_surface_get_number_of_streams(MirSurface* surface);
...
88 +void mir_surface_get_streams(
89 + MirSurface* surface, MirBufferStream** streams, MirRectangle* positions, unsigned int num_streams);

I'm always nervous of an "atomic" operation (get all the streams) that takes two calls. Can't we at least have a return value from mir_surface_get_streams() that can indicates the number of streams actually returned if num_streams is greater than the number available?

[Alan Griffiths (alan-griffiths)]

Makes a lot more sense. But I'm not sure how it applies to a multi-windowed client - or are windows no longer surfaces?

Nits:

MirSurfaceArrangements - probably not the right name for arranging buffer streams.

65 + * \param [in] stream_to_move The stream to movee

s/ee/e/

[Kevin DuBois (kdub)]

Addressed nits and made the the function return the maximum possible number of streams, which is a useful allocation hint for the client, and also lets some surface-specs report a maximum of 1 stream. The function that returns the info now reports how many of the array were filled with valid data.

> Makes a lot more sense. But I'm not sure how it applies to a multi-windowed
> client - or are windows no longer surfaces?

Eh, I'm still a bit hazy on the difference myself, but in the course of this discussing RFC, it seems that some differences have evolved. Namely, a surface can get input and has a specification, whereas a buffer stream doesn't have that rigging.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2441
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/254982/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3443/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1939
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1938/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1889/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1440/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1889
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1889/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4836/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19400

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

> Makes a lot more sense. But I'm not sure how it applies to a multi-windowed
> client - or are windows no longer surfaces?

It applies to multi-window clients the same way as single-window clients; each window can now have a bunch of BufferStreams associated with it, and the client has full control over the arrangement of those BufferStreams.

A multi-window client has no special control over the arrangements of its windows; I don't believe that's necessary for the nested-bypass case (or for any of the other use-cases raised), and I don't think it's a good idea to implement.

> Addressed nits and made the the function return the maximum possible number of
> streams, which is a useful allocation hint for the client, and also lets some
> surface-specs report a maximum of 1 stream. The function that returns the info
> now reports how many of the array were filled with valid data.

I wonder whether this should actually be an accessor on MirSurfaceSpec, rather than on the Surface? We'd probably want to rename mir_surface_begin_changes() to mir_surface_snapshot_state() or equivalent, but that seems OK. We don't need or want to do any locking (do we?) in the time between mir_surface_begin_changes() and mir_surface_commit_changes(). We would probably want to disallow commit_changes() if any surface state had changed between snapshot_create() and commit()?

This would give us easy atomicity - the SurfaceSpec is a coherent snapshot of Surface state at time of creation.

It also gets around needing to tell the client how much memory to dynamically allocate; they can instead just iterate from 0 to mir_surface_spec_get_num_streams(spec) (or moral equivalent).

It *also* makes it easy to extend - at the moment mir_surface_get_streams just reports location and stacking data, but its an obvious extension to add alpha blending, buffer transform, and such. We probably wouldn't want to extend mir_surface_get_streams to take an extra two arrays to fill.

[Chris Halse Rogers (raof)]

Also, garbled comment remains:

56+/** Specify the position of the stream within the surface. This establishes
57+ * positions relative to the other streams in the surface, and is not a
58+ * guarantee of the final composited ons

I presume that's going to say “...and is not a guarantee of the final composited onscreen position”? I don't think we need to say anything about that? We guarantee that we'll respect the relative positioning, and say nothing about absolute positioning.

[Chris Halse Rogers (raof)]

Oh, and I think this is close enough to mark as needs-fixing :)

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

I like the direction this took. Now there are only a few details missing. Like hidding/removing of buffer streams, and the obvious state changes like transformation and alpha that are part of the current Renderbuffer APIR. I wonder if above/below is practical, but it seems sufficient.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> A multi-window client has no special control over the arrangements of its
> windows; I don't believe that's necessary for the nested-bypass case (or for
> any of the other use-cases raised), and I don't think it's a good idea to
> implement.

"...it seemed useful that advanced clients (such as a *multi-windowed client*, a OS virtualizer, or a nested mir server) need a way to specify the ordering of the surfaces that they have created."

I'm not insisting on the use-case but it is both plausible and mentioned in the description. (It is also one of the volatile parts of the window management specification - so I'm not yet prepared to ignore it.)

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

> > A multi-window client has no special control over the arrangements of its
> > windows; I don't believe that's necessary for the nested-bypass case (or for
> > any of the other use-cases raised), and I don't think it's a good idea to
> > implement.
>
> "...it seemed useful that advanced clients (such as a *multi-windowed client*,
> a OS virtualizer, or a nested mir server) need a way to specify the ordering
> of the surfaces that they have created."
>
> I'm not insisting on the use-case but it is both plausible and mentioned in
> the description. (It is also one of the volatile parts of the window
> management specification - so I'm not yet prepared to ignore it.)

I believe the fundamental difference between those two use cases is that in the one described here the server is supposed to treat the additional 'overlays' as if they are part of the surface. While in the other the case the additional things shall be treated as if they are not even part of the same session. mir connection/surface proxy vs partially forwarding render buffers. So I believe those shall be handled separately and probably will not interfere each other?

[Chris Halse Rogers (raof)]

On Tue, Apr 7, 2015 at 11:39 PM, Alan Griffiths <email address hidden>
wrote:
> Review: Needs Information
>
>> A multi-window client has no special control over the arrangements
>> of its
>> windows; I don't believe that's necessary for the nested-bypass
>> case (or for
>> any of the other use-cases raised), and I don't think it's a good
>> idea to
>> implement.
>
> "...it seemed useful that advanced clients (such as a *multi-windowed
> client*, a OS virtualizer, or a nested mir server) need a way to
> specify the ordering of the surfaces that they have created."

So, I don't think that a multi-windowed client *should* have the
ability to position its surfaces; as you note, that raises all sorts of
odd behaviour.

For the OS virtualiser case - I presume we're talking about Parallels'
seamless desktop integration - it does *not* need to restack its own
windows. The whole point there is that the proxy surfaces behave like
they're regular windows in the host server.

Now, we certainly need some API for clients to request their surfaces
to be raised, but I don't think this wants to be that API.

[Kevin DuBois (kdub)]

The suggestion to move the number-of-surfaces accessor to surfacespec is a good idea... working on changing the MP/rfc.

[Kevin DuBois (kdub)]

> For the OS virtualiser case - I presume we're talking about Parallels'
> seamless desktop integration - it does *not* need to restack its own
> windows. The whole point there is that the proxy surfaces behave like
> they're regular windows in the host server.
>
> Now, we certainly need some API for clients to request their surfaces
> to be raised, but I don't think this wants to be that API.

Right, these api changes for nested and other clients that would benefit from pass-through don't wade into changing inter-surface stacking order.

[Alan Griffiths (alan-griffiths)]

> I believe the fundamental difference between those two use cases is that in
> the one described here the server is supposed to treat the additional
> 'overlays' as if they are part of the surface. While in the other the case the
> additional things shall be treated as if they are not even part of the same
> session. mir connection/surface proxy vs partially forwarding render buffers.
> So I believe those shall be handled separately and probably will not interfere
> each other?

In that case the description shouldn't mention the use case that isn't addressed.

[Kevin DuBois (kdub)]

[side-comment] I still find this RFC branch useful for discussion, easier to change stubs than an implementation.

Changed the api RFC to need no dynamically-sized client-allocated memory.
In light of lp:~alan-griffiths/mir/first-pass-of-surface-spec-modification, this or that branch will probably needs some changes, as I think the ideas behind 'what is a MirSurfaceSpec' is a bit different between the two branches, and should be unified.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2442
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/254982/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3503/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2027
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2026/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1977/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1500/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1977
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1977/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4911/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19551

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

Other than the surface_spec harmonisation, I think the final thing required here is to determine when the spec is applied. I still think that it needs to be buffered until swap_buffers on the parent surface, which would require a swap_buffers call in the acceptance test.

[Andreas Pokorny (andreas-pokorny)]

> > I believe the fundamental difference between those two use cases is that in
> > the one described here the server is supposed to treat the additional
> > 'overlays' as if they are part of the surface. While in the other the case
> the
> > additional things shall be treated as if they are not even part of the same
> > session. mir connection/surface proxy vs partially forwarding render
> buffers.
> > So I believe those shall be handled separately and probably will not
> interfere
> > each other?
>
> In that case the description shouldn't mention the use case that isn't
> addressed.

+1

[Alan Griffiths (alan-griffiths)]

> [side-comment] I still find this RFC branch useful for discussion, easier to
> change stubs than an implementation.
>
> Changed the api RFC to need no dynamically-sized client-allocated memory.
> In light of lp:~alan-griffiths/mir/first-pass-of-surface-spec-modification,
> this or that branch will probably needs some changes, as I think the ideas
> behind 'what is a MirSurfaceSpec' is a bit different between the two branches,
> and should be unified.

I don't think there are any fundamental incompatibilities between the approaches - in both cases a MirSurfaceSpec is a collection of surface attributes the client asks the server to apply.

In this branch the attribute in question is a collection of buffer streams in the other it the attributes are name, size and height. (There are some existing issues with buffer streams that are likely to cause more churn - like "class Surface : public BufferStream"!)

Or is there a difference in thinking I'm not yet aware of?

[Kevin DuBois (kdub)]

@Alan
I don't think that there's a fundamental difference either, but there is the remaining difference of how the spec is generated... This MP generates the spec from a MirSurface*, and first-pass-of-surface-spec-modification generates from MirConnection*. Generating from the MirSurface creates a spec from the existing surface, and generating from MirConnection seems to imply that its a clean-slate that's generated, filled, and reapplied to the surface.

[Kevin DuBois (kdub)]

@Chris,
Agreed that it needs some sorting...

So, given that "parent surface" == the /stream/ that is generated by default when the surface is created...

For the "glorious future" nested-passthrough scenario, we'll have a certain set of surfaces that we're passing through, and then one surface that's used as the GL surface nested can optionally draw to. We might want to hide the parent surface's stream, and not swap it. I'm still sketching through how synchronizing the swaps+updates might work, but it makes sense to me to somehow delay the swaps of the selected surfaces until a mir_surface_spec_commit_changes() is called.

[Alan Griffiths (alan-griffiths)]

> @Alan
> I don't think that there's a fundamental difference either, but there is the
> remaining difference of how the spec is generated... This MP generates the
> spec from a MirSurface*, and first-pass-of-surface-spec-modification generates
> from MirConnection*. Generating from the MirSurface creates a spec from the
> existing surface, and generating from MirConnection seems to imply that its a
> clean-slate that's generated, filled, and reapplied to the surface.

If there is there a useful scenario for unclean-slate then that can have a corresponding method of generating a MirSurfaceSpec.

[Kevin DuBois (kdub)]

rebased and refactored the acceptance test based on the first-pass-of-surface-spec-modification branch

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2443
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/256186/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3522/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2054
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2053/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2004/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1519/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2004
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2004/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4937/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19617

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The operations of mir_surface_spec_insert_stream_at() and mir_surface_spec_set_buffer_stream_position() seem weird to me.

In mir_surface_spec_insert_stream_at() why don't we specify the x & y coordinates? Surely a new stream needs them? And how are the indexes interpreted if some of the existing streams are reordered (and some not mentioned)?

E.g.

start with {bs1, bs2, bs3} and: insert(bs1 at 23), insert(bs4 at 2) then and apply.
Do we get {bs2, bs4, bs3, bs1} or {bs4, bs2, bs3, bs1}

In mir_surface_spec_set_buffer_stream_position() how are the indexes interpreted? As they are in the surface before applying the spec? Or after? Or depending on the order of calls to mir_surface_spec_insert_stream_at() and mir_surface_spec_set_buffer_stream_position()?

start with {bs1, bs2, bs3} and: position(3) then insert(bs4 at 2) then and apply.
Do we position bz3 or bs2?

~~~~

the description shouldn't mention the use case that isn't addressed.

review: Needs Fixing

[Kevin DuBois (kdub)]

So the idea was to start with a blank MirSurfaceSpec (as its generated from a MirConnection, not a MirSurface), and then the user has to populate the MirSurfaceSpec with the streams it wants.

This is based on the comment about clients maintaining their own surface states here: https://code.launchpad.net/~alan-griffiths/mir/first-pass-of-surface-spec-modification/+merge/255707/comments/636992

[Kevin DuBois (kdub)]

> start with {bs1, bs2, bs3} and: insert(bs1 at 23), insert(bs4 at 2) then and
> apply.
> Do we get {bs2, bs4, bs3, bs1} or {bs4, bs2, bs3, bs1}

So, we start with {}, and following that order we would get
{bs1, bs4}

if we start with {}, then:
insert(bs1, 0) --> {bs1}
insert(bs2, 1) --> {bs1, bs2}
insert(bs3, 2) --> {bs1, bs2, bs3}
insert(bs1 at 23) --> {bs2, bs3, bs1}
insert(bs4 at 2) --> {bs2, bs3, bs4, bs1}

I do appreciate though that the idea needed a fair amount of explaining though, can come up with some other scheme.

[Alan Griffiths (alan-griffiths)]

On 16 Apr 2015 19:49, Kevin DuBois <email address hidden> wrote:
>
> > start with {bs1, bs2, bs3} and: insert(bs1 at 23), insert(bs4 at 2) then and
> > apply.
> > Do we get {bs2, bs4, bs3, bs1} or {bs4, bs2, bs3, bs1}
>
> So, we start with {}, and following that order we would get
> {bs1, bs4}
>

I guess I wasn't clear. I meant that the surface has {bs1, bs2, bs3} already.

And we then create a spec and do the bs1, bs4 inserts and apply to the above surface.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2445
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/256186/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3550/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2091
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2090/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2041/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1547/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2041
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2041/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4975/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19729

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

review: Needs Fixing (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

I guess thats the best way of doing that.

review: Approve

[Alan Griffiths (alan-griffiths)]

The description still describes managing surfaces while the proposal is to manages buffer streams.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

43+/** Reorder the streams associated with the spec.

It isn't "reorder"ing any more. It is "assign"ing.

~~~~

41+void mir_surface_buffer_stream_info_at(MirSurface* surface, unsigned int index, MirBufferStreamInfo*);
...
57+void mir_surface_spec_reorder_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, unsigned int size);

These seem oddly asymmetric. Can't we have

size_t mir_surface_buffer_get_streams(MirSurface* surface, MirBufferStreamInfo* streams, size_t count);

and

void mir_surface_spec_set_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, size_t count);

[Kevin DuBois (kdub)]

> The description still describes managing surfaces while the proposal is to
> manages buffer streams.
Sorry, misinterpreted what was said in the previous requests. Hopefully is fixed now

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2447
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/RFC-surface-arrangements/+merge/256186/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/3566/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2111
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2110/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2061/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1563/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2061
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2061/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4992/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19767

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Makes enough sense for now

review: Abstain

=== modified file 'include/client/mir_toolkit/client_types.h'
--- include/client/mir_toolkit/client_types.h	2015-03-31 02:35:42 +0000
+++ include/client/mir_toolkit/client_types.h	2015-04-17 14:28:26 +0000
@@ -278,6 +278,13 @@
     MirDisplayCard *cards;
 } MirDisplayConfiguration;
 
+typedef struct MirBufferStreamInfo
+{
+    MirBufferStream * stream;
+    int x;
+    int y;
+} MirBufferStreamInfo;
+
 typedef struct MirRectangle
 {
     int left;

=== modified file 'include/client/mir_toolkit/mir_surface.h'
--- include/client/mir_toolkit/mir_surface.h	2015-04-14 11:29:11 +0000
+++ include/client/mir_toolkit/mir_surface.h	2015-04-17 14:28:26 +0000
@@ -643,6 +643,39 @@
  */
 void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec);
 
+/** The number of streams currently associated with the surface.
+ *
+ *  \param [in] surface  The surface of interest
+ *  \return              The number of streams associated with spec
+ */
+unsigned int mir_surface_num_streams(MirSurface* surface);
+
+/**
+ * Retreive the buffer stream at the index specified
+ *  \param [in]  surface  The surface of interest
+ *  \param [in]  index    The index of the buffer stream info in the surface.
+ *                        An index of 0 is the bottom-most surface; and index of
+ *                        (mir_surface_num_streams() - 1) is the top-most surface.
+ *  \param [out] info     The info for buffer stream at the index.
+ */ 
+void mir_surface_buffer_stream_info_at(MirSurface* surface, unsigned int index, MirBufferStreamInfo*);
+
+/** 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.
+ *
+ *  \warning no entry of streams may be null
+ *  \warning disassociating streams from the surface will not release() them.
+ *
+ *  \param [in] spec      The spec to accumulate the request in.
+ *  \param [in] streams   The an array of streams info.
+ *  \param [in] size      The size of streams.
+ */
+void mir_surface_spec_set_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, unsigned int size);
+
 #ifdef __cplusplus
 }
 /**@}*/

=== modified file 'src/client/mir_surface_api.cpp'
--- src/client/mir_surface_api.cpp	2015-04-16 08:32:47 +0000
+++ src/client/mir_surface_api.cpp	2015-04-17 14:28:26 +0000
@@ -632,3 +632,23 @@
     MIR_LOG_UNCAUGHT_EXCEPTION(ex);
     // Keep calm and carry on
 }
+
+void mir_surface_spec_set_streams(MirSurfaceSpec* spec, MirBufferStreamInfo* streams, unsigned int size)
+{
+    (void) spec;
+    (void) streams;
+    (void) size;
+}
+
+unsigned int mir_surface_num_streams(MirSurface* surface)
+{
+    (void) surface;
+    return 1;
+}
+
+void mir_surface_buffer_stream_info_at(MirSurface* surface, unsigned int index, MirBufferStreamInfo* info)
+{
+    (void) surface;
+    (void) index;
+    (void) info;
+}

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2015-04-09 06:20:31 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2015-04-17 14:28:26 +0000
@@ -30,6 +30,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-04-17 14:28:26 +0000
@@ -0,0 +1,263 @@
+/*
+ * 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 <[e-mail]>
+ */
+
+#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 <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}
+    {
+    }
+
+    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 const& sequence)
+    {
+        std::unique_lock<decltype(mutex)> lk(mutex);
+        rectangles.clear();
+        for(auto const& element : sequence)
+            rectangles.emplace_back(element->renderable()->screen_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);});
+    }
+
+    bool ensure_last_ordering_is_consistent_with(
+        std::vector<geom::Point> const& arrangement)
+    {
+        if (rectangles.size() != arrangement.size())
+            return false;
+        for(auto i = 0u; i < rectangles.size(); i++)
+        {
+            if (rectangles[i].top_left != arrangement[i])
+                return false;
+        }
+        return true;
+    }
+private:
+    std::mutex mutex;
+    std::condition_variable cv;
+    unsigned int post_count{0};
+    std::vector<geom::Rectangle> rectangles;
+};
+
+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();
+
+        primary_stream = std::unique_ptr<Stream>(
+            new 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::unique_ptr<Stream>(
+                new 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::unique_ptr<Stream> primary_stream;
+    std::vector<std::unique_ptr<Stream>> streams;
+};
+}
+
+TEST_F(BufferStreamArrangement, arrangements_are_applied)
+{
+    using namespace testing;
+    auto num_streams = streams.size() + 1;
+    std::vector<MirBufferStreamInfo> info(num_streams);
+    auto i = 0u;
+    info[i++] = MirBufferStreamInfo{
+        primary_stream->handle(),
+        primary_stream->position().x.as_int(),
+        primary_stream->position().y.as_int()};
+    for(auto &stream : streams)
+    {
+        info[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, info.data(), info.size());
+    mir_surface_apply_spec(surface, change_spec);
+    mir_surface_spec_release(change_spec);
+
+    num_streams = mir_surface_num_streams(surface);
+    EXPECT_THAT(num_streams, Eq(streams.size() + 1));
+
+    std::vector<geom::Point> points;
+    for(auto i = 0u; i < num_streams; i++)
+    {
+        MirBufferStreamInfo info{nullptr, 0, 0};
+        mir_surface_buffer_stream_info_at(surface, i, &info);
+        points.emplace_back(geom::Point{info.x, info.y});
+        if (i == 0)
+        {
+            EXPECT_THAT(info.stream, Eq(primary_stream->handle()));
+            EXPECT_THAT(points.back(), Eq(primary_stream->position()));
+        }
+        else
+        {
+            EXPECT_THAT(info.stream, Eq(streams[i-1]->handle()));
+            EXPECT_THAT(points.back(), Eq(streams[i-1]->position()));
+        }
+    } 
+
+    //check that the compositor rendered correctly
+    using namespace std::literals::chrono_literals;
+    EXPECT_TRUE(ordering->wait_for_another_post_within(1s))
+         << "timed out waiting for another post";
+    EXPECT_TRUE(ordering->ensure_last_ordering_is_consistent_with(points))
+         << "surface ordering was not consistent with the client request";
+}