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

Rounded corners for legacy glx backend #558

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Conversation

yshui
Copy link
Owner

@yshui yshui commented Dec 8, 2020

Authored-by: bhagwan bhagwan@disroot.org
Authored-by: Samuel Hand samuel.d.hand@gmail.com

@yshui yshui requested a review from tryone144 December 8, 2020 07:21
@yshui yshui force-pushed the rounded-corners-legacy-glx2 branch 2 times, most recently from c23f058 to 469bc39 Compare December 8, 2020 07:32
@yshui
Copy link
Owner Author

yshui commented Dec 8, 2020

Implementation is a bit messy, but it's for the legacy backend so I don't really mind.

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #558 (0f5f013) into next (3c38b36) will decrease coverage by 0.64%.
The diff coverage is 1.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #558      +/-   ##
==========================================
- Coverage   38.00%   37.36%   -0.65%     
==========================================
  Files          46       46              
  Lines        9203     9364     +161     
==========================================
+ Hits         3498     3499       +1     
- Misses       5705     5865     +160     
Impacted Files Coverage Δ
src/opengl.c 0.00% <0.00%> (ø)
src/options.c 21.53% <0.00%> (+0.06%) ⬆️
src/render.c 4.14% <0.00%> (-0.11%) ⬇️
src/win.h 78.12% <ø> (ø)
src/opengl.h 60.46% <100.00%> (+2.32%) ⬆️

Copy link
Collaborator

@tryone144 tryone144 left a comment

Choose a reason for hiding this comment

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

Implementation is a bit messy

I can certainly see what you mean…
The back-buffer caching can be simplified, there is a second round_corner routine that isn't used, a second shader that looks like a leftover from debugging, and some unneccesary stuff left over from copying the blur code.

With these changes, the xrender backend is broken due to missing checks for the glx backend before calling glx_bind_texture() and glx_round_corners_dst0() in paint_all().

Furthermore, shadow clipping isn't working as intended. The shadow is just clipped by the rough bounding shape that excludes the corners. This leads to the shadow still being drawn behind the rounded corners. Adding the inverse clipping logic to the window shader (analogous to the one used by glx_round_corners_dst0()) could work, but I have to take a closer look at how the shader works first to say for sure.
And while we are at it, we could skip the creation of the respective clip image (render.c line 746) for the xrender backend when using the glx backend.

src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/render.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
@yshui
Copy link
Owner Author

yshui commented Dec 11, 2020

@tryone144 thanks for the review. I need some time to address all of these...

@ibhagwan
Copy link

Hi @yshui, @tryone144,

Not sure why I only got the notification email about this today, I saw you managed to answer some of the questions yourselves.

The dst0/dst1, are simply copies I used for experimentation with minor differences, I ended up using just one of the, and when I rebased the code on @sdhand’s repo I believe I only left one of those.

Let me know what other help you need and I’ll do my best to respond ASAP, I’ll also read this thread today carefully and see if there are any open questions.

@yshui yshui force-pushed the rounded-corners-legacy-glx2 branch 4 times, most recently from 23444bd to d83c8dc Compare December 15, 2020 18:08
@yshui
Copy link
Owner Author

yshui commented Dec 15, 2020

@tryone144 I think I've addressed everything, can you have a look again?

@yshui yshui force-pushed the rounded-corners-legacy-glx2 branch from d83c8dc to d4a2b36 Compare December 15, 2020 18:15
@yshui
Copy link
Owner Author

yshui commented Dec 15, 2020

Also glx_render doesn't need the cr parameter. It was used to decide whether to enable GL_BLEND in glx_render. But the gl implementation of rounded corners doesn't depend on that.

Hmm, not sure if this is true. Yep, it's true.

Copy link
Collaborator

@tryone144 tryone144 left a comment

Choose a reason for hiding this comment

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

The opengl code is definitely in better shape now. Thanks for fixing. These are just some minor things I would change before merging.

And while we are at it, we could skip the creation of the respective clip image (render.c line 746) for the xrender backend when using the glx backend.

Did you take a look into this?

Also glx_render doesn't need the cr parameter. It was used to decide whether to enable GL_BLEND in glx_render. But the gl implementation of rounded corners doesn't depend on that.

I think the cr parameter would have been useful when glx_render() would actually clip the shadow image for rounded windows if we have full-shadow = false. But this is currently not the case as mentioned before.

src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
src/opengl.c Show resolved Hide resolved
src/opengl.h Outdated Show resolved Hide resolved
src/opengl.c Outdated Show resolved Hide resolved
@yshui
Copy link
Owner Author

yshui commented Dec 16, 2020

@tryone144 I don't really want to put to much effort into clipping the shadow properly here. I'll make sure it works properly for the new backends.

Authored-by: bhagwan <bhagwan@disroot.org>
Authored-by: Samuel Hand <samuel.d.hand@gmail.com>
@yshui yshui force-pushed the rounded-corners-legacy-glx2 branch from 4f1052f to 0f5f013 Compare December 16, 2020 18:10
@yshui yshui merged commit 79f1d4e into next Dec 16, 2020
@yshui yshui deleted the rounded-corners-legacy-glx2 branch December 16, 2020 21:22
@SolsticeSpectrum
Copy link

Have you figured out the memory leaks yshui? If so I would switch from tryone to your repo.

@yshui
Copy link
Owner Author

yshui commented Dec 18, 2020

@DarkReaper231 what memory leak?

@SolsticeSpectrum
Copy link

SolsticeSpectrum commented Dec 18, 2020

Wait no I messed up. It is not tryone but ibhagwan. Anyway ibgagwan's picom has memory leak when using rounded corners and if I recall correctly he said that corners will be in your repo. So I am wondering if the issue was fixed https://github.com/ibhagwan/picom/issues/8 also mentioned here https://github.com/ibhagwan/picom/issues/21

I am using rounded corners a lot so it would be nice if this was fixed at least in one repo.

@ibhagwan
Copy link

@DarkReaper231 the code is already merged in next so there’s only one way to find out :)

@SolsticeSpectrum
Copy link

Oh it is fixed. At least I think it is. Is it? It seems to be. I am not sure.

@SolsticeSpectrum
Copy link

Okay so today it happened. There is still slight memory leak. It is waaay better but it can still cause slow down overtime. Not as much as before but it can still be noticable.

@yshui
Copy link
Owner Author

yshui commented Dec 18, 2020

@DarkReaper231 can you get a trace of picom with apitrace. I am assuming that picom isn't actually showing a lot of memory usage, but the entire system has high memory usage? And you are using the glx backend?

@tryone144 tryone144 mentioned this pull request Jul 17, 2021
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