Merge lp:~kdub/mir/snapshot-error-checking into lp:mir

[Kevin DuBois]

provide the n7 relief from locking up (as in bug 1238695).

This is not a full fix to this problem, as it simply fails nonfatally instead of fatally when we hit the problem that the nexus7 cannot glFramebufferTexture2d with an eglImage (GL_OUT_OF_MEMORY is generated in glEGLTargetTexture2D)

provide the n7 relief from locking up (as in bug 1238695).

This is not a full fix to this problem, as it simply fails nonfatally instead of fatally when we hit the problem that the nexus7 cannot glFramebufferTexture2d with an eglImage (GL_OUT_OF_MEMORY is generated in glEGLTargetTexture2D)

tested with n4 (working as before) and n7 (no screenshots, but unity8 does not go down all the time)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1317
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/617/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/567
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/563
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/171/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/343
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/343/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/346
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/346/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/171
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/171/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/195/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2741

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1317
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/618/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/568
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/564
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/172
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/344
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/344/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/347
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/347/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/172
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/172/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/196
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2743

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

review: Approve (continuous-integration)

Unmerged revisions

1317. By Kevin DuBois on 2014-01-06

cleanup printf

1316. By Kevin DuBois on 2014-01-06

tests to pass

1315. By Kevin DuBois on 2014-01-06

more tests to check gl error codes more often

1314. By Kevin DuBois on 2014-01-06

test to check bad bind

1313. By Kevin DuBois on 2014-01-06

test to pass

1312. By Kevin DuBois on 2014-01-06

add a test to check the driver before taking a SS

=== modified file 'src/platform/graphics/android/buffer.cpp'
--- src/platform/graphics/android/buffer.cpp	2013-12-18 02:19:19 +0000
+++ src/platform/graphics/android/buffer.cpp	2014-01-06 21:37:02 +0000
@@ -96,7 +96,7 @@
                                   native_buffer->anwb(), image_attrs);
         if (image == EGL_NO_IMAGE_KHR)
         {
-            BOOST_THROW_EXCEPTION(std::runtime_error("error binding buffer to texture\n"));
+            BOOST_THROW_EXCEPTION(std::runtime_error("error creating EGLImage from buffer\n"));
         }
         egl_image_map[disp] = image;
     }
@@ -105,8 +105,12 @@
         image = it->second;
     }
 
+    glGetError(); //clear error code before checking
     egl_extensions->glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, image);
-
+    if(glGetError() != GL_NO_ERROR)
+    {
+        BOOST_THROW_EXCEPTION(std::runtime_error("error binding buffer to texture\n"));
+    }
     //TODO: we should make use of the android egl fence extension here to update the fence.
     //      if the extension is not available, we should pass out a token that the user
     //      will have to keep until the completion of the gl draw

=== modified file 'src/server/scene/gl_pixel_buffer.cpp'
--- src/server/scene/gl_pixel_buffer.cpp	2013-12-18 02:19:19 +0000
+++ src/server/scene/gl_pixel_buffer.cpp	2014-01-06 21:37:02 +0000
@@ -71,7 +71,19 @@
         glDeleteFramebuffers(1, &fbo);
 }
 
-void ms::GLPixelBuffer::prepare()
+void ms::GLPixelBuffer::handle_error()
+{
+    unsigned const int green_argb = 0xFF33FF33;
+    pixels.resize(sizeof(unsigned int));
+    size_ = geom::Size{1,1};
+
+    auto pix = reinterpret_cast<unsigned int*>(pixels.data());
+    *pix = green_argb;
+
+    glBindFramebuffer(GL_FRAMEBUFFER, 0);
+}
+
+void ms::GLPixelBuffer::fill_from(graphics::Buffer& buffer)
 {
     gl_context->make_current();
 
@@ -85,23 +97,34 @@
         glGenFramebuffers(1, &fbo);
 
     glBindFramebuffer(GL_FRAMEBUFFER, fbo);
-}
 
-void ms::GLPixelBuffer::fill_from(graphics::Buffer& buffer)
-{
     auto width = buffer.size().width.as_uint32_t();
     auto height = buffer.size().height.as_uint32_t();
 
     pixels.resize(width * height * 4);
 
-    prepare();
-
-    buffer.bind_to_texture();
+    try
+    {
+        buffer.bind_to_texture();
+    }
+    catch (std::runtime_error& e)
+    {
+        handle_error();
+        throw e;
+    }
 
     glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, tex, 0);
 
+    if ((GL_FRAMEBUFFER_COMPLETE != glCheckFramebufferStatus(GL_FRAMEBUFFER)) ||
+        (GL_NO_ERROR != glGetError()))
+        
+    {
+        handle_error();
+        BOOST_THROW_EXCEPTION(std::runtime_error(
+            "fbo is incomplete or not supported. cannot take snapshot"));
+    }
+
     /* First try to get pixels as BGRA */
-    glGetError();
     gl_pixel_format = GL_BGRA_EXT;
     glReadPixels(0, 0, width, height, gl_pixel_format, GL_UNSIGNED_BYTE, pixels.data());
 

=== modified file 'src/server/scene/gl_pixel_buffer.h'
--- src/server/scene/gl_pixel_buffer.h	2013-12-18 02:19:19 +0000
+++ src/server/scene/gl_pixel_buffer.h	2014-01-06 21:37:02 +0000
@@ -49,7 +49,7 @@
     geometry::Stride stride() const;
 
 private:
-    void prepare();
+    void handle_error();
     void copy_and_convert_pixel_line(char* src, char* dst);
 
     std::unique_ptr<graphics::GLContext> const gl_context;

=== modified file 'src/server/scene/threaded_snapshot_strategy.cpp'
--- src/server/scene/threaded_snapshot_strategy.cpp	2013-12-18 02:19:19 +0000
+++ src/server/scene/threaded_snapshot_strategy.cpp	2014-01-06 21:37:02 +0000
@@ -75,7 +75,13 @@
         wi.surface_buffer_access->with_most_recent_buffer_do(
             [this](graphics::Buffer& buffer)
             {
-                pixels->fill_from(buffer);
+                try
+                {
+                    pixels->fill_from(buffer);
+                } catch (...)
+                {
+                    // todo: log or rethrow
+                }
             });
 
 

=== modified file 'tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp'
--- tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp	2014-01-06 21:37:02 +0000
@@ -19,6 +19,7 @@
 #include "src/platform/graphics/android/buffer.h"
 #include "mir/graphics/egl_extensions.h"
 #include "mir_test_doubles/mock_egl.h"
+#include "mir_test_doubles/mock_gl.h"
 #include "mir_test_doubles/mock_fence.h"
 #include "mir_test_doubles/mock_android_native_buffer.h"
 
@@ -48,6 +49,7 @@
     MirPixelFormat pf;
 
     testing::NiceMock<mtd::MockEGL> mock_egl;
+    testing::NiceMock<mtd::MockGL> mock_gl;
     std::shared_ptr<mg::EGLExtensions> extensions;
     std::shared_ptr<mtd::MockAndroidNativeBuffer> mock_native_buffer;
 };
@@ -349,3 +351,20 @@
     mga::Buffer buffer(mock_native_buffer, extensions);
     buffer.bind_to_texture();
 }
+
+
+TEST_F(AndroidBufferBinding, error_code_is_checked_after_bind)
+{
+    using namespace testing;
+
+    EXPECT_CALL(mock_egl, glEGLImageTargetTexture2DOES(_, _))
+        .Times(1);
+    EXPECT_CALL(mock_gl, glGetError())
+        .Times(2)
+        .WillRepeatedly(Return(GL_OUT_OF_MEMORY));
+
+    mga::Buffer buffer(mock_native_buffer, extensions);
+    EXPECT_THROW({
+        buffer.bind_to_texture();
+    }, std::runtime_error);
+}

=== modified file 'tests/unit-tests/scene/test_gl_pixel_buffer.cpp'
--- tests/unit-tests/scene/test_gl_pixel_buffer.cpp	2013-12-20 05:06:28 +0000
+++ tests/unit-tests/scene/test_gl_pixel_buffer.cpp	2014-01-06 21:37:02 +0000
@@ -110,6 +110,107 @@
     EXPECT_EQ(geom::Stride(), pixels.stride());
 }
 
+/* specifically, the tegra3 based N7 generates an out of memory error when binding to texture */
+TEST_F(GLPixelBufferTest, abort_on_bad_texture_bind)
+{
+    using namespace testing;
+    GLuint const tex{10};
+    GLuint const fbo{20};
+    {
+        InSequence s;
+
+        /* The GL context is made current */
+        EXPECT_CALL(mock_context, make_current());
+
+        /* The texture and framebuffer are prepared */
+        EXPECT_CALL(mock_gl, glGenTextures(_,_))
+            .WillOnce(SetArgPointee<1>(tex));
+        EXPECT_CALL(mock_gl, glBindTexture(_,tex));
+        EXPECT_CALL(mock_gl, glGenFramebuffers(_,_))
+            .WillOnce(SetArgPointee<1>(fbo));
+        EXPECT_CALL(mock_gl, glBindFramebuffer(_,fbo));
+
+        EXPECT_CALL(mock_buffer, bind_to_texture())
+            .Times(1)
+            .WillOnce(Throw(std::runtime_error("bad bind.\n")));
+
+        /* recover default FB */
+        EXPECT_CALL(mock_gl, glBindFramebuffer(_,0));
+
+        /* do not call these if the bind was not okay */
+        EXPECT_CALL(mock_gl, glFramebufferTexture2D(_,_,_,_,_))
+            .Times(0);
+        EXPECT_CALL(mock_gl, glReadPixels(_,_,_,_,_,_,_))
+            .Times(0);
+    }
+
+    ms::GLPixelBuffer pixels{std::move(context)};
+
+    geom::Size bkp_size{1,1};
+    geom::Stride bkp_stride{4};
+    unsigned int pixel_color = 0xFF33FF33;
+
+    EXPECT_THROW({
+        pixels.fill_from(mock_buffer);
+    }, std::runtime_error);
+    auto data = pixels.as_argb_8888();
+    
+    EXPECT_EQ(bkp_size, pixels.size());
+    EXPECT_EQ(bkp_stride, pixels.stride());
+    ASSERT_NE(nullptr, data);
+    EXPECT_EQ(pixel_color, static_cast<uint32_t const*>(data)[0]);
+}
+
+TEST_F(GLPixelBufferTest, unable_to_bind_fb_results_in_dark_green_pixel)
+{
+    using namespace testing;
+    GLuint const tex{10};
+    GLuint const fbo{20};
+    {
+        InSequence s;
+
+        /* The GL context is made current */
+        EXPECT_CALL(mock_context, make_current());
+
+        /* The texture and framebuffer are prepared */
+        EXPECT_CALL(mock_gl, glGenTextures(_,_))
+            .WillOnce(SetArgPointee<1>(tex));
+        EXPECT_CALL(mock_gl, glBindTexture(_,tex));
+        EXPECT_CALL(mock_gl, glGenFramebuffers(_,_))
+            .WillOnce(SetArgPointee<1>(fbo));
+        EXPECT_CALL(mock_gl, glBindFramebuffer(_,fbo));
+        EXPECT_CALL(mock_buffer, bind_to_texture());
+        EXPECT_CALL(mock_gl, glFramebufferTexture2D(_,_,_,_,_));
+        EXPECT_CALL(mock_gl, glCheckFramebufferStatus(GL_FRAMEBUFFER))
+            .Times(1)
+            .WillOnce(Return(GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT));
+
+        /* recover default FB */
+        EXPECT_CALL(mock_gl, glBindFramebuffer(_,0));
+
+        /* do not try to read if the fbo status is not okay */
+        EXPECT_CALL(mock_gl, glReadPixels(_,_,_,_,_,_,_))
+            .Times(0);
+    }
+
+    ms::GLPixelBuffer pixels{std::move(context)};
+
+    geom::Size bkp_size{1,1};
+    geom::Stride bkp_stride{4};
+    unsigned int pixel_color = 0xFF33FF33;
+
+    EXPECT_THROW({
+        pixels.fill_from(mock_buffer);
+    }, std::runtime_error);
+    auto data = pixels.as_argb_8888();
+
+    
+    EXPECT_EQ(bkp_size, pixels.size());
+    EXPECT_EQ(bkp_stride, pixels.stride());
+    ASSERT_NE(nullptr, data);
+    EXPECT_EQ(pixel_color, static_cast<uint32_t const*>(data)[0]);
+}
+
 TEST_F(GLPixelBufferTest, returns_data_from_bgra_buffer_texture)
 {
     using namespace testing;
@@ -135,6 +236,8 @@
         /* The buffer texture is read as BGRA */
         EXPECT_CALL(mock_buffer, bind_to_texture());
         EXPECT_CALL(mock_gl, glFramebufferTexture2D(_,_,_,tex,0));
+        EXPECT_CALL(mock_gl, glCheckFramebufferStatus(GL_FRAMEBUFFER))
+            .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE));
         EXPECT_CALL(mock_gl, glReadPixels(0, 0, width, height,
                                           GL_BGRA_EXT, GL_UNSIGNED_BYTE, _))
             .WillOnce(FillPixels());
@@ -184,6 +287,10 @@
         /* Try to read the FBO as BGRA but fail */
         EXPECT_CALL(mock_buffer, bind_to_texture());
         EXPECT_CALL(mock_gl, glFramebufferTexture2D(_,_,_,tex,0));
+        EXPECT_CALL(mock_gl, glCheckFramebufferStatus(GL_FRAMEBUFFER))
+            .Times(1)
+            .WillOnce(Return(GL_FRAMEBUFFER_COMPLETE));
+
         EXPECT_CALL(mock_gl, glGetError())
             .WillOnce(Return(GL_NO_ERROR));
         EXPECT_CALL(mock_gl, glReadPixels(0, 0, width, height,