Merge lp:~vanvugt/mir/shape into lp:mir

[Daniel van Vugt]

Ensure nonrectangular surfaces are never bypassed, even if they are
fullscreen and on top (e.g. Unity8 shell at present).
(LP: #1236264)

More importantly, the shape() method is required to fix bug 1227739 on the phone, soon.

[Alan Griffiths (alan-griffiths)]

145 + virtual bool shaped() const = 0; // meaning the pixel format has alpha

When comments and code disagree: trust neither!

Surely we can come up with clearer terminology?

[Alan Griffiths (alan-griffiths)]

I think there's some conflation of shaped/alpha/opacity but otherwise seems to work as advertised.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1117
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/120/
Executed test runs:
    SUCCESS: http://10.97.0.26:8080/job/mir-android-saucy-i386-build/2270
    SUCCESS: http://10.97.0.26:8080/job/mir-clang-saucy-amd64-build/2155
    SUCCESS: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/117
        deb: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/117/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/120/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

To me 'nonrectangular' doesn't immediately mean to me 'surface has alpha channel', (alpha_masked(), perhaps)? but its ok.

review: Approve

[Daniel van Vugt (vanvugt)]

I agree the terminology is confusing. Present terminology:
Mir alpha() == Compiz opacity()
Mir shaped() == Compiz alpha()

Since they're distinctly different things, and both related to alpha blending, I suggest not mentioning "alpha" at all. Hence bug 1236224.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/31/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2279
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2164
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-autolanding/30/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

The failure is a bug which seems to already exist on the target branch. I just reproduced it using development-branch.

It has nothing to do with this proposal. Retry...

[Daniel van Vugt (vanvugt)]

The failure is now logged as bug 1236698.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2013-09-19 13:24:22 +0000
+++ examples/eglapp.c	2013-10-07 10:02:32 +0000
@@ -26,6 +26,8 @@
 
 #include <xkbcommon/xkbcommon-keysyms.h>
 
+float mir_eglapp_background_opacity = 1.0f;
+
 static const char appname[] = "egldemo";
 
 static MirConnection *connection;
@@ -182,6 +184,26 @@
             {
                 switch (arg[1])
                 {
+                case 'b':
+                    {
+                        float alpha = 1.0f;
+                        arg += 2;
+                        if (!arg[0] && i < argc-1)
+                        {
+                            i++;
+                            arg = argv[i];
+                        }
+                        if (sscanf(arg, "%f", &alpha) == 1)
+                        {
+                            mir_eglapp_background_opacity = alpha;
+                        }
+                        else
+                        {
+                            printf("Invalid opacity value: %s\n", arg);
+                            help = 1;
+                        }
+                    }
+                    break;
                 case 'n':
                     swapinterval = 0;
                     break;
@@ -247,6 +269,7 @@
             if (help)
             {
                 printf("Usage: %s [<options>]\n"
+                       "  -b               Background opacity (0.0 - 1.0)\n"
                        "  -h               Show this help text\n"
                        "  -f               Force full screen\n"
                        "  -o ID            Force placement on output monitor ID\n"
@@ -277,9 +300,27 @@
 
     const MirDisplayMode *mode = &output->modes[output->current_mode];
 
-    unsigned int valid_formats;
+    const unsigned int max_formats = 10;
+    unsigned int format[max_formats];
+    unsigned int nformats;
+
     mir_connection_get_available_surface_formats(connection,
-        &surfaceparm.pixel_format, 1, &valid_formats);
+        format, max_formats, &nformats);
+
+    surfaceparm.pixel_format = format[0];
+    for (unsigned int f = 0; f < nformats; f++)
+    {
+        const int opaque = (format[f] == mir_pixel_format_xbgr_8888 ||
+                            format[f] == mir_pixel_format_xrgb_8888 ||
+                            format[f] == mir_pixel_format_bgr_888);
+
+        if ((mir_eglapp_background_opacity == 1.0f && opaque) ||
+            (mir_eglapp_background_opacity < 1.0f && !opaque))
+        {
+            surfaceparm.pixel_format = format[f];
+            break;
+        }
+    }
 
     printf("Connected to display: resolution (%dx%d), position(%dx%d), "
            "supports %d pixel formats\n",
 
=== modified file 'examples/eglapp.h'
--- examples/eglapp.h	2013-09-19 13:24:22 +0000
+++ examples/eglapp.h	2013-10-07 10:02:32 +0000
@@ -27,6 +27,8 @@
 struct MirConnection;
 struct MirSurface;
 
+extern float mir_eglapp_background_opacity;
+
 mir_eglapp_bool mir_eglapp_init(int argc, char *argv[],
                                 unsigned int *width, unsigned int *height);
 void            mir_eglapp_swap_buffers(void);
 
=== modified file 'examples/eglflash.c'
--- examples/eglflash.c	2013-09-19 13:24:22 +0000
+++ examples/eglflash.c	2013-10-07 10:02:32 +0000
@@ -33,17 +33,17 @@
     /* This is probably the simplest GL you can do */
     while (mir_eglapp_running())
     {
-        glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
-        glClear(GL_COLOR_BUFFER_BIT);
-        mir_eglapp_swap_buffers();
-        sleep(1);
-
-        glClearColor(0.0f, 1.0f, 0.0f, 1.0f);
-        glClear(GL_COLOR_BUFFER_BIT);
-        mir_eglapp_swap_buffers();
-        sleep(1);
-
-        glClearColor(0.0f, 0.0f, 1.0f, 1.0f);
+        glClearColor(1.0f, 0.0f, 0.0f, mir_eglapp_background_opacity);
+        glClear(GL_COLOR_BUFFER_BIT);
+        mir_eglapp_swap_buffers();
+        sleep(1);
+
+        glClearColor(0.0f, 1.0f, 0.0f, mir_eglapp_background_opacity);
+        glClear(GL_COLOR_BUFFER_BIT);
+        mir_eglapp_swap_buffers();
+        sleep(1);
+
+        glClearColor(0.0f, 0.0f, 1.0f, mir_eglapp_background_opacity);
         glClear(GL_COLOR_BUFFER_BIT);
         mir_eglapp_swap_buffers();
         sleep(1);
 
=== modified file 'examples/egltriangle.c'
--- examples/egltriangle.c	2013-09-19 13:24:22 +0000
+++ examples/egltriangle.c	2013-10-07 10:02:32 +0000
@@ -105,7 +105,7 @@
         return 2;
     }
 
-    glClearColor(MID_AUBERGINE, 1.0);
+    glClearColor(MID_AUBERGINE, mir_eglapp_background_opacity);
     glViewport(0, 0, width, height);
 
     glUseProgram(prog);
 
=== modified file 'include/server/mir/compositor/compositing_criteria.h'
--- include/server/mir/compositor/compositing_criteria.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/compositor/compositing_criteria.h	2013-10-07 10:02:32 +0000
@@ -36,6 +36,7 @@
     virtual float alpha() const = 0;
     virtual glm::mat4 const& transformation() const = 0;
     virtual bool should_be_rendered_in(geometry::Rectangle const& rect) const = 0;
+    virtual bool shaped() const = 0;  // meaning the pixel format has alpha
 
     virtual ~CompositingCriteria() = default;
 
 
=== modified file 'include/shared/mir/geometry/pixel_format.h'
--- include/shared/mir/geometry/pixel_format.h	2013-06-12 09:36:20 +0000
+++ include/shared/mir/geometry/pixel_format.h	2013-10-07 10:02:32 +0000
@@ -42,6 +42,12 @@
     return (fmt == PixelFormat::bgr_888) ? 3 : 4;
 }
 
+static inline bool has_alpha(PixelFormat fmt)
+{
+    return (fmt == PixelFormat::abgr_8888) ||
+           (fmt == PixelFormat::argb_8888);
+}
+
 }
 }
 
 
=== modified file 'include/test/mir_test_doubles/mock_compositing_criteria.h'
--- include/test/mir_test_doubles/mock_compositing_criteria.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_compositing_criteria.h	2013-10-07 10:02:32 +0000
@@ -36,6 +36,7 @@
     MOCK_CONST_METHOD0(alpha, float());
     MOCK_CONST_METHOD0(transformation, glm::mat4 const&());
     MOCK_CONST_METHOD1(should_be_rendered_in, bool(geometry::Rectangle const&));
+    MOCK_CONST_METHOD0(shaped, bool());
 };
 
 }
 
=== modified file 'include/test/mir_test_doubles/mock_surface_state.h'
--- include/test/mir_test_doubles/mock_surface_state.h	2013-10-03 06:01:11 +0000
+++ include/test/mir_test_doubles/mock_surface_state.h	2013-10-07 10:02:32 +0000
@@ -60,6 +60,7 @@
     MOCK_METHOD0(frame_posted, void());
     MOCK_METHOD1(set_hidden, void(bool));
     MOCK_CONST_METHOD1(should_be_rendered_in, bool(geometry::Rectangle const&));
+    MOCK_CONST_METHOD0(shaped, bool());
 };
 
 typedef ::testing::NiceMock<MockSurfaceState> StubSurfaceState;
 
=== modified file 'include/test/mir_test_doubles/stub_compositing_criteria.h'
--- include/test/mir_test_doubles/stub_compositing_criteria.h	2013-08-13 07:07:12 +0000
+++ include/test/mir_test_doubles/stub_compositing_criteria.h	2013-10-07 10:02:32 +0000
@@ -34,9 +34,11 @@
 {
 public:
     StubCompositingCriteria(int x, int y, int width, int height,
-                            float opacity=1.0f)
+                            float opacity=1.0f,
+                            bool rectangular=true)
         : rect{{x, y}, {width, height}},
-          opacity(opacity)
+          opacity(opacity),
+          rectangular(rectangular)
     {
         const glm::mat4 ident;
         glm::vec3 size(width, height, 0.0f);
@@ -59,10 +61,16 @@
         return rect.overlaps(r);
     }
 
+    bool shaped() const override
+    {
+        return !rectangular;
+    }
+
 private:
     mir::geometry::Rectangle rect;
     glm::mat4 trans;
     float opacity;
+    bool rectangular;
 };
 
 } // namespace doubles
 
=== modified file 'src/server/compositor/bypass.cpp'
--- src/server/compositor/bypass.cpp	2013-08-13 07:14:11 +0000
+++ src/server/compositor/bypass.cpp	2013-10-07 10:02:32 +0000
@@ -57,7 +57,7 @@
 
 bool BypassFilter::operator()(const CompositingCriteria &criteria)
 {
-    if (criteria.alpha() != 1.0f)
+    if (criteria.alpha() != 1.0f || criteria.shaped())
         return false;
 
     if (!all_orthogonal)
 
=== modified file 'src/server/surfaces/surface_allocator.cpp'
--- src/server/surfaces/surface_allocator.cpp	2013-10-03 03:44:08 +0000
+++ src/server/surfaces/surface_allocator.cpp	2013-10-07 10:02:32 +0000
@@ -46,9 +46,12 @@
                                            params.buffer_usage};
     auto buffer_stream = buffer_stream_factory->create_buffer_stream(buffer_properties);
     auto actual_size = geom::Rectangle{params.top_left, buffer_stream->stream_size()};
+
+    bool nonrectangular = has_alpha(params.pixel_format);
     auto state = std::make_shared<ms::SurfaceData>(params.name,
                                                     actual_size,
-                                                    change_callback);
+                                                    change_callback,
+                                                    nonrectangular);
     auto input_channel = input_factory->make_input_channel();
     return std::make_shared<ms::Surface>(state, buffer_stream, input_channel);
 }
 
=== modified file 'src/server/surfaces/surface_data.cpp'
--- src/server/surfaces/surface_data.cpp	2013-10-03 03:44:08 +0000
+++ src/server/surfaces/surface_data.cpp	2013-10-07 10:02:32 +0000
@@ -22,7 +22,7 @@
 namespace geom=mir::geometry;
 namespace ms = mir::surfaces;
 
-ms::SurfaceData::SurfaceData(std::string const& name, geom::Rectangle rect, std::function<void()> change_cb)
+ms::SurfaceData::SurfaceData(std::string const& name, geom::Rectangle rect, std::function<void()> change_cb, bool nonrectangular)
     : notify_change(change_cb),
       surface_name(name),
       surface_rect(rect),
@@ -30,6 +30,7 @@
       surface_alpha(1.0f),
       first_frame_posted(false),
       hidden(false),
+      nonrectangular(nonrectangular),
       input_rectangles{surface_rect}
 {
 }
@@ -91,6 +92,11 @@
     return rect.overlaps(surface_rect);
 }
 
+bool ms::SurfaceData::shaped() const
+{
+    return nonrectangular;
+}
+
 void ms::SurfaceData::apply_alpha(float alpha)
 {
     {
 
=== modified file 'src/server/surfaces/surface_data.h'
--- src/server/surfaces/surface_data.h	2013-10-03 08:42:27 +0000
+++ src/server/surfaces/surface_data.h	2013-10-07 10:02:32 +0000
@@ -34,12 +34,14 @@
 {
 public:
     SurfaceData(std::string const& name, geometry::Rectangle rect,
-                std::function<void()> change_cb);
+                std::function<void()> change_cb,
+                bool nonrectangular);
 
     //mc::CompositingCriteria
     glm::mat4 const& transformation() const;
     float alpha() const;
     bool should_be_rendered_in(geometry::Rectangle const& rect) const;
+    bool shaped() const override;
 
     //mi::Surface
     std::string const& name() const;
@@ -67,6 +69,7 @@
     float surface_alpha;
     bool first_frame_posted;
     bool hidden;
+    const bool nonrectangular;
     std::vector<geometry::Rectangle> input_rectangles;
 };
 
 
=== modified file 'tests/unit-tests/compositor/test_bypass.cpp'
--- tests/unit-tests/compositor/test_bypass.cpp	2013-08-13 07:18:22 +0000
+++ tests/unit-tests/compositor/test_bypass.cpp	2013-10-07 10:02:32 +0000
@@ -88,6 +88,16 @@
     EXPECT_FALSE(filter.fullscreen_on_top());
 }
 
+TEST_F(BypassFilterTest, shaped_fullscreen_window_not_bypassed)
+{
+    BypassFilter filter(display_buffer[0]);
+
+    StubCompositingCriteria win(0, 0, 1920, 1200, 1.0f, false);
+
+    EXPECT_FALSE(filter(win));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+}
+
 TEST_F(BypassFilterTest, offset_fullscreen_window_not_bypassed)
 {
     BypassFilter filter(display_buffer[0]);
@@ -167,6 +177,39 @@
     EXPECT_TRUE(filter.fullscreen_on_top());
 }
 
+TEST_F(BypassFilterTest, many_fullscreen_windows_only_bypass_top_rectangular)
+{
+    BypassFilter filter(display_buffer[0]);
+
+    StubCompositingCriteria a(0, 0, 1920, 1200, 1.0f, false);
+    EXPECT_FALSE(filter(a));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria b(1, 2, 3, 4);
+    EXPECT_FALSE(filter(b));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria c(0, 0, 1920, 1200);
+    EXPECT_TRUE(filter(c));
+    EXPECT_TRUE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria d(5, 6, 7, 8);
+    EXPECT_FALSE(filter(d));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria e(0, 0, 1920, 1200, 1.0f, true);
+    EXPECT_TRUE(filter(e));
+    EXPECT_TRUE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria f(9, 10, 11, 12);
+    EXPECT_FALSE(filter(f));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+
+    StubCompositingCriteria g(0, 0, 1920, 1200, 0.5f, false);
+    EXPECT_FALSE(filter(g));
+    EXPECT_FALSE(filter.fullscreen_on_top());
+}
+
 TEST_F(BypassFilterTest, multimonitor_one_bypassed)
 {
     BypassFilter left(display_buffer[0]);
 
=== modified file 'tests/unit-tests/surfaces/test_surface_data.cpp'
--- tests/unit-tests/surfaces/test_surface_data.cpp	2013-10-03 03:44:08 +0000
+++ tests/unit-tests/surfaces/test_surface_data.cpp	2013-10-07 10:02:32 +0000
@@ -67,10 +67,11 @@
 
 TEST_F(SurfaceDataTest, basics)
 { 
-    ms::SurfaceData data{name, rect, null_change_cb};
+    ms::SurfaceData data{name, rect, null_change_cb, false};
     EXPECT_EQ(name, data.name());
     EXPECT_EQ(rect.size, data.size());
     EXPECT_EQ(rect.top_left, data.position());
+    EXPECT_FALSE(data.shaped());
 }
 
 TEST_F(SurfaceDataTest, update_position)
@@ -78,7 +79,7 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData storage{name, rect, mock_change_cb};
+    ms::SurfaceData storage{name, rect, mock_change_cb, false};
     EXPECT_EQ(rect.top_left, storage.position());
 
     auto new_top_left = geom::Point{geom::X{6}, geom::Y{10}};
@@ -91,7 +92,7 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData storage{name, rect, mock_change_cb};
+    ms::SurfaceData storage{name, rect, mock_change_cb, false};
     auto original_transformation = storage.transformation();
 
     storage.apply_rotation(60.0f, glm::vec3{0.0f, 0.0f, 1.0f});
@@ -106,7 +107,7 @@
     const geom::Size sz{geom::Width{85}, geom::Height{43}};
     const geom::Rectangle origin{geom::Point{geom::X{77}, geom::Y{88}}, sz};
     const geom::Rectangle moved_pt{geom::Point{geom::X{55}, geom::Y{66}}, sz};
-    ms::SurfaceData storage{name, origin, null_change_cb};
+    ms::SurfaceData storage{name, origin, null_change_cb, false};
 
     glm::mat4 t0 = storage.transformation();
     storage.move_to(moved_pt.top_left);
@@ -125,7 +126,7 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
 
     float alpha = 0.5f;
     surface_state.apply_alpha(0.5f);
@@ -135,8 +136,9 @@
 TEST_F(SurfaceDataTest, test_surface_is_opaque_by_default)
 {
     using namespace testing;
-    ms::SurfaceData surface_state{name, rect, null_change_cb};
+    ms::SurfaceData surface_state{name, rect, null_change_cb, false};
     EXPECT_THAT(1.0f, FloatEq(surface_state.alpha()));
+    EXPECT_FALSE(surface_state.shaped());
 }
 
 TEST_F(SurfaceDataTest, test_surface_apply_rotation)
@@ -144,13 +146,13 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
     surface_state.apply_rotation(60.0f, glm::vec3{0.0f, 0.0f, 1.0f});
 }
 
 TEST_F(SurfaceDataTest, test_surface_should_be_rendered_in)
 {
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
     geom::Rectangle output_rect{geom::Point{0,0}, geom::Size{100, 100}};
 
     //not renderable by default
@@ -179,7 +181,7 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
     surface_state.set_hidden(true);
 }
 
@@ -189,7 +191,7 @@
     EXPECT_CALL(mock_callback, call())
         .Times(1);
 
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
     surface_state.frame_posted();
 }
 
@@ -198,7 +200,7 @@
 {
     geom::Point pt(1,1);
     geom::Size one_by_one{geom::Width{1}, geom::Height{1}};
-    ms::SurfaceData surface_state{name, geom::Rectangle{pt, one_by_one}, mock_change_cb};
+    ms::SurfaceData surface_state{name, geom::Rectangle{pt, one_by_one}, mock_change_cb, false};
 
     std::vector<geom::Point> contained_pt
     {
@@ -230,7 +232,7 @@
         {{geom::X{1}, geom::Y{1}}, {geom::Width{1}, geom::Height{1}}}  //region1
     };
 
-    ms::SurfaceData surface_state{name, rect, mock_change_cb};
+    ms::SurfaceData surface_state{name, rect, mock_change_cb, false};
     surface_state.set_input_region(rectangles);
 
     std::vector<geom::Point> contained_pt