Skip to content
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

WIP: Postprocess shader support #1058

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

WIP: Postprocess shader support #1058

wants to merge 3 commits into from

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Dec 13, 2024

This renames MirroringState to OffscreenState, which is now used for mirroring (with a different size) and/or postprocessing.

Currently a shader can be set by manually putting a shader path in ~/.config/cosmic/com.system76.CosmicComp/v1/postprocess_shader_path. This can be modified from https://github.com/Smithay/smithay/blob/master/src/backend/renderer/gles/shaders/implicit/texture.frag.

This is handy for testing. It may be better to have an enum of default shaders for supported features. But it would be nice for development, debugging, and creative applications to be able to set a shader path, or store the shader directly in cosmic-config.

It seems that GL shaders can be shared between EGL contexts, but state like uniforms is part of the program, so only one thread should be using it at a time. So it seems we need to compile the program once per render device, per kms surface thread.

Getting this to work in the X11 and Winit backends is not hugely important, but I still need to try doing that. Shouldn't be too hard.

I wanted to refactorSurfaceThreadState::redraw into a couple parts, though when I tried that earlier, a couple things I tried ran into lifetime issues (some of which seem to be borrow checker bugs). I may try something else, but this doesn't complicate that function too much more in this version.

Example uses

Invert color

We want to add an option for this in accessibility settings.

color.rgb = 1.0 - color.rgb;

Greyscale

float value = (color.r + color.g + color.b) / 3.0;
color = vec4(value, value, value, color.a);

More advanced methods of colorblindness simulation can be done with a process called "daltonization" based on the LMS color space, apparently. I haven't look into the exact formulate for different types of color blindness.

Color temperature

A hacky example just using an arbitrary hard-coded value from https://gitlab.com/chinstrap/gammastep/-/blob/master/src/colorramp.c:

color.rgb *= vec3(1.00000000,  0.86860704,  0.73688797);

Shader format

Instead of duplicating everything in smithay/backend/renderer/gles/shaders/implicit/texture.frag, we could accept shaders that are just a function mapping a color value to a color value. If that's all we need to support.

I guess DEBUG_FLAGS and tint shouldn't be used in a postprocess shader; only for normal rendering.

If we have multiple features that use postprocess shaders that should be possible to use simultaneously, we may need to combine them as one parameterized shader. Possibly we could also generate shader text chaining multiple separate functions in series. We definitely don't want to actually do multiple postprocess passes.

Some types of shaders (like color temperature) might benefit from a way to add custom uniforms. Not sure how we'd want to manage that.

This was mapping `UnderlyingStorage::Memory` to `None`. I don't see any
reason for this. Though this also shouldn't change behavior since
`TextureRenderElement` doesn't provide `underlying_storage` currently.
@ids1024
Copy link
Member Author

ids1024 commented Dec 13, 2024

I see this doesn't work properly with rotated outputs. Probably something that needs to be fixed with mirroring too.

@ids1024
Copy link
Member Author

ids1024 commented Dec 13, 2024

Per output shaders could also be a neat feature, but alas I can't think of a use case at the moment. So probably not worthwhile at the moment.

@Drakulix
Copy link
Member

Drakulix commented Dec 13, 2024

This renames MirroringState to OffscreenState, which is now used for mirroring (with a different size) and/or postprocessing.

+1 for the approach

Currently a shader can be set by manually putting a shader path in ~/.config/cosmic/com.system76.CosmicComp/v1/postprocess_shader_path. This can be modified from https://github.com/Smithay/smithay/blob/master/src/backend/renderer/gles/shaders/implicit/texture.frag.

This is handy for testing. It may be better to have an enum of default shaders for supported features. But it would be nice for development, debugging, and creative applications to be able to set a shader path, or store the shader directly in cosmic-config.

With this part I don't agree as much. First of all, yes we probably want for hard-coded shaders and an enum. I am fine with keeping custom shaders with a path as a config-file only option.

Regarding the shader, I don't think we want to allow a full-blown fragment shader, as this will be impossible to integrate with our future color pipeline.

Instead of duplicating everything in smithay/backend/renderer/gles/shaders/implicit/texture.frag, we could accept shaders that are just a function mapping a color value to a color value. If that's all we need to support.

That is what I wanted to suggest for now. We can always extend that, once we feel like our color pipeline is somewhat stable.

I guess DEBUG_FLAGS and tint shouldn't be used in a postprocess shader; only for normal rendering.

+1

We also should make sure we have some error handling, if said shader doesn't compile.

It seems that GL shaders can be shared between EGL contexts, but state like uniforms is part of the program, so only one thread should be using it at a time. So it seems we need to compile
the program once per render device, per kms surface thread.

Sounds like pain, but yeah makes sense.

Getting this to work in the X11 and Winit backends is not hugely important, but I still need to try doing that. Shouldn't be too hard.

+1

I wanted to refactorSurfaceThreadState::redraw into a couple parts, though when I tried that earlier, a couple things I tried ran into lifetime issues (some of which seem to be borrow checker bugs). I may try something else, but this doesn't complicate that function too much more in this version.

Yeah I ran into this multiple times as well. For now this is fine, but I guess we might run into more issue here, once we have a software rendered pipeline as well.

Some types of shaders (like color temperature) might benefit from a way to add custom uniforms. Not sure how we'd want to manage that.

Let's not do that for now. Color temperature will be a natively supported feature eventually anyhow and I would like to figure out how to do additional customization beyond the accessibility features we need after the color/hdr-rework.

@Quackdoc
Copy link
Contributor

creative applications to be able to set a shader path, or store the shader directly in cosmic-config.

Depending on the scope of this, I could see this being extremely useful for gaming enthusiasts, Especially retro ones would love to be able to use CRT shaders or custom scaling shaders at a compositor level. whether it be on a per app basis or for the entire compositor.

I can also see this being very useful for implementing custom tonemappers when color management/HDR support is added.

@ids1024
Copy link
Member Author

ids1024 commented Dec 13, 2024

Whatever exact format we use here, it should probably be understood that any support for custom shaders isn't a supported stable interface for the time being, and may change in the future.

I assume for CRT shaders you'd need to consider more than just a pure function applied to each color value. And if adjacent pixels have an impact that could affect damage tracking.

@Vixea
Copy link

Vixea commented Dec 13, 2024

Whoo, this would solve one of the reasons I don't main cosmic yet.. Colorblindness correction. Now all I have to figure out is the correct transform for all the different types.

@ids1024
Copy link
Member Author

ids1024 commented Dec 13, 2024

Colorblindness correction could also be a good feature to support officially. Windows and macOS have some version of that, apparently?

@ids1024
Copy link
Member Author

ids1024 commented Dec 13, 2024

We also should make sure we have some error handling, if said shader doesn't compile.

Error handling is a little weird here. For the kms backend, we the shader is compiled on each kms thread. It could send an error on a channel. But then there's the question of what happens if the shader compiles successfully on one GPU but not another. Currently it just logs errors where the shader is compiled, and doesn't try to propagate errors beyond there, and if a shader fails to compile it reverts to no shader.

I guess we probably want to fail if a shader doesn't compile on any GPU, or dynamically changing the render GPU would get weird. It definitely would be nicer if we could compile the programs once per GPU instead of separately for each thread...

ids1024 added a commit that referenced this pull request Dec 24, 2024
Apply inverse of output transform to mode to get render size, and apply
no transform during rendering. The transform of the output being
mirrored from shouldn't affect the final render.

Fixes issues when source output for mirroring has a transform, and also
fixes issues in #1058
when this code is used for postprocessing, where this resulted in the
same transform being applied twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants