Mir

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

Proposed by Daniel van Vugt
Status: Work in progress
Proposed branch: lp:~vanvugt/mir/set-subpixel
Merge into: lp:mir
Diff against target: 195 lines (+115/-1)
6 files modified
include/client/mir_toolkit/mir_display_configuration.h (+12/-0)
src/client/display_configuration_api.cpp (+6/-0)
src/client/symbols.map (+1/-0)
src/server/frontend/session_mediator.cpp (+2/-1)
src/utils/out.c (+38/-0)
tests/acceptance-tests/test_new_display_configuration.cpp (+56/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/set-subpixel
Reviewer Review Type Date Requested Status
Chris Halse Rogers Disapprove
Mir CI Bot continuous-integration Approve
Review via email: mp+317590@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Chris Halse Rogers (raof) wrote :

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
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

Revision history for this message
Chris Halse Rogers (raof) wrote :

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.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

"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 updated
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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/client/mir_toolkit/mir_display_configuration.h'
2--- include/client/mir_toolkit/mir_display_configuration.h 2017-02-17 08:46:05 +0000
3+++ include/client/mir_toolkit/mir_display_configuration.h 2017-04-12 09:45:14 +0000
4@@ -501,6 +501,18 @@
5 MirSubpixelArrangement mir_output_get_subpixel_arrangement(MirOutput const* output);
6
7 /**
8+ * Set the subpixel arrangement of a display.
9+ *
10+ * Often it's not possible to automatically detect the subpixel arrangement
11+ * of a display so one may wish to set it explicitly.
12+ *
13+ * \param [in] output The MirOutput to modify
14+ * \param [in] sub The subpixel arrangement to set
15+ */
16+void mir_output_set_subpixel_arrangement(MirOutput* output,
17+ MirSubpixelArrangement sub);
18+
19+/**
20 * Get the form-factor of a connected output.
21 *
22 * This call succeeds even if the output is not connected, but may return nonsense values.
23
24=== modified file 'src/client/display_configuration_api.cpp'
25--- src/client/display_configuration_api.cpp 2017-02-17 08:46:05 +0000
26+++ src/client/display_configuration_api.cpp 2017-04-12 09:45:14 +0000
27@@ -310,6 +310,12 @@
28 return static_cast<MirSubpixelArrangement>(output->subpixel_arrangement());
29 }
30
31+void mir_output_set_subpixel_arrangement(MirOutput* output,
32+ MirSubpixelArrangement sub)
33+{
34+ output->set_subpixel_arrangement(sub);
35+}
36+
37 uint32_t mir_output_get_gamma_size(MirOutput const* output)
38 {
39 return (output->gamma_red().size() / (sizeof(uint16_t) / sizeof(char)));
40
41=== modified file 'src/client/symbols.map'
42--- src/client/symbols.map 2017-03-29 11:53:31 +0000
43+++ src/client/symbols.map 2017-04-12 09:45:14 +0000
44@@ -581,6 +581,7 @@
45
46 MIR_CLIENT_0.27 { # New functions in Mir 0.27 or 1.0
47 global:
48+ mir_output_set_subpixel_arrangement;
49 mir_connection_apply_session_input_config;
50 mir_connection_set_base_input_config;
51 mir_create_freestyle_window_spec;
52
53=== modified file 'src/server/frontend/session_mediator.cpp'
54--- src/server/frontend/session_mediator.cpp 2017-04-07 00:48:39 +0000
55+++ src/server/frontend/session_mediator.cpp 2017-04-12 09:45:14 +0000
56@@ -1269,7 +1269,8 @@
57 dest.power_mode = static_cast<MirPowerMode>(src.power_mode());
58 dest.orientation = static_cast<MirOrientation>(src.orientation());
59 dest.scale = src.scale_factor();
60-
61+ dest.subpixel_arrangement =
62+ static_cast<MirSubpixelArrangement>(src.subpixel_arrangement());
63 dest.gamma = {convert_string_to_gamma_curve(src.gamma_red()),
64 convert_string_to_gamma_curve(src.gamma_green()),
65 convert_string_to_gamma_curve(src.gamma_blue())};
66
67=== modified file 'src/utils/out.c'
68--- src/utils/out.c 2017-03-28 03:49:44 +0000
69+++ src/utils/out.c 2017-04-12 09:45:14 +0000
70@@ -70,6 +70,7 @@
71 "HBGR",
72 "VRGB",
73 "VBGR",
74+ "none",
75 };
76 return ((unsigned)s < sizeof(name)/sizeof(name[0])) ? name[s]
77 : "out-of-range";
78@@ -288,6 +289,42 @@
79 mir_output_set_orientation(target[t], orientation[i]);
80 }
81 }
82+ else if (!strcmp(*action, "subpixel"))
83+ {
84+ if (++action >= action_end)
85+ {
86+ fprintf(stderr, "Missing parameter after `%s'\n", action[-1]);
87+ return false;
88+ }
89+ char const* arg = *action;
90+ enum {arrangements = 6};
91+ static const MirSubpixelArrangement arrangement[arrangements] =
92+ {
93+ mir_subpixel_arrangement_unknown,
94+ mir_subpixel_arrangement_horizontal_rgb,
95+ mir_subpixel_arrangement_horizontal_bgr,
96+ mir_subpixel_arrangement_vertical_rgb,
97+ mir_subpixel_arrangement_vertical_bgr,
98+ mir_subpixel_arrangement_none
99+ };
100+
101+ int i;
102+ for (i = 0; i < arrangements; ++i)
103+ if (!strcmp(arg, subpixel_name(arrangement[i])))
104+ break;
105+
106+ if (i >= arrangements)
107+ {
108+ fprintf(stderr, "Unknown arrangement `%s'\n", arg);
109+ return false;
110+ }
111+ else
112+ {
113+ for (int t = 0; t < targets; ++t)
114+ mir_output_set_subpixel_arrangement(target[t],
115+ arrangement[i]);
116+ }
117+ }
118 else if (!strcmp(*action, "place"))
119 {
120 int x, y;
121@@ -469,6 +506,7 @@
122 " place +X+Y\n"
123 " mode (WIDTHxHEIGHT | preferred) [rate HZ]\n"
124 " rate HZ\n"
125+ " subpixel (unknown | HRGB | HBGR | VRGB | VBGR | none)\n"
126 , argv[0]);
127 return 0;
128 }
129
130=== modified file 'tests/acceptance-tests/test_new_display_configuration.cpp'
131--- tests/acceptance-tests/test_new_display_configuration.cpp 2017-04-11 14:13:20 +0000
132+++ tests/acceptance-tests/test_new_display_configuration.cpp 2017-04-12 09:45:14 +0000
133@@ -977,6 +977,62 @@
134 client.disconnect();
135 }
136
137+TEST_F(DisplayConfigurationTest, client_can_set_subpixel_arrangement)
138+{
139+ DisplayClient client{new_connection()};
140+
141+ client.connect();
142+
143+ auto client_config = client.get_base_config();
144+ int const test_arrangements = 2;
145+ MirSubpixelArrangement const test_arrangement[test_arrangements] =
146+ {
147+ mir_subpixel_arrangement_horizontal_rgb,
148+ mir_subpixel_arrangement_vertical_bgr
149+ };
150+
151+ int const n = mir_display_config_get_num_outputs(client_config.get());
152+ for (int i = 0; i < n; ++i)
153+ {
154+ auto output =
155+ mir_display_config_get_mutable_output(client_config.get(), i);
156+
157+ mir_output_set_subpixel_arrangement(output,
158+ test_arrangement[i % test_arrangements]);
159+ }
160+
161+ DisplayConfigMatchingContext context;
162+ context.matcher = [c = client_config.get()](MirDisplayConfig* conf)
163+ {
164+ EXPECT_THAT(conf, mt::DisplayConfigMatches(c));
165+ };
166+
167+ mir_connection_set_display_config_change_callback(
168+ client.connection,
169+ &new_display_config_matches,
170+ &context);
171+
172+ mir_connection_preview_base_display_configuration(client.connection,
173+ client_config.get(), 10);
174+ EXPECT_TRUE(context.done.wait_for(std::chrono::seconds(10)));
175+ mir_connection_confirm_base_display_configuration(client.connection,
176+ client_config.get());
177+
178+ std::shared_ptr<mg::DisplayConfiguration> current_config =
179+ server.the_display()->configuration();
180+
181+ current_config->for_each_output(
182+ [test_arrangement, test_arrangements, i = 0]
183+ (mg::UserDisplayConfigurationOutput& output) mutable
184+ {
185+ EXPECT_EQ(test_arrangement[i % test_arrangements],
186+ output.subpixel_arrangement);
187+ ++i;
188+ });
189+
190+ client.disconnect();
191+}
192+
193 TEST_F(DisplayConfigurationTest, client_sees_server_set_gamma)
194 {
195 uint32_t const size = 4;

Subscribers

People subscribed via source and target branches