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

Status:	Work in progress
Proposed branch:	lp://qastaging/~vanvugt/mir/no-SurfaceBufferAccess
Merge into:	lp://qastaging/mir
Prerequisite:	lp://qastaging/~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://qastaging/~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.qastaging.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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~vanvugt/mir/no-SurfaceBufferAccess into lp://qastaging/mir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers