Merge lp:~peat-new/mir/avoid-hardcoding-color-format-in-device-quirks into lp:mir

[Ratchanan Srirattanamet]

Originally, DeviceQuirks creates a PbufferGLContext with mir_pixel_format_abgr_8888 hard-coded as a format. This causes a problem on a device without mir_pixel_format_abgr_8888 support (in particular, Samsung Galaxy Tab 2 7.0). This MP creates new PbufferGLContext constructor which doesn't have to specify a display format. Then, the DeviceQuirks is modified to use this new constructor. This makes the code not depending on the availability of mir_pixel_format_abgr_8888 format on the device.

[Daniel van Vugt (vanvugt)]

Cool, thanks. Always nice to add support for more devices.

Hard-coding of pixel formats is a problem we continue to fix throughout the whole software stack...

I'm assuming our system compositor doesn't have a strict need for an alpha channel on the framebuffer, because we're no longer enforcing that here (would require EGL_ALPHA_SIZE).

I've verified tests pass locally but it would be nice if we could get the Mir CI bot to run all tests.

review: Approve

[Alan Griffiths (alan-griffiths)]

LGTM, my only concern is to verify that it passes CI on krillin

review: Approve

[Kevin DuBois (kdub)]

alright, wonder why ci isnt running on branch.

review: Approve

[Ratchanan Srirattanamet (peat-new)]

> alright, wonder why ci isnt running on branch.

I guess that CI is configured to run only on MP by developer members.
From https://mir-jenkins.ubuntu.com/job/trigger-ci/lastSuccessfulBuild/console

>DEBUG: Users "peat-new" not allowed to trigger jobs

So, I think one of the developers have to do something special to kick off the CI.

[Alan Griffiths (alan-griffiths)]

> So, I think one of the developers have to do something special to kick off the
> CI.

I *thought* Daniel did that (by requesting mir-ci-bot review). I was obviously wrong. :(

[Cemil Azizoglu (cemil-azizoglu)]

Sure

review: Approve

[Daniel van Vugt (vanvugt)]

I was hoping adding mir-ci-bot would work, but did not expect it to.

Now let's see if autolanding runs all the CI jobs at least. Or if it doesn't work at all...

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

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~peat-new/mir/avoid-hardcoding-color-format-in-device-quirks/+merge/302248/+edit-commit-message

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

It's not yet clear that CI has been run and the bot might be trying to land it untested.

Now waiting on full test run:
https://code.launchpad.net/~mir-team/mir/avoid-hardcoding-color-format-in-device-quirks/+merge/302367

Just in case.

[Daniel van Vugt (vanvugt)]

CI passed when I submitted it.

https://mir-jenkins.ubuntu.com/job/mir-ci/1413/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1723
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1776
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1745/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/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1745/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1745/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/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1745/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/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1745/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/1745
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1745/artifact/output/*zip*/output.zip

=== modified file 'src/platforms/android/server/device_quirks.cpp'
--- src/platforms/android/server/device_quirks.cpp	2016-07-28 09:10:43 +0000
+++ src/platforms/android/server/device_quirks.cpp	2016-08-08 06:26:59 +0000
@@ -50,13 +50,13 @@
 std::string const egl_sync_default = "default";
 std::string const egl_sync_force_on = "force_on";
 std::string const egl_sync_force_off = "force_off";
- 
+
 
 std::string determine_device_name(mga::PropertiesWrapper const& properties)
 {
     char const default_value[] = "";
     char value[PROP_VALUE_MAX] = "";
-    char const key[] = "ro.product.device"; 
+    char const key[] = "ro.product.device";
     properties.property_get(key, value, default_value);
     return std::string{value};
 }
@@ -167,13 +167,13 @@
 }
 
 mga::DeviceQuirks::DeviceQuirks(PropertiesWrapper const& properties) :
-    DeviceQuirks(properties, mga::PbufferGLContext{mir_pixel_format_abgr_8888, config, report})
+    DeviceQuirks(properties, mga::PbufferGLContext{config, report})
 {
 }
 
 mga::DeviceQuirks::DeviceQuirks(PropertiesWrapper const& properties, mo::Option const& options)
     : device_name(determine_device_name(properties)),
-      gpu_info(determine_gpu_info(mga::PbufferGLContext(mir_pixel_format_abgr_8888, config, report))),
+      gpu_info(determine_gpu_info(mga::PbufferGLContext(config, report))),
       num_framebuffers_(num_framebuffers_for(device_name, options.get(num_framebuffers_opt, true))),
       gralloc_cannot_be_closed_safely_(gralloc_cannot_be_closed_safely_for(device_name, options.get(gralloc_cannot_be_closed_safely_opt, true))),
       enable_width_alignment_quirk(options.get(width_alignment_opt, true)),

=== modified file 'src/platforms/android/server/gl_context.cpp'
--- src/platforms/android/server/gl_context.cpp	2016-07-28 09:10:43 +0000
+++ src/platforms/android/server/gl_context.cpp	2016-08-08 06:26:59 +0000
@@ -103,6 +103,30 @@
     return *pegl_config;
 }
 
+//Sometimes it's useful not to specify a format when one doesn't know which format to use yet.
+//This is used by DeviceQuirks to create a context for finding out GL vendor and renderer.
+EGLConfig select_egl_config_with_any_format(
+    EGLDisplay egl_display, mg::GLConfig const& gl_config)
+{
+    EGLint const required_egl_config_attr [] =
+    {
+        EGL_SURFACE_TYPE, EGL_WINDOW_BIT | EGL_PBUFFER_BIT,
+        EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
+        EGL_DEPTH_SIZE, gl_config.depth_buffer_bits(),
+        EGL_STENCIL_SIZE, gl_config.stencil_buffer_bits(),
+        EGL_NONE
+    };
+
+    EGLConfig the_config;
+    EGLint num_returned_config;
+
+    eglChooseConfig(egl_display, required_egl_config_attr, &the_config, 1, &num_returned_config);
+
+    if (num_returned_config != 1)
+        BOOST_THROW_EXCEPTION(std::runtime_error("could not select EGL config"));
+
+    return the_config;
+}
 }
 
 void mga::GLContext::make_current(EGLSurface egl_surface) const
@@ -139,6 +163,16 @@
     report.report_egl_configuration(egl_display, egl_config);
 }
 
+mga::GLContext::GLContext(mg::GLConfig const& gl_config, mg::DisplayReport& report) :
+    egl_display(create_and_initialize_display()),
+    egl_config(select_egl_config_with_any_format(egl_display, gl_config)),
+    egl_context{egl_display,
+                eglCreateContext(egl_display, egl_config, EGL_NO_CONTEXT, default_egl_context_attr)},
+    own_display(true)
+{
+    report.report_egl_configuration(egl_display, egl_config);
+}
+
 mga::GLContext::GLContext(GLContext const& shared_gl_context) :
     mir::renderer::gl::Context(),
     egl_display(shared_gl_context.egl_display),
@@ -158,6 +192,13 @@
 {
 }
 
+mga::PbufferGLContext::PbufferGLContext(mg::GLConfig const& gl_config, mg::DisplayReport& report) :
+    GLContext(gl_config, report),
+    egl_surface{egl_display,
+                eglCreatePbufferSurface(egl_display, egl_config, dummy_pbuffer_attribs)}
+{
+}
+
 mga::PbufferGLContext::PbufferGLContext(PbufferGLContext const& shared_gl_context) :
     GLContext(shared_gl_context),
     egl_surface{egl_display,

=== modified file 'src/platforms/android/server/gl_context.h'
--- src/platforms/android/server/gl_context.h	2016-07-28 09:10:43 +0000
+++ src/platforms/android/server/gl_context.h	2016-08-08 06:26:59 +0000
@@ -46,6 +46,8 @@
     GLContext(MirPixelFormat display_format,
               GLConfig const& gl_config,
               DisplayReport& report);
+    GLContext(GLConfig const& gl_config,
+              DisplayReport& report);
 
     GLContext(GLContext const& shared_gl_context);
 
@@ -68,6 +70,9 @@
               GLConfig const& gl_config,
               DisplayReport& report);
 
+    PbufferGLContext(GLConfig const& gl_config,
+              DisplayReport& report);
+
     PbufferGLContext(PbufferGLContext const& shared_gl_context);
 
     void make_current() const override;

=== modified file 'tests/unit-tests/graphics/android/test_device_detection.cpp'
--- tests/unit-tests/graphics/android/test_device_detection.cpp	2016-06-28 14:58:46 +0000
+++ tests/unit-tests/graphics/android/test_device_detection.cpp	2016-08-08 06:26:59 +0000
@@ -387,3 +387,22 @@
     mga::DeviceQuirks broken_quirks(mock_ops, options);
     EXPECT_FALSE(broken_quirks.working_egl_sync());
 }
+
+TEST_F(DeviceQuirks, detects_gl_vendor_on_device_with_only_argb_8888_support)
+{
+    using namespace testing;
+    ON_CALL(mock_egl, eglGetConfigAttrib(_, _, EGL_NATIVE_VISUAL_ID, _))
+        .WillByDefault(DoAll(
+                          SetArgPointee<3>(HAL_PIXEL_FORMAT_BGRA_8888),
+                          Return(EGL_TRUE)));
+    //In the reality, Qualcomm does support abgr_8888. But we do this to make sure gl_vendor will be checked.
+    EXPECT_CALL(mock_gl, glGetString(GL_VENDOR))
+        .Times(1)
+        .WillOnce(Return(reinterpret_cast<const unsigned char*>("Qualcomm")));
+    EXPECT_CALL(mock_gl, glGetString(GL_RENDERER))
+        .Times(1);
+    MockOps mock_ops;
+
+    mga::DeviceQuirks quirks(mock_ops, options);
+    EXPECT_TRUE(quirks.working_egl_sync());
+}