[Differential] [Commented On] D4366: WIP: Add screen recorder interface

[Fredrik Höglund]

fredrik added a comment.


  I know this revision has been abandoned, but I wanted to comment on a few things for future reference.

INLINE COMMENTS

> screencast.cpp:105
> +
> +    QRect geometry = effects->virtualScreenGeometry();
> +

I strongly suggest that you use the damage information from kwin and only download the parts of the framebuffer that have actually changed. This can make a massive difference in performance.

You can keep a screen sized QImage around and keep it in sync by updating areas as they change.

> screencast.cpp:107
> +
> +    GLTexture tex(GL_RGBA8, geometry.width(), geometry.height());
> +    GLRenderTarget target(tex);

Why do you use an intermediate texture instead of reading directly from the framebuffer in the desktop GL case?

You create the texture and blit the framebuffer into it in the GLES case, even though you don't use it.

> screencast.cpp:109
> +    GLRenderTarget target(tex);
> +    target.blitFromFramebuffer(geometry);
> +

There is no need to use a RenderTarget here. Use glCopyTexSubImage2D() instead, which copies from the framebuffer to the currently bound texture.

> screencast.cpp:114
> +
> +    auto img = QImage(geometry.size(), QImage::Format_RGB888);
> +    if (GLPlatform::instance()->isGLES()) {

Don't read back data directly to client memory. Instead create a GL_PIXEL_PACK_BUFFER and download the image into it. That way glReadPixels()/glGetTexImage() schedules the copy, but doesn't block and wait for it to finish. Wait at least one frame before you access the contents of the buffer.

> screencast.cpp:116
> +    if (GLPlatform::instance()->isGLES()) {
> +        glReadPixels(0, 0, img.width(), img.height(), GL_RGB, GL_UNSIGNED_BYTE, (GLvoid*)img.bits());
> +    } else {

Never read back data as GL_RGB. Three component formats are not supported in hardware, so this involves at least a partial software fallback.

The only format/type combination a GLES implementation is required to support is also GL_RGBA/GL_UNSIGNED_BYTE.

I also note that you immediately convert the image to a four-component format below.

> screencast.cpp:118
> +    } else {
> +        glGetTexImage(GL_TEXTURE_2D, 0, GL_RGB, GL_UNSIGNED_BYTE, (GLvoid*)img.bits());
> +    }

If you need the image to be in QImage::Format_ARGB32, you should read back the data as GL_BGRA/GL_UNSIGNED_INT_8_8_8_8_REV. This is the GL equivalent of QImage::Format_ARGB32.

> screencast.cpp:121
> +    img = img.convertToFormat(QImage::Format_ARGB32);
> +    img = img.mirrored();
> +    tex.unbind();

Use GL_MESA_pack_invert and GL_ANGLE_pack_reverse_row_order so the image is downloaded in the correct orientation.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D4366

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: fredrik, graesslin, subdiff, plasma-devel, kwin, #kwin, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170208/f52df806/attachment.html>