Created attachment 110790 [details]
Left edge of google chrome is very jaggy

Created attachment 110791 [details]
Top & bottom edges of windows in flip switch are jaggy too

Created attachment 110792 [details]
the same with cover switch(also it has problems with window decorations)

Back when I started working on KWin and introduced the first 3d effects I also observed this problem and tried to fix it. It failed in a horrible way: the fonts got affected by it. The jigsaw is less an issue than what antialising does to fonts.

 On the other hand that is ten years ago and gpus changed. We have shaders instead of fixed functionality. So maybe today it is possible.

(In reply to Martin Flöser from comment #4)
> Back when I started working on KWin and introduced the first 3d effects I
> also observed this problem and tried to fix it. It failed in a horrible way:
> the fonts got affected by it. The jigsaw is less an issue than what
> antialising does to fonts.

Yes, that's why I'm proposing to enable multisampling only for windows which really need this. For example, do we really need pixel perfect text in flip switch? (maybe that question is dumb because I haven't seen how much text is blurred with multisampling)

>  On the other hand that is ten years ago and gpus changed. We have shaders
> instead of fixed functionality. So maybe today it is possible.

back in the days with flip switch one would need to enable multisampling for the front window at least. And that's the most prominent one. Imagine the browser window with all text blurred.

But in the end we need to test how that looks like with modern hardware. Back in the days all we had was GL_POLYGON_SMOOTH.

I think this issue should be revised. Windows Vista and Windows 7 had smooth(as far as I remember) edges in the Flip 3D effect a long time ago.

----------

Offtopic: Martin, did you give any talks or have articles about KWin architecture? I wish I could help somehow but my current level of knowledge of KWin is slightly low.

Created attachment 110801 [details]
Comparison of different cases with/without MSAA

Top row: without MSAA
Bottom row: with MSAA
Left column: without shadows
Right column: with shadows

The number of samples: 4

Surprisingly, text on the MSAA sample looks pretty good. (maybe I did something wrong)

Created attachment 110803 [details]
There is still present aliasing

This is not a silver bullet. There is still little aliasing.
+ I think it would have "slight" impact on performance.
+ Is it possible to force MSAA only for a specific window?

Another way would be to render a window into offscreen texture, extend it a little bit(do nothing if it has shadow), set linear filter and render it.

(In reply to Vlad Zagorodniy from comment #7)
> Offtopic: Martin, did you give any talks or have articles about KWin
> architecture? I wish I could help somehow but my current level of knowledge
> of KWin is slightly low.

I did write one blog post about the effect rendering pipeline years ago. I fear it hardly matches reality any more. The general design of the compositor is described in scene.cpp, but I fear it's too high level to be of much help.

(In reply to Martin Flöser from comment #10)
> The general design of the compositor is described in scene.cpp,
> but I fear it's too high level to be of much help.

Thank you for pointing out. After digging the source code,
I have basic understanding how KWin works:

<-- the part with ApplicationX11/ApplicationWayland is omitted -->

- Application creates an instance of Workspace

- Workspace creates a bunch of objects, setups different connections,
  starts listening for new clients, etc.
  One of those objects it creates is Compositor

- Because Compositor is constructed in the constructor of Workspace
  class, it delays its "full initialization"(Workspace::self() is
  undefined). Compositor initializaton code does several things:
    * picks scene plugin to use
    * chooses refresh rate
    * creates EffectsHandlerImpl(not really, it is provided by a platform)
    * schedules performCompositing to be called every fpsInterval(not really,
      it can be adjusted)

<-- PERFORM_PAINTING -->

performCompositing does several things:
    * first, it makes a copy of the stacking order
    * moves elevated windows to the top of the copied stacking order 
    * calls Scene::paint()
    * makes bookkeeping stuff
    * schedules itself

- Scene::paint() is implemented by a scene plugin(scene plugins live inside of
  plugins/scenes/ directory) and it is supposed to call paintScreen.

paintScreen in turn:
    * gets the time diff between the last frame and the current frame
    * sets painting region
    * calls EffectsHandler::paintScreen
    <-- at this point screen is painted -->
    * calls EffectsHandler::postPaintWindow for each window

Effects in KWin are implemented as a chain. So, by calling EffectHandler::paintScreen
we "activate" the first effect, that one would activate the second, and so on. In the
end, finalPaintScreen is called which would call either paintGenericScreen or 
paintSimpleScreen(it is optimized). paintGenericScreen and paintSimpleScreen share
some logic:
     for each window w in the stacking order
         - call prePaintWindow

     call EffectsHandler::paintWindow if w should be painted

     the effect chain is traversed and at some point Scene::finalPaintWindow() is called

     Scene::finalPaintWindow calls EffectsHander::drawWindow

     it traverses the effect chain, again; in the end, Scene::finalDrawWindow is called

     Scene::finalDrawWindow calls Scene::Window::performPaint

- Scene::Window::performPaint is implemented by a scene plugin and does actual painting.

<-- compositeTimer ticked, go to PERFORM_PAINTING -->

I have also been thinking about this problem, and here is how I would solve it:

Multisample rendering is enabled by effects in prePaintScreen() by setting a new PAINT_SCREEN_MULTISAMPLE flag. When this flag is set, the scene creates a multisample render target and a pushes it to the render target stack in finalPaintScreen(). At the end of painting, the scene resolves the render target by blitting it to the backbuffer.

When a window has a complex transformation, it is first rendered untransformed on the base level of an offscreen mipmap texture. Miplevels > 0 are then generated - preferably using a higher quality filter than the default. The texture is then drawn on the framebuffer using an anisotropic texture filter. To draw the texture, a new quad type is needed - WindowQuadCombined. This quad type is effectively the union of the shadow, decoration and content quads. This is the only quad type that should be transformed by effects such as wobbly windows.

This new quad type and method of rendering is coincidentally also what is needed to make wayland subsurfaces conformant.

Turning on fullscreen multisampling makes sense with effects like Cube, Cover, etc.

What about effects which transform individual windows? There could be a short time when all fonts look blurry.

MSAA doesn't blur anything. It works by computing a pixel coverage value in the range [0..1] for each rasterized fragment, and multiplying that value with the alpha value prior to blending.

You may be confusing it with post-processing anti-aliasing techniques, such as FXAA.

(In reply to Fredrik Höglund from comment #14)
> MSAA doesn't blur anything. It works by computing a pixel coverage value in
> the range [0..1] for each rasterized fragment, and multiplying that value
> with the alpha value prior to blending.
> 
> You may be confusing it with post-processing anti-aliasing techniques, such
> as FXAA.

Oh, yeah, you're right. I was thinking MSAA smooths also everything inside textures. My bad, sorry.

It seems like GLRenderTarget should be refactored in order to use render buffers. I suggest to use multisampled render buffers because they come from OpenGL 3.0 while multisampled textures are available only from OpenGL 3.2 onwards also render buffers fit better for this kind of task(we won't read, right?).

Fredrik, what do you mean by "a complex transformation"?

So, I've managed to get antialiased edges of transformed windows. 

https://github.com/zzag/kwin/commits/opengl-multisampling (PoC)

Short summary of changes:
* effects set PAINT_SCREEN_MULTISAMPLE if they need multisampling. Please notice that
  each effect has an option "Use multisampling" so it's possible to turn off multisampling
  for each of them. Currently, only the following effects use multisampling:
   - Cover switch
   - Cube
   - Flip switch
   - Magic lamp
* GLRenderTarget is totally wrecked so it is possible to attach textures or render buffers.
* libkwineffects has a new data structure: GLRenderbuffer
* Scene has two new virtual methods: finalPrePaintScreen and finalPostPaintScreen
* Default number of samples is 8

I think this bug should have normal status because:
* there are no any issues with fonts whatsoever
* 3D effects were for so long and should have proper antialising
* and it's possible to fix the problem

Created attachment 111744 [details]
Flip switch + multisampling

Created attachment 111745 [details]
Cover switch + multisampling

Created attachment 111746 [details]
Cube + multisampling

Created attachment 111747 [details]
A personal effect + multisampling

(In reply to Vlad Zagorodniy from comment #16)
> Fredrik, what do you mean by "a complex transformation"?

Any transformation that does not just translate and/or scale the window. Although scaling can also benefit from this, so I guess any transformation that does not just translate the window.

Some comments on the branch:

There is no need to call unbind() before deleting an object - deleting an object also unbinds it from any bind points it is bound to in the current context.

[libkwineffects] simplify GLRenderTarget:

Please split this commit so each change is done as a separate commit. It makes the changes easier to review.

I think finalPrePaintScreen() should create the render buffer and render target on-demand, and also destroy them when PAINT_SCREEN_MULTISAMPLE is not set. They consume a lot of memory, so we don't want to keep them around when they are not being used.

I would rename m_multisamplingCtx to m_multisampling. Ctx suggests that it's a GL context.

The changes look pretty good otherwise!

(In reply to Fredrik Höglund from comment #22)
> Any transformation that does not just translate and/or scale the window.
> Although scaling can also benefit from this, so I guess any transformation
> that does not just translate the window.

I think it makes sense to render a window in an offscreen texture if only affine
transformations were applied on it. Like, there is no point to render a window 
in an offscreen texture if the Fall apart effect was applied on it.

Or, there should be another flag, like PAINT_WINDOW_SMOOTH. So, when opengl scene
sees that flag, it renders a window into an offscreen texture, then maps the
offscreen texture onto the window's grid.

Or, there should be some heuristic so opengl scene knows when to render a window into 
an offscreen texture.

But, it's mostly unrelated to the multisampling stuff, right?

> There is no need to call unbind() before deleting an object - deleting an
> object also unbinds it from any bind points it is bound to in the current
> context.

Done.

> [libkwineffects] simplify GLRenderTarget:
>
> Please split this commit so each change is done as a separate commit. It
> makes the changes easier to review.

Done. I've splitted it into ac394bdb60201b49636e31dfabf591420578d79b and
0161f0ec73eedc0c5047107fd579defb7afc2c21. Please note that I deleted
the part which marks textures as dirty. It's not used anywhere and makes
only harder to do changes. Do you know what purpose was of
GLTexture::setDirty(), and GLTexture::isDirty()?
 
> I think finalPrePaintScreen() should create the render buffer and render
> target on-demand, and also destroy them when PAINT_SCREEN_MULTISAMPLE is not
> set. They consume a lot of memory, so we don't want to keep them around when
> they are not being used.

In other words, keep a multisampled render buffer till finalPrePaintScreen is 
being called with the PAINT_SCREEN_MULTISAMPLE flag, right?

> I would rename m_multisamplingCtx to m_multisampling. Ctx suggests that it's
> a GL context.

Done.

> The changes look pretty good otherwise!

Thanks, but I posted that code as a proof-of-concept to stimulate work on fixing
this bug.

I could continue work on fixing this bug, but only under supervision(I'm not so good
at OpenGL).

Also, it has issues with the Background contrast and blur effect. https://www.youtube.com/watch?v=aniol9Hcr2U
IIRC, they copy data from a framebuffer. Is it a good idea to use render buffers in this case?

I think we could blit from a current framebuffer(the one with multisampling) to a texture and
then do blur or change contrast. This would require adding another method, like blitTo,
to the GLRenderTarget class.

I fixed the issue with background contrast and blur effect, a bug in GLRenderTarget::blitFromFramebuffer, and did some semantic changes to GLRenderTarget::blit* methods.

I pushed my multisampling work to zzag/scene-opengl-multisampling branch.

(In reply to Fredrik Höglund from comment #22)
> I think finalPrePaintScreen() should create the render buffer and render
> target on-demand, and also destroy them when PAINT_SCREEN_MULTISAMPLE is not
> set. They consume a lot of memory, so we don't want to keep them around when
> they are not being used.

Done.

https://github.com/zzag/kwin/commit/12cc19c69059f2db62ce5e077469ef31f7576099

Nice work, Vlad. If you think it is ready for review, please post the patch to https://phabricator.kde.org/differential/diff/create/

(In reply to Christoph Feck from comment #27)
> Nice work, Vlad. If you think it is ready for review, please post the patch
> to https://phabricator.kde.org/differential/diff/create/

There are several commits which break compatibility with previous versions of libkwineffects. So, given that, I'll wait for the next major version(6, I guess).

Well, using render buffers was a bad choice. Apparently, glCopySubTexImaga2D
is used many times throughout KWin..

I've been trying to change GLTexture and GLRenderTarget so it's possible to
render to a multisampled texture but that's like walking through a minefield,
every small required change breaks compatibility. I hope that libkwineffects
will be redesigned/rewritten in the future.

I'm done with multisampling patches.

How to ensure this is looked at for KDE 6.X?