-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sway/commands/output: Add command to set color profile #7681
Conversation
de50c62
to
d42ec48
Compare
4d3e7e2
to
e7b2b9e
Compare
e7b2b9e
to
d6a469c
Compare
d6a469c
to
2a56c09
Compare
2a56c09
to
e0b0a78
Compare
e0b0a78
to
1b3abca
Compare
1b3abca
to
76d77dd
Compare
76d77dd
to
72b9879
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments, overall looks good
Sets the color profile for an output. The default is _srgb_. <file> should be a | ||
path to a display ICC profile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with HDR? I would assume that ICC is incompatible with HDR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know; I made this PR mainly to test the wlroots MR, and do not know what a good command interface for the long term would look like. Marking this PR as draft until this gets figured out; maybe the answer will be that, like render_bit_depth, this is a short-term approach to be replaced later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a warning that setting a color profile may have odd effects in conjunction with HDR.
Color profiles probably still have a meaningful interpretation even when when the HDR metadata is being sent to a display -- I believe the main difference is that the monitor handles pixel data differently in HDR mode, using absolute luminance, and uses PQ EOTF instead of approximate-sRGB. For example, it should still be possible to use color profiles to correct for monitor miscalibration; or just to apply general color transforms (e.g.: adjusting contrast, making everything brighter, rendering in grayscale, applying hue shifts -- although it might be cleaner to have such color changes be handled with a separate command, using 'Abstract' instead of 'Display' ICC profiles.) Of course, due to the difffering EOTFs being applied by the monitor, any profile used for SDR would produce invalid output for HDR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is unhappy it seems like, I think we need to add #ifs (that's a bit cumbersome) and missing deps in CI manifests?
sway/commands/output/color_profile.c
Outdated
@@ -0,0 +1,103 @@ | |||
#define _XOPEN_SOURCE 700 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we #undef _POSIX_C_SOURCE
if we need _XOPEN_SOURCE
? Otherwise we provide two feature test macros and they potentially conflict (in which case what we get is implementation-defined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think _XOPEN_SOURCE is needed at all -- this code has been edited a lot since its original version -- so I just dropped this.
sway/commands/output/color_profile.c
Outdated
|
||
return NULL; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: stray newline it seems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
sway/sway-output.5.scd
Outdated
path to a display ICC profile. | ||
|
||
Not all renderers support this feature; currently it only works with the | ||
the Vulkan renderer works. Even where supported, the application of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "it only works with the Vulkan renderer."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
I've added missing dependencies for lcms2, so that the wlroots build (and by extension the sway build) uses color management features. I have not yet made it possible to build this without lcms2 support, and will update this PR eventually to do that. |
Tested it on my Framework 13 AMD. Seems to work as expected 👍 Just an idea, would it be reasonable to be able to Maybe something like: |
The default profile is
|
I've been wondering whether we should always make
Weird, the ICC profile should only affect the final post-blending image. Maybe there is a small difference not noticable without the ICC profile applied, which becomes visible once the ICC profile is applied? |
I've also been considering this: 1) one might want to use CICP tags to pick output color profiles (e.g. to work with HDR10) in which case one can do color management without LCMS 2) the use of LCMS in wlroots is already confined to converting ICC file data to |
Right. AFAIU, Weston plans to translate the CICP stuff into LCMS pipelines (with the float point number transforms that nobody uses in ICC). Other compositors seem to do math in shaders. |
I still have the issue, even with the env var |
This makes it possible to render output buffers in a different color space, by specifying an ICC profile for the output.
Now that the wlroots patch has been merged, just need to guard |
This is already done: wlroots compiles it to a NULL-returning stub, and the code checks for an returns an error on a NULL return value. |
Oh right, didn't even remember about the patch I wrote. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is there a reason this is still marked as a draft?
No; as this is no longer waiting on wlroots changes I have marked it as ready. |
Thanks! |
This PR matches the very WIP wlroots MR 4280.
This requires the Vulkan renderer to be useful. (Use
WLR_RENDERER=vulkan
; settingVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation
to enable validation layers may be useful for testing.)Example usage:
Known issues:
some race condition in which only some frames render correctly; causes flickering(fixed)Only applies with bit depth 10, at the moment(fixed)Having to copy the(now using reference counting)struct wlr_color_transform
a number of times is awkward