Differential D25611

[scene] Fix decoration texture bleeding
ClosedPublic
Actions

Authored by zzag on Nov 29 2019, 11:27 AM.

Details

Reviewers

None

Group Reviewers

KWin

Commits

R108:ac4dce1c20a1: [scene] Fix decoration texture bleeding
R108:af71763be530: [scene] Fix decoration texture bleeding

Summary

Quite long time ago, window decorations were painted on real X11 windows.
The nicest thing about that approach is that we get both contents of the
client and the frame window at the same time. However, somewhere around
KDE 4.2 - 4.3 times, decoration rendering architecture had been changed
to what we have now.

I've mentioned the previous decoration rendering design because it didn't
have a problem that the new design has, namely the texture bleeding issue.

In the name of better performance, opengl scene puts all decoration parts
to an atlas. This is totally reasonable, however we must be super cautious
about things such as the GL_LINEAR filter.

The GL_LINEAR filter may need to sample a couple of neighboring texels
in order to produce the final texel value. However, since all decoration
parts now live in a single texture, we have to make sure that we don't
sample texels that belong to another decoration part.

This patch fixes the texture bleeding problem by padding each individual
decoration part in the atlas. There is another solution for this problem
though. We could render a window into an offscreen texture and then map
that texture on the transformed window geometry. This would work well and
we definitely need an offscreen rendering path in the opengl scene,
however it's not feasible at the moment since we need to break the window
quads API. Also, it would be great to have as less as possible stuff going
on between invocation of Scene::Window::performPaint() and getting the
corresponding pixel data on the screen.

There is a good chance that the new padding stuff may make you vomit. If
it does so, I'm all ears for the suggestions how to make the code more
nicer.

BUG: 257566
BUG: 360549
CCBUG: 412573
FIXED-IN: 5.18.0

Diff Detail

Repository

R108 KWin

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

zzag created this revision.Nov 29 2019, 11:27 AM

Restricted Application added a project: KWin. · View Herald TranscriptNov 29 2019, 11:27 AM

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

zzag requested review of this revision.Nov 29 2019, 11:27 AM

Harbormaster completed remote builds in B19272: Diff 70553.Nov 29 2019, 11:27 AM

zzag added a dependency: D25610: [x11] Name client pixmap instead of frame pixmap.Nov 29 2019, 11:34 AM

(before)

(after)

The decoration parts should already be padded. The commit message in the commit that introduced the atlas (6ad4c775d7840e64a07e27d6719a3ea7c3ee5eb8) even says so:
"The images are separated by a row of transparent texels to minimize artifacts from oversampling."

scene.cpp
888	That's the reason for the +1, +2 and +3 in the vertical positions here.

In D25611#569453, @fredrik wrote:

The decoration parts should already be padded. The commit message in the commit that introduced the atlas (6ad4c775d7840e64a07e27d6719a3ea7c3ee5eb8) even says so:
"The images are separated by a row of transparent texels to minimize artifacts from oversampling."

Yes, it does and that's the problem since the transparent texels contribute to the final output value. Am I missing something?

zzag edited the summary of this revision. (Show Details)Nov 29 2019, 6:17 PM

In D25611#569457, @zzag wrote:

In D25611#569453, @fredrik wrote:

The decoration parts should already be padded. The commit message in the commit that introduced the atlas (6ad4c775d7840e64a07e27d6719a3ea7c3ee5eb8) even says so:
"The images are separated by a row of transparent texels to minimize artifacts from oversampling."

Yes, it does and that's the problem since the transparent texels contribute to the final output value. Am I missing something?

Actually I misunderstood what the patch does. I thought it added transparent padding.

plugins/scenes/opengl/scene_opengl.cpp
2639	How does this perform compared to the rotate() function?

zzag added inline comments.Nov 30 2019, 7:56 PM

plugins/scenes/opengl/scene_opengl.cpp
2639	Both QPainter::rotate() and QPainter::translate() operate on the world transform matrix. Hence, performance should be the same. Would you prefer to use the rotate() function instead?

zzag added inline comments.Nov 30 2019, 10:20 PM

plugins/scenes/opengl/scene_opengl.cpp
2639	It seems like I misunderstood your question. To be honest, I didn't do benchmarks. I saw a todo comment and decided to fix it. I will revert this change.

Add the rotate() function back

Harbormaster completed remote builds in B19318: Diff 70639.Nov 30 2019, 10:22 PM

zzag added inline comments.Nov 30 2019, 10:35 PM

plugins/scenes/opengl/scene_opengl.cpp
2639	Back to the question: I expect performance to be the same if the decoration theme fills borders with some solid color. If it layers multiple gradients, then I expect performance to go down due to cache misses. But the only way to find whether that's true is to do lots of benchmarks...

Rename a variable

Harbormaster completed remote builds in B19319: Diff 70640.Nov 30 2019, 11:06 PM

This revision was not accepted when it landed; it landed in state Needs Review.Dec 2 2019, 1:11 PM

Closed by commit R108:ac4dce1c20a1: [scene] Fix decoration texture bleeding (authored by zzag). · Explain Why

This revision was automatically updated to reflect the committed changes.

davidedmundson added a subscriber: davidedmundson.Dec 2 2019, 1:27 PM

davidedmundson added inline comments.

plugins/scenes/opengl/scene_opengl.cpp
2625	QRect point should also be *= devicePixelRatio?
2670	We rotate the vertical bars so everything is horizontal, so in theory we could: GL_CLAMP_S=GL_CLAMP_TO_EDGE and only pad T. Though maybe that's just making things more complicated...

There is a good chance that the new padding stuff may make you vomit. If

it does so, I'm all ears for the suggestions how to make the code more
nicer.

The QSG code looks quite simple, but they go via another QImage buffer.

QPainter p(&tmp);
p.setCompositionMode(QPainter::CompositionMode_Source);

int w = r.width(); //Dave <- image.width+padding*2
int h = r.height();
int iw = image.width();
int ih = image.height();
p.drawImage(1, 1, image);
p.drawImage(1, 0, image, 0, 0, iw, 1);
p.drawImage(1, h - 1, image, 0, ih - 1, iw, 1);
p.drawImage(0, 1, image, 0, 0, 1, ih);
p.drawImage(w - 1, 1, image, iw - 1, 0, 1, ih);
p.drawImage(0, 0, image, 0, 0, 1, 1);
p.drawImage(0, h - 1, image, 0, ih - 1, 1, 1);
p.drawImage(w - 1, 0, image, iw - 1, 0, 1, 1);
p.drawImage(w - 1, h - 1, image, iw - 1, ih - 1, 1, 1);

Assuming Qt copes with rendering into the image it reads from we could have done the same.
We have the extra complexity of partial updates, but if we have the clipRect set correctly that should just quietly no-op

In D25611#570783, @davidedmundson wrote:
The QSG code looks quite simple, but they go via another QImage buffer.
QPainter p(&tmp);
p.setCompositionMode(QPainter::CompositionMode_Source);
 
int w = r.width(); //Dave <- image.width+padding*2
int h = r.height();
int iw = image.width();
int ih = image.height();
p.drawImage(1, 1, image);
p.drawImage(1, 0, image, 0, 0, iw, 1);
p.drawImage(1, h - 1, image, 0, ih - 1, iw, 1);
p.drawImage(0, 1, image, 0, 0, 1, ih);
p.drawImage(w - 1, 1, image, iw - 1, 0, 1, ih);
p.drawImage(0, 0, image, 0, 0, 1, 1);
p.drawImage(0, h - 1, image, 0, ih - 1, 1, 1);
p.drawImage(w - 1, 0, image, iw - 1, 0, 1, 1);
p.drawImage(w - 1, h - 1, image, iw - 1, ih - 1, 1, 1);
Assuming Qt copes with rendering into the image it reads from we could have done the same.
We have the extra complexity of partial updates, but if we have the clipRect set correctly that should just quietly no-op

That's definitely an option, I'll see what I can do.

plugins/scenes/opengl/scene_opengl.cpp
2625	No, since it must be in device-independent pixels.
2670	This will be the case only if all decoration parts have the same width.

Revision Contents
Changeset List

			Path	Packages
M			decorations/decorationrenderer.h (1 line)
M			decorations/decorationrenderer.cpp (7 lines)
M			plugins/scenes/opengl/scene_opengl.cpp (117 lines)
M			scene.cpp (15 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	70553	29bfdde		Nov 29 2019, 11:27 AM	★	★
Diff 2	70639	960e76e	Add the rotate() function back	Nov 30 2019, 10:22 PM	★	★
Diff 3	70640	960e76e	Rename a variable	Nov 30 2019, 11:06 PM	★	★
Diff 4	70743	754b72d	R108:ac4dce1c20a1cbfd3974bf8da6dd7557d35a4ca1	Dec 2 2019, 1:08 PM	★	★