On wayland blur on secondary monitor would not render correctly.
BUG: 393723
Depends on D12452
graesslin | |
davidedmundson |
KWin |
On wayland blur on secondary monitor would not render correctly.
BUG: 393723
Depends on D12452
No Linters Available |
No Unit Test Coverage |
effects/blur/blur.cpp | ||
---|---|---|
620 | Please test vertically stacked monitors. I'm 90% sure we now have a bug here. We have 3 possible co-ordinate systems:
it makes sense to move expandedBlurRegion from 1 to 2, but your y translate looks like you're moving it from 1 to 3 which is problematic as glCopyTexSubImage2D flips y again. |
effects/blur/blur.cpp | ||
---|---|---|
620 | I tried:
All work perfectly. |
All work perfectly.
Not sure about that because you're flipping coords twice.
Also, could you please simplify calculations? Right now, they're overcomplicated without any good reason. Maybe, it's better to calculate copyRect in the "virtual space" and then map it to the OpenGL screen space when you're doing copy.
Yet another also, why glCopyTexSubImage2D passes xoffset and yoffset? I ask here because git blame says you did that change.
effects/blur/blur.cpp | ||
---|---|---|
617 | Why QRegion? | |
638–639 | Unrelated change. |
I switched to blitFromFramebuffer() because it simplifies the code and also enabled me to solve the scaling issue on Wayland much easier in D12700.
I don't quite understand. Is this still the case with the current state of the revision?
Yet another also, why glCopyTexSubImage2D passes xoffset and yoffset? I ask here because git blame says you did that change.
That was because unlike the old method where the blur area was copied in a texture that was the size of that area, we now copy it into a screen sized texture to the same position where it is on the screen.
Yes, it's still the case. blitFromFramebuffer will flip for you y-coords. Also, please notice that source and destination rect in blitFromFramebuffer live in two different spaces.
I suggest you to calculate copyRect in the virtual space, don't do manual "Virtual Space" -> "Screen Space" transformations. @davidedmundson what do you think?
That was because unlike the old method where the blur area was copied in a texture that was the size of that area, we now copy it into a screen sized texture to the same position where it is on the screen.
Why? I still don't get it.
it makes sense to move expandedBlurRegion from 1 to 2, but your y translate looks like you're moving it from 1 to 3 which is problematic as glCopyTexSubImage2D flips y again.
@davidedmundson could you explain this to me please?
what do you mean by moving expandedBlurRegion 1 to 2 and 1 to 3?
is this still the case?
I tested all kinds of setups and it works.
OK, nvm my nvm, that's really hard to keep track of all things in my head. Sorry for that. sourceRect lives in 1(relative to global space), which is fine. destRect lives in 3(in GL co-ordinates), which is wrong (roughly speaking, it should be in 2).
So, why does it work anyway? My guess is that you flip v-coords somewhere else.
effects/blur/blur.cpp | ||
---|---|---|
616 | to clarify in simpler terms. I'm sure this needs to be can you please test with that. |
Could you please test @davidedmundson's requested change as following:
e.g. it should look something like this
@davidedmundson @zzag
I tested yTranslate = -screen.y(); with kwin_wayland --windowed --xwayland --output-count 2
Once there is a height difference between the displays it doesn't work.
I've just noticed that destRect is not scaled. blitFromFramebuffer scales source rect, but not destination rect.
I've just noticed that destRect is not scaled. blitFromFramebuffer scales source rect, but not destination rect.
That's fine. In wayland only the GL co-ordinates are in device pixels. The screen space is still in logical pixels.
Then the destination rectangle should be also scaled. Otherwise, the copied image is downscaled. Or I'm still missing something?
UPDATE: All right, because the blur effect is not continuous w.r.t. to "blur radius", it's not possible to smoothly increase the sampling rate. Also, I'm not sure whether it has similar behaviour to Gaussian blur. So, yeah, that's fine to have a downscaled "scratch" texture.
If you can push to the KWin repo, please land it [D12452]. (I don't have the Developer accoutn)
Whoops, I didn't check if there was already a 5.13 branch before pushing.
How should I handle this? Do I just push these to that branch as well?
At this point, we need to cherry-pick from master into 5.13.
Do *not* merge master into 5.13.
At this point commits will be end up in the release 5.13.1, but that's fine.
Ping if I can help.
effects/blur/blur.cpp | ||
---|---|---|
620 | There is bug on Wayland. https://bugs.kde.org/show_bug.cgi?id=396055 |