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
