Merge lp:~afrantzis/mir/gbm-alloc-validate-buffer-format into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

gbm: Don't try to allocate buffers with unsupported formats

gbm: Don't try to allocate buffers with unsupported formats

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:936
http://jenkins.qa.ubuntu.com/job/mir-ci/1241/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1639
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1524
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/479
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/479/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

17 +uint32_t mgg::invalid_gbm_format()
18 +{
19 + /* There is no explicit invalid GBM pixel format! */
20 + return std::numeric_limits<uint32_t>::max();
21 +}

Is there any need for this to be a function (as opposed to an enum constant)?

review: Approve

[lp:~afrantzis/mir/gbm-alloc-validate-buffer-format]

937. By Alexandros Frantzis on 2013-08-07

gbm: Use an enumeration constant for invalid_gbm_format

[Alexandros Frantzis (afrantzis)]

> Is there any need for this to be a function (as opposed to an enum constant)?

Fixed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:937
http://jenkins.qa.ubuntu.com/job/mir-ci/1244/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1643
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1528
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/482
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/482/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'src/server/graphics/gbm/gbm_buffer.cpp'
--- src/server/graphics/gbm/gbm_buffer.cpp	2013-07-22 02:18:21 +0000
+++ src/server/graphics/gbm/gbm_buffer.cpp	2013-08-07 11:53:28 +0000
@@ -27,7 +27,6 @@
 #include <boost/exception/errinfo_errno.hpp>
 #include <boost/throw_exception.hpp>
 
-#include <limits>
 #include <stdexcept>
 
 namespace mgg=mir::graphics::gbm;
@@ -68,8 +67,7 @@
         gbm_pf = GBM_FORMAT_XRGB8888;
         break;
     default:
-        /* There is no explicit invalid GBM pixel format! */
-        gbm_pf = std::numeric_limits<uint32_t>::max();
+        gbm_pf = mgg::invalid_gbm_format;
         break;
     }
 

=== modified file 'src/server/graphics/gbm/gbm_buffer.h'
--- src/server/graphics/gbm/gbm_buffer.h	2013-07-22 02:18:21 +0000
+++ src/server/graphics/gbm/gbm_buffer.h	2013-08-07 11:53:28 +0000
@@ -25,6 +25,7 @@
 #include <gbm.h>
 
 #include <memory>
+#include <limits>
 
 namespace mir
 {
@@ -35,6 +36,7 @@
 
 geometry::PixelFormat gbm_format_to_mir_format(uint32_t format);
 uint32_t mir_format_to_gbm_format(geometry::PixelFormat format);
+enum : uint32_t { invalid_gbm_format = std::numeric_limits<uint32_t>::max() };
 
 class BufferTextureBinder;
 

=== modified file 'src/server/graphics/gbm/gbm_buffer_allocator.cpp'
--- src/server/graphics/gbm/gbm_buffer_allocator.cpp	2013-07-31 14:01:17 +0000
+++ src/server/graphics/gbm/gbm_buffer_allocator.cpp	2013-08-07 11:53:28 +0000
@@ -31,6 +31,7 @@
 #include <GLES2/gl2.h>
 #include <GLES2/gl2ext.h>
 
+#include <algorithm>
 #include <stdexcept>
 #include <gbm.h>
 #include <cassert>
@@ -119,6 +120,15 @@
 {
     uint32_t bo_flags{GBM_BO_USE_RENDERING};
 
+    uint32_t gbm_format = mgg::mir_format_to_gbm_format(buffer_properties.format);
+
+    if (!is_pixel_format_supported(buffer_properties.format) ||
+        gbm_format == mgg::invalid_gbm_format)
+    {
+        BOOST_THROW_EXCEPTION(
+            std::runtime_error("Trying to create GBM buffer with unsupported pixel format"));
+    }
+
     /* Create the GBM buffer object */
     if (buffer_properties.usage == BufferUsage::software)
         bo_flags |= GBM_BO_USE_WRITE;
@@ -127,7 +137,7 @@
         platform->gbm.device,
         buffer_properties.size.width.as_uint32_t(),
         buffer_properties.size.height.as_uint32_t(),
-        mgg::mir_format_to_gbm_format(buffer_properties.format),
+        gbm_format,
         bo_flags);
 
     if (!bo_raw)
@@ -155,3 +165,12 @@
 
     return pixel_formats;
 }
+
+bool mgg::GBMBufferAllocator::is_pixel_format_supported(geom::PixelFormat format)
+{
+    auto formats = supported_pixel_formats();
+
+    auto iter = std::find(formats.begin(), formats.end(), format);
+
+    return iter != formats.end();
+}

=== modified file 'src/server/graphics/gbm/gbm_buffer_allocator.h'
--- src/server/graphics/gbm/gbm_buffer_allocator.h	2013-07-31 11:18:25 +0000
+++ src/server/graphics/gbm/gbm_buffer_allocator.h	2013-08-07 11:53:28 +0000
@@ -47,6 +47,8 @@
     std::vector<geometry::PixelFormat> supported_pixel_formats();
 
 private:
+    bool is_pixel_format_supported(geometry::PixelFormat format);
+
     std::shared_ptr<GBMPlatform> platform;
     std::shared_ptr<graphics::BufferInitializer> buffer_initializer;
     std::shared_ptr<EGLExtensions> const egl_extensions;

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_buffer_allocator.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_buffer_allocator.cpp	2013-07-31 14:01:17 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_buffer_allocator.cpp	2013-08-07 11:53:28 +0000
@@ -112,16 +112,6 @@
     allocator->alloc_buffer(mg::BufferProperties{size, geom::PixelFormat::xrgb_8888, usage});
 }
 
-TEST_F(GBMBufferAllocatorTest, correct_buffer_format_translation_abgr_8888_to_invalid)
-{
-    using namespace testing;
-
-    EXPECT_CALL(mock_gbm, gbm_bo_create(_,_,_,std::numeric_limits<uint32_t>::max(),_));
-    EXPECT_CALL(mock_gbm, gbm_bo_destroy(_));
-
-    allocator->alloc_buffer(mg::BufferProperties{size, geom::PixelFormat::abgr_8888, usage});
-}
-
 MATCHER_P(has_flag_set, flag, "")
 {
     return arg & flag;
@@ -244,3 +234,15 @@
     ASSERT_FALSE(supported_pixel_formats.empty());
     EXPECT_EQ(geom::PixelFormat::argb_8888, supported_pixel_formats[0]);
 }
+
+TEST_F(GBMBufferAllocatorTest, alloc_with_unsupported_pixel_format_throws)
+{
+    using namespace testing;
+
+    /* We shouldn't try to create a buffer with an unsupported format */
+    EXPECT_CALL(mock_gbm, gbm_bo_create(_,_,_,_,_)).Times(0);
+
+    EXPECT_THROW({
+        allocator->alloc_buffer(mg::BufferProperties{size, geom::PixelFormat::abgr_8888, usage});
+    }, std::runtime_error);
+}