Merge lp:~kdub/mir/fix-1413211 into lp:mir

[Kevin DuBois]

Ensure that mg::Buffer::write won't block when writing the SoftwareCursor by allocating a fresh buffer each time an update is needed.

fixes: LP: #1413211

Ensure that mg::Buffer::write won't block when writing the SoftwareCursor by allocating a fresh buffer each time an update is needed.

fixes: LP: #1413211

This seemed like the least obtrusive fix for this problem. Basically, the mg::Buffer::write() to update pixel contents was blocking because the cursor was always onscreen, and there wasn't an opportunity to write to the buffer.

[Daniel van Vugt (vanvugt)]

The same bugs exist for touchspots and other overlays. Shouldn't we be fixing this in some common code?

The bug description is certainly generalised, so if we do just fix it for software cursor, then this is not a fix for the attached bug. Call it a partial workaround and keep the bug open?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2293
http://jenkins.qa.ubuntu.com/job/mir-ci/2908/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1204
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1203
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/905
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/905/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1161
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1161/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4206
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17760

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

The compositor jerkiness when touchspots are on (lp: #1373692) and this bug (lp: #1413211) are two different root causes. This one is a lockup because another part of the system tries to write to a buffer that is onscreen, and the android code disallows this to avoid tearing on the cursor image. The other bug's root cause is more that certain android devices don't have any fences or waits that stop the composition threads from spiking if we're trying to post the exact same thing twice in a row.

[Alexandros Frantzis (afrantzis)]

Looks good.

Also tested on Mesa/intel.

review: Approve

[Daniel van Vugt (vanvugt)]

review: Abstain

[Alan Griffiths (alan-griffiths)]

Maybe I'm just not attuned to this bit of the code but this doesn't feel elegant.

But I don't have another idea ready.

review: Approve

=== modified file 'src/server/graphics/software_cursor.cpp'
--- src/server/graphics/software_cursor.cpp	2015-01-21 09:03:53 +0000
+++ src/server/graphics/software_cursor.cpp	2015-02-04 14:35:56 +0000
@@ -127,8 +127,11 @@
     // Do a lock dance to make this function threadsafe,
     // while avoiding calling scene methods under lock
     {
+        geom::Point position{0,0};
         std::lock_guard<std::mutex> lg{guard};
-        new_renderable = create_renderable_for(cursor_image);
+        if (renderable)
+            position = renderable->screen_position().top_left;
+        new_renderable = create_renderable_for(cursor_image, position);
     }
 
     // Add the new renderable first, then remove the old one to avoid
@@ -148,26 +151,11 @@
 }
 
 std::shared_ptr<mg::detail::CursorRenderable>
-mg::SoftwareCursor::create_renderable_for(CursorImage const& cursor_image)
+mg::SoftwareCursor::create_renderable_for(CursorImage const& cursor_image, geom::Point position)
 {
-    std::shared_ptr<detail::CursorRenderable> new_renderable;
-
-    // Reuse buffer if possible, to minimize buffer reallocations
-    if (renderable && renderable->buffer()->size() == cursor_image.size())
-    {
-        new_renderable = std::make_shared<detail::CursorRenderable>(
-            renderable->buffer(),
-            renderable->screen_position().top_left + hotspot -
-                cursor_image.hotspot());
-    }
-    else
-    {
-        auto const buffer = allocator->alloc_buffer(
-            {cursor_image.size(), format, mg::BufferUsage::software});
-        new_renderable = std::make_shared<detail::CursorRenderable>(
-            buffer,
-            geom::Point{0,0} - cursor_image.hotspot());
-    }
+    auto new_renderable = std::make_shared<detail::CursorRenderable>(
+        allocator->alloc_buffer({cursor_image.size(), format, mg::BufferUsage::software}),
+        position + hotspot - cursor_image.hotspot());
 
     size_t const pixels_size =
         cursor_image.size().width.as_uint32_t() *

=== modified file 'src/server/graphics/software_cursor.h'
--- src/server/graphics/software_cursor.h	2015-01-13 13:23:21 +0000
+++ src/server/graphics/software_cursor.h	2015-02-04 14:35:56 +0000
@@ -51,7 +51,7 @@
 
 private:
     std::shared_ptr<detail::CursorRenderable> create_renderable_for(
-        CursorImage const& cursor_image);
+        CursorImage const& cursor_image, geometry::Point position);
 
     std::shared_ptr<GraphicBufferAllocator> const allocator;
     std::shared_ptr<input::Scene> const scene;

=== modified file 'tests/unit-tests/graphics/test_software_cursor.cpp'
--- tests/unit-tests/graphics/test_software_cursor.cpp	2015-01-13 13:23:21 +0000
+++ tests/unit-tests/graphics/test_software_cursor.cpp	2015-02-04 14:35:56 +0000
@@ -274,3 +274,23 @@
 
     cursor.show(another_stub_cursor_image);
 }
+
+//lp: #1413211
+TEST_F(SoftwareCursor, new_buffer_on_each_show)
+{
+    struct MockBufferAllocator : public mg::GraphicBufferAllocator
+    {
+        MOCK_METHOD1(alloc_buffer, std::shared_ptr<mg::Buffer>(mg::BufferProperties const&));
+        std::vector<MirPixelFormat> supported_pixel_formats() { return {mir_pixel_format_abgr_8888}; } 
+    } mock_allocator;
+
+    EXPECT_CALL(mock_allocator, alloc_buffer(testing::_))
+        .Times(3)
+        .WillRepeatedly(testing::Return(std::make_shared<mtd::StubBuffer>()));;
+    mg::SoftwareCursor cursor{
+        mt::fake_shared(mock_allocator),
+        mt::fake_shared(mock_input_scene)};
+    cursor.show(another_stub_cursor_image);
+    cursor.show(another_stub_cursor_image);
+    cursor.show(stub_cursor_image);
+}