Merge lp:~afrantzis/mir/different-mesa-display-validation-functions into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

gbm: Provide different functions for validating server and client Mesa EGL native displays

gbm: Provide different functions for validating server and client Mesa EGL native displays

[Alan Griffiths (alan-griffiths)]

I assume the next step is to make use of these different functions?

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:818
http://jenkins.qa.ubuntu.com/job/mir-ci/922/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1186
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1070
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/160
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/160/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> I assume the next step is to make use of these different functions?

The related change in Mesa to use these was merged earlier today: https://github.com/RAOF/mesa/pull/2

People will need to upgrade to the new Mesa after this is merged (but they update Mesa can be used even without this change). I will send an email to the mir-devel to notify about the update.

[Kevin DuBois (kdub)]

+1!

review: Approve

[Daniel van Vugt (vanvugt)]

Can we find a way to make the new function names less wordy and more consistent?...

mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)

By wordy, I mean they're almost too long. By consistent I mean the words are in a different order in the function name to the parameter: egl_mesa vs MesaEGL

Another inconsistency is use of the word "client". No other client functions do that. So maybe drop "_client_". But admittedly that would be inconsistent too.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Following further discussion on IRC, I am not very opinionated on this. Land it when you've made a decision.

review: Abstain

=== modified file 'include/shared/mir_toolkit/mesa/native_display.h'
--- include/shared/mir_toolkit/mesa/native_display.h	2013-06-13 21:24:29 +0000
+++ include/shared/mir_toolkit/mesa/native_display.h	2013-07-05 14:12:25 +0000
@@ -52,8 +52,7 @@
                                    MirSurfaceParameters* surface_parameters);
 };
 
-int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);
-int mir_server_internal_display_is_valid(MirMesaEGLNativeDisplay* display);
+typedef int (*MirMesaEGLNativeDisplayIsValidFunc)(MirMesaEGLNativeDisplay* display);
 
 #ifdef __cplusplus
 } // extern "C"

=== modified file 'src/client/gbm/mesa_native_display_container.cpp'
--- src/client/gbm/mesa_native_display_container.cpp	2013-06-13 22:10:04 +0000
+++ src/client/gbm/mesa_native_display_container.cpp	2013-07-05 14:12:25 +0000
@@ -18,7 +18,6 @@
 
 #include "mesa_native_display_container.h"
 
-#include "mir_toolkit/mesa/native_display.h"
 #include "mir_toolkit/mir_client_library.h"
 
 #include <cstring>
@@ -41,7 +40,7 @@
     return MIR_MESA_TRUE;
 }
 
-int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
+int mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
 {
     return mcl::EGLNativeDisplayContainer::instance().validate(display);
 }

=== modified file 'src/client/gbm/mesa_native_display_container.h'
--- src/client/gbm/mesa_native_display_container.h	2013-06-13 22:10:04 +0000
+++ src/client/gbm/mesa_native_display_container.h	2013-07-05 14:12:25 +0000
@@ -22,6 +22,7 @@
 #include "../egl_native_display_container.h"
 
 #include "mir_toolkit/client_types.h"
+#include "mir_toolkit/mesa/native_display.h"
 
 #include <unordered_set>
 #include <mutex>
@@ -30,9 +31,9 @@
 {
 namespace client
 {
-
 namespace gbm
 {
+
 class MesaNativeDisplayContainer : public EGLNativeDisplayContainer
 {
 public:
@@ -52,6 +53,9 @@
     std::mutex mutable guard;
     std::unordered_set<MirEGLNativeDisplayType> valid_displays;
 };
+
+extern "C" int mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);
+
 }
 }
 } // namespace mir

=== modified file 'src/server/graphics/gbm/gbm_platform.cpp'
--- src/server/graphics/gbm/gbm_platform.cpp	2013-07-03 17:48:30 +0000
+++ src/server/graphics/gbm/gbm_platform.cpp	2013-07-05 14:12:25 +0000
@@ -153,20 +153,8 @@
     return std::make_shared<mgg::GBMPlatform>(report, vt);
 }
 
-extern "C"
-{
-int mir_server_internal_display_is_valid(MirMesaEGLNativeDisplay* display)
+extern "C" int mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
 {
     return ((mgg::GBMPlatform::internal_display_clients_present) &&
             (display == mgg::GBMPlatform::internal_native_display.get()));
 }
-
-/* TODO: this function is a bit fragile because libmirserver and libmirclient both have very different
- *       implementations and both have symbols for it.
- *       bug filed: lp:1177902
- */
-int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
-{
-    return mir_server_internal_display_is_valid(display);
-}
-}

=== modified file 'src/server/graphics/gbm/gbm_platform.h'
--- src/server/graphics/gbm/gbm_platform.h	2013-06-21 07:47:57 +0000
+++ src/server/graphics/gbm/gbm_platform.h	2013-07-05 14:12:25 +0000
@@ -68,6 +68,8 @@
     static std::shared_ptr<InternalNativeDisplay> internal_native_display;
 };
 
+extern "C" int mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);
+
 }
 }
 }

=== modified file 'tests/unit-tests/client/gbm/test_gbm_client_platform.cpp'
--- tests/unit-tests/client/gbm/test_gbm_client_platform.cpp	2013-06-05 22:52:53 +0000
+++ tests/unit-tests/client/gbm/test_gbm_client_platform.cpp	2013-07-05 14:12:25 +0000
@@ -18,6 +18,7 @@
 
 #include "src/client/client_platform.h"
 #include "src/client/native_client_platform_factory.h"
+#include "src/client/gbm/mesa_native_display_container.h"
 #include "mir_test_doubles/mock_client_context.h"
 #include "mir_test_doubles/mock_client_surface.h"
 
@@ -26,13 +27,10 @@
 #include <gtest/gtest.h>
 
 namespace mcl = mir::client;
+namespace mclg = mir::client::gbm;
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
-/* TODO: mir_egl_mesa_display_is_valid is a bit fragile because libmirserver and libmirclient both have very
- *       different implementations and both have symbols for it. If the linking order of the test changes,
- *       specifically, if mir_egl_mesa_display_is_valid resolves into libmirserver, then this test will break. 
- */
 TEST(GBMClientPlatformTest, egl_native_display_is_valid_until_released)
 {
     mtd::MockClientContext context;
@@ -44,7 +42,7 @@
         std::shared_ptr<EGLNativeDisplayType> native_display = platform->create_egl_native_display();
 
         nd = reinterpret_cast<MirMesaEGLNativeDisplay*>(*native_display);
-        EXPECT_TRUE(mir_egl_mesa_display_is_valid(nd));
+        EXPECT_TRUE(mclg::mir_client_egl_mesa_display_is_valid(nd));
     }
-    EXPECT_FALSE(mir_egl_mesa_display_is_valid(nd));
+    EXPECT_FALSE(mclg::mir_client_egl_mesa_display_is_valid(nd));
 }

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_platform.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_platform.cpp	2013-07-01 06:53:02 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_platform.cpp	2013-07-05 14:12:25 +0000
@@ -190,21 +190,17 @@
     }, std::runtime_error);
 }
 
-/* TODO: this function is a bit fragile because libmirserver and libmirclient both have very different
- *       implementations and both have symbols for it. If the linking order of the test changes,
- *       specifically, if mir_egl_mesa_display_is_valid resolves into libmirclient, then this test will break. 
- */
 TEST_F(GBMGraphicsPlatform, platform_provides_validation_of_display_for_internal_clients)
 {
     MirMesaEGLNativeDisplay* native_display = nullptr;
-    EXPECT_EQ(0, mir_server_internal_display_is_valid(native_display));
+    EXPECT_EQ(0, mgg::mir_server_egl_mesa_display_is_valid(native_display));
     {
         auto platform = create_platform();
         auto client = platform->create_internal_client();
         native_display = reinterpret_cast<MirMesaEGLNativeDisplay*>(client->egl_native_display());
-        EXPECT_EQ(1, mir_server_internal_display_is_valid(native_display));
+        EXPECT_EQ(1, mgg::mir_server_egl_mesa_display_is_valid(native_display));
     }
-    EXPECT_EQ(0, mir_server_internal_display_is_valid(native_display));
+    EXPECT_EQ(0, mgg::mir_server_egl_mesa_display_is_valid(native_display));
 }
 
 namespace