Merge lp:~mir-team/mir/expose-debug-buffer-id-to-client into lp:~mir-team/mir/trunk

[Chris Halse Rogers]

Add a little extra debugging API

Add a little extra debugging API to get the buffer ID

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:882
http://jenkins.qa.ubuntu.com/job/mir-ci/1100/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1441
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1326
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/338
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/338/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

could this be in a side header (that we don't give an API guarantee about), maybe mir_client_library_debug.h? Just so we aren't locked into supporting this debug function forever as part of the api.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> could this be in a side header (that we don't give an API guarantee about),
> maybe mir_client_library_debug.h? Just so we aren't locked into supporting
> this debug function forever as part of the api.

+1

[Alan Griffiths (alan-griffiths)]

I agree about separating the debug APIs, but this is an existing issue. Vis:

/**
 * Return the ID of a surface (only useful for debug output).
 * \param [in] surface The surface
 * \return An internal ID that identifies the surface
 */
int mir_surface_get_id(MirSurface *surface);

(I'm sure that used to be called mir_debug_surface_id())

review: Approve

[Alexandros Frantzis (afrantzis)]

> I agree about separating the debug APIs, but this is an existing issue

Perhaps this is a good opportunity to fix it then?

[Daniel van Vugt (vanvugt)]

"current_buffer_id" is confusing. It could mean either the back buffer or the currently visible front buffer.

Please clarify the docs, if not the function name.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:883
http://jenkins.qa.ubuntu.com/job/mir-ci/1220/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1605
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1490
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/458/console

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Looks OK, but the CI failure is weird - setting off a rebuild.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:883
http://jenkins.qa.ubuntu.com/job/mir-ci/1221/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1609
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1494
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/459
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/459/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Kevin DuBois (kdub)]

looks good

review: Approve

[Robert Ancell (robert-ancell)]

This breaks ABI - please leave mir_surface_get_id, mark it as deprecated and call mir_debug_surface_get_id.

Also, I would have though mir_surface_get_debug_id would be a more appropriate name (since we're not getting the id from a "DebugSurface".

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> This breaks ABI - please leave mir_surface_get_id, mark it as deprecated and call mir_debug_surface_get_id.

What's the plan for deprecating APIs at this stage (I don't think we are, or should, really be providing API stability yet)? Should we keep the old functions for a while and then remove them?

[Robert Ancell (robert-ancell)]

For libmirclient we are providing stability, so a Mir update doesn't break mesa/XMir etc. Just mark functions as deprecated and we'll remove them in one big change (libmirclient1 -> libmirclient2). If you really want to get rid of a function now, then you have to bump ABI but this will probably be more work overall than just leaving the old function there.

[Alan Griffiths (alan-griffiths)]

> For libmirclient we are providing stability, so a Mir update doesn't break
> mesa/XMir etc. Just mark functions as deprecated and we'll remove them in one
> big change (libmirclient1 -> libmirclient2). If you really want to get rid of
> a function now, then you have to bump ABI but this will probably be more work
> overall than just leaving the old function there.

Does it really call this functionally useless function?

[Kevin DuBois (kdub)]

> For libmirclient we are providing stability, so a Mir update doesn't break
> mesa/XMir etc. Just mark functions as deprecated and we'll remove them in one
> big change (libmirclient1 -> libmirclient2). If you really want to get rid of
> a function now, then you have to bump ABI but this will probably be more work
> overall than just leaving the old function there.

I hope the same goes for protobuf too? (that we'll just clean up all dead code at once to minimize disruption). I'd feel better if this cleanup/old-api-breakage was scheduled at regular intervals or something (every three months perhaps), just so we keep bloat at a 3-month maximum.

[Robert Ancell (robert-ancell)]

> > For libmirclient we are providing stability, so a Mir update doesn't break
> > mesa/XMir etc. Just mark functions as deprecated and we'll remove them in
> one
> > big change (libmirclient1 -> libmirclient2). If you really want to get rid
> of
> > a function now, then you have to bump ABI but this will probably be more
> work
> > overall than just leaving the old function there.
>
> I hope the same goes for protobuf too? (that we'll just clean up all dead code
> at once to minimize disruption). I'd feel better if this cleanup/old-api-
> breakage was scheduled at regular intervals or something (every three months
> perhaps), just so we keep bloat at a 3-month maximum.

protobuf is harder - because we can't stop any old client connecting we have to be able to handle that case. In practise, one we have bumped the ABI on libmirclient we can drop old protobuf changes (since we know all the clients currently). But we can only do this in this phase of the project - once we are widely distributed we can't do this any longer. Also make sure to make a note in the .proto of old field numbers that have been used to avoid weird binary mismatches.

[Robert Ancell (robert-ancell)]

> > For libmirclient we are providing stability, so a Mir update doesn't break
> > mesa/XMir etc. Just mark functions as deprecated and we'll remove them in
> one
> > big change (libmirclient1 -> libmirclient2). If you really want to get rid
> of
> > a function now, then you have to bump ABI but this will probably be more
> work
> > overall than just leaving the old function there.
>
> Does it really call this functionally useless function?

It seems unlikely that any client uses this function, however if we're going to be API/ABI stable then we need to be consistent.

[Daniel van Vugt (vanvugt)]

I think we need to improve the docs still. See my comment from 2013-07-31.

review: Needs Fixing

[Chris Halse Rogers (raof)]

I've kept the name mir_debug_* as I think the "debugness" is the most important thing about these functions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:884
http://jenkins.qa.ubuntu.com/job/mir-ci/1270/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1682
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1567
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/508
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/508/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

Happy now :)

review: Approve

[Daniel van Vugt (vanvugt)]

Wow, seems I'm the blocker here. Just a few issues:

1. DEPRECATED should use proper Doxygen: \deprecated

2. Lines too long; need wrapping before col 80:
include/client/mir_toolkit/mir_client_library_debug.h

3. Should be const:
86 + uint32_t current_buffer_id();

Overall I think the line between "debug" and regular client API is still too blurry. But I can't think of a way to make it clearer right now. So just worry about the above nitpicks.

review: Needs Fixing

[Robert Ancell (robert-ancell)]

> 2. Lines too long; need wrapping before col 80:
> include/client/mir_toolkit/mir_client_library_debug.h

There is no requirement to wrap at column 80, and plenty of existing code does not do this.

[Daniel van Vugt (vanvugt)]

Our client headers are /mostly/ wrapped before col 80 right now, so it would be nice to not go backwards.

[Alan Griffiths (alan-griffiths)]

On 12/08/13 04:22, Daniel van Vugt wrote:
> Our client headers are /mostly/ wrapped before col 80 right now, so it would be nice to not go backwards.
>

I don't care much about line length, but we do have a project guideline:

http://unity.ubuntu.com/mir/cppguide/index.html?showone=Line_Length#Line_Length

--
Alan Griffiths [phone]
Octopull Ltd http://www.octopull.co.uk/

[Daniel van Vugt (vanvugt)]

There are very solid reasons for limiting yourself to 80 characters, founded in the limitations of the human brain and readability. See also; print media. But I will drop the issue now...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:887
http://jenkins.qa.ubuntu.com/job/mir-ci/1293/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1723
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1608
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/531
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/531/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

review: Approve

[Daniel van Vugt (vanvugt)]

I had already decided to not block on my previous comments, but thanks for getting to them before I got around to hitting Approve.

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-08-08 02:34:31 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-08-13 10:28:51 +0000
@@ -95,8 +95,9 @@
 void mir_connection_get_platform(MirConnection *connection, MirPlatformPackage *platform_package);
 
 /** 
- * DEPRECATED. use mir_connection_create_display_config
+ * \deprecated Use mir_connection_create_display_config
  */
+__attribute__((__deprecated__("Use mir_connection_create_display_config()")))
 void mir_connection_get_display_info(MirConnection *connection, MirDisplayInfo *display_info);
 
 /**
@@ -325,10 +326,9 @@
 void mir_wait_for_one(MirWaitHandle *wait_handle);
 
 /**
- * Return the ID of a surface (only useful for debug output).
- *   \param [in] surface  The surface
- *   \return              An internal ID that identifies the surface
+ * \deprecated Use mir_debug_surface_id()
  */
+__attribute__((__deprecated__("Use mir_debug_surface_id()")))
 int mir_surface_get_id(MirSurface *surface);
 
 /**

=== added file 'include/client/mir_toolkit/mir_client_library_debug.h'
--- include/client/mir_toolkit/mir_client_library_debug.h	1970-01-01 00:00:00 +0000
+++ include/client/mir_toolkit/mir_client_library_debug.h	2013-08-13 10:28:51 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MIR_CLIENT_LIBRARY_DEBUG_H
+#define MIR_CLIENT_LIBRARY_DEBUG_H
+
+#include <mir_toolkit/mir_client_library.h>
+
+/* This header defines debug interfaces that aren't expected to be generally useful
+ * and do not have the same API-stability guarantees that the main API has */
+
+/**
+ * Return the ID of a surface (only useful for debug output).
+ *   \param [in] surface  The surface
+ *   \return              An internal ID that identifies the surface
+ */
+int mir_debug_surface_id(MirSurface *surface);
+
+/**
+ * Get the ID of the surface's current buffer (only useful for debug purposes)
+ *   \pre                         The surface is valid
+ *   \param   [in] surface        The surface
+ *   \return                      The internal buffer ID of the surface's current buffer.
+ *                                This is the buffer that is currently being drawn to,
+ *                                and would be returned by mir_surface_get_current_buffer.
+ */
+uint32_t mir_debug_surface_current_buffer_id(MirSurface *surface); 
+
+#endif /* MIR_CLIENT_LIBRARY_DEBUG_H */

=== modified file 'src/client/client_buffer_depository.cpp'
--- src/client/client_buffer_depository.cpp	2013-04-24 05:22:20 +0000
+++ src/client/client_buffer_depository.cpp	2013-08-13 10:28:51 +0000
@@ -65,3 +65,8 @@
 {
     return buffers.front().second;
 }
+
+uint32_t mcl::ClientBufferDepository::current_buffer_id() const
+{
+    return buffers.front().first;
+}

=== modified file 'src/client/client_buffer_depository.h'
--- src/client/client_buffer_depository.h	2013-05-02 00:11:18 +0000
+++ src/client/client_buffer_depository.h	2013-08-13 10:28:51 +0000
@@ -61,6 +61,7 @@
     void deposit_package(std::shared_ptr<MirBufferPackage> const&, int id,
                          geometry::Size, geometry::PixelFormat);
     std::shared_ptr<ClientBuffer> current_buffer();
+    uint32_t current_buffer_id() const;
 
 private:
     std::shared_ptr<ClientBufferFactory> const factory;

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-08-08 02:34:31 +0000
+++ src/client/mir_client_library.cpp	2013-08-13 10:28:51 +0000
@@ -19,6 +19,7 @@
 #include "mir/default_configuration.h"
 #include "mir_toolkit/mir_client_library.h"
 #include "mir_toolkit/mir_client_library_drm.h"
+#include "mir_toolkit/mir_client_library_debug.h"
 
 #include "mir_connection.h"
 #include "display_configuration.h"
@@ -203,7 +204,12 @@
 
 int mir_surface_get_id(MirSurface * surface)
 {
-    return surface->id();
+    return mir_debug_surface_id(surface);
+}
+
+int mir_debug_surface_id(MirSurface * surface)
+{
+    return surface->id();    
 }
 
 int mir_surface_is_valid(MirSurface* surface)
@@ -232,6 +238,11 @@
     *buffer_package_out = package.get();
 }
 
+uint32_t mir_debug_surface_current_buffer_id(MirSurface * surface)
+{
+    return surface->get_current_buffer_id();
+}
+
 void mir_connection_get_platform(MirConnection *connection, MirPlatformPackage *platform_package)
 {
     connection->populate(*platform_package);

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-06-28 09:25:09 +0000
+++ src/client/mir_surface.cpp	2013-08-13 10:28:51 +0000
@@ -239,6 +239,13 @@
     return buffer_depository->current_buffer();
 }
 
+uint32_t MirSurface::get_current_buffer_id() const
+{
+    std::lock_guard<std::recursive_mutex> lock(mutex);
+
+    return buffer_depository->current_buffer_id();    
+}
+
 void MirSurface::populate(MirBufferPackage& buffer_package)
 {
     std::lock_guard<std::recursive_mutex> lock(mutex);

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-06-28 11:10:25 +0000
+++ src/client/mir_surface.h	2013-08-13 10:28:51 +0000
@@ -82,6 +82,7 @@
     std::shared_ptr<MirNativeBuffer> get_current_buffer_package();
     MirPlatformType platform_type();
     std::shared_ptr<mir::client::ClientBuffer> get_current_buffer();
+    uint32_t get_current_buffer_id() const;
     void get_cpu_region(MirGraphicsRegion& region);
     void release_cpu_region();
     EGLNativeWindowType generate_native_window();

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-08-06 01:27:08 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-08-13 10:28:51 +0000
@@ -19,6 +19,7 @@
 #include "mir_test_framework/display_server_test_fixture.h"
 
 #include "mir_toolkit/mir_client_library.h"
+#include "mir_toolkit/mir_client_library_debug.h"
 #include "src/client/client_buffer.h"
 
 #include "mir/frontend/communicator.h"
@@ -389,7 +390,7 @@
 
             mir_surface_set_event_handler(surface, &delegate);
 
-            int surface_id = mir_surface_get_id(surface);
+            int surface_id = mir_debug_surface_id(surface);
 
             mir_wait_for(mir_surface_set_state(surface,
                                                mir_surface_state_fullscreen));

=== modified file 'tests/acceptance-tests/test_surfaceloop.cpp'
--- tests/acceptance-tests/test_surfaceloop.cpp	2013-07-11 16:54:41 +0000
+++ tests/acceptance-tests/test_surfaceloop.cpp	2013-08-13 10:28:51 +0000
@@ -20,6 +20,7 @@
 #include "mir/geometry/size.h"
 
 #include "mir_toolkit/mir_client_library.h"
+#include "mir_toolkit/mir_client_library_debug.h"
 #include "src/client/mir_connection.h"
 
 #include "mir_protobuf.pb.h"
@@ -234,8 +235,8 @@
             wait_for_surface_create(ssync+1);
 
             EXPECT_NE(
-                mir_surface_get_id(ssync[0].surface),
-                mir_surface_get_id(ssync[1].surface));
+                mir_debug_surface_id(ssync[0].surface),
+                mir_debug_surface_id(ssync[1].surface));
 
             mir_surface_release(ssync[1].surface, release_surface_callback, ssync+1);
             wait_for_surface_release(ssync+1);
@@ -280,12 +281,12 @@
                 {
                     if (i == j)
                         EXPECT_EQ(
-                            mir_surface_get_id(ssync[i].surface),
-                            mir_surface_get_id(ssync[j].surface));
+                            mir_debug_surface_id(ssync[i].surface),
+                            mir_debug_surface_id(ssync[j].surface));
                     else
                         EXPECT_NE(
-                            mir_surface_get_id(ssync[i].surface),
-                            mir_surface_get_id(ssync[j].surface));
+                            mir_debug_surface_id(ssync[i].surface),
+                            mir_debug_surface_id(ssync[j].surface));
                 }
             }