Mir

Merge lp:~vanvugt/mir/GammaCurve into lp:mir

  1. GammaCurve
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 3717
Proposed branch: lp:~vanvugt/mir/GammaCurve
Merge into: lp:mir
Diff against target: 148 lines (+29/-26)
6 files modified
include/platform/mir/graphics/gamma_curves.h (+8/-6)
src/platform/graphics/gamma_curves.cpp (+3/-3)
src/platforms/mesa/server/kms/real_kms_display_configuration.cpp (+3/-3)
src/server/frontend/session_mediator.cpp (+6/-5)
tests/acceptance-tests/test_new_display_configuration.cpp (+6/-6)
tests/unit-tests/graphics/test_gamma_curves.cpp (+3/-3)
To merge this branch: bzr merge lp:~vanvugt/mir/GammaCurve
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
Alan Griffiths Abstain
Mir CI Bot continuous-integration Approve
Review via email: mp+306734@code.launchpad.net

Commit message

One definition for GammaCurve

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

PASSED: Continuous integration, rev:3715
https://mir-jenkins.ubuntu.com/job/mir-ci/1812/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2264
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2327
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2318
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2318
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2318
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2292
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2292/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/2292
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2292/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2292
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2292/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/2292
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2292/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/2292
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2292/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

+typedef std::vector<uint16_t> GammaCurve;
...
- GammaCurves(std::vector<uint16_t> const& red,
- std::vector<uint16_t> const& green,
- std::vector<uint16_t> const& blue);
+ GammaCurves(GammaCurve const& red,
+ GammaCurve const& green,
+ GammaCurve const& blue);

One has to wonder if there are implicit constraints (like the three vectors having the same length) and if *all* the functionality of std::vector<> is appropriate to GammaCurve.

But this MP doesn't create those issues, it just fails to address them.

review: Abstain
Revision history for this message
Kevin DuBois (kdub) wrote :

> +typedef std::vector<uint16_t> GammaCurve;
> ...
> - GammaCurves(std::vector<uint16_t> const& red,
> - std::vector<uint16_t> const& green,
> - std::vector<uint16_t> const& blue);
> + GammaCurves(GammaCurve const& red,
> + GammaCurve const& green,
> + GammaCurve const& blue);
>
> One has to wonder if there are implicit constraints (like the three vectors
> having the same length) and if *all* the functionality of std::vector<> is
> appropriate to GammaCurve.
>
> But this MP doesn't create those issues, it just fails to address them.

Yeah, they could be improved, but this MP by itself looks alright to me. IIRC from a few weeks ago, they're 3 independent lookup tables, and they are all the same size in mesa, but not sure if that holds generally.

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

I had the same thoughts. But decided to only solve one problem at a time, one branch at a time.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/platform/mir/graphics/gamma_curves.h'
2--- include/platform/mir/graphics/gamma_curves.h 2016-09-23 07:37:34 +0000
3+++ include/platform/mir/graphics/gamma_curves.h 2016-09-26 05:31:33 +0000
4@@ -27,18 +27,20 @@
5 namespace graphics
6 {
7
8+typedef std::vector<uint16_t> GammaCurve;
9+
10 class GammaCurves
11 {
12 public:
13 GammaCurves() = default;
14
15- GammaCurves(std::vector<uint16_t> const& red,
16- std::vector<uint16_t> const& green,
17- std::vector<uint16_t> const& blue);
18+ GammaCurves(GammaCurve const& red,
19+ GammaCurve const& green,
20+ GammaCurve const& blue);
21
22- std::vector<uint16_t> red;
23- std::vector<uint16_t> green;
24- std::vector<uint16_t> blue;
25+ GammaCurve red;
26+ GammaCurve green;
27+ GammaCurve blue;
28 };
29
30 }
31
32=== modified file 'src/platform/graphics/gamma_curves.cpp'
33--- src/platform/graphics/gamma_curves.cpp 2016-09-19 04:16:15 +0000
34+++ src/platform/graphics/gamma_curves.cpp 2016-09-26 05:31:33 +0000
35@@ -23,9 +23,9 @@
36
37 namespace mg = mir::graphics;
38
39-mg::GammaCurves::GammaCurves(std::vector<uint16_t> const& red,
40- std::vector<uint16_t> const& green,
41- std::vector<uint16_t> const& blue) :
42+mg::GammaCurves::GammaCurves(GammaCurve const& red,
43+ GammaCurve const& green,
44+ GammaCurve const& blue) :
45 red(red),
46 green(green),
47 blue(blue)
48
49=== modified file 'src/platforms/mesa/server/kms/real_kms_display_configuration.cpp'
50--- src/platforms/mesa/server/kms/real_kms_display_configuration.cpp 2016-09-19 04:16:15 +0000
51+++ src/platforms/mesa/server/kms/real_kms_display_configuration.cpp 2016-09-26 05:31:33 +0000
52@@ -191,9 +191,9 @@
53 std::system_error(errno, std::system_category(), "drmModeCrtcGetGamma Failed"));
54 }
55
56- return {std::vector<uint16_t>(red, red + gamma_size),
57- std::vector<uint16_t>(green, green + gamma_size),
58- std::vector<uint16_t>(blue, blue + gamma_size)};
59+ return {GammaCurve(red, red + gamma_size),
60+ GammaCurve(green, green + gamma_size),
61+ GammaCurve(blue, blue + gamma_size)};
62 }
63
64 void mgm::RealKMSDisplayConfiguration::update()
65
66=== modified file 'src/server/frontend/session_mediator.cpp'
67--- src/server/frontend/session_mediator.cpp 2016-09-19 04:16:15 +0000
68+++ src/server/frontend/session_mediator.cpp 2016-09-26 05:31:33 +0000
69@@ -41,6 +41,7 @@
70 #include "mir/graphics/platform_ipc_operations.h"
71 #include "mir/graphics/platform_ipc_package.h"
72 #include "mir/graphics/platform_operation_message.h"
73+#include "mir/graphics/gamma_curves.h"
74 #include "mir/frontend/client_constants.h"
75 #include "mir/frontend/event_sink.h"
76 #include "mir/frontend/screencast.h"
77@@ -79,9 +80,9 @@
78
79 namespace
80 {
81-std::vector<uint16_t> convert_string_to_uint16_vector(std::string const& str_bytes)
82+mg::GammaCurve convert_string_to_gamma_curve(std::string const& str_bytes)
83 {
84- std::vector<uint16_t> out(str_bytes.size() / (sizeof(uint16_t) / sizeof(char)));
85+ mg::GammaCurve out(str_bytes.size() / (sizeof(mg::GammaCurve::value_type) / sizeof(char)));
86 std::copy(std::begin(str_bytes), std::end(str_bytes), reinterpret_cast<char*>(out.data()));
87 return out;
88 }
89@@ -1231,9 +1232,9 @@
90 dest.power_mode = static_cast<MirPowerMode>(src.power_mode());
91 dest.orientation = static_cast<MirOrientation>(src.orientation());
92
93- dest.gamma = {convert_string_to_uint16_vector(src.gamma_red()),
94- convert_string_to_uint16_vector(src.gamma_green()),
95- convert_string_to_uint16_vector(src.gamma_blue())};
96+ dest.gamma = {convert_string_to_gamma_curve(src.gamma_red()),
97+ convert_string_to_gamma_curve(src.gamma_green()),
98+ convert_string_to_gamma_curve(src.gamma_blue())};
99 });
100
101 return config;
102
103=== modified file 'tests/acceptance-tests/test_new_display_configuration.cpp'
104--- tests/acceptance-tests/test_new_display_configuration.cpp 2016-09-19 04:16:15 +0000
105+++ tests/acceptance-tests/test_new_display_configuration.cpp 2016-09-26 05:31:33 +0000
106@@ -816,9 +816,9 @@
107 TEST_F(DisplayConfigurationTest, client_sees_server_set_gamma)
108 {
109 uint32_t const size = 4;
110- std::vector<uint16_t> const a{0, 1, 2, 3};
111- std::vector<uint16_t> const b{1, 2, 3, 4};
112- std::vector<uint16_t> const c{65532, 65533, 65534, 65535};
113+ mg::GammaCurve const a{0, 1, 2, 3};
114+ mg::GammaCurve const b{1, 2, 3, 4};
115+ mg::GammaCurve const c{65532, 65533, 65534, 65535};
116 std::vector<mg::GammaCurves> const gammas = {
117 {a, b, c},
118 {b, c, a},
119@@ -882,9 +882,9 @@
120
121 TEST_F(DisplayConfigurationTest, client_can_set_gamma)
122 {
123- std::vector<uint16_t> const a{0, 1, 2, 3};
124- std::vector<uint16_t> const b{1, 2, 3, 4};
125- std::vector<uint16_t> const c{65532, 65533, 65534, 65535};
126+ mg::GammaCurve const a{0, 1, 2, 3};
127+ mg::GammaCurve const b{1, 2, 3, 4};
128+ mg::GammaCurve const c{65532, 65533, 65534, 65535};
129 std::vector<mg::GammaCurves> const gammas = {
130 {a, b, c},
131 {b, c, a},
132
133=== modified file 'tests/unit-tests/graphics/test_gamma_curves.cpp'
134--- tests/unit-tests/graphics/test_gamma_curves.cpp 2016-09-01 17:02:56 +0000
135+++ tests/unit-tests/graphics/test_gamma_curves.cpp 2016-09-26 05:31:33 +0000
136@@ -25,9 +25,9 @@
137
138 namespace
139 {
140-std::vector<uint16_t> const r{1};
141-std::vector<uint16_t> const g{2};
142-std::vector<uint16_t> const b{3};
143+mg::GammaCurve const r{1};
144+mg::GammaCurve const g{2};
145+mg::GammaCurve const b{3};
146 }
147
148 class MockGammaCurves : public testing::Test

Subscribers

People subscribed via source and target branches