Merge lp:~vanvugt/mir/fix-1546933-2 into lp:mir

[Daniel van Vugt]

Fix incomplete client performance report (LP: #1546933)

Although the regression test is more interesting than the fix.

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3379
https://mir-jenkins.ubuntu.com/job/mir-ci/478/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/305/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/330
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/322
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/322
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/314/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/314
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/314/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/314
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/314/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/314
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/314/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/314/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/478/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Wouldn't it be easier to call set_logger(fake_shared(string_stream_logger)) and avoid needing StubConnectionConfiguration and the "Awkwardly static" data member?

review: Needs Information

[Daniel van Vugt (vanvugt)]

Good observation, but that did not work when I was previously sharing the ClientSurfaces test case. So I had to do it this complicated way (which took an extra afternoon).

Since moving the test to a new source file however, it might now be possible...

[Daniel van Vugt (vanvugt)]

I'm hoping this bug fix lands in one form or another soon. We've been arguing over test case syntax for a fortnight now, while the trivial fix itself has remained unchanged.

[Daniel van Vugt (vanvugt)]

Alan:

I have prototyped the set_logger approach but it does not work, for shared process reasons... If you think you can fix it, be my guest:
  https://code.launchpad.net/~mir-team/mir/fix-1546933-3/+merge/288056

The only working solutions at present are this branch and the old one:
  https://code.launchpad.net/~vanvugt/mir/fix-1546933/+merge/286782

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3381
https://mir-jenkins.ubuntu.com/job/mir-ci/489/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/320/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/346
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/338
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/338
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/329/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/329
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/329/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/329
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/329/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/329
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/329/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/329/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/489/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Oh dear. A regression of bug 1540731?

[ FAILED ] PromptSessionClientAPI.client_pid_is_associated_with_session

Not related to this branch.

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3382
https://mir-jenkins.ubuntu.com/job/mir-ci/491/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/325/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/351
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/343
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/343
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/334/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/334/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/334
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/334/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/334
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/334/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/334/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/491/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Alan:
>
> I have prototyped the set_logger approach but it does not work, for shared
> process reasons... If you think you can fix it, be my guest:
> https://code.launchpad.net/~mir-team/mir/fix-1546933-3/+merge/288056

I've investigated this a bit and the reason is not "shared process" but that
mcl::DefaultConnectionConfiguration::the_logger() is not "joined up" with ml::set_logger() and simply allocates its own logger.

So, what I've been thinking will work, doesn't. (I think that is wrong, but pre-existing.)

review: Approve

[Alan Griffiths (alan-griffiths)]

> So, what I've been thinking will work, doesn't. (I think that is wrong, but
> pre-existing.)

...so I've created lp:1553219

[Mir CI Bot (mir-ci-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/52/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/354/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/57/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/384
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/376
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/376
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/363
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/363/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/363/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/363
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/363/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/363
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/363/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/363
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/363/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

LGTM

review: Approve

[Mir CI Bot (mir-ci-bot)]

review: Approve (continuous-integration)

=== renamed file 'tests/include/mir_test_framework/stub_client_connection_configuration.h' => 'include/test/mir_test_framework/stub_client_connection_configuration.h'
=== renamed file 'tests/include/mir_test_framework/using_client_platform.h' => 'include/test/mir_test_framework/using_client_platform.h'
=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2016-03-02 16:35:02 +0000
+++ src/client/mir_connection.cpp	2016-03-04 06:58:16 +0000
@@ -352,9 +352,12 @@
 
     try
     {
+        std::string name{spec.surface_name.is_set() ?
+                         spec.surface_name.value() : ""};
+
         stream = std::make_shared<mcl::BufferStream>(
             this, request->wh, server, platform,
-            surface_proto->buffer_stream(), make_perf_report(logger), std::string{},
+            surface_proto->buffer_stream(), make_perf_report(logger), name,
             mir::geometry::Size{surface_proto->width(), surface_proto->height()}, nbuffers);
     }
     catch (std::exception const& error)

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2016-02-26 14:45:41 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2016-03-04 06:58:16 +0000
@@ -20,6 +20,7 @@
   test_client_surface_swap_buffers.cpp
   test_command_line_handling.cpp
   test_client_surfaces.cpp
+  test_client_logging.cpp
   test_custom_window_management.cpp
   test_custom_input_dispatcher.cpp
   test_server_shutdown.cpp

=== added file 'tests/acceptance-tests/test_client_logging.cpp'
--- tests/acceptance-tests/test_client_logging.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_client_logging.cpp	2016-03-04 06:58:16 +0000
@@ -0,0 +1,118 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+#include "mir_test_framework/stub_client_connection_configuration.h"
+#include "mir_test_framework/using_client_platform.h"
+#include "mir_test_framework/connected_client_headless_server.h"
+#include "mir/logging/logger.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <string>
+#include <sstream>
+
+using namespace testing;
+using namespace mir_test_framework;
+
+namespace {
+
+class StringStreamLogger : public mir::logging::Logger
+{
+public:
+    void log(mir::logging::Severity,
+             std::string const& message,
+             std::string const& component) override
+    {
+        ss << "[StringStreamLogger] "
+           << component << ": " << message << std::endl;
+    }
+    // Awkwardly static. Because the instance is hidden in UsingClientPlatform
+    static std::stringstream ss;
+};
+
+std::stringstream StringStreamLogger::ss;
+
+struct Conf : StubConnectionConfiguration
+{
+    Conf(std::string const& socket)
+        : StubConnectionConfiguration(socket)
+    {
+    }
+    std::shared_ptr<mir::logging::Logger> the_logger() override
+    {
+        return std::make_shared<StringStreamLogger>();
+    }
+};
+
+struct ClientLogging : ConnectedClientHeadlessServer
+{
+    UsingClientPlatform<Conf> with_custom_logger;
+    std::stringstream& client_log{StringStreamLogger::ss};
+};
+
+} // namespace
+
+TEST_F(ClientLogging, reports_performance)
+{
+    TemporaryEnvironmentValue env("MIR_CLIENT_PERF_REPORT", "log");
+    (void)env; // Avoid clang warning/error
+
+    auto spec = mir_connection_create_spec_for_normal_surface(
+                   connection, 123, 456, mir_pixel_format_abgr_8888);
+    ASSERT_THAT(spec, NotNull());
+    mir_surface_spec_set_name(spec, "Rumpelstiltskin");
+    mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_software);
+    auto surf = mir_surface_create_sync(spec);
+    ASSERT_THAT(surf, NotNull());
+    mir_surface_spec_release(spec);
+
+    int const target_fps = 10;
+    int const nseconds = 3;
+    auto bs = mir_surface_get_buffer_stream(surf);
+    for (int s = 0; s < nseconds; ++s)
+    {
+        for (int f = 0; f < target_fps; ++f)
+            mir_buffer_stream_swap_buffers_sync(bs);
+
+        std::this_thread::sleep_for(std::chrono::seconds(1));
+    }
+
+    int reports = 0;
+    while (!client_log.eof())
+    {
+        std::string line;
+        std::getline(client_log, line);
+        auto perf = line.find(" perf: ");
+        if (perf != line.npos)
+        {
+            ++reports;
+            char name[256];
+            float fps;
+            int fields = sscanf(line.c_str() + perf,
+                                " perf: %255[^:]: %f FPS,", name, &fps);
+            ASSERT_EQ(2, fields) << "Log line = {" << line << "}";
+            EXPECT_STREQ("Rumpelstiltskin", name);
+            EXPECT_NEAR(target_fps, fps, 5.0f);
+        }
+    }
+
+    EXPECT_THAT(reports, Ge(nseconds-1));
+
+    mir_surface_release_sync(surf);
+}