Merge lp:~mir-team/mir/fix-1475571 into lp:mir

[Daniel van Vugt]

Don't throw when gbm_device_is_format_supported returns false. We might have given it the scanout flag spuriously thanks to our weak heuristic, making it report false incorrectly. (LP: #1475571)

But rather than fixing the bo_flags, we can drop the whole call. Just use the existing error checking on the gbm_bo_create() call instead.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2755
http://jenkins.qa.ubuntu.com/job/mir-ci/4350/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3205
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/58/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/730/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3153
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/499/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3153
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3153/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5957
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21965

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

That check is correct and should probably not be deleted.

I suspect the issue is now we're more careful with pixel format semantics, an invalid format used screencast is being detected. This could be a simple matter of accidentally requesting RGBA for an RGBX framebuffer or mixed up byte ordering. I'm somewhat confident the bug lies in mirscreencast but have not looked yet. Will do so today...

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Oh I see. It's probably the bo_flags getting GBM_BO_USE_SCANOUT due to the size of the surface (which is a very ugly heuristic). With the SCANOUT flag, gbm will refuse RGBA and only allow RGBX. That's spurious because we actually have no desire for scan-out support in screencasting and there might be a real alpha channel that we want.

I think if gbm_bo_create throws adequate errors still then this change is fine. Providing it passes CI.

review: Approve

[Daniel van Vugt (vanvugt)]

An alternate fix could be to change:
  gbm_device_is_format_supported(device, gbm_format, bo_flags)
to:
  gbm_device_is_format_supported(device, gbm_format, 0)

[Daniel van Vugt (vanvugt)]

Note the lack of a replacement regression test. That's because one already exists (out of view of the diff):

TEST_F(MesaBufferAllocatorTest, throws_on_buffer_creation_failure)

[Daniel van Vugt (vanvugt)]

review: Approve

[Chris Halse Rogers (raof)]

If it really is the GBM_BO_USE_SCANOUT flag making the difference, then wouldn't it be a better idea to fix the allocator to not try to allocate a scanout buffer for screencasts (which doesn't make sense anyway).

I'd be concerned that just dropping the check will instead bite us later when we try and scanout from a BO that's not scanout-capable.

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2757
http://jenkins.qa.ubuntu.com/job/mir-ci/4354/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3212
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/65
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/737
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/503
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/503/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3160
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3160/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5964
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21973

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2758
http://jenkins.qa.ubuntu.com/job/mir-ci/4355/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3213
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/66
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/738
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/504
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/504/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3161
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3161/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5965
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21974

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Chris: We never had that check before. I only introduced it last week so dropping it loses nothing. And the logic it replaced was even worse.

If buffer creation is going to fail, let it fail. We check for that already and should never have been trying to predict if it will fail before it does.

[Chris Halse Rogers (raof)]

Yeah, fair enough. If gbm_bo_create doesn't return a scanoutable buffer when asked to provide one that's probably a gbm bug.

26 +TEST_F(MesaBufferAllocatorTest, is_format_supported_not_called)
should just be removed. We don't need to test that we don't call a function. It's a duplicate of the throws_on_buffer_creation_failure test a couple of tests up.

If you really want to test something, test that buffer allocation succeeds even if gbm_device_is_format_supported returns false. I don't think that's particularly necessary, though.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

26 +TEST_F(MesaBufferAllocatorTest, is_format_supported_not_called)

The test name doesn't reflect a useful intent. I'm open argument that the test is useful for other reasons - with an appropriate name.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

That's just a grammatical naming ambiguity (which isn't ambiguous after you look at the test). It actually means:
   gbm_device_is_format_supported__is_not_called

But that was too long to fit on one line, IIRC.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2760
http://jenkins.qa.ubuntu.com/job/mir-ci/4370/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3231
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/84
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/756
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3179
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/519
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/519/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3179
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3179/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5981
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22005

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Daniel van Vugt (vanvugt)]

Given the last change made was exactly what Chris requested, I'll take that as final approval.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/platforms/mesa/server/common/buffer_allocator.cpp'
--- src/platforms/mesa/server/common/buffer_allocator.cpp	2015-07-16 07:03:19 +0000
+++ src/platforms/mesa/server/common/buffer_allocator.cpp	2015-07-21 04:11:41 +0000
@@ -157,12 +157,6 @@
         bo_flags |= GBM_BO_USE_SCANOUT;
     }
 
-    if (!gbm_device_is_format_supported(device, gbm_format, bo_flags))
-    {
-        BOOST_THROW_EXCEPTION(
-            std::runtime_error("Trying to create GBM buffer with unsupported pixel format"));
-    }
-
     gbm_bo *bo_raw = gbm_bo_create(
         device,
         buffer_properties.size.width.as_uint32_t(),

=== modified file 'tests/unit-tests/graphics/mesa/common/test_buffer_allocator.cpp'
--- tests/unit-tests/graphics/mesa/common/test_buffer_allocator.cpp	2015-07-16 07:03:19 +0000
+++ tests/unit-tests/graphics/mesa/common/test_buffer_allocator.cpp	2015-07-21 04:11:41 +0000
@@ -271,18 +271,22 @@
     EXPECT_EQ(mir_pixel_format_argb_8888, supported_pixel_formats[0]);
 }
 
-TEST_F(MesaBufferAllocatorTest, alloc_with_unsupported_pixel_format_throws)
-{
+TEST_F(MesaBufferAllocatorTest, screencast_can_create_buffer)
+{   // Regression test for LP: #1475571
     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_CALL(mock_gbm, gbm_device_is_format_supported(_,_,_))
-        .WillOnce(Return(0));
-
-    EXPECT_THROW({
-        allocator->alloc_buffer(mg::BufferProperties{size, mir_pixel_format_abgr_8888, usage});
-    }, std::runtime_error);
+    // Not expected to be called any more, but if it is...
+    ON_CALL(mock_gbm, gbm_device_is_format_supported(_,_,GBM_BO_USE_SCANOUT))
+        .WillByDefault(Return(0));
+
+    EXPECT_CALL(mock_gbm, gbm_bo_create(_,_,_,_,_));
+
+    EXPECT_NO_THROW({
+        allocator->alloc_buffer(
+            mg::BufferProperties{{1920,1080},
+                                 mir_pixel_format_abgr_8888,
+                                 mg::BufferUsage::hardware});
+    });
 }
 
 MATCHER_P(GbmImportMatch, value, "import data matches")