Merge lp:~vanvugt/mir/set-subpixel into lp:mir

[Daniel van Vugt]

Add support for setting the subpixel arrangement.

Because many people like myself will find that the system doesn't
know how to automatically detect it.

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

PASSED: Continuous integration, rev:4047
https://mir-jenkins.ubuntu.com/job/mir-ci/3011/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4021
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4098
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4098
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4098
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4048/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4048/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4048/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/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4048/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/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4048/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4048
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4048/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

I'm a mild -1 on this. This is not a configuration property, and should not be a part of the configuration interface.

The shell has all the pieces it needs to implement overrides; that's an appropriate place for this functionality. I'd also fully support this being implemented in a Mir server configuration file - there is a broad need for some quirking infrastructure, and Mir could usefully provide that.

It's inappropriate for client configuration.

review: Disapprove

[Daniel van Vugt (vanvugt)]

The common case this branch aims to deal with is when the detected order is "unknown" and the user must choose one (and usually HRGB will be a good guess).

We will need this branch only if the shell chooses to store the display config as a blob, so this setter would be required. If the shell goes to the trouble of translating the display config to its own storage format (with potential overrides) then yes this branch would not be required.

But there's another lesser use case of this branch, which is for shells (demo servers) that don't remember their configurations. For our demo servers, having a setter like this is the only way to avoid forcing the override and custom configuration logic into the toolkits (which is then an O(N) problem as opposed to an O(1) problem with this branch).

So to me this branch is useful right now. I understand it may prove to not be useful to Unity8, but it also doesn't hurt anyone to have this flexibility.

[Daniel van Vugt (vanvugt)]

And I just realised Chris is wrong. We do need this branch more than I thought...

If configuration tools don't have the ability to ever set the correct subpixel order, then clients who query the subpixel order from the display config of their current output will never be able to get the right value. And text in apps will have incorrect appearance.

Chris is assuming all monitor configuration tooling will be built into the shells, in all cases. That needlessly blocks the ability to have third-party configuration tools like mirout, and blocks our ability to do correct subpixel rendering in USC on the login screen for example. We really do need this setter.

[Daniel van Vugt (vanvugt)]

BTW, "Mir server configuration files" are a bad idea, as I've highlighted before. It's a bad idea because that commits all Mir servers/shells to accessing some fixed format/path for one thing which is a different format/path to all of its other configuration settings.

Mir can certainly provide some serialization helper functions, but should never imply a complete file format or location.

[Chris Halse Rogers (raof)]

Yeah, “Mir server configuration file” is unclear. I meant “file that Mir loads for quirks”, something that we already do on Android. That does not make sense to be shell-specific, because it's hardware-specific rather than user configuration.

I continue to think that the functionality this branch introduces should not be implemented.

Systems are simpler when there are fewer things that can change, and fewer places where those things are changeable. Sometimes added complexity is required, but in this case toolkits already have settings to override the hardware reported values (and, indeed, I'm not sure if GTK uses the hardware reported value *at all*).

This is unnecessary complexity, and should be avoided.

[Daniel van Vugt (vanvugt)]

"toolkits already have settings to override the hardware reported values"

Therein lays the flaw in your suggestion. Even if every toolkit supported overrides, the user would need to learn each toolkit's override hack and apply them all. That's not very user friendly.

More user friendly is for Mir to store the override the user chose in some graphical configuration dialog, and export that to all Mir clients.

Certainly that already works if you assume all monitor configuration tooling run insides the shell process, but that's not a great assumption, and is already proven false by sufficient prior art. If the monitor configuration tool is separate to the shell process (ie. is a Mir client rather than a shell-specific client), and you want a single GUI to configure your subpixel override that all toolkits will honour, then we need this branch.

[lp:~vanvugt/mir/set-subpixel]

4048. By Daniel van Vugt on 2017-02-17

Merge latest trunk

4049. By Daniel van Vugt on 2017-02-28

Merge latest trunk

4050. By Daniel van Vugt on 2017-04-12

Merge latest trunk

Unmerged revisions

4050. By Daniel van Vugt on 2017-04-12

Merge latest trunk

4049. By Daniel van Vugt on 2017-02-28

Merge latest trunk

4048. By Daniel van Vugt on 2017-02-17

Merge latest trunk

4047. By Daniel van Vugt on 2017-02-17

Uncomment the fix. Test now passes.

4046. By Daniel van Vugt on 2017-02-17

Add a regression test (failing)

4045. By Daniel van Vugt on 2017-02-17

Correct mirout syntax help

4044. By Daniel van Vugt on 2017-02-17

Implement a fix, but commented out for now.

4043. By Daniel van Vugt on 2017-02-17

Add a subcommand to mirout

4042. By Daniel van Vugt on 2017-02-17

Prototype

=== modified file 'include/client/mir_toolkit/mir_display_configuration.h'
--- include/client/mir_toolkit/mir_display_configuration.h	2017-02-17 08:46:05 +0000
+++ include/client/mir_toolkit/mir_display_configuration.h	2017-04-12 09:45:14 +0000
@@ -501,6 +501,18 @@
 MirSubpixelArrangement mir_output_get_subpixel_arrangement(MirOutput const* output);
 
 /**
+ * Set the subpixel arrangement of a display.
+ *
+ * Often it's not possible to automatically detect the subpixel arrangement
+ * of a display so one may wish to set it explicitly.
+ *
+ * \param [in]  output  The MirOutput to modify
+ * \param [in]  sub     The subpixel arrangement to set
+ */
+void mir_output_set_subpixel_arrangement(MirOutput* output,
+                                         MirSubpixelArrangement sub);
+
+/**
  * Get the form-factor of a connected output.
  *
  * This call succeeds even if the output is not connected, but may return nonsense values.
 
=== modified file 'src/client/display_configuration_api.cpp'
--- src/client/display_configuration_api.cpp	2017-02-17 08:46:05 +0000
+++ src/client/display_configuration_api.cpp	2017-04-12 09:45:14 +0000
@@ -310,6 +310,12 @@
     return static_cast<MirSubpixelArrangement>(output->subpixel_arrangement());
 }
 
+void mir_output_set_subpixel_arrangement(MirOutput* output,
+                                         MirSubpixelArrangement sub)
+{
+    output->set_subpixel_arrangement(sub);
+}
+
 uint32_t mir_output_get_gamma_size(MirOutput const* output)
 {
     return (output->gamma_red().size() / (sizeof(uint16_t) / sizeof(char)));
 
=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2017-03-29 11:53:31 +0000
+++ src/client/symbols.map	2017-04-12 09:45:14 +0000
@@ -581,6 +581,7 @@
 
 MIR_CLIENT_0.27 {  # New functions in Mir 0.27 or 1.0
   global:
+    mir_output_set_subpixel_arrangement;
     mir_connection_apply_session_input_config;
     mir_connection_set_base_input_config;
     mir_create_freestyle_window_spec;
 
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2017-04-07 00:48:39 +0000
+++ src/server/frontend/session_mediator.cpp	2017-04-12 09:45:14 +0000
@@ -1269,7 +1269,8 @@
         dest.power_mode = static_cast<MirPowerMode>(src.power_mode());
         dest.orientation = static_cast<MirOrientation>(src.orientation());
         dest.scale = src.scale_factor();
-
+        dest.subpixel_arrangement =
+            static_cast<MirSubpixelArrangement>(src.subpixel_arrangement());
         dest.gamma = {convert_string_to_gamma_curve(src.gamma_red()),
                       convert_string_to_gamma_curve(src.gamma_green()),
                       convert_string_to_gamma_curve(src.gamma_blue())};
 
=== modified file 'src/utils/out.c'
--- src/utils/out.c	2017-03-28 03:49:44 +0000
+++ src/utils/out.c	2017-04-12 09:45:14 +0000
@@ -70,6 +70,7 @@
         "HBGR",
         "VRGB",
         "VBGR",
+        "none",
     };
     return ((unsigned)s < sizeof(name)/sizeof(name[0])) ? name[s]
                                                         : "out-of-range";
@@ -288,6 +289,42 @@
                     mir_output_set_orientation(target[t], orientation[i]);
             }
         }
+        else if (!strcmp(*action, "subpixel"))
+        {
+            if (++action >= action_end)
+            {
+                fprintf(stderr, "Missing parameter after `%s'\n", action[-1]);
+                return false;
+            }
+            char const* arg = *action;
+            enum {arrangements = 6};
+            static const MirSubpixelArrangement arrangement[arrangements] =
+            {
+                mir_subpixel_arrangement_unknown,
+                mir_subpixel_arrangement_horizontal_rgb,
+                mir_subpixel_arrangement_horizontal_bgr,
+                mir_subpixel_arrangement_vertical_rgb,
+                mir_subpixel_arrangement_vertical_bgr,
+                mir_subpixel_arrangement_none
+            };
+
+            int i;
+            for (i = 0; i < arrangements; ++i)
+                if (!strcmp(arg, subpixel_name(arrangement[i])))
+                    break;
+
+            if (i >= arrangements)
+            {
+                fprintf(stderr, "Unknown arrangement `%s'\n", arg);
+                return false;
+            }
+            else
+            {
+                for (int t = 0; t < targets; ++t)
+                    mir_output_set_subpixel_arrangement(target[t],
+                                                        arrangement[i]);
+            }
+        }
         else if (!strcmp(*action, "place"))
         {
             int x, y;
@@ -469,6 +506,7 @@
                            "    place +X+Y\n"
                            "    mode (WIDTHxHEIGHT | preferred) [rate HZ]\n"
                            "    rate HZ\n"
+                           "    subpixel (unknown | HRGB | HBGR | VRGB | VBGR | none)\n"
                            , argv[0]);
                     return 0;
             }
 
=== modified file 'tests/acceptance-tests/test_new_display_configuration.cpp'
--- tests/acceptance-tests/test_new_display_configuration.cpp	2017-04-11 14:13:20 +0000
+++ tests/acceptance-tests/test_new_display_configuration.cpp	2017-04-12 09:45:14 +0000
@@ -977,6 +977,62 @@
     client.disconnect();
 }
 
+TEST_F(DisplayConfigurationTest, client_can_set_subpixel_arrangement)
+{
+    DisplayClient client{new_connection()};
+
+    client.connect();
+
+    auto client_config = client.get_base_config();
+    int const test_arrangements = 2;
+    MirSubpixelArrangement const test_arrangement[test_arrangements] =
+    {
+        mir_subpixel_arrangement_horizontal_rgb,
+        mir_subpixel_arrangement_vertical_bgr
+    };
+
+    int const n = mir_display_config_get_num_outputs(client_config.get());
+    for (int i = 0; i < n; ++i)
+    {
+        auto output =
+            mir_display_config_get_mutable_output(client_config.get(), i);
+
+        mir_output_set_subpixel_arrangement(output,
+            test_arrangement[i % test_arrangements]);
+    }
+
+    DisplayConfigMatchingContext context;
+    context.matcher = [c = client_config.get()](MirDisplayConfig* conf)
+        {
+            EXPECT_THAT(conf, mt::DisplayConfigMatches(c));
+        };
+
+    mir_connection_set_display_config_change_callback(
+        client.connection,
+        &new_display_config_matches,
+        &context);
+
+    mir_connection_preview_base_display_configuration(client.connection,
+        client_config.get(), 10);
+    EXPECT_TRUE(context.done.wait_for(std::chrono::seconds(10)));
+    mir_connection_confirm_base_display_configuration(client.connection,
+        client_config.get());
+
+    std::shared_ptr<mg::DisplayConfiguration> current_config =
+        server.the_display()->configuration();
+
+    current_config->for_each_output(
+        [test_arrangement, test_arrangements, i = 0]
+        (mg::UserDisplayConfigurationOutput& output) mutable
+        {
+            EXPECT_EQ(test_arrangement[i % test_arrangements],
+                      output.subpixel_arrangement);
+            ++i;
+        });
+
+    client.disconnect();
+}
+
 TEST_F(DisplayConfigurationTest, client_sees_server_set_gamma)
 {
     uint32_t const size = 4;