D12388: Output device color curves correction
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R127:3d06279076f1: Output device color curves correction (authored by romangg, committed by davidedmundson). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D12388?vs=37879&id=39134#toc REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=37879&id=39134 REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: davidedmundson, #frameworks, graesslin, romangg Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
romangg accepted this revision. romangg added inline comments. INLINE COMMENTS > outputconfiguration.h:203 > /** > +<<< HEAD > * Scale rendering of this output. rm REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: davidedmundson, #frameworks, graesslin, romangg Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
davidedmundson updated this revision to Diff 37879. davidedmundson added a comment. Rebase and fix conflicts REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=34770&id=37879 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: davidedmundson, #frameworks, graesslin, romangg Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
davidedmundson commandeered this revision. davidedmundson edited reviewers, added: romangg; removed: davidedmundson. davidedmundson added a comment. Comadeering to rebase on top of my changes that also moved outputdevice to version 2. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: davidedmundson, #frameworks, graesslin, romangg Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
romangg marked 2 inline comments as done. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 34770. romangg added a comment. - Send color curves only for version 2 and above REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33564&id=34770 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/client/registry.cpp src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h src/server/outputmanagement_interface.cpp To: romangg, #frameworks, davidedmundson, graesslin Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
graesslin added inline comments. Restricted Application added a subscriber: kde-frameworks-devel. INLINE COMMENTS > outputchangeset.cpp:91 > } > + > bool OutputChangeSet::positionChanged() const nitpick: added unrelated new line > outputdevice_interface.cpp:427 > + > +org_kde_kwin_outputdevice_send_colorcurves(data.resource, &wlRed, > &wlGreen, &wlBlue); > + 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. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: kde-frameworks-devel, graesslin, davidedmundson, zzag, cfeck, michaelh, ngraham, bruns
D12388: Output device color curves correction
zzag added inline comments. INLINE COMMENTS > zzag wrote in outputconfiguration_interface.cpp:221 > constify wl_array? > constify pos, e.g. `uint16_t const *pos`? You can resize v and call memcpy (or use std::copy?) REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
zzag added a comment. const uint16_t *pos = (uint16_t*)array->data; Well, now you throw away const qualifiers. I think it should be something like this const uint16_t *pos = reinterpret_cast(array->data); I suggest to get rid of C style casts because they aren't checked by the compiler. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33564. romangg added a comment. Increase org_kde_kwin_outputmanagement version as well (needed to create config objects of version 2) REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33563&id=33564 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/client/registry.cpp src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h src/server/outputmanagement_interface.cpp To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg marked 6 inline comments as done. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33563. romangg added a comment. - Use const size_t instead of int - Remove struct keyword on function calls - More const keyword - memcpy in sendColorCurves REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33554&id=33563 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/client/registry.cpp src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
zzag added inline comments.
INLINE COMMENTS
> outputdevice.cpp:321
> +
> +auto setCurve = [](struct wl_array *curve, QVector
> *destination) {
> +destination->resize(curve->size / sizeof(uint16_t));
That's not C so you can get rid of `struct` keyword. ;-)
Also, you could constify `curve` to show that it's read-only.
> outputconfiguration_interface.cpp:208
> +
> +auto checkArg = [](wl_array *newColor, const QVector &oldColor)
> {
> +return (newColor->size % sizeof(uint16_t) == 0) &&
constify newColor?
Also, you could cast oldColor.size() in C++ manner, e.g.
static_cast(...)?
> outputconfiguration_interface.cpp:221
> +
> +auto fillVector = [](wl_array *array, QVector *v) {
> +uint16_t *pos = (uint16_t*)array->data;
constify wl_array?
constify pos, e.g. `uint16_t const *pos`?
> outputdevice_interface.cpp:420
> +
> +auto fillArray = [](QVector &origin, wl_array *dest) {
> +for (auto c : origin) {
constify origin?
Can't you memcpy here?
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D12388
To: romangg, #frameworks, davidedmundson, graesslin
Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33554. romangg added a comment. Rebase on current master REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33551&id=33554 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/client/registry.cpp src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
zzag added inline comments. INLINE COMMENTS > outputconfiguration.cpp:180 > +wl_array_init(dest); > +int memLength = sizeof(uint16_t) * origin.size(); > +void *s = wl_array_add(dest, memLength); You forgot `const`. :-D Also, I would use `size_t` instead of `int`. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg marked 5 inline comments as done. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33551. romangg added a comment. - Fix protocol versions, events/requests order REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33453&id=33551 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/client/registry.cpp src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
graesslin requested changes to this revision. graesslin added a comment. This revision now requires changes to proceed. Please increase the versions. INLINE COMMENTS > output-management.xml:82 > > > please increment version > output-management.xml:142 > > + > + since is missing > output-management.xml:142 > > + > + you cannot add requests in between, a new request must be the last. > outputdevice.xml:32 > > > version needs incrementation > outputdevice.xml:179 > > + > + you cannot add events in between, new one needs to be last. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson, graesslin Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
davidedmundson accepted this revision. davidedmundson added a comment. This revision is now accepted and ready to land. Note frameworks tags are in 3 days. It would be reckless to rush merging anything. REPOSITORY R127 KWayland BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks, davidedmundson Cc: davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg marked an inline comment as done. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks Cc: davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33453. romangg added a comment. - memcpy wl_array in colorcurvesCallback REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=33397&id=33453 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks Cc: davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
davidedmundson added a comment.
Looks pretty good. +1
INLINE COMMENTS
> outputdevice.cpp:323
> +uint16_t *curvePtr = reinterpret_cast(curve->data);
> +for (size_t i = 0; i < curve->size / sizeof(uint16_t); i++) {
> +destination->append(curvePtr[i]);
You can do this all in once
destination->resize(curve->size);
memcpy(...)
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D12388
To: romangg, #frameworks
Cc: davidedmundson, zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg updated this revision to Diff 33397. romangg added a comment. - autotests - improve doc - memcpy wl_array REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12388?vs=32669&id=33397 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES autotests/client/test_wayland_outputdevice.cpp autotests/client/test_wayland_outputmanagement.cpp src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks Cc: zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg added inline comments. INLINE COMMENTS > zzag wrote in outputconfiguration.cpp:184 > 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; > > wl_array_init(&wlRed); > auto* redDest = wl_array_add(&wlRed, sizeof(uint16_t) * red.count()); > memcpy(redDest, red.data(), sizeof(uint16_t) * red.count()); Good point about allocating directly all of the memory. I looked up the wl_array definition and should be no problem. > cfeck wrote in outputconfiguration.h:211 > 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)." 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. > cfeck wrote in outputconfiguration_interface.cpp:217 > 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. 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. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks Cc: zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
zzag added inline comments. INLINE COMMENTS > outputconfiguration.cpp:184 > +uint16_t *s = reinterpret_cast(wl_array_add(dest, > sizeof(uint16_t))); > +*s = (uint16_t)c; > +} 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; wl_array_init(&wlRed); auto* redDest = wl_array_add(&wlRed, sizeof(uint16_t) * red.count()); memcpy(redDest, red.data(), sizeof(uint16_t) * red.count()); REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D12388 To: romangg, #frameworks Cc: zzag, cfeck, michaelh, bruns
D12388: Output device color curves correction
cfeck added inline comments.
INLINE COMMENTS
> outputconfiguration.h:211
> + */
> +void setColorCurves(OutputDevice *outputdevice, QVector red,
> QVector green, QVector blue);
> +
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)."
> outputconfiguration_interface.cpp:217
> +while((char*)pos < (char*)array->data + array->size) {
> +// TODO: check value not bigger than size (i.e. for 8bit display
> < 255)?
> +v->append(*pos);
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.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D12388
To: romangg, #frameworks
Cc: cfeck, michaelh, bruns
D12388: Output device color curves correction
romangg created this revision. romangg added a reviewer: Frameworks. Restricted Application added a project: Frameworks. romangg requested review of this revision. REVISION SUMMARY 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. REPOSITORY R127 KWayland BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL https://phabricator.kde.org/D12388 AFFECTED FILES src/client/outputconfiguration.cpp src/client/outputconfiguration.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/protocols/output-management.xml src/client/protocols/outputdevice.xml src/server/outputchangeset.cpp src/server/outputchangeset.h src/server/outputchangeset_p.h src/server/outputconfiguration_interface.cpp src/server/outputdevice_interface.cpp src/server/outputdevice_interface.h To: romangg, #frameworks Cc: michaelh, bruns
