Merge lp:~kdub/mir/add-streams-to-surface into lp:mir
| Status: | Merged |
|---|---|
| Approved by: | Daniel van Vugt on 2015-06-05 |
| Approved revision: | 2575 |
| Merged at revision: | 2629 |
| Proposed branch: | lp:~kdub/mir/add-streams-to-surface |
| Merge into: | lp:mir |
| Diff against target: |
924 lines (+426/-109) 10 files modified
include/server/mir/scene/surface.h (+11/-2) src/server/scene/basic_surface.cpp (+50/-21) src/server/scene/basic_surface.h (+4/-1) src/server/scene/surface_stack.cpp (+13/-8) tests/acceptance-tests/test_surfaces_with_output_id.cpp (+6/-1) tests/include/mir_test_doubles/stub_scene_surface.h (+2/-1) tests/integration-tests/surface_composition.cpp (+1/-1) tests/unit-tests/scene/test_basic_surface.cpp (+252/-31) tests/unit-tests/scene/test_surface.cpp (+6/-2) tests/unit-tests/scene/test_surface_stack.cpp (+81/-41) |
| To merge this branch: | bzr merge lp:~kdub/mir/add-streams-to-surface |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel van Vugt | 2015-05-21 | Abstain on 2015-06-05 | |
| PS Jenkins bot | continuous-integration | Approve on 2015-06-04 | |
| Alberto Aguirre | Approve on 2015-06-03 | ||
| Alexandros Frantzis (community) | Needs Fixing on 2015-06-02 | ||
| Chris Halse Rogers | Approve on 2015-06-01 | ||
| Cemil Azizoglu (community) | Approve on 2015-05-28 | ||
| Robert Carr (community) | Approve on 2015-05-27 | ||
| Alan Griffiths | Approve on 2015-05-27 | ||
|
Review via email:
|
|||
Commit Message
provide a way to add multiple streams to ms::Surface. ms::Surface now generates a list-of-renderables instead of a single renderable.
Description of the Change
provide a way to add multiple streams to ms::Surface. ms::Surface now generates a list-of-renderables instead of a single renderable.
Most of the 'primary buffer stream' concept is rooted out, although I address the surface-
| Daniel van Vugt (vanvugt) wrote : | # |
Architecturally I find this confusing. When does a surface have multiple buffer streams?... As opposed to sub-surfaces? Or is this a replacement for the sub-surface concept?
| Kevin DuBois (kdub) wrote : | # |
It has multiple buffer streams when the client adds them. Also, the server (via the window manager) would be able to add additional server-side buffer streams to the surface (in the same sort of way that the demo-shell canonical window manager does)
It is mostly useful in the case where a client has on Surface (in the concept of the "spec document"), but finds it convenient to have multiple streams/multiple contexts that are composed together by the servers compositor. (eg, the nested server passthrough, video, and things that do some sort of client-side decoration, maybe browsers).
Its a bit clearer to think about ms::Surface as "Window" (with inputs, and relation to other "Windows" per the surface spec), and the mc::BufferStreams as "Surfaces", but our renaming hasn't caught up yet.
| Alan Griffiths (alan-griffiths) wrote : | # |
23 +struct StreamInfo
24 +{
25 + std::shared_
26 + geometry:
27 +};
I'm still thinking about whether the position should be an attribute of the stream.
| Kevin DuBois (kdub) wrote : | # |
> 23 +struct StreamInfo
> 24 +{
> 25 + std::shared_
> 26 + geometry:
> 27 +};
>
> I'm still thinking about whether the position should be an attribute of the
> stream.
Could this be because I (poorly) named the displacement "position"?
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2567
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
23 +struct StreamInfo
24 +{
25 + std::shared_
26 + geometry:
27 +};
I'm still thinking about whether the displacement should be an attribute of the stream.
| Daniel van Vugt (vanvugt) wrote : | # |
I'm not sure this design is sensible, as demonstrated in:
23 +struct StreamInfo
24 +{
25 + std::shared_
26 + geometry:
27 +};
"displacement" is not enough for non-trivial cases because most streams won't be the primary client area so won't respond to resize() in the same way as the client stream.
Rather than specifying displacement I think a generic rectangle (size and displacement) makes more sense. Also each stream needs some indicator of its purpose so it gets handled differently during resizing. e.g. the client area matches the window dimensions but a titlebar's height never changes. Also the titlebar stream is ideally sized in _physical_ units (mir::geometry:
It looks like we're re-inventing the sub-surface concept in a less elegant way here and it's going to rapidly become very painful to manage. Better would be to relate surfaces together (as planned already) rather than introduce two completely different parent/
| Daniel van Vugt (vanvugt) wrote : | # |
We should also continue refining the Surface classes so that BasicSurface becomes truly basic. Then there will be very little difference between BufferStream and a BasicSurface and it's less tempting to make BasicSurface more complex.
| Alan Griffiths (alan-griffiths) wrote : | # |
> Rather than specifying displacement I think a generic rectangle (size and
> displacement) makes more sense. Also each stream needs some indicator of its
> purpose so it gets handled differently during resizing...
That is another step down the slope towards a generic layout manager. We either do the minimum that could possibly work or have to consider the existing solution domain of layout managers (allowing the specification of anchors, insets, fills, weights, flow, etc.).
I thought we'd already agreed to start with the minimal support needed for nested and get that landed - and that's what this MP does.
| Kevin DuBois (kdub) wrote : | # |
I had hoped the disapprovals and suggestions for sweeping improvements would have come in the first branch in this line of work (back here: https:/
So for resize, this is a snag that's left to work out before the work is completed. A position+size probably makes sense as a per-stream attribute.
A stream's purpose is to show content on the screen. The server doesn't know about the relations between the client streams in the same surface on purpose, this is for the client to work out the best way to present the content on the Surface. Multiple streams aren't "subsurfaces" (although admittedly I don't remember that conversation in detail); they are ways for a surface to present more than one stream in their surface out of convenience or necessity.
The relationships between surfaces are captured in the spec document. The idea here is that the client can arrange its own drawing.
| Kevin DuBois (kdub) wrote : | # |
> We should also continue refining the Surface classes so that BasicSurface
> becomes truly basic. Then there will be very little difference between
> BufferStream and a BasicSurface and it's less tempting to make BasicSurface
> more complex.
Separating the Surface/
| Kevin DuBois (kdub) wrote : | # |
@Alan, right, the primary goal here is to support the plan for nested passthrough. I hope that I didn't muddy the waters too much by talking about other potential (future) uses of allowing one surface to have multiple drawing contexts.
| Cemil Azizoglu (cemil-azizoglu) wrote : | # |
Nested passthrough is (obviously) very important future to have. And we want to get there as fast as we can. I'm ok concentrating on that use-case and letting this through even if things have to change (slightly) in the future for other use-cases (as we agreed in Dallas, we should prevent paralysis - this is software and we can always change it). As Kevin mentioned there was already agreement on the RFC so changes should not be all that drastic.
Kevin, perhaps 'position' can be added (hopefully it'll suffice to remove the 'Disapprove').
| Daniel van Vugt (vanvugt) wrote : | # |
The resizing and placement issue is a big one. We don't want Mir to be a generic layout manager, so position should go away. Once it goes away you have an immediate need for the shell to provide the placement logic instead (e.g. a titlebar if displayed at all is usually at the top with fixed height and variable width). If you provide so much as a "displacement" you also have to provide all the logic describing what gets painted between and around the buffer streams, how they're blended etc. So not a problem Mir should solve in its core.
Using the word "paralysis" is an excuse for making short term progress at the expense of Mir's longer term. Sometimes large charges are too large and interwoven to be feasibly undone. So you need to catch architectural issues early and stop them taking hold.
"Needs fixing" in the least. Mir should never express any layout information for the inter-stream structure of a window ("displacement"). That's the shell's job. The shell defines how streams are combined. Mir's default should be just to display the client area / main stream.
(footnotes)
I again have to remind myself that when you say "surface" it means window. And what we call "BufferStream" is what was originally a simple surface. Transpose the terminology mentally and it's OK, but I hope we can change it in reality eventually too.
From a high-level perspective this feels dirty to me. We're adding piles of complexity to implement nested bypass which itself may only be a short-term problem. Today it would save us two frames, tomorrow after 'ddouble' lands nested bypass will only save us one frame. In the longer term if people build other shells with a focus on performance and power management then nesting is likely to be skipped completely. So the value of solving this problem and justification in adding complexity is shrinking.
| Daniel van Vugt (vanvugt) wrote : | # |
"displacement" should be replaced by an enum of the stream purpose/type for the shell to interpret as it sees fit:
enum StreamType {client, titlebar, icon, ...}
Then your collection of streams is just a map between type and stream:
std:
So primarily you will do most things with "stream[client]". The other types are optional.
| Kevin DuBois (kdub) wrote : | # |
> The resizing and placement issue is a big one. We don't want Mir to be a
> generic layout manager, so position should go away. Once it goes away you have
> an immediate need for the shell to provide the placement logic instead (e.g. a
> titlebar if displayed at all is usually at the top with fixed height and
> variable width). If you provide so much as a "displacement" you also have to
> provide all the logic describing what gets painted between and around the
> buffer streams, how they're blended etc. So not a problem Mir should solve in
> its core.
I agree that the resizing and placement is a big issue; and having streams within a surface without absolute coordinates allows the client to have a guarantee that its contexts will be composited in a certain way, and allows the server to still manage the windowing (ms::Surface) positions.
If the client is to offload simple compositing work, it needs to guarantee that its multiple graphics contexts (streams) get painted within its Surface in a certain way. It took a bit of tinkering to get to this point, but displacement (as opposed to an absolute position) keeps the window management contained, as the positioning is always relative to the decision of the window manager. The structure of the buffer streams is more of an internal way that the client would like to offload work to the server; as opposed to influencing window management policy.
Ideally, the shell could crop the surfaces that overhang, but in the meantime (in the upcoming branches), the window managers have the option to just reject requests that exceed the bounds of the surface. Crop-related tasks are in the backlog and would mean less rejected relative placement requests.
So, part of this line-of-MPs goal is explicitly NOT to give the clients a backdoor that they can use to breech the WM policy. (which hopefully addresses the architectural concerns)
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2569
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Chris Halse Rogers (raof) wrote : | # |
69 + for(auto info : streams)
70 + visible |= info.stream-
I don't think this is the right logic; a surface still has a “main” buffer stream, and I think that should determine visibility. (And, later, should gate a whole bunch of state changes).
178 + return std::move(list);
Pessimization: this breaks copy elision. Clang 3.7 generates a warning for this.
Needs fixing for the visibility calculation.
| Chris Halse Rogers (raof) wrote : | # |
Additionally: I'm not sure what's difficult about resize. With the current behaviour when resizing clients the subsurfaces will jump around a bit because there's no synchronisation for those updates, but this synchronisation is a conceptually simple problem to solve that we've deliberately punted on for the first iteration.
Of course, that's a lie; *currently* there's no way for a client to currently specify >1 buffer stream for its surface ☺.
Hm. Now that I've reread the RFC branches your visibility calculation is correct for some use-cases. I don't like that, but I don't have a better suggestion at the moment.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2570
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel van Vugt (vanvugt) wrote : | # |
(0) If you think back to Brussels we realized there are three modes of decoration and the two dominant modes are not the one being addressed here (not that they need to be):
* "Client" decorates itself (e.g. GTK 3)
* "Server" decorates clients (using simple data like a title string and button mask, e.g. X window managers and Microsoft Windows).
* "Hybrid": Server combines multiple client surface/streams into a single window (e.g. menu blended into the titlebar). Server still needs to add some decoration around the combined streams (e.g. titlebar background/
This proposal addresses the hybrid mode. And while it is a real use case I want to make sure we don't go designing something that gets too much in the way of the pure client or server modes.
(1) X apps (and possibly others) would like to have a stream/image for storing their icons. In fact it's the only kind of secondary image/stream that actually exists as a real use case already, so we should try to cover that one. It needs to support streams that do not have any displacement relative to the surface. How should we represent such a null displacement? Just add a type enum and ignore displacement for certain types?
| Chris Halse Rogers (raof) wrote : | # |
You're thinking of a fundamentally different problem. This isn't about
decoration at all, and this interface won't be used to provide our
funky decoration options.
This is about a client delegating some of it's *internal* compositing
needs to Mir. The motivating use-case is nested-bypass (where the
nested server delegates its compositing to the host Mir for simple
cases). This sort of interface is also necessary for proper use of
hardware overlays by clients.
This is why the window management code doesn't have, or need, any
influence on the client decisions here - this is purely an optimisation
for something the app could have done itself.
| Daniel van Vugt (vanvugt) wrote : | # |
You mean this is for a video overlay (YUV) sitting above some client-rendered controls (RGB)? That sounds reasonable then.
But if we're not thinking about decorations yet then we should. We don't want to re-invent the wheel and an entirely separate API when what's proposed here is roughly what we've been talking about for hybrid decorations all along.
I guess what I'm asking for is a future enhancement to this then. And displacement is all we care for right now.
| Daniel van Vugt (vanvugt) wrote : | # |
(2) Just noticed the same visible() regression Chris pointed out:
65 bool ms::BasicSurfac
66 {
67 - return !hidden && surface_
68 + bool visible{false};
69 + for(auto info : streams)
70 + visible |= info.stream-
71 + return !hidden && visible;
72 }
This allows a surface to get rendered even if the client hasn't painted its main layer yet. Sounds like a user-visible bug waiting to happen.
I think the right answer is that a surface only becomes visible when all of its layers are ready:
bool visible = true;
for (auto s : streams)
{
if (!s->has_
{
visible = false;
break;
}
}
P.S. Did anyone suggest the word "layer" yet, as in "BasicSurface:
| Alexandros Frantzis (afrantzis) wrote : | # |
69+ for(auto info : streams)
140+ for(auto info : streams)
... and elsewhere
No need to copy, 'for (auto const& info : streams)'.
Formatting nits:
55+ if(
248+ for(
... and elsewhere
Space before opening parenthesis.
> This isn't about decoration at all, and this interface won't be used to provide our
> funky decoration options.
I was under the impression that the buffer stream mechanism could eventually be used for some decoration use cases (but I haven't followed the discussions closely).
> I think the right answer is that a surface only becomes visible when all of its layers are ready:
It really depends on the use case. For the nested bypass use case, where only one buffer stream (i.e. one client surface of the nested server) should be displayed, I would say that the surface can be considered visible if the "active" buffer stream has a valid buffer. I guess what Kevin is proposing will also work, as long as the nested server is smart enough not to "activate" a client buffer stream that doesn't have a valid buffer.
| Kevin DuBois (kdub) wrote : | # |
> You mean this is for a video overlay (YUV) sitting above some client-rendered
> controls (RGB)? That sounds reasonable then.
>
Right, this is one compelling use. The one I'm presently interested in is a nested server handing off its clients buffers to the host, and the host using bypass/overlay to composite using those methods on behalf of the nested server.
> But if we're not thinking about decorations yet then we should. We don't want
> to re-invent the wheel and an entirely separate API when what's proposed here
> is roughly what we've been talking about for hybrid decorations all along.
>
> I guess what I'm asking for is a future enhancement to this then. And
> displacement is all we care for right now.
Right, I agree that this is a good avenue to having hybrid decorations. I think its reasonable that this can be expanded in certain ways that help out decorations, but its a bit off-topic of the immediate goal of helping the client out with its composition.
| Kevin DuBois (kdub) wrote : | # |
@Visibility topic
If the user is specifying its surfaces, it doesn't make sense to assume that the created-with stream is somehow more important than the other surfaces. 'main stream' is a concept that I'm trying to root out (just one or two places to root this out).
So, thinking from the a nested composition topic, it doesn't make sense for the surface to become invisible briefly because the nested server adds an additional stream to its surface. Like, if the nested server has 3 clients, and adds a 4th, which takes a long time to gets its first frame out, the 3 existing streams should be shown in the host server while we wait for all the streams to be visible. The plan is to count on the nested server to be smart enough... we're giving it more power, so it has to have more responsibility. We might also have to give it better synchronization guarantees, but even having multiple buffer streams seems the first bridge to cross.
| Kevin DuBois (kdub) wrote : | # |
> I was under the impression that the buffer stream mechanism could eventually
> be used for some decoration use cases (but I haven't followed the discussions
> closely).
I think that Daniel and I see that this could be the mechanism to build on for client-side decorations, but for the nested-passthrough topic (or the video player, other more advanced compositors), its not necessary to build this idea up now.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2573
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2573
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
| Daniel van Vugt (vanvugt) wrote : | # |
(2) Hmm, no I think BasicSurface:
If streams are stored in the stacking order then "visible" must gate on the primary (background) stream being visible, and only the primary stream. Otherwise your window could have transparent holes in it (e.g. video playing but your browser window isn't rendered yet).
(3) Terminology: "layer" as the variable name I think would explain the intent better than "streams".
- 2574. By Kevin DuBois on 2015-06-04
-
merge in mir
- 2575. By Kevin DuBois on 2015-06-04
-
use layers instead of streams in BasicSurface member variable
| Kevin DuBois (kdub) wrote : | # |
1) With the current setup, the client can arrange the alpha channel to have transparent holes, too. We have to rely on the client to assemble the content that it wants within its surface region (even with only the created-with stream, we have to rely on the client).
As Chris has pointed out here and there (there being the RFC branch for the client api), some sort of 'rearrange and swap all at once' will probably be needed to have polished stream changes. In the meantime though, just going for the first cut to just have the swaps and the arrangements be 'async'
There is still a concept of a 'primary' buffer stream (or layer), but this is vestigial, and is being removed in the branches stacked upon this one. The buffer stream that is automatically created with the surface doesn't have to have a topmost or bottommost position in the layering.
2) Changed to use 'layers' as the name.
| Kevin DuBois (kdub) wrote : | # |
1) With the current setup, the client can arrange the alpha channel to have transparent holes. We have to rely on the client to assemble the content that it wants within its surface region (even with only the created-with stream, we have to rely on the client).
There is still a concept of a 'primary' buffer stream (or layer), but this is vestigial, and is being removed in the branches stacked upon this one. The buffer stream that is automatically created with the surface doesn't have to have a topmost or bottommost position in the layering.
2) Changed to use 'layers' as the name.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2575
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel van Vugt (vanvugt) wrote : | # |
OK, my concerns have been aired sufficiently. And I can see the last remaining "Needs Fixing" was fixed already. Top approving...
PASSED: Continuous integration, rev:2566
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://