[libkwineffects] Set original backend framebuffer for render targets
AcceptedPublic

Authored by romangg on Dec 11 2019, 7:09 PM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Summary

KWin only renders into the default framebuffer, which is for example an EGL
surface.

To prepare a post-processing step with a different framebuffer allow the
framebuffer to be changable. For that KWin's current framebuffer must be
communicated to the GLRenderTarget class, which otherwise does not set it back
to KWin's current one when a render target is disabled again.

Test Plan

Compiles, with other patches for Gl based screen rotation

Diff Detail

Repository
R108 KWin
Branch
framebuffer-dynamic
Lint
Lint ErrorsExcuse: unrelated
SeverityLocationCodeMessage
Errorlibkwineffects/kwinglobals.h:150CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 20765
Build 20783: arc lint + arc unit
romangg created this revision.Dec 11 2019, 7:09 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 11 2019, 7:09 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Dec 11 2019, 7:09 PM
romangg retitled this revision from [libkwineffects] Allow changing of KWin's framebuffer to [libkwineffects] Set original backend framebuffer for render targets.Dec 11 2019, 7:16 PM
zzag added a subscriber: zzag.EditedDec 11 2019, 8:09 PM

If we need to render something into a texture, a GLRenderTarget must be pushed on a stack, e.g.

GLRenderTarget::push(target);

// Draw

GLRenderTarget::pop();

But it's worth to mention that a couple of methods don't play along very nice with this design, namely GLRenderTarget::blitFramebuffer(). It just binds the default framebuffer object to GL_READ_FRAMEBUFFER, it doesn't care whether there is a GLRenderTarget on the stack, etc. I recommend to fix these methods instead. I don't think that we need to change the value of the default framebuffer object.

In D25904#575817, @zzag wrote:

If we need to render something into a texture, a GLRenderTarget must be pushed on a stack, e.g.

GLRenderTarget::push(target);

// Draw

GLRenderTarget::pop();

But it's worth to mention that a couple of methods don't play along very nice with this design, namely GLRenderTarget::blitFramebuffer(). It just binds the default framebuffer object to GL_READ_FRAMEBUFFER, it doesn't care whether there is a GLRenderTarget on the stack, etc. I recommend to fix these methods instead. I don't think that we need to change the value of the default framebuffer object.

The use case is a bit different here. The change is meant for allowing internal post-processing steps to be applied as in D25907 after the Scene rendering is done. But GlRenderTarget is meant for being used in the main Scene rendering.

For example it assumes the viewport to be of the same size as the texture it renders to and at position (0, 0). But for our final rendering we need the viewport to be of different nature because of scaling and since we "cut out" a piece of the overall screen geometry.

I'm not saying it is impossible to reuse GlRenderTarget for post-processing after Scene rendering has concluded but it was not designed for that and adapting it might become a hassle. But if in the future we find more use cases for post-processing besides rotation in aforementioned patch (I think of Night Color in case hardware gamma manipulation is not possible or not desirable) it might make sense to extend the API around it.

zzag added a comment.Dec 11 2019, 9:37 PM

Hmm, okay, fair enough. Can you rename setKWinFramebuffer to setDefaultFramebufferObject? It's not clear what "kwin framebuffer" is.

romangg added a comment.EditedDec 11 2019, 9:48 PM
In D25904#575886, @zzag wrote:

Hmm, okay, fair enough. Can you rename setKWinFramebuffer to setDefaultFramebufferObject? It's not clear what "kwin framebuffer" is.

I used the name s_defaultFramebuffer all the time in my working branch but changed it now for upload. ;)

The reason is that the Default Framebuffer is in OpenGl a fixed notion that names the framebuffer with id 0, see [1].

Since we have this here to use it also with framebuffer objects the variable would need some other name. But I'm open to other suggestions. I thougth of originalFramebuffer or backendFramebuffer.

[1] https://www.khronos.org/opengl/wiki/Default_Framebuffer

zzag added a comment.Dec 11 2019, 9:51 PM

backendFramebuffer is a good one.

fredrik added inline comments.
libkwineffects/kwinglutils.cpp
1091

I'd also like to see a matching setDefaultViewport(), because calls to glGet*() forces serialization of the internal driver threads.

libkwineffects/kwinglutils.h
552

Could we name this setDefaultFramebuffer()?

I think the documentation is also a bit misleading, because it is not necessarily the framebuffer object currently being rendered to. It's the framebuffer at the very bottom of the stack.

romangg added inline comments.Dec 11 2019, 10:44 PM
libkwineffects/kwinglutils.cpp
1091

Can you explain some more? So whenever we change the current viewport call "setDefaultViewport"?

libkwineffects/kwinglutils.h
552

I find naming this API something like defaultFramebuffer difficult (as outlined in D25904#575910): because the default framebuffer is not changed when the function is called. The default framebuffer is what this can be changed to but it is not always the default framebuffer. Would setBackendFramebuffer be alright with you? Or setBottomFramebuffer?

I agree the documentation is misleading. I will change it up a bit.

fredrik added inline comments.Dec 12 2019, 12:26 AM
libkwineffects/kwinglutils.cpp
1091

No, the idea is that whoever calls setKWinFramebuffer() would also call setDefaultViewport() to define the viewport that should be used when that framebuffer is bound.

Right now pushRenderTarget() queries GL_VIEWPORT so popRenderTarget() can restore it when the last custom render target is popped. But if we already know what the viewport should be, we don't have to query it from the GL.

libkwineffects/kwinglutils.h
552

For some reason phabricator didn't warn me that the revision had been updated, so I didn't see that Vlad had already suggested that.

Between those two I think I would prefer setBottomFramebuffer(). But how about setSurfaceFramebuffer()?

romangg updated this revision to Diff 73007.Tue, Jan 7, 5:38 PM
  • Rename framebuffer setter
romangg added inline comments.Tue, Jan 7, 5:40 PM
libkwineffects/kwinglutils.cpp
1091

So this is a performance optimization but without this it shouldn't break, correct? Let's put it then in a separate patch.

davidedmundson accepted this revision.Tue, Jan 14, 5:48 PM
This revision is now accepted and ready to land.Tue, Jan 14, 5:48 PM