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

Better blur texture sampling (in experimental backends) #428

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

tryone144
Copy link
Collaborator

Overview

Use correct wrap/repeat setting for blur textures to prevent darkening around the screen borders. This occurs due to sampling outside of the texture, especially noticeable with larger blur radii.
The required changes to the fragment shader allowed for further improvements by utilizing the linear interpolation hardware in a GPU in the glx backend.

Old behavior: blur_wrap_old
New behavior: blur_wrap_new

Changes

  1. Use correct REPEAT mode PAD when creating blur pictures in the xrender backend to pad the
    image with edge pixels.
  2. Use correct WRAP mode CLAMP_TO_EDGE when creating blur textures in the glx backend to clamp sampling to the edge pixels. Required changes to the fragment shader:
    • Use texture2D() instead of texelFetch(), as the latter does not respect WRAP settings.
    • Supply size of a pixel in normalized texture coordinates as new uniform to keep using pixel coordinates and offsets.
  3. Since we are now using texture2D() we can use the hardware accelerated linear interpolation on texture sampling to effectively sample two pixels with one texture2D() call. By precomputing the required offsets when creating the blur-shader we can reduce the number of texture accesses by half. This technique has been described in detail in this blog article.

This PR only fixes it for the new experimental backends. The legacy xrender backend has the same darkening problem while the glx backend seems to sample random junk. If wanted these may be fixed in a second PR.

Create texture with GL_LINEAR filtering and GL_CLAMP_TO_EDGE wrapping. Change
`texelFetch()`-call in fragement shader to `texture2D()` to be taken into
account. This requires supplying the size of a pixel in normalized texture
coordinates via an additional uniform.

Fixes darkening at the screen edges with larger blur radii caused by
sampling coordinates being out of texture bounds. This is undefined behaviour
unless the context has set the flag *GL_ARB_robust_buffer_access_behaviour*,
in which case "zero"-pixels are returned (i.e. black). Current behaviour
seems to depend on the driver.
Make use of hardware linear interpolation in a GPU to sample 2 pixels
with a single texture access inside the blur shaders by sampling between
both pixels based on their relative weight.

This is significantly easier for a single dimension as 2D bilinear
filtering would raise additional constraints on the kernels (not single
zero-entries, no zero-diagonals, ...) which require additional checks
with limited improvements. Therfore, only use interpolation along the
larger dimension should be a sufficient improvement.

Using this will effectively half the number of texture accesses and
additions needed for a kernel. E.g. a 1D-pass of the gaussian blur
with radius 15 will only need 16 samples instead of 31.
Create pictures used for bluring with REPEAT attribute set to PAD (same logic
as GL_CLAMP_TO_EDGE in OpenGL). Fixes darkening at the screen edges with
larger blur radii caused by sampling out of texture bounds.

Related: 4b0ff37
@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #428 into next will increase coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #428      +/-   ##
==========================================
+ Coverage   36.99%   37.09%   +0.09%     
==========================================
  Files          45       45              
  Lines        8572     8614      +42     
==========================================
+ Hits         3171     3195      +24     
- Misses       5401     5419      +18     
Impacted Files Coverage Δ
src/backend/gl/gl_common.c 0.00% <0.00%> (ø)
src/backend/gl/gl_common.h 0.00% <0.00%> (ø)
src/backend/xrender/xrender.c 0.00% <0.00%> (ø)
src/log.h 68.75% <0.00%> (-12.50%) ⬇️
src/common.h 30.76% <0.00%> (-7.70%) ⬇️
src/picom.c 68.08% <0.00%> (-0.29%) ⬇️
src/x.h 52.63% <0.00%> (ø)
src/win.c 64.74% <0.00%> (+0.10%) ⬆️
src/event.c 58.06% <0.00%> (+0.53%) ⬆️
src/utils.h 52.63% <0.00%> (+31.57%) ⬆️
... and 1 more

Comment on lines 1010 to 1012
int nele = width * height;
// '%.7g' is at most 14 characters, inserted 3 times
size_t body_len = (strlen(shader_add) + 42) * (uint)nele;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since one of the for loops will only run for ceil(width/2) or ceil(height/2) iterations, body_len will be too large. Being some bytes to large shouldn't be a problem. Being at least one to short however is bad.
If the buffer should be tighter, this may be computed depending on which loop will run with less iterations.

@yshui
Copy link
Owner

yshui commented Aug 4, 2020

@tryone144 Thanks for the PR. Great work as always!

Sorry I couldn't get back to you sooner, I will merge this now.

BTW, do you mind becoming a collaborator on picom?

@yshui yshui merged commit 6eb6a24 into yshui:next Aug 4, 2020
@vide0hanz
Copy link

Forgive my ignorance, but could someone give me the rundown of what exactly this includes?

I've been using @tryone144 's branch for dual-kawase blur on experimental backends for a while, does any of this correlate to this new merge?

Just anxiously awaiting until picom becomes all that everyone wants it to be!

@yshui
Copy link
Owner

yshui commented Aug 7, 2020

@vide0hanz This PR fix the problem where you would see dark edges around the screen when blur is enabled; as well as improve the performance of blur by using a clever trick to speed up the blur kernel.

This doesn't contribute directly to merging dual-kawase. But yes, I would like to merge that ASAP too.

@tryone144
Copy link
Collaborator Author

@vide0hanz These changes aren't directly related to the new updated blur code. But merging them first allows for a bit cleaner changes to the existing kernel-blur / feature-parity with the new dual-kawase code.

As yshui already summarized, this PR just fixes some visual "glitches" and improves performance of the box/gaussian blur.
The long awaited dual-filter kawase blur should be ready shortly with #382. 😉

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.

3 participants