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
=== modified file 'include/platform/mir/graphics/gamma_curves.h'
--- include/platform/mir/graphics/gamma_curves.h 2016-09-23 07:37:34 +0000
+++ include/platform/mir/graphics/gamma_curves.h 2016-09-26 05:31:33 +0000
@@ -27,18 +27,20 @@
27namespace graphics27namespace graphics
28{28{
2929
30typedef std::vector<uint16_t> GammaCurve;
31
30class GammaCurves32class GammaCurves
31{33{
32public:34public:
33 GammaCurves() = default;35 GammaCurves() = default;
3436
35 GammaCurves(std::vector<uint16_t> const& red,37 GammaCurves(GammaCurve const& red,
36 std::vector<uint16_t> const& green,38 GammaCurve const& green,
37 std::vector<uint16_t> const& blue);39 GammaCurve const& blue);
3840
39 std::vector<uint16_t> red;41 GammaCurve red;
40 std::vector<uint16_t> green;42 GammaCurve green;
41 std::vector<uint16_t> blue;43 GammaCurve blue;
42};44};
4345
44}46}
4547
=== modified file 'src/platform/graphics/gamma_curves.cpp'
--- src/platform/graphics/gamma_curves.cpp 2016-09-19 04:16:15 +0000
+++ src/platform/graphics/gamma_curves.cpp 2016-09-26 05:31:33 +0000
@@ -23,9 +23,9 @@
2323
24namespace mg = mir::graphics;24namespace mg = mir::graphics;
2525
26mg::GammaCurves::GammaCurves(std::vector<uint16_t> const& red,26mg::GammaCurves::GammaCurves(GammaCurve const& red,
27 std::vector<uint16_t> const& green,27 GammaCurve const& green,
28 std::vector<uint16_t> const& blue) :28 GammaCurve const& blue) :
29 red(red),29 red(red),
30 green(green),30 green(green),
31 blue(blue)31 blue(blue)
3232
=== modified file 'src/platforms/mesa/server/kms/real_kms_display_configuration.cpp'
--- src/platforms/mesa/server/kms/real_kms_display_configuration.cpp 2016-09-19 04:16:15 +0000
+++ src/platforms/mesa/server/kms/real_kms_display_configuration.cpp 2016-09-26 05:31:33 +0000
@@ -191,9 +191,9 @@
191 std::system_error(errno, std::system_category(), "drmModeCrtcGetGamma Failed"));191 std::system_error(errno, std::system_category(), "drmModeCrtcGetGamma Failed"));
192 }192 }
193193
194 return {std::vector<uint16_t>(red, red + gamma_size),194 return {GammaCurve(red, red + gamma_size),
195 std::vector<uint16_t>(green, green + gamma_size),195 GammaCurve(green, green + gamma_size),
196 std::vector<uint16_t>(blue, blue + gamma_size)};196 GammaCurve(blue, blue + gamma_size)};
197}197}
198198
199void mgm::RealKMSDisplayConfiguration::update()199void mgm::RealKMSDisplayConfiguration::update()
200200
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp 2016-09-19 04:16:15 +0000
+++ src/server/frontend/session_mediator.cpp 2016-09-26 05:31:33 +0000
@@ -41,6 +41,7 @@
41#include "mir/graphics/platform_ipc_operations.h"41#include "mir/graphics/platform_ipc_operations.h"
42#include "mir/graphics/platform_ipc_package.h"42#include "mir/graphics/platform_ipc_package.h"
43#include "mir/graphics/platform_operation_message.h"43#include "mir/graphics/platform_operation_message.h"
44#include "mir/graphics/gamma_curves.h"
44#include "mir/frontend/client_constants.h"45#include "mir/frontend/client_constants.h"
45#include "mir/frontend/event_sink.h"46#include "mir/frontend/event_sink.h"
46#include "mir/frontend/screencast.h"47#include "mir/frontend/screencast.h"
@@ -79,9 +80,9 @@
7980
80namespace81namespace
81{82{
82std::vector<uint16_t> convert_string_to_uint16_vector(std::string const& str_bytes)83mg::GammaCurve convert_string_to_gamma_curve(std::string const& str_bytes)
83{84{
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)));
85 std::copy(std::begin(str_bytes), std::end(str_bytes), reinterpret_cast<char*>(out.data()));86 std::copy(std::begin(str_bytes), std::end(str_bytes), reinterpret_cast<char*>(out.data()));
86 return out;87 return out;
87}88}
@@ -1231,9 +1232,9 @@
1231 dest.power_mode = static_cast<MirPowerMode>(src.power_mode());1232 dest.power_mode = static_cast<MirPowerMode>(src.power_mode());
1232 dest.orientation = static_cast<MirOrientation>(src.orientation());1233 dest.orientation = static_cast<MirOrientation>(src.orientation());
12331234
1234 dest.gamma = {convert_string_to_uint16_vector(src.gamma_red()),1235 dest.gamma = {convert_string_to_gamma_curve(src.gamma_red()),
1235 convert_string_to_uint16_vector(src.gamma_green()),1236 convert_string_to_gamma_curve(src.gamma_green()),
1236 convert_string_to_uint16_vector(src.gamma_blue())};1237 convert_string_to_gamma_curve(src.gamma_blue())};
1237 });1238 });
12381239
1239 return config;1240 return config;
12401241
=== modified file 'tests/acceptance-tests/test_new_display_configuration.cpp'
--- tests/acceptance-tests/test_new_display_configuration.cpp 2016-09-19 04:16:15 +0000
+++ tests/acceptance-tests/test_new_display_configuration.cpp 2016-09-26 05:31:33 +0000
@@ -816,9 +816,9 @@
816TEST_F(DisplayConfigurationTest, client_sees_server_set_gamma)816TEST_F(DisplayConfigurationTest, client_sees_server_set_gamma)
817{817{
818 uint32_t const size = 4;818 uint32_t const size = 4;
819 std::vector<uint16_t> const a{0, 1, 2, 3};819 mg::GammaCurve const a{0, 1, 2, 3};
820 std::vector<uint16_t> const b{1, 2, 3, 4};820 mg::GammaCurve const b{1, 2, 3, 4};
821 std::vector<uint16_t> const c{65532, 65533, 65534, 65535};821 mg::GammaCurve const c{65532, 65533, 65534, 65535};
822 std::vector<mg::GammaCurves> const gammas = {822 std::vector<mg::GammaCurves> const gammas = {
823 {a, b, c},823 {a, b, c},
824 {b, c, a},824 {b, c, a},
@@ -882,9 +882,9 @@
882882
883TEST_F(DisplayConfigurationTest, client_can_set_gamma)883TEST_F(DisplayConfigurationTest, client_can_set_gamma)
884{884{
885 std::vector<uint16_t> const a{0, 1, 2, 3};885 mg::GammaCurve const a{0, 1, 2, 3};
886 std::vector<uint16_t> const b{1, 2, 3, 4};886 mg::GammaCurve const b{1, 2, 3, 4};
887 std::vector<uint16_t> const c{65532, 65533, 65534, 65535};887 mg::GammaCurve const c{65532, 65533, 65534, 65535};
888 std::vector<mg::GammaCurves> const gammas = {888 std::vector<mg::GammaCurves> const gammas = {
889 {a, b, c},889 {a, b, c},
890 {b, c, a},890 {b, c, a},
891891
=== modified file 'tests/unit-tests/graphics/test_gamma_curves.cpp'
--- tests/unit-tests/graphics/test_gamma_curves.cpp 2016-09-01 17:02:56 +0000
+++ tests/unit-tests/graphics/test_gamma_curves.cpp 2016-09-26 05:31:33 +0000
@@ -25,9 +25,9 @@
2525
26namespace26namespace
27{27{
28std::vector<uint16_t> const r{1};28mg::GammaCurve const r{1};
29std::vector<uint16_t> const g{2};29mg::GammaCurve const g{2};
30std::vector<uint16_t> const b{3};30mg::GammaCurve const b{3};
31}31}
3232
33class MockGammaCurves : public testing::Test33class MockGammaCurves : public testing::Test

Subscribers

People subscribed via source and target branches