Merge lp:~alan-griffiths/mir/fix-Surface-hierarchy-remove-ClientTrackingSurface into lp:mir

[Alan Griffiths]

frontend: remove ClientTrackingSurface from the Surface class hierarchy

frontend: remove ClientTrackingSurface from the Surface class hierarchy

Shifts the functionality to SessionMediator where it is actually needed.

[Alexandros Frantzis (afrantzis)]

+ void mf::SessionMediator::advance_buffer(bool& need_full_ipc, SurfaceId surf_id, Surface& surface, std::shared_ptr<graphics::Buffer>& client_buffer)

looks a bit confusing, perphaps it would be more straightforward as:

std::shared_ptr<graphics::Buffer> mf::SessionMediator::advance_buffer(Surface& surface, SurfaceId surf_id, bool& need_full_ipc) ?

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

Abstaining in order not to block this.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> + void mf::SessionMediator::advance_buffer(bool& need_full_ipc, SurfaceId
> surf_id, Surface& surface, std::shared_ptr<graphics::Buffer>& client_buffer)
>
> looks a bit confusing, perphaps it would be more straightforward as:
>
> std::shared_ptr<graphics::Buffer> mf::SessionMediator::advance_buffer(Surface&
> surface, SurfaceId surf_id, bool& need_full_ipc) ?

I guess I should follow (or fix) our coding guidelines:

 http://unity.ubuntu.com/mir/cppguide/index.html?showone=Function_Parameter_Ordering#Function_Parameter_Ordering

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1193
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/316/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/97
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/94
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/45/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/316/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1196
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/319/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/97
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/48
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/48/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/48
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/48/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/319/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

LGTM.

Also, tuple unpacking with std::tie! Hurray for learning new C++11isms.

review: Approve

[Daniel van Vugt (vanvugt)]

A nice step in the right direction.

1. Needs fixing: This is definitely a server ABI break. Please bump MIRSERVER_ABI.

2. Suggest fixing: Repeated use of std::tuple will cause significantly higher code bloat and less readability than simply defining a structure and giving it a name. So I'd prefer to see a struct.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> 1. Needs fixing: This is definitely a server ABI break. Please bump MIRSERVER_ABI.

Aren't we bumping the ABI once per development-branch -> lp:mir merge?

> 2. Suggest fixing: Repeated use of std::tuple will cause significantly higher code
> bloat and less readability than simply defining a structure and giving it a name.
> So I'd prefer to see a struct.

It's true that using templates increases the code that needs to be compiled per translation unit. However, in most cases it's code that would have to be written anyway in some form, so even if there is a slight increase in build time, there is significant gain from using code that people are familiar with, instead of using custom code. Plus, modern linkers de-duplicate code for identical template instantiations, so the final executable size increase is usually small.

std::tuple and std::tie, in particular, are templates that have minimal impact (if any), as they just do automatically what you suggest doing manually. For a proof of concept try building: http://paste.ubuntu.com/6358068/

[Alexandros Frantzis (afrantzis)]

review: Approve

[Alan Griffiths (alan-griffiths)]

> A nice step in the right direction.
>
> 1. Needs fixing: This is definitely a server ABI break. Please bump
> MIRSERVER_ABI.

We've been bumping this in a separate MP each time we land development-branch. I'll submit that separately.

> 2. Suggest fixing: Repeated use of std::tuple will cause significantly higher
> code bloat and less readability than simply defining a structure and giving it
> a name. So I'd prefer to see a struct.

I disagree. Writing equivalent functionality by hand will:

1.at best be as efficient as using the standard library at runtime;
2.introduce significant sourcecode bloat; and,
3 eliminate possible link-time optimization if the template is used elsewhere.

[Alan Griffiths (alan-griffiths)]

Resubmitted with an ABI bump as a prerequisite

[kevin gunn (kgunn72)]

WRT

>> 2. Suggest fixing: Repeated use of std::tuple will cause significantly higher
>> code bloat and less readability than simply defining a structure and giving it
>> a name. So I'd prefer to see a struct.
>
>I disagree. Writing equivalent functionality by hand will:
>
>1.at best be as efficient as using the standard library at runtime;
>2.introduce significant sourcecode bloat; and,
>3 eliminate possible link-time optimization if the template is used elsewhere.

and
>It's true that using templates increases the code that needs to be compiled per
>translation unit. However, in most cases it's code that would have to be
>written anyway in some form, so even if there is a slight increase in build
>time, there is significant gain from using code that people are familiar with,
>instead of using custom code. Plus, modern linkers de-duplicate code for
>identical template instantiations, so the final executable size increase
>is usually small.
>
>std::tuple and std::tie, in particular, are templates that have minimal impact
>(if any), as they just do automatically what you suggest doing manually.
>For a proof of concept try building: http://paste.ubuntu.com/6358068/

I believe the team should strive to utilize std objects (not for the sake of it
...but i'm assuming when the need arises).
Not only for the reasons listed above, but also, customizing lends itself
to generating new errors - rather than something provided by the standard that
should be optimized & debugged (and if not it surely will be over time).
as to the comment about naming (i assume wondering...what's it used for?) surely
we could use more comments in the code - i think that would help

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1196
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/330/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/110
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/59/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/59/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/330/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Actually, we've been bumping ABIs inside the offending MPs. Ever since v0.1.0.

[Daniel van Vugt (vanvugt)]

Alan,

In non-trivial cases like that proposed here, using a tuple is more difficult to read:
    std::tuple<std::shared_ptr<mg::Buffer>, bool> somefunction()
vs:
    SomeType somefunction()
but I understand the argument for verbosity and in some cases naming SomeType could create confusion.

Regarding code bloat, I suggest you compile your own example code:
    http://paste.ubuntu.com/6358068/
You will find the resulting binary with -DUSE_TUPLE is double the size.

[Alexandros Frantzis (afrantzis)]

> Regarding code bloat, I suggest you compile your own example code:
> http://paste.ubuntu.com/6358068/
> You will find the resulting binary with -DUSE_TUPLE is double the size.

It's double the size only if we don't use an optimization flag. Even -O1 brings them to the same size.

[Daniel van Vugt (vanvugt)]

That's true, but...

(a) Including debug symbols (-g) as well as optimization, -DUSE_TUPLE can only get to within 132% the size of the struct approach. And generating debug symbols is something all real-world builds will do, including for releases.

(b) Compilation is predictably slower by about 20%.

(c) Needing to use optimization flags is probably not ideal. The best optimizations will always occur before the code sees a compiler.

So it's slower to build, generates bigger code, and is (arguably) harder to read. Such is life with many C++ templates. You just have to use them carefully.

[Alan Griffiths (alan-griffiths)]

On 05/11/13 04:16, Daniel van Vugt wrote:
> In non-trivial cases like that proposed here, using a tuple is more difficult to read:
> std::tuple<std::shared_ptr<mg::Buffer>, bool> somefunction()
> vs:
> SomeType somefunction()
> but I understand the argument for verbosity and in some cases naming SomeType could create confusion.

But that's the wrong comparison. The difference is between

    ... std::tuple<std::shared_ptr<mg::Buffer>, bool> ...

and
    ...class { std::shared_ptr<mg::Buffer> first; bool second; }...

For example:

    std::tuple<std::shared_ptr<mg::Buffer>, bool> somefunction()
vs
    class { std::shared_ptr<mg::Buffer> first; bool second; } somefunction()

In any case, there's a clear rationale for using standard library
facilities where they do the job: they are better known, better
specified and better tested than something custom and new.

Anyone not familiar with std::tuple can look it up easily and apply the
knowledge they gain to future encounters. (They will not meet SomeType
again,)

--
Alan Griffiths [phone]
Octopull Ltd http://www.octopull.co.uk/

=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h	2013-10-15 08:53:10 +0000
+++ include/server/mir/frontend/surface.h	2013-11-04 15:13:14 +0000
@@ -55,12 +55,7 @@
     virtual geometry::Size size() const = 0;
     virtual geometry::PixelFormat pixel_format() const = 0;
 
-    /// Submit the current client buffer, return the next client buffer
-    ///
-    /// \param [out] need_ipc:  True if the buffer content must be sent via IPC
-    ///                         False if only the buffer's ID must be sent.
-    /// \returns                The next client buffer
-    virtual std::shared_ptr<graphics::Buffer> advance_client_buffer(bool& need_full_ipc) = 0;
+    virtual std::shared_ptr<graphics::Buffer> advance_client_buffer() = 0;
 
     virtual bool supports_input() const = 0;
     virtual int client_input_fd() const = 0;
@@ -75,21 +70,7 @@
 
 auto as_internal_surface(std::shared_ptr<Surface> const& surface)
     -> std::shared_ptr<graphics::InternalSurface>;
-
-class ClientTrackingSurface : public Surface
-{
-public:
-    ClientTrackingSurface();
-    virtual ~ClientTrackingSurface() = default;
-    
-    virtual std::shared_ptr<graphics::Buffer> advance_client_buffer(bool& need_full_ipc) override;
-
-    virtual std::shared_ptr<graphics::Buffer> advance_client_buffer() = 0;
-private:
-    std::shared_ptr<ClientBufferTracker> client_tracker;
-};
-
-}   
+}
 }
 
 

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-10-15 08:53:10 +0000
+++ include/server/mir/shell/surface.h	2013-11-04 15:13:14 +0000
@@ -44,7 +44,7 @@
 class SurfaceController;
 struct SurfaceCreationParameters;
 
-class Surface : public frontend::ClientTrackingSurface, public shell::SurfaceBufferAccess
+class Surface : public frontend::Surface, public shell::SurfaceBufferAccess
 {
 public:
     Surface(
@@ -86,8 +86,9 @@
     virtual void set_input_region(std::vector<geometry::Rectangle> const& region);
 
     virtual void allow_framedropping(bool); 
-    
+
     virtual void raise(std::shared_ptr<SurfaceController> const& controller);
+
 private:
     bool set_type(MirSurfaceType t);  // Use configure() to make public changes
     bool set_state(MirSurfaceState s);

=== modified file 'include/test/mir_test_doubles/mock_frontend_surface.h'
--- include/test/mir_test_doubles/mock_frontend_surface.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_frontend_surface.h	2013-11-04 15:13:14 +0000
@@ -29,7 +29,7 @@
 {
 namespace doubles
 {
-struct MockFrontendSurface : public frontend::ClientTrackingSurface
+struct MockFrontendSurface : public frontend::Surface
 {
     MockFrontendSurface()
     {

=== modified file 'include/test/mir_test_doubles/stub_surface.h'
--- include/test/mir_test_doubles/stub_surface.h	2013-10-15 08:53:10 +0000
+++ include/test/mir_test_doubles/stub_surface.h	2013-11-04 15:13:14 +0000
@@ -28,7 +28,7 @@
 namespace doubles
 {
 
-class StubSurface : public frontend::ClientTrackingSurface
+class StubSurface : public frontend::Surface
 {
 public:
     virtual ~StubSurface() = default;

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-10-25 08:41:50 +0000
+++ src/server/frontend/session_mediator.cpp	2013-11-04 15:13:14 +0000
@@ -17,6 +17,8 @@
  */
 
 #include "session_mediator.h"
+#include "client_buffer_tracker.h"
+
 #include "mir/frontend/session_mediator_report.h"
 #include "mir/frontend/shell.h"
 #include "mir/frontend/session.h"
@@ -92,7 +94,7 @@
         std::unique_lock<std::mutex> lock(session_mutex);
         weak_session = shell->open_session(request->application_name(), event_sink);
     }
-    
+
     auto ipc_package = graphics_platform->get_ipc_package();
     auto platform = response->mutable_platform();
 
@@ -111,28 +113,50 @@
     done->Run();
 }
 
+std::tuple<std::shared_ptr<mg::Buffer>, bool>
+mf::SessionMediator::advance_buffer(SurfaceId surf_id, Surface& surface)
+{
+    auto& tracker = client_buffer_tracker[surf_id];
+    if (!tracker) tracker = std::make_shared<ClientBufferTracker>(client_buffer_cache_size);
+
+    auto client_buffer = surface.advance_client_buffer();
+    auto id = client_buffer->id();
+    auto need_full_ipc = !tracker->client_has(id);
+    tracker->add(id);
+
+    client_buffer_resource[surf_id] = client_buffer;
+
+    return std::tie(client_buffer, need_full_ipc);
+}
+
+
 void mf::SessionMediator::create_surface(
     google::protobuf::RpcController* /*controller*/,
     const mir::protobuf::SurfaceParameters* request,
     mir::protobuf::Surface* response,
     google::protobuf::Closure* done)
 {
-    std::unique_lock<std::mutex> lock(session_mutex);
-
-    auto session = weak_session.lock();
-    
-    if (session.get() == nullptr)
-        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-    
-    report->session_create_surface_called(session->name());
-
-    auto const surf_id = session->create_surface(msh::SurfaceCreationParameters()
-        .of_name(request->surface_name())
-        .of_size(request->width(), request->height())
-        .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
-        .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
-        .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
+    bool need_full_ipc;
+    std::shared_ptr<graphics::Buffer> client_buffer;
+    std::shared_ptr<Session> session;
+
     {
+        std::unique_lock<std::mutex> lock(session_mutex);
+
+        session = weak_session.lock();
+
+        if (session.get() == nullptr)
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+        report->session_create_surface_called(session->name());
+
+        auto const surf_id = session->create_surface(msh::SurfaceCreationParameters()
+            .of_name(request->surface_name())
+            .of_size(request->width(), request->height())
+            .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
+            .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
+            .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
+
         auto surface = session->get_surface(surf_id);
         response->mutable_id()->set_value(surf_id.as_value());
         response->set_width(surface->size().width.as_uint32_t());
@@ -143,16 +167,14 @@
         if (surface->supports_input())
             response->add_fd(surface->client_input_fd());
 
-        bool need_full_ipc;
-        auto client_buffer = surface->advance_client_buffer(need_full_ipc);
-        client_buffer_resource[surf_id] = client_buffer;
-
-        auto buffer = response->mutable_buffer();
-        pack_protobuf_buffer(*buffer, client_buffer, need_full_ipc);
+        std::tie(client_buffer, need_full_ipc) = advance_buffer(surf_id, *surface);
     }
 
+    auto buffer = response->mutable_buffer();
+    pack_protobuf_buffer(*buffer, client_buffer, need_full_ipc);
+
     // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
-    // This guarantees that notifications such as, gained focus, etc, can be correctly interpreted by the client. 
+    // This guarantees that notifications such as, gained focus, etc, can be correctly interpreted by the client.
     // To achieve this order we rely on done->Run() sending messages synchronously. As documented in mfd::SocketMessenger::send.
     // this will require additional synchronization if mfd::SocketMessenger::send changes.
     done->Run();
@@ -171,14 +193,14 @@
 
     {
         std::unique_lock<std::mutex> lock(session_mutex);
-        
+
         auto session = weak_session.lock();
 
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
 
         report->session_next_buffer_called(session->name());
-        
+
         // We ensure the client has not powered down the outputs, so that
         // swap_buffer will not block indefinitely, leaving the client
         // in a position where it can not turn back on the
@@ -187,9 +209,7 @@
 
         auto surface = session->get_surface(surf_id);
 
-        client_buffer_resource[surf_id].reset();
-        client_buffer = surface->advance_client_buffer(need_full_ipc);
-        client_buffer_resource[surf_id] = client_buffer;
+        std::tie(client_buffer, need_full_ipc) = advance_buffer(surf_id, *surface);
     }
 
     pack_protobuf_buffer(*response, client_buffer, need_full_ipc);
@@ -216,6 +236,7 @@
         auto const id = SurfaceId(request->value());
 
         session->destroy_surface(id);
+        client_buffer_tracker.erase(id);
     }
 
     // TODO: We rely on this sending responses synchronously.
@@ -294,7 +315,7 @@
 
         auto config = display_changer->active_configuration();
         for (auto i=0; i < request->display_output_size(); i++)
-        {   
+        {
             auto& output = request->display_output(i);
             mg::DisplayConfigurationOutputId output_id{static_cast<int>(output.output_id())};
             config->configure_output(output_id, output.used(),

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2013-10-24 14:55:07 +0000
+++ src/server/frontend/session_mediator.h	2013-11-04 15:13:14 +0000
@@ -42,11 +42,12 @@
 /// processes and the core of the mir system.
 namespace frontend
 {
+class ClientBufferTracker;
 class Shell;
 class Session;
+class Surface;
 class ResourceCache;
 class SessionMediatorReport;
-class ClientBufferTracker;
 class EventSink;
 class DisplayChanger;
 
@@ -113,6 +114,7 @@
                               std::shared_ptr<graphics::Buffer> const& graphics_buffer,
                               bool need_full_ipc);
 
+    std::tuple<std::shared_ptr<graphics::Buffer>, bool> advance_buffer(SurfaceId surf_id, Surface& surface);
     std::shared_ptr<Shell> const shell;
     std::shared_ptr<graphics::Platform> const graphics_platform;
 
@@ -125,6 +127,7 @@
     std::shared_ptr<ResourceCache> const resource_cache;
 
     std::unordered_map<SurfaceId,std::shared_ptr<graphics::Buffer>> client_buffer_resource;
+    std::unordered_map<SurfaceId, std::shared_ptr<ClientBufferTracker>> client_buffer_tracker;
 
     std::mutex session_mutex;
     std::weak_ptr<Session> weak_session;

=== modified file 'src/server/frontend/surface.cpp'
--- src/server/frontend/surface.cpp	2013-08-28 03:41:48 +0000
+++ src/server/frontend/surface.cpp	2013-11-04 15:13:14 +0000
@@ -39,8 +39,7 @@
     private:
         virtual std::shared_ptr<mg::Buffer> advance_client_buffer()
         {
-            bool unused;
-            return surface->advance_client_buffer(unused);
+            return surface->advance_client_buffer();
         }
         virtual mir::geometry::Size size() const { return surface->size(); }
         virtual MirPixelFormat pixel_format() const { return static_cast<MirPixelFormat>(surface->pixel_format()); }
@@ -50,17 +49,3 @@
 
     return std::make_shared<ForwardingInternalSurface>(surface);
 }
-
-mf::ClientTrackingSurface::ClientTrackingSurface()
-    : client_tracker(std::make_shared<mf::ClientBufferTracker>(mf::client_buffer_cache_size))
-{
-}
-
-std::shared_ptr<mg::Buffer> mf::ClientTrackingSurface::advance_client_buffer(bool& need_full_ipc)
-{
-    auto buffer = advance_client_buffer();
-    auto id = buffer->id();
-    need_full_ipc = !client_tracker->client_has(id);
-    client_tracker->add(id);
-    return buffer;
-}