Merge lp:~albaguirre/mir/fix-1539268 into lp:mir

[Alberto Aguirre]

nested-display: Use matching egl configs when creating any context (display, display buffer, shared gl context) (LP: #1539268)

Do not use arbitrary attributes when choosing an egl config for the context created during create_gl_context.

As described in bug report:

The surfacelesscontext created uses a default attribute list which may not match the actual attribute list used to create the egl context associated with the nested display.

This is problematic for qtmir - as it creates a temporary context in an effort to query the underlying eglconfig used for the display and inform Qt about R/G/B/A/Depth/stencil sizes.

If the surfaceless context happens to choose a config that has no alpha, qtmir will inform Qt that there's no alpha (when there actually is) and rendering artifacts can occur.

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

PASSED: Continuous integration, rev:3276
https://mir-jenkins.ubuntu.com/job/mir-ci/182/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/182/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3276
http://jenkins.qa.ubuntu.com/job/mir-ci/6156/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5728
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4635
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5684
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/356/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/480
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/480/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/480
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/480/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5681
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5681/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8108
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27150
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/352
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/352/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/208/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27152

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Looks harmless, but I wonder; is this a case where mir_connection_get_egl_pixel_format or something similar would be more robust?

review: Needs Information

[Chris Halse Rogers (raof)]

Looks sensible.

review: Approve

[Alberto Aguirre (albaguirre)]

> Looks harmless, but I wonder; is this a case where
> mir_connection_get_egl_pixel_format or something similar would be more robust?

But we already know what mir pixel format has been selected for the nested display. This MP just makes sure all other contexts created also match the same egl configuration.

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

=== modified file 'src/server/graphics/nested/display.cpp'
--- src/server/graphics/nested/display.cpp	2016-01-20 23:59:18 +0000
+++ src/server/graphics/nested/display.cpp	2016-01-28 22:38:43 +0000
@@ -67,7 +67,8 @@
     std::shared_ptr<GLConfig> const& gl_config)
     : egl_display(EGL_NO_DISPLAY),
       egl_context_(EGL_NO_CONTEXT),
-      gl_config{gl_config}
+      gl_config{gl_config},
+      pixel_format{mir_pixel_format_invalid}
 {
     egl_display = eglGetDisplay(native_display);
     if (egl_display == EGL_NO_DISPLAY)
@@ -84,6 +85,7 @@
         BOOST_THROW_EXCEPTION(mg::egl_error("Nested Mir Display Error: Failed to initialize EGL."));
     }
 
+    pixel_format = format;
     egl_context_ = eglCreateContext(egl_display, choose_windowed_es_config(format), EGL_NO_CONTEXT, detail::nested_egl_context_attribs);
 
     if (egl_context_ == EGL_NO_CONTEXT)
@@ -119,6 +121,23 @@
     return egl_context_;
 }
 
+std::unique_ptr<mg::GLContext> mgn::detail::EGLDisplayHandle::create_gl_context()
+{
+    EGLint const attribs[] =
+    {
+       EGL_SURFACE_TYPE, EGL_DONT_CARE,
+       EGL_RED_SIZE, mg::red_channel_depth(pixel_format),
+       EGL_GREEN_SIZE, mg::green_channel_depth(pixel_format),
+       EGL_BLUE_SIZE, mg::blue_channel_depth(pixel_format),
+       EGL_ALPHA_SIZE, mg::alpha_channel_depth(pixel_format),
+       EGL_DEPTH_SIZE, gl_config->depth_buffer_bits(),
+       EGL_STENCIL_SIZE, gl_config->stencil_buffer_bits(),
+       EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
+       EGL_NONE
+    };
+    return std::make_unique<SurfacelessEGLContext>(egl_display, attribs, EGL_NO_CONTEXT);
+}
+
 mgn::detail::EGLDisplayHandle::~EGLDisplayHandle() noexcept
 {
     eglTerminate(egl_display);
@@ -349,5 +368,5 @@
 
 std::unique_ptr<mg::GLContext> mgn::Display::create_gl_context()
 {
-    return std::make_unique<SurfacelessEGLContext>(egl_display, EGL_NO_CONTEXT);
+    return egl_display.create_gl_context();
 }

=== modified file 'src/server/graphics/nested/display.h'
--- src/server/graphics/nested/display.h	2016-01-20 23:59:18 +0000
+++ src/server/graphics/nested/display.h	2016-01-28 22:38:43 +0000
@@ -80,12 +80,15 @@
     void initialize(MirPixelFormat format);
     EGLConfig choose_windowed_es_config(MirPixelFormat format) const;
     EGLContext egl_context() const;
+    std::unique_ptr<graphics::GLContext> create_gl_context();
+
     operator EGLDisplay() const { return egl_display; }
 
 private:
     EGLDisplay egl_display;
     EGLContext egl_context_;
     std::shared_ptr<GLConfig> const gl_config;
+    MirPixelFormat pixel_format;
 
     EGLDisplayHandle(EGLDisplayHandle const&) = delete;
     EGLDisplayHandle operator=(EGLDisplayHandle const&) = delete;

=== modified file 'tests/unit-tests/graphics/nested/test_nested_display.cpp'
--- tests/unit-tests/graphics/nested/test_nested_display.cpp	2015-06-29 03:29:31 +0000
+++ tests/unit-tests/graphics/nested/test_nested_display.cpp	2016-01-28 22:38:43 +0000
@@ -200,3 +200,47 @@
     EXPECT_CALL(mock_egl,
                 eglMakeCurrent(_, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
 }
+
+TEST_F(NestedDisplay, shared_context_uses_matching_egl_attributes)
+{
+    using namespace testing;
+
+    NiceMock<mtd::MockGLConfig> mock_gl_config;
+    EGLint const depth_bits{24};
+    EGLint const stencil_bits{8};
+
+    std::string extensions{"EGL_KHR_surfaceless_context"};
+    ON_CALL(mock_gl_config, depth_buffer_bits())
+        .WillByDefault(Return(depth_bits));
+    ON_CALL(mock_gl_config, stencil_buffer_bits())
+        .WillByDefault(Return(stencil_bits));
+    ON_CALL(mock_egl, eglChooseConfig(_,_,_,_,_))
+        .WillByDefault(DoAll(SetArgPointee<2>(mock_egl.fake_configs[0]),
+                       SetArgPointee<4>(1),
+                       Return(EGL_TRUE)));
+    ON_CALL(mock_egl, eglQueryString(_, EGL_EXTENSIONS))
+        .WillByDefault(Return(extensions.c_str()));
+
+    // mt::build_trivial_configuration sets mir_pixel_format_abgr_8888
+    EGLint const expected_alpha_bits = 8;
+    EGLint const expected_red_bits = 8;
+    EGLint const expected_green_bits = 8;
+    EGLint const expected_blue_bits = 8;
+    EXPECT_CALL(mock_egl,
+                eglChooseConfig(
+                    _,
+                    AllOf(mtd::EGLConfigContainsAttrib(EGL_DEPTH_SIZE, depth_bits),
+                          mtd::EGLConfigContainsAttrib(EGL_STENCIL_SIZE, stencil_bits),
+                          mtd::EGLConfigContainsAttrib(EGL_RED_SIZE, expected_red_bits),
+                          mtd::EGLConfigContainsAttrib(EGL_GREEN_SIZE, expected_green_bits),
+                          mtd::EGLConfigContainsAttrib(EGL_BLUE_SIZE, expected_blue_bits),
+                          mtd::EGLConfigContainsAttrib(EGL_ALPHA_SIZE, expected_alpha_bits)),
+                    _,_,_))
+        .Times(AtLeast(1));
+
+    auto const nested_display = create_nested_display(
+        null_platform,
+        mt::fake_shared(mock_gl_config));
+
+    auto dummy_context = nested_display->create_gl_context();
+}