Merge lp:~kdub/mir/fix-1158564 into lp:~mir-team/mir/trunk

[Kevin DuBois]

fixes lp:1158564 (integration test failure)

(test bug caused destruction to go out-of-order, and some fields for the software buffers were not filled out correctly)

these did not cause issue with the sgx/nvidia drivers, but did with the adreno ones

fixes lp:1158564 (integration test failure)

(test bug caused destruction to go out-of-order, and some fields for the software buffers were not filled out correctly)

these did not cause issue with the sgx/nvidia drivers, but did with the adreno ones

[Kevin DuBois (kdub)]

after this, all nex4 tests pass :)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:534
http://jenkins.qa.ubuntu.com/job/mir-ci/200/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/131
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/201
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/201/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

53 + std::shared_ptr<struct alloc_device_t> alloc_device;
54 std::shared_ptr<mga::AndroidBuffer> android_buffer;
55 std::shared_ptr<mga::AndroidBuffer> second_android_buffer;

Yikes. If you're having that issue, what I would suggest doing would be either:

a) Add a comment saying that this needs to be the order and hope it stays up-to-date
b) Wrap it all up into a new class, e.g:

class DrawTestAndroidBuffers
{
    public:

        // We require std::shared_ptr<struct alloc_device_t as a construction parameter to signify that the relevant mga::AndroidBuffers require it to be around during their lifetime.
        DrawTestAndroidBuffers (std::shared_ptr<struct alloc_device_t const&) { ... }

 std::shared_ptr<mga::AndroidBuffer> android_buffer;
 std::shared_ptr<mga::AndroidBuffer> second_android_buffer;
};

class DrawTestAndroidDevice
{
    public:

        std::shared_ptr<struct alloc_device_t> alloc_device;
        DrawTestAndroidBuffers android_buffers;
};

What that does is ensure that in order to initialize the android_buffers, you have to have an alloc_device first. The dependency is certainly implicit, but at least any future maintainer will think twice before changing it, and it makes it more difficult to accidentally change the order.

[Alexandros Frantzis (afrantzis)]

> 53 + std::shared_ptr<struct alloc_device_t> alloc_device;
> 54 std::shared_ptr<mga::AndroidBuffer> android_buffer;
> 55 std::shared_ptr<mga::AndroidBuffer> second_android_buffer;
>
> Yikes. If you're having that issue, what I would suggest doing would be
> either:
>
> a) Add a comment saying that this needs to be the order and hope it stays up-
> to-date
> b) Wrap it all up into a new class, e.g:

I prefer (if nothing is blocking it in this particular case):

c) Make it so that destruction order doesn't matter. If all components that need an alloc_device_t hold a shared_ptr to it, then it will be destroyed at the right time, i.e., when no one needs it anymore.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

It isn't clear to me how all of the changes contribute to the solution.

/1/ Using "t->common.close((hw_device_t*)t)" in place of "gralloc_close(alloc_device.get())" for destruction.

/2/ Using make_shared() in place of fake_shared() to make destruction implicit instead of explicit.

/3/ Moving the shared_ptr member variable.

review: Needs Information

[Sam Spilsbury (smspillaz)]

> > 53 + std::shared_ptr<struct alloc_device_t> alloc_device;
> > 54 std::shared_ptr<mga::AndroidBuffer> android_buffer;
> > 55 std::shared_ptr<mga::AndroidBuffer> second_android_buffer;
> >
> > Yikes. If you're having that issue, what I would suggest doing would be
> > either:
> >
> > a) Add a comment saying that this needs to be the order and hope it stays
> up-
> > to-date
> > b) Wrap it all up into a new class, e.g:
>
> I prefer (if nothing is blocking it in this particular case):
>
> c) Make it so that destruction order doesn't matter. If all components that
> need an alloc_device_t hold a shared_ptr to it, then it will be destroyed at
> the right time, i.e., when no one needs it anymore.

This is also a good solution too as it doesn't seem like there would be an ownership cycle between both the buffers and the allocator here.

Originally I had quickly cast my mind to the case of relying on some implicit side-effect of alloc_device_t being open, and then not being able to have alloc_device_t own the buffers, but that was wrong.

Kevin, please disregard my earlier comment.

[Kevin DuBois (kdub)]

Based on the comments, I changed this to integrate more parts from mga::, namely, AndroidBufferAllocator to take care of allocation. This removes the construction/destruction order dependency and makes the test cleaner

Also, this test is need of some cleanup, I think the bug is addressed well enough by these changes; there are still things that could be cleaned up in this test that are best done in later cleanups.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/mir-ci/213/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/147
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/214
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/214/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

21 #include "src/server/frontend/protobuf_socket_communicator.h"
22 #include "mir/frontend/resource_cache.h"
26 #include "src/server/graphics/android/android_alloc_adaptor.h"
... perhaps more?

Unused headers (some of them were not being used even before the changes in this branch).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> 21 #include "src/server/frontend/protobuf_socket_communicator.h"
> 22 #include "mir/frontend/resource_cache.h"
> 26 #include "src/server/graphics/android/android_alloc_adaptor.h"
> ... perhaps more?
>
> Unused headers (some of them were not being used even before the changes in
> this branch).

Agreed. But this looks much clearer.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:538
http://jenkins.qa.ubuntu.com/job/mir-ci/221/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/155
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/38
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/222
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/222/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Alexandros Frantzis (afrantzis)]

Loosk good.

review: Approve

=== modified file 'tests/draw/android_graphics.cpp'
--- tests/draw/android_graphics.cpp	2013-02-01 19:25:41 +0000
+++ tests/draw/android_graphics.cpp	2013-03-27 15:12:20 +0000
@@ -103,6 +103,7 @@
 {
     native_handle_t* handle;
     handle = (native_handle_t*) malloc(sizeof(int) * ( 3 + package->ipc_data.size() + package->ipc_fds.size() ));
+    handle->version = sizeof(native_handle_t);
     handle->numInts = package->ipc_data.size();
     handle->numFds  = package->ipc_fds.size();
     int i;

=== modified file 'tests/integration-tests/client/test_client_render.cpp'
--- tests/integration-tests/client/test_client_render.cpp	2013-03-22 10:34:16 +0000
+++ tests/integration-tests/client/test_client_render.cpp	2013-03-27 15:12:20 +0000
@@ -18,31 +18,28 @@
 
 #include "mir_test_framework/process.h"
 
-#include "mir_toolkit/mir_client_library.h"
-
 #include "mir/compositor/buffer_ipc_package.h"
-#include "src/server/frontend/protobuf_socket_communicator.h"
-#include "mir/frontend/resource_cache.h"
+#include "mir/compositor/buffer_properties.h"
+#include "mir/graphics/buffer_initializer.h"
 #include "src/server/graphics/android/android_buffer.h"
-#include "src/server/graphics/android/android_alloc_adaptor.h"
+#include "src/server/graphics/android/android_buffer_allocator.h"
 
 #include "mir_test/draw/android_graphics.h"
 #include "mir_test/draw/patterns.h"
 #include "mir_test/stub_server_tool.h"
 #include "mir_test/test_protobuf_server.h"
-#include "mir_test/fake_shared.h"
 
 #include <gmock/gmock.h>
-
 #include <thread>
+#include <hardware/gralloc.h>
 #include <GLES2/gl2.h>
-#include <hardware/gralloc.h>
 
 namespace mtf = mir_test_framework;
 namespace mt=mir::test;
 namespace mtd=mir::test::draw;
 namespace mc=mir::compositor;
 namespace mga=mir::graphics::android;
+namespace mg=mir::graphics;
 namespace geom=mir::geometry;
 
 namespace
@@ -443,21 +440,13 @@
         size = geom::Size{geom::Width{test_width}, geom::Height{test_height}};
         pf = geom::PixelFormat::abgr_8888;
 
-        /* allocate an android buffer */
-        int err;
-        const hw_module_t *hw_module;
-        struct alloc_device_t *alloc_device_raw;
-        err = hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hw_module);
-        if (err < 0)
-            throw std::runtime_error("Could not open hardware module");
-        gralloc_open(hw_module, &alloc_device_raw);
-        alloc_device = mt::fake_shared(*alloc_device_raw);
-        auto alloc_adaptor = std::make_shared<mga::AndroidAllocAdaptor>(alloc_device);
-        buffer_converter = std::make_shared<mtd::TestGrallocMapper>(hw_module, alloc_device.get());
-
-
-        android_buffer = std::make_shared<mga::AndroidBuffer>(alloc_adaptor, size, pf);
-        second_android_buffer = std::make_shared<mga::AndroidBuffer>(alloc_adaptor, size, pf);
+        auto initializer = std::make_shared<mg::NullBufferInitializer>(); 
+        allocator = std::make_shared<mga::AndroidBufferAllocator> (initializer);
+        mc::BufferProperties properties(size, pf, mc::BufferUsage::hardware);
+        android_buffer = allocator->alloc_buffer(properties);
+        second_android_buffer = allocator->alloc_buffer(properties);
+
+        buffer_converter = std::make_shared<mtd::TestGrallocMapper>();
 
         package = android_buffer->get_ipc_package();
         second_package = second_android_buffer->get_ipc_package();
@@ -471,7 +460,6 @@
     void TearDown()
     {
         test_server.reset();
-        gralloc_close(alloc_device.get());
     }
 
     mir::protobuf::Connection response;
@@ -485,11 +473,13 @@
     mc::BufferID id2;
     std::shared_ptr<mtd::TestGrallocMapper> buffer_converter;
     std::shared_ptr<mtf::Process> client_process;
+
     std::shared_ptr<mc::BufferIPCPackage> package;
     std::shared_ptr<mc::BufferIPCPackage> second_package;
-    std::shared_ptr<mga::AndroidBuffer> android_buffer;
-    std::shared_ptr<mga::AndroidBuffer> second_android_buffer;
-    std::shared_ptr<struct alloc_device_t> alloc_device;
+
+    std::shared_ptr<mc::Buffer> android_buffer;
+    std::shared_ptr<mc::Buffer> second_android_buffer;
+    std::shared_ptr<mga::AndroidBufferAllocator>  allocator;
 
     static std::shared_ptr<mtf::Process> render_single_client_process;
     static std::shared_ptr<mtf::Process> render_double_client_process;