[scene] Fix decoration texture bleeding
ClosedPublic

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

Details

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
zzag added a comment.Nov 29 2019, 11:55 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.

zzag added a comment.Nov 29 2019, 5:25 PM

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:

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.

zzag updated this revision to Diff 70639.Nov 30 2019, 10:22 PM

Add the rotate() function back

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...

zzag updated this revision to Diff 70640.Nov 30 2019, 11:06 PM

Rename a variable

This revision was not accepted when it landed; it landed in state Needs Review.Dec 2 2019, 1:11 PM
This revision was automatically updated to reflect the committed changes.
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

zzag added a comment.Dec 2 2019, 2:24 PM

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.