D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180116/070eab67/attachment-0001.html>