-
Notifications
You must be signed in to change notification settings - Fork 1.2k
sway/output: new "edid" and "gamma22" modes for color_profile #8934
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
base: master
Are you sure you want to change the base?
Conversation
|
Blocked on two unmerged wlroots MR's, so converted to draft for good measure. |
I'm not sure it makes sense for this to be gamma22 only, it's perfectly valid to use the display's self reported primaries along with either srgb/gamma22 transfer depending on what the display actually does |
|
I agree 'edid' is more useful as a modifier for the first two. I've switched to doing that Also adding a check to not build identity color matrix, which it turns out would happen on more displays that I thought |
|
Would it be possible to use an ICC file as sole source for primaries, without having the whole ICC used as it currently exists? Alternatively, being able to specify custom primaries may be useful. But that might be out of scope for this PR. Most likely I should just use the existing |
|
In principle yes, but I think your use case would be best served by some sort of "fast" option for the icc setting The modes from this PR are not optimal either : all except the default will disallow direct scanout wlroots still has margin for improvement around hardware offload, that would help both |
|
The required wlroots work is merged : this now looks as intended Gaining direct scanout for the new modes is still some changes away, though |
|
I wonder if it would make sense to support this for the |
That wouldn't really work - I imagine you were thinking of was instead rules that apply whenever a particular window is on screen or fullscreen, which |
|
Do you mean a That would be a way to complete the signal chain and give the user full control over the srgb gamma question, but it touches different bits than this PR |
Yes, and I agree that it wouldn't need to be a part of this PR |
No I was describing shortcomings of a for_window rule to apply the output commands implemented here to change TF - given that those commands are the scope of the PR I thought that was your suggestion. Properties like color management are surface traits, while a window can be comprised of many surfaces with intentionally different traits. Trying to override things there would be tricky and likely to cause issues, especially if the goal would be to override already set image descriptions rather than, say, a default image description for surfaces without color management. |
emersion
left a comment
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.
Thanks for the patch! Here are a few comments.
sway/config/output.c
Outdated
| wlr_color_transform_ref(oc->color_transform); | ||
| } | ||
| wlr_color_transform_unref(output->color_transform); | ||
| output->color_transform = oc->color_transform; |
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.
This updates sway_output, however this is called from queue_output_config() so may be rolled back in case the output commit fails.
queue_output_config() should only update wlr_output.pending and shouldn't update other fields.
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.
moved the call to apply_resolved_output_configs.
Still a bit confused over how this could be moved back to finalize_output_config without showing a wrong frame
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.
Hm, I'm still a bit antsy about applying after test() but before commit(): commit() (or operations before that call) may fail still, and we'd end up applying the change even though we shouldn't…
Maybe we could extract the color transform from the output config instead of using cfg->output->color_transform in apply_resolved_output_configs()?
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.
Maybe we could extract the color transform from the output config instead of using cfg->output->color_transform in apply_resolved_output_configs()?
Ah, I suppose this approach will not scale with the rest of the output properties, such as output position?
We probably need to apply after test() and rollback on error? This would have the downside of potentially flip-flopping some state (which might be visible externally via e.g. IPC), on the other hand commit() and ENOMEM errors should be quite rare (test() passed, so most errors should've been caught already).
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.
Trying with a new 'state' struct, to use the new color transform for the first commit, but move it into sway_output in finalize_output_config
41dfe1f to
ac38b49
Compare
Currently, config apply sets the output's image description before the initial commit, but sets the output's color transform after the commit. In the case of a config reload removing a color profile and enabling HDR, both the color transform and image description will be set, which trips an assert in wlroots
Document that "gamma22" replaced the previous default.
When a display is connected, create a color transform from its self-reported color characteristics
|
updated for previous remarks + changed error handling in |
This adds the two modes mentioned in #8922 (comment)
output <criteria> color_profile gamma22takes the place of the current defaultsrgb(no color processing)output <criteria> color_profile srgbassumes the display uses the piece-wise srgb transfer functionedidargument can be added to either, to build a color profile using it and the display's self-reported color characteristics