fredrik added a comment.
Restricted Application edited projects, added Plasma; removed KWin.
Don't forget to add your name to the license headers.
INLINE COMMENTS
> blur.cpp:46
> m_simpleShader =
> ShaderManager::instance()->generateShaderFromResources(ShaderTrait::MapTexture,
> QString(), QStringLiteral("logout-blur.frag"));
> + m_simpleTarget = new GLRenderTarget(GLTexture(GL_RGBA8, 1, 1));
> +
You should add a GLRenderTarget constructor that takes no arguments.
This is working around deficiencies in the API.
> blur.cpp:116
> + return !target->valid();
> + }) == m_renderTargets.cend();
> +}
I suggest doing this check after creating the render targets, and caching the
result.
> blur.cpp:135
> + deleteFBOs();
> +
> + for (int i = 0; i <= m_downSampleIterations; i++) {
Call reserve() on m_renderTextures and m_renderTargets here.
> blur.cpp:137
> + for (int i = 0; i <= m_downSampleIterations; i++) {
> + m_renderTextures.append(GLTexture(GL_RGBA8,
> effects->virtualScreenSize() / qPow(2, i)));
> + m_renderTextures.last().setFilter(GL_LINEAR);
1 << i
> blur.cpp:145
> + // This last set is used as a temporary helper texture
> + m_renderTextures.append(GLTexture(GL_RGBA8,
> effects->virtualScreenSize()));
> + m_renderTextures.last().setFilter(GL_LINEAR);
Why is this needed?
I'm probably missing something here, but it looks to me as if the effect copies
the contents of the framebuffer to the helper texture, then copies the contents
of that texture to m_renderTextures[0], after which the contents of helper
texture is not used again. Can't copyScreenSampleTexture() copy directly from
the framebuffer to m_renderTextures[0]?
> blur.cpp:694
> + m_simpleTarget->attachTexture(blurTexture);
> + m_simpleTarget->blitFromFramebuffer(w->geometry(), QRect(QPoint(0, 0),
> w->size()));
>
Detach the texture from the render target before you return from this method -
otherwise the FBO continues to hold a reference to the texture, preventing it
from being deleted.
> blur.cpp:123
> +{
> + for (int i = 0; i < m_renderTargets.size(); i++) {
> + delete m_renderTargets[i];
You can use qDeleteAll() here.
> blur.cpp:127
> +
> + m_renderTextures[i].discard();
> + }
There is no need to call discard on the textures - just clear the vector.
> blur.cpp:374
> + for (int i = 0; i <= downSampleIterations; i++) {
> + const int divisionRatio = qPow(2, i);
> +
1 << i
> blur.cpp:717
> + vbo->draw(GL_TRIANGLES, blurRectCount * i, blurRectCount);
> + GLRenderTarget::popRenderTarget();
> + }
These three lines of code will expand to an unfortunate sequence of GL calls:
// Iteration N
glGetIntegerv(GL_VIEWPORT, ...);
glBindFramebuffer(GL_FRAMEBUFFER, ...);
glViewport(...);
glDrawArrays(...);
glBindFramebuffer(GL_FRAMEBUFFER, 0);
glViewport(...);
// Iteration N+1
glGetIntegerv(GL_VIEWPORT, ..);
glBindFramebuffer(GL_FRAMEBUFFER, ...);
glViewport(...);
glDrawArrays(...);
glBindFramebuffer(GL_FRAMEBUFFER, 0);
glViewport(...);
Note the redundant calls to glViewport() and glBindFramebuffer(). The worst
offender here is glGetIntegerv() however, because it forces serialization of
the internal driver threads.
Ideally the call sequence should look like this:
// Iteration N
glBindFramebuffer(GL_FRAMEBUFFER, ...);
glViewport(...);
glDrawArrays(...);
// Iteration N+1
glBindFramebuffer(GL_FRAMEBUFFER, ...);
glViewport(...);
glDrawArrays(...);
This can be fixed in a followup patch though.
> blur.h:41
>
> +static const int borderSize = 5;
> +
This number could use an explanation.
> blurshader.cpp:241
> + "uniform float offset;\n"
> + "uniform vec2 textureSize;\n";
> +
textureSize is also the name of a built-in function in GLSL, so I suggest
changing the name to targetSize, renderTargetSize, framebufferSize or something
similar. That also makes it clear that it is not the size of the texture being
sampled.
> blurshader.h:95
> + GLShader *m_shaderUpsample = nullptr;
> + GLShader *m_shaderCopysample = nullptr;
> +
I think you could simplify the code quite a bit by having an array of
struct {
GLShader *shader;
int mvpMatrixLocation;
...
};
and use m_activeSampleType as an index into that array.
> blurshader.h:47
> + virtual void setOffset(float offset) = 0;
> + virtual void setTextureSize(QSize textureSize) = 0;
> + virtual void setBlurRect(QRect blurRect, QSize screenSize) = 0;
I suggest changing the name to setTargetSize() to make it clear that the size
is the size of the render target, and not the size of the texture being sampled.
> blurshader.h:50
>
> - virtual void bind() = 0;
> + virtual void bind(int sampleType) = 0;
> virtual void unbind() = 0;
sampleTypeEnum sampleType
> blurshader.h:53
> +
> + enum sampleTypeEnum {
> + downSampleType,
The first letter in enums should be capitalized. I think SampleType would also
be a better name.
> blurshader.h:84
> void setModelViewProjectionMatrix(const QMatrix4x4 &matrix);
> + void setOffset(float offset);
> + void setTextureSize(QSize textureSize);
override final
This goes for all reimplemented virtual functions.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D9848
To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson,
fredrik, ngraham, plasma-devel, kwin, #kwin, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol
