[libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in GLRenderTarget::blitFromFramebuffer
ClosedPublic
Actions

Authored by zzag on Apr 22 2018, 4:11 PM.

Details

Reviewers

fredrik
davidedmundson

Group Reviewers

KWin

Commits

R108:982316072d5f: [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in…
R108:8342cdd3b6d9: [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in…

Summary

There are several spaces that have to be considered in GLRenderTarget::blitFromFramebuffer:

KWin logical space: the origin is located at the global top-left corner
display space: the origin is located at the top-left corner of monitor/display
OpenGL screen space: the origin is located at the bottom-left corner of monitor/display

Given s, which is in the KWin logical space, we have to transform it to the display space, then to the OpenGL screen space:

KWin logical space -> display space: y' = s.y() - s_virtualScreenGeometry.y()
display space -> OpenGL screen space: y'' = s_virtualScreenGeometry.height() - y'

Overall, srcY0 and srcY1 should be written as follows:

srcY0 = s_virtualScreenGeometry.height() - (s.y() - s_virtualScreenGeometry.y() + s.height())
srcY1 = s_virtualScreenGeometry.height() - (s.y() - s_virtualScreenGeometry.y())

Test Plan

Tweak background contrast effect to use GLRenderTarget::blitFromFramebuffer

diff --git a/effects/backgroundcontrast/contrast.cpp b/effects/backgroundcontrast/contrast.cpp
index f920fcd88..5247d83b8 100644
--- a/effects/backgroundcontrast/contrast.cpp
+++ b/effects/backgroundcontrast/contrast.cpp
@@ -447,11 +447,10 @@ void ContrastEffect::doContrast(EffectWindow *w, const QRegion& shape, const QRe
     GLTexture scratch(GL_RGBA8, r.width() * scale, r.height() * scale);
     scratch.setFilter(GL_LINEAR);
     scratch.setWrapMode(GL_CLAMP_TO_EDGE);
-    scratch.bind();

-    const QRect sg = GLRenderTarget::virtualScreenGeometry();
-    glCopyTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, (r.x() - sg.x()) * scale, (sg.height() - sg.y() - r.y() - r.height()) * scale,
-                        scratch.width(), scratch.height());
+    GLRenderTarget scratchTarget(scratch);
+    scratchTarget.blitFromFramebuffer(r);
+    scratch.bind();

     // Draw the texture on the offscreen framebuffer object, while blurring it horizontally

GLRenderTarget::blitFromFramebuffer without this change:

Diff Detail

Repository

R108 KWin

Branch

kwinglutils-fix-blitfromframebuffer

Lint

No Linters Available

Unit

No Unit Test Coverage

zzag created this revision.Apr 22 2018, 4:11 PM

Restricted Application added a project: KWin. · View Herald TranscriptApr 22 2018, 4:11 PM

Restricted Application added a subscriber: kwin. · View Herald Transcript

zzag requested review of this revision.Apr 22 2018, 4:11 PM

zzag edited the test plan for this revision. (Show Details)Apr 22 2018, 4:12 PM

zzag added a reviewer: fredrik.Apr 22 2018, 4:15 PM

zzag retitled this revision from [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 to [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in GLRenderTarget::blitFromFramebuffer.

Looks good to me, but I'd also check with @davidedmundson.

Does this fix bug 391387?

No, it fixes only ::blitFromFramebuffer.

For reference, I use multisampling patches, so this is how Kickoff looks on my system

I haven't seen any artifacts like those from 391387.

And this is how Kickoff looks when screen is scaled (on X11)

FWIW I use the new blur effect.

This line was modded with 9cafbb117984f746d4b71213effc3d27e1435f36
That also claimed to be a bug fix that fixing things, and that one was tested with vertically stacked multiscreens.

As this code is super confusing I'll explain the function in words.

converting a global x,y to be relative within the screen
converting from y==0 is top to y==0 is bottom

So we want:

relativeY = s.y - virtualScreenGeometry.y (with one of the args also having + s.height)
final = virtualScreenGeometry.height - relativeY

On X kwin doesn't do any scaling s_virtualScreenScale will always be 1.

In D12452#251956, @davidedmundson wrote:

On X kwin doesn't do any scaling s_virtualScreenScale will always be 1.

Yeah, I just tested on Wayland. Are http://blog.davidedmundson.co.uk/blog/kwin_and_scaling/ and 391387 related?

In D12452#251952, @davidedmundson wrote:

This line was modded with 9cafbb117984f746d4b71213effc3d27e1435f36
That also claimed to be a bug fix that fixing things, and that one was tested with vertically stacked multiscreens.

As this code is super confusing I'll explain the function in words.

converting a global x,y to be relative within the screen
converting from y==0 is top to y==0 is bottom

So we want:

relativeY = s.y - virtualScreenGeometry.y (with one of the args also having + s.height)
final = virtualScreenGeometry.height - relativeY

You didn't have to ELI5 OpenGL screen coordinates.

And take a look at derivation steps, I did exactly what you wrote. ;-)

Also, I'd like to make it clear that this change only to fix srcY{0/1} in GLRenderTarget::blitFromFramebuffer(), not about 391387.

In the sense that they both involve scaling, yes. But otherwise not really. See my patch that first fixed it. But as you say this is very unrelated. Blur doesn't even use this function.

As for this patch: It does indeed look right (though frankly I'd keep it to be like your derivation so it's at least somewhat slightly readable).
But then it should have failed when the previous person did that screenshot test when they modded it. I can test on a vertically stacked monitor next week.

It seems like 391387 is caused by blur.

Background contrast + GLRenderTarget::blitFromFramebuffer works fine.

I'll test this change on a multi-screen setup today or tomorrow.

zzag added a comment.Apr 22 2018, 6:18 PM

This comment was removed by zzag.

I can test your change on my multiscreen setup. 9cafbb117984f746d4b71213effc3d27e1435f36 fixes for me making screenshots on Wayland session, but there was still problem with blur effect on X session - https://bugs.kde.org/show_bug.cgi?id=389658

In D12452#251997, @kpiwowarski wrote:

I can test your change on my multiscreen setup. 9cafbb117984f746d4b71213effc3d27e1435f36 fixes for me making screenshots on Wayland session, but there was still problem with blur effect on X session - https://bugs.kde.org/show_bug.cgi?id=389658

I would be really grateful if you could test this on Wayland. (it takes a while for me to package kwin)

I tested on X11, so far no problems:

Everything is okay behind Kickoff.

Screenshot of the screen #1

Screenshot of the screen #2

Tested on Wayland, everything works as expected.

Also, I encountered a bug in plasma shell: if I scale both 1366x768 monitors, plasma shell crashes on startup.

In D12452#251978, @davidedmundson wrote:

But then it should have failed when the previous person did that screenshot test when they modded it. I can test on a vertically stacked monitor next week.

I think that screen geometries were passed to blitFromFramebuffer so most terms cancel each other and we get "correct" results, e.g.

// s_virtualScreenGeometry = QRect(0, 0, 1366, 768)
// s = QRect(0, 0, 1366, 768)

srcX0 = s.x() - s_virtualScreenGeometry.x()
      = 0 - 0
      = 0 // OK

srcY0 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y() - s.height()
      = 768 - 0 + 0 - 768
      = 0 // OK

srcX1 = s.x() - s_virtualScreenGeometry.x() + s.width()
      = 0 - 0 + 1366
      = 1366 // OK

srcY1 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y()
      = 768 - 0 + 0
      = 768 // OK

// First screen: QSize(1366, 768)
// Second screen: QSize(1366, 1000)
// 
// s_virtualScreenGeometry = QRect(0, 1366, 1768)

// s = QRect(0, 0, 1366, 768)
srcX0 = s.x() - s_virtualScreenGeometry.x()
      = 0 - 0
      = 0 // OK

srcY0 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y() - s.height()
      = 1768 - 0 + 0 - 768
      = 1000 // OK

srcX1 = s.x() - s_virtualScreenGeometry.x() + s.width()
      = 0 - 0 + 1366
      = 1366 // OK

srcY1 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y()
      = 1768 - 0 + 0
      = 1768 // OK

// s = QRect(0, 768, 1368, 1000)
srcX0 = s.x() - s_virtualScreenGeometry.x()
      = 0 - 0
      = 0 // OK

srcY0 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y() - s.height()
      = 1768 - 0 + 768 - 1000
      = 0 // OK

srcX1 = s.x() - s_virtualScreenGeometry.x() + s.width()
      = 0 - 0 + 1366
      = 1366 // OK

srcY1 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y()
      = 1768 - 0 + 768
      = 1000 // OK

But if we pass anything else but screen geometry, we'll get totally wrong results

// s_virtualScreenGeometry = QRect(0, 0, 1366, 768)
// s = QRect(0, 758, 1366, 10)

srcX0 = s.x() - s_virtualScreenGeometry.x()
      = 0 - 0
      = 0 // OK

srcY0 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y() - s.height()
      = 768 - 0 + 758 - 10
      = 1516 // :'(

srcX1 = s.x() - s_virtualScreenGeometry.x() + s.width()
      = 0 - 0 + 1366
      = 1366 // OK

srcY1 = s_virtualScreenGeometry.height() - s_virtualScreenGeometry.y() + s.y()
      = 768 - 0 + 758
      = 1536 // :'(

zzag mentioned this in D12466: [effects/backgroundcontrast] Calculate correct y-coord of the copy rect.Apr 23 2018, 10:53 AM

zzag mentioned this in D12678: Fix multimonitor blur.May 3 2018, 1:59 PM

anemeth added a dependent revision: D12678: Fix multimonitor blur.May 4 2018, 6:45 PM

davidedmundson mentioned this in R108:309a77eedfed: [effects/backgroundcontrast] Calculate correct y-coord of the copy rect.May 17 2018, 9:52 AM

I would prefer it written in the more readable format that you use in your derviations.

This revision is now accepted and ready to land.May 22 2018, 6:21 PM

In D12452#266593, @davidedmundson wrote:

I would prefer it written in the more readable format that you use in your derviations.

Sure! I'll update this diff, just don't land yet.

Also, I can make it slightly readable as here https://github.com/zzag/kwin/blob/zzag/scene-opengl-multisampling/libkwineffects/kwinglutils.cpp#L1338

zzag edited the summary of this revision. (Show Details)May 22 2018, 11:17 PM

zzag edited the test plan for this revision. (Show Details)

Don't obfuscate math

zzag edited the summary of this revision. (Show Details)May 22 2018, 11:29 PM

zzag edited the summary of this revision. (Show Details)

Closed by commit R108:8342cdd3b6d9: [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in… (authored by anemeth). · Explain WhyMay 29 2018, 1:27 PM

This revision was automatically updated to reflect the committed changes.

Authorship information is a bit off.

Did you land with arc land?

Revision Contents
Changeset List

			Path	Packages
M			libkwineffects/kwinglutils.cpp (4 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	32824	ddda524		Apr 22 2018, 4:11 PM	★	★
Diff 2	34676	267b5a1	Don't obfuscate math	May 22 2018, 11:18 PM	★	★
Diff 3	35112	393af85	R108:8342cdd3b6d97ee3f12836cdcbf63ee92134bd42	May 29 2018, 1:27 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
0512e2d5a319	c97470d37fd3	267b5a11222e	Vlad Zagorodniy	[libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in… (Show More…)	May 22 2018, 10:45 PM

Status	Author	Revision
Closed	anemeth	D12700 Fix blur on Wayland when scaling is used
Closed	anemeth	D12678 Fix multimonitor blur
Closed	zzag	D12452 [libkwineffects/kwinglutils] Calculate correct srcY0 and srcY1 in GLRenderTarget::blitFromFramebuffer

Diff 34676

View Options

libkwineffects/kwinglutils.cpp