Merge lp:~mterry/mir/fix-nested-crash into lp:mir

[Michael Terry]

Fix a crash due to a failed eglMakeCurrent() call when in nested mode.

Fix a crash due to a failed eglMakeCurrent() call when in nested mode.

I'm not sure how to give reproduction steps that don't involve all of unity8. But if you happen to have set up unity8 in nested mode, you can see this by doing:
1) Open an app
2) Swipe from left (launcher) or right (switch app)

Either swipe will crash unity8, due to a "cannot bind buffer to texture without EGL context" error. kdub helped me trace it down. Apparently NestedGLContext should act a little more like AndroidGLContext and use a dummy pbuffer surface instead of EGL_NO_SURFACE. It's eglMakeCurrent() call was silently failing, and this caused crashes later.

So I've made NestedGLContext do some of the things AndroidGLContext does:
1) Release in destructor
2) Use a dummy pbuffer surface
3) Throw an exception if eglMakeCurrent fails.

I've tested and it fixes the crash for me.

[Chris Halse Rogers (raof)]

Hm. This should be necessary only if the underlying EGL doesn't support EGL_KHR_surfaceless_context, and the Mesa stack (should!) supports that.

[Kevin DuBois (kdub)]

> Hm. This should be necessary only if the underlying EGL doesn't support
> EGL_KHR_surfaceless_context, and the Mesa stack (should!) supports that.

Yes. all the implementations of mg::GLContext needs to be de-duped. I think we should wait for 1 branch I have that dedupes 3 android glcontext implementations and for alf's offscreen display work to land before we go and consolidate this.

[Kevin DuBois (kdub)]

looks okay to me... we should have a test though. :/
I think we could get this under test when we do the aforementioned consolidation, so i'll +1

review: Approve

[Daniel van Vugt (vanvugt)]

namespace { static ... is redundant. You only need one or the other :)

[Daniel van Vugt (vanvugt)]

OK, only minor style complaints:

1. Please don't overload the initializer list with function calls. It would be clearer to call the functions in the constructor.
27 + egl_context{egl_display, eglCreateContext(egl_display, egl_config, EGL_NO_CONTEXT, detail::nested_egl_context_attribs)},
28 + egl_surface{egl_display, eglCreatePbufferSurface(egl_display, egl_config, dummy_pbuffer_attribs)}

2. "namespace { static" is redundant. You only need one or the other.

review: Needs Fixing

[Michael Terry (mterry)]

> 1. Please don't overload the initializer list with function calls. It would be clearer to call the functions in the constructor.

Er, the function-in-initializer-list pattern is all over the place in mir, right? Especially in surrounding code and nearby android_display.cpp. Also, EGLSurfaceStore doesn't define a public operator= nor the ability to set the surface pointer after the fact. So I'm not sure how to move the function to the constructor?

> 2. "namespace { static" is redundant. You only need one or the other.

Fixed. If you're on a cleaning rampage, note that mine was just copied code from android_display.cpp.

[Daniel van Vugt (vanvugt)]

Oh, I was confused by the braces. I thought even in C++11 that a constructor call still had to use parentheses:
  egl_context(...)

review: Approve

=== modified file 'debian/rules' (properties changed: -x to +x)
=== modified file 'src/server/graphics/nested/nested_display.cpp'
--- src/server/graphics/nested/nested_display.cpp	2013-10-25 09:40:26 +0000
+++ src/server/graphics/nested/nested_display.cpp	2013-11-15 04:26:53 +0000
@@ -273,6 +273,16 @@
     return std::weak_ptr<Cursor>();
 }
 
+namespace
+{
+EGLint const dummy_pbuffer_attribs[] =
+{
+    EGL_WIDTH, 1,
+    EGL_HEIGHT, 1,
+    EGL_NONE
+};
+}
+
 std::unique_ptr<mg::GLContext> mgn::NestedDisplay::create_gl_context()
 {
     class NestedGLContext : public mg::GLContext
@@ -281,13 +291,24 @@
         NestedGLContext(detail::EGLDisplayHandle const& egl_display) :
             egl_display{egl_display},
             egl_config{egl_display.choose_config(detail::nested_egl_config_attribs)},
-            egl_context{egl_display, eglCreateContext(egl_display, egl_config, EGL_NO_CONTEXT, detail::nested_egl_context_attribs)}
-        {
+            egl_context{egl_display, eglCreateContext(egl_display, egl_config, EGL_NO_CONTEXT, detail::nested_egl_context_attribs)},
+            egl_surface{egl_display, eglCreatePbufferSurface(egl_display, egl_config, dummy_pbuffer_attribs)}
+        {
+        }
+
+        ~NestedGLContext() noexcept
+        {
+            if (eglGetCurrentContext() == egl_context)
+                release_current();
         }
 
         void make_current() override
         {
-            eglMakeCurrent(egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, egl_context);
+            if (eglMakeCurrent(egl_display, egl_surface, egl_surface, egl_context) == EGL_FALSE)
+            {
+                BOOST_THROW_EXCEPTION(
+                    std::runtime_error("could not activate dummy surface with eglMakeCurrent\n"));
+            }
         }
 
         void release_current() override
@@ -299,6 +320,7 @@
         EGLDisplay const egl_display;
         EGLConfig const egl_config;
         EGLContextStore const egl_context;
+        EGLSurfaceStore const egl_surface;
     };
 
     return std::unique_ptr<mg::GLContext>{new NestedGLContext(egl_display)};