Merge lp:~kdub/mir/surface-buffer-access into lp:mir

[Kevin DuBois]

Have the mf::BufferStream interface inherit from the ms::SurfaceBufferAccess interface, as the streams are what have the buffers, not the Surfaces. (previously ms::Surface inherited from the interface). This gets rid of the pass-through function in surface, as well as makes it more obvious about which buffer is being accessed (as surfaces already have some cursor buffers and client buffers, and will soon have more than one stream).

Also rename the interface from "SurfaceBufferAccess" to just BufferAccess.

Have the mf::BufferStream interface inherit from the ms::SurfaceBufferAccess interface, as the streams are what have the buffers, not the Surfaces. (previously ms::Surface inherited from the interface). This gets rid of the pass-through function in surface, as well as makes it more obvious about which buffer is being accessed (as surfaces already have some cursor buffers and client buffers, and will soon have more than one stream).

Also rename the interface from "SurfaceBufferAccess" to just BufferAccess.

[Kevin DuBois (kdub)]

IIRC, SurfaceBufferAccess was originally created to avoid the snapshotting code from having access to the full Surface interface just to take a snapshot. Now that the concepts have been teased out a bit, I wouldn't mind just having the snapshotting code take a ptr to mf::BufferStream. mf::BufferStream already has a 'with_most_recent_buffer_do" function on it, so removing the interface would just have the effect of giving the screenshotting code a wider interface.

Removal has been talked about in previous relevant discussions:
https://code.launchpad.net/~kdub/mir/surface-interface-reductions/+merge/258493
https://code.launchpad.net/~vanvugt/mir/no-SurfaceBufferAccess/+merge/238376

So, needs-info from the team on the question "should I just remove BufferAcccess?". The part I need to address in the card I'm working on is the inheritance, but removing the interface altogether seems a nice cleanup.

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2573
http://jenkins.qa.ubuntu.com/job/mir-ci/3832/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2476
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2475
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2424
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1829
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1829/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2424
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2424/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5300
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20479

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> So, needs-info from the team on the question "should I just remove BufferAcccess?".
> The part I need to address in the card I'm working on is the inheritance, but
> removing the interface altogether seems a nice cleanup.

If I have understood the plan correctly, a surface is going to consist of potentially multiple bufferstreams for content (plus other bufferstreams for decoration, but we don't care about them here). So if we want a proper snapshot, the snapshotting code will need to access multiple buffers and should be able to perform some kind of basic blending to provide the final image. In order to combine the buffers, it will at least need to know their positions, so passing buffer streams to the snapshotting code is not enough.

If this is indeed the way we are heading, we probably still need some kind of function at the Surface/BufferAcccess level, like with_most_recent_buffers_do(<std::function accepting vector of buffers+positions>) or with_most_recent_renderables_do(...).

Since the snapshotting code is becoming more complicated (need to combine buffers etc), it's worth ensuring that our downstreams still find it useful and there is no easier alternative they can pursue. I will talk to Gerry about this.

[Alexandros Frantzis (afrantzis)]

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> If this is indeed the way we are heading, we probably still need some kind of function at the
> Surface/BufferAcccess level, like with_most_recent_buffers_do(<std::function accepting vector of
> buffers+positions>) or with_most_recent_renderables_do(...).

Even if we drop the wrap-around idiom and opt for a different/direct approach, the function will still need to be at the Surface level, and a separate interface (like BufferAccess) will still be preferable.

[Daniel van Vugt (vanvugt)]

This is not a review so I won't block anyone with tangential thoughts... But I would prefer to see us use a design that doesn't have the word "Access" as the primary noun in a class name. "Access" is too vague a word and doesn't give the reader any hint of what the class might be for. It's one of those cases where the class name is so useless you just have to treat the name as opaque and infer meaning from the implementation/context instead.

[Kevin DuBois (kdub)]

@Alexandros, (recap of irc discussion)
The goal is to have take a snapshot of the Surface, including all bufferstreams contained. We'll avoid having a Surface::get_all_the_streams() function by shifting the SnapshotStrategy from the ApplicationSession to the Surface. The snapshot strategy will shift from getting one stream to getting all the streams comprising the surface (still via wrap-around).

@Daniel,
I think its a good point that every interface provides access to something, and that tagging Access on the end is about the same as tagging Interface on the end... will see if I can come up with a better name while I'm renaming anyways

[lp:~kdub/mir/surface-buffer-access]

2574. By Kevin DuBois on 2015-05-14

use StreamDepiction instead of BufferAccess as the name of the interface

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2574
http://jenkins.qa.ubuntu.com/job/mir-ci/3837/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2482
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2481
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1834
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1834/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2430
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2430/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5307
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20495

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

17 +class BufferStream : public scene::StreamDepiction

It seems the wrong way around for a frontend interface to be extending a scene interface.

Its pre-existing but probably contributes to this oddity that this BufferStream interface is wrong - its isn't specifying the requirements of frontend. Vis:

    virtual void add_observer(std::shared_ptr<scene::SurfaceObserver> const& observer) = 0;
    virtual void remove_observer(std::weak_ptr<scene::SurfaceObserver> const& observer) = 0;

these functions are not needed by frontend.

I don't think we need scene::StreamDepiction (we might need a scene BufferStream : public frontend::BufferStream to fix the existing mess - but that's a separate discussion).

Anything I'm missing in this analysis?

review: Needs Information

[Robert Carr (robertcarr)]

"mga::BufferAccess::write"

->

"mga::StreamDepiction::write

I think we've gone from a name that makes sense to one which is obviously wrong in order to placate review suggestions that add nothing besides being hard to prove incorrect.

[Robert Carr (robertcarr)]

s/placate/respect/

[Robert Carr (robertcarr)]

LGTM though I think mga::BufferAccess was better than mga::StreamDepiction (scene name change is fine I guess...)

review: Approve

[Kevin DuBois (kdub)]

@bufferstream analysis, the current structure is a bit funny if you look at it in a vacuum, but given the history, we've at least got all the buffer-stream-ish functionality grouped together now in the "mc::BufferStream : mf::BufferStream : ms::StreamDepiction" structure. a ms::Surface : ms::SurfaceBufferAccess doesn't make much sense ever since we split the streams and the surfaces into different objects.

I did try to "flip it" to "mc::BufferStream : ms::StreamDepiction : mf::BufferStream", but the server api gives has more mf::BufferStreams (eg in the Session), so flipping means that we have shift those interfaces around too (for only the benefit of the snapshotting code, really). I'd be fine collapsing the function into mf::BufferStream (as mf::BufferStream has somewhat become the 'public face' of the streams for the server API).

So, I'd be fine with any of:
1) collapsing the only function into mf::BufferStream
2) renaming to StreamDepiction (or any other clever name)
3) dropping "Surface" from "SurfaceBufferAccess", as the interface doesn't have anything to do with surfaces anymore really
in order to resolve the standing structural question of 'which stream's buffer should SurfaceBufferAccess access?'

[Alan Griffiths (alan-griffiths)]

> So, I'd be fine with any of:
> 1) collapsing the only function into mf::BufferStream

That seems least bad to me. It is in line with the preexisting problem with this class hierarchy. (Which I wasn't suggesting you fix for this MP.)

[Alexandros Frantzis (afrantzis)]

Ideally I would like us to keep a narrow interface for BufferAccess/StreamDepiction to be used by the snapshotting code. However, I won't block on it, because:

1. I hope the snapshotting code will go away in the short/medium-term (replaced by Qt code in qtmir)
2. *::BufferStream is narrower than Surface, not as hard to mock/stub
3. The hierarchy needs a cleanup, as noted in other comments

review: Abstain

[lp:~kdub/mir/surface-buffer-access]

2575. By Kevin DuBois on 2015-05-29

sidestep naming discussion by removing the interface altogether

2576. By Kevin DuBois on 2015-05-29

rollback an overzealous sed command that also changed mga::BufferAccess

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2576
http://jenkins.qa.ubuntu.com/job/mir-ci/3952/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2637
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/148
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2585
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/108
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/108/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2585
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2585/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5435
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20849

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't think this gets us to the right place, but it Bob Martin's "boy scouts rule".

review: Approve

[Kevin DuBois (kdub)]

review: Abstain

=== modified file 'include/server/mir/scene/surface.h'
--- include/server/mir/scene/surface.h	2015-05-19 21:34:34 +0000
+++ include/server/mir/scene/surface.h	2015-05-29 17:54:05 +0000
@@ -21,7 +21,6 @@
 
 #include "mir/graphics/renderable.h"
 #include "mir/input/surface.h"
-#include "mir/scene/surface_buffer_access.h"
 #include "mir/frontend/surface.h"
 
 #include <vector>
@@ -39,8 +38,7 @@
 
 class Surface :
     public input::Surface,
-    public frontend::Surface,
-    public SurfaceBufferAccess
+    public frontend::Surface
 {
 public:
     // resolve ambiguous member function names

=== removed file 'include/server/mir/scene/surface_buffer_access.h'
--- include/server/mir/scene/surface_buffer_access.h	2015-05-11 20:07:43 +0000
+++ include/server/mir/scene/surface_buffer_access.h	1970-01-01 00:00:00 +0000
@@ -1,50 +0,0 @@
-/*
- * Copyright © 2013-2014 Canonical Ltd.
- *
- * This program is free software: you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Alexandros Frantzis <[e-mail]>
- */
-
-#ifndef MIR_SCENE_SURFACE_BUFFER_ACCESS_H_
-#define MIR_SCENE_SURFACE_BUFFER_ACCESS_H_
-
-#include <functional>
-
-namespace mir
-{
-namespace graphics
-{
-class Buffer;
-}
-namespace scene
-{
-
-class SurfaceBufferAccess
-{
-public:
-    virtual ~SurfaceBufferAccess() = default;
-
-    virtual void with_most_recent_buffer_do(
-        std::function<void(graphics::Buffer&)> const& exec) = 0;
-
-protected:
-    SurfaceBufferAccess() = default;
-    SurfaceBufferAccess(SurfaceBufferAccess const&) = delete;
-    SurfaceBufferAccess& operator=(SurfaceBufferAccess const&) = delete;
-};
-
-}
-}
-
-#endif /* MIR_SCENE_SURFACE_BUFFER_ACCESS_H_ */

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2015-05-19 21:34:34 +0000
+++ src/server/scene/application_session.cpp	2015-05-29 17:54:05 +0000
@@ -190,10 +190,20 @@
 
 void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)
 {
-    if (auto surface = default_surface())
-        snapshot_strategy->take_snapshot_of(surface, snapshot_taken);
-    else
-        snapshot_taken(Snapshot());
+    //TODO: taking a snapshot of a session doesn't make much sense. Snapshots can be on surfaces
+    //or bufferstreams, as those represent some content. A multi-surface session doesn't have enough
+    //info to cobble together a snapshot buffer without WM info.
+    for(auto const& surface_it : surfaces)
+    {
+        if (default_surface() == surface_it.second)
+        {
+            auto id = mf::BufferStreamId(surface_it.first.as_value());
+            snapshot_strategy->take_snapshot_of(checked_find(id)->second, snapshot_taken);
+            return;
+        }
+    }
+
+    snapshot_taken(Snapshot());
 }
 
 std::shared_ptr<ms::Surface> ms::ApplicationSession::default_surface() const

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2015-05-19 21:34:34 +0000
+++ src/server/scene/basic_surface.cpp	2015-05-29 17:54:05 +0000
@@ -361,13 +361,6 @@
     observers.reception_mode_set_to(mode);
 }
 
-void ms::BasicSurface::with_most_recent_buffer_do(
-    std::function<void(mg::Buffer&)> const& exec)
-{
-    surface_buffer_stream->with_most_recent_buffer_do(exec);
-}
-
-
 MirSurfaceType ms::BasicSurface::type() const
 {    
     std::unique_lock<std::mutex> lg(guard);

=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h	2015-05-19 21:34:34 +0000
+++ src/server/scene/basic_surface.h	2015-05-29 17:54:05 +0000
@@ -116,9 +116,6 @@
     std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const override;
     int buffers_ready_for_compositor(void const* compositor_id) const override;
 
-    void with_most_recent_buffer_do(
-        std::function<void(graphics::Buffer&)> const& exec) override;
-
     MirSurfaceType type() const override;
     MirSurfaceState state() const override;
     int configure(MirSurfaceAttrib attrib, int value) override;

=== modified file 'src/server/scene/snapshot_strategy.h'
--- src/server/scene/snapshot_strategy.h	2015-05-13 13:07:31 +0000
+++ src/server/scene/snapshot_strategy.h	2015-05-29 17:54:05 +0000
@@ -25,9 +25,9 @@
 
 namespace mir
 {
+namespace compositor { class BufferStream; }
 namespace scene
 {
-class SurfaceBufferAccess;
 
 class SnapshotStrategy
 {
@@ -35,7 +35,7 @@
     virtual ~SnapshotStrategy() = default;
 
     virtual void take_snapshot_of(
-        std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
+        std::shared_ptr<compositor::BufferStream> const& surface_buffer_access,
         SnapshotCallback const& snapshot_taken) = 0;
 
 protected:

=== modified file 'src/server/scene/threaded_snapshot_strategy.cpp'
--- src/server/scene/threaded_snapshot_strategy.cpp	2015-05-19 17:17:57 +0000
+++ src/server/scene/threaded_snapshot_strategy.cpp	2015-05-29 17:54:05 +0000
@@ -18,7 +18,7 @@
 
 #include "threaded_snapshot_strategy.h"
 #include "pixel_buffer.h"
-#include "mir/scene/surface_buffer_access.h"
+#include "mir/compositor/buffer_stream.h"
 #include "mir/thread_name.h"
 
 #include <deque>
@@ -35,7 +35,7 @@
 
 struct WorkItem
 {
-    std::shared_ptr<SurfaceBufferAccess> const stream;
+    std::shared_ptr<compositor::BufferStream> const stream;
     ms::SnapshotCallback const snapshot_taken;
 };
 
@@ -123,7 +123,7 @@
 }
 
 void ms::ThreadedSnapshotStrategy::take_snapshot_of(
-    std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
+    std::shared_ptr<compositor::BufferStream> const& surface_buffer_access,
     SnapshotCallback const& snapshot_taken)
 {
     functor->schedule_snapshot(WorkItem{surface_buffer_access, snapshot_taken});

=== modified file 'src/server/scene/threaded_snapshot_strategy.h'
--- src/server/scene/threaded_snapshot_strategy.h	2015-05-13 13:07:31 +0000
+++ src/server/scene/threaded_snapshot_strategy.h	2015-05-29 17:54:05 +0000
@@ -39,7 +39,7 @@
     ~ThreadedSnapshotStrategy() noexcept;
 
     void take_snapshot_of(
-        std::shared_ptr<SurfaceBufferAccess> const& surface_buffer_access,
+        std::shared_ptr<compositor::BufferStream> const& surface_buffer_access,
         SnapshotCallback const& snapshot_taken);
 
 private:

=== modified file 'src/server/symbols.map'
--- src/server/symbols.map	2015-05-21 19:25:51 +0000
+++ src/server/symbols.map	2015-05-29 17:54:05 +0000
@@ -106,9 +106,6 @@
     mir::scene::SessionListener::operator*;
     mir::scene::SessionListener::?SessionListener*;
     mir::scene::SessionListener::SessionListener*;
-    mir::scene::SurfaceBufferAccess::operator*;
-    mir::scene::SurfaceBufferAccess::?SurfaceBufferAccess*;
-    mir::scene::SurfaceBufferAccess::SurfaceBufferAccess*;
     mir::scene::SurfaceConfigurator::operator*;
     mir::scene::SurfaceConfigurator::?SurfaceConfigurator*;
     mir::scene::SurfaceConfigurator::SurfaceConfigurator*;
@@ -302,7 +299,6 @@
     non-virtual?thunk?to?mir::scene::PromptSessionManager::?PromptSessionManager*;
     non-virtual?thunk?to?mir::scene::SessionCoordinator::?SessionCoordinator*;
     non-virtual?thunk?to?mir::scene::SessionListener::?SessionListener*;
-    non-virtual?thunk?to?mir::scene::SurfaceBufferAccess::?SurfaceBufferAccess*;
     non-virtual?thunk?to?mir::scene::SurfaceConfigurator::?SurfaceConfigurator*;
     non-virtual?thunk?to?mir::scene::SurfaceCoordinator::?SurfaceCoordinator*;
     non-virtual?thunk?to?mir::scene::SurfaceObserver::?SurfaceObserver*;
@@ -391,7 +387,6 @@
     typeinfo?for?mir::scene::SessionListener;
     typeinfo?for?mir::scene::Snapshot;
     typeinfo?for?mir::scene::Surface;
-    typeinfo?for?mir::scene::SurfaceBufferAccess;
     typeinfo?for?mir::scene::SurfaceConfigurator;
     typeinfo?for?mir::scene::SurfaceCoordinator;
     typeinfo?for?mir::scene::SurfaceCreationParameters;
@@ -449,7 +444,6 @@
     vtable?for?mir::scene::SessionListener;
     vtable?for?mir::scene::Snapshot;
     vtable?for?mir::scene::Surface;
-    vtable?for?mir::scene::SurfaceBufferAccess;
     vtable?for?mir::scene::SurfaceConfigurator;
     vtable?for?mir::scene::SurfaceCoordinator;
     vtable?for?mir::scene::SurfaceCreationParameters;

=== modified file 'tests/include/mir_test_doubles/null_snapshot_strategy.h'
--- tests/include/mir_test_doubles/null_snapshot_strategy.h	2015-05-13 13:07:31 +0000
+++ tests/include/mir_test_doubles/null_snapshot_strategy.h	2015-05-29 17:54:05 +0000
@@ -31,7 +31,7 @@
 struct NullSnapshotStrategy : public scene::SnapshotStrategy
 {
     void take_snapshot_of(
-        std::shared_ptr<scene::SurfaceBufferAccess> const&,
+        std::shared_ptr<compositor::BufferStream> const&,
         scene::SnapshotCallback const&)
     {
     }

=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp	2015-05-19 21:34:34 +0000
+++ tests/unit-tests/scene/test_application_session.cpp	2015-05-29 17:54:05 +0000
@@ -67,7 +67,7 @@
     ~MockSnapshotStrategy() noexcept {}
 
     MOCK_METHOD2(take_snapshot_of,
-                void(std::shared_ptr<ms::SurfaceBufferAccess> const&,
+                void(std::shared_ptr<mc::BufferStream> const&,
                      ms::SnapshotCallback const&));
 };
 
@@ -410,20 +410,20 @@
 
     auto mock_surface = make_mock_surface();
     NiceMock<MockSurfaceFactory> surface_factory;
+    MockBufferStreamFactory mock_buffer_stream_factory;
+    std::shared_ptr<mc::BufferStream> const mock_stream = std::make_shared<mtd::MockBufferStream>();
+    ON_CALL(mock_buffer_stream_factory, create_buffer_stream(_)).WillByDefault(Return(mock_stream));
     ON_CALL(surface_factory, create_surface(_,_)).WillByDefault(Return(mock_surface));
     NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
 
-    auto const default_surface_buffer_access =
-        std::static_pointer_cast<ms::SurfaceBufferAccess>(mock_surface);
     auto const snapshot_strategy = std::make_shared<MockSnapshotStrategy>();
 
-    EXPECT_CALL(*snapshot_strategy,
-                take_snapshot_of(default_surface_buffer_access, _));
+    EXPECT_CALL(*snapshot_strategy, take_snapshot_of(mock_stream, _));
 
     ms::ApplicationSession app_session(
         mt::fake_shared(surface_coordinator),
         mt::fake_shared(surface_factory),
-        stub_buffer_stream_factory,
+        mt::fake_shared(mock_buffer_stream_factory),
         pid, name,
         snapshot_strategy,
         std::make_shared<ms::NullSessionListener>(),

=== modified file 'tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp'
--- tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp	2015-05-13 13:07:31 +0000
+++ tests/unit-tests/scene/test_threaded_snapshot_strategy.cpp	2015-05-29 17:54:05 +0000
@@ -18,11 +18,11 @@
 
 #include "src/server/scene/threaded_snapshot_strategy.h"
 #include "src/server/scene/pixel_buffer.h"
-#include "mir/scene/surface_buffer_access.h"
 #include "mir/graphics/buffer.h"
 
 #include "mir_test_doubles/stub_buffer.h"
 #include "mir_test_doubles/null_pixel_buffer.h"
+#include "mir_test_doubles/stub_buffer_stream.h"
 #include "mir_test/fake_shared.h"
 #include "mir_test/wait_condition.h"
 #include "mir_test/current_thread_name.h"
@@ -43,22 +43,6 @@
 namespace
 {
 
-class StubSurfaceBufferAccess : public ms::SurfaceBufferAccess
-{
-public:
-    ~StubSurfaceBufferAccess() noexcept {}
-
-    void with_most_recent_buffer_do(
-        std::function<void(mg::Buffer&)> const& exec)
-    {
-        thread_name = mt::current_thread_name();
-        exec(buffer);
-    }
-
-    mtd::StubBuffer buffer;
-    std::string thread_name;
-};
-
 class MockPixelBuffer : public ms::PixelBuffer
 {
 public:
@@ -72,7 +56,7 @@
 
 struct ThreadedSnapshotStrategyTest : testing::Test
 {
-    StubSurfaceBufferAccess buffer_access;
+    mtd::StubBufferStream buffer_access;
 };
 
 }
@@ -87,7 +71,7 @@
 
     MockPixelBuffer pixel_buffer;
 
-    EXPECT_CALL(pixel_buffer, fill_from(Ref(buffer_access.buffer)));
+    EXPECT_CALL(pixel_buffer, fill_from(Ref(*buffer_access.stub_compositor_buffer)));
     EXPECT_CALL(pixel_buffer, as_argb_8888())
         .WillOnce(Return(pixels));
     EXPECT_CALL(pixel_buffer, size())