Merge lp:~vanvugt/mir/eglapp-bits into lp:mir

[Daniel van Vugt]

eglapp: Add a parameter "-eN" for adjusting the EGL colour channel size
in bits. Using -e5 on some devices like Nexus 4 is sufficient to
reproduce LP: #1460149.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2721
http://jenkins.qa.ubuntu.com/job/mir-ci/4240/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3064
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/585
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3012
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/396
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/396/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3012
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3012/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5823
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21589

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Does this work reliably? From the EGL spec:

"3.4.1.2 Sorting of EGLConfig

...

3. Special: by larger total number of color bits (for an RGB color buffer,
this is the sum of EGL_RED_SIZE , EGL_GREEN_SIZE , EGL_BLUE_SIZE ,
and EGL_ALPHA_SIZE ;"

Which means that for 5,5,5,1 we would get the largest buffer size with at least those r,g,b,a sizes, i.e. probably a 32-bit 8,8,8,8.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Good observation!

The simple answer is we have buggy/non-conformant EGL implementations on Nexus 4 and other devices which do not follow that rule. If you ask for 555, they give you 565 instead of 888.

So yes, the branch works reliably. Though it would not be too useful /if/ we did not have buggy platforms to support :)

[Daniel van Vugt (vanvugt)]

Although the EGL spec itself is questionable and I understand people not following it properly. Because if all implementations really did just choose the highest colour depth available then specifying any value<=max in the EGLConfig would always give the same result (maximum colour depth). Hence those channel attributes should not exist at all if the spec really expects everyone to choose the maximum depth all the time. Sounds like the EGL spec needs rewording.

Back in the real world we should not make any assumptions about the chosen EGLConfig. It clearly might not be what we expected, and we should query what was chosen.

[Alexandros Frantzis (afrantzis)]

I agree that the sorting order in the spec is broken, but we do have a way around this: use eglGetConfigs to get all configs and manually select a matching EGLConfig with the properties we want. This ensures that it works reliably on all platforms and it's only slightly more complicated.

[Daniel van Vugt (vanvugt)]

Yes you could do it that way. Although both approaches require a stupid/ugly linear search.

I suggest what is proposed here is a bit cleaner though. Because it allows the toolkit/app to choose its config in a way that is not bound to the limited/changeable list of Mir pixel formats. In fact, this way the toolkit/app never even has to see what its Mir pixel format is.

Setting up EGL before creating the native window is also how the EGL docs approach it:
https://www.khronos.org/registry/egl/sdk/docs/man/html/eglIntro.xhtml

[Daniel van Vugt (vanvugt)]

Other readers: Please ignore the above conversation. It actually belongs in:
https://code.launchpad.net/~vanvugt/mir/eglapp-choose-pixel-format/+merge/263622

[Daniel van Vugt (vanvugt)]

Actually alf's approach might be better. Still investigating, but it does not affect this branch. Only later work... So you can still ignore the above discussion :)

[Alexandros Frantzis (afrantzis)]

> Still investigating, but it does not affect this branch. Only later work... So you can still ignore the above discussion :)

OK. Perhaps we should support -e r:g:b:a instead, but not a blocker.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2723
http://jenkins.qa.ubuntu.com/job/mir-ci/4269/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3088
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/609
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3036
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/418
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/418/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3036
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3036/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5843
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21689

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Robert Carr (robertcarr)]

LGTM

review: Approve

=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2015-06-17 05:20:42 +0000
+++ examples/eglapp.c	2015-07-07 02:31:43 +0000
@@ -197,6 +197,7 @@
     unsigned int output_id = mir_display_output_id_invalid;
     char *mir_socket = NULL;
     char const* cursor_name = mir_default_cursor_name;
+    unsigned int rgb_bits = 8;
 
     if (argc > 1)
     {
@@ -230,6 +231,21 @@
                         }
                     }
                     break;
+                case 'e':
+                    {
+                        arg += 2;
+                        if (!arg[0] && i < argc-1)
+                        {
+                            ++i;
+                            arg = argv[i];
+                        }
+                        if (sscanf(arg, "%u", &rgb_bits) != 1)
+                        {
+                            printf("Invalid colour channel depth: %s\n", arg);
+                            help = 1;
+                        }
+                    }
+                    break;
                 case 'n':
                     swapinterval = 0;
                     break;
@@ -305,6 +321,7 @@
             {
                 printf("Usage: %s [<options>]\n"
                        "  -b               Background opacity (0.0 - 1.0)\n"
+                       "  -e               EGL colour channel size in bits\n"
                        "  -h               Show this help text\n"
                        "  -f               Force full screen\n"
                        "  -o ID            Force placement on output monitor ID\n"
@@ -371,13 +388,15 @@
 
     printf("Server supports %d of %d surface pixel formats. Using format: %d\n",
         nformats, mir_pixel_formats, pixel_format);
-    unsigned int bpp = 8 * MIR_BYTES_PER_PIXEL(pixel_format);
     EGLint attribs[] =
     {
         EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
         EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
         EGL_COLOR_BUFFER_TYPE, EGL_RGB_BUFFER,
-        EGL_BUFFER_SIZE, bpp,
+        EGL_RED_SIZE, rgb_bits,
+        EGL_GREEN_SIZE, rgb_bits,
+        EGL_BLUE_SIZE, rgb_bits,
+        EGL_ALPHA_SIZE, mir_eglapp_background_opacity == 1.0f ? 0 : rgb_bits,
         EGL_NONE
     };