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

[Kevin DuBois]

android: disable overlays when a swapinterval 0 client is part of the RenderableList. This won't affect the overlay percentage for usc/u8, as u8 runs swapinterval 1.

mg::Renderable providing swapinterval information is useful to android to fix this bug, as well as for nested-passthrough, where nested-passthrough has to coordinate the passthrough surface with the host server, so the host server doesn't end up grabbing all the buffers from the passthrough surface.

platform abi number was already increased for the 0.25 series.

Fixes: LP: #1369763

android: disable overlays when a swapinterval 0 client is part of the RenderableList. This won't affect the overlay percentage for usc/u8, as u8 runs swapinterval 1.

mg::Renderable providing swapinterval information is useful to android to fix this bug, as well as for nested-passthrough, where nested-passthrough has to coordinate the passthrough surface with the host server, so the host server doesn't end up grabbing all the buffers from the passthrough surface.

platform abi number was already increased for the 0.25 series.

Fixes: LP: #1369763

[Kevin DuBois (kdub)]

This can go in the 'why didn't I think of this before" pile.

[Alexandros Frantzis (afrantzis)]

Looks good, except for:

1. a left over debugging comment:

+ printf("SET SWAPINTERVAL %i\n", interval);

2. a clang build error (and perhaps there are others):

src/platforms/android/utils/render_overlays.cpp:145:18: error: 'swap_interval' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

review: Needs Fixing

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3744
https://mir-jenkins.ubuntu.com/job/mir-ci/1911/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2426/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2489
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2482
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2482
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2482
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2456/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2456
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2456/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2456
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2456/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2456
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2456/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2456
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2456/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2456
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2456/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1911/rebuild

review: Needs Fixing (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

Ok

review: Approve

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3747
https://mir-jenkins.ubuntu.com/job/mir-ci/1922/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2438
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2501
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2467/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2467/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2467/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2467/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2467/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2467
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2467/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1922/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Thanks for looking at this. Just wondering; if you're disabling overlays for anything that's interval 0 on the server side and we hope to make everything on the server side interval 0 (when interval 1 becomes a function of the libmirclient), then won't we need to revert this? Would it not just be a better idea to leave the bug unresolved until a final solution is feasible?

By landing this now, we create a regression that's worse than the bug itself. That is a future mirserver will never overlay unless we remember to also revert this change.

review: Needs Information

[Kevin DuBois (kdub)]

> Thanks for looking at this. Just wondering; if you're disabling overlays for
> anything that's interval 0 on the server side and we hope to make everything
> on the server side interval 0 (when interval 1 becomes a function of the
> libmirclient), then won't we need to revert this? Would it not just be a
> better idea to leave the bug unresolved until a final solution is feasible?
>

Once we have dropping-only, the server won't really have a concept of 'swapinterval' anymore (which makes it simpler there). We will probably be best to remove the Renderable::swap_interval information.

> By landing this now, we create a regression that's worse than the bug itself.
> That is a future mirserver will never overlay unless we remember to also
> revert this change.

Its not a regression once it lands, its only a regression if we forget how this works when we change the submission mode in the future. I can keep tabs on it so that when we switch to dropping-only, we will change the behavior. Nested passthrough's behavior will have to change even more when that part of the system is reworked.

Its useful now for fixing android support for this option, and the interval information is needed for nested passthrough to prevent loss of swapinterval 0 on passthrough clients.

[Brandon Schaefer (brandontschaefer)]

lgtm

review: Approve

[Kevin DuBois (kdub)]

subsequent branches showing how the swapinterval() function are used in nested pasthrough are up.

[Daniel van Vugt (vanvugt)]

Alright

review: Approve

=== modified file 'include/platform/mir/graphics/renderable.h'
--- include/platform/mir/graphics/renderable.h	2015-04-28 07:54:10 +0000
+++ include/platform/mir/graphics/renderable.h	2016-10-07 18:58:00 +0000
@@ -70,6 +70,7 @@
 
     virtual bool shaped() const = 0;  // meaning the pixel format has alpha
 
+    virtual unsigned int swap_interval() const = 0;
 protected:
     Renderable() = default;
     Renderable(Renderable const&) = delete;
 
=== modified file 'src/include/server/mir/compositor/buffer_stream.h'
--- src/include/server/mir/compositor/buffer_stream.h	2016-05-03 06:55:25 +0000
+++ src/include/server/mir/compositor/buffer_stream.h	2016-10-07 18:58:00 +0000
@@ -50,6 +50,7 @@
     virtual int buffers_ready_for_compositor(void const* user_id) const = 0;
     virtual void drop_old_buffers() = 0;
     virtual bool has_submitted_buffer() const = 0;
+    virtual bool framedropping() const = 0;
 };
 
 }
 
=== modified file 'src/platform/symbols.map'
--- src/platform/symbols.map	2016-09-19 06:09:48 +0000
+++ src/platform/symbols.map	2016-10-07 18:58:00 +0000
@@ -90,6 +90,7 @@
     mir::graphics::Renderable::screen_position*;
     mir::graphics::Renderable::shaped*;
     mir::graphics::Renderable::transformation*;
+    mir::graphics::Renderable::swap_interval*;
     mir::graphics::UserDisplayConfigurationOutput::extents*;
     mir::graphics::UserDisplayConfigurationOutput::UserDisplayConfigurationOutput*;
     mir::options::arw_server_socket_opt*;
 
=== modified file 'src/platforms/android/server/hwc_device.cpp'
--- src/platforms/android/server/hwc_device.cpp	2016-05-03 06:55:25 +0000
+++ src/platforms/android/server/hwc_device.cpp	2016-10-07 18:58:00 +0000
@@ -56,7 +56,8 @@
         //TODO: enable planeAlpha for (hwc version >= 1.2), 90 deg rotation
         static glm::mat4 const identity;
         if (plane_alpha_is_translucent(*renderable) ||
-           (renderable->transformation() != identity))
+           (renderable->transformation() != identity) ||
+           (renderable->swap_interval() == 0)) //See LP: #1369763
         {
             return false;
         }
 
=== modified file 'src/platforms/android/utils/render_overlays.cpp'
--- src/platforms/android/utils/render_overlays.cpp	2016-09-19 06:09:48 +0000
+++ src/platforms/android/utils/render_overlays.cpp	2016-10-07 18:58:00 +0000
@@ -142,6 +142,11 @@
     {
     }
 
+    unsigned int swap_interval() const override
+    {
+        return 1u;
+    }
+
     ID id() const override
     {
         return this;
 
=== modified file 'src/platforms/android/utils/test_android_hardware_sanity.cpp'
--- src/platforms/android/utils/test_android_hardware_sanity.cpp	2016-09-19 06:09:48 +0000
+++ src/platforms/android/utils/test_android_hardware_sanity.cpp	2016-10-07 18:58:00 +0000
@@ -240,6 +240,10 @@
             buffer_(buffer)
         {
         }
+        unsigned int swap_interval() const override
+        {
+            return 1u;
+        }
         ID id() const override
         {
             return this;
 
=== modified file 'src/server/compositor/stream.cpp'
--- src/server/compositor/stream.cpp	2016-09-22 11:38:46 +0000
+++ src/server/compositor/stream.cpp	2016-10-07 18:58:00 +0000
@@ -164,6 +164,11 @@
     }
 }
 
+bool mc::Stream::framedropping() const
+{
+    return schedule_mode == ScheduleMode::Dropping;
+}
+
 void mc::Stream::transition_schedule(
     std::shared_ptr<mc::Schedule>&& new_schedule, std::lock_guard<std::mutex> const&)
 {
 
=== modified file 'src/server/compositor/stream.h'
--- src/server/compositor/stream.h	2016-09-22 11:38:46 +0000
+++ src/server/compositor/stream.h	2016-10-07 18:58:00 +0000
@@ -57,6 +57,7 @@
     geometry::Size stream_size() override;
     void resize(geometry::Size const& size) override;
     void allow_framedropping(bool) override;
+    bool framedropping() const override;
     void drop_outstanding_requests() override;
     int buffers_ready_for_compositor(void const* user_id) const override;
     void drop_old_buffers() override;
 
=== modified file 'src/server/graphics/software_cursor.cpp'
--- src/server/graphics/software_cursor.cpp	2016-09-15 13:44:43 +0000
+++ src/server/graphics/software_cursor.cpp	2016-10-07 18:58:00 +0000
@@ -65,6 +65,11 @@
     {
     }
 
+    unsigned int swap_interval() const override
+    {
+        return 1;
+    }
+
     mg::Renderable::ID id() const override
     {
         return this;
 
=== modified file 'src/server/input/touchspot_controller.cpp'
--- src/server/input/touchspot_controller.cpp	2016-08-16 19:34:28 +0000
+++ src/server/input/touchspot_controller.cpp	2016-10-07 18:58:00 +0000
@@ -52,6 +52,12 @@
     }
 
 // mg::Renderable
+
+    unsigned int swap_interval() const override
+    {
+        return 1;
+    }
+
     mg::Renderable::ID id() const override
     {
         return this;
 
=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2016-09-19 04:16:15 +0000
+++ src/server/scene/basic_surface.cpp	2016-10-07 18:58:00 +0000
@@ -825,6 +825,11 @@
     ~SurfaceSnapshot()
     {
     }
+
+    unsigned int swap_interval() const override
+    {
+        return underlying_buffer_stream->framedropping() ? 0 : 1;
+    }
  
     std::shared_ptr<mg::Buffer> buffer() const override
     {
 
=== modified file 'tests/include/mir/test/doubles/fake_renderable.h'
--- tests/include/mir/test/doubles/fake_renderable.h	2015-04-28 07:54:10 +0000
+++ tests/include/mir/test/doubles/fake_renderable.h	2016-10-07 18:58:00 +0000
@@ -95,6 +95,11 @@
         return rect;
     }
 
+    unsigned int swap_interval() const override
+    {
+        return 1u;
+    }
+
 private:
     std::shared_ptr<graphics::Buffer> buf;
     mir::geometry::Rectangle rect;
 
=== modified file 'tests/include/mir/test/doubles/mock_buffer_stream.h'
--- tests/include/mir/test/doubles/mock_buffer_stream.h	2016-08-16 19:34:28 +0000
+++ tests/include/mir/test/doubles/mock_buffer_stream.h	2016-10-07 18:58:00 +0000
@@ -69,6 +69,7 @@
     MOCK_METHOD1(resize, void(geometry::Size const&));
     MOCK_METHOD0(force_client_completion, void());
     MOCK_METHOD1(allow_framedropping, void(bool));
+    MOCK_CONST_METHOD0(framedropping, bool());
 
     MOCK_METHOD0(drop_outstanding_requests, void());
     MOCK_CONST_METHOD1(buffers_ready_for_compositor, int(void const*));
 
=== modified file 'tests/include/mir/test/doubles/mock_renderable.h'
--- tests/include/mir/test/doubles/mock_renderable.h	2015-06-25 03:00:08 +0000
+++ tests/include/mir/test/doubles/mock_renderable.h	2016-10-07 18:58:00 +0000
@@ -52,6 +52,7 @@
     MOCK_CONST_METHOD0(transformation, glm::mat4());
     MOCK_CONST_METHOD0(visible, bool());
     MOCK_CONST_METHOD0(shaped, bool());
+    MOCK_CONST_METHOD0(swap_interval, unsigned int());
 };
 }
 }
 
=== modified file 'tests/include/mir/test/doubles/stub_buffer_stream.h'
--- tests/include/mir/test/doubles/stub_buffer_stream.h	2016-09-19 04:16:15 +0000
+++ tests/include/mir/test/doubles/stub_buffer_stream.h	2016-10-07 18:58:00 +0000
@@ -63,7 +63,10 @@
     void allow_framedropping(bool) override
     {
     }
-
+    bool framedropping() const override
+    {
+        return false;
+    }
     int buffers_ready_for_compositor(void const*) const override { return nready; }
 
     void drop_old_buffers() override {}
 
=== modified file 'tests/include/mir/test/doubles/stub_renderable.h'
--- tests/include/mir/test/doubles/stub_renderable.h	2016-09-08 18:59:56 +0000
+++ tests/include/mir/test/doubles/stub_renderable.h	2016-10-07 18:58:00 +0000
@@ -85,6 +85,10 @@
     {
         return false;
     }
+    unsigned int swap_interval() const override
+    {
+        return 1;
+    }
 
 private:
     std::shared_ptr<graphics::Buffer> make_stub_buffer(geometry::Rectangle const& rect)
@@ -146,6 +150,15 @@
         return 1.0f - ( 3.0f / 1024.0f );
     }
 };
+
+struct IntervalZeroRenderable : StubRenderable
+{
+    unsigned int swap_interval() const override
+    {
+        return 0;
+    }
+};
+
 }
 }
 }
 
=== modified file 'tests/unit-tests/platforms/android/server/test_hwc_device.cpp'
--- tests/unit-tests/platforms/android/server/test_hwc_device.cpp	2016-05-03 06:55:25 +0000
+++ tests/unit-tests/platforms/android/server/test_hwc_device.cpp	2016-10-07 18:58:00 +0000
@@ -599,6 +599,17 @@
     EXPECT_FALSE(device.compatible_renderlist(renderlist));
 }
 
+//LP: #1369763. We could get swapinterval 0 to work with overlays, but we'd need
+//the vsync signal from hwc to reach the client, so the client can rate-limit its submissions.
+TEST_F(HwcDevice, rejects_list_containing_interval_0)
+{
+    mga::HwcDevice device(mock_device);
+
+    auto renderable = std::make_shared<mtd::StubTransformedRenderable>();
+    mg::RenderableList renderlist{renderable};
+    EXPECT_FALSE(device.compatible_renderlist(renderlist));
+}
+
 //TODO: we could accept a 90 degree transform
 TEST_F(HwcDevice, rejects_list_containing_transformed)
 {