Output device color curves correction

Authored by davidedmundson on Apr 20 2018, 6:29 PM.



Extends the output device and output configuration interfaces with the ability
to query and set the RGB color intensity curves (gamma ramps) of the
associated output.

Test Plan

Manually. Auto tests will be added to this diff soon.

Diff Detail

R127 KWayland
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
romangg created this revision.Apr 20 2018, 6:29 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 20 2018, 6:29 PM
romangg requested review of this revision.Apr 20 2018, 6:29 PM
cfeck added a subscriber: cfeck.Apr 20 2018, 7:45 PM
cfeck added inline comments.

The documentation could state how many elements need to be in the vectors. Ideally, every component could have any number of elements, and if there are too few, the other elements are interpolated.

But it is probably simpler to just state "The number of elements in each vector must be 256 or 1024, depending on the depth of the framebuffer (24 bits or 30 bits)."


X11 uses 16 bit values even for 24 bit screens, because the actual values that are sent to the DAC can have higher precision than the screen pixmap. I doubt that Wayland has reduced the precision, so I don't think checking values here is right.

zzag added a subscriber: zzag.Apr 20 2018, 8:10 PM
zzag added inline comments.

Maybe static_cast?

I haven't used wl_array but can't you allocate big enough contiguous chunk of memory and call memcpy, e.g.

wl_array wlRed;

auto* redDest = wl_array_add(&wlRed, sizeof(uint16_t) * red.count());
memcpy(redDest, red.data(), sizeof(uint16_t) * red.count());
romangg added inline comments.May 1 2018, 2:26 PM

Good point about allocating directly all of the memory. I looked up the wl_array definition and should be no problem.


Number of elements depends on the OutputDevice technical possibilities (i.e. for example as you said 256 for 8bit color ramps and 1024 for 10bit ones and every other value between 1 and 16bit ramp size).

Will add a comment to the doc.


I forget to remove this comment. This is not an issue since we can "stretch" the image over the whole uint16_t value domain linearly such that image 1 is associated with the largest uint16_t value. By that we have well defined arguments in any case.

The array size though is checked before that in the checkArg call by comparing it with the size set first by the compositor.

romangg updated this revision to Diff 33397.May 1 2018, 2:27 PM
  • autotests
  • improve doc
  • memcpy wl_array

Looks pretty good. +1


You can do this all in once


romangg updated this revision to Diff 33453.May 1 2018, 8:52 PM
  • memcpy wl_array in colorcurvesCallback
romangg marked an inline comment as done.May 1 2018, 8:53 PM
davidedmundson accepted this revision.May 1 2018, 10:55 PM

Note frameworks tags are in 3 days. It would be reckless to rush merging anything.

This revision is now accepted and ready to land.May 1 2018, 10:55 PM
graesslin requested changes to this revision.May 2 2018, 4:50 PM
graesslin added a subscriber: graesslin.

Please increase the versions.


please increment version


since is missing


you cannot add requests in between, a new request must be the last.


version needs incrementation


you cannot add events in between, new one needs to be last.

This revision now requires changes to proceed.May 2 2018, 4:50 PM
romangg updated this revision to Diff 33551.May 3 2018, 2:35 PM
  • Fix protocol versions, events/requests order
romangg marked 5 inline comments as done.May 3 2018, 2:37 PM
zzag added inline comments.May 3 2018, 2:45 PM

You forgot const. :-D
Also, I would use size_t instead of int.

romangg updated this revision to Diff 33554.May 3 2018, 2:57 PM

Rebase on current master

zzag added inline comments.May 3 2018, 2:59 PM

That's not C so you can get rid of struct keyword. ;-)
Also, you could constify curve to show that it's read-only.


constify newColor?
Also, you could cast oldColor.size() in C++ manner, e.g. static_cast<size_t>(...)?


constify wl_array?
constify pos, e.g. uint16_t const *pos?


constify origin?

Can't you memcpy here?

romangg updated this revision to Diff 33563.May 3 2018, 5:14 PM
  • Use const size_t instead of int
  • Remove struct keyword on function calls
  • More const keyword
  • memcpy in sendColorCurves
romangg marked 6 inline comments as done.May 3 2018, 5:15 PM
romangg updated this revision to Diff 33564.May 3 2018, 5:19 PM

Increase org_kde_kwin_outputmanagement version as well
(needed to create config objects of version 2)

zzag added a comment.May 3 2018, 9:54 PM
This comment was removed by zzag.
zzag added inline comments.May 4 2018, 12:56 AM

You can resize v and call memcpy (or use std::copy?)

graesslin added inline comments.May 15 2018, 6:51 PM

nitpick: added unrelated new line


you need to do a version check and only send to clients which have the required version. A FOO_SINCE macro should be defined by the generated wayland header.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 15 2018, 6:51 PM
romangg updated this revision to Diff 34770.EditedMay 24 2018, 5:19 AM
  • Send color curves only for version 2 and above
  • Rebase on master
romangg marked 2 inline comments as done.May 24 2018, 5:20 AM
davidedmundson commandeered this revision.Jul 16 2018, 3:03 PM
davidedmundson edited reviewers, added: romangg; removed: davidedmundson.

Comadeering to rebase on top of my changes that also moved outputdevice to version 2.

Rebase and fix conflicts

romangg accepted this revision.Jul 30 2018, 12:38 PM
romangg added inline comments.


This revision was not accepted when it landed; it landed in state Needs Review.Aug 5 2018, 4:05 PM
This revision was automatically updated to reflect the committed changes.