Fix multimonitor blur
ClosedPublic

Authored by anemeth on May 3 2018, 10:45 AM.

Details

Summary

On wayland blur on secondary monitor would not render correctly.

BUG: 393723
Depends on D12452

Test Plan
  • use more than one output
  • log in in a wayland session
  • open a transparent window (for example: Konsole with transparent and blur enabled profile)
  • drag the window to another screen
  • blurs the content under the window corretly

Diff Detail

Repository
R108 KWin
Branch
multimonitor_blur (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth created this revision.May 3 2018, 10:45 AM
Restricted Application added a project: KWin. · View Herald TranscriptMay 3 2018, 10:45 AM
Restricted Application added subscribers: KWin, kwin. · View Herald Transcript
anemeth requested review of this revision.May 3 2018, 10:45 AM
anemeth edited the summary of this revision. (Show Details)May 3 2018, 10:49 AM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: KWin, graesslin.
davidedmundson added inline comments.
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:

  1. relative to global space
  2. relative to the screen
  3. in GL co-ordinates (i.e y flipped - and in theory scaled, but lets ingore that bit for now)

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.

anemeth added inline comments.May 3 2018, 12:18 PM
effects/blur/blur.cpp
620

I tried:

  • vertical and horizontal stacked displays
  • different positions, even intersecting
  • different resolutions
  • X11 and Wayland

All work perfectly.
Scaled blur under Wayland does not currently work, but I'm looking into it.

zzag added a subscriber: zzag.EditedMay 3 2018, 1:59 PM

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.

For math details, take a look at D12452 and D12466.

effects/blur/blur.cpp
617

Why QRegion?

638–639

Unrelated change.

anemeth updated this revision to Diff 33648.May 4 2018, 6:40 PM
  • Switch to blitting
anemeth updated this revision to Diff 33649.May 4 2018, 6:43 PM
  • Remove accidental whitespace 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.

In D12678#257695, @zzag wrote:

Not sure about that because you're flipping coords twice.

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.

zzag added a comment.May 4 2018, 8:21 PM

I don't quite understand. Is this still the case with the current state of the revision?

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.

zzag added a comment.May 7 2018, 10:07 PM
In D12678#258307, @zzag wrote:

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?

OK, nvm, I was wrong.

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.

zzag added a comment.EditedMay 7 2018, 11:50 PM

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.

davidedmundson added inline comments.May 17 2018, 9:55 AM
effects/blur/blur.cpp
616

to clarify in simpler terms.

I'm sure this needs to be
yTranslate = -screen.y();

can you please test with that.

zzag added a comment.May 17 2018, 10:13 AM

Could you please test @davidedmundson's requested change as following:

  • Make blur lighter
  • Put a window behind Kickoff

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.

zzag added a comment.EditedMay 17 2018, 12:18 PM

Are v-coords flipped somewhere else? [not related to the picture above]

zzag added a comment.May 17 2018, 12:46 PM

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.

zzag added a comment.EditedMay 17 2018, 1:01 PM

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.

apol added a subscriber: apol.May 22 2018, 4:42 PM

What's the status of this?

davidedmundson accepted this revision.May 22 2018, 10:37 PM
This revision is now accepted and ready to land.May 22 2018, 10:37 PM

This revision depends on D12452
@zzag could you please tell us the status of D12452? I see it's accepted, but not yet landed.

zzag added a comment.May 25 2018, 6:28 PM

This revision depends on D12452
@zzag could you please tell us the status of D12452? I see it's accepted, but not yet landed.

If you can push to the KWin repo, please land it [D12452]. (I don't have the Developer accoutn)

This revision was automatically updated to reflect the committed changes.
apol added a comment.Jun 8 2018, 4:44 PM

Shouldn't this go to to Plasma/5.13?

anemeth added a comment.EditedJun 8 2018, 4:51 PM
In D12678#275913, @apol wrote:

Shouldn't this go to to Plasma/5.13?

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.

This comment was removed by kpiwowarski.
effects/blur/blur.cpp
620