effects: attempt to screenshot on OpenGL ES 2.0 instead of failing
ClosedPublic

Authored by bshah on Nov 10 2018, 12:37 PM.

Details

Summary

Current code path was attempting to use both framebuffer blit and
glReadPixels on OpenGL ES, instead change the code to use framebuffer
blit and glGetTexImage on OpenGL and glReadPixels on the OpenGLES as it
doesn't have glGetTexImage available.

Test Plan

tested on Nexus 5X.

Diff Detail

Repository
R108 KWin
Branch
bshah/screenshot
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4796
Build 4814: arc lint + arc unit
bshah created this revision.Nov 10 2018, 12:37 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 10 2018, 12:37 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
bshah requested review of this revision.Nov 10 2018, 12:37 PM
graesslin requested changes to this revision.Nov 10 2018, 7:39 PM
graesslin added a subscriber: graesslin.

Performing this on other systems could in worst case result in a crash - OpenGL drivers are extremely buggy. If blit is not supported, we cannot just do it. Please adjust GLRenderTarget::blitSupported in a way that it returns true for your blob. E.g. use GLPlatform to add a workaround for it.

This revision now requires changes to proceed.Nov 10 2018, 7:39 PM
bshah added a comment.Nov 11 2018, 7:17 AM

This is super weird tbh, if I add workaround for Adreno, in blitSupported to make it return true. It crashes when doing screenshot, with following

"No provider of glBlitFramebuffer found.  Requires one of:\n"
"    Desktop OpenGL 3.0\n"                                   
"    GL extension \"GL_ARB_framebuffer_object\"\n"           
"    OpenGL ES 3.0\n"                                        
"    GL extension \"GL_EXT_framebuffer_blit\"\n"             
"    GL extension \"GL_NV_framebuffer_blit\"\n"

But if I just ignore blitSupported in the screenshot effect it doesn't crash.. it is one "WAT" moment.

bshah added a comment.Nov 11 2018, 7:39 AM

Ah, now that I read code of GLRenderTarget::blitFromFramebuffer this makes sense,

This code path is executed only when blitSupported is true. Which makes it crash. But on adreno devices somehow I get working screenshot even without framebufferBlit by just doing glReadPixels.

Ah, now that I read code of GLRenderTarget::blitFromFramebuffer this makes sense,

This code path is executed only when blitSupported is true. Which makes it crash.

The problem is that calls into OpenGL extensions can crash if it's not available. The proper behavior is to return a null pointer if an extension is missing. So the change you did here would crash on any driver not supporting blit and properly returning null for the blit functionality.

But on adreno devices somehow I get working screenshot even without framebufferBlit by just doing glReadPixels.

glReadPixels is slow. That's not a general solution and I would recommend against using it. It's better to not do a screenshot than screenshot with glReadPixels.

bshah added a comment.Nov 11 2018, 8:02 AM

glReadPixels is slow. That's not a general solution and I would recommend against using it. It's better to not do a screenshot than screenshot with glReadPixels.

Well, we are dong that already.. this code is confusing.. see Line 561+.

bshah added a comment.Nov 11 2018, 8:06 AM

The problem is that calls into OpenGL extensions can crash if it's not available. The proper behavior is to return a null pointer if an extension is missing. So the change you did here would crash on any driver not supporting blit and properly returning null for the blit functionality.

No, the call won't ever reach OpenGL extnesion,

1273 void GLRenderTarget::blitFromFramebuffer(const QRect &source, const QRect &destination, GLenum filter)                                                                  
1274 {
1275     if (!GLRenderTarget::blitSupported()) {
1276         return;
1277     }
1278

This is current code in kwinglutils.cpp, so target.blitFromFramebuffer call is just no-op if blit is not supported.

glReadPixels is slow. That's not a general solution and I would recommend against using it. It's better to not do a screenshot than screenshot with glReadPixels.

Well, we are dong that already.. this code is confusing.. see Line 561+.

ah the complete code block looks wrong. There is no glGetTexImage support in OpenGL ES, so that's why glReadPixels is used. On the other hand the complete blitting doesn't make sense if we use glReadPixels. So the code needs to be reworked to if (blit && !gles) else glReadPixels

bshah updated this revision to Diff 45278.Nov 11 2018, 11:56 AM
bshah edited the summary of this revision. (Show Details)
bshah removed a subscriber: graesslin.

use glReadPixels when blit is not supported

graesslin added inline comments.Nov 11 2018, 12:42 PM
effects/screenshot/screenshot.cpp
569

just use else here. glReadPixels is available on desktop gl.

bshah updated this revision to Diff 45279.Nov 11 2018, 1:11 PM

simplify

bshah marked an inline comment as done.Nov 11 2018, 1:11 PM
graesslin accepted this revision.Nov 11 2018, 1:20 PM
This revision is now accepted and ready to land.Nov 11 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.