Merge lp:~kdub/mir/nbs-option into lp:mir

[Kevin DuBois]

add an option to enable the new buffer semantics under the hood, by setting "--nbuffers 0"

add an option to enable the new buffer semantics under the hood, by setting "--nbuffers 0".

There are still a few optimizations (overallocation, timeout-dropping) and performance studies that need to be added before switching over entirely, but its useful to have this option around to play with the new system. Titlebars on the demo server are also not drawn yet, have to transition from surface-based titlebars.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2906
http://jenkins.qa.ubuntu.com/job/mir-ci/5178/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4322
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3229
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4264
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/222/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1332
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1332/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/222/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4265
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4265/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6924
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24007
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/223/console

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

review: Approve (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

- if (nbuffers < 2)
- throw std::logic_error("nbuffers must be at least 2");

Should we at lease assert nbuffers >= 0? (Or throw if its lower then 0)

Otherwise LGTM

review: Approve

[Daniel van Vugt (vanvugt)]

Nice transitional option. Although nbuffers==1 will probably break things so better fix that error checking.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

What does the EventSink => BufferSink change have to do with adding the option?

I'm not sure that "--nbuffers 0" is an obvious way to make this choice. Is this a transitional mechanism or will it remain around?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2909
http://jenkins.qa.ubuntu.com/job/mir-ci/5203/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4356
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3263
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4298
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/256/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1357
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1357/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/256/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4299
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4299/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6954
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24047
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/257/console

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

You could split out the EventSink → BufferSink change (I agree that it's correct and sensible).

Under the assumption that --nbuffers is a transitional option that will go away entirely once NBS is the supported buffer mechanism, +1.

review: Approve

[Daniel van Vugt (vanvugt)]

Thanks for fixing that.

(1) The EventSink->BufferSink change is syntactically required only because BufferMap is constructed in the absence of the former. But I find it odd they're apparently compatible, while not type compatible though. Can we separate that or find a way to give the right EventSink to BufferMap?

(2) I think --nbuffers will linger for a while yet. We'll need it as a fallback even when using NBS in production, because we need to be able to compare functionality and performance for a while. The timeframe for large architectural changes like this seems to be around a year; which is the average time we seem to take to flesh out all the significant regressions from any large change. Using the repo history unfortunately is no longer an alternative to this. Because we keep breaking backward compatibility, you can't rely on an old copy of the Mir source code to be buildable in future distros. So instead, keep the old functionality in the current source code.

review: Needs Information

[Kevin DuBois (kdub)]

With the EventSink to BufferSink change, this is some correction that should have happened in rev2765 when EventSink was kept private, and BufferSink was made a public header. Its better to give BufferMap a BufferSink then to change the factory to take an EventSink, as all the BufferMap needs is the BufferSink interfaces.

--nbuffers is a transitional option useful to testing that will go away eventually. It will (probably) be around for a release or three while we ensure the new system is up-to-par with the old.

[Alan Griffiths (alan-griffiths)]

OK. (I'm still not convinced "--nbuffers 0" is a good name for this but it isn't worth blocking over.)

review: Approve

[Daniel van Vugt (vanvugt)]

I agree with Alan that it would be more self explanatory as something like:
  --bufferqueue=0 (meaning BufferQueue is disabled)

However we are bound to the existing option syntax. Better to have something microscopically confusing than to break backward-compatibility too much.

review: Approve

=== modified file 'src/server/compositor/buffer_map.cpp'
--- src/server/compositor/buffer_map.cpp	2015-08-05 19:01:39 +0000
+++ src/server/compositor/buffer_map.cpp	2015-10-07 18:29:25 +0000
@@ -17,7 +17,7 @@
  */
 
 #include "mir/graphics/graphic_buffer_allocator.h"
-#include "mir/frontend/event_sink.h"
+#include "mir/frontend/buffer_sink.h"
 #include "buffer_map.h"
 #include <boost/throw_exception.hpp>
 
@@ -27,7 +27,7 @@
 
 mc::BufferMap::BufferMap(
     mf::BufferStreamId id,
-    std::shared_ptr<mf::EventSink> const& sink,
+    std::shared_ptr<mf::BufferSink> const& sink,
     std::shared_ptr<mg::GraphicBufferAllocator> const& allocator) :
     stream_id(id),
     sink(sink),

=== modified file 'src/server/compositor/buffer_map.h'
--- src/server/compositor/buffer_map.h	2015-08-05 19:01:39 +0000
+++ src/server/compositor/buffer_map.h	2015-10-07 18:29:25 +0000
@@ -27,7 +27,7 @@
 namespace mir
 {
 namespace graphics { class GraphicBufferAllocator; }
-namespace frontend { class EventSink; }
+namespace frontend { class BufferSink; }
 namespace compositor
 {
 class BufferMap : public frontend::ClientBuffers
@@ -35,7 +35,7 @@
 public:
     BufferMap(
         frontend::BufferStreamId id,
-        std::shared_ptr<frontend::EventSink> const& sink,
+        std::shared_ptr<frontend::BufferSink> const& sink,
         std::shared_ptr<graphics::GraphicBufferAllocator> const& allocator);
 
     graphics::BufferID add_buffer(graphics::BufferProperties const& properties) override;
@@ -51,7 +51,7 @@
     Map::iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&);
 
     frontend::BufferStreamId const stream_id;
-    std::shared_ptr<frontend::EventSink> const sink;
+    std::shared_ptr<frontend::BufferSink> const sink;
     std::shared_ptr<graphics::GraphicBufferAllocator> const allocator;
 };
 }

=== modified file 'src/server/compositor/buffer_stream_factory.cpp'
--- src/server/compositor/buffer_stream_factory.cpp	2015-07-22 02:54:31 +0000
+++ src/server/compositor/buffer_stream_factory.cpp	2015-10-07 18:29:25 +0000
@@ -22,6 +22,8 @@
 #include "buffer_stream_surfaces.h"
 #include "mir/graphics/buffer_properties.h"
 #include "buffer_queue.h"
+#include "stream.h"
+#include "buffer_map.h"
 #include "mir/graphics/buffer.h"
 #include "mir/graphics/buffer_id.h"
 #include "mir/graphics/graphic_buffer_allocator.h"
@@ -45,8 +47,8 @@
 {
     assert(gralloc);
     assert(policy_factory);
-    if (nbuffers < 2)
-        throw std::logic_error("nbuffers must be at least 2");
+    if (nbuffers == 1)
+        throw std::logic_error("nbuffers cannot be 1");
 }
 
 
@@ -58,10 +60,19 @@
 }
 
 std::shared_ptr<mc::BufferStream> mc::BufferStreamFactory::create_buffer_stream(
-    mf::BufferStreamId, std::shared_ptr<mf::BufferSink> const&,
+    mf::BufferStreamId id, std::shared_ptr<mf::BufferSink> const& sink,
     int nbuffers, mg::BufferProperties const& buffer_properties)
 {
-    auto switching_bundle = std::make_shared<mc::BufferQueue>(
-        nbuffers, gralloc, buffer_properties, *policy_factory);
-    return std::make_shared<mc::BufferStreamSurfaces>(switching_bundle);
+    if (nbuffers == 0)
+    {
+        return std::make_shared<mc::Stream>(
+            std::make_unique<mc::BufferMap>(id, sink, gralloc),
+            buffer_properties.size, buffer_properties.format);
+    }
+    else
+    {
+        auto switching_bundle = std::make_shared<mc::BufferQueue>(
+            nbuffers, gralloc, buffer_properties, *policy_factory);
+        return std::make_shared<mc::BufferStreamSurfaces>(switching_bundle);
+    }
 }