Merge into development-branch : no-SurfaceBufferAccess : Code : Mir

Status:	Work in progress
Proposed branch:	lp:~vanvugt/mir/no-SurfaceBufferAccess
Merge into:	lp:mir
Prerequisite:	lp:~vanvugt/mir/fix-1376324-more
Diff against target:	515 lines (+49/-138) 16 files modified include/server/mir/scene/surface.h (+8/-3) include/server/mir/scene/surface_buffer_access.h (+0/-50) server-ABI-sha1sums (+1/-2) src/server/scene/application_session.cpp (+1/-1) src/server/scene/basic_surface.cpp (+1/-9) src/server/scene/basic_surface.h (+1/-4) src/server/scene/snapshot_strategy.h (+3/-2) src/server/scene/threaded_snapshot_strategy.cpp (+9/-20) src/server/scene/threaded_snapshot_strategy.h (+1/-1) src/server/symbols.map (+0/-7) tests/include/mir_test_doubles/mock_surface.h (+2/-0) tests/include/mir_test_doubles/null_snapshot_strategy.h (+1/-1) tests/include/mir_test_doubles/stub_scene_surface.h (+2/-1) tests/unit-tests/scene/test_application_session.cpp (+8/-5) tests/unit-tests/scene/test_surface_impl.cpp (+3/-10) tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp (+8/-22)
To merge this branch:	bzr merge lp:~vanvugt/mir/no-SurfaceBufferAccess
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-10-15	Approve on 2014-12-01
Alan Griffiths		2014-10-15	Abstain on 2014-11-25
Kevin DuBois (community)			Needs Fixing on 2014-11-18
Cemil Azizoglu (community)		2014-10-15	Disapprove on 2014-11-14
Alberto Aguirre (community)		2014-10-15	Disapprove on 2014-10-31
Mir development team		2014-11-27	Pending
Review via email: mp+238376@code.launchpad.net

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-10-22:

#

This changes the way ownership of the snapshot buffer is managed. The existing code ensures that ownership is released after the snapshot is taken by not passing it to the snapshot strategy.

In this MP instead of being retained in the scene code ownership is passed to the snapshot strategy. This approach is unnecessarily fragile - as we need to remember this requirement in any future code MPs.

review: Disapprove

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-10-22:

#

You propose to leak information to the wider world that it's unsafe to own a Buffer without also owning the corresponding surface? I think we can do better than that.

Safety of a callback should simply be ensured either by the callee or the caller. Not a third party as you propose, because that leaks unnecessary information to the wider world about how safe it is to hold a reference to a Buffer. It should simply always be safe.

Reply

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-10-31:

#

> You propose to leak information to the wider world that it's unsafe to own a
> Buffer without also owning the corresponding surface? I think we can do better
> than that.
>
> Safety of a callback should simply be ensured either by the callee or the
> caller. Not a third party as you propose, because that leaks unnecessary
> information to the wider world about how safe it is to hold a reference to a
> Buffer. It should simply always be safe.

Owning a buffer and taking a snapshot of it without a corresponding surface doesn't make sense to me.

It's much easier to reason about lifetime concerns if the surface is passed instead.

Reply

Revision history for this message

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

#

review: Disapprove

Reply

Revision history for this message

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

#

A buffer is just a buffer. You don't and shouldn't need a Surface to reason about a Buffer. We have explicitly designed all our classes with this in mind. Isolation, high cohesion, low coupling. That's how we have testable classes.

Last I checked these were basic principles of software engineering. I'm not quite sure why people are still opposed to the clean up. Although as an alternative, if we remove the downstream code that uses this feature then it can simply be deleted instead of simplified...

Reply

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-03:

#

> A buffer is just a buffer. You don't and shouldn't need a Surface to reason
> about a Buffer. We have explicitly designed all our classes with this in mind.
> Isolation, high cohesion, low coupling. That's how we have testable classes.
>
> Last I checked these were basic principles of software engineering. I'm not
> quite sure why people are still opposed to the clean up. Although as an
> alternative, if we remove the downstream code that uses this feature then it
> can simply be deleted instead of simplified...

Oh but you do. The snapshot request is associated with a surface after all not a buffer.

Handing out a shared_ptr to a buffer from a surface makes you wonder some of these questions:

1. Why would you need to snapshot a buffer if the surface has gone away?
2. What happens if I still hold the buffer and the surface goes away? Why do I care about the buffer then? Is it safe?
3. If I hold a buffer do I implicitly hold the surface too?

With SurfaceBufferAccess I don't have to ask those questions which is why I oppose removing it.

Also why would you want to delete a feature that it's in current use from downstream? That doesn't make any sense.

Reply

Revision history for this message

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

#

Those are simple questions:

1. Not an important question either way. You can argue the higher level API requires all snapshot operations complete eventually. The decision of whether a surface "going away" results in either (a) a stale snapshot, (b) a black snapshot, or (c) no snapshot completion is not important. We can implement any of those and I suspect our existing logic doesn't really care which.

2. See #1 above. You might care about the buffer still so as to guarantee that a snapshot command always returns some pixels, which might be visibly important for your GUI. On the other hand, it's totally unimportant -- the delay between scheduling a snapshot and it starting is one context switch. In the unlikely event that there's no surface any more when the snapshot happens, present behaviour is to still return the latest frame from before the surface died. Which is OK, but also not the only option.

3. No, holding a buffer does not imply anything about the lifetime of a surface. What we have designed around is the guarantee that a buffer's contents do not change while you hold it. That remains true.

So they're not important issues and if they were then the behaviour is easy to change according to desire.

Q: Also why would you want to delete a feature that it's in current use from downstream? That doesn't make any sense.
A: I don't think it is in much "current use" down stream. The introduction of qtmir means we use live textures and don't need snapshotting for the switcher any more. But even if there is some remaining downstream use for snapshotting, we can still work to simplify the upstream implementation in Mir for the sake of future maintainability, as proposed here.

It's also worth noting that snapshot() and compositor_snapshot() have the same basic purpose and only differ in where the pixels go. The latter used for compositing is already designed around having a Buffer without holding a reference to the surface. So I'm not proposing anything new here. Just a simplification which will help to speed up future work. The benefits are exponential.

Those are simple questions:

1. Not an important question either way. You can argue the higher level API requires all snapshot operations complete eventually. The decision of whether a surface "going away" results in either (a) a stale snapshot, (b) a black snapshot, or (c) no snapshot completion is not important. We can implement any of those and I suspect our existing logic doesn't really care which.

2. See #1 above. You might care about the buffer still so as to guarantee that a snapshot command always returns some pixels, which might be visibly important for your GUI. On the other hand, it's totally unimportant -- the delay between scheduling a snapshot and it starting is one context switch. In the unlikely event that there's no surface any more when the snapshot happens, present behaviour is to still return the latest frame from before the surface died. Which is OK, but also not the only option.

3. No, holding a buffer does not imply anything about the lifetime of a surface. What we have designed around is the guarantee that a buffer's contents do not change while you hold it. That remains true.

So they're not important issues and if they were then the behaviour is easy to change according to desire.

Q: Also why would you want to delete a feature that it's in current use from downstream? That doesn't make any sense.
A: I don't think it is in much "current use" down stream. The introduction of qtmir means we use live textures and don't need snapshotting for the switcher any more. But even if there is some remaining downstream use for snapshotting, we can still work to simplify the upstream implementation in Mir for the sake of future maintainability, as proposed here.

It's also worth noting that snapshot() and compositor_snapshot() have the same basic purpose and only differ in where the pixels go. The latter used for compositing is already designed around having a Buffer without holding a reference to the surface. So I'm not proposing anything new here. Just a simplification which will help to speed up future work. The benefits are exponential.

Reply

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-05:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-14:

#

If there is no associated/coupled surface, then how do we even know that the buffer holds pixel data? Once you decouple them then a buffer is nothing but some memory area, we can't/shouldn't make any assumptions about it being an image.

review: Disapprove

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-17:

#

We guarantee it in BufferQueue via the lifetime of a "snapshot" acquired buffer.

It's completely safe, and designed that way.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-17:

#

See:

std::shared_ptr<mg::Buffer> mc::BufferStreamSurfaces::lock_snapshot_buffer()
{
return std::make_shared<mc::TemporarySnapshotBuffer>(buffer_bundle);
}

and

    /* Don't give to the client just yet if there's a pending snapshot */
    if (!resize_buffer && contains(buffer, pending_snapshots))
    {
        snapshot_released.wait(lock,
            [&]{ return !contains(buffer, pending_snapshots); });
    }

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-17:

#

All the above Disapprove comments have now been dis-proven so please re-review.

Not only is this proposal safe, but a very handy architectural simplification...

Reply

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-11-18:

#

It does seem strange that ms::Surface has:
std::shared_ptr<graphics::Buffer> snapshot_buffer() const;
and
void ms::BasicSurface::with_most_recent_buffer_do(std::function<void(mg::Buffer&)> const& exec)

with_most_recent_buffer_do() seems like it avoids ownership questions a bit better than handing out a shared_ptr, so I'd favor removing snapshot_buffer() from ms::Surface (snapshot_buffer should be private or removed)

review: Needs Fixing

Reply

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-11-18:

#

(my 2 cents on cleanup-strategy)
It is a good point that ms::Surface is a big gob of functions that needs some refinement. I guess I'd rather have input::Surface, frontend::Surface, and scene::SurfaceBufferAccess remain distinct then subsume them all into ms::Surface because at least those 3 distinctions give us some hints at what various parts of the system want in order to function.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-19:

#

with_most_recent_buffer_do() actually does not provide any useful ownership advantage.

Buffer exclusivity is of course a non-issue because we've designed it to be safe.

The only remaining issue (arguably) is whether it matters what happens if a surface is destroyed at the same time as a snapshot's requested:

  schedule snapshot
  snapshot completes
  surface destroyed
  snapshot called back

The fact that the last two steps can now occur (safely) in any order is the point of contention. I don't think it needs to be though, because our users don't care which order they occurred in during that brief period of time. There's no logical requirement that the surface still exists after the snapshot is returned, only that the buffer existed long enough for the snapshot to get correct pixels. And that requirement is still guaranteed here.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-19:

#

Also, _if_ the user did have some requirement that the surface still exists after the snapshot callback then they already trivially guarantee it for themselves: Hold a shared_ptr to the surface for as long as you need it.

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-20:

#

As a general strategy, making code depend on wide interfaces instead of narrow ones increases the coupling in the code. If this were changing a dependency from SurfaceBufferAccess to Surface then this would be an argument against the MP. (That is, the implication of several comments above that having fewer, wider interfaces reduces coupling is just wrong.)

In this case, however, change is from a dependency on SurfaceBufferAccess to Buffer and is probably a simplification.

However, I prefer the pre-existing approach to resource management (the ExecuteAroundMethod design pattern). That is passing a resource to the code that uses it from the code responsible from the lifetime of the resource. It is, of course, possible to use smart pointers or other tools for management, but this doesn't mean this is the best approach.

We're certainly not consistent in this area of the code (and buffer ownership does e.g. need to be passed out to clients) but where there is no need to share the ownership I'm against doing so.

Reply

lp:~vanvugt/mir/no-SurfaceBufferAccess updated on 2014-11-21

1993. By Daniel van Vugt on 2014-11-17: Merge latest trunk
1994. By Daniel van Vugt on 2014-11-18: Merge latest trunk
1995. By Daniel van Vugt on 2014-11-19: Merge latest trunk
1996. By Daniel van Vugt on 2014-11-20: Merge latest trunk
1997. By Daniel van Vugt on 2014-11-21: Merge latest trunk and fix conflicts.
1998. By Daniel van Vugt on 2014-11-21: Shrink the diff

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-21:

#

I recognize that you prefer the "execute around" pattern. Unfortunately the current approach executes around in a way that's not obvious or guaranteed in the public API.

If you look at the public API, there is no indication or guarantee of surface lifetime because the surface is implied/hidden:
Session::take_snapshot(SnapshotCallback const& snapshot_taken)

The "execute around" approach while a valid stylistic preference is not communicated in the public side of the API, and also less important than the class hierarchy simplification we get with this branch.

The "execute around" approach is also unimportant internally, as we already have safeguards to ensure a snapshot completes regardless of surface lifetime.

The future maintenance benefit of a simplification (code reduction) like this I think is more important than a minor style preference.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-21:

#

"not communicated in the public side of the API" because the callback is actually asynchronous, which we do not document in mir/scene/session.h

Also note: The one and only user of this function I have ever found uses it synchronously - waiting for the result immediately. So we can easily remove all the threading and even the need for callbacks.

Reply

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-21:

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-24:

#

My reasons for preferring the ExecuteAround pattern isn't a "minor stylistic preference". My experience (over decades) and that of other pundits is that it leads to a better separation of concerns and more maintainable code.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-25:

#

That's quite reasonable as an end goal, but I disagree with your means of achieving it.

Similarly, I argue that lower coupling and simpler code leads to "better separation of concerns and more maintainable code".

I'm not sure anyone could reasonably argue that this branch does not succeed in reducing coupling and simplifying the code. The only question is, do you want to ignore that in favour of a different style.

Reply

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-25:

#

The impact of this change is insignificant compared to the impact of this discussion.

review: Abstain

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-26:

#

Agreed. And I don't intentionally seek to use up peoples' time. Although I didn't expect this branch to be controversial.

What I don't want is this to become a branch that is rejected on non-technical grounds of simply creating too much discussion. If there is a logical reason why this simplification is not beneficial (or less beneficial than not landing it) then I'm all ears.

Reply

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-27:

#

The technical aspects and logical reasons have already been discussed on previous comments. It's fine if you disagree, but it seems the majority of the team does not agree with this change.

Reply

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-27:

#

I am disappointed. Although still waiting for more votes, quietly confident that a larger sample size will yield more approvals.

It's really important that we push for simplifications where they can be made. Complexity scales exponentially with code size and so does maintenance burden. So it's in everyone's interest to strive to simplify where possible.

Reply

lp:~vanvugt/mir/no-SurfaceBufferAccess updated on 2014-11-28

1999. By Daniel van Vugt on 2014-11-24: Merge latest trunk
2000. By Daniel van Vugt on 2014-11-25: Merge latest trunk
2001. By Daniel van Vugt on 2014-11-26: Merge latest trunk
2002. By Daniel van Vugt on 2014-11-27: Merge latest trunk and fix a conflict
2003. By Daniel van Vugt on 2014-11-28: Merge latest trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-29:

#

FAILED: Continuous integration, rev:2003
http://jenkins.qa.ubuntu.com/job/mir-ci/2284/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/380
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/380
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/354
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/275
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/275/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/354
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/354/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

Reply

lp:~vanvugt/mir/no-SurfaceBufferAccess updated on 2014-12-01

2004. By Daniel van Vugt on 2014-12-01: Ping, Jenkins

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-12-01:

#

> I am disappointed. Although still waiting for more votes, quietly confident
> that a larger sample size will yield more approvals.
>
> It's really important that we push for simplifications where they can be made.
> Complexity scales exponentially with code size and so does maintenance burden.
> So it's in everyone's interest to strive to simplify where possible.

When there are disapprovals on an MP, it's best to get those addressed/removed before expecting others to join in and review. When there are existing disapprovals and some refrains, especially after an MP has been up for a while, a 'refrain' should be considered equivalent to 'disapprove', or otherwise they'd join in the discussion with their point of view if they don't agree with the disapprove. Just my observation so far on how the review process works.

Reply

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Reply

Revision history for this message

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

#

Cemil:
I have answered all comments so far and if a review is flawed in reasoning, also pointed that out. AFAIK, there's nothing left that needs fixing here.

Also, I agree that to abstain is often a weak disapproval. However this is a process the mir-team has come to an agreement on over the past few years. We all occasionally disagree with each other's work but also all recognise it's not helpful to block it every time. So the process we've settled on is to state an opinion but unless serious bugs/issues will follow then Abstain instead of Disapprove. And it's not one-sided; we do this to each other regularly.

If "refrain" means the absence of any comments and not an Abstain, then I disagree. We all have different reasons for wanting to stay out of some reviews. And if you're intentionally staying out of one then you probably have had the discipline to not look at it in detail, so have no opinion.

Reply

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-12-02:

#

>> What I don't want is this to become a branch that is rejected on non-technical grounds of simply >> creating too much discussion. If there is a logical reason why this simplification is not
>> beneficial (or less beneficial than not landing it) then I'm all ears.

Hi! I think the fate of this MP is decided. I want to chime in though because I appreciate you trying to clean things up and don't want you to be frustrated by silence :)

Ultimately most discussions of interface design take place in rhetoric as much as logic. This is ok!

Examples of rhetoric:
Daniel: "It's really important that we push for simplifications where they can be made. Complexity scales exponentially with code size and so does maintenance burden. So it's in everyone's interest to strive to simplify where possible."
Alan: "My reasons for preferring the ExecuteAround pattern isn't a "minor stylistic preference". My experience (over decades) and that of other pundits is that it leads to a better separation of concerns and more maintainable code."

As for me, I've already been rhetorically persuaded (for years) that the execute-around pattern is a nice way to handle resource access. Even though this code ('logically') does not break anything with the current code base, it weakens my mental model of the code under refactoring (over time).

Peace!

Reply

Revision history for this message

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

#

OK, I think something just like this branch will land in the future as the snapshot feature either goes away completely or gets simplified to match the one remaining use case for it. Paths such as those are more decisive and convincing. I can see the path presented here is less popular.

Reply

lp:~vanvugt/mir/no-SurfaceBufferAccess updated on 2014-12-03

2005. By Daniel van Vugt on 2014-12-02: Merge latest trunk
2006. By Daniel van Vugt on 2014-12-03: Merge latest trunk

Mir

Merge lp:~vanvugt/mir/no-SurfaceBufferAccess into lp:mir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

1	=== modified file 'include/server/mir/scene/surface.h'
2	--- include/server/mir/scene/surface.h 2014-11-27 03:48:20 +0000
3	+++ include/server/mir/scene/surface.h 2014-12-03 02:28:05 +0000
4	@@ -21,7 +21,6 @@
5
6	#include "mir/graphics/renderable.h"
7	#include "mir/input/surface.h"
8	-#include "mir/scene/surface_buffer_access.h"
9	#include "mir/frontend/surface.h"
10
11	#include <vector>
12	@@ -39,8 +38,7 @@
13
14	class Surface :
15	public input::Surface,
16	- public frontend::Surface,
17	- public SurfaceBufferAccess
18	+ public frontend::Surface
19	{
20	public:
21	// resolve ambiguous member function names
22	@@ -56,6 +54,13 @@
23	/// Size of the surface including window frame (if any)
24	virtual geometry::Size size() const = 0;
25
26	+ // Snapshots do not consume buffers (only observe)
27	+ virtual std::shared_ptr<graphics::Buffer> snapshot() const = 0;
28	+
29	+ // Compositors consume buffers, thereby dictating the client frame rate
30	+ // TODO: Maybe remove confusing word "snapshot" here, or even consider
31	+ // if we can merge snapshotting into this function by careful use of
32	+ // compositor_id.
33	virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const = 0;
34
35	virtual float alpha() const = 0; //only used in examples/
36
37	=== removed file 'include/server/mir/scene/surface_buffer_access.h'
38	--- include/server/mir/scene/surface_buffer_access.h 2014-11-24 02:16:00 +0000
39	+++ include/server/mir/scene/surface_buffer_access.h 1970-01-01 00:00:00 +0000
40	@@ -1,50 +0,0 @@
41	-/*
42	- * Copyright © 2013-2014 Canonical Ltd.
43	- *
44	- * This program is free software: you can redistribute it and/or modify it
45	- * under the terms of the GNU General Public License version 3,
46	- * as published by the Free Software Foundation.
47	- *
48	- * This program is distributed in the hope that it will be useful,
49	- * but WITHOUT ANY WARRANTY; without even the implied warranty of
50	- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
51	- * GNU General Public License for more details.
52	- *
53	- * You should have received a copy of the GNU General Public License
54	- * along with this program. If not, see <http://www.gnu.org/licenses/>.
55	- *
56	- * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
57	- */
58	-
59	-#ifndef MIR_SCENE_SURFACE_BUFFER_ACCESS_H_
60	-#define MIR_SCENE_SURFACE_BUFFER_ACCESS_H_
61	-
62	-#include <functional>
63	-
64	-namespace mir
65	-{
66	-namespace graphics
67	-{
68	-class Buffer;
69	-}
70	-namespace scene
71	-{
72	-
73	-class SurfaceBufferAccess
74	-{
75	-public:
76	- virtual ~SurfaceBufferAccess() = default;
77	-
78	- virtual void with_most_recent_buffer_do(
79	- std::function<void(graphics::Buffer&)> const& exec) = 0;
80	-
81	-protected:
82	- SurfaceBufferAccess() = default;
83	- SurfaceBufferAccess(SurfaceBufferAccess const&) = delete;
84	- SurfaceBufferAccess& operator=(SurfaceBufferAccess const&) = delete;
85	-};
86	-
87	-}
88	-}
89	-
90	-#endif /* MIR_SCENE_SURFACE_BUFFER_ACCESS_H_ */
91
92	=== modified file 'server-ABI-sha1sums'
93	--- server-ABI-sha1sums 2014-11-27 03:48:20 +0000
94	+++ server-ABI-sha1sums 2014-12-03 02:28:05 +0000
95	@@ -93,11 +93,10 @@
96	f148a69caa39756d43cfaf7cc868ad681bda9ef6 include/server/mir/scene/session.h
97	ee94083f10f890e24c0e0bbdb94842e2dd788deb include/server/mir/scene/session_listener.h
98	7f5f26000fd2312373817d05c488a6a950a47c82 include/server/mir/scene/snapshot.h
99	-5bab4dc0a6488b8b9d47e958c6bab94f1dcf2c57 include/server/mir/scene/surface_buffer_access.h
100	bbc9e2e2bb71634cd6f1c5c0430093e10e74fa23 include/server/mir/scene/surface_configurator.h
101	e5e4dd7bcaf186810043fa0f05be42d7e49b0843 include/server/mir/scene/surface_coordinator.h
102	dd8f054f786b9746d9e513f2656e931234b5283a include/server/mir/scene/surface_creation_parameters.h
103	-24d59fc6f0245769f607d93b41a8d5e6cf9949b4 include/server/mir/scene/surface.h
104	+5cd9c2443a7388db61a473785dd7944eff232f02 include/server/mir/scene/surface.h
105	587e22d751656ce2d9536afdf5659276ff9bbc46 include/server/mir/scene/surface_observer.h
106	7ef3e99901168cda296d74d05a979f47bf9c3ff1 include/server/mir/server_action_queue.h
107	8d83a51c278b8b71866d2178d9b6387c1f91a7d0 include/server/mir/server_configuration.h
108
109	=== modified file 'src/server/scene/application_session.cpp'
110	--- src/server/scene/application_session.cpp 2014-11-24 02:16:00 +0000
111	+++ src/server/scene/application_session.cpp 2014-12-03 02:28:05 +0000
112	@@ -108,7 +108,7 @@
113	void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)
114	{
115	if (auto surface = default_surface())
116	- snapshot_strategy->take_snapshot_of(surface, snapshot_taken);
117	+ snapshot_strategy->take_snapshot_of(surface->snapshot(), snapshot_taken);
118	else
119	snapshot_taken(Snapshot());
120	}
121
122	=== modified file 'src/server/scene/basic_surface.cpp'
123	--- src/server/scene/basic_surface.cpp 2014-11-27 03:48:20 +0000
124	+++ src/server/scene/basic_surface.cpp 2014-12-03 02:28:05 +0000
125	@@ -234,7 +234,7 @@
126	surface_buffer_stream->allow_framedropping(allow);
127	}
128
129	-std::shared_ptr<mg::Buffer> ms::BasicSurface::snapshot_buffer() const
130	+std::shared_ptr<mg::Buffer> ms::BasicSurface::snapshot() const
131	{
132	return surface_buffer_stream->lock_snapshot_buffer();
133	}
134	@@ -379,14 +379,6 @@
135	observers.reception_mode_set_to(mode);
136	}
137
138	-void ms::BasicSurface::with_most_recent_buffer_do(
139	- std::function<void(mg::Buffer&)> const& exec)
140	-{
141	- auto buf = snapshot_buffer();
142	- exec(*buf);
143	-}
144	-
145	-
146	MirSurfaceType ms::BasicSurface::type() const
147	{
148	std::unique_lock<std::mutex> lg(guard);
149
150	=== modified file 'src/server/scene/basic_surface.h'
151	--- src/server/scene/basic_surface.h 2014-11-27 03:48:20 +0000
152	+++ src/server/scene/basic_surface.h 2014-12-03 02:28:05 +0000
153	@@ -101,7 +101,7 @@
154
155	MirPixelFormat pixel_format() const override;
156
157	- std::shared_ptr<graphics::Buffer> snapshot_buffer() const;
158	+ std::shared_ptr<graphics::Buffer> snapshot() const override;
159	void swap_buffers(graphics::Buffer* old_buffer, std::function<void(graphics::Buffer* new_buffer)> complete) override;
160	void force_requests_to_complete() override;
161
162	@@ -129,9 +129,6 @@
163
164	std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const override;
165
166	- void with_most_recent_buffer_do(
167	- std::function<void(graphics::Buffer&)> const& exec) override;
168	-
169	MirSurfaceType type() const override;
170	MirSurfaceState state() const override;
171	void take_input_focus(std::shared_ptr<shell::InputTargeter> const& targeter) override;
172
173	=== modified file 'src/server/scene/snapshot_strategy.h'
174	--- src/server/scene/snapshot_strategy.h 2014-04-16 02:37:09 +0000
175	+++ src/server/scene/snapshot_strategy.h 2014-12-03 02:28:05 +0000
176	@@ -25,9 +25,10 @@
177
178	namespace mir
179	{
180	+namespace graphics { class Buffer; }
181	+
182	namespace scene
183	{
184	-class SurfaceBufferAccess;
185
186	class SnapshotStrategy
187	{
188	@@ -35,7 +36,7 @@
189	virtual ~SnapshotStrategy() = default;
190
191	virtual void take_snapshot_of(
192	- std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
193	+ std::shared_ptr<graphics::Buffer> const& buf,
194	SnapshotCallback const& snapshot_taken) = 0;
195
196	protected:
197
198	=== modified file 'src/server/scene/threaded_snapshot_strategy.cpp'
199	--- src/server/scene/threaded_snapshot_strategy.cpp 2014-11-24 02:16:00 +0000
200	+++ src/server/scene/threaded_snapshot_strategy.cpp 2014-12-03 02:28:05 +0000
201	@@ -18,8 +18,8 @@
202
203	#include "threaded_snapshot_strategy.h"
204	#include "pixel_buffer.h"
205	-#include "mir/scene/surface_buffer_access.h"
206	#include "mir/thread_name.h"
207	+#include "mir/graphics/buffer.h"
208
209	#include <deque>
210	#include <mutex>
211	@@ -35,7 +35,7 @@
212
213	struct WorkItem
214	{
215	- std::shared_ptr<SurfaceBufferAccess> const surface_buffer_access;
216	+ std::shared_ptr<graphics::Buffer> buffer;
217	ms::SnapshotCallback const snapshot_taken;
218	};
219
220	@@ -64,28 +64,17 @@
221
222	lock.unlock();
223
224	- take_snapshot(wi);
225	+ pixels->fill_from(*wi.buffer);
226	+ wi.buffer.reset(); // No need to hold it any longer
227	+ wi.snapshot_taken(ms::Snapshot{pixels->size(),
228	+ pixels->stride(),
229	+ pixels->as_argb_8888()});
230
231	lock.lock();
232	}
233	}
234	}
235
236	- void take_snapshot(WorkItem const& wi)
237	- {
238	- wi.surface_buffer_access->with_most_recent_buffer_do(
239	- [this](graphics::Buffer& buffer)
240	- {
241	- pixels->fill_from(buffer);
242	- });
243	-
244	-
245	- wi.snapshot_taken(
246	- ms::Snapshot{pixels->size(),
247	- pixels->stride(),
248	- pixels->as_argb_8888()});
249	- }
250	-
251	void schedule_snapshot(WorkItem const& wi)
252	{
253	std::lock_guard<std::mutex> lg{work_mutex};
254	@@ -126,8 +115,8 @@
255	}
256
257	void ms::ThreadedSnapshotStrategy::take_snapshot_of(
258	- std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
259	+ std::shared_ptr<graphics::Buffer> const& buf,
260	SnapshotCallback const& snapshot_taken)
261	{
262	- functor->schedule_snapshot(WorkItem{surface_buffer_access, snapshot_taken});
263	+ functor->schedule_snapshot(WorkItem{buf, snapshot_taken});
264	}
265
266	=== modified file 'src/server/scene/threaded_snapshot_strategy.h'
267	--- src/server/scene/threaded_snapshot_strategy.h 2014-04-16 02:37:09 +0000
268	+++ src/server/scene/threaded_snapshot_strategy.h 2014-12-03 02:28:05 +0000
269	@@ -39,7 +39,7 @@
270	~ThreadedSnapshotStrategy() noexcept;
271
272	void take_snapshot_of(
273	- std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
274	+ std::shared_ptr<graphics::Buffer> const& buf,
275	SnapshotCallback const& snapshot_taken);
276
277	private:
278
279	=== modified file 'src/server/symbols.map'
280	--- src/server/symbols.map 2014-11-27 03:48:20 +0000
281	+++ src/server/symbols.map 2014-12-03 02:28:05 +0000
282	@@ -319,10 +319,6 @@
283	mir::scene::Surface::add_observer*;
284	mir::scene::Surface::allow_framedropping*;
285	mir::scene::Surface::alpha*;
286	- mir::scene::SurfaceBufferAccess::operator*;
287	- mir::scene::SurfaceBufferAccess::?SurfaceBufferAccess*;
288	- mir::scene::SurfaceBufferAccess::SurfaceBufferAccess*;
289	- mir::scene::SurfaceBufferAccess::with_most_recent_buffer_do*;
290	mir::scene::Surface::client_size*;
291	mir::scene::Surface::compositor_snapshot*;
292	mir::scene::SurfaceConfigurator::attribute_set*;
293	@@ -633,7 +629,6 @@
294	non-virtual?thunk?to?mir::scene::PromptSessionListener::?PromptSessionListener*;
295	non-virtual?thunk?to?mir::scene::PromptSessionManager::?PromptSessionManager*;
296	non-virtual?thunk?to?mir::scene::SessionListener::?SessionListener*;
297	- non-virtual?thunk?to?mir::scene::SurfaceBufferAccess::?SurfaceBufferAccess*;
298	non-virtual?thunk?to?mir::scene::SurfaceConfigurator::?SurfaceConfigurator*;
299	non-virtual?thunk?to?mir::scene::SurfaceCoordinator::?SurfaceCoordinator*;
300	non-virtual?thunk?to?mir::scene::SurfaceObserver::?SurfaceObserver*;
301	@@ -696,7 +691,6 @@
302	typeinfo?for?mir::scene::SessionListener;
303	typeinfo?for?mir::scene::Snapshot;
304	typeinfo?for?mir::scene::Surface;
305	- typeinfo?for?mir::scene::SurfaceBufferAccess;
306	typeinfo?for?mir::scene::SurfaceConfigurator;
307	typeinfo?for?mir::scene::SurfaceCoordinator;
308	typeinfo?for?mir::scene::SurfaceCreationParameters;
309	@@ -756,7 +750,6 @@
310	vtable?for?mir::scene::SessionListener;
311	vtable?for?mir::scene::Snapshot;
312	vtable?for?mir::scene::Surface;
313	- vtable?for?mir::scene::SurfaceBufferAccess;
314	vtable?for?mir::scene::SurfaceConfigurator;
315	vtable?for?mir::scene::SurfaceCoordinator;
316	vtable?for?mir::scene::SurfaceCreationParameters;
317
318	=== modified file 'tests/include/mir_test_doubles/mock_surface.h'
319	--- tests/include/mir_test_doubles/mock_surface.h 2014-11-24 02:16:00 +0000
320	+++ tests/include/mir_test_doubles/mock_surface.h 2014-12-03 02:28:05 +0000
321	@@ -67,6 +67,8 @@
322	MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));
323	MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
324	MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::SurfaceObserver> const&));
325	+
326	+ MOCK_CONST_METHOD0(snapshot, std::shared_ptr<graphics::Buffer>());
327	};
328
329	}
330
331	=== modified file 'tests/include/mir_test_doubles/null_snapshot_strategy.h'
332	--- tests/include/mir_test_doubles/null_snapshot_strategy.h 2014-04-16 02:37:09 +0000
333	+++ tests/include/mir_test_doubles/null_snapshot_strategy.h 2014-12-03 02:28:05 +0000
334	@@ -31,7 +31,7 @@
335	struct NullSnapshotStrategy : public scene::SnapshotStrategy
336	{
337	void take_snapshot_of(
338	- std::shared_ptr<scene::SurfaceBufferAccess> const&,
339	+ std::shared_ptr<graphics::Buffer> const&,
340	scene::SnapshotCallback const&)
341	{
342	}
343
344	=== modified file 'tests/include/mir_test_doubles/stub_scene_surface.h'
345	--- tests/include/mir_test_doubles/stub_scene_surface.h 2014-11-27 03:48:20 +0000
346	+++ tests/include/mir_test_doubles/stub_scene_surface.h 2014-12-03 02:28:05 +0000
347	@@ -99,7 +99,8 @@
348	int client_input_fd() const override { return fd;}
349	int configure(MirSurfaceAttrib, int) override { return 0; }
350	int query(MirSurfaceAttrib) override { return 0; }
351	- void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override {}
352	+ std::shared_ptr<graphics::Buffer> snapshot() const override
353	+ { return {}; }
354	};
355
356	}
357
358	=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
359	--- tests/unit-tests/scene/test_application_session.cpp 2014-11-27 03:48:20 +0000
360	+++ tests/unit-tests/scene/test_application_session.cpp 2014-12-03 02:28:05 +0000
361	@@ -28,6 +28,7 @@
362	#include "mir_test_doubles/null_snapshot_strategy.h"
363	#include "mir_test_doubles/null_event_sink.h"
364	#include "mir_test_doubles/null_prompt_session.h"
365	+#include "mir_test_doubles/stub_buffer.h"
366
367	#include <gmock/gmock.h>
368	#include <gtest/gtest.h>
369	@@ -52,7 +53,7 @@
370	~MockSnapshotStrategy() noexcept {}
371
372	MOCK_METHOD2(take_snapshot_of,
373	- void(std::shared_ptr<ms::SurfaceBufferAccess> const&,
374	+ void(std::shared_ptr<mir::graphics::Buffer> const&,
375	ms::SnapshotCallback const&));
376	};
377
378	@@ -279,18 +280,20 @@
379	{
380	using namespace ::testing;
381
382	+ std::shared_ptr<mir::graphics::Buffer> const snapped =
383	+ std::make_shared<mtd::StubBuffer>();
384	auto mock_surface = make_mock_surface();
385	+ EXPECT_CALL(*mock_surface, snapshot())
386	+ .WillOnce(Return(snapped));
387	+
388	NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
389
390	EXPECT_CALL(surface_coordinator, add_surface(_, _))
391	.WillOnce(Return(mock_surface));
392
393	- auto const default_surface_buffer_access =
394	- std::static_pointer_cast<ms::SurfaceBufferAccess>(mock_surface);
395	auto const snapshot_strategy = std::make_shared<MockSnapshotStrategy>();
396
397	- EXPECT_CALL(*snapshot_strategy,
398	- take_snapshot_of(default_surface_buffer_access, _));
399	+ EXPECT_CALL(*snapshot_strategy, take_snapshot_of(snapped, _));
400
401	ms::ApplicationSession app_session(
402	mt::fake_shared(surface_coordinator),
403
404	=== modified file 'tests/unit-tests/scene/test_surface_impl.cpp'
405	--- tests/unit-tests/scene/test_surface_impl.cpp 2014-11-27 03:48:20 +0000
406	+++ tests/unit-tests/scene/test_surface_impl.cpp 2014-12-03 02:28:05 +0000
407	@@ -405,7 +405,7 @@
408	surf.take_input_focus(mt::fake_shared(targeter));
409	}
410
411	-TEST_F(Surface, with_most_recent_buffer_do_uses_compositor_buffer)
412	+TEST_F(Surface, snapshot_uses_compositor_buffer)
413	{
414	auto stub_buffer_stream = std::make_shared<mtd::StubBufferStream>();
415
416	@@ -420,15 +420,8 @@
417	std::shared_ptr<mg::CursorImage>(),
418	report);
419
420	- mg::Buffer* buf_ptr{nullptr};
421	-
422	- surf.with_most_recent_buffer_do(
423	- [&](mg::Buffer& buffer)
424	- {
425	- buf_ptr = &buffer;
426	- });
427	-
428	- EXPECT_EQ(stub_buffer_stream->stub_compositor_buffer.get(), buf_ptr);
429	+ EXPECT_EQ(stub_buffer_stream->stub_compositor_buffer.get(),
430	+ surf.snapshot().get());
431	}
432
433	TEST_F(Surface, emits_client_close_events)
434
435	=== modified file 'tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp'
436	--- tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp 2014-11-24 02:16:00 +0000
437	+++ tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp 2014-12-03 02:28:05 +0000
438	@@ -18,7 +18,6 @@
439
440	#include "src/server/scene/threaded_snapshot_strategy.h"
441	#include "src/server/scene/pixel_buffer.h"
442	-#include "mir/scene/surface_buffer_access.h"
443	#include "mir/graphics/buffer.h"
444
445	#include "mir_test_doubles/stub_buffer.h"
446	@@ -43,22 +42,6 @@
447	namespace
448	{
449
450	-class StubSurfaceBufferAccess : public ms::SurfaceBufferAccess
451	-{
452	-public:
453	- ~StubSurfaceBufferAccess() noexcept {}
454	-
455	- void with_most_recent_buffer_do(
456	- std::function<void(mg::Buffer&)> const& exec)
457	- {
458	- thread_name = mt::current_thread_name();
459	- exec(buffer);
460	- }
461	-
462	- mtd::StubBuffer buffer;
463	- std::string thread_name;
464	-};
465	-
466	class MockPixelBuffer : public ms::PixelBuffer
467	{
468	public:
469	@@ -72,7 +55,7 @@
470
471	struct ThreadedSnapshotStrategyTest : testing::Test
472	{
473	- StubSurfaceBufferAccess buffer_access;
474	+ mtd::StubBuffer buffer;
475	};
476
477	}
478	@@ -87,7 +70,7 @@
479
480	MockPixelBuffer pixel_buffer;
481
482	- EXPECT_CALL(pixel_buffer, fill_from(Ref(buffer_access.buffer)));
483	+ EXPECT_CALL(pixel_buffer, fill_from(Ref(buffer)));
484	EXPECT_CALL(pixel_buffer, as_argb_8888())
485	.WillOnce(Return(pixels));
486	EXPECT_CALL(pixel_buffer, size())
487	@@ -102,7 +85,7 @@
488	ms::Snapshot snapshot;
489
490	strategy.take_snapshot_of(
491	- mt::fake_shared(buffer_access),
492	+ mt::fake_shared(buffer),
493	[&](ms::Snapshot const& s)
494	{
495	snapshot = s;
496	@@ -126,14 +109,17 @@
497
498	mt::WaitCondition snapshot_taken;
499
500	+ std::string thread_name;
501	+
502	strategy.take_snapshot_of(
503	- mt::fake_shared(buffer_access),
504	+ mt::fake_shared(buffer),
505	[&](ms::Snapshot const&)
506	{
507	+ thread_name = mt::current_thread_name();
508	snapshot_taken.wake_up_everyone();
509	});
510
511	snapshot_taken.wait_for_at_most_seconds(5);
512
513	- EXPECT_THAT(buffer_access.thread_name, Eq("Mir/Snapshot"));
514	+ EXPECT_THAT(thread_name, Eq("Mir/Snapshot"));
515	}