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

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,

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=37879 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12388

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

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=34770 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL

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 > + > +

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

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

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=33564 BRANCH

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

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

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=33554 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL

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`.

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=33551 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL

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 > > + > +

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

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=33453 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL

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

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=33397 BRANCH outputColorCorrectionCurvesOnly REVISION DETAIL

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(); > auto* redDest

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

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

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