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

[Alberto Aguirre]

make current before deleting gl resources in GLPixelBuffer (LP: #1256360)

make current before deleting gl resources in GLPixelBuffer (LP: #1256360)

[Kevin DuBois (kdub)]

looks good

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1538
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1253/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1500
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1498
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1076
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/985
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/985/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/990
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/990/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1077
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1077/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/995
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5645

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1253/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Great!

review: Approve

[Alexandros Frantzis (afrantzis)]

One caveat: to be completely safe, we need to ensure that the gl_context is not current to any other thread (e.g. the snapshoting thread) when we make current in the destructor. You would guess that destroying a thread with a current context (e.g. when the internal snapshotting thread is destroyed) would unbind that context, but unfortunately, at least in Mesa, that's not the case. That is:

T1: create context C
T2: make current C
T2: ~destroy
T1: make current C => fails at least with Mesa, haven't tried on the phone

This sounds like a problem with Mesa since the current context is thread specific state, and it make no sense to keep a context associated with a non-existent thread, but perhaps the spec is vague and Mesa interprets it differently. We should verify the expected behavior with Khronos and fix Mesa if needed.

=== modified file 'src/server/scene/gl_pixel_buffer.cpp'
--- src/server/scene/gl_pixel_buffer.cpp	2014-03-06 06:05:17 +0000
+++ src/server/scene/gl_pixel_buffer.cpp	2014-04-04 22:59:35 +0000
@@ -65,6 +65,13 @@
 
 ms::GLPixelBuffer::~GLPixelBuffer() noexcept
 {
+    /*
+     * This may be called from a different thread
+     * than the one that called prepare
+     */
+    if (tex != 0 || fbo != 0)
+        gl_context->make_current();
+
     if (tex != 0)
         glDeleteTextures(1, &tex);
     if (fbo != 0)

=== modified file 'tests/unit-tests/scene/test_gl_pixel_buffer.cpp'
--- tests/unit-tests/scene/test_gl_pixel_buffer.cpp	2014-03-12 02:46:58 +0000
+++ tests/unit-tests/scene/test_gl_pixel_buffer.cpp	2014-04-04 22:59:35 +0000
@@ -138,6 +138,11 @@
         EXPECT_CALL(mock_gl, glReadPixels(0, 0, width, height,
                                           GL_BGRA_EXT, GL_UNSIGNED_BYTE, _))
             .WillOnce(FillPixels());
+
+        /* at destruction */
+        EXPECT_CALL(mock_context, make_current());
+        EXPECT_CALL(mock_gl, glDeleteTextures(_,_));
+        EXPECT_CALL(mock_gl, glDeleteFramebuffers(_,_));
     }
 
     ms::GLPixelBuffer pixels{std::move(context)};
@@ -194,6 +199,11 @@
         EXPECT_CALL(mock_gl, glReadPixels(0, 0, width, height,
                                           GL_RGBA, GL_UNSIGNED_BYTE, _))
             .WillOnce(FillPixelsRGBA());
+
+        /* at destruction */
+        EXPECT_CALL(mock_context, make_current());
+        EXPECT_CALL(mock_gl, glDeleteTextures(_,_));
+        EXPECT_CALL(mock_gl, glDeleteFramebuffers(_,_));
     }
 
     ms::GLPixelBuffer pixels{std::move(context)};